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

Henrique Carvalho
2025-05-02T12:39:24Z

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