[CIFS] Fix match_server check to allow for multidialect negotiate
diff mbox series

Message ID CAH2r5mvRQj1hyDbBY8DTMtDShr2uxmJqYWWJg+H=iO3RUDc3oQ@mail.gmail.com
State New
Headers show
Series
  • [CIFS] Fix match_server check to allow for multidialect negotiate
Related show

Commit Message

Steve French June 8, 2019, 9:05 a.m. UTC
When using multidialect negotiate (default or allowing any smb3
dialect via vers=3)
allow matching on existing server session.  Before this fix if you mount
a second time to a different share on the same server, we will only reuse
the existing smb session if a single dialect is requested (e.g. specifying
vers=2.1 or vers=3.0 or vers=3.1.1 on the mount command). If a default mount
(e.g. not specifying vers=) is done then we will create a new socket
connection and SMB3 (or SMB3.1.1) session each time we connect to a
different share
on the same server rather than reusing the existing one.

Signed-off-by: Steve French <stfrench@microsoft.com>
---
 fs/cifs/connect.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

     if (!net_eq(cifs_net_ns(server), current->nsproxy->net_ns))

Comments

Pavel Shilovsky June 10, 2019, 6:41 p.m. UTC | #1
сб, 8 июн. 2019 г. в 02:06, Steve French <smfrench@gmail.com>:
>
> When using multidialect negotiate (default or allowing any smb3
> dialect via vers=3)
> allow matching on existing server session.  Before this fix if you mount
> a second time to a different share on the same server, we will only reuse
> the existing smb session if a single dialect is requested (e.g. specifying
> vers=2.1 or vers=3.0 or vers=3.1.1 on the mount command). If a default mount
> (e.g. not specifying vers=) is done then we will create a new socket
> connection and SMB3 (or SMB3.1.1) session each time we connect to a
> different share
> on the same server rather than reusing the existing one.
>
> Signed-off-by: Steve French <stfrench@microsoft.com>
> ---
>  fs/cifs/connect.c | 21 +++++++++++++++++++--
>  1 file changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index 8c4121da624e..6200207565db 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -2542,8 +2542,25 @@ static int match_server(struct TCP_Server_Info
> *server, struct smb_vol *vol)
>      if (vol->nosharesock)
>          return 0;
>
> -    /* BB update this for smb3any and default case */
> -    if ((server->vals != vol->vals) || (server->ops != vol->ops))
> +    /* If multidialect negotiation see if existing sessions match one */
> +    if (strcmp(vol->vals->version_string, SMB3ANY_VERSION_STRING) == 0) {
> +        if (server->vals->protocol_id == SMB20_PROT_ID)
> +            return 0;
> +        else if (server->vals->protocol_id == SMB21_PROT_ID)
> +            return 0;
> +        else if (strcmp(server->vals->version_string,
> +             SMB1_VERSION_STRING) == 0)
> +            return 0;
> +        /* else SMB3 or later, which is fine */

May be better to check

if (server->vals->protocol_id < SMB30_PROT_ID)
    return 0;

? SMB1 case should work too because protocol_id is 0.


> +    } else if (strcmp(vol->vals->version_string,
> +           SMBDEFAULT_VERSION_STRING) == 0) {
> +        if (server->vals->protocol_id == SMB20_PROT_ID)
> +            return 0;
> +        else if (strcmp(server->vals->version_string,
> +             SMB1_VERSION_STRING) == 0)
> +            return 0;

and here the same way:

if (server->vals->protocol_id < SMB21_PROT_ID)
    return 0;

> +        /* else SMB2.1 or later, which is fine */
> +    } else if ((server->vals != vol->vals) || (server->ops != vol->ops))
>          return 0;
>
>      if (!net_eq(cifs_net_ns(server), current->nsproxy->net_ns))
> --
> 2.20.1

In this case we can avoid nested IFs - should be cleaner I guess.


--
Best regards,
Pavel Shilovsky
Steve French June 13, 2019, 7:32 p.m. UTC | #2
Updated version with your suggestion - see attached

