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

[Shyam Prasad N <nspmangalore@gmail.com>]

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