On Mon, Jun 26, 2023 at 11:28 AM Winston Wen <wentao@xxxxxxxxxxxxx> wrote:
>
> On Mon, 26 Jun 2023 10:57:32 +0530
> Shyam Prasad N <nspmangalore@xxxxxxxxx> wrote:
>
> > On Mon, Jun 26, 2023 at 9:24 AM Winston Wen <wentao@xxxxxxxxxxxxx>
> > wrote:
> > >
> > > Don't collect exiting session in smb2_reconnect_server(), because it
> > > will be released soon.
> > >
> > > Note that the exiting session will stay in server->smb_ses_list
> > > until it complete the cifs_free_ipc() and logoff() and then delete
> > > itself from the list.
> > >
> > > Signed-off-by: Winston Wen <wentao@xxxxxxxxxxxxx>
> > > ---
> > > fs/smb/client/smb2pdu.c | 6 ++++++
> > > 1 file changed, 6 insertions(+)
> > >
> > > diff --git a/fs/smb/client/smb2pdu.c b/fs/smb/client/smb2pdu.c
> > > index 17fe212ab895..e04766fe6f80 100644
> > > --- a/fs/smb/client/smb2pdu.c
> > > +++ b/fs/smb/client/smb2pdu.c
> > > @@ -3797,6 +3797,12 @@ void smb2_reconnect_server(struct
> > > work_struct *work)
> > >
> > > spin_lock(&cifs_tcp_ses_lock);
> > > list_for_each_entry(ses, &pserver->smb_ses_list,
> > > smb_ses_list) {
> > > + spin_lock(&ses->ses_lock);
> > > + if (ses->ses_status == SES_EXITING) {
> > > + spin_unlock(&ses->ses_lock);
> > > + continue;
> > > + }
> > > + spin_unlock(&ses->ses_lock);
> > >
> > > tcon_selected = false;
> > >
> > > --
> > > 2.40.1
> > >
> >
> > Hi Winston,
> >
> > We already have this check in smb2_reconnect, which gets called from
> > smb2_reconnect_server.
> > But one additional check here will not hurt.
> >
> > Reviewed-by: Shyam Prasad N <sprasad@xxxxxxxxxxxxx>
> >
>
> Hi Shyam,
>
> Thanks for the review! And sorry for my mistake that when I replied a
> minute ago I forgot to cc others...
>
> I think the check in smb2_reconnect is not enough for this situation,
> but maybe I missed something...
>
> Consider the following process:
>
> smb2_reconnect_server():
> spin_lock(&cifs_tcp_ses_lock)
> list_for_each_entry(ses, ...)
> ...
> if (ses->tcon_ipc && ses->tcon_ipc->need_reconnect)
> cifs_smb_ses_inc_refcount(ses)
> spin_unlock(&cifs_tcp_ses_lock)
>
> /* -> session may have been released before smb2_reconnect */
> list_for_each_entry_safe(tcon, tcon2, &tmp_list, rlist)
> smb2_reconnect()
> list_del_init(&tcon->rlist)
> if (tcon->ipc)
> cifs_put_smb_ses(tcon->ses)
> else
> cifs_put_tcon(tcon)
>
> When we do smb2_reconnect(), the session may have been released, and all
> the access to its field in smb2_reconnect(), such as ses->status or
> ses->ses_lock, is illegal. And when we call the cifs_put_smb_ses() on it
> again, it will crash...
I see what you mean.
I think __cifs_put_smb_ses is at fault here.
Once the ses_count reaches 0, it should do all the following before it
drops cifs_tcp_ses_lock:
1. Mark as SES_EXITING
2. Remove the session from it's list.
That way, smb2_reconnect_server should not even be able to find the
session in the list.
--
Regards,
Shyam
