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

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

[nspmangalore]

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

[PATCH 2/5] cifs: do not return an invalidated cfid

[nspmangalore]

From: Shyam Prasad N <sprasad@microsoft.com>

open_cached_dir should either return an existing valid cfid
or create a new one. Validity of cfid depends on both
cfid->has_lease and cfid->time to be true. However, if has_lease
was invalidated by a worker thread in parallel, we could end up
leaking both a dentry and a server handle.

This change checks if the entry was invalidated and returns
a -ENOENT in such a case.

Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
---
 fs/smb/client/cached_dir.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/fs/smb/client/cached_dir.c b/fs/smb/client/cached_dir.c
index f074675fa6be..d307636c2679 100644
--- a/fs/smb/client/cached_dir.c
+++ b/fs/smb/client/cached_dir.c
@@ -198,9 +198,11 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
 	}
 
 	/*
+	 * check again that the cfid is valid (with mutex held this time).
 	 * 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.
+	 * from @cfids->entries.  If the latter, we drop the refcount and return
+	 * an error to the caller.
 	 */
 	spin_lock(&cfid->fid_lock);
 	if (cfid->has_lease && cfid->time) {
@@ -208,6 +210,12 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
 		*ret_cfid = cfid;
 		kfree(utf16_path);
 		return 0;
+	} else if (!cfid->has_lease) {
+		spin_unlock(&cfid->fid_lock);
+		/* drop the ref that we have */
+		kref_put(&cfid->refcount, smb2_close_cached_fid);
+		kfree(utf16_path);
+		return -ENOENT;
 	}
 	spin_unlock(&cfid->fid_lock);
 
-- 
2.43.0

[PATCH 3/5] cifs: serialize initialization and cleanup of cfid

[nspmangalore]

From: Shyam Prasad N <sprasad@microsoft.com>

Today we can have multiple processes calling open_cached_dir
and other workers freeing the cached dir all in parallel.
Although small sections of this code is locked to protect
individual fields, there can be races between these threads
which can be hard to debug.

This patch serializes all initialization and cleanup of
the cfid struct and the associated resources: dentry and
the server handle.

Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
---
 fs/smb/client/cached_dir.c | 16 ++++++++++++++++
 fs/smb/client/cached_dir.h |  1 +
 2 files changed, 17 insertions(+)

diff --git a/fs/smb/client/cached_dir.c b/fs/smb/client/cached_dir.c
index d307636c2679..9aedb6cf66df 100644
--- a/fs/smb/client/cached_dir.c
+++ b/fs/smb/client/cached_dir.c
@@ -197,6 +197,12 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
 		return -ENOENT;
 	}
 
+	/*
+	 * the following is a critical section. We need to make sure that the
+	 * callers are serialized per-cfid
+	 */
+	mutex_lock(&cfid->cfid_mutex);
+
 	/*
 	 * check again that the cfid is valid (with mutex held this time).
 	 * Return cached fid if it is valid (has a lease and has a time).
@@ -207,11 +213,13 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
 	spin_lock(&cfid->fid_lock);
 	if (cfid->has_lease && cfid->time) {
 		spin_unlock(&cfid->fid_lock);
+		mutex_unlock(&cfid->cfid_mutex);
 		*ret_cfid = cfid;
 		kfree(utf16_path);
 		return 0;
 	} else if (!cfid->has_lease) {
 		spin_unlock(&cfid->fid_lock);
+		mutex_unlock(&cfid->cfid_mutex);
 		/* drop the ref that we have */
 		kref_put(&cfid->refcount, smb2_close_cached_fid);
 		kfree(utf16_path);
@@ -228,6 +236,7 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
 	 */
 	npath = path_no_prefix(cifs_sb, path);
 	if (IS_ERR(npath)) {
+		mutex_unlock(&cfid->cfid_mutex);
 		rc = PTR_ERR(npath);
 		goto out;
 	}
@@ -389,6 +398,8 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
 		*ret_cfid = cfid;
 		atomic_inc(&tcon->num_remote_opens);
 	}
+	mutex_unlock(&cfid->cfid_mutex);
+
 	kfree(utf16_path);
 
 	if (is_replayable_error(rc) &&
@@ -432,6 +443,9 @@ smb2_close_cached_fid(struct kref *ref)
 					       refcount);
 	int rc;
 
+	/* make sure not to race with server open */
+	mutex_lock(&cfid->cfid_mutex);
+
 	spin_lock(&cfid->cfids->cfid_list_lock);
 	if (cfid->on_list) {
 		list_del(&cfid->entry);
@@ -454,6 +468,7 @@ smb2_close_cached_fid(struct kref *ref)
 	}
 
 	free_cached_dir(cfid);
+	mutex_unlock(&cfid->cfid_mutex);
 }
 
 void drop_cached_dir_by_name(const unsigned int xid, struct cifs_tcon *tcon,
@@ -666,6 +681,7 @@ static struct cached_fid *init_cached_dir(const char *path)
 	INIT_LIST_HEAD(&cfid->entry);
 	INIT_LIST_HEAD(&cfid->dirents.entries);
 	mutex_init(&cfid->dirents.de_mutex);
+	mutex_init(&cfid->cfid_mutex);
 	spin_lock_init(&cfid->fid_lock);
 	kref_init(&cfid->refcount);
 	return cfid;
diff --git a/fs/smb/client/cached_dir.h b/fs/smb/client/cached_dir.h
index 1dfe79d947a6..93c936af2253 100644
--- a/fs/smb/client/cached_dir.h
+++ b/fs/smb/client/cached_dir.h
@@ -42,6 +42,7 @@ struct cached_fid {
 	struct kref refcount;
 	struct cifs_fid fid;
 	spinlock_t fid_lock;
+	struct mutex cfid_mutex;
 	struct cifs_tcon *tcon;
 	struct dentry *dentry;
 	struct work_struct put_work;
-- 
2.43.0

Re: [PATCH 3/5] cifs: serialize initialization and cleanup of cfid

[Henrique Carvalho]

Hi Shyam,

On Fri, May 02, 2025 at 05:13:42AM +0000, nspmangalore@gmail.com wrote:
> From: Shyam Prasad N <sprasad@microsoft.com>
> 
> Today we can have multiple processes calling open_cached_dir
> and other workers freeing the cached dir all in parallel.
> Although small sections of this code is locked to protect
> individual fields, there can be races between these threads
> which can be hard to debug.
> 
> This patch serializes all initialization and cleanup of
> the cfid struct and the associated resources: dentry and
> the server handle.
> 
> Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
> ---
>  fs/smb/client/cached_dir.c | 16 ++++++++++++++++
>  fs/smb/client/cached_dir.h |  1 +
>  2 files changed, 17 insertions(+)
> 
> diff --git a/fs/smb/client/cached_dir.c b/fs/smb/client/cached_dir.c
> index d307636c2679..9aedb6cf66df 100644
> --- a/fs/smb/client/cached_dir.c
> +++ b/fs/smb/client/cached_dir.c
> @@ -197,6 +197,12 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
>  		return -ENOENT;
>  	}
>  
> +	/*
> +	 * the following is a critical section. We need to make sure that the
> +	 * callers are serialized per-cfid
> +	 */
> +	mutex_lock(&cfid->cfid_mutex);
> +
>  	/*
>  	 * check again that the cfid is valid (with mutex held this time).
>  	 * Return cached fid if it is valid (has a lease and has a time).
> @@ -207,11 +213,13 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
>  	spin_lock(&cfid->fid_lock);
>  	if (cfid->has_lease && cfid->time) {
>  		spin_unlock(&cfid->fid_lock);
> +		mutex_unlock(&cfid->cfid_mutex);
>  		*ret_cfid = cfid;
>  		kfree(utf16_path);
>  		return 0;
>  	} else if (!cfid->has_lease) {
>  		spin_unlock(&cfid->fid_lock);
> +		mutex_unlock(&cfid->cfid_mutex);
>  		/* drop the ref that we have */
>  		kref_put(&cfid->refcount, smb2_close_cached_fid);
>  		kfree(utf16_path);
> @@ -228,6 +236,7 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
>  	 */
>  	npath = path_no_prefix(cifs_sb, path);
>  	if (IS_ERR(npath)) {
> +		mutex_unlock(&cfid->cfid_mutex);
>  		rc = PTR_ERR(npath);
>  		goto out;
>  	}
> @@ -389,6 +398,8 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
>  		*ret_cfid = cfid;
>  		atomic_inc(&tcon->num_remote_opens);
>  	}
> +	mutex_unlock(&cfid->cfid_mutex);
> +
>  	kfree(utf16_path);
>  
>  	if (is_replayable_error(rc) &&
> @@ -432,6 +443,9 @@ smb2_close_cached_fid(struct kref *ref)
>  					       refcount);
>  	int rc;
>  
> +	/* make sure not to race with server open */
> +	mutex_lock(&cfid->cfid_mutex);
> +
>  	spin_lock(&cfid->cfids->cfid_list_lock);
>  	if (cfid->on_list) {
>  		list_del(&cfid->entry);
> @@ -454,6 +468,7 @@ smb2_close_cached_fid(struct kref *ref)
>  	}
>  
>  	free_cached_dir(cfid);
> +	mutex_unlock(&cfid->cfid_mutex);
>  }
>  
>  void drop_cached_dir_by_name(const unsigned int xid, struct cifs_tcon *tcon,
> @@ -666,6 +681,7 @@ static struct cached_fid *init_cached_dir(const char *path)
>  	INIT_LIST_HEAD(&cfid->entry);
>  	INIT_LIST_HEAD(&cfid->dirents.entries);
>  	mutex_init(&cfid->dirents.de_mutex);
> +	mutex_init(&cfid->cfid_mutex);
>  	spin_lock_init(&cfid->fid_lock);
>  	kref_init(&cfid->refcount);
>  	return cfid;
> diff --git a/fs/smb/client/cached_dir.h b/fs/smb/client/cached_dir.h
> index 1dfe79d947a6..93c936af2253 100644
> --- a/fs/smb/client/cached_dir.h
> +++ b/fs/smb/client/cached_dir.h
> @@ -42,6 +42,7 @@ struct cached_fid {
>  	struct kref refcount;
>  	struct cifs_fid fid;
>  	spinlock_t fid_lock;
> +	struct mutex cfid_mutex;
>  	struct cifs_tcon *tcon;
>  	struct dentry *dentry;
>  	struct work_struct put_work;
> -- 
> 2.43.0
> 
>

I might be missing something, but...

First, if smb2_close_cached_fid is the release function, meaning I just
released the last cfid ref. So in my understanding I want to, as fast as
possible, remove this cfid from list so it is not found anymore on
find_or_create_cached_dir and then free the cfid. That mutex inside the
function has the intention of preventing a race with open, but if I have
another cfid waiting to acquire the mutex that will just cause an UAF.

Second, I am not fully convinced that we need a mutex there. :/ I have
thought about it many times and I could not get a proof that there is a
race happening there.

Third, (referencing PATCH 2) even if we have a mutex there, shouldn't we
just let the thread that just acquired the mutex retry to acquire the
lease (which I believe is the current behavior).

Thanks,
Henrique

Re: [PATCH 3/5] cifs: serialize initialization and cleanup of cfid

[Henrique Carvalho]

On Fri, May 02, 2025 at 12:10:04PM -0300, Henrique Carvalho wrote:
> 
> Second, I am not fully convinced that we need a mutex there. :/ I have
> thought about it many times and I could not get a proof that there is a
> race happening there.
> 
> Third, (referencing PATCH 2) even if we have a mutex there, shouldn't we
> just let the thread that just acquired the mutex retry to acquire the
> lease (which I believe is the current behavior).

Here "there" means open_cached_dir.

> 
> Thanks,
> Henrique

[PATCH 4/5] cifs: update the lock ordering comments with new mutex

[nspmangalore]

From: Shyam Prasad N <sprasad@microsoft.com>

The lock ordering rules listed as comments in cifsglob.h were
missing some lock details and the newly introduced cfid_mutex.

Updated those notes in this commit.

Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
---
 fs/smb/client/cifsglob.h | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/fs/smb/client/cifsglob.h b/fs/smb/client/cifsglob.h
index 3b32116b0b49..a330abeea64f 100644
--- a/fs/smb/client/cifsglob.h
+++ b/fs/smb/client/cifsglob.h
@@ -1988,8 +1988,8 @@ require use of the stronger protocol */
  * TCP_Server_Info->		TCP_Server_Info			cifs_get_tcp_session
  * reconnect_mutex
  * TCP_Server_Info->srv_mutex	TCP_Server_Info			cifs_get_tcp_session
- * cifs_ses->session_mutex		cifs_ses		sesInfoAlloc
- *				cifs_tcon
+ * cifs_ses->session_mutex	cifs_ses			sesInfoAlloc
+ * cached_fid->cfid_mutex	cached_fid			init_cached_dir
  * cifs_tcon->open_file_lock	cifs_tcon->openFileList		tconInfoAlloc
  *				cifs_tcon->pending_opens
  * cifs_tcon->stat_lock		cifs_tcon->bytes_read		tconInfoAlloc
@@ -2008,21 +2008,25 @@ require use of the stronger protocol */
  *				->oplock_credits
  *				->reconnect_instance
  * cifs_ses->ses_lock		(anything that is not protected by another lock and can change)
+ *								sesInfoAlloc
  * cifs_ses->iface_lock		cifs_ses->iface_list		sesInfoAlloc
  *				->iface_count
  *				->iface_last_update
- * cifs_ses->chan_lock		cifs_ses->chans
+ * cifs_ses->chan_lock		cifs_ses->chans			sesInfoAlloc
  *				->chans_need_reconnect
  *				->chans_in_reconnect
  * cifs_tcon->tc_lock		(anything that is not protected by another lock and can change)
+ *								tcon_info_alloc
  * inode->i_rwsem, taken by fs/netfs/locking.c e.g. should be taken before cifsInodeInfo locks
  * cifsInodeInfo->open_file_lock	cifsInodeInfo->openFileList	cifs_alloc_inode
  * cifsInodeInfo->writers_lock	cifsInodeInfo->writers		cifsInodeInfo_alloc
  * cifsInodeInfo->lock_sem	cifsInodeInfo->llist		cifs_init_once
  *				->can_cache_brlcks
  * cifsInodeInfo->deferred_lock	cifsInodeInfo->deferred_closes	cifsInodeInfo_alloc
- * cached_fids->cfid_list_lock	cifs_tcon->cfids->entries	 init_cached_dirs
- * cifsFileInfo->fh_mutex		cifsFileInfo			cifs_new_fileinfo
+ * cached_fids->cfid_list_lock	cifs_tcon->cfids->entries	init_cached_dirs
+ * cached_fid->fid_lock		(anything that is not protected by another lock and can change)
+ *								init_cached_dir
+ * cifsFileInfo->fh_mutex	cifsFileInfo			cifs_new_fileinfo
  * cifsFileInfo->file_info_lock	cifsFileInfo->count		cifs_new_fileinfo
  *				->invalidHandle			initiate_cifs_search
  *				->oplock_break_cancelled
-- 
2.43.0

[PATCH 5/5] cifs: add new field to track the last access time of cfid

[nspmangalore]

From: Shyam Prasad N <sprasad@microsoft.com>

The handlecache code today tracks the time at which dir lease was
acquired and the laundromat thread uses that to check for old
entries to cleanup.

However, if a directory is actively accessed, it should not
be chosen to expire first.

This change adds a new last_access_time field to cfid and
uses that to decide expiry of the cfid.

Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
---
 fs/smb/client/cached_dir.c | 6 ++++--
 fs/smb/client/cached_dir.h | 1 +
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/fs/smb/client/cached_dir.c b/fs/smb/client/cached_dir.c
index 9aedb6cf66df..34d21b7d701e 100644
--- a/fs/smb/client/cached_dir.c
+++ b/fs/smb/client/cached_dir.c
@@ -212,6 +212,7 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
 	 */
 	spin_lock(&cfid->fid_lock);
 	if (cfid->has_lease && cfid->time) {
+		cfid->last_access_time = jiffies;
 		spin_unlock(&cfid->fid_lock);
 		mutex_unlock(&cfid->cfid_mutex);
 		*ret_cfid = cfid;
@@ -365,6 +366,7 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
 	cfid->tcon = tcon;
 	cfid->is_open = true;
 	cfid->time = jiffies;
+	cfid->last_access_time = jiffies;
 	spin_unlock(&cfid->fid_lock);
 
 oshr_free:
@@ -739,8 +741,8 @@ 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)) {
+		if (cfid->last_access_time &&
+		    time_after(jiffies, cfid->last_access_time + HZ * dir_cache_timeout)) {
 			cfid->on_list = false;
 			list_move(&cfid->entry, &entry);
 			cfids->num_entries--;
diff --git a/fs/smb/client/cached_dir.h b/fs/smb/client/cached_dir.h
index 93c936af2253..6d4b9413aa67 100644
--- a/fs/smb/client/cached_dir.h
+++ b/fs/smb/client/cached_dir.h
@@ -39,6 +39,7 @@ struct cached_fid {
 	bool on_list:1;
 	bool file_all_info_is_valid:1;
 	unsigned long time; /* jiffies of when lease was taken */
+	unsigned long last_access_time; /* jiffies of when last accessed */
 	struct kref refcount;
 	struct cifs_fid fid;
 	spinlock_t fid_lock;
-- 
2.43.0

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

[Henrique Carvalho]

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

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

[Henrique Carvalho]

On Fri, May 02, 2025 at 09:37:34AM -0300, Henrique Carvalho wrote:
> 
> You are calling dput() here with a lock held, both in path_to_dentry and
> in smb2_close_cached_fid. Is this correct?
> 

Correction: it is in cfids_laundromat_worker.

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

[Shyam Prasad N]

On Fri, May 2, 2025 at 6:09 PM Henrique Carvalho
<henrique.carvalho@suse.com> wrote:
>
> 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?

Hi Henrique,
Thanks for reviewing the patches.

Do you see any obvious problem with it?
dput would call into VFS layer and might end up calling
cifs_free_inode. But that does not take any of the competing locks.

>
> 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?

Can you please elaborate which code path will result in this lock ordering?

>
> Thanks,
> Henrique



-- 
Regards,
Shyam

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

[Henrique Carvalho]

On Sat, May 03, 2025 at 08:24:13AM +0530, Shyam Prasad N wrote:
> On Fri, May 2, 2025 at 6:09 PM Henrique Carvalho
> <henrique.carvalho@suse.com> wrote:
> >
> > 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(-)
> > >
> >
> > You are calling dput() here with a lock held, both in path_to_dentry and
> > in smb2_close_cached_fid. Is this correct?
> 
> Hi Henrique,
> Thanks for reviewing the patches.
> 
> Do you see any obvious problem with it?
> dput would call into VFS layer and might end up calling
> cifs_free_inode. But that does not take any of the competing locks.
> 

Hi Shyam,

Yes, dput() starts with might_sleep(), which means it may preemp (e.g.,
due to disk I/O), so it must not be called while holding a spinlock.

If you compile the kernel with CONFIG_DEBUG_ATOMIC_SLEEP=y you will see
this kind of stack dump.

[  305.667062][  T940] BUG: sleeping function called from invalid context at security/selinux/hooks.c:283
[  305.668291][  T940] in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 940, name: ls
[  305.669199][  T940] preempt_count: 1, expected: 0
[  305.669493][  T940] RCU nest depth: 0, expected: 0
[  305.670092][  T940] 3 locks held by ls/940:
[  305.670362][  T940]  #0: ffff8881099b8f08 (&f->f_pos_lock){+.+.}-{4:4}, at: fdget_pos+0x18a/0x1c0
[  305.671009][  T940]  #1: ffff88811c490158 (&type->i_mutex_dir_key#7){.+.+}-{4:4}, at: iterate_dir+0x85/0x270
[  305.671615][  T940]  #2: ffff88810cc620b0 (&cfid->fid_lock){+.+.}-{3:3}, at: open_cached_dir+0x1098/0x14a0 [cifs]
[ ... stack trace continues ... ]

> >
> > 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?
> 
> Can you please elaborate which code path will result in this lock ordering?

I was referring to the following pattern in cifs_laundromat_worker():

  spin_lock(&cfid->fid_lock);
  ...
  spin_lock(&cifs_tcp_ses_lock);
  spin_unlock(&cifs_tcp_ses_lock);
  ...
  spin_unlock(&cfid->fid_lock);

This was more of an open question. I am not certain this causes any issues,
and I could not find any concrete problem with it.

I brought it up because cifs_tcp_ses_lock is a more global lock than 
cfid->fid_lock.


Best,
Henrique

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

[Steve French]

On Sun, May 4, 2025 at 7:27 PM Henrique Carvalho
<henrique.carvalho@suse.com> wrote:
<snip>
> > > 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?
> >
> > Can you please elaborate which code path will result in this lock ordering?
>
> I was referring to the following pattern in cifs_laundromat_worker():
>
>   spin_lock(&cfid->fid_lock);
>   ...
>   spin_lock(&cifs_tcp_ses_lock);
>   spin_unlock(&cifs_tcp_ses_lock);
>   ...
>   spin_unlock(&cfid->fid_lock);
>
> This was more of an open question. I am not certain this causes any issues,
> and I could not find any concrete problem with it.
>
> I brought it up because cifs_tcp_ses_lock is a more global lock than
> cfid->fid_lock.

That does look like a good catch

In the lock ordering list (see cifsglob.h line 1980ff)
   cached_fid->fid_lock
is almost at the end of the list so is after
   cifs_tcp_ses_lock

"if two locks are to be held together, the lock that appears higher in
this list needs to be taken before the other" so this does look like
the wrong locking order in the example you pointed out.

The updated lock ordering list with the proposed ordering for the new
locks fid_lock and cfid_mutex is in Shyam's patch:
"[PATCH 4/8] cifs: update the lock ordering comments with new mutex"

-- 
Thanks,

Steve

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

[Shyam Prasad N]

On Mon, May 5, 2025 at 6:18 AM Steve French <smfrench@gmail.com> wrote:
>
> On Sun, May 4, 2025 at 7:27 PM Henrique Carvalho
> <henrique.carvalho@suse.com> wrote:
> <snip>
> > > > 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?
> > >
> > > Can you please elaborate which code path will result in this lock ordering?
> >
> > I was referring to the following pattern in cifs_laundromat_worker():
> >
> >   spin_lock(&cfid->fid_lock);
> >   ...
> >   spin_lock(&cifs_tcp_ses_lock);
> >   spin_unlock(&cifs_tcp_ses_lock);
> >   ...
> >   spin_unlock(&cfid->fid_lock);
> >
> > This was more of an open question. I am not certain this causes any issues,
> > and I could not find any concrete problem with it.
> >
> > I brought it up because cifs_tcp_ses_lock is a more global lock than
> > cfid->fid_lock.
>
> That does look like a good catch
>
> In the lock ordering list (see cifsglob.h line 1980ff)
>    cached_fid->fid_lock
> is almost at the end of the list so is after
>    cifs_tcp_ses_lock
>
> "if two locks are to be held together, the lock that appears higher in
> this list needs to be taken before the other" so this does look like
> the wrong locking order in the example you pointed out.
>
> The updated lock ordering list with the proposed ordering for the new
> locks fid_lock and cfid_mutex is in Shyam's patch:
> "[PATCH 4/8] cifs: update the lock ordering comments with new mutex"
>
> --
> Thanks,
>
> Steve

cifs_tcp_ses_lock is not the right lock to use here in the first place.
These large locks have caused us plenty of problems in the past with deadlocks.
We later decided to break it up into smaller locks for protecting
individual fields. cifs_tcp_ses_lock is meant to protect
cifs_tcp_ses_list only.
tc_count should be protected by tc_lock. I'll make the changes here.

-- 
Regards,
Shyam

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

[Shyam Prasad N]

On Mon, May 5, 2025 at 5:57 AM Henrique Carvalho
<henrique.carvalho@suse.com> wrote:
>
> On Sat, May 03, 2025 at 08:24:13AM +0530, Shyam Prasad N wrote:
> > On Fri, May 2, 2025 at 6:09 PM Henrique Carvalho
> > <henrique.carvalho@suse.com> wrote:
> > >
> > > 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(-)
> > > >
> > >
> > > You are calling dput() here with a lock held, both in path_to_dentry and
> > > in smb2_close_cached_fid. Is this correct?
> >
> > Hi Henrique,
> > Thanks for reviewing the patches.
> >
> > Do you see any obvious problem with it?
> > dput would call into VFS layer and might end up calling
> > cifs_free_inode. But that does not take any of the competing locks.
> >
>
> Hi Shyam,
>
> Yes, dput() starts with might_sleep(), which means it may preemp (e.g.,
> due to disk I/O), so it must not be called while holding a spinlock.
>
> If you compile the kernel with CONFIG_DEBUG_ATOMIC_SLEEP=y you will see
> this kind of stack dump.
>
> [  305.667062][  T940] BUG: sleeping function called from invalid context at security/selinux/hooks.c:283
> [  305.668291][  T940] in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 940, name: ls
> [  305.669199][  T940] preempt_count: 1, expected: 0
> [  305.669493][  T940] RCU nest depth: 0, expected: 0
> [  305.670092][  T940] 3 locks held by ls/940:
> [  305.670362][  T940]  #0: ffff8881099b8f08 (&f->f_pos_lock){+.+.}-{4:4}, at: fdget_pos+0x18a/0x1c0
> [  305.671009][  T940]  #1: ffff88811c490158 (&type->i_mutex_dir_key#7){.+.+}-{4:4}, at: iterate_dir+0x85/0x270
> [  305.671615][  T940]  #2: ffff88810cc620b0 (&cfid->fid_lock){+.+.}-{3:3}, at: open_cached_dir+0x1098/0x14a0 [cifs]
> [ ... stack trace continues ... ]
>
That's a good point. I'll make sure to dput outside spinlocks.

> > >
> > > 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?
> >
> > Can you please elaborate which code path will result in this lock ordering?
>
> I was referring to the following pattern in cifs_laundromat_worker():
>
>   spin_lock(&cfid->fid_lock);
>   ...
>   spin_lock(&cifs_tcp_ses_lock);
>   spin_unlock(&cifs_tcp_ses_lock);
>   ...
>   spin_unlock(&cfid->fid_lock);
>
> This was more of an open question. I am not certain this causes any issues,
> and I could not find any concrete problem with it.
>
> I brought it up because cifs_tcp_ses_lock is a more global lock than
> cfid->fid_lock.
>
>
> Best,
> Henrique



-- 
Regards,
Shyam

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

[Enzo Matsumiya]

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
>>
>>