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

[Steve French <smfrench@gmail.com>]

> 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