| Message ID | 234ee19f9706fa55af3bae3e339e39c42d5b0b0a.1701060106.git.pierre.mariani@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/4] smb: client: Delete unused value | expand |
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
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);
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(+)