Re: [PATCH] smb: client: avoid dentry leak by not overwriting cfid->dentry

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

On Tue, May 06, 2025 at 11:16:46PM -0700, Paul Aurich wrote:
> On 2025-05-06 19:31:56 -0300, Henrique Carvalho wrote:
> > A race, likely between lease break and open, can cause cfid->dentry to
> > be valid when open_cached_dir() tries to set it again. This overwrites
> > the old dentry without dput(), leaking it.
> > 
> > Skip assignment if cfid->dentry is already set.
> > 
> > Signed-off-by: Henrique Carvalho <henrique.carvalho@suse.com>
> > ---
> > fs/smb/client/cached_dir.c | 23 +++++++++++++++--------
> > 1 file changed, 15 insertions(+), 8 deletions(-)
> > 
> > diff --git a/fs/smb/client/cached_dir.c b/fs/smb/client/cached_dir.c
> > index 43228ec2424d..8c1f00a3fc29 100644
> > --- a/fs/smb/client/cached_dir.c
> > +++ b/fs/smb/client/cached_dir.c
> > @@ -219,16 +219,23 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
> > 		goto out;
> > 	}
> > 
> > -	if (!npath[0]) {
> > -		dentry = dget(cifs_sb->root);
> > -	} else {
> > -		dentry = path_to_dentry(cifs_sb, npath);
> > -		if (IS_ERR(dentry)) {
> > -			rc = -ENOENT;
> > -			goto out;
> > +	/*
> > +	 * BB: cfid->dentry should be NULL here; if not, we're likely racing with
> > +	 * a lease break. This is a temporary workaround to avoid overwriting
> > +	 * a valid dentry. Needs proper fix.
> > +	 */
> 
> Ah ha. I think this is trying to address the same race as Shyam's 'cifs: do
> not return an invalidated cfid' [1].
> 
> What about modifying open_cached_dir to hold cfid_list_lock across the call
> to find_or_create_cached_dir through where it tests for validity, and then
> dropping the locking in find_or_create_cached_dir itself (see attached in
> case my text description isn't clear)?
> 
> That's the only way I can see that a pre-existing cfid could escape to the
> rest of open_cached_dir. I think.
> 
> ~Paul
> 
> [1] https://lore.kernel.org/linux-cifs/20250502051517.10449-2-sprasad@microsoft.com/T/#u
> 
> > +	if (!cfid->dentry) {
> > +		if (!npath[0]) {
> > +			dentry = dget(cifs_sb->root);
> > +		} else {
> > +			dentry = path_to_dentry(cifs_sb, npath);
> > +			if (IS_ERR(dentry)) {
> > +				rc = -ENOENT;
> > +				goto out;
> > +			}
> > 		}
> > +		cfid->dentry = dentry;
> > 	}
> > -	cfid->dentry = dentry;
> > 	cfid->tcon = tcon;
> > 
> > 	/*
> > -- 
> > 2.47.0

> >From 583824153dd901f1f7d63ce9364d32c9c249fd43 Mon Sep 17 00:00:00 2001
> From: Paul Aurich <paul@darkrain42.org>
> Date: Tue, 6 May 2025 22:28:09 -0700
> Subject: [PATCH] smb: client: Avoid race in open_cached_dir with lease breaks
> 
> A pre-existing valid cfid returned from find_or_create_cached_dir might
> race with a lease break, meaning open_cached_dir doesn't consider it
> valid, and thinks it's newly-constructed. This leaks a dentry reference
> if the allocation occurs before the queued lease break work runs.
> 
> Avoid the race by extending holding the cfid_list_lock across
> find_or_create_cached_dir and when the result is checked.
> 
> Signed-off-by: Paul Aurich <paul@darkrain42.org>
> Cc: stable@vger.kernel.org
> ---
>  fs/smb/client/cached_dir.c | 10 ++--------
>  1 file changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/smb/client/cached_dir.c b/fs/smb/client/cached_dir.c
> index 10aad87ce679..cb8477c18905 100644
> --- a/fs/smb/client/cached_dir.c
> +++ b/fs/smb/client/cached_dir.c
> @@ -27,38 +27,32 @@ static struct cached_fid *find_or_create_cached_dir(struct cached_fids *cfids,
>  						    bool lookup_only,
>  						    __u32 max_cached_dirs)
>  {
>  	struct cached_fid *cfid;
>  
> -	spin_lock(&cfids->cfid_list_lock);
>  	list_for_each_entry(cfid, &cfids->entries, entry) {
>  		if (!strcmp(cfid->path, path)) {
>  			/*
>  			 * If it doesn't have a lease it is either not yet
>  			 * fully cached or it may be in the process of
>  			 * being deleted due to a lease break.
>  			 */
>  			if (!cfid->time || !cfid->has_lease) {
> -				spin_unlock(&cfids->cfid_list_lock);
>  				return NULL;
>  			}
>  			kref_get(&cfid->refcount);
> -			spin_unlock(&cfids->cfid_list_lock);
>  			return cfid;
>  		}
>  	}
>  	if (lookup_only) {
> -		spin_unlock(&cfids->cfid_list_lock);
>  		return NULL;
>  	}
>  	if (cfids->num_entries >= max_cached_dirs) {
> -		spin_unlock(&cfids->cfid_list_lock);
>  		return NULL;
>  	}
>  	cfid = init_cached_dir(path);
>  	if (cfid == NULL) {
> -		spin_unlock(&cfids->cfid_list_lock);
>  		return NULL;
>  	}
>  	cfid->cfids = cfids;
>  	cfids->num_entries++;
>  	list_add(&cfid->entry, &cfids->entries);
> @@ -72,11 +66,10 @@ static struct cached_fid *find_or_create_cached_dir(struct cached_fids *cfids,
>  	 * Concurrent processes won't be to use it yet due to @cfid->time being
>  	 * zero.
>  	 */
>  	cfid->has_lease = true;
>  
> -	spin_unlock(&cfids->cfid_list_lock);
>  	return cfid;
>  }
>  
>  static struct dentry *
>  path_to_dentry(struct cifs_sb_info *cifs_sb, const char *path)
> @@ -185,21 +178,22 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
>  
>  	utf16_path = cifs_convert_path_to_utf16(path, cifs_sb);
>  	if (!utf16_path)
>  		return -ENOMEM;
>  
> +	spin_lock(&cfids->cfid_list_lock);
>  	cfid = find_or_create_cached_dir(cfids, path, lookup_only, tcon->max_cached_dirs);
>  	if (cfid == NULL) {
> +		spin_unlock(&cfids->cfid_list_lock);
>  		kfree(utf16_path);
>  		return -ENOENT;
>  	}
>  	/*
>  	 * Return cached fid if it is valid (has a lease and has a time).
>  	 * Otherwise, it is either a new entry or laundromat worker removed it
>  	 * from @cfids->entries.  Caller will put last reference if the latter.
>  	 */
> -	spin_lock(&cfids->cfid_list_lock);
>  	if (cfid->has_lease && cfid->time) {
>  		spin_unlock(&cfids->cfid_list_lock);
>  		*ret_cfid = cfid;
>  		kfree(utf16_path);
>  		return 0;
> -- 
> 2.47.2
> 

Yes!

Closing this gap seems to be the only viable fix.

I'm running the tests, but LGTM.


Best,
Henrique