Re: [PATCH] cifs: smbd: Calculate the correct maximum packet size for segmented SMBDirect send/receive
From: ronnie sahlberg
Date: Wed Mar 25 2020 - 23:04:05 EST
On Thu, Mar 26, 2020 at 12:43 PM Long Li <longli@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx> wrote:
> >>
> >> From: Long Li <longli@xxxxxxxxxxxxx>
> >>
> >> 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@xxxxxxxxxxxxx>
> >> ---
> >> 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
> >>
> >>