On Mon, Jun 10, 2019 at 1:41 PM Pavel Shilovsky <piastryyy@gmail.com> wrote:
>
> сб, 8 июн. 2019 г. в 02:06, Steve French <smfrench@gmail.com>:
> >
> > When using multidialect negotiate (default or allowing any smb3
> > dialect via vers=3)
> > allow matching on existing server session.  Before this fix if you mount
> > a second time to a different share on the same server, we will only reuse
> > the existing smb session if a single dialect is requested (e.g. specifying
> > vers=2.1 or vers=3.0 or vers=3.1.1 on the mount command). If a default mount
> > (e.g. not specifying vers=) is done then we will create a new socket
> > connection and SMB3 (or SMB3.1.1) session each time we connect to a
> > different share
> > on the same server rather than reusing the existing one.
> >
> > Signed-off-by: Steve French <stfrench@microsoft.com>
> > ---
> >  fs/cifs/connect.c | 21 +++++++++++++++++++--
> >  1 file changed, 19 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> > index 8c4121da624e..6200207565db 100644
> > --- a/fs/cifs/connect.c
> > +++ b/fs/cifs/connect.c
> > @@ -2542,8 +2542,25 @@ static int match_server(struct TCP_Server_Info
> > *server, struct smb_vol *vol)
> >      if (vol->nosharesock)
> >          return 0;
> >
> > -    /* BB update this for smb3any and default case */
> > -    if ((server->vals != vol->vals) || (server->ops != vol->ops))
> > +    /* If multidialect negotiation see if existing sessions match one */
> > +    if (strcmp(vol->vals->version_string, SMB3ANY_VERSION_STRING) == 0) {
> > +        if (server->vals->protocol_id == SMB20_PROT_ID)
> > +            return 0;
> > +        else if (server->vals->protocol_id == SMB21_PROT_ID)
> > +            return 0;
> > +        else if (strcmp(server->vals->version_string,
> > +             SMB1_VERSION_STRING) == 0)
> > +            return 0;
> > +        /* else SMB3 or later, which is fine */
>
> May be better to check
>
> if (server->vals->protocol_id < SMB30_PROT_ID)
>     return 0;
>
> ? SMB1 case should work too because protocol_id is 0.
>
>
> > +    } else if (strcmp(vol->vals->version_string,
> > +           SMBDEFAULT_VERSION_STRING) == 0) {
> > +        if (server->vals->protocol_id == SMB20_PROT_ID)
> > +            return 0;
> > +        else if (strcmp(server->vals->version_string,
> > +             SMB1_VERSION_STRING) == 0)
> > +            return 0;
>
> and here the same way:
>
> if (server->vals->protocol_id < SMB21_PROT_ID)
>     return 0;
>
> > +        /* else SMB2.1 or later, which is fine */
> > +    } else if ((server->vals != vol->vals) || (server->ops != vol->ops))
> >          return 0;
> >
> >      if (!net_eq(cifs_net_ns(server), current->nsproxy->net_ns))
> > --
> > 2.20.1
>
> In this case we can avoid nested IFs - should be cleaner I guess.
>
>
> --
> Best regards,
> Pavel Shilovsky
Steve French June 13, 2019, 7:34 p.m. UTC | #3
Since this patch - I also fixed the misspelled word 'specifying'

