From: nspmangalore@gmail.com
To: smfrench@gmail.com, bharathsm.hsk@gmail.com, ematsumiya@suse.de,
pc@manguebit.com, paul@darkrain42.org, ronniesahlberg@gmail.com,
linux-cifs@vger.kernel.org
Cc: Shyam Prasad N <sprasad@microsoft.com>
Subject: [PATCH 1/5] cifs: protect cfid accesses with fid_lock
Date: Fri, 2 May 2025 05:13:40 +0000 [thread overview]
Message-ID: <20250502051517.10449-1-sprasad@microsoft.com> (raw)
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
next reply other threads:[~2025-05-02 5:15 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-05-02 5:13 nspmangalore [this message]
2025-05-02 5:13 ` [PATCH 2/5] cifs: do not return an invalidated cfid nspmangalore
2025-05-02 5:13 ` [PATCH 3/5] cifs: serialize initialization and cleanup of cfid nspmangalore
2025-05-02 15:10 ` Henrique Carvalho
2025-05-02 15:12 ` Henrique Carvalho
2025-05-02 5:13 ` [PATCH 4/5] cifs: update the lock ordering comments with new mutex nspmangalore
2025-05-02 5:13 ` [PATCH 5/5] cifs: add new field to track the last access time of cfid nspmangalore
2025-05-02 12:37 ` [PATCH 1/5] cifs: protect cfid accesses with fid_lock Henrique Carvalho
2025-05-02 12:40 ` Henrique Carvalho
2025-05-03 2:54 ` Shyam Prasad N
2025-05-05 0:25 ` Henrique Carvalho
2025-05-05 0:48 ` Steve French
2025-05-10 14:03 ` Shyam Prasad N
2025-05-10 14:04 ` Shyam Prasad N
[not found] ` <CAH2r5mv+CmYtEZ8oGcQQYzwmh0HYgBpaFwLSR3NqtUWxNwTL=Q@mail.gmail.com>
2025-05-02 15:35 ` Enzo Matsumiya
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20250502051517.10449-1-sprasad@microsoft.com \
--to=nspmangalore@gmail.com \
--cc=bharathsm.hsk@gmail.com \
--cc=ematsumiya@suse.de \
--cc=linux-cifs@vger.kernel.org \
--cc=paul@darkrain42.org \
--cc=pc@manguebit.com \
--cc=ronniesahlberg@gmail.com \
--cc=smfrench@gmail.com \
--cc=sprasad@microsoft.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.