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

Henrique Carvalho
2025-05-02T15:11:51Z

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