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

Shyam Prasad N
2025-05-10T14:04:32Z

On Mon, May 5, 2025 at 5:57 AM Henrique Carvalho
<henrique.carvalho@suse.com> wrote:
>
> 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 ... ]
>
That's a good point. I'll make sure to dput outside spinlocks.

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

--
Regards,
Shyam