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

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

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(-)
> 
> diff --git a/fs/smb/client/cached_dir.c b/fs/smb/client/cached_dir.c
> index fe738623cf1b..f074675fa6be 100644
> --- a/fs/smb/client/cached_dir.c
> +++ b/fs/smb/client/cached_dir.c
> @@ -31,6 +31,7 @@ static struct cached_fid *find_or_create_cached_dir(struct cached_fids *cfids,
>  
>  	spin_lock(&cfids->cfid_list_lock);
>  	list_for_each_entry(cfid, &cfids->entries, entry) {
> +		spin_lock(&cfid->fid_lock);
>  		if (!strcmp(cfid->path, path)) {
>  			/*
>  			 * If it doesn't have a lease it is either not yet
> @@ -38,13 +39,16 @@ static struct cached_fid *find_or_create_cached_dir(struct cached_fids *cfids,
>  			 * being deleted due to a lease break.
>  			 */
>  			if (!cfid->time || !cfid->has_lease) {
> +				spin_unlock(&cfid->fid_lock);
>  				spin_unlock(&cfids->cfid_list_lock);
>  				return NULL;
>  			}
>  			kref_get(&cfid->refcount);
> +			spin_unlock(&cfid->fid_lock);
>  			spin_unlock(&cfids->cfid_list_lock);
>  			return cfid;
>  		}
> +		spin_unlock(&cfid->fid_lock);
>  	}
>  	if (lookup_only) {
>  		spin_unlock(&cfids->cfid_list_lock);
> @@ -192,19 +196,20 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
>  		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);
> +	spin_lock(&cfid->fid_lock);
>  	if (cfid->has_lease && cfid->time) {
> -		spin_unlock(&cfids->cfid_list_lock);
> +		spin_unlock(&cfid->fid_lock);
>  		*ret_cfid = cfid;
>  		kfree(utf16_path);
>  		return 0;
>  	}
> -	spin_unlock(&cfids->cfid_list_lock);
> +	spin_unlock(&cfid->fid_lock);
>  
>  	/*
>  	 * Skip any prefix paths in @path as lookup_positive_unlocked() ends up
> @@ -219,17 +224,6 @@ 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;
> -		}
> -	}
> -	cfid->dentry = dentry;
> -	cfid->tcon = tcon;
>  
>  	/*
>  	 * We do not hold the lock for the open because in case
> @@ -301,9 +295,6 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
>  		}
>  		goto oshr_free;
>  	}
> -	cfid->is_open = true;
> -
> -	spin_lock(&cfids->cfid_list_lock);
>  
>  	o_rsp = (struct smb2_create_rsp *)rsp_iov[0].iov_base;
>  	oparms.fid->persistent_fid = o_rsp->PersistentFileId;
> @@ -314,7 +305,6 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
>  
>  
>  	if (o_rsp->OplockLevel != SMB2_OPLOCK_LEVEL_LEASE) {
> -		spin_unlock(&cfids->cfid_list_lock);
>  		rc = -EINVAL;
>  		goto oshr_free;
>  	}
> @@ -323,21 +313,15 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
>  				 &oparms.fid->epoch,
>  				 oparms.fid->lease_key,
>  				 &oplock, NULL, NULL);
> -	if (rc) {
> -		spin_unlock(&cfids->cfid_list_lock);
> +	if (rc)
>  		goto oshr_free;
> -	}
>  
>  	rc = -EINVAL;
> -	if (!(oplock & SMB2_LEASE_READ_CACHING_HE)) {
> -		spin_unlock(&cfids->cfid_list_lock);
> +	if (!(oplock & SMB2_LEASE_READ_CACHING_HE))
>  		goto oshr_free;
> -	}
>  	qi_rsp = (struct smb2_query_info_rsp *)rsp_iov[1].iov_base;
> -	if (le32_to_cpu(qi_rsp->OutputBufferLength) < sizeof(struct smb2_file_all_info)) {
> -		spin_unlock(&cfids->cfid_list_lock);
> +	if (le32_to_cpu(qi_rsp->OutputBufferLength) < sizeof(struct smb2_file_all_info))
>  		goto oshr_free;
> -	}
>  	if (!smb2_validate_and_copy_iov(
>  				le16_to_cpu(qi_rsp->OutputBufferOffset),
>  				sizeof(struct smb2_file_all_info),
> @@ -345,10 +329,26 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
>  				(char *)&cfid->file_all_info))
>  		cfid->file_all_info_is_valid = true;
>  
> -	cfid->time = jiffies;
> -	spin_unlock(&cfids->cfid_list_lock);
>  	/* At this point the directory handle is fully cached */
>  	rc = 0;
> +	spin_lock(&cfid->fid_lock);
> +	if (!cfid->dentry) {
> +		if (!npath[0]) {
> +			dentry = dget(cifs_sb->root);
> +		} else {
> +			dentry = path_to_dentry(cifs_sb, npath);
> +			if (IS_ERR(dentry)) {
> +				spin_unlock(&cfid->fid_lock);
> +				rc = -ENOENT;
> +				goto out;
> +			}
> +		}
> +		cfid->dentry = dentry;
> +	}
> +	cfid->tcon = tcon;
> +	cfid->is_open = true;
> +	cfid->time = jiffies;
> +	spin_unlock(&cfid->fid_lock);
>  
>  oshr_free:
>  	SMB2_open_free(&rqst[0]);
> @@ -363,6 +363,7 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
>  			cfid->on_list = false;
>  			cfids->num_entries--;
>  		}
> +		spin_lock(&cfid->fid_lock);
>  		if (cfid->has_lease) {
>  			/*
>  			 * We are guaranteed to have two references at this
> @@ -372,6 +373,7 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
>  			cfid->has_lease = false;
>  			kref_put(&cfid->refcount, smb2_close_cached_fid);
>  		}
> +		spin_unlock(&cfid->fid_lock);
>  		spin_unlock(&cfids->cfid_list_lock);
>  
>  		kref_put(&cfid->refcount, smb2_close_cached_fid);
> @@ -400,13 +402,16 @@ int open_cached_dir_by_dentry(struct cifs_tcon *tcon,
>  
>  	spin_lock(&cfids->cfid_list_lock);
>  	list_for_each_entry(cfid, &cfids->entries, entry) {
> +		spin_lock(&cfid->fid_lock);
>  		if (dentry && cfid->dentry == dentry) {
>  			cifs_dbg(FYI, "found a cached file handle by dentry\n");
>  			kref_get(&cfid->refcount);
> +			spin_unlock(&cfid->fid_lock);
>  			*ret_cfid = cfid;
>  			spin_unlock(&cfids->cfid_list_lock);
>  			return 0;
>  		}
> +		spin_unlock(&cfid->fid_lock);
>  	}
>  	spin_unlock(&cfids->cfid_list_lock);
>  	return -ENOENT;
> @@ -427,8 +432,11 @@ smb2_close_cached_fid(struct kref *ref)
>  	}
>  	spin_unlock(&cfid->cfids->cfid_list_lock);
>  
> -	dput(cfid->dentry);
> -	cfid->dentry = NULL;
> +	/* no locking necessary as we're the last user of this cfid */
> +	if (cfid->dentry) {
> +		dput(cfid->dentry);
> +		cfid->dentry = NULL;
> +	}
>  
>  	if (cfid->is_open) {
>  		rc = SMB2_close(0, cfid->tcon, cfid->fid.persistent_fid,
> @@ -451,12 +459,13 @@ void drop_cached_dir_by_name(const unsigned int xid, struct cifs_tcon *tcon,
>  		cifs_dbg(FYI, "no cached dir found for rmdir(%s)\n", name);
>  		return;
>  	}
> -	spin_lock(&cfid->cfids->cfid_list_lock);
> +	spin_lock(&cfid->fid_lock);
>  	if (cfid->has_lease) {
> +		/* mark as invalid */
>  		cfid->has_lease = false;
>  		kref_put(&cfid->refcount, smb2_close_cached_fid);
>  	}
> -	spin_unlock(&cfid->cfids->cfid_list_lock);
> +	spin_unlock(&cfid->fid_lock);
>  	close_cached_dir(cfid);
>  }
>  
> @@ -538,6 +547,7 @@ void invalidate_all_cached_dirs(struct cifs_tcon *tcon)
>  		cfids->num_entries--;
>  		cfid->is_open = false;
>  		cfid->on_list = false;
> +		spin_lock(&cfid->fid_lock);
>  		if (cfid->has_lease) {
>  			/*
>  			 * The lease was never cancelled from the server,
> @@ -546,6 +556,7 @@ void invalidate_all_cached_dirs(struct cifs_tcon *tcon)
>  			cfid->has_lease = false;
>  		} else
>  			kref_get(&cfid->refcount);
> +		spin_unlock(&cfid->fid_lock);
>  	}
>  	/*
>  	 * Queue dropping of the dentries once locks have been dropped
> @@ -600,6 +611,7 @@ int cached_dir_lease_break(struct cifs_tcon *tcon, __u8 lease_key[16])
>  
>  	spin_lock(&cfids->cfid_list_lock);
>  	list_for_each_entry(cfid, &cfids->entries, entry) {
> +		spin_lock(&cfid->fid_lock);
>  		if (cfid->has_lease &&
>  		    !memcmp(lease_key,
>  			    cfid->fid.lease_key,
> @@ -612,6 +624,7 @@ int cached_dir_lease_break(struct cifs_tcon *tcon, __u8 lease_key[16])
>  			 */
>  			list_del(&cfid->entry);
>  			cfid->on_list = false;
> +			spin_unlock(&cfid->fid_lock);
>  			cfids->num_entries--;
>  
>  			++tcon->tc_count;
> @@ -621,6 +634,7 @@ int cached_dir_lease_break(struct cifs_tcon *tcon, __u8 lease_key[16])
>  			spin_unlock(&cfids->cfid_list_lock);
>  			return true;
>  		}
> +		spin_unlock(&cfid->fid_lock);
>  	}
>  	spin_unlock(&cfids->cfid_list_lock);
>  	return false;
> @@ -656,9 +670,6 @@ static void free_cached_dir(struct cached_fid *cfid)
>  	WARN_ON(work_pending(&cfid->close_work));
>  	WARN_ON(work_pending(&cfid->put_work));
>  
> -	dput(cfid->dentry);
> -	cfid->dentry = NULL;
> -
>  	/*
>  	 * Delete all cached dirent names
>  	 */
> @@ -703,6 +714,7 @@ static void cfids_laundromat_worker(struct work_struct *work)
>  
>  	spin_lock(&cfids->cfid_list_lock);
>  	list_for_each_entry_safe(cfid, q, &cfids->entries, entry) {
> +		spin_lock(&cfid->fid_lock);
>  		if (cfid->time &&
>  		    time_after(jiffies, cfid->time + HZ * dir_cache_timeout)) {
>  			cfid->on_list = false;
> @@ -717,6 +729,7 @@ static void cfids_laundromat_worker(struct work_struct *work)
>  			} else
>  				kref_get(&cfid->refcount);
>  		}
> +		spin_unlock(&cfid->fid_lock);
>  	}
>  	spin_unlock(&cfids->cfid_list_lock);
>  
> @@ -726,7 +739,6 @@ static void cfids_laundromat_worker(struct work_struct *work)
>  		spin_lock(&cfid->fid_lock);
>  		dentry = cfid->dentry;
>  		cfid->dentry = NULL;
> -		spin_unlock(&cfid->fid_lock);
>  
>  		dput(dentry);
>  		if (cfid->is_open) {
> @@ -742,6 +754,7 @@ static void cfids_laundromat_worker(struct work_struct *work)
>  			 * was one) or the extra one acquired.
>  			 */
>  			kref_put(&cfid->refcount, smb2_close_cached_fid);
> +		spin_unlock(&cfid->fid_lock);
>  	}
>  	queue_delayed_work(cfid_put_wq, &cfids->laundromat_work,
>  			   dir_cache_timeout * HZ);

You are calling dput() here with a lock held, both in path_to_dentry and
in smb2_close_cached_fid. Is this correct?

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?

Thanks,
Henrique