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