linux-cifs.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] cifs: smbd: Calculate the correct maximum packet size for segmented SMBDirect send/receive
@ 2020-03-25 19:30 longli
  2020-03-26  2:02 ` ronnie sahlberg
  0 siblings, 1 reply; 5+ messages in thread
From: longli @ 2020-03-25 19:30 UTC (permalink / raw)
  To: Steve French, linux-cifs, samba-technical, linux-kernel; +Cc: Long Li

From: Long Li <longli@microsoft.com>

The packet size needs to take account of SMB2 header size and possible
encryption header size. This is only done when signing is used and it is for
RDMA send/receive, not read/write.

Also remove the dead SMBD code in smb2_negotiate_r(w)size.

Signed-off-by: Long Li <longli@microsoft.com>
---
 fs/cifs/smb2ops.c   | 38 ++++++++++++++++----------------------
 fs/cifs/smbdirect.c |  3 +--
 2 files changed, 17 insertions(+), 24 deletions(-)

diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
index 4c0922596467..9043d34eef43 100644
--- a/fs/cifs/smb2ops.c
+++ b/fs/cifs/smb2ops.c
@@ -332,16 +332,6 @@ smb2_negotiate_wsize(struct cifs_tcon *tcon, struct smb_vol *volume_info)
 	/* start with specified wsize, or default */
 	wsize = volume_info->wsize ? volume_info->wsize : CIFS_DEFAULT_IOSIZE;
 	wsize = min_t(unsigned int, wsize, server->max_write);
-#ifdef CONFIG_CIFS_SMB_DIRECT
-	if (server->rdma) {
-		if (server->sign)
-			wsize = min_t(unsigned int,
-				wsize, server->smbd_conn->max_fragmented_send_size);
-		else
-			wsize = min_t(unsigned int,
-				wsize, server->smbd_conn->max_readwrite_size);
-	}
-#endif
 	if (!(server->capabilities & SMB2_GLOBAL_CAP_LARGE_MTU))
 		wsize = min_t(unsigned int, wsize, SMB2_MAX_BUFFER_SIZE);
 
@@ -360,8 +350,15 @@ smb3_negotiate_wsize(struct cifs_tcon *tcon, struct smb_vol *volume_info)
 #ifdef CONFIG_CIFS_SMB_DIRECT
 	if (server->rdma) {
 		if (server->sign)
+			/*
+			 * Account for SMB2 data transfer packet header
+			 * SMB2_READ/SMB2_WRITE (49) and possible encryption
+			 * headers
+			 */
 			wsize = min_t(unsigned int,
-				wsize, server->smbd_conn->max_fragmented_send_size);
+				wsize,
+				server->smbd_conn->max_fragmented_send_size -
+					49 - sizeof(struct smb2_transform_hdr));
 		else
 			wsize = min_t(unsigned int,
 				wsize, server->smbd_conn->max_readwrite_size);
@@ -382,16 +379,6 @@ smb2_negotiate_rsize(struct cifs_tcon *tcon, struct smb_vol *volume_info)
 	/* start with specified rsize, or default */
 	rsize = volume_info->rsize ? volume_info->rsize : CIFS_DEFAULT_IOSIZE;
 	rsize = min_t(unsigned int, rsize, server->max_read);
-#ifdef CONFIG_CIFS_SMB_DIRECT
-	if (server->rdma) {
-		if (server->sign)
-			rsize = min_t(unsigned int,
-				rsize, server->smbd_conn->max_fragmented_recv_size);
-		else
-			rsize = min_t(unsigned int,
-				rsize, server->smbd_conn->max_readwrite_size);
-	}
-#endif
 
 	if (!(server->capabilities & SMB2_GLOBAL_CAP_LARGE_MTU))
 		rsize = min_t(unsigned int, rsize, SMB2_MAX_BUFFER_SIZE);
@@ -411,8 +398,15 @@ smb3_negotiate_rsize(struct cifs_tcon *tcon, struct smb_vol *volume_info)
 #ifdef CONFIG_CIFS_SMB_DIRECT
 	if (server->rdma) {
 		if (server->sign)
+			/*
+			 * Account for SMB2 data transfer packet header
+			 * SMB2_READ/SMB2_WRITE (49) and possible encryption
+			 * headers
+			 */
 			rsize = min_t(unsigned int,
-				rsize, server->smbd_conn->max_fragmented_recv_size);
+				rsize,
+				server->smbd_conn->max_fragmented_recv_size -
+					49 - sizeof(struct smb2_transform_hdr));
 		else
 			rsize = min_t(unsigned int,
 				rsize, server->smbd_conn->max_readwrite_size);
diff --git a/fs/cifs/smbdirect.c b/fs/cifs/smbdirect.c
index eb1e40af9f3a..0327b575ab87 100644
--- a/fs/cifs/smbdirect.c
+++ b/fs/cifs/smbdirect.c
@@ -2097,8 +2097,7 @@ int smbd_send(struct TCP_Server_Info *server,
 	for (i = 0; i < num_rqst; i++)
 		remaining_data_length += smb_rqst_len(server, &rqst_array[i]);
 
-	if (remaining_data_length + sizeof(struct smbd_data_transfer) >
-		info->max_fragmented_send_size) {
+	if (remaining_data_length > info->max_fragmented_send_size) {
 		log_write(ERR, "payload size %d > max size %d\n",
 			remaining_data_length, info->max_fragmented_send_size);
 		rc = -EINVAL;
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] cifs: smbd: Calculate the correct maximum packet size for segmented SMBDirect send/receive
  2020-03-25 19:30 [PATCH] cifs: smbd: Calculate the correct maximum packet size for segmented SMBDirect send/receive longli
@ 2020-03-26  2:02 ` ronnie sahlberg
  2020-03-26  2:43   ` Long Li
  0 siblings, 1 reply; 5+ messages in thread
From: ronnie sahlberg @ 2020-03-26  2:02 UTC (permalink / raw)
  To: Long Li; +Cc: Steve French, linux-cifs, samba-technical, LKML, longli

Long Li,

Mostly looks good  but the magic constant 49 I think is wrong.
49 is the structure size of the read/write request header
but for these sizes, if they are odd it just means that the header contain a
variable sized blob.
I.e. the size is 48 bytes (for the fixed part of the header) + a variable part
which in this case are the ChannelInfo blobs.

So we should probably add to smb2pdu.h a
#define MAX_SMB2_READWRITE_RESPONSE_SIZE 48
and use this in the calculations. Then we need to add the maximum size
we will use for ChannelInfo.

Maybe we should have a define also for the MAX_SMB2_CHANNEL_INFO_SIZE

regards
ronnie sahlberg

On Thu, Mar 26, 2020 at 5:31 AM longli--- via samba-technical
<samba-technical@lists.samba.org> wrote:
>
> From: Long Li <longli@microsoft.com>
>
> The packet size needs to take account of SMB2 header size and possible
> encryption header size. This is only done when signing is used and it is for
> RDMA send/receive, not read/write.
>
> Also remove the dead SMBD code in smb2_negotiate_r(w)size.
>
> Signed-off-by: Long Li <longli@microsoft.com>
> ---
>  fs/cifs/smb2ops.c   | 38 ++++++++++++++++----------------------
>  fs/cifs/smbdirect.c |  3 +--
>  2 files changed, 17 insertions(+), 24 deletions(-)
>
> diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> index 4c0922596467..9043d34eef43 100644
> --- a/fs/cifs/smb2ops.c
> +++ b/fs/cifs/smb2ops.c
> @@ -332,16 +332,6 @@ smb2_negotiate_wsize(struct cifs_tcon *tcon, struct smb_vol *volume_info)
>         /* start with specified wsize, or default */
>         wsize = volume_info->wsize ? volume_info->wsize : CIFS_DEFAULT_IOSIZE;
>         wsize = min_t(unsigned int, wsize, server->max_write);
> -#ifdef CONFIG_CIFS_SMB_DIRECT
> -       if (server->rdma) {
> -               if (server->sign)
> -                       wsize = min_t(unsigned int,
> -                               wsize, server->smbd_conn->max_fragmented_send_size);
> -               else
> -                       wsize = min_t(unsigned int,
> -                               wsize, server->smbd_conn->max_readwrite_size);
> -       }
> -#endif
>         if (!(server->capabilities & SMB2_GLOBAL_CAP_LARGE_MTU))
>                 wsize = min_t(unsigned int, wsize, SMB2_MAX_BUFFER_SIZE);
>
> @@ -360,8 +350,15 @@ smb3_negotiate_wsize(struct cifs_tcon *tcon, struct smb_vol *volume_info)
>  #ifdef CONFIG_CIFS_SMB_DIRECT
>         if (server->rdma) {
>                 if (server->sign)
> +                       /*
> +                        * Account for SMB2 data transfer packet header
> +                        * SMB2_READ/SMB2_WRITE (49) and possible encryption
> +                        * headers
> +                        */
>                         wsize = min_t(unsigned int,
> -                               wsize, server->smbd_conn->max_fragmented_send_size);
> +                               wsize,
> +                               server->smbd_conn->max_fragmented_send_size -
> +                                       49 - sizeof(struct smb2_transform_hdr));
>                 else
>                         wsize = min_t(unsigned int,
>                                 wsize, server->smbd_conn->max_readwrite_size);
> @@ -382,16 +379,6 @@ smb2_negotiate_rsize(struct cifs_tcon *tcon, struct smb_vol *volume_info)
>         /* start with specified rsize, or default */
>         rsize = volume_info->rsize ? volume_info->rsize : CIFS_DEFAULT_IOSIZE;
>         rsize = min_t(unsigned int, rsize, server->max_read);
> -#ifdef CONFIG_CIFS_SMB_DIRECT
> -       if (server->rdma) {
> -               if (server->sign)
> -                       rsize = min_t(unsigned int,
> -                               rsize, server->smbd_conn->max_fragmented_recv_size);
> -               else
> -                       rsize = min_t(unsigned int,
> -                               rsize, server->smbd_conn->max_readwrite_size);
> -       }
> -#endif
>
>         if (!(server->capabilities & SMB2_GLOBAL_CAP_LARGE_MTU))
>                 rsize = min_t(unsigned int, rsize, SMB2_MAX_BUFFER_SIZE);
> @@ -411,8 +398,15 @@ smb3_negotiate_rsize(struct cifs_tcon *tcon, struct smb_vol *volume_info)
>  #ifdef CONFIG_CIFS_SMB_DIRECT
>         if (server->rdma) {
>                 if (server->sign)
> +                       /*
> +                        * Account for SMB2 data transfer packet header
> +                        * SMB2_READ/SMB2_WRITE (49) and possible encryption
> +                        * headers
> +                        */
>                         rsize = min_t(unsigned int,
> -                               rsize, server->smbd_conn->max_fragmented_recv_size);
> +                               rsize,
> +                               server->smbd_conn->max_fragmented_recv_size -
> +                                       49 - sizeof(struct smb2_transform_hdr));
>                 else
>                         rsize = min_t(unsigned int,
>                                 rsize, server->smbd_conn->max_readwrite_size);
> diff --git a/fs/cifs/smbdirect.c b/fs/cifs/smbdirect.c
> index eb1e40af9f3a..0327b575ab87 100644
> --- a/fs/cifs/smbdirect.c
> +++ b/fs/cifs/smbdirect.c
> @@ -2097,8 +2097,7 @@ int smbd_send(struct TCP_Server_Info *server,
>         for (i = 0; i < num_rqst; i++)
>                 remaining_data_length += smb_rqst_len(server, &rqst_array[i]);
>
> -       if (remaining_data_length + sizeof(struct smbd_data_transfer) >
> -               info->max_fragmented_send_size) {
> +       if (remaining_data_length > info->max_fragmented_send_size) {
>                 log_write(ERR, "payload size %d > max size %d\n",
>                         remaining_data_length, info->max_fragmented_send_size);
>                 rc = -EINVAL;
> --
> 2.17.1
>
>

^ permalink raw reply	[flat|nested] 5+ messages in thread

* RE: [PATCH] cifs: smbd: Calculate the correct maximum packet size for segmented SMBDirect send/receive
  2020-03-26  2:02 ` ronnie sahlberg
@ 2020-03-26  2:43   ` Long Li
  2020-03-26  3:03     ` ronnie sahlberg
  0 siblings, 1 reply; 5+ messages in thread
From: Long Li @ 2020-03-26  2:43 UTC (permalink / raw)
  To: ronnie sahlberg; +Cc: Steve French, linux-cifs, samba-technical, LKML, longli

>Subject: Re: [PATCH] cifs: smbd: Calculate the correct maximum packet size
>for segmented SMBDirect send/receive
>
>Long Li,
>
>Mostly looks good  but the magic constant 49 I think is wrong.
>49 is the structure size of the read/write request header but for these sizes, if
>they are odd it just means that the header contain a variable sized blob.
>I.e. the size is 48 bytes (for the fixed part of the header) + a variable part
>which in this case are the ChannelInfo blobs.

Agreed. Using a magic number is not a good idea.

I was looking at request size definitions in 
"fs/cifs/smb2pdu.c",  they are defined at
static const int smb2_req_struct_sizes[NUMBER_OF_SMB2_COMMANDS]

for read and write, the sizes are:
        /* SMB2_READ */ 49,
        /* SMB2_WRITE */ 49,

They are used to calculate command header sizes, and fill in the request headers. Maybe I should use this variable to get the size?

>
>So we should probably add to smb2pdu.h a #define
>MAX_SMB2_READWRITE_RESPONSE_SIZE 48 and use this in the calculations.
>Then we need to add the maximum size we will use for ChannelInfo.
>
>Maybe we should have a define also for the
>MAX_SMB2_CHANNEL_INFO_SIZE
>
>regards
>ronnie sahlberg
>
>On Thu, Mar 26, 2020 at 5:31 AM longli--- via samba-technical <samba-
>technical@lists.samba.org> wrote:
>>
>> From: Long Li <longli@microsoft.com>
>>
>> The packet size needs to take account of SMB2 header size and possible
>> encryption header size. This is only done when signing is used and it
>> is for RDMA send/receive, not read/write.
>>
>> Also remove the dead SMBD code in smb2_negotiate_r(w)size.
>>
>> Signed-off-by: Long Li <longli@microsoft.com>
>> ---
>>  fs/cifs/smb2ops.c   | 38 ++++++++++++++++----------------------
>>  fs/cifs/smbdirect.c |  3 +--
>>  2 files changed, 17 insertions(+), 24 deletions(-)
>>
>> diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c index
>> 4c0922596467..9043d34eef43 100644
>> --- a/fs/cifs/smb2ops.c
>> +++ b/fs/cifs/smb2ops.c
>> @@ -332,16 +332,6 @@ smb2_negotiate_wsize(struct cifs_tcon *tcon,
>struct smb_vol *volume_info)
>>         /* start with specified wsize, or default */
>>         wsize = volume_info->wsize ? volume_info->wsize :
>CIFS_DEFAULT_IOSIZE;
>>         wsize = min_t(unsigned int, wsize, server->max_write); -#ifdef
>> CONFIG_CIFS_SMB_DIRECT
>> -       if (server->rdma) {
>> -               if (server->sign)
>> -                       wsize = min_t(unsigned int,
>> -                               wsize, server->smbd_conn->max_fragmented_send_size);
>> -               else
>> -                       wsize = min_t(unsigned int,
>> -                               wsize, server->smbd_conn->max_readwrite_size);
>> -       }
>> -#endif
>>         if (!(server->capabilities & SMB2_GLOBAL_CAP_LARGE_MTU))
>>                 wsize = min_t(unsigned int, wsize,
>> SMB2_MAX_BUFFER_SIZE);
>>
>> @@ -360,8 +350,15 @@ smb3_negotiate_wsize(struct cifs_tcon *tcon,
>> struct smb_vol *volume_info)  #ifdef CONFIG_CIFS_SMB_DIRECT
>>         if (server->rdma) {
>>                 if (server->sign)
>> +                       /*
>> +                        * Account for SMB2 data transfer packet header
>> +                        * SMB2_READ/SMB2_WRITE (49) and possible encryption
>> +                        * headers
>> +                        */
>>                         wsize = min_t(unsigned int,
>> -                               wsize, server->smbd_conn->max_fragmented_send_size);
>> +                               wsize,
>> +                               server->smbd_conn->max_fragmented_send_size -
>> +                                       49 - sizeof(struct
>> + smb2_transform_hdr));
>>                 else
>>                         wsize = min_t(unsigned int,
>>                                 wsize,
>> server->smbd_conn->max_readwrite_size);
>> @@ -382,16 +379,6 @@ smb2_negotiate_rsize(struct cifs_tcon *tcon, struct
>smb_vol *volume_info)
>>         /* start with specified rsize, or default */
>>         rsize = volume_info->rsize ? volume_info->rsize : CIFS_DEFAULT_IOSIZE;
>>         rsize = min_t(unsigned int, rsize, server->max_read); -#ifdef
>> CONFIG_CIFS_SMB_DIRECT
>> -       if (server->rdma) {
>> -               if (server->sign)
>> -                       rsize = min_t(unsigned int,
>> -                               rsize, server->smbd_conn->max_fragmented_recv_size);
>> -               else
>> -                       rsize = min_t(unsigned int,
>> -                               rsize, server->smbd_conn->max_readwrite_size);
>> -       }
>> -#endif
>>
>>         if (!(server->capabilities & SMB2_GLOBAL_CAP_LARGE_MTU))
>>                 rsize = min_t(unsigned int, rsize,
>> SMB2_MAX_BUFFER_SIZE); @@ -411,8 +398,15 @@
>> smb3_negotiate_rsize(struct cifs_tcon *tcon, struct smb_vol *volume_info)
>#ifdef CONFIG_CIFS_SMB_DIRECT
>>         if (server->rdma) {
>>                 if (server->sign)
>> +                       /*
>> +                        * Account for SMB2 data transfer packet header
>> +                        * SMB2_READ/SMB2_WRITE (49) and possible encryption
>> +                        * headers
>> +                        */
>>                         rsize = min_t(unsigned int,
>> -                               rsize, server->smbd_conn->max_fragmented_recv_size);
>> +                               rsize,
>> +                               server->smbd_conn->max_fragmented_recv_size -
>> +                                       49 - sizeof(struct
>> + smb2_transform_hdr));
>>                 else
>>                         rsize = min_t(unsigned int,
>>                                 rsize,
>> server->smbd_conn->max_readwrite_size);
>> diff --git a/fs/cifs/smbdirect.c b/fs/cifs/smbdirect.c index
>> eb1e40af9f3a..0327b575ab87 100644
>> --- a/fs/cifs/smbdirect.c
>> +++ b/fs/cifs/smbdirect.c
>> @@ -2097,8 +2097,7 @@ int smbd_send(struct TCP_Server_Info *server,
>>         for (i = 0; i < num_rqst; i++)
>>                 remaining_data_length += smb_rqst_len(server,
>> &rqst_array[i]);
>>
>> -       if (remaining_data_length + sizeof(struct smbd_data_transfer) >
>> -               info->max_fragmented_send_size) {
>> +       if (remaining_data_length > info->max_fragmented_send_size) {
>>                 log_write(ERR, "payload size %d > max size %d\n",
>>                         remaining_data_length, info->max_fragmented_send_size);
>>                 rc = -EINVAL;
>> --
>> 2.17.1
>>
>>

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] cifs: smbd: Calculate the correct maximum packet size for segmented SMBDirect send/receive
  2020-03-26  2:43   ` Long Li
@ 2020-03-26  3:03     ` ronnie sahlberg
  2020-03-26  3:08       ` Long Li
  0 siblings, 1 reply; 5+ messages in thread
From: ronnie sahlberg @ 2020-03-26  3:03 UTC (permalink / raw)
  To: Long Li; +Cc: Steve French, linux-cifs, samba-technical, LKML, longli

On Thu, Mar 26, 2020 at 12:43 PM Long Li <longli@microsoft.com> wrote:
>
> >Subject: Re: [PATCH] cifs: smbd: Calculate the correct maximum packet size
> >for segmented SMBDirect send/receive
> >
> >Long Li,
> >
> >Mostly looks good  but the magic constant 49 I think is wrong.
> >49 is the structure size of the read/write request header but for these sizes, if
> >they are odd it just means that the header contain a variable sized blob.
> >I.e. the size is 48 bytes (for the fixed part of the header) + a variable part
> >which in this case are the ChannelInfo blobs.
>
> Agreed. Using a magic number is not a good idea.
>
> I was looking at request size definitions in
> "fs/cifs/smb2pdu.c",  they are defined at
> static const int smb2_req_struct_sizes[NUMBER_OF_SMB2_COMMANDS]
>
> for read and write, the sizes are:
>         /* SMB2_READ */ 49,
>         /* SMB2_WRITE */ 49,
>
> They are used to calculate command header sizes, and fill in the request headers. Maybe I should use this variable to get the size?

Nah, those values only specify what values must be set as the
structure size in the actual packet headers.
But they do not represent the actual sizes.
All command headers start with a 2 byte StructureSize field where
everything except the least significant bit indicates the fixed
portion of the response header
and if the least significant bit is set, then it indicates that the
header contains an additional
variable sized part of header,  (the size of which can be determined
by looking at the individual fields in the fixed portion of the
header).

So 49 does not indicate that the header is 49 bytes in size. It means
the header contains a 48 byte fixed part that is present in every such
command headers but it also contains 0 or more bytes of additional header.

So see these numbers as "these are the values we must put in the
StructureSize for the header"  but they do not represent the actual
size of the header. (well, except if the value is even, then it does
represent the header size.)


So for SMB2_READ and SMB2_WRITE commands, If we do not use RDMA, then
Channel will be set to 0
and we will not have any ReadWriteChannelInfo data.
In this case the READ/WRITE header size is always 48  (==49 & 0xfffe)

If we are using RDMA, then we still have the 48 byte normal header but
we also have 1 or more SMB_DIRECT_BUFFER_DESCRIPTOR_V1 structures
there.
So for that case the maximum header size would be 48  + <max number of
structures> * sizeof(SMB_DIRECT_BUFFER_DESCRIPTOR_V1)


regards
ronnie sahlberg



>
> >
> >So we should probably add to smb2pdu.h a #define
> >MAX_SMB2_READWRITE_RESPONSE_SIZE 48 and use this in the calculations.
> >Then we need to add the maximum size we will use for ChannelInfo.
> >
> >Maybe we should have a define also for the
> >MAX_SMB2_CHANNEL_INFO_SIZE
> >
> >regards
> >ronnie sahlberg
> >
> >On Thu, Mar 26, 2020 at 5:31 AM longli--- via samba-technical <samba-
> >technical@lists.samba.org> wrote:
> >>
> >> From: Long Li <longli@microsoft.com>
> >>
> >> The packet size needs to take account of SMB2 header size and possible
> >> encryption header size. This is only done when signing is used and it
> >> is for RDMA send/receive, not read/write.
> >>
> >> Also remove the dead SMBD code in smb2_negotiate_r(w)size.
> >>
> >> Signed-off-by: Long Li <longli@microsoft.com>
> >> ---
> >>  fs/cifs/smb2ops.c   | 38 ++++++++++++++++----------------------
> >>  fs/cifs/smbdirect.c |  3 +--
> >>  2 files changed, 17 insertions(+), 24 deletions(-)
> >>
> >> diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c index
> >> 4c0922596467..9043d34eef43 100644
> >> --- a/fs/cifs/smb2ops.c
> >> +++ b/fs/cifs/smb2ops.c
> >> @@ -332,16 +332,6 @@ smb2_negotiate_wsize(struct cifs_tcon *tcon,
> >struct smb_vol *volume_info)
> >>         /* start with specified wsize, or default */
> >>         wsize = volume_info->wsize ? volume_info->wsize :
> >CIFS_DEFAULT_IOSIZE;
> >>         wsize = min_t(unsigned int, wsize, server->max_write); -#ifdef
> >> CONFIG_CIFS_SMB_DIRECT
> >> -       if (server->rdma) {
> >> -               if (server->sign)
> >> -                       wsize = min_t(unsigned int,
> >> -                               wsize, server->smbd_conn->max_fragmented_send_size);
> >> -               else
> >> -                       wsize = min_t(unsigned int,
> >> -                               wsize, server->smbd_conn->max_readwrite_size);
> >> -       }
> >> -#endif
> >>         if (!(server->capabilities & SMB2_GLOBAL_CAP_LARGE_MTU))
> >>                 wsize = min_t(unsigned int, wsize,
> >> SMB2_MAX_BUFFER_SIZE);
> >>
> >> @@ -360,8 +350,15 @@ smb3_negotiate_wsize(struct cifs_tcon *tcon,
> >> struct smb_vol *volume_info)  #ifdef CONFIG_CIFS_SMB_DIRECT
> >>         if (server->rdma) {
> >>                 if (server->sign)
> >> +                       /*
> >> +                        * Account for SMB2 data transfer packet header
> >> +                        * SMB2_READ/SMB2_WRITE (49) and possible encryption
> >> +                        * headers
> >> +                        */
> >>                         wsize = min_t(unsigned int,
> >> -                               wsize, server->smbd_conn->max_fragmented_send_size);
> >> +                               wsize,
> >> +                               server->smbd_conn->max_fragmented_send_size -
> >> +                                       49 - sizeof(struct
> >> + smb2_transform_hdr));
> >>                 else
> >>                         wsize = min_t(unsigned int,
> >>                                 wsize,
> >> server->smbd_conn->max_readwrite_size);
> >> @@ -382,16 +379,6 @@ smb2_negotiate_rsize(struct cifs_tcon *tcon, struct
> >smb_vol *volume_info)
> >>         /* start with specified rsize, or default */
> >>         rsize = volume_info->rsize ? volume_info->rsize : CIFS_DEFAULT_IOSIZE;
> >>         rsize = min_t(unsigned int, rsize, server->max_read); -#ifdef
> >> CONFIG_CIFS_SMB_DIRECT
> >> -       if (server->rdma) {
> >> -               if (server->sign)
> >> -                       rsize = min_t(unsigned int,
> >> -                               rsize, server->smbd_conn->max_fragmented_recv_size);
> >> -               else
> >> -                       rsize = min_t(unsigned int,
> >> -                               rsize, server->smbd_conn->max_readwrite_size);
> >> -       }
> >> -#endif
> >>
> >>         if (!(server->capabilities & SMB2_GLOBAL_CAP_LARGE_MTU))
> >>                 rsize = min_t(unsigned int, rsize,
> >> SMB2_MAX_BUFFER_SIZE); @@ -411,8 +398,15 @@
> >> smb3_negotiate_rsize(struct cifs_tcon *tcon, struct smb_vol *volume_info)
> >#ifdef CONFIG_CIFS_SMB_DIRECT
> >>         if (server->rdma) {
> >>                 if (server->sign)
> >> +                       /*
> >> +                        * Account for SMB2 data transfer packet header
> >> +                        * SMB2_READ/SMB2_WRITE (49) and possible encryption
> >> +                        * headers
> >> +                        */
> >>                         rsize = min_t(unsigned int,
> >> -                               rsize, server->smbd_conn->max_fragmented_recv_size);
> >> +                               rsize,
> >> +                               server->smbd_conn->max_fragmented_recv_size -
> >> +                                       49 - sizeof(struct
> >> + smb2_transform_hdr));
> >>                 else
> >>                         rsize = min_t(unsigned int,
> >>                                 rsize,
> >> server->smbd_conn->max_readwrite_size);
> >> diff --git a/fs/cifs/smbdirect.c b/fs/cifs/smbdirect.c index
> >> eb1e40af9f3a..0327b575ab87 100644
> >> --- a/fs/cifs/smbdirect.c
> >> +++ b/fs/cifs/smbdirect.c
> >> @@ -2097,8 +2097,7 @@ int smbd_send(struct TCP_Server_Info *server,
> >>         for (i = 0; i < num_rqst; i++)
> >>                 remaining_data_length += smb_rqst_len(server,
> >> &rqst_array[i]);
> >>
> >> -       if (remaining_data_length + sizeof(struct smbd_data_transfer) >
> >> -               info->max_fragmented_send_size) {
> >> +       if (remaining_data_length > info->max_fragmented_send_size) {
> >>                 log_write(ERR, "payload size %d > max size %d\n",
> >>                         remaining_data_length, info->max_fragmented_send_size);
> >>                 rc = -EINVAL;
> >> --
> >> 2.17.1
> >>
> >>

^ permalink raw reply	[flat|nested] 5+ messages in thread

* RE: [PATCH] cifs: smbd: Calculate the correct maximum packet size for segmented SMBDirect send/receive
  2020-03-26  3:03     ` ronnie sahlberg
@ 2020-03-26  3:08       ` Long Li
  0 siblings, 0 replies; 5+ messages in thread
From: Long Li @ 2020-03-26  3:08 UTC (permalink / raw)
  To: ronnie sahlberg; +Cc: Steve French, linux-cifs, samba-technical, LKML, longli

>Subject: Re: [PATCH] cifs: smbd: Calculate the correct maximum packet size
>for segmented SMBDirect send/receive
>
>On Thu, Mar 26, 2020 at 12:43 PM Long Li <longli@microsoft.com> wrote:
>>
>> >Subject: Re: [PATCH] cifs: smbd: Calculate the correct maximum packet
>> >size for segmented SMBDirect send/receive
>> >
>> >Long Li,
>> >
>> >Mostly looks good  but the magic constant 49 I think is wrong.
>> >49 is the structure size of the read/write request header but for
>> >these sizes, if they are odd it just means that the header contain a variable
>sized blob.
>> >I.e. the size is 48 bytes (for the fixed part of the header) + a
>> >variable part which in this case are the ChannelInfo blobs.
>>
>> Agreed. Using a magic number is not a good idea.
>>
>> I was looking at request size definitions in "fs/cifs/smb2pdu.c",
>> they are defined at static const int
>> smb2_req_struct_sizes[NUMBER_OF_SMB2_COMMANDS]
>>
>> for read and write, the sizes are:
>>         /* SMB2_READ */ 49,
>>         /* SMB2_WRITE */ 49,
>>
>> They are used to calculate command header sizes, and fill in the request
>headers. Maybe I should use this variable to get the size?
>
>Nah, those values only specify what values must be set as the structure size in
>the actual packet headers.
>But they do not represent the actual sizes.
>All command headers start with a 2 byte StructureSize field where everything
>except the least significant bit indicates the fixed portion of the response
>header and if the least significant bit is set, then it indicates that the header
>contains an additional variable sized part of header,  (the size of which can be
>determined by looking at the individual fields in the fixed portion of the
>header).
>
>So 49 does not indicate that the header is 49 bytes in size. It means the header
>contains a 48 byte fixed part that is present in every such command headers
>but it also contains 0 or more bytes of additional header.
>
>So see these numbers as "these are the values we must put in the
>StructureSize for the header"  but they do not represent the actual size of the
>header. (well, except if the value is even, then it does represent the header
>size.)
>
>
>So for SMB2_READ and SMB2_WRITE commands, If we do not use RDMA,
>then Channel will be set to 0 and we will not have any ReadWriteChannelInfo
>data.
>In this case the READ/WRITE header size is always 48  (==49 & 0xfffe)
>
>If we are using RDMA, then we still have the 48 byte normal header but we
>also have 1 or more SMB_DIRECT_BUFFER_DESCRIPTOR_V1 structures there.
>So for that case the maximum header size would be 48  + <max number of
>structures> * sizeof(SMB_DIRECT_BUFFER_DESCRIPTOR_V1)

I see, I will send an updated patch.

Thanks,
Long

>
>
>regards
>ronnie sahlberg
>
>
>
>>
>> >
>> >So we should probably add to smb2pdu.h a #define
>> >MAX_SMB2_READWRITE_RESPONSE_SIZE 48 and use this in the
>calculations.
>> >Then we need to add the maximum size we will use for ChannelInfo.
>> >
>> >Maybe we should have a define also for the
>MAX_SMB2_CHANNEL_INFO_SIZE
>> >
>> >regards
>> >ronnie sahlberg
>> >
>> >On Thu, Mar 26, 2020 at 5:31 AM longli--- via samba-technical <samba-
>> >technical@lists.samba.org> wrote:
>> >>
>> >> From: Long Li <longli@microsoft.com>
>> >>
>> >> The packet size needs to take account of SMB2 header size and
>> >> possible encryption header size. This is only done when signing is
>> >> used and it is for RDMA send/receive, not read/write.
>> >>
>> >> Also remove the dead SMBD code in smb2_negotiate_r(w)size.
>> >>
>> >> Signed-off-by: Long Li <longli@microsoft.com>
>> >> ---
>> >>  fs/cifs/smb2ops.c   | 38 ++++++++++++++++----------------------
>> >>  fs/cifs/smbdirect.c |  3 +--
>> >>  2 files changed, 17 insertions(+), 24 deletions(-)
>> >>
>> >> diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c index
>> >> 4c0922596467..9043d34eef43 100644
>> >> --- a/fs/cifs/smb2ops.c
>> >> +++ b/fs/cifs/smb2ops.c
>> >> @@ -332,16 +332,6 @@ smb2_negotiate_wsize(struct cifs_tcon *tcon,
>> >struct smb_vol *volume_info)
>> >>         /* start with specified wsize, or default */
>> >>         wsize = volume_info->wsize ? volume_info->wsize :
>> >CIFS_DEFAULT_IOSIZE;
>> >>         wsize = min_t(unsigned int, wsize, server->max_write);
>> >> -#ifdef CONFIG_CIFS_SMB_DIRECT
>> >> -       if (server->rdma) {
>> >> -               if (server->sign)
>> >> -                       wsize = min_t(unsigned int,
>> >> -                               wsize, server->smbd_conn-
>>max_fragmented_send_size);
>> >> -               else
>> >> -                       wsize = min_t(unsigned int,
>> >> -                               wsize, server->smbd_conn->max_readwrite_size);
>> >> -       }
>> >> -#endif
>> >>         if (!(server->capabilities & SMB2_GLOBAL_CAP_LARGE_MTU))
>> >>                 wsize = min_t(unsigned int, wsize,
>> >> SMB2_MAX_BUFFER_SIZE);
>> >>
>> >> @@ -360,8 +350,15 @@ smb3_negotiate_wsize(struct cifs_tcon *tcon,
>> >> struct smb_vol *volume_info)  #ifdef CONFIG_CIFS_SMB_DIRECT
>> >>         if (server->rdma) {
>> >>                 if (server->sign)
>> >> +                       /*
>> >> +                        * Account for SMB2 data transfer packet header
>> >> +                        * SMB2_READ/SMB2_WRITE (49) and possible encryption
>> >> +                        * headers
>> >> +                        */
>> >>                         wsize = min_t(unsigned int,
>> >> -                               wsize, server->smbd_conn-
>>max_fragmented_send_size);
>> >> +                               wsize,
>> >> +                               server->smbd_conn->max_fragmented_send_size -
>> >> +                                       49 - sizeof(struct
>> >> + smb2_transform_hdr));
>> >>                 else
>> >>                         wsize = min_t(unsigned int,
>> >>                                 wsize,
>> >> server->smbd_conn->max_readwrite_size);
>> >> @@ -382,16 +379,6 @@ smb2_negotiate_rsize(struct cifs_tcon *tcon,
>> >> struct
>> >smb_vol *volume_info)
>> >>         /* start with specified rsize, or default */
>> >>         rsize = volume_info->rsize ? volume_info->rsize :
>CIFS_DEFAULT_IOSIZE;
>> >>         rsize = min_t(unsigned int, rsize, server->max_read);
>> >> -#ifdef CONFIG_CIFS_SMB_DIRECT
>> >> -       if (server->rdma) {
>> >> -               if (server->sign)
>> >> -                       rsize = min_t(unsigned int,
>> >> -                               rsize, server->smbd_conn-
>>max_fragmented_recv_size);
>> >> -               else
>> >> -                       rsize = min_t(unsigned int,
>> >> -                               rsize, server->smbd_conn->max_readwrite_size);
>> >> -       }
>> >> -#endif
>> >>
>> >>         if (!(server->capabilities & SMB2_GLOBAL_CAP_LARGE_MTU))
>> >>                 rsize = min_t(unsigned int, rsize,
>> >> SMB2_MAX_BUFFER_SIZE); @@ -411,8 +398,15 @@
>> >> smb3_negotiate_rsize(struct cifs_tcon *tcon, struct smb_vol
>> >> *volume_info)
>> >#ifdef CONFIG_CIFS_SMB_DIRECT
>> >>         if (server->rdma) {
>> >>                 if (server->sign)
>> >> +                       /*
>> >> +                        * Account for SMB2 data transfer packet header
>> >> +                        * SMB2_READ/SMB2_WRITE (49) and possible encryption
>> >> +                        * headers
>> >> +                        */
>> >>                         rsize = min_t(unsigned int,
>> >> -                               rsize, server->smbd_conn-
>>max_fragmented_recv_size);
>> >> +                               rsize,
>> >> +                               server->smbd_conn->max_fragmented_recv_size -
>> >> +                                       49 - sizeof(struct
>> >> + smb2_transform_hdr));
>> >>                 else
>> >>                         rsize = min_t(unsigned int,
>> >>                                 rsize,
>> >> server->smbd_conn->max_readwrite_size);
>> >> diff --git a/fs/cifs/smbdirect.c b/fs/cifs/smbdirect.c index
>> >> eb1e40af9f3a..0327b575ab87 100644
>> >> --- a/fs/cifs/smbdirect.c
>> >> +++ b/fs/cifs/smbdirect.c
>> >> @@ -2097,8 +2097,7 @@ int smbd_send(struct TCP_Server_Info *server,
>> >>         for (i = 0; i < num_rqst; i++)
>> >>                 remaining_data_length += smb_rqst_len(server,
>> >> &rqst_array[i]);
>> >>
>> >> -       if (remaining_data_length + sizeof(struct smbd_data_transfer) >
>> >> -               info->max_fragmented_send_size) {
>> >> +       if (remaining_data_length > info->max_fragmented_send_size)
>> >> + {
>> >>                 log_write(ERR, "payload size %d > max size %d\n",
>> >>                         remaining_data_length, info->max_fragmented_send_size);
>> >>                 rc = -EINVAL;
>> >> --
>> >> 2.17.1
>> >>
>> >>

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2020-03-26  3:08 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-25 19:30 [PATCH] cifs: smbd: Calculate the correct maximum packet size for segmented SMBDirect send/receive longli
2020-03-26  2:02 ` ronnie sahlberg
2020-03-26  2:43   ` Long Li
2020-03-26  3:03     ` ronnie sahlberg
2020-03-26  3:08       ` Long Li

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).