* [PATCH linux-next V2] cifs: Make big endian multiplex ID sequences monotonic on the wire
@ 2013-10-16 16:32 Tim Gardner
2013-10-16 20:03 ` Jeff Layton
2013-11-01 18:50 ` Steve French
0 siblings, 2 replies; 5+ messages in thread
From: Tim Gardner @ 2013-10-16 16:32 UTC (permalink / raw)
To: linux-cifs, samba-technical, linux-kernel
Cc: Tim Gardner, Jeff Layton, 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: Jeff Layton <jlayton@redhat.com>
Cc: Steve French <sfrench@samba.org>
Signed-off-by: Tim Gardner <timg@tpi.com>
---
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);
--
1.7.9.5
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH linux-next V2] cifs: Make big endian multiplex ID sequences monotonic on the wire
2013-10-16 16:32 [PATCH linux-next V2] cifs: Make big endian multiplex ID sequences monotonic on the wire Tim Gardner
@ 2013-10-16 20:03 ` Jeff Layton
2013-10-28 14:38 ` Steve French
2013-11-01 18:50 ` Steve French
1 sibling, 1 reply; 5+ messages in thread
From: Jeff Layton @ 2013-10-16 20:03 UTC (permalink / raw)
To: Tim Gardner; +Cc: linux-cifs, samba-technical, linux-kernel, Steve French
On Wed, 16 Oct 2013 10:32:42 -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: Jeff Layton <jlayton@redhat.com>
> Cc: Steve French <sfrench@samba.org>
> Signed-off-by: Tim Gardner <timg@tpi.com>
> ---
>
> 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 <jlayton@redhat.com>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH linux-next V2] cifs: Make big endian multiplex ID sequences monotonic on the wire
2013-10-16 20:03 ` Jeff Layton
@ 2013-10-28 14:38 ` Steve French
0 siblings, 0 replies; 5+ messages in thread
From: Steve French @ 2013-10-28 14:38 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 3:03 PM, Jeff Layton <jlayton@redhat.com> wrote:
> On Wed, 16 Oct 2013 10:32:42 -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: Jeff Layton <jlayton@redhat.com>
>> Cc: Steve French <sfrench@samba.org>
>> Signed-off-by: Tim Gardner <timg@tpi.com>
>> ---
>>
>> 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 <jlayton@redhat.com>
--
Thanks,
Steve
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH linux-next V2] cifs: Make big endian multiplex ID sequences monotonic on the wire
2013-10-16 16:32 [PATCH linux-next V2] cifs: Make big endian multiplex ID sequences monotonic on the wire Tim Gardner
2013-10-16 20:03 ` Jeff Layton
@ 2013-11-01 18:50 ` Steve French
2013-11-01 19:04 ` Tim Gardner
1 sibling, 1 reply; 5+ messages in thread
From: Steve French @ 2013-11-01 18:50 UTC (permalink / raw)
To: Tim Gardner
Cc: linux-cifs@vger.kernel.org, samba-technical, LKML, Jeff Layton,
Steve French
This has a couple of obvious endian errors. I will correct your patch
and remerge into cifs-2.6.git for-next
Please always remember to run endian checks against cifs builds when
submitting a patch (and make sure sparse is installed)
e.g.
make C=1 M=fs/cifs modules CF=-D__CHECK_ENDIAN__
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index bc7c978..1a1fdcc 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -630,7 +630,7 @@ get_next_mid64(struct TCP_Server_Info *server)
return server->ops->get_next_mid(server);
}
-static inline __u16
+static inline __le16
get_next_mid(struct TCP_Server_Info *server)
{
__u16 mid = get_next_mid64(server);
diff --git a/fs/cifs/cifspdu.h b/fs/cifs/cifspdu.h
index f9bb497..9e5ee34 100644
--- a/fs/cifs/cifspdu.h
+++ b/fs/cifs/cifspdu.h
@@ -428,7 +428,7 @@ struct smb_hdr {
__u16 Tid;
__le16 Pid;
__u16 Uid;
- __u16 Mid;
+ __le16 Mid;
__u8 WordCount;
} __attribute__((packed));
On Wed, Oct 16, 2013 at 11:32 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: Jeff Layton <jlayton@redhat.com>
> Cc: Steve French <sfrench@samba.org>
> Signed-off-by: Tim Gardner <timg@tpi.com>
> ---
>
> 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);
> --
> 1.7.9.5
>
--
Thanks,
Steve
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH linux-next V2] cifs: Make big endian multiplex ID sequences monotonic on the wire
2013-11-01 18:50 ` Steve French
@ 2013-11-01 19:04 ` Tim Gardner
0 siblings, 0 replies; 5+ messages in thread
From: Tim Gardner @ 2013-11-01 19:04 UTC (permalink / raw)
To: Steve French
Cc: linux-cifs@vger.kernel.org, samba-technical, LKML, Jeff Layton,
Steve French
On 11/01/2013 11:50 AM, Steve French wrote:
> This has a couple of obvious endian errors. I will correct your patch
> and remerge into cifs-2.6.git for-next
>
> Please always remember to run endian checks against cifs builds when
> submitting a patch (and make sure sparse is installed)
>
> e.g.
>
> make C=1 M=fs/cifs modules CF=-D__CHECK_ENDIAN__
>
Didn't know about __CHECK_ENDIAN__, but will do so in the future. Your
changes look fine.
rtg
--
Tim Gardner timg@tpi.com www.tpi.com
OR 503-601-0234 x102 MT 406-443-5357
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-11-01 19:04 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-10-16 16:32 [PATCH linux-next V2] cifs: Make big endian multiplex ID sequences monotonic on the wire Tim Gardner
2013-10-16 20:03 ` Jeff Layton
2013-10-28 14:38 ` Steve French
2013-11-01 18:50 ` Steve French
2013-11-01 19:04 ` Tim Gardner
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox