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@xxxxxxxxx> 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@xxxxxxxxx>
> > ---
> > 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