On Thu, Jun 13, 2019 at 2:32 PM Steve French <smfrench@gmail.com> wrote:
>
> Updated version with your suggestion - see attached
>
> On Mon, Jun 10, 2019 at 1:41 PM Pavel Shilovsky <piastryyy@gmail.com> wrote:
> >
> > сб, 8 июн. 2019 г. в 02:06, Steve French <smfrench@gmail.com>:
> > >
> > > When using multidialect negotiate (default or allowing any smb3
> > > dialect via vers=3)
> > > allow matching on existing server session.  Before this fix if you mount
> > > a second time to a different share on the same server, we will only reuse
> > > the existing smb session if a single dialect is requested (e.g. specifying
> > > vers=2.1 or vers=3.0 or vers=3.1.1 on the mount command). If a default mount
> > > (e.g. not specifying vers=) is done then we will create a new socket
> > > connection and SMB3 (or SMB3.1.1) session each time we connect to a
> > > different share
> > > on the same server rather than reusing the existing one.
> > >
> > > Signed-off-by: Steve French <stfrench@microsoft.com>
> > > ---
> > >  fs/cifs/connect.c | 21 +++++++++++++++++++--
> > >  1 file changed, 19 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> > > index 8c4121da624e..6200207565db 100644
> > > --- a/fs/cifs/connect.c
> > > +++ b/fs/cifs/connect.c
> > > @@ -2542,8 +2542,25 @@ static int match_server(struct TCP_Server_Info
> > > *server, struct smb_vol *vol)
> > >      if (vol->nosharesock)
> > >          return 0;
> > >
> > > -    /* BB update this for smb3any and default case */
> > > -    if ((server->vals != vol->vals) || (server->ops != vol->ops))
> > > +    /* If multidialect negotiation see if existing sessions match one */
> > > +    if (strcmp(vol->vals->version_string, SMB3ANY_VERSION_STRING) == 0) {
> > > +        if (server->vals->protocol_id == SMB20_PROT_ID)
> > > +            return 0;
> > > +        else if (server->vals->protocol_id == SMB21_PROT_ID)
> > > +            return 0;
> > > +        else if (strcmp(server->vals->version_string,
> > > +             SMB1_VERSION_STRING) == 0)
> > > +            return 0;
> > > +        /* else SMB3 or later, which is fine */
> >
> > May be better to check
> >
> > if (server->vals->protocol_id < SMB30_PROT_ID)
> >     return 0;
> >
> > ? SMB1 case should work too because protocol_id is 0.
> >
> >
> > > +    } else if (strcmp(vol->vals->version_string,
> > > +           SMBDEFAULT_VERSION_STRING) == 0) {
> > > +        if (server->vals->protocol_id == SMB20_PROT_ID)
> > > +            return 0;
> > > +        else if (strcmp(server->vals->version_string,
> > > +             SMB1_VERSION_STRING) == 0)
> > > +            return 0;
> >
> > and here the same way:
> >
> > if (server->vals->protocol_id < SMB21_PROT_ID)
> >     return 0;
> >
> > > +        /* else SMB2.1 or later, which is fine */
> > > +    } else if ((server->vals != vol->vals) || (server->ops != vol->ops))
> > >          return 0;
> > >
> > >      if (!net_eq(cifs_net_ns(server), current->nsproxy->net_ns))
> > > --
> > > 2.20.1
> >
> > In this case we can avoid nested IFs - should be cleaner I guess.
> >
> >
> > --
> > Best regards,
> > Pavel Shilovsky
>
>
>
> --
> Thanks,
>
> Steve
Pavel Shilovsky June 13, 2019, 11:33 p.m. UTC | #4
чт, 13 июн. 2019 г. в 12:34, Steve French <smfrench@gmail.com>:
>
> Since this patch - I also fixed the misspelled word 'specifying'
>
> On Thu, Jun 13, 2019 at 2:32 PM Steve French <smfrench@gmail.com> wrote:
> >
> > Updated version with your suggestion - see attached
> >
> > On Mon, Jun 10, 2019 at 1:41 PM Pavel Shilovsky <piastryyy@gmail.com> wrote:
> > >
> > > сб, 8 июн. 2019 г. в 02:06, Steve French <smfrench@gmail.com>:
> > > >
> > > > When using multidialect negotiate (default or allowing any smb3
> > > > dialect via vers=3)
> > > > allow matching on existing server session.  Before this fix if you mount
> > > > a second time to a different share on the same server, we will only reuse
> > > > the existing smb session if a single dialect is requested (e.g. specifying
> > > > vers=2.1 or vers=3.0 or vers=3.1.1 on the mount command). If a default mount
> > > > (e.g. not specifying vers=) is done then we will create a new socket
> > > > connection and SMB3 (or SMB3.1.1) session each time we connect to a
> > > > different share
> > > > on the same server rather than reusing the existing one.
> > > >
> > > > Signed-off-by: Steve French <stfrench@microsoft.com>
> > > > ---
> > > >  fs/cifs/connect.c | 21 +++++++++++++++++++--
> > > >  1 file changed, 19 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> > > > index 8c4121da624e..6200207565db 100644
> > > > --- a/fs/cifs/connect.c
> > > > +++ b/fs/cifs/connect.c
> > > > @@ -2542,8 +2542,25 @@ static int match_server(struct TCP_Server_Info
> > > > *server, struct smb_vol *vol)
> > > >      if (vol->nosharesock)
> > > >          return 0;
> > > >
> > > > -    /* BB update this for smb3any and default case */
> > > > -    if ((server->vals != vol->vals) || (server->ops != vol->ops))
> > > > +    /* If multidialect negotiation see if existing sessions match one */
> > > > +    if (strcmp(vol->vals->version_string, SMB3ANY_VERSION_STRING) == 0) {
> > > > +        if (server->vals->protocol_id == SMB20_PROT_ID)
> > > > +            return 0;
> > > > +        else if (server->vals->protocol_id == SMB21_PROT_ID)
> > > > +            return 0;
> > > > +        else if (strcmp(server->vals->version_string,
> > > > +             SMB1_VERSION_STRING) == 0)
> > > > +            return 0;
> > > > +        /* else SMB3 or later, which is fine */
> > >
> > > May be better to check
> > >
> > > if (server->vals->protocol_id < SMB30_PROT_ID)
> > >     return 0;
> > >
> > > ? SMB1 case should work too because protocol_id is 0.
> > >
> > >
> > > > +    } else if (strcmp(vol->vals->version_string,
> > > > +           SMBDEFAULT_VERSION_STRING) == 0) {
> > > > +        if (server->vals->protocol_id == SMB20_PROT_ID)
> > > > +            return 0;
> > > > +        else if (strcmp(server->vals->version_string,
> > > > +             SMB1_VERSION_STRING) == 0)
> > > > +            return 0;
> > >
> > > and here the same way:
> > >
> > > if (server->vals->protocol_id < SMB21_PROT_ID)
> > >     return 0;
> > >
> > > > +        /* else SMB2.1 or later, which is fine */
> > > > +    } else if ((server->vals != vol->vals) || (server->ops != vol->ops))
> > > >          return 0;
> > > >
> > > >      if (!net_eq(cifs_net_ns(server), current->nsproxy->net_ns))
> > > > --
> > > > 2.20.1
> > >
> > > In this case we can avoid nested IFs - should be cleaner I guess.
> > >
> > >
> > > --
> > > Best regards,
> > > Pavel Shilovsky
> >
> >
> >
> > --
> > Thanks,
> >
> > Steve
>
>
>
> --
> Thanks,
>
> Steve

