* [PATCH 1/2 linux-next] cifs: Remove redundant multiplex identifier check from check_smb_hdr() @ 2013-10-16 15:09 Tim Gardner 2013-10-16 15:09 ` [PATCH 2/2 linux-next] cifs: Make big endian multiplex ID sequences monotonic on the wire Tim Gardner 2013-10-16 15:36 ` [PATCH 1/2 linux-next] cifs: Remove redundant multiplex identifier check from check_smb_hdr() Jeff Layton 0 siblings, 2 replies; 8+ messages in thread From: Tim Gardner @ 2013-10-16 15:09 UTC (permalink / raw) To: linux-cifs, samba-technical, linux-kernel; +Cc: Tim Gardner, Steve French The only call site for check_smb_header() assigns 'mid' from the SMB packet, which is then checked again in check_smb_header(). This seems like redundant redundancy. Cc: Steve French <sfrench@samba.org> Signed-off-by: Tim Gardner <timg@tpi.com> --- fs/cifs/misc.c | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c index 138a011..298e31e 100644 --- a/fs/cifs/misc.c +++ b/fs/cifs/misc.c @@ -278,7 +278,7 @@ header_assemble(struct smb_hdr *buffer, char smb_command /* command */ , } static int -check_smb_hdr(struct smb_hdr *smb, __u16 mid) +check_smb_hdr(struct smb_hdr *smb) { /* does it have the right SMB "signature" ? */ if (*(__le32 *) smb->Protocol != cpu_to_le32(0x424d53ff)) { @@ -287,13 +287,6 @@ check_smb_hdr(struct smb_hdr *smb, __u16 mid) return 1; } - /* Make sure that message ids match */ - if (mid != smb->Mid) { - cifs_dbg(VFS, "Mids do not match. received=%u expected=%u\n", - smb->Mid, mid); - return 1; - } - /* if it's a response then accept */ if (smb->Flags & SMBFLG_RESPONSE) return 0; @@ -310,7 +303,6 @@ int checkSMB(char *buf, unsigned int total_read) { struct smb_hdr *smb = (struct smb_hdr *)buf; - __u16 mid = smb->Mid; __u32 rfclen = be32_to_cpu(smb->smb_buf_length); __u32 clc_len; /* calculated length */ cifs_dbg(FYI, "checkSMB Length: 0x%x, smb_buf_length: 0x%x\n", @@ -348,7 +340,7 @@ checkSMB(char *buf, unsigned int total_read) } /* otherwise, there is enough to get to the BCC */ - if (check_smb_hdr(smb, mid)) + if (check_smb_hdr(smb)) return -EIO; clc_len = smbCalcSize(smb); -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/2 linux-next] cifs: Make big endian multiplex ID sequences monotonic on the wire 2013-10-16 15:09 [PATCH 1/2 linux-next] cifs: Remove redundant multiplex identifier check from check_smb_hdr() Tim Gardner @ 2013-10-16 15:09 ` Tim Gardner 2013-10-16 15:40 ` Jeff Layton 2013-10-16 16:25 ` Shirish Pargaonkar 2013-10-16 15:36 ` [PATCH 1/2 linux-next] cifs: Remove redundant multiplex identifier check from check_smb_hdr() Jeff Layton 1 sibling, 2 replies; 8+ messages in thread From: Tim Gardner @ 2013-10-16 15:09 UTC (permalink / raw) To: linux-cifs, samba-technical, linux-kernel; +Cc: Tim Gardner, Steve French 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: Steve French <sfrench@samba.org> Signed-off-by: Tim Gardner <timg@tpi.com> --- I'm looking at some of this code in excrutiating detail because I'm having trouble with a backport of CIFS from 3.9.7 on an embedded 2.6.31 powerpc. Its failing the RawNTLMSSP authentication and is almost certainly an endian issue. x86 on the same code base works quite well. Am I making a rash assumption that CIFS in 3.9 stable worked on big endian ? fs/cifs/cifsglob.h | 25 ++++++++++++++++++++++++- fs/cifs/misc.c | 9 +++++---- fs/cifs/smb1ops.c | 4 ++-- fs/cifs/smb2transport.c | 2 +- fs/cifs/transport.c | 2 +- 5 files changed, 33 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..c851d41 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", + le16_to_cpu(smb->Mid)); return 1; } @@ -358,11 +359,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, le16_to_cpu(smb->Mid)); if (4 + rfclen < clc_len) { cifs_dbg(VFS, "RFC1001 size %u smaller than SMB for mid=%u\n", - rfclen, smb->Mid); + rfclen, le16_to_cpu(smb->Mid)); return -EIO; } else if (rfclen > clc_len + 512) { /* @@ -375,7 +376,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, le16_to_cpu(smb->Mid)); return -EIO; } } diff --git a/fs/cifs/smb1ops.c b/fs/cifs/smb1ops.c index 8233b17..5598af9 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); + le16_to_cpu(in_buf->Mid), 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); -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2 linux-next] cifs: Make big endian multiplex ID sequences monotonic on the wire 2013-10-16 15:09 ` [PATCH 2/2 linux-next] cifs: Make big endian multiplex ID sequences monotonic on the wire Tim Gardner @ 2013-10-16 15:40 ` Jeff Layton 2013-10-16 16:02 ` Tim Gardner 2013-10-16 16:25 ` Shirish Pargaonkar 1 sibling, 1 reply; 8+ messages in thread From: Jeff Layton @ 2013-10-16 15:40 UTC (permalink / raw) To: Tim Gardner; +Cc: linux-cifs, samba-technical, linux-kernel, Steve French On Wed, 16 Oct 2013 09:09:50 -0600 Tim Gardner <timg@tpi.com> 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: Steve French <sfrench@samba.org> > Signed-off-by: Tim Gardner <timg@tpi.com> > --- > > I'm looking at some of this code in excrutiating detail because I'm having trouble > with a backport of CIFS from 3.9.7 on an embedded 2.6.31 powerpc. Its failing the RawNTLMSSP > authentication and is almost certainly an endian issue. x86 on the same code base works > quite well. Am I making a rash assumption that CIFS in 3.9 stable worked on big endian ? > > fs/cifs/cifsglob.h | 25 ++++++++++++++++++++++++- > fs/cifs/misc.c | 9 +++++---- > fs/cifs/smb1ops.c | 4 ++-- > fs/cifs/smb2transport.c | 2 +- > fs/cifs/transport.c | 2 +- > 5 files changed, 33 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..c851d41 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", > + le16_to_cpu(smb->Mid)); > return 1; > } > > @@ -358,11 +359,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, le16_to_cpu(smb->Mid)); > > if (4 + rfclen < clc_len) { > cifs_dbg(VFS, "RFC1001 size %u smaller than SMB for mid=%u\n", > - rfclen, smb->Mid); > + rfclen, le16_to_cpu(smb->Mid)); > return -EIO; > } else if (rfclen > clc_len + 512) { > /* > @@ -375,7 +376,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, le16_to_cpu(smb->Mid)); > return -EIO; > } > } It would be more efficient to byte-swap smb->Mid only once and store that in a u16 here and then print that out instead of doing it multiple times. > diff --git a/fs/cifs/smb1ops.c b/fs/cifs/smb1ops.c > index 8233b17..5598af9 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); > + le16_to_cpu(in_buf->Mid), 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); Otherwise, this seems like a good thing to do... -- Jeff Layton <jlayton@redhat.com> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2 linux-next] cifs: Make big endian multiplex ID sequences monotonic on the wire 2013-10-16 15:40 ` Jeff Layton @ 2013-10-16 16:02 ` Tim Gardner 0 siblings, 0 replies; 8+ messages in thread From: Tim Gardner @ 2013-10-16 16:02 UTC (permalink / raw) To: Jeff Layton; +Cc: linux-cifs, samba-technical, linux-kernel, Steve French On 10/16/2013 08:40 AM, Jeff Layton wrote: > On Wed, 16 Oct 2013 09:09:50 -0600 > Tim Gardner <timg@tpi.com> 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: Steve French <sfrench@samba.org> >> Signed-off-by: Tim Gardner <timg@tpi.com> >> --- >> >> I'm looking at some of this code in excrutiating detail because I'm having trouble >> with a backport of CIFS from 3.9.7 on an embedded 2.6.31 powerpc. Its failing the RawNTLMSSP >> authentication and is almost certainly an endian issue. x86 on the same code base works >> quite well. Am I making a rash assumption that CIFS in 3.9 stable worked on big endian ? >> >> fs/cifs/cifsglob.h | 25 ++++++++++++++++++++++++- >> fs/cifs/misc.c | 9 +++++---- >> fs/cifs/smb1ops.c | 4 ++-- >> fs/cifs/smb2transport.c | 2 +- >> fs/cifs/transport.c | 2 +- >> 5 files changed, 33 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..c851d41 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", >> + le16_to_cpu(smb->Mid)); >> return 1; >> } >> >> @@ -358,11 +359,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, le16_to_cpu(smb->Mid)); >> >> if (4 + rfclen < clc_len) { >> cifs_dbg(VFS, "RFC1001 size %u smaller than SMB for mid=%u\n", >> - rfclen, smb->Mid); >> + rfclen, le16_to_cpu(smb->Mid)); >> return -EIO; >> } else if (rfclen > clc_len + 512) { >> /* >> @@ -375,7 +376,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, le16_to_cpu(smb->Mid)); >> return -EIO; >> } >> } > > It would be more efficient to byte-swap smb->Mid only once and store > that in a u16 here and then print that out instead of doing it multiple > times. > OK, I'll send out V2. -- Tim Gardner timg@tpi.com www.tpi.com OR 503-601-0234 x102 MT 406-443-5357 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2 linux-next] cifs: Make big endian multiplex ID sequences monotonic on the wire 2013-10-16 15:09 ` [PATCH 2/2 linux-next] cifs: Make big endian multiplex ID sequences monotonic on the wire Tim Gardner 2013-10-16 15:40 ` Jeff Layton @ 2013-10-16 16:25 ` Shirish Pargaonkar 2013-10-16 16:39 ` Tim Gardner 1 sibling, 1 reply; 8+ messages in thread From: Shirish Pargaonkar @ 2013-10-16 16:25 UTC (permalink / raw) To: Tim Gardner; +Cc: linux-cifs, samba-technical, LKML, Steve French On Wed, Oct 16, 2013 at 10:09 AM, Tim Gardner <timg@tpi.com> 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: Steve French <sfrench@samba.org> > Signed-off-by: Tim Gardner <timg@tpi.com> > --- > > I'm looking at some of this code in excrutiating detail because I'm having trouble > with a backport of CIFS from 3.9.7 on an embedded 2.6.31 powerpc. Its failing the RawNTLMSSP > authentication and is almost certainly an endian issue. x86 on the same code base works > quite well. Am I making a rash assumption that CIFS in 3.9 stable worked on big endian ? Do you have this commit fdf96a907c1fbb93c633e2b7ede3b8df26d6a4c0 ? This might help if you do not have it in your code base. > > fs/cifs/cifsglob.h | 25 ++++++++++++++++++++++++- > fs/cifs/misc.c | 9 +++++---- > fs/cifs/smb1ops.c | 4 ++-- > fs/cifs/smb2transport.c | 2 +- > fs/cifs/transport.c | 2 +- > 5 files changed, 33 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..c851d41 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", > + le16_to_cpu(smb->Mid)); > return 1; > } > > @@ -358,11 +359,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, le16_to_cpu(smb->Mid)); > > if (4 + rfclen < clc_len) { > cifs_dbg(VFS, "RFC1001 size %u smaller than SMB for mid=%u\n", > - rfclen, smb->Mid); > + rfclen, le16_to_cpu(smb->Mid)); > return -EIO; > } else if (rfclen > clc_len + 512) { > /* > @@ -375,7 +376,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, le16_to_cpu(smb->Mid)); > return -EIO; > } > } > diff --git a/fs/cifs/smb1ops.c b/fs/cifs/smb1ops.c > index 8233b17..5598af9 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); > + le16_to_cpu(in_buf->Mid), 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); > -- > 1.7.9.5 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-cifs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2 linux-next] cifs: Make big endian multiplex ID sequences monotonic on the wire 2013-10-16 16:25 ` Shirish Pargaonkar @ 2013-10-16 16:39 ` Tim Gardner 0 siblings, 0 replies; 8+ messages in thread From: Tim Gardner @ 2013-10-16 16:39 UTC (permalink / raw) To: Shirish Pargaonkar; +Cc: linux-cifs, samba-technical, LKML, Steve French On 10/16/2013 09:25 AM, Shirish Pargaonkar wrote: > On Wed, Oct 16, 2013 at 10:09 AM, Tim Gardner <timg@tpi.com> 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: Steve French <sfrench@samba.org> >> Signed-off-by: Tim Gardner <timg@tpi.com> >> --- >> >> I'm looking at some of this code in excrutiating detail because I'm having trouble >> with a backport of CIFS from 3.9.7 on an embedded 2.6.31 powerpc. Its failing the RawNTLMSSP >> authentication and is almost certainly an endian issue. x86 on the same code base works >> quite well. Am I making a rash assumption that CIFS in 3.9 stable worked on big endian ? > > Do you have this commit fdf96a907c1fbb93c633e2b7ede3b8df26d6a4c0 ? > This might help if you do not have it in your code base. > Yep - I found that one pretty early on. It solved a powerpc issue with session startup to an x86 Linux CIFS server. The current failing session startup is to an Apple 10.8.5 (Lion) server (serverNOS=@(#)PROGRAM:smbd PROJECT:smbx-136.20.1). rtg -- Tim Gardner timg@tpi.com www.tpi.com OR 503-601-0234 x102 MT 406-443-5357 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2 linux-next] cifs: Remove redundant multiplex identifier check from check_smb_hdr() 2013-10-16 15:09 [PATCH 1/2 linux-next] cifs: Remove redundant multiplex identifier check from check_smb_hdr() Tim Gardner 2013-10-16 15:09 ` [PATCH 2/2 linux-next] cifs: Make big endian multiplex ID sequences monotonic on the wire Tim Gardner @ 2013-10-16 15:36 ` Jeff Layton 2013-10-28 14:37 ` Steve French 1 sibling, 1 reply; 8+ messages in thread From: Jeff Layton @ 2013-10-16 15:36 UTC (permalink / raw) To: Tim Gardner; +Cc: linux-cifs, samba-technical, linux-kernel, Steve French On Wed, 16 Oct 2013 09:09:49 -0600 Tim Gardner <timg@tpi.com> wrote: > The only call site for check_smb_header() assigns 'mid' from the SMB > packet, which is then checked again in check_smb_header(). This seems > like redundant redundancy. > > Cc: Steve French <sfrench@samba.org> > Signed-off-by: Tim Gardner <timg@tpi.com> > --- > fs/cifs/misc.c | 12 ++---------- > 1 file changed, 2 insertions(+), 10 deletions(-) > > diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c > index 138a011..298e31e 100644 > --- a/fs/cifs/misc.c > +++ b/fs/cifs/misc.c > @@ -278,7 +278,7 @@ header_assemble(struct smb_hdr *buffer, char smb_command /* command */ , > } > > static int > -check_smb_hdr(struct smb_hdr *smb, __u16 mid) > +check_smb_hdr(struct smb_hdr *smb) > { > /* does it have the right SMB "signature" ? */ > if (*(__le32 *) smb->Protocol != cpu_to_le32(0x424d53ff)) { > @@ -287,13 +287,6 @@ check_smb_hdr(struct smb_hdr *smb, __u16 mid) > return 1; > } > > - /* Make sure that message ids match */ > - if (mid != smb->Mid) { > - cifs_dbg(VFS, "Mids do not match. received=%u expected=%u\n", > - smb->Mid, mid); > - return 1; > - } > - > /* if it's a response then accept */ > if (smb->Flags & SMBFLG_RESPONSE) > return 0; > @@ -310,7 +303,6 @@ int > checkSMB(char *buf, unsigned int total_read) > { > struct smb_hdr *smb = (struct smb_hdr *)buf; > - __u16 mid = smb->Mid; > __u32 rfclen = be32_to_cpu(smb->smb_buf_length); > __u32 clc_len; /* calculated length */ > cifs_dbg(FYI, "checkSMB Length: 0x%x, smb_buf_length: 0x%x\n", > @@ -348,7 +340,7 @@ checkSMB(char *buf, unsigned int total_read) > } > > /* otherwise, there is enough to get to the BCC */ > - if (check_smb_hdr(smb, mid)) > + if (check_smb_hdr(smb)) > return -EIO; > clc_len = smbCalcSize(smb); > Nice... Reviewed-by: Jeff Layton <jlayton@redhat.com> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2 linux-next] cifs: Remove redundant multiplex identifier check from check_smb_hdr() 2013-10-16 15:36 ` [PATCH 1/2 linux-next] cifs: Remove redundant multiplex identifier check from check_smb_hdr() Jeff Layton @ 2013-10-28 14:37 ` Steve French 0 siblings, 0 replies; 8+ messages in thread From: Steve French @ 2013-10-28 14:37 UTC (permalink / raw) To: Jeff Layton Cc: Tim Gardner, linux-cifs@vger.kernel.org, samba-technical, LKML, Steve French merged into cifs-2.6.git for-next On Wed, Oct 16, 2013 at 10:36 AM, Jeff Layton <jlayton@redhat.com> wrote: > On Wed, 16 Oct 2013 09:09:49 -0600 > Tim Gardner <timg@tpi.com> wrote: > >> The only call site for check_smb_header() assigns 'mid' from the SMB >> packet, which is then checked again in check_smb_header(). This seems >> like redundant redundancy. >> >> Cc: Steve French <sfrench@samba.org> >> Signed-off-by: Tim Gardner <timg@tpi.com> >> --- >> fs/cifs/misc.c | 12 ++---------- >> 1 file changed, 2 insertions(+), 10 deletions(-) >> >> diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c >> index 138a011..298e31e 100644 >> --- a/fs/cifs/misc.c >> +++ b/fs/cifs/misc.c >> @@ -278,7 +278,7 @@ header_assemble(struct smb_hdr *buffer, char smb_command /* command */ , >> } >> >> static int >> -check_smb_hdr(struct smb_hdr *smb, __u16 mid) >> +check_smb_hdr(struct smb_hdr *smb) >> { >> /* does it have the right SMB "signature" ? */ >> if (*(__le32 *) smb->Protocol != cpu_to_le32(0x424d53ff)) { >> @@ -287,13 +287,6 @@ check_smb_hdr(struct smb_hdr *smb, __u16 mid) >> return 1; >> } >> >> - /* Make sure that message ids match */ >> - if (mid != smb->Mid) { >> - cifs_dbg(VFS, "Mids do not match. received=%u expected=%u\n", >> - smb->Mid, mid); >> - return 1; >> - } >> - >> /* if it's a response then accept */ >> if (smb->Flags & SMBFLG_RESPONSE) >> return 0; >> @@ -310,7 +303,6 @@ int >> checkSMB(char *buf, unsigned int total_read) >> { >> struct smb_hdr *smb = (struct smb_hdr *)buf; >> - __u16 mid = smb->Mid; >> __u32 rfclen = be32_to_cpu(smb->smb_buf_length); >> __u32 clc_len; /* calculated length */ >> cifs_dbg(FYI, "checkSMB Length: 0x%x, smb_buf_length: 0x%x\n", >> @@ -348,7 +340,7 @@ checkSMB(char *buf, unsigned int total_read) >> } >> >> /* otherwise, there is enough to get to the BCC */ >> - if (check_smb_hdr(smb, mid)) >> + if (check_smb_hdr(smb)) >> return -EIO; >> clc_len = smbCalcSize(smb); >> > > Nice... > > Reviewed-by: Jeff Layton <jlayton@redhat.com> -- Thanks, Steve ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2013-10-28 14:37 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-10-16 15:09 [PATCH 1/2 linux-next] cifs: Remove redundant multiplex identifier check from check_smb_hdr() Tim Gardner 2013-10-16 15:09 ` [PATCH 2/2 linux-next] cifs: Make big endian multiplex ID sequences monotonic on the wire Tim Gardner 2013-10-16 15:40 ` Jeff Layton 2013-10-16 16:02 ` Tim Gardner 2013-10-16 16:25 ` Shirish Pargaonkar 2013-10-16 16:39 ` Tim Gardner 2013-10-16 15:36 ` [PATCH 1/2 linux-next] cifs: Remove redundant multiplex identifier check from check_smb_hdr() Jeff Layton 2013-10-28 14:37 ` Steve French
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox