* [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).