Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932071Ab3J1Oi1 (ORCPT ); Mon, 28 Oct 2013 10:38:27 -0400 Received: from mail-pa0-f45.google.com ([209.85.220.45]:60580 "EHLO mail-pa0-f45.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756404Ab3J1OiZ (ORCPT ); Mon, 28 Oct 2013 10:38:25 -0400 MIME-Version: 1.0 In-Reply-To: <20131016160334.61dc2a5c@tlielax.poochiereds.net> References: <1381941162-72831-1-git-send-email-timg@tpi.com> <20131016160334.61dc2a5c@tlielax.poochiereds.net> Date: Mon, 28 Oct 2013 09:38:24 -0500 Message-ID: Subject: Re: [PATCH linux-next V2] cifs: Make big endian multiplex ID sequences monotonic on the wire From: Steve French To: Jeff Layton Cc: Tim Gardner , "linux-cifs@vger.kernel.org" , samba-technical , LKML , Steve French Content-Type: text/plain; charset=ISO-8859-1 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 7418 Lines: 197 merged into cifs-2.6.git for-next On Wed, Oct 16, 2013 at 3:03 PM, Jeff Layton wrote: > On Wed, 16 Oct 2013 10:32:42 -0600 > Tim Gardner wrote: > >> The multiplex identifier (MID) in the SMB header is only >> ever used by the client, in conjunction with PID, to match responses >> from the server. As such, the endianess of the MID is not important. >> However, When tracing packet sequences on the wire, protocol analyzers >> such as wireshark display MID as little endian. It is much more informative >> for the on-the-wire MID sequences to match debug information emitted by the >> CIFS driver. Therefore, one should write and read MID in the SMB header >> assuming it is always little endian. >> >> Observed from wireshark during the protocol negotiation >> and session setup: >> >> Multiplex ID: 256 >> Multiplex ID: 256 >> Multiplex ID: 512 >> Multiplex ID: 512 >> Multiplex ID: 768 >> Multiplex ID: 768 >> >> After this patch on-the-wire MID values begin at 1 and increase monotonically. >> >> Introduce get_next_mid64() for the internal consumers that use the full 64 bit >> multiplex identifier. >> >> Introduce the helpers get_mid() and compare_mid() to make the endian >> translation clear. >> >> Cc: Jeff Layton >> Cc: Steve French >> Signed-off-by: Tim Gardner >> --- >> >> V2 - get an endian appropriate copy of 'mid' for debug output in checkSMB(). >> Actually use the new helper get_mid() wherever smb->Mid is referenced. >> >> fs/cifs/cifsglob.h | 25 ++++++++++++++++++++++++- >> fs/cifs/misc.c | 10 ++++++---- >> fs/cifs/smb1ops.c | 4 ++-- >> fs/cifs/smb2transport.c | 2 +- >> fs/cifs/transport.c | 2 +- >> 5 files changed, 34 insertions(+), 9 deletions(-) >> >> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h >> index 52b6f6c..535e324 100644 >> --- a/fs/cifs/cifsglob.h >> +++ b/fs/cifs/cifsglob.h >> @@ -620,11 +620,34 @@ set_credits(struct TCP_Server_Info *server, const int val) >> } >> >> static inline __u64 >> -get_next_mid(struct TCP_Server_Info *server) >> +get_next_mid64(struct TCP_Server_Info *server) >> { >> return server->ops->get_next_mid(server); >> } >> >> +static inline __u16 >> +get_next_mid(struct TCP_Server_Info *server) >> +{ >> + __u16 mid = get_next_mid64(server); >> + /* >> + * The value in the SMB header should be little endian for easy >> + * on-the-wire decoding. >> + */ >> + return cpu_to_le16(mid); >> +} >> + >> +static inline __u16 >> +get_mid(const struct smb_hdr *smb) >> +{ >> + return le16_to_cpu(smb->Mid); >> +} >> + >> +static inline bool >> +compare_mid(__u16 mid, const struct smb_hdr *smb) >> +{ >> + return mid == le16_to_cpu(smb->Mid); >> +} >> + >> /* >> * When the server supports very large reads and writes via POSIX extensions, >> * we can allow up to 2^24-1, minus the size of a READ/WRITE_AND_X header, not >> diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c >> index 298e31e..2f9f379 100644 >> --- a/fs/cifs/misc.c >> +++ b/fs/cifs/misc.c >> @@ -295,7 +295,8 @@ check_smb_hdr(struct smb_hdr *smb) >> if (smb->Command == SMB_COM_LOCKING_ANDX) >> return 0; >> >> - cifs_dbg(VFS, "Server sent request, not response. mid=%u\n", smb->Mid); >> + cifs_dbg(VFS, "Server sent request, not response. mid=%u\n", >> + get_mid(smb)); >> return 1; >> } >> >> @@ -351,6 +352,7 @@ checkSMB(char *buf, unsigned int total_read) >> } >> >> if (4 + rfclen != clc_len) { >> + __u16 mid = get_mid(smb); >> /* check if bcc wrapped around for large read responses */ >> if ((rfclen > 64 * 1024) && (rfclen > clc_len)) { >> /* check if lengths match mod 64K */ >> @@ -358,11 +360,11 @@ checkSMB(char *buf, unsigned int total_read) >> return 0; /* bcc wrapped */ >> } >> cifs_dbg(FYI, "Calculated size %u vs length %u mismatch for mid=%u\n", >> - clc_len, 4 + rfclen, smb->Mid); >> + clc_len, 4 + rfclen, mid); >> >> if (4 + rfclen < clc_len) { >> cifs_dbg(VFS, "RFC1001 size %u smaller than SMB for mid=%u\n", >> - rfclen, smb->Mid); >> + rfclen, mid); >> return -EIO; >> } else if (rfclen > clc_len + 512) { >> /* >> @@ -375,7 +377,7 @@ checkSMB(char *buf, unsigned int total_read) >> * data to 512 bytes. >> */ >> cifs_dbg(VFS, "RFC1001 size %u more than 512 bytes larger than SMB for mid=%u\n", >> - rfclen, smb->Mid); >> + rfclen, mid); >> return -EIO; >> } >> } >> diff --git a/fs/cifs/smb1ops.c b/fs/cifs/smb1ops.c >> index 8233b17..09ef8f3 100644 >> --- a/fs/cifs/smb1ops.c >> +++ b/fs/cifs/smb1ops.c >> @@ -67,7 +67,7 @@ send_nt_cancel(struct TCP_Server_Info *server, void *buf, >> mutex_unlock(&server->srv_mutex); >> >> cifs_dbg(FYI, "issued NT_CANCEL for mid %u, rc = %d\n", >> - in_buf->Mid, rc); >> + get_mid(in_buf), rc); >> >> return rc; >> } >> @@ -101,7 +101,7 @@ cifs_find_mid(struct TCP_Server_Info *server, char *buffer) >> >> spin_lock(&GlobalMid_Lock); >> list_for_each_entry(mid, &server->pending_mid_q, qhead) { >> - if (mid->mid == buf->Mid && >> + if (compare_mid(mid->mid, buf) && >> mid->mid_state == MID_REQUEST_SUBMITTED && >> le16_to_cpu(mid->command) == buf->Command) { >> spin_unlock(&GlobalMid_Lock); >> diff --git a/fs/cifs/smb2transport.c b/fs/cifs/smb2transport.c >> index 340abca..c523617 100644 >> --- a/fs/cifs/smb2transport.c >> +++ b/fs/cifs/smb2transport.c >> @@ -466,7 +466,7 @@ smb2_verify_signature(struct smb_rqst *rqst, struct TCP_Server_Info *server) >> static inline void >> smb2_seq_num_into_buf(struct TCP_Server_Info *server, struct smb2_hdr *hdr) >> { >> - hdr->MessageId = get_next_mid(server); >> + hdr->MessageId = get_next_mid64(server); >> } >> >> static struct mid_q_entry * >> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c >> index 6fdcb1b..057b2c0 100644 >> --- a/fs/cifs/transport.c >> +++ b/fs/cifs/transport.c >> @@ -58,7 +58,7 @@ AllocMidQEntry(const struct smb_hdr *smb_buffer, struct TCP_Server_Info *server) >> return temp; >> else { >> memset(temp, 0, sizeof(struct mid_q_entry)); >> - temp->mid = smb_buffer->Mid; /* always LE */ >> + temp->mid = get_mid(smb_buffer); >> temp->pid = current->pid; >> temp->command = cpu_to_le16(smb_buffer->Command); >> cifs_dbg(FYI, "For smb_command %d\n", smb_buffer->Command); > > Looks good... > > Reviewed-by: Jeff Layton -- Thanks, Steve -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/