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

Enzo Matsumiya
2025-05-02T15:35:57Z

On 05/02, Steve French wrote:
>In my testing this first patch led to hangs very quickly running xfstests
>(in that example was to windows server with multichannel). More testing and
>review feedback would be appreciated

Same here. Very easy reproducer:

# mount.cifs -o ... //srv/share /mnt
# rm -rf /mnt/*
<RCU stall on open_cached_dir_by_dentry() when spinning trying to lock
fid_lock on loop>

I'll debug it and reply with what I find.

Cheers,

Enzo

>Thanks,
>
>Steve
>
>On Fri, May 2, 2025, 12:15 AM <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);
>> --
>> 2.43.0
>>
>>