Looks good.

Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com>

--
Best regards,
Pavel Shilovsky

Patch
diff mbox series

From fd5e01a89742bb85d758a6651294a8a20193bc27 Mon Sep 17 00:00:00 2001
From: Steve French <stfrench@microsoft.com>
Date: Sat, 8 Jun 2019 03:56:29 -0500
Subject: [PATCH] [CIFS] Fix match_server check to allow for multidialect
 negotiate

When using multidialect negotiate (default or allowing any smb3 dialect via vers=3)
allow matching on existing server session.  Before this fix if you mount
a second time to a different share on the same server, we will only reuse
the existing smb session if a single dialect is requested (e.g. specifying
vers=2.1 or vers=3.0 or vers=3.1.1 on the mount command). If a default mount
(e.g. not specifying vers=) is done then we will create a new socket
connection and SMB3 (or SMB3.1.1) session each time we connect to a different share
on the same server rather than reusing the existing one.

Signed-off-by: Steve French <stfrench@microsoft.com>
---
 fs/cifs/connect.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 8c4121da624e..6200207565db 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -2542,8 +2542,25 @@  static int match_server(struct TCP_Server_Info *server, struct smb_vol *vol)
 	if (vol->nosharesock)
 		return 0;
 
-	/* BB update this for smb3any and default case */
-	if ((server->vals != vol->vals) || (server->ops != vol->ops))
+	/* If multidialect negotiation see if existing sessions match one */
+	if (strcmp(vol->vals->version_string, SMB3ANY_VERSION_STRING) == 0) {
+		if (server->vals->protocol_id == SMB20_PROT_ID)
+			return 0;
+		else if (server->vals->protocol_id == SMB21_PROT_ID)
+			return 0;
+		else if (strcmp(server->vals->version_string,
+			 SMB1_VERSION_STRING) == 0)
+			return 0;
+		/* else SMB3 or later, which is fine */
+	} else if (strcmp(vol->vals->version_string,
+		   SMBDEFAULT_VERSION_STRING) == 0) {
+		if (server->vals->protocol_id == SMB20_PROT_ID)
+			return 0;
+		else if (strcmp(server->vals->version_string,
+			 SMB1_VERSION_STRING) == 0)
+			return 0;
+		/* else SMB2.1 or later, which is fine */
+	} else if ((server->vals != vol->vals) || (server->ops != vol->ops))
 		return 0;
 
 	if (!net_eq(cifs_net_ns(server), current->nsproxy->net_ns))
-- 
2.20.1