[PATCH][SMB3] fix multiuser mount regression

[PATCH][SMB3] fix multiuser mount regression

[Steve French]

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

cifssmb3: fix incorrect session setup check for multiuser mounts

A recent change to how the SMB3 server (socket) and session status
is managed regressed multiuser mounts by changing the check
for whether session setup is needed to the socket (TCP_Server_info)
structure instead of the session struct (cifs_ses). Add additional
check in cifs_setup_sesion to fix this.

Fixes: 73f9bfbe3d81 ("cifs: maintain a state machine for tcp/smb/tcon sessions")
Reported-by: Ronnie Sahlberg <lsahlber@redhat.com>
Signed-off-by: Steve French <stfrench@microsoft.com>
---
 fs/cifs/connect.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 053cb449eb16..d3020abfe404 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -3924,7 +3924,8 @@ cifs_setup_session(const unsigned int xid,
struct cifs_ses *ses,

  /* only send once per connect */
  spin_lock(&cifs_tcp_ses_lock);
- if (server->tcpStatus != CifsNeedSessSetup) {
+ if ((server->tcpStatus != CifsNeedSessSetup) &&
+     (ses->status == CifsGood)) {
  spin_unlock(&cifs_tcp_ses_lock);
  return 0;
  }

-- 
Thanks,

Steve

[-- Attachment #2: 0001-smb3-fix-incorrect-session-setup-check-for-multiuser.patch --]
[-- Type: text/x-patch, Size: 1294 bytes --]

From 6567400dc936ca8add39be36af34c8722671f8ef Mon Sep 17 00:00:00 2001
From: Steve French <stfrench@microsoft.com>
Date: Wed, 16 Mar 2022 22:08:43 -0500
Subject: [PATCH] smb3: fix incorrect session setup check for multiuser mounts

A recent change to how the SMB3 server (socket) and session status
is managed regressed multiuser mounts by changing the check
for whether session setup is needed to the socket (TCP_Server_info)
structure instead of the session struct (cifs_ses). Add additional
check in cifs_setup_sesion to fix this.

Fixes: 73f9bfbe3d81 ("cifs: maintain a state machine for tcp/smb/tcon sessions")
Reported-by: Ronnie Sahlberg <lsahlber@redhat.com>
Signed-off-by: Steve French <stfrench@microsoft.com>
---
 fs/cifs/connect.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 053cb449eb16..d3020abfe404 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -3924,7 +3924,8 @@ cifs_setup_session(const unsigned int xid, struct cifs_ses *ses,
 
 	/* only send once per connect */
 	spin_lock(&cifs_tcp_ses_lock);
-	if (server->tcpStatus != CifsNeedSessSetup) {
+	if ((server->tcpStatus != CifsNeedSessSetup) &&
+	    (ses->status == CifsGood)) {
 		spin_unlock(&cifs_tcp_ses_lock);
 		return 0;
 	}
-- 
2.32.0

Re: [PATCH][SMB3] fix multiuser mount regression

[ronnie sahlberg]

On Thu, Mar 17, 2022 at 1:20 PM Steve French <smfrench@gmail.com> wrote:
>
> cifssmb3: fix incorrect session setup check for multiuser mounts

If it fixes multiuser then Acked-by me.
We are so close to rc8 that we can not do intrusive changes,   so if
it fixes it short term.
For longer term, post rc8 we need to rewrite the statemaching completely
and separate out "what happens in server->tcpStatus" as one statemachine and
"what happens in server->status" as a separate one. Right now it is a mess.


>
> A recent change to how the SMB3 server (socket) and session status
> is managed regressed multiuser mounts by changing the check
> for whether session setup is needed to the socket (TCP_Server_info)
> structure instead of the session struct (cifs_ses). Add additional
> check in cifs_setup_sesion to fix this.
>
> Fixes: 73f9bfbe3d81 ("cifs: maintain a state machine for tcp/smb/tcon sessions")
> Reported-by: Ronnie Sahlberg <lsahlber@redhat.com>
> Signed-off-by: Steve French <stfrench@microsoft.com>
> ---
>  fs/cifs/connect.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index 053cb449eb16..d3020abfe404 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -3924,7 +3924,8 @@ cifs_setup_session(const unsigned int xid,
> struct cifs_ses *ses,
>
>   /* only send once per connect */
>   spin_lock(&cifs_tcp_ses_lock);
> - if (server->tcpStatus != CifsNeedSessSetup) {
> + if ((server->tcpStatus != CifsNeedSessSetup) &&
> +     (ses->status == CifsGood)) {
>   spin_unlock(&cifs_tcp_ses_lock);
>   return 0;
>   }
>
> --
> Thanks,
>
> Steve

Re: [PATCH][SMB3] fix multiuser mount regression

[Satadru Pramanik]

FYI this patch did not fix mount after resume from suspend.

I still see "CIFS: VFS: cifs_tree_connect: could not find superblock:
-22" errors with the cifs mount unusable after resume.

Regards,

Satadru

On Wed, Mar 16, 2022 at 11:44 PM ronnie sahlberg
<ronniesahlberg@gmail.com> wrote:
>
> On Thu, Mar 17, 2022 at 1:20 PM Steve French <smfrench@gmail.com> wrote:
> >
> > cifssmb3: fix incorrect session setup check for multiuser mounts
>
> If it fixes multiuser then Acked-by me.
> We are so close to rc8 that we can not do intrusive changes,   so if
> it fixes it short term.
> For longer term, post rc8 we need to rewrite the statemaching completely
> and separate out "what happens in server->tcpStatus" as one statemachine and
> "what happens in server->status" as a separate one. Right now it is a mess.
>
>
> >
> > A recent change to how the SMB3 server (socket) and session status
> > is managed regressed multiuser mounts by changing the check
> > for whether session setup is needed to the socket (TCP_Server_info)
> > structure instead of the session struct (cifs_ses). Add additional
> > check in cifs_setup_sesion to fix this.
> >
> > Fixes: 73f9bfbe3d81 ("cifs: maintain a state machine for tcp/smb/tcon sessions")
> > Reported-by: Ronnie Sahlberg <lsahlber@redhat.com>
> > Signed-off-by: Steve French <stfrench@microsoft.com>
> > ---
> >  fs/cifs/connect.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> > index 053cb449eb16..d3020abfe404 100644
> > --- a/fs/cifs/connect.c
> > +++ b/fs/cifs/connect.c
> > @@ -3924,7 +3924,8 @@ cifs_setup_session(const unsigned int xid,
> > struct cifs_ses *ses,
> >
> >   /* only send once per connect */
> >   spin_lock(&cifs_tcp_ses_lock);
> > - if (server->tcpStatus != CifsNeedSessSetup) {
> > + if ((server->tcpStatus != CifsNeedSessSetup) &&
> > +     (ses->status == CifsGood)) {
> >   spin_unlock(&cifs_tcp_ses_lock);
> >   return 0;
> >   }
> >
> > --
> > Thanks,
> >
> > Steve

Re: [PATCH][SMB3] fix multiuser mount regression

[Shyam Prasad N]

I looked at this problem in more detail.

Here's a summary of what happened:
Before my recent changes, when mchan was used, and a
negotiate/sess-setup happened, all other channels were paused. So
things were a lot simpler during a connect/reconnect.
What I did with my recent changes is to allow I/O to flow in other
channels when connect/reconnect happened on one of the channels. This
meant that there could be multiple channels that do negotiate/session
setup for the same session in parallel. i.e. first channel would
create a new session. Other channels would bind to the same session.
This meant that the server->tcpStatus needed to indicate an ongoing
sess setup. So that multiple channels could do session setup in
parallel.
Unfortunately, I did not account for multiuser scenario, which does
the reverse. i.e. uses the same server for different tcp sessions.

Here's a patch I propose to fix this issue. Please review and let me
know what you think.
https://github.com/sprasad-microsoft/smb3-kernel-client/commit/34333e9de1526c46e9ae5ff9a54f0199b827fd0e.patch

Essentially, I'm doing 3 changes in this patch:
1. Making sure that tcpStatus is only till the end of tcp connection
establishment. This means that tcpStatus reaches CifsGood when the tcp
connection is good
2. Once cifs_negotiate_protocol starts, anything done will affect the
session state, and should not modify tcpStatus.
3. To detect the small window between tcp connection setup and before
negotiate, use need_neg()

On Thu, Mar 17, 2022 at 9:14 AM ronnie sahlberg
<ronniesahlberg@gmail.com> wrote:
>
> On Thu, Mar 17, 2022 at 1:20 PM Steve French <smfrench@gmail.com> wrote:
> >
> > cifssmb3: fix incorrect session setup check for multiuser mounts
>
> If it fixes multiuser then Acked-by me.
> We are so close to rc8 that we can not do intrusive changes,   so if
> it fixes it short term.
> For longer term, post rc8 we need to rewrite the statemaching completely
> and separate out "what happens in server->tcpStatus" as one statemachine and
> "what happens in server->status" as a separate one. Right now it is a mess.
>
>
> >
> > A recent change to how the SMB3 server (socket) and session status
> > is managed regressed multiuser mounts by changing the check
> > for whether session setup is needed to the socket (TCP_Server_info)
> > structure instead of the session struct (cifs_ses). Add additional
> > check in cifs_setup_sesion to fix this.
> >
> > Fixes: 73f9bfbe3d81 ("cifs: maintain a state machine for tcp/smb/tcon sessions")
> > Reported-by: Ronnie Sahlberg <lsahlber@redhat.com>
> > Signed-off-by: Steve French <stfrench@microsoft.com>
> > ---
> >  fs/cifs/connect.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> > index 053cb449eb16..d3020abfe404 100644
> > --- a/fs/cifs/connect.c
> > +++ b/fs/cifs/connect.c
> > @@ -3924,7 +3924,8 @@ cifs_setup_session(const unsigned int xid,
> > struct cifs_ses *ses,
> >
> >   /* only send once per connect */
> >   spin_lock(&cifs_tcp_ses_lock);
> > - if (server->tcpStatus != CifsNeedSessSetup) {
> > + if ((server->tcpStatus != CifsNeedSessSetup) &&
> > +     (ses->status == CifsGood)) {
> >   spin_unlock(&cifs_tcp_ses_lock);
> >   return 0;
> >   }
> >
> > --
> > Thanks,
> >
> > Steve



-- 
Regards,
Shyam

Re: [PATCH][SMB3] fix multiuser mount regression

[Thorsten Leemhuis]

On 17.03.22 16:47, Shyam Prasad N wrote:
> I looked at this problem in more detail.

Thx. Is that patch something Satadru should test to see if it fixes his
regression?
https://lore.kernel.org/linux-cifs/CAFrh3J9soC36%2BBVuwHB=g9z_KB5Og2%2Bp2_W%2BBBoBOZveErz14w@mail.gmail.com/

And if it does, out of curiosity: Is the patch considered to be too
invasive for 5.17 as the final is just a few days away?

Ciao, Thorsten

> Here's a summary of what happened:
> Before my recent changes, when mchan was used, and a
> negotiate/sess-setup happened, all other channels were paused. So
> things were a lot simpler during a connect/reconnect.
> What I did with my recent changes is to allow I/O to flow in other
> channels when connect/reconnect happened on one of the channels. This
> meant that there could be multiple channels that do negotiate/session
> setup for the same session in parallel. i.e. first channel would
> create a new session. Other channels would bind to the same session.
> This meant that the server->tcpStatus needed to indicate an ongoing
> sess setup. So that multiple channels could do session setup in
> parallel.
> Unfortunately, I did not account for multiuser scenario, which does
> the reverse. i.e. uses the same server for different tcp sessions.
> 
> Here's a patch I propose to fix this issue. Please review and let me
> know what you think.
> https://github.com/sprasad-microsoft/smb3-kernel-client/commit/34333e9de1526c46e9ae5ff9a54f0199b827fd0e.patch
> 
> Essentially, I'm doing 3 changes in this patch:
> 1. Making sure that tcpStatus is only till the end of tcp connection
> establishment. This means that tcpStatus reaches CifsGood when the tcp
> connection is good
> 2. Once cifs_negotiate_protocol starts, anything done will affect the
> session state, and should not modify tcpStatus.
> 3. To detect the small window between tcp connection setup and before
> negotiate, use need_neg()
> 
> On Thu, Mar 17, 2022 at 9:14 AM ronnie sahlberg
> <ronniesahlberg@gmail.com> wrote:
>>
>> On Thu, Mar 17, 2022 at 1:20 PM Steve French <smfrench@gmail.com> wrote:
>>>
>>> cifssmb3: fix incorrect session setup check for multiuser mounts
>>
>> If it fixes multiuser then Acked-by me.
>> We are so close to rc8 that we can not do intrusive changes,   so if
>> it fixes it short term.
>> For longer term, post rc8 we need to rewrite the statemaching completely
>> and separate out "what happens in server->tcpStatus" as one statemachine and
>> "what happens in server->status" as a separate one. Right now it is a mess.
>>
>>
>>>
>>> A recent change to how the SMB3 server (socket) and session status
>>> is managed regressed multiuser mounts by changing the check
>>> for whether session setup is needed to the socket (TCP_Server_info)
>>> structure instead of the session struct (cifs_ses). Add additional
>>> check in cifs_setup_sesion to fix this.
>>>
>>> Fixes: 73f9bfbe3d81 ("cifs: maintain a state machine for tcp/smb/tcon sessions")
>>> Reported-by: Ronnie Sahlberg <lsahlber@redhat.com>
>>> Signed-off-by: Steve French <stfrench@microsoft.com>
>>> ---
>>>  fs/cifs/connect.c | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
>>> index 053cb449eb16..d3020abfe404 100644
>>> --- a/fs/cifs/connect.c
>>> +++ b/fs/cifs/connect.c
>>> @@ -3924,7 +3924,8 @@ cifs_setup_session(const unsigned int xid,
>>> struct cifs_ses *ses,
>>>
>>>   /* only send once per connect */
>>>   spin_lock(&cifs_tcp_ses_lock);
>>> - if (server->tcpStatus != CifsNeedSessSetup) {
>>> + if ((server->tcpStatus != CifsNeedSessSetup) &&
>>> +     (ses->status == CifsGood)) {
>>>   spin_unlock(&cifs_tcp_ses_lock);
>>>   return 0;
>>>   }
>>>
>>> --
>>> Thanks,
>>>
>>> Steve
> 
> 
> 

Re: [PATCH][SMB3] fix multiuser mount regression

[Steve French]

> And if it does, out of curiosity: Is the patch considered to be too
invasive for 5.17

Good question - I am testing it right now and weighing the option of
the one line patch which fixes at least one of the problems, vs. the
longer patch (about 50 lines of code IIRC) of Shyam's that I am
testing now.    We will defer the larger changes Ronnie, Shyam and I
have discussed to make the state machine more readable/understandable
(and less likely to run into this in the future by changing the state
names in the enum) for the three structures (server socket we are
connected to, authenticated user session, tree connect for the share
we have mounted)

On Thu, Mar 17, 2022 at 11:57 AM Thorsten Leemhuis
<regressions@leemhuis.info> wrote:
>
> On 17.03.22 16:47, Shyam Prasad N wrote:
> > I looked at this problem in more detail.
>
> Thx. Is that patch something Satadru should test to see if it fixes his
> regression?
> https://lore.kernel.org/linux-cifs/CAFrh3J9soC36%2BBVuwHB=g9z_KB5Og2%2Bp2_W%2BBBoBOZveErz14w@mail.gmail.com/
>
> And if it does, out of curiosity: Is the patch considered to be too
> invasive for 5.17 as the final is just a few days away?
>
> Ciao, Thorsten
>
> > Here's a summary of what happened:
> > Before my recent changes, when mchan was used, and a
> > negotiate/sess-setup happened, all other channels were paused. So
> > things were a lot simpler during a connect/reconnect.
> > What I did with my recent changes is to allow I/O to flow in other
> > channels when connect/reconnect happened on one of the channels. This
> > meant that there could be multiple channels that do negotiate/session
> > setup for the same session in parallel. i.e. first channel would
> > create a new session. Other channels would bind to the same session.
> > This meant that the server->tcpStatus needed to indicate an ongoing
> > sess setup. So that multiple channels could do session setup in
> > parallel.
> > Unfortunately, I did not account for multiuser scenario, which does
> > the reverse. i.e. uses the same server for different tcp sessions.
> >
> > Here's a patch I propose to fix this issue. Please review and let me
> > know what you think.
> > https://github.com/sprasad-microsoft/smb3-kernel-client/commit/34333e9de1526c46e9ae5ff9a54f0199b827fd0e.patch
> >
> > Essentially, I'm doing 3 changes in this patch:
> > 1. Making sure that tcpStatus is only till the end of tcp connection
> > establishment. This means that tcpStatus reaches CifsGood when the tcp
> > connection is good
> > 2. Once cifs_negotiate_protocol starts, anything done will affect the
> > session state, and should not modify tcpStatus.
> > 3. To detect the small window between tcp connection setup and before
> > negotiate, use need_neg()
> >
> > On Thu, Mar 17, 2022 at 9:14 AM ronnie sahlberg
> > <ronniesahlberg@gmail.com> wrote:
> >>
> >> On Thu, Mar 17, 2022 at 1:20 PM Steve French <smfrench@gmail.com> wrote:
> >>>
> >>> cifssmb3: fix incorrect session setup check for multiuser mounts
> >>
> >> If it fixes multiuser then Acked-by me.
> >> We are so close to rc8 that we can not do intrusive changes,   so if
> >> it fixes it short term.
> >> For longer term, post rc8 we need to rewrite the statemaching completely
> >> and separate out "what happens in server->tcpStatus" as one statemachine and
> >> "what happens in server->status" as a separate one. Right now it is a mess.
> >>
> >>
> >>>
> >>> A recent change to how the SMB3 server (socket) and session status
> >>> is managed regressed multiuser mounts by changing the check
> >>> for whether session setup is needed to the socket (TCP_Server_info)
> >>> structure instead of the session struct (cifs_ses). Add additional
> >>> check in cifs_setup_sesion to fix this.
> >>>
> >>> Fixes: 73f9bfbe3d81 ("cifs: maintain a state machine for tcp/smb/tcon sessions")
> >>> Reported-by: Ronnie Sahlberg <lsahlber@redhat.com>
> >>> Signed-off-by: Steve French <stfrench@microsoft.com>
> >>> ---
> >>>  fs/cifs/connect.c | 3 ++-
> >>>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> >>> index 053cb449eb16..d3020abfe404 100644
> >>> --- a/fs/cifs/connect.c
> >>> +++ b/fs/cifs/connect.c
> >>> @@ -3924,7 +3924,8 @@ cifs_setup_session(const unsigned int xid,
> >>> struct cifs_ses *ses,
> >>>
> >>>   /* only send once per connect */
> >>>   spin_lock(&cifs_tcp_ses_lock);
> >>> - if (server->tcpStatus != CifsNeedSessSetup) {
> >>> + if ((server->tcpStatus != CifsNeedSessSetup) &&
> >>> +     (ses->status == CifsGood)) {
> >>>   spin_unlock(&cifs_tcp_ses_lock);
> >>>   return 0;
> >>>   }
> >>>
> >>> --
> >>> Thanks,
> >>>
> >>> Steve
> >
> >
> >



-- 
Thanks,

Steve

Re: [PATCH][SMB3] fix multiuser mount regression

[Satadru Pramanik]

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

Neither the one line patch nor the longer patch solve the issue.

Dmesg is attached from booting up with a kernel built with the longer
patch. It covers sleeping, rebooting server, noticing cifs mount is
broken, unmounting and remounting the cifs mount, putting the laptop
to sleep again, rebooting the server, waking the laptop, and noticing
the mount is broken again.


On Thu, Mar 17, 2022 at 1:01 PM Steve French <smfrench@gmail.com> wrote:
>
> > And if it does, out of curiosity: Is the patch considered to be too
> invasive for 5.17
>
> Good question - I am testing it right now and weighing the option of
> the one line patch which fixes at least one of the problems, vs. the
> longer patch (about 50 lines of code IIRC) of Shyam's that I am
> testing now.    We will defer the larger changes Ronnie, Shyam and I
> have discussed to make the state machine more readable/understandable
> (and less likely to run into this in the future by changing the state
> names in the enum) for the three structures (server socket we are
> connected to, authenticated user session, tree connect for the share
> we have mounted)
>
> On Thu, Mar 17, 2022 at 11:57 AM Thorsten Leemhuis
> <regressions@leemhuis.info> wrote:
> >
> > On 17.03.22 16:47, Shyam Prasad N wrote:
> > > I looked at this problem in more detail.
> >
> > Thx. Is that patch something Satadru should test to see if it fixes his
> > regression?
> > https://lore.kernel.org/linux-cifs/CAFrh3J9soC36%2BBVuwHB=g9z_KB5Og2%2Bp2_W%2BBBoBOZveErz14w@mail.gmail.com/
> >
> > And if it does, out of curiosity: Is the patch considered to be too
> > invasive for 5.17 as the final is just a few days away?
> >
> > Ciao, Thorsten
> >
> > > Here's a summary of what happened:
> > > Before my recent changes, when mchan was used, and a
> > > negotiate/sess-setup happened, all other channels were paused. So
> > > things were a lot simpler during a connect/reconnect.
> > > What I did with my recent changes is to allow I/O to flow in other
> > > channels when connect/reconnect happened on one of the channels. This
> > > meant that there could be multiple channels that do negotiate/session
> > > setup for the same session in parallel. i.e. first channel would
> > > create a new session. Other channels would bind to the same session.
> > > This meant that the server->tcpStatus needed to indicate an ongoing
> > > sess setup. So that multiple channels could do session setup in
> > > parallel.
> > > Unfortunately, I did not account for multiuser scenario, which does
> > > the reverse. i.e. uses the same server for different tcp sessions.
> > >
> > > Here's a patch I propose to fix this issue. Please review and let me
> > > know what you think.
> > > https://github.com/sprasad-microsoft/smb3-kernel-client/commit/34333e9de1526c46e9ae5ff9a54f0199b827fd0e.patch
> > >
> > > Essentially, I'm doing 3 changes in this patch:
> > > 1. Making sure that tcpStatus is only till the end of tcp connection
> > > establishment. This means that tcpStatus reaches CifsGood when the tcp
> > > connection is good
> > > 2. Once cifs_negotiate_protocol starts, anything done will affect the
> > > session state, and should not modify tcpStatus.
> > > 3. To detect the small window between tcp connection setup and before
> > > negotiate, use need_neg()
> > >
> > > On Thu, Mar 17, 2022 at 9:14 AM ronnie sahlberg
> > > <ronniesahlberg@gmail.com> wrote:
> > >>
> > >> On Thu, Mar 17, 2022 at 1:20 PM Steve French <smfrench@gmail.com> wrote:
> > >>>
> > >>> cifssmb3: fix incorrect session setup check for multiuser mounts
> > >>
> > >> If it fixes multiuser then Acked-by me.
> > >> We are so close to rc8 that we can not do intrusive changes,   so if
> > >> it fixes it short term.
> > >> For longer term, post rc8 we need to rewrite the statemaching completely
> > >> and separate out "what happens in server->tcpStatus" as one statemachine and
> > >> "what happens in server->status" as a separate one. Right now it is a mess.
> > >>
> > >>
> > >>>
> > >>> A recent change to how the SMB3 server (socket) and session status
> > >>> is managed regressed multiuser mounts by changing the check
> > >>> for whether session setup is needed to the socket (TCP_Server_info)
> > >>> structure instead of the session struct (cifs_ses). Add additional
> > >>> check in cifs_setup_sesion to fix this.
> > >>>
> > >>> Fixes: 73f9bfbe3d81 ("cifs: maintain a state machine for tcp/smb/tcon sessions")
> > >>> Reported-by: Ronnie Sahlberg <lsahlber@redhat.com>
> > >>> Signed-off-by: Steve French <stfrench@microsoft.com>
> > >>> ---
> > >>>  fs/cifs/connect.c | 3 ++-
> > >>>  1 file changed, 2 insertions(+), 1 deletion(-)
> > >>>
> > >>> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> > >>> index 053cb449eb16..d3020abfe404 100644
> > >>> --- a/fs/cifs/connect.c
> > >>> +++ b/fs/cifs/connect.c
> > >>> @@ -3924,7 +3924,8 @@ cifs_setup_session(const unsigned int xid,
> > >>> struct cifs_ses *ses,
> > >>>
> > >>>   /* only send once per connect */
> > >>>   spin_lock(&cifs_tcp_ses_lock);
> > >>> - if (server->tcpStatus != CifsNeedSessSetup) {
> > >>> + if ((server->tcpStatus != CifsNeedSessSetup) &&
> > >>> +     (ses->status == CifsGood)) {
> > >>>   spin_unlock(&cifs_tcp_ses_lock);
> > >>>   return 0;
> > >>>   }
> > >>>
> > >>> --
> > >>> Thanks,
> > >>>
> > >>> Steve
> > >
> > >
> > >
>
>
>
> --
> Thanks,
>
> Steve

[-- Attachment #2: dmesg.txt.gz --]
[-- Type: application/gzip, Size: 35318 bytes --]

Re: [PATCH][SMB3] fix multiuser mount regression

[Shyam Prasad N]

As discussed in the other email thread that Satadru started, I feel
that these two (multiuser regression and the sleep/resume issue) are
different issues.
Let's continue the discussion on that one in the dedicated email thread.

As for the multiuser regression, here's a slightly updated version of
my last patch here.
https://github.com/sprasad-microsoft/smb3-kernel-client/commit/9bc6cde4609c98cf53a5f3e0462c68708dbb54b7.patch

Steve, I leave it upto you whether you want to take for 5.17 or 5.18-rc1.
For 5.18-rc1, I'll have another patch on top of this one that should
fix parallel reconnects for multichannel for good. (I'm planning to
split up session status to individual channels there).

Regards,
Shyam

On Thu, Mar 17, 2022 at 10:59 PM Satadru Pramanik <satadru@gmail.com> wrote:
>
> Neither the one line patch nor the longer patch solve the issue.
>
> Dmesg is attached from booting up with a kernel built with the longer
> patch. It covers sleeping, rebooting server, noticing cifs mount is
> broken, unmounting and remounting the cifs mount, putting the laptop
> to sleep again, rebooting the server, waking the laptop, and noticing
> the mount is broken again.
>
>
> On Thu, Mar 17, 2022 at 1:01 PM Steve French <smfrench@gmail.com> wrote:
> >
> > > And if it does, out of curiosity: Is the patch considered to be too
> > invasive for 5.17
> >
> > Good question - I am testing it right now and weighing the option of
> > the one line patch which fixes at least one of the problems, vs. the
> > longer patch (about 50 lines of code IIRC) of Shyam's that I am
> > testing now.    We will defer the larger changes Ronnie, Shyam and I
> > have discussed to make the state machine more readable/understandable
> > (and less likely to run into this in the future by changing the state
> > names in the enum) for the three structures (server socket we are
> > connected to, authenticated user session, tree connect for the share
> > we have mounted)
> >
> > On Thu, Mar 17, 2022 at 11:57 AM Thorsten Leemhuis
> > <regressions@leemhuis.info> wrote:
> > >
> > > On 17.03.22 16:47, Shyam Prasad N wrote:
> > > > I looked at this problem in more detail.
> > >
> > > Thx. Is that patch something Satadru should test to see if it fixes his
> > > regression?
> > > https://lore.kernel.org/linux-cifs/CAFrh3J9soC36%2BBVuwHB=g9z_KB5Og2%2Bp2_W%2BBBoBOZveErz14w@mail.gmail.com/
> > >
> > > And if it does, out of curiosity: Is the patch considered to be too
> > > invasive for 5.17 as the final is just a few days away?
> > >
> > > Ciao, Thorsten
> > >
> > > > Here's a summary of what happened:
> > > > Before my recent changes, when mchan was used, and a
> > > > negotiate/sess-setup happened, all other channels were paused. So
> > > > things were a lot simpler during a connect/reconnect.
> > > > What I did with my recent changes is to allow I/O to flow in other
> > > > channels when connect/reconnect happened on one of the channels. This
> > > > meant that there could be multiple channels that do negotiate/session
> > > > setup for the same session in parallel. i.e. first channel would
> > > > create a new session. Other channels would bind to the same session.
> > > > This meant that the server->tcpStatus needed to indicate an ongoing
> > > > sess setup. So that multiple channels could do session setup in
> > > > parallel.
> > > > Unfortunately, I did not account for multiuser scenario, which does
> > > > the reverse. i.e. uses the same server for different tcp sessions.
> > > >
> > > > Here's a patch I propose to fix this issue. Please review and let me
> > > > know what you think.
> > > > https://github.com/sprasad-microsoft/smb3-kernel-client/commit/34333e9de1526c46e9ae5ff9a54f0199b827fd0e.patch
> > > >
> > > > Essentially, I'm doing 3 changes in this patch:
> > > > 1. Making sure that tcpStatus is only till the end of tcp connection
> > > > establishment. This means that tcpStatus reaches CifsGood when the tcp
> > > > connection is good
> > > > 2. Once cifs_negotiate_protocol starts, anything done will affect the
> > > > session state, and should not modify tcpStatus.
> > > > 3. To detect the small window between tcp connection setup and before
> > > > negotiate, use need_neg()
> > > >
> > > > On Thu, Mar 17, 2022 at 9:14 AM ronnie sahlberg
> > > > <ronniesahlberg@gmail.com> wrote:
> > > >>
> > > >> On Thu, Mar 17, 2022 at 1:20 PM Steve French <smfrench@gmail.com> wrote:
> > > >>>
> > > >>> cifssmb3: fix incorrect session setup check for multiuser mounts
> > > >>
> > > >> If it fixes multiuser then Acked-by me.
> > > >> We are so close to rc8 that we can not do intrusive changes,   so if
> > > >> it fixes it short term.
> > > >> For longer term, post rc8 we need to rewrite the statemaching completely
> > > >> and separate out "what happens in server->tcpStatus" as one statemachine and
> > > >> "what happens in server->status" as a separate one. Right now it is a mess.
> > > >>
> > > >>
> > > >>>
> > > >>> A recent change to how the SMB3 server (socket) and session status
> > > >>> is managed regressed multiuser mounts by changing the check
> > > >>> for whether session setup is needed to the socket (TCP_Server_info)
> > > >>> structure instead of the session struct (cifs_ses). Add additional
> > > >>> check in cifs_setup_sesion to fix this.
> > > >>>
> > > >>> Fixes: 73f9bfbe3d81 ("cifs: maintain a state machine for tcp/smb/tcon sessions")
> > > >>> Reported-by: Ronnie Sahlberg <lsahlber@redhat.com>
> > > >>> Signed-off-by: Steve French <stfrench@microsoft.com>
> > > >>> ---
> > > >>>  fs/cifs/connect.c | 3 ++-
> > > >>>  1 file changed, 2 insertions(+), 1 deletion(-)
> > > >>>
> > > >>> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> > > >>> index 053cb449eb16..d3020abfe404 100644
> > > >>> --- a/fs/cifs/connect.c
> > > >>> +++ b/fs/cifs/connect.c
> > > >>> @@ -3924,7 +3924,8 @@ cifs_setup_session(const unsigned int xid,
> > > >>> struct cifs_ses *ses,
> > > >>>
> > > >>>   /* only send once per connect */
> > > >>>   spin_lock(&cifs_tcp_ses_lock);
> > > >>> - if (server->tcpStatus != CifsNeedSessSetup) {
> > > >>> + if ((server->tcpStatus != CifsNeedSessSetup) &&
> > > >>> +     (ses->status == CifsGood)) {
> > > >>>   spin_unlock(&cifs_tcp_ses_lock);
> > > >>>   return 0;
> > > >>>   }
> > > >>>
> > > >>> --
> > > >>> Thanks,
> > > >>>
> > > >>> Steve
> > > >
> > > >
> > > >
> >
> >
> >
> > --
> > Thanks,
> >
> > Steve



-- 
Regards,
Shyam