linux-cifs.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH][SMB3] 3 small multichannel client patches
@ 2021-05-08  1:13 Steve French
  2021-05-08 12:30 ` Shyam Prasad N
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Steve French @ 2021-05-08  1:13 UTC (permalink / raw)
  To: Aurélien Aptel, Shyam Prasad N; +Cc: CIFS

[-- Attachment #1: Type: text/plain, Size: 287 bytes --]

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

[-- Attachment #2: 0002-smb3-if-max_channels-set-to-more-than-one-channel-re.patch --]
[-- Type: text/x-patch, Size: 1231 bytes --]

From 16fcf9422d70cb28056518b30c377fe88a7ad7b9 Mon Sep 17 00:00:00 2001
From: Steve French <stfrench@microsoft.com>
Date: Fri, 7 May 2021 19:33:51 -0500
Subject: [PATCH 2/4] smb3: if max_channels set to more than one channel
 request multichannel

Mounting with "multichannel" is obviously implied if user requested
more than one channel on mount (ie mount parm max_channels>1).
Currently both have to be specified. Fix that so that if max_channels
is greater than 1 on mount, enable multichannel rather than silently
falling back to non-multichannel.

Signed-off-by: Steve French <stfrench@microsoft.com>
---
 fs/cifs/fs_context.c | 3 +++
 1 file changed, 3 insertions(+)

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;
 		break;
 	case Opt_handletimeout:
 		ctx->handle_timeout = result.uint_32;
-- 
2.27.0


[-- Attachment #3: 0003-smb3-do-not-attempt-multichannel-to-server-which-doe.patch --]
[-- Type: text/x-patch, Size: 1120 bytes --]

From 9984843c7cb5be498c3fea50e9fa016dc44424ef Mon Sep 17 00:00:00 2001
From: Steve French <stfrench@microsoft.com>
Date: Fri, 7 May 2021 20:00:41 -0500
Subject: [PATCH 3/4] smb3: do not attempt multichannel to server which does
 not support it

We were ignoring CAP_MULTI_CHANNEL in the server response - if the
server doesn't support multichannel we should not be attempting it.

See MS-SMB2 section 3.2.5.2

Signed-off-by: Steve French <stfrench@microsoft.com>
---
 fs/cifs/sess.c | 6 ++++++
 1 file changed, 6 insertions(+)

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) {
+		cifs_dbg(VFS, "server does not support CAP_MULTI_CHANNEL, multichannel disabled\n");
+		ses->chan_max = 1;
+		return 0;
+	}
+
 	/*
 	 * Make a copy of the iface list at the time and use that
 	 * instead so as to not hold the iface spinlock for opening
-- 
2.27.0


[-- Attachment #4: 0001-smb3-when-mounting-with-multichannel-include-it-in-r.patch --]
[-- Type: text/x-patch, Size: 1641 bytes --]

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


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

* Re: [PATCH][SMB3] 3 small multichannel client patches
  2021-05-08  1:13 [PATCH][SMB3] 3 small multichannel client patches Steve French
@ 2021-05-08 12:30 ` Shyam Prasad N
  2021-05-08 13:29 ` Tom Talpey
  2021-05-11 15:53 ` Aurélien Aptel
  2 siblings, 0 replies; 7+ messages in thread
From: Shyam Prasad N @ 2021-05-08 12:30 UTC (permalink / raw)
  To: Steve French; +Cc: Aurélien Aptel, CIFS

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



-- 
Regards,
Shyam

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

* Re: [PATCH][SMB3] 3 small multichannel client patches
  2021-05-08  1:13 [PATCH][SMB3] 3 small multichannel client patches Steve French
  2021-05-08 12:30 ` Shyam Prasad N
@ 2021-05-08 13:29 ` Tom Talpey
  2021-05-08 15:10   ` Steve French
  2021-05-11 15:53 ` Aurélien Aptel
  2 siblings, 1 reply; 7+ messages in thread
From: Tom Talpey @ 2021-05-08 13:29 UTC (permalink / raw)
  To: Steve French, Aurélien Aptel, Shyam Prasad N; +Cc: CIFS

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)" ?

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

