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

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(+)

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.

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

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