| Message ID | CAH2r5mu3m6FWWqrfOeQugXWGZOPiEE+Xgk8wc0rn8OgLRVPSWQ@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [SMB3] 3 small multichannel client patches | expand |
Reviewed-by: Shyam Prasad N <sprasad@microsoft.com> ... for all three patches. For 0003, logging is okay for now. However, when we have a way to pass custom messages to userspace with new mount API, we should have this message there too. Regards, Shyam On Sat, May 8, 2021 at 6:43 AM Steve French <smfrench@gmail.com> wrote: > > 1) we were not setting CAP_MULTICHANNEL on negotiate request > 2) we were ignoring whether the server set CAP_NEGOTIATE in the response > 3) we were silently ignoring multichannel when "max_channels" was > 1 > but the user forgot to include "multichannel" in mount line. > > > > -- > Thanks, > > Steve
On 5/7/2021 9:13 PM, Steve French wrote: > 1) we were not setting CAP_MULTICHANNEL on negotiate request > diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c > index e36c2a867783..a8bf43184773 100644 > --- a/fs/cifs/smb2pdu.c > +++ b/fs/cifs/smb2pdu.c > @@ -841,6 +841,8 @@ SMB2_negotiate(const unsigned int xid, struct cifs_ses *ses) > req->SecurityMode = 0; > > req->Capabilities = cpu_to_le32(server->vals->req_capabilities); > + if (ses->chan_max > 1) > + req->Capabilities |= cpu_to_le32(SMB2_GLOBAL_CAP_MULTI_CHANNEL); > > /* ClientGUID must be zero for SMB2.02 dialect */ > if (server->vals->protocol_id == SMB20_PROT_ID) > @@ -1032,6 +1034,9 @@ int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon) > > pneg_inbuf->Capabilities = > cpu_to_le32(server->vals->req_capabilities); > + if (tcon->ses->chan_max > 1) > + pneg_inbuf->Capabilities |= cpu_to_le32(SMB2_GLOBAL_CAP_MULTI_CHANNEL); > + This doesn't look quite right, and it can lead to failed negotiate by setting CAP_MULTI_CHANNEL when the server didn't actually send the bit. Have you tested this with servers that don't do multichannel? > 2) we were ignoring whether the server set CAP_NEGOTIATE in the response Is this "CAP_NEGOTIATE" a typo? I think you mean CAP_MULTI_CHANNEL. In any case: > diff --git a/fs/cifs/sess.c b/fs/cifs/sess.c > index 63d517b9f2ff..a391ca3166f3 100644 > --- a/fs/cifs/sess.c > +++ b/fs/cifs/sess.c > @@ -97,6 +97,12 @@ int cifs_try_adding_channels(struct cifs_sb_info *cifs_sb, struct cifs_ses *ses) > return 0; > } > > + if ((ses->server->capabilities & SMB2_GLOBAL_CAP_MULTI_CHANNEL) == false) { This compares a bit to a boolean. "false" should be "0"? > + cifs_dbg(VFS, "server does not support CAP_MULTI_CHANNEL, multichannel disabled\n"); The wording could be clearer. Technically speaking, the server does not support _multichannel_, which it indicated by not setting CAP_MULTI_CHANNEL. Also, wouldn't it be more useful to add the servername to this message? "server %s does not support multichannel, using single channel" or similar. > 3) we were silently ignoring multichannel when "max_channels" was > 1 > but the user forgot to include "multichannel" in mount line. > diff --git a/fs/cifs/fs_context.c b/fs/cifs/fs_context.c > index 3bcf881c3ae9..8f7af6fcdc76 100644 > --- a/fs/cifs/fs_context.c > +++ b/fs/cifs/fs_context.c > @@ -1021,6 +1021,9 @@ static int smb3_fs_context_parse_param(struct fs_context *fc, > goto cifs_parse_mount_err; > } > ctx->max_channels = result.uint_32; > + /* If more than one channel requested ... they want multichan */ > + if ((ctx->multichannel == false) && (result.uint_32 > 1)) > + ctx->multichannel = true; Wouldn't this be clearer and simpler as just "if (result.uint32 > 1)" ?
On Sat, May 8, 2021 at 8:29 AM Tom Talpey <tom@talpey.com> wrote: > > On 5/7/2021 9:13 PM, Steve French wrote: > > 1) we were not setting CAP_MULTICHANNEL on negotiate request > > > diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c > > index e36c2a867783..a8bf43184773 100644 > > --- a/fs/cifs/smb2pdu.c > > +++ b/fs/cifs/smb2pdu.c > > @@ -841,6 +841,8 @@ SMB2_negotiate(const unsigned int xid, struct cifs_ses *ses) > > req->SecurityMode = 0; > > > > req->Capabilities = cpu_to_le32(server->vals->req_capabilities); > > + if (ses->chan_max > 1) > > + req->Capabilities |= cpu_to_le32(SMB2_GLOBAL_CAP_MULTI_CHANNEL); > > > > /* ClientGUID must be zero for SMB2.02 dialect */ > > if (server->vals->protocol_id == SMB20_PROT_ID) > > @@ -1032,6 +1034,9 @@ int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon) > > > > pneg_inbuf->Capabilities = > > cpu_to_le32(server->vals->req_capabilities); > > + if (tcon->ses->chan_max > 1) > > + pneg_inbuf->Capabilities |= cpu_to_le32(SMB2_GLOBAL_CAP_MULTI_CHANNEL); > > + > > This doesn't look quite right, and it can lead to failed negotiate by > setting CAP_MULTI_CHANNEL when the server didn't actually send the bit. > Have you tested this with servers that don't do multichannel? Yes. Validate negotiate ioctl request is supposed to validate what the client sent not what the server responded, so according to MS-SMB2, I must send in the ioctl what I sent before on negprot request Section 3.2.5.5 says for validate negotiate "Capabilities is set to Connection.ClientCapabilities." where "Connection.ClientCapabilities: The capabilities sent by the client in the SMB2 NEGOTIATE Request" (not what the server responded with, what the ClientCapabilities were sent) I tested it with two cases that don't support multichannel: Samba, and also an azure server target where multichannel was disabled. > > > 2) we were ignoring whether the server set CAP_NEGOTIATE in the response > > Is this "CAP_NEGOTIATE" a typo? I think you mean CAP_MULTI_CHANNEL. Yes - typo > > > diff --git a/fs/cifs/sess.c b/fs/cifs/sess.c > > index 63d517b9f2ff..a391ca3166f3 100644 > > --- a/fs/cifs/sess.c > > +++ b/fs/cifs/sess.c > > @@ -97,6 +97,12 @@ int cifs_try_adding_channels(struct cifs_sb_info *cifs_sb, struct cifs_ses *ses) > > return 0; > > } > > > > + if ((ses->server->capabilities & SMB2_GLOBAL_CAP_MULTI_CHANNEL) == false) { > > This compares a bit to a boolean. "false" should be "0"? I changed it to the more common style if (!(ses->...capabilities & SMB@....)) > > > + cifs_dbg(VFS, "server does not support CAP_MULTI_CHANNEL, multichannel disabled\n"); > > The wording could be clearer. Technically speaking, the server does not > support _multichannel_, which it indicated by not setting CAP_MULTI_CHANNEL. > Also, wouldn't it be more useful to add the servername to this message? > "server %s does not support multichannel, using single channel" > or similar. Good idea > > 3) we were silently ignoring multichannel when "max_channels" was > 1 > > but the user forgot to include "multichannel" in mount line. > > > diff --git a/fs/cifs/fs_context.c b/fs/cifs/fs_context.c > > index 3bcf881c3ae9..8f7af6fcdc76 100644 > > --- a/fs/cifs/fs_context.c > > +++ b/fs/cifs/fs_context.c > > @@ -1021,6 +1021,9 @@ static int smb3_fs_context_parse_param(struct > fs_context *fc, > > goto cifs_parse_mount_err; > > } > > ctx->max_channels = result.uint_32; > > + /* If more than one channel requested ... they want multichan */ > > + if ((ctx->multichannel == false) && (result.uint_32 > 1)) > > + ctx->multichannel = true; > > Wouldn't this be clearer and simpler as just "if (result.uint32 > 1)" ? made that change Updated two of the patches as described above - attached.
LGTM Reviewed-By: Tom Talpey <tom@talpey.com> On 5/8/2021 11:10 AM, Steve French wrote: > On Sat, May 8, 2021 at 8:29 AM Tom Talpey <tom@talpey.com> wrote: >> >> On 5/7/2021 9:13 PM, Steve French wrote: >>> 1) we were not setting CAP_MULTICHANNEL on negotiate request >> >>> diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c >>> index e36c2a867783..a8bf43184773 100644 >>> --- a/fs/cifs/smb2pdu.c >>> +++ b/fs/cifs/smb2pdu.c >>> @@ -841,6 +841,8 @@ SMB2_negotiate(const unsigned int xid, struct cifs_ses *ses) >>> req->SecurityMode = 0; >>> >>> req->Capabilities = cpu_to_le32(server->vals->req_capabilities); >>> + if (ses->chan_max > 1) >>> + req->Capabilities |= cpu_to_le32(SMB2_GLOBAL_CAP_MULTI_CHANNEL); >>> >>> /* ClientGUID must be zero for SMB2.02 dialect */ >>> if (server->vals->protocol_id == SMB20_PROT_ID) >>> @@ -1032,6 +1034,9 @@ int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon) >>> >>> pneg_inbuf->Capabilities = >>> cpu_to_le32(server->vals->req_capabilities); >>> + if (tcon->ses->chan_max > 1) >>> + pneg_inbuf->Capabilities |= cpu_to_le32(SMB2_GLOBAL_CAP_MULTI_CHANNEL); >>> + >> >> This doesn't look quite right, and it can lead to failed negotiate by >> setting CAP_MULTI_CHANNEL when the server didn't actually send the bit. >> Have you tested this with servers that don't do multichannel? > > Yes. Validate negotiate ioctl request is supposed to validate what > the client sent not what the server responded, so according to > MS-SMB2, I must send in the ioctl what I sent before on negprot > request > > Section 3.2.5.5 says for validate negotiate "Capabilities is set to > Connection.ClientCapabilities." where > "Connection.ClientCapabilities: The capabilities sent by the client in > the SMB2 NEGOTIATE Request" (not what the server responded with, > what the ClientCapabilities were sent) > > I tested it with two cases that don't support multichannel: Samba, and > also an azure server target where multichannel was disabled. > > >> >>> 2) we were ignoring whether the server set CAP_NEGOTIATE in the response >> >> Is this "CAP_NEGOTIATE" a typo? I think you mean CAP_MULTI_CHANNEL. > > Yes - typo > >> >>> diff --git a/fs/cifs/sess.c b/fs/cifs/sess.c >>> index 63d517b9f2ff..a391ca3166f3 100644 >>> --- a/fs/cifs/sess.c >>> +++ b/fs/cifs/sess.c >>> @@ -97,6 +97,12 @@ int cifs_try_adding_channels(struct cifs_sb_info *cifs_sb, struct cifs_ses *ses) >>> return 0; >>> } >>> >>> + if ((ses->server->capabilities & SMB2_GLOBAL_CAP_MULTI_CHANNEL) == false) { >> >> This compares a bit to a boolean. "false" should be "0"? > > I changed it to the more common style if (!(ses->...capabilities & SMB@....)) >> >>> + cifs_dbg(VFS, "server does not support CAP_MULTI_CHANNEL, multichannel disabled\n"); >> >> The wording could be clearer. Technically speaking, the server does not >> support _multichannel_, which it indicated by not setting CAP_MULTI_CHANNEL. >> Also, wouldn't it be more useful to add the servername to this message? >> "server %s does not support multichannel, using single channel" >> or similar. > > Good idea > >>> 3) we were silently ignoring multichannel when "max_channels" was > 1 >>> but the user forgot to include "multichannel" in mount line. >> >> > diff --git a/fs/cifs/fs_context.c b/fs/cifs/fs_context.c >> > index 3bcf881c3ae9..8f7af6fcdc76 100644 >> > --- a/fs/cifs/fs_context.c >> > +++ b/fs/cifs/fs_context.c >> > @@ -1021,6 +1021,9 @@ static int smb3_fs_context_parse_param(struct >> fs_context *fc, >> > goto cifs_parse_mount_err; >> > } >> > ctx->max_channels = result.uint_32; >> > + /* If more than one channel requested ... they want multichan */ >> > + if ((ctx->multichannel == false) && (result.uint_32 > 1)) >> > + ctx->multichannel = true; >> >> Wouldn't this be clearer and simpler as just "if (result.uint32 > 1)" ? > > made that change > > Updated two of the patches as described above - attached. >
added RB tag and added cc:stable to those two as well On Sat, May 8, 2021 at 10:20 AM Tom Talpey <tom@talpey.com> wrote: > > LGTM > > Reviewed-By: Tom Talpey <tom@talpey.com> > > On 5/8/2021 11:10 AM, Steve French wrote: > > On Sat, May 8, 2021 at 8:29 AM Tom Talpey <tom@talpey.com> wrote: > >> > >> On 5/7/2021 9:13 PM, Steve French wrote: > >>> 1) we were not setting CAP_MULTICHANNEL on negotiate request > >> > >>> diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c > >>> index e36c2a867783..a8bf43184773 100644 > >>> --- a/fs/cifs/smb2pdu.c > >>> +++ b/fs/cifs/smb2pdu.c > >>> @@ -841,6 +841,8 @@ SMB2_negotiate(const unsigned int xid, struct cifs_ses *ses) > >>> req->SecurityMode = 0; > >>> > >>> req->Capabilities = cpu_to_le32(server->vals->req_capabilities); > >>> + if (ses->chan_max > 1) > >>> + req->Capabilities |= cpu_to_le32(SMB2_GLOBAL_CAP_MULTI_CHANNEL); > >>> > >>> /* ClientGUID must be zero for SMB2.02 dialect */ > >>> if (server->vals->protocol_id == SMB20_PROT_ID) > >>> @@ -1032,6 +1034,9 @@ int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon) > >>> > >>> pneg_inbuf->Capabilities = > >>> cpu_to_le32(server->vals->req_capabilities); > >>> + if (tcon->ses->chan_max > 1) > >>> + pneg_inbuf->Capabilities |= cpu_to_le32(SMB2_GLOBAL_CAP_MULTI_CHANNEL); > >>> + > >> > >> This doesn't look quite right, and it can lead to failed negotiate by > >> setting CAP_MULTI_CHANNEL when the server didn't actually send the bit. > >> Have you tested this with servers that don't do multichannel? > > > > Yes. Validate negotiate ioctl request is supposed to validate what > > the client sent not what the server responded, so according to > > MS-SMB2, I must send in the ioctl what I sent before on negprot > > request > > > > Section 3.2.5.5 says for validate negotiate "Capabilities is set to > > Connection.ClientCapabilities." where > > "Connection.ClientCapabilities: The capabilities sent by the client in > > the SMB2 NEGOTIATE Request" (not what the server responded with, > > what the ClientCapabilities were sent) > > > > I tested it with two cases that don't support multichannel: Samba, and > > also an azure server target where multichannel was disabled. > > > > > >> > >>> 2) we were ignoring whether the server set CAP_NEGOTIATE in the response > >> > >> Is this "CAP_NEGOTIATE" a typo? I think you mean CAP_MULTI_CHANNEL. > > > > Yes - typo > > > >> > >>> diff --git a/fs/cifs/sess.c b/fs/cifs/sess.c > >>> index 63d517b9f2ff..a391ca3166f3 100644 > >>> --- a/fs/cifs/sess.c > >>> +++ b/fs/cifs/sess.c > >>> @@ -97,6 +97,12 @@ int cifs_try_adding_channels(struct cifs_sb_info *cifs_sb, struct cifs_ses *ses) > >>> return 0; > >>> } > >>> > >>> + if ((ses->server->capabilities & SMB2_GLOBAL_CAP_MULTI_CHANNEL) == false) { > >> > >> This compares a bit to a boolean. "false" should be "0"? > > > > I changed it to the more common style if (!(ses->...capabilities & SMB@....)) > >> > >>> + cifs_dbg(VFS, "server does not support CAP_MULTI_CHANNEL, multichannel disabled\n"); > >> > >> The wording could be clearer. Technically speaking, the server does not > >> support _multichannel_, which it indicated by not setting CAP_MULTI_CHANNEL. > >> Also, wouldn't it be more useful to add the servername to this message? > >> "server %s does not support multichannel, using single channel" > >> or similar. > > > > Good idea > > > >>> 3) we were silently ignoring multichannel when "max_channels" was > 1 > >>> but the user forgot to include "multichannel" in mount line. > >> > >> > diff --git a/fs/cifs/fs_context.c b/fs/cifs/fs_context.c > >> > index 3bcf881c3ae9..8f7af6fcdc76 100644 > >> > --- a/fs/cifs/fs_context.c > >> > +++ b/fs/cifs/fs_context.c > >> > @@ -1021,6 +1021,9 @@ static int smb3_fs_context_parse_param(struct > >> fs_context *fc, > >> > goto cifs_parse_mount_err; > >> > } > >> > ctx->max_channels = result.uint_32; > >> > + /* If more than one channel requested ... they want multichan */ > >> > + if ((ctx->multichannel == false) && (result.uint_32 > 1)) > >> > + ctx->multichannel = true; > >> > >> Wouldn't this be clearer and simpler as just "if (result.uint32 > 1)" ? > > > > made that change > > > > Updated two of the patches as described above - attached. > >
Steve French <smfrench@gmail.com> writes: > 1) we were not setting CAP_MULTICHANNEL on negotiate request > 2) we were ignoring whether the server set CAP_NEGOTIATE in the response > 3) we were silently ignoring multichannel when "max_channels" was > 1 > but the user forgot to include "multichannel" in mount line. Thanks for those patches, lgtm too. Cheers,
From a2bdd5ceb7090ea4f3ee091da2418f23d270b391 Mon Sep 17 00:00:00 2001 From: Steve French <stfrench@microsoft.com> Date: Fri, 7 May 2021 18:24:11 -0500 Subject: [PATCH 1/4] smb3: when mounting with multichannel include it in requested capabilities In the SMB3/SMB3.1.1 negotiate protocol request, we are supposed to advertise CAP_MULTICHANNEL capability when establishing multiple channels has been requested by the user doing the mount. See MS-SMB2 sections 2.2.3 and 3.2.5.2 Without setting it there is some risk that multichannel could fail if the server interpreted the field strictly. Cc: <stable@vger.kernel.org> # v5.8+ Signed-off-by: Steve French <stfrench@microsoft.com> --- fs/cifs/smb2pdu.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c index e36c2a867783..a8bf43184773 100644 --- a/fs/cifs/smb2pdu.c +++ b/fs/cifs/smb2pdu.c @@ -841,6 +841,8 @@ SMB2_negotiate(const unsigned int xid, struct cifs_ses *ses) req->SecurityMode = 0; req->Capabilities = cpu_to_le32(server->vals->req_capabilities); + if (ses->chan_max > 1) + req->Capabilities |= cpu_to_le32(SMB2_GLOBAL_CAP_MULTI_CHANNEL); /* ClientGUID must be zero for SMB2.02 dialect */ if (server->vals->protocol_id == SMB20_PROT_ID) @@ -1032,6 +1034,9 @@ int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon) pneg_inbuf->Capabilities = cpu_to_le32(server->vals->req_capabilities); + if (tcon->ses->chan_max > 1) + pneg_inbuf->Capabilities |= cpu_to_le32(SMB2_GLOBAL_CAP_MULTI_CHANNEL); + memcpy(pneg_inbuf->Guid, server->client_guid, SMB2_CLIENT_GUID_SIZE); -- 2.27.0