Re: [PATCH v3] smb: client: fix race in smb2_close_cached_fid()

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

On Tue, May 27, 2025 at 04:06:01PM -0300, Paulo Alcantara wrote:
> Henrique Carvalho <henrique.carvalho@suse.com> writes:
> 
> > find_or_create_cached_dir() could grab a new reference after kref_put()
> > had seen the refcount drop to zero but before cfid_list_lock is acquired
> > in smb2_close_cached_fid(), leading to use-after-free.
> >
> > Switch to kref_put_lock() so cfid_release() runs with cfid_list_lock
> > held, closing that gap.
> 
> Does this mean that SMB2_close() will be called with @cfid_list_lock
> held?  If so, that's wrong.

What I mean here is "Switch to kref_put_lock() so
smb2_close_cached_fid() *IS CALLED* with cfid_list_lock held". Poor
wording on my part.

The lock will be released after cfid is removed from list.

My argument is that cfid can still be found before locking
cfid_list_lock in smb2_close_cached_fid. Here is the current code:

    static void
    smb2_close_cached_fid(struct kref *ref)
    {
            struct cached_fid *cfid = container_of(ref, struct cached_fid,
                                                   refcount);
            int rc;
    
+           /* kref is 0, but cfid can still be found here */
    
            spin_lock(&cfid->cfids->cfid_list_lock);
            if (cfid->on_list) {
                    list_del(&cfid->entry);
                    cfid->on_list = false;
                    cfid->cfids->num_entries--;
            }
            spin_unlock(&cfid->cfids->cfid_list_lock);
    
            dput(cfid->dentry);
            cfid->dentry = NULL;
    
            if (cfid->is_open) {
                    rc = SMB2_close(0, cfid->tcon, cfid->fid.persistent_fid,
                               cfid->fid.volatile_fid);
                    if (rc) /* should we retry on -EBUSY or -EAGAIN? */
                            cifs_dbg(VFS, "close cached dir rc %d\n", rc);
            }
     [...]