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

[Henrique Carvalho <henrique.carvalho@suse.com>]

On Sat, May 03, 2025 at 08:24:13AM +0530, Shyam Prasad N wrote:
> On Fri, May 2, 2025 at 6:09 PM Henrique Carvalho
> <henrique.carvalho@suse.com> wrote:
> >
> > Hi Shyam,
> >
> > On Fri, May 02, 2025 at 05:13:40AM +0000, nspmangalore@gmail.com wrote:
> > > From: Shyam Prasad N <sprasad@microsoft.com>
> > >
> > > There are several accesses to cfid structure today without
> > > locking fid_lock. This can lead to race conditions that are
> > > hard to debug.
> > >
> > > With this change, I'm trying to make sure that accesses to cfid
> > > struct members happen with fid_lock held.
> > >
> > > Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
> > > ---
> > >  fs/smb/client/cached_dir.c | 87 ++++++++++++++++++++++----------------
> > >  1 file changed, 50 insertions(+), 37 deletions(-)
> > >
> >
> > You are calling dput() here with a lock held, both in path_to_dentry and
> > in smb2_close_cached_fid. Is this correct?
> 
> Hi Henrique,
> Thanks for reviewing the patches.
> 
> Do you see any obvious problem with it?
> dput would call into VFS layer and might end up calling
> cifs_free_inode. But that does not take any of the competing locks.
> 

Hi Shyam,

Yes, dput() starts with might_sleep(), which means it may preemp (e.g.,
due to disk I/O), so it must not be called while holding a spinlock.

If you compile the kernel with CONFIG_DEBUG_ATOMIC_SLEEP=y you will see
this kind of stack dump.

[  305.667062][  T940] BUG: sleeping function called from invalid context at security/selinux/hooks.c:283
[  305.668291][  T940] in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 940, name: ls
[  305.669199][  T940] preempt_count: 1, expected: 0
[  305.669493][  T940] RCU nest depth: 0, expected: 0
[  305.670092][  T940] 3 locks held by ls/940:
[  305.670362][  T940]  #0: ffff8881099b8f08 (&f->f_pos_lock){+.+.}-{4:4}, at: fdget_pos+0x18a/0x1c0
[  305.671009][  T940]  #1: ffff88811c490158 (&type->i_mutex_dir_key#7){.+.+}-{4:4}, at: iterate_dir+0x85/0x270
[  305.671615][  T940]  #2: ffff88810cc620b0 (&cfid->fid_lock){+.+.}-{3:3}, at: open_cached_dir+0x1098/0x14a0 [cifs]
[ ... stack trace continues ... ]

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


Best,
Henrique