On Mon, 26 Jun 2023 11:57:16 +0530
Shyam Prasad N <nspmangalore@xxxxxxxxx> wrote:
> 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.
>
Hi Shyam,
Please see the reply in patch 1.
--
Thanks,
Winston
