Re: [PATCH 2/4] smb: client: Protect ses->chans update with chan_lock spin lock

[Pierre Mariani <pierre.mariani@gmail.com>]

On Wed, Nov 29, 2023 at 02:30:37PM +0530, Shyam Prasad N wrote:
> On Mon, Nov 27, 2023 at 10:22 AM Pierre Mariani
> <pierre.mariani@gmail.com> wrote:
> >
> > Protect the update of ses->chans with chan_lock spin lock as per documentation
> > from cifsglob.h.
> > Fixes Coverity 1561738.
> >
> > Signed-off-by: Pierre Mariani <pierre.mariani@gmail.com>
> > ---
> >  fs/smb/client/connect.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
> > index 449d56802692..0512835f399c 100644
> > --- a/fs/smb/client/connect.c
> > +++ b/fs/smb/client/connect.c
> > @@ -2055,6 +2055,7 @@ void __cifs_put_smb_ses(struct cifs_ses *ses)
> >         spin_unlock(&cifs_tcp_ses_lock);
> >
> >         /* close any extra channels */
> > +       spin_lock(&ses->chan_lock);
> >         for (i = 1; i < ses->chan_count; i++) {
> >                 if (ses->chans[i].iface) {
> >                         kref_put(&ses->chans[i].iface->refcount, release_iface);
> > @@ -2063,11 +2064,14 @@ void __cifs_put_smb_ses(struct cifs_ses *ses)
> >                 cifs_put_tcp_session(ses->chans[i].server, 0);
> >                 ses->chans[i].server = NULL;
> >         }
> > +       spin_unlock(&ses->chan_lock);
> >
> >         /* we now account for primary channel in iface->refcount */
> >         if (ses->chans[0].iface) {
> >                 kref_put(&ses->chans[0].iface->refcount, release_iface);
> > +               spin_lock(&ses->chan_lock);
> >                 ses->chans[0].server = NULL;
> > +               spin_unlock(&ses->chan_lock);
> >         }
> >
> >         sesInfoFree(ses);
> > --
> > 2.39.2
> >
> >
> 
> Hi Pierre,
> 
> Thanks for proposing this change.
> 
> While it is true in general that chan_lock needs to be locked when
> dealing with session channel details, this particular instance above
> is during __cifs_put_smb_ses.
> And this code is reached when ses_count has already reached 0. i.e.
> this process is the last user of the session.
> So taking chan_lock can be avoided. We did have this under a lock
> before, but it resulted in deadlocks due to calls to
> cifs_put_tcp_session, which locks bigger locks.
> So the quick and dirty fix at that point was to not take chan_lock
> here, knowing that we'll be the last user.
> 
> Perhaps a better fix exists?
> Or we should probably document this as a comment for now.
> 
> This version of the patch will result in the deadlocks again.

Thank you for educating me on this, Shyam. I will re-read the code from that
point of view and see if I can think of any improvement.

> 
> -- 
> Regards,
> Shyam