* Re: [PATCH][SMB3] 3 small multichannel client patches
  2021-05-08 13:29 ` Tom Talpey
@ 2021-05-08 15:10   ` Steve French
  2021-05-08 15:20     ` Tom Talpey
  0 siblings, 1 reply; 7+ messages in thread
From: Steve French @ 2021-05-08 15:10 UTC (permalink / raw)
  To: Tom Talpey; +Cc: Aurélien Aptel, Shyam Prasad N, CIFS

[-- Attachment #1: Type: text/plain, Size: 3993 bytes --]

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.
-- 
Thanks,

Steve

[-- Attachment #2: 0003-smb3-if-max_channels-set-to-more-than-one-channel-re.patch --]
[-- Type: text/x-patch, Size: 1249 bytes --]

From 1fae9cf8242f7d7028fa95f1cfd24b67942b8b4e Mon Sep 17 00:00:00 2001
From: Steve French <stfrench@microsoft.com>
Date: Fri, 7 May 2021 19:33:51 -0500
Subject: [PATCH 3/4] smb3: if max_channels set to more than one channel
 request multichannel

Mounting with "multichannel" is obviously implied if user requested
more than one channel on mount (ie mount parm max_channels>1).
Currently both have to be specified. Fix that so that if max_channels
is greater than 1 on mount, enable multichannel rather than silently
falling back to non-multichannel.

Signed-off-by: Steve French <stfrench@microsoft.com>
Reviewed-by: Shyam Prasad N <sprasad@microsoft.com>
---
 fs/cifs/fs_context.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/cifs/fs_context.c b/fs/cifs/fs_context.c
index 3bcf881c3ae9..5d21cd905315 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 (result.uint_32 > 1)
+			ctx->multichannel = true;
 		break;
 	case Opt_handletimeout:
 		ctx->handle_timeout = result.uint_32;
-- 
2.27.0


[-- Attachment #3: 0002-smb3-do-not-attempt-multichannel-to-server-which-doe.patch --]
[-- Type: text/x-patch, Size: 1162 bytes --]

From f2421e5efcc25e1f7a5661d0ace059c1ddaf4b8d Mon Sep 17 00:00:00 2001
From: Steve French <stfrench@microsoft.com>
Date: Fri, 7 May 2021 20:00:41 -0500
Subject: [PATCH 2/4] smb3: do not attempt multichannel to server which does
 not support it

We were ignoring CAP_MULTI_CHANNEL in the server response - if the
server doesn't support multichannel we should not be attempting it.

See MS-SMB2 section 3.2.5.2

Reviewed-by: Shyam Prasad N <sprasad@microsoft.com>
Signed-off-by: Steve French <stfrench@microsoft.com>
---
 fs/cifs/sess.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/fs/cifs/sess.c b/fs/cifs/sess.c
index 63d517b9f2ff..a92a1fb7cb52 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)) {
+		cifs_dbg(VFS, "server %s does not support multichannel\n", ses->server->hostname);
+		ses->chan_max = 1;
+		return 0;
+	}
+
 	/*
 	 * Make a copy of the iface list at the time and use that
 	 * instead so as to not hold the iface spinlock for opening
-- 
2.27.0


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

* Re: [PATCH][SMB3] 3 small multichannel client patches
  2021-05-08 15:10   ` Steve French
@ 2021-05-08 15:20     ` Tom Talpey
  2021-05-08 15:51       ` Steve French
  0 siblings, 1 reply; 7+ messages in thread
From: Tom Talpey @ 2021-05-08 15:20 UTC (permalink / raw)
  To: Steve French; +Cc: Aurélien Aptel, Shyam Prasad N, CIFS

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.
> 

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

* Re: [PATCH][SMB3] 3 small multichannel client patches
  2021-05-08 15:20     ` Tom Talpey
@ 2021-05-08 15:51       ` Steve French
  0 siblings, 0 replies; 7+ messages in thread
From: Steve French @ 2021-05-08 15:51 UTC (permalink / raw)
  To: Tom Talpey; +Cc: Aurélien Aptel, Shyam Prasad N, CIFS

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.
> >



-- 
Thanks,

Steve

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

* Re: [PATCH][SMB3] 3 small multichannel client patches
  2021-05-08  1:13 [PATCH][SMB3] 3 small multichannel client patches Steve French
  2021-05-08 12:30 ` Shyam Prasad N
  2021-05-08 13:29 ` Tom Talpey
@ 2021-05-11 15:53 ` Aurélien Aptel
  2 siblings, 0 replies; 7+ messages in thread
From: Aurélien Aptel @ 2021-05-11 15:53 UTC (permalink / raw)
  To: Steve French, Shyam Prasad N; +Cc: CIFS

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,
-- 
Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München)


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

end of thread, other threads:[~2021-05-11 15:53 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-08  1:13 [PATCH][SMB3] 3 small multichannel client patches Steve French
2021-05-08 12:30 ` Shyam Prasad N
2021-05-08 13:29 ` Tom Talpey
2021-05-08 15:10   ` Steve French
2021-05-08 15:20     ` Tom Talpey
2021-05-08 15:51       ` Steve French
2021-05-11 15:53 ` Aurélien Aptel

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