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

Shyam Prasad N
2025-05-10T14:03:26Z

On Mon, May 5, 2025 at 6:18 AM Steve French <smfrench@gmail.com> wrote:
>
> 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

cifs_tcp_ses_lock is not the right lock to use here in the first place.
These large locks have caused us plenty of problems in the past with deadlocks.
We later decided to break it up into smaller locks for protecting
individual fields. cifs_tcp_ses_lock is meant to protect
cifs_tcp_ses_list only.
tc_count should be protected by tc_lock. I'll make the changes here.

--
Regards,
Shyam