Re: [PATCH 4/7] cifs: serialize initialization and cleanup of cfid

[Paul Aurich <paul@darkrain42.org>]

On 2025-06-04 15:48:13 +0530, 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 94538f52dfc8..2746d693d80a 100644
>--- a/fs/smb/client/cached_dir.c
>+++ b/fs/smb/client/cached_dir.c
>@@ -198,6 +198,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).
>@@ -208,11 +214,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);
>@@ -229,6 +237,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);

Double mutex_unlock?  (It's also unlocked unconditionally in the 'out' path)

> 		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);
>@@ -452,6 +466,7 @@ smb2_close_cached_fid(struct kref *ref)
> 		if (rc) /* should we retry on -EBUSY or -EAGAIN? */
> 			cifs_dbg(VFS, "close cached dir rc %d\n", rc);
> 	}
>+	mutex_unlock(&cfid->cfid_mutex);
>
> 	free_cached_dir(cfid);
> }
>@@ -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
>

~Paul