Re: [PATCH 1/5] cifs: protect cfid accesses with fid_lock

Steve French
2025-05-05T00:48:55Z

On Sun, May 4, 2025 at 7:27 PM Henrique Carvalho
<henrique.carvalho@suse.com> wrote:
<snip>
> > > Also, the lock ordering here is lock(fid_lock) -> lock(cifs_tcp_ses_lock) ->
> > > unlock(cifs_tcp_ses_lock) -> unlock(fid_lock), won't this blow up in
> > > another path?
> >
> > Can you please elaborate which code path will result in this lock ordering?
>
> I was referring to the following pattern in cifs_laundromat_worker():
>
> spin_lock(&cfid->fid_lock);
> ...
> spin_lock(&cifs_tcp_ses_lock);
> spin_unlock(&cifs_tcp_ses_lock);
> ...
> spin_unlock(&cfid->fid_lock);
>
> This was more of an open question. I am not certain this causes any issues,
> and I could not find any concrete problem with it.
>
> I brought it up because cifs_tcp_ses_lock is a more global lock than
> cfid->fid_lock.

That does look like a good catch

In the lock ordering list (see cifsglob.h line 1980ff)
cached_fid->fid_lock
is almost at the end of the list so is after
cifs_tcp_ses_lock

"if two locks are to be held together, the lock that appears higher in
this list needs to be taken before the other" so this does look like
the wrong locking order in the example you pointed out.

The updated lock ordering list with the proposed ordering for the new
locks fid_lock and cfid_mutex is in Shyam's patch:
"[PATCH 4/8] cifs: update the lock ordering comments with new mutex"

--
Thanks,

Steve