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.
--
Regards,
Shyam
