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

[Thorsten Leemhuis <regressions@leemhuis.info>]

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