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 04:30:54PM -0800, Pierre Mariani wrote:
> 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.
> 

Looking at the code in more details, I can see how the order of relevant locks
is cifs_tcp_ses_lock > srv_lock > chan_lock.
cifs_tcp_ses_lock and srv_lock are locked by cifs_put_tcp_session.
__cifs_put_smb_ses calls cifs_put_tcp_session. Hence we cannot lock chan_lock
and then call cifs_put_tcp_session or the lock order will not be respected.

To work around this issue, the following change could be made to read the value
of ses->chan_lock while holding chan_lock, but release it before starting the
loop where cifs_put_tcp_session is called.

-       for (i = 1; i < ses->chan_count; i++) {
+       spin_lock(&ses->chan_lock);
+       int ses_chan_count = ses->chan_count;
+
+       spin_unlock(&ses->chan_lock);
+
+       for (i = 1; i < ses_chan_count; i++) {

Please, let me know if this is acceptable.

> > 
> > -- 
> > Regards,
> > Shyam