* [PATCH] smb: cached_dir.c: fix race in cfid release
@ 2025-05-02 18:01 Henrique Carvalho
2025-05-02 19:07 ` Steve French
0 siblings, 1 reply; 7+ messages in thread
From: Henrique Carvalho @ 2025-05-02 18:01 UTC (permalink / raw)
To: ematsumiya, sfrench
Cc: pc, ronniesahlberg, sprasad, paul, bharathsm, linux-cifs,
Henrique Carvalho
find_or_create_cached_dir() could grab a new reference after kref_put()
had seen the refcount drop to zero but before cfid_list_lock is acquired
in smb2_close_cached_fid(), leading to use-after-free.
Switch to kref_put_lock() so cfid_release() runs with cfid_list_lock
held, closing that gap.
While we are at it:
* rename the functions smb2_close_cached_fid() and close_cached_dir()
for clarity;
* replace the calls to kref_put() for cfid_put().
Signed-off-by: Henrique Carvalho <henrique.carvalho@suse.com>
---
fs/smb/client/cached_dir.c | 28 ++++++++++++++++------------
fs/smb/client/cached_dir.h | 2 +-
fs/smb/client/inode.c | 4 ++--
fs/smb/client/readdir.c | 4 ++--
fs/smb/client/smb2inode.c | 2 +-
fs/smb/client/smb2ops.c | 8 ++++----
6 files changed, 26 insertions(+), 22 deletions(-)
diff --git a/fs/smb/client/cached_dir.c b/fs/smb/client/cached_dir.c
index fe738623cf1b..ff4f9f8150cf 100644
--- a/fs/smb/client/cached_dir.c
+++ b/fs/smb/client/cached_dir.c
@@ -14,7 +14,7 @@
static struct cached_fid *init_cached_dir(const char *path);
static void free_cached_dir(struct cached_fid *cfid);
-static void smb2_close_cached_fid(struct kref *ref);
+static void cfid_release(struct kref *ref);
static void cfids_laundromat_worker(struct work_struct *work);
struct cached_dir_dentry {
@@ -370,11 +370,13 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
* lease. Release one here, and the second below.
*/
cfid->has_lease = false;
- kref_put(&cfid->refcount, smb2_close_cached_fid);
+
+ /* If this cfid_put calls back cfid_release the code is wrong anyway. */
+ cfid_put(cfid);
}
spin_unlock(&cfids->cfid_list_lock);
- kref_put(&cfid->refcount, smb2_close_cached_fid);
+ cfid_put(cfid);
} else {
*ret_cfid = cfid;
atomic_inc(&tcon->num_remote_opens);
@@ -413,13 +415,12 @@ int open_cached_dir_by_dentry(struct cifs_tcon *tcon,
}
static void
-smb2_close_cached_fid(struct kref *ref)
+cfid_release(struct kref *ref)
{
struct cached_fid *cfid = container_of(ref, struct cached_fid,
refcount);
int rc;
- spin_lock(&cfid->cfids->cfid_list_lock);
if (cfid->on_list) {
list_del(&cfid->entry);
cfid->on_list = false;
@@ -454,16 +455,19 @@ void drop_cached_dir_by_name(const unsigned int xid, struct cifs_tcon *tcon,
spin_lock(&cfid->cfids->cfid_list_lock);
if (cfid->has_lease) {
cfid->has_lease = false;
- kref_put(&cfid->refcount, smb2_close_cached_fid);
+
+ /* If this cfid_put calls back cfid_release the code is wrong anyway. */
+ cfid_put(cfid);
}
spin_unlock(&cfid->cfids->cfid_list_lock);
- close_cached_dir(cfid);
+ cfid_put(cfid);
}
-void close_cached_dir(struct cached_fid *cfid)
+void cfid_put(struct cached_fid *cfid)
{
- kref_put(&cfid->refcount, smb2_close_cached_fid);
+ struct cached_fids *cfids = cfid->tcon->cfids;
+ kref_put_lock(&cfid->refcount, cfid_release, &cfids->cfid_list_lock);
}
/*
@@ -564,7 +568,7 @@ cached_dir_offload_close(struct work_struct *work)
WARN_ON(cfid->on_list);
- kref_put(&cfid->refcount, smb2_close_cached_fid);
+ cfid_put(cfid);
cifs_put_tcon(tcon, netfs_trace_tcon_ref_put_cached_close);
}
@@ -688,7 +692,7 @@ static void cfids_invalidation_worker(struct work_struct *work)
list_for_each_entry_safe(cfid, q, &entry, entry) {
list_del(&cfid->entry);
/* Drop the ref-count acquired in invalidate_all_cached_dirs */
- kref_put(&cfid->refcount, smb2_close_cached_fid);
+ cfid_put(cfid);
}
}
@@ -741,7 +745,7 @@ static void cfids_laundromat_worker(struct work_struct *work)
* Drop the ref-count from above, either the lease-ref (if there
* was one) or the extra one acquired.
*/
- kref_put(&cfid->refcount, smb2_close_cached_fid);
+ cfid_put(cfid);
}
queue_delayed_work(cfid_put_wq, &cfids->laundromat_work,
dir_cache_timeout * HZ);
diff --git a/fs/smb/client/cached_dir.h b/fs/smb/client/cached_dir.h
index 1dfe79d947a6..f4fc7818dda5 100644
--- a/fs/smb/client/cached_dir.h
+++ b/fs/smb/client/cached_dir.h
@@ -73,7 +73,7 @@ extern int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
extern int open_cached_dir_by_dentry(struct cifs_tcon *tcon,
struct dentry *dentry,
struct cached_fid **cfid);
-extern void close_cached_dir(struct cached_fid *cfid);
+extern void cfid_put(struct cached_fid *cfid);
extern void drop_cached_dir_by_name(const unsigned int xid,
struct cifs_tcon *tcon,
const char *name,
diff --git a/fs/smb/client/inode.c b/fs/smb/client/inode.c
index 75be4b46bc6f..c3ef1f93a80d 100644
--- a/fs/smb/client/inode.c
+++ b/fs/smb/client/inode.c
@@ -2610,10 +2610,10 @@ cifs_dentry_needs_reval(struct dentry *dentry)
if (!open_cached_dir_by_dentry(tcon, dentry->d_parent, &cfid)) {
if (cfid->time && cifs_i->time > cfid->time) {
- close_cached_dir(cfid);
+ cfid_put(cfid);
return false;
}
- close_cached_dir(cfid);
+ cfid_put(cfid);
}
/*
* depending on inode type, check if attribute caching disabled for
diff --git a/fs/smb/client/readdir.c b/fs/smb/client/readdir.c
index 50f96259d9ad..1e1768152803 100644
--- a/fs/smb/client/readdir.c
+++ b/fs/smb/client/readdir.c
@@ -1093,7 +1093,7 @@ int cifs_readdir(struct file *file, struct dir_context *ctx)
* find_cifs_entry in case there will be reconnects during
* query_directory.
*/
- close_cached_dir(cfid);
+ cfid_put(cfid);
cfid = NULL;
cache_not_found:
@@ -1199,7 +1199,7 @@ int cifs_readdir(struct file *file, struct dir_context *ctx)
rddir2_exit:
if (cfid)
- close_cached_dir(cfid);
+ cfid_put(cfid);
free_dentry_path(page);
free_xid(xid);
return rc;
diff --git a/fs/smb/client/smb2inode.c b/fs/smb/client/smb2inode.c
index 57d9bfbadd97..f805d71a8d19 100644
--- a/fs/smb/client/smb2inode.c
+++ b/fs/smb/client/smb2inode.c
@@ -975,7 +975,7 @@ int smb2_query_path_info(const unsigned int xid,
cfid->fid.volatile_fid,
&data->fi);
}
- close_cached_dir(cfid);
+ cfid_put(cfid);
return rc;
}
cmds[num_cmds++] = SMB2_OP_QUERY_INFO;
diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
index 2fe8eeb98535..97c4d44c9a69 100644
--- a/fs/smb/client/smb2ops.c
+++ b/fs/smb/client/smb2ops.c
@@ -889,7 +889,7 @@ smb3_qfs_tcon(const unsigned int xid, struct cifs_tcon *tcon,
if (cfid == NULL)
SMB2_close(xid, tcon, fid.persistent_fid, fid.volatile_fid);
else
- close_cached_dir(cfid);
+ cfid_put(cfid);
}
static void
@@ -940,10 +940,10 @@ smb2_is_path_accessible(const unsigned int xid, struct cifs_tcon *tcon,
rc = open_cached_dir(xid, tcon, full_path, cifs_sb, true, &cfid);
if (!rc) {
if (cfid->has_lease) {
- close_cached_dir(cfid);
+ cfid_put(cfid);
return 0;
}
- close_cached_dir(cfid);
+ cfid_put(cfid);
}
utf16_path = cifs_convert_path_to_utf16(full_path, cifs_sb);
@@ -2804,7 +2804,7 @@ smb2_query_info_compound(const unsigned int xid, struct cifs_tcon *tcon,
free_rsp_buf(resp_buftype[0], rsp_iov[0].iov_base);
free_rsp_buf(resp_buftype[2], rsp_iov[2].iov_base);
if (cfid)
- close_cached_dir(cfid);
+ cfid_put(cfid);
kfree(vars);
out_free_path:
kfree(utf16_path);
--
2.47.0
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH] smb: cached_dir.c: fix race in cfid release
2025-05-02 18:01 [PATCH] smb: cached_dir.c: fix race in cfid release Henrique Carvalho
@ 2025-05-02 19:07 ` Steve French
2025-05-02 19:58 ` Enzo Matsumiya
0 siblings, 1 reply; 7+ messages in thread
From: Steve French @ 2025-05-02 19:07 UTC (permalink / raw)
To: Henrique Carvalho
Cc: ematsumiya, pc, ronniesahlberg, sprasad, paul, bharathsm,
linux-cifs, Meetakshi Setiya
I fixed a minor checkpatch warning but also noticed this compile
warning - is there a missing lock call?
cached_dir.c:429:20: warning: context imbalance in 'cfid_release' -
unexpected unlock
On Fri, May 2, 2025 at 1:04 PM Henrique Carvalho
<henrique.carvalho@suse.com> wrote:
>
> find_or_create_cached_dir() could grab a new reference after kref_put()
> had seen the refcount drop to zero but before cfid_list_lock is acquired
> in smb2_close_cached_fid(), leading to use-after-free.
>
> Switch to kref_put_lock() so cfid_release() runs with cfid_list_lock
> held, closing that gap.
>
> While we are at it:
> * rename the functions smb2_close_cached_fid() and close_cached_dir()
> for clarity;
> * replace the calls to kref_put() for cfid_put().
>
> Signed-off-by: Henrique Carvalho <henrique.carvalho@suse.com>
> ---
> fs/smb/client/cached_dir.c | 28 ++++++++++++++++------------
> fs/smb/client/cached_dir.h | 2 +-
> fs/smb/client/inode.c | 4 ++--
> fs/smb/client/readdir.c | 4 ++--
> fs/smb/client/smb2inode.c | 2 +-
> fs/smb/client/smb2ops.c | 8 ++++----
> 6 files changed, 26 insertions(+), 22 deletions(-)
>
> diff --git a/fs/smb/client/cached_dir.c b/fs/smb/client/cached_dir.c
> index fe738623cf1b..ff4f9f8150cf 100644
> --- a/fs/smb/client/cached_dir.c
> +++ b/fs/smb/client/cached_dir.c
> @@ -14,7 +14,7 @@
>
> static struct cached_fid *init_cached_dir(const char *path);
> static void free_cached_dir(struct cached_fid *cfid);
> -static void smb2_close_cached_fid(struct kref *ref);
> +static void cfid_release(struct kref *ref);
> static void cfids_laundromat_worker(struct work_struct *work);
>
> struct cached_dir_dentry {
> @@ -370,11 +370,13 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
> * lease. Release one here, and the second below.
> */
> cfid->has_lease = false;
> - kref_put(&cfid->refcount, smb2_close_cached_fid);
> +
> + /* If this cfid_put calls back cfid_release the code is wrong anyway. */
> + cfid_put(cfid);
> }
> spin_unlock(&cfids->cfid_list_lock);
>
> - kref_put(&cfid->refcount, smb2_close_cached_fid);
> + cfid_put(cfid);
> } else {
> *ret_cfid = cfid;
> atomic_inc(&tcon->num_remote_opens);
> @@ -413,13 +415,12 @@ int open_cached_dir_by_dentry(struct cifs_tcon *tcon,
> }
>
> static void
> -smb2_close_cached_fid(struct kref *ref)
> +cfid_release(struct kref *ref)
> {
> struct cached_fid *cfid = container_of(ref, struct cached_fid,
> refcount);
> int rc;
>
> - spin_lock(&cfid->cfids->cfid_list_lock);
> if (cfid->on_list) {
> list_del(&cfid->entry);
> cfid->on_list = false;
> @@ -454,16 +455,19 @@ void drop_cached_dir_by_name(const unsigned int xid, struct cifs_tcon *tcon,
> spin_lock(&cfid->cfids->cfid_list_lock);
> if (cfid->has_lease) {
> cfid->has_lease = false;
> - kref_put(&cfid->refcount, smb2_close_cached_fid);
> +
> + /* If this cfid_put calls back cfid_release the code is wrong anyway. */
> + cfid_put(cfid);
> }
> spin_unlock(&cfid->cfids->cfid_list_lock);
> - close_cached_dir(cfid);
> + cfid_put(cfid);
> }
>
>
> -void close_cached_dir(struct cached_fid *cfid)
> +void cfid_put(struct cached_fid *cfid)
> {
> - kref_put(&cfid->refcount, smb2_close_cached_fid);
> + struct cached_fids *cfids = cfid->tcon->cfids;
> + kref_put_lock(&cfid->refcount, cfid_release, &cfids->cfid_list_lock);
> }
>
> /*
> @@ -564,7 +568,7 @@ cached_dir_offload_close(struct work_struct *work)
>
> WARN_ON(cfid->on_list);
>
> - kref_put(&cfid->refcount, smb2_close_cached_fid);
> + cfid_put(cfid);
> cifs_put_tcon(tcon, netfs_trace_tcon_ref_put_cached_close);
> }
>
> @@ -688,7 +692,7 @@ static void cfids_invalidation_worker(struct work_struct *work)
> list_for_each_entry_safe(cfid, q, &entry, entry) {
> list_del(&cfid->entry);
> /* Drop the ref-count acquired in invalidate_all_cached_dirs */
> - kref_put(&cfid->refcount, smb2_close_cached_fid);
> + cfid_put(cfid);
> }
> }
>
> @@ -741,7 +745,7 @@ static void cfids_laundromat_worker(struct work_struct *work)
> * Drop the ref-count from above, either the lease-ref (if there
> * was one) or the extra one acquired.
> */
> - kref_put(&cfid->refcount, smb2_close_cached_fid);
> + cfid_put(cfid);
> }
> queue_delayed_work(cfid_put_wq, &cfids->laundromat_work,
> dir_cache_timeout * HZ);
> diff --git a/fs/smb/client/cached_dir.h b/fs/smb/client/cached_dir.h
> index 1dfe79d947a6..f4fc7818dda5 100644
> --- a/fs/smb/client/cached_dir.h
> +++ b/fs/smb/client/cached_dir.h
> @@ -73,7 +73,7 @@ extern int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
> extern int open_cached_dir_by_dentry(struct cifs_tcon *tcon,
> struct dentry *dentry,
> struct cached_fid **cfid);
> -extern void close_cached_dir(struct cached_fid *cfid);
> +extern void cfid_put(struct cached_fid *cfid);
> extern void drop_cached_dir_by_name(const unsigned int xid,
> struct cifs_tcon *tcon,
> const char *name,
> diff --git a/fs/smb/client/inode.c b/fs/smb/client/inode.c
> index 75be4b46bc6f..c3ef1f93a80d 100644
> --- a/fs/smb/client/inode.c
> +++ b/fs/smb/client/inode.c
> @@ -2610,10 +2610,10 @@ cifs_dentry_needs_reval(struct dentry *dentry)
>
> if (!open_cached_dir_by_dentry(tcon, dentry->d_parent, &cfid)) {
> if (cfid->time && cifs_i->time > cfid->time) {
> - close_cached_dir(cfid);
> + cfid_put(cfid);
> return false;
> }
> - close_cached_dir(cfid);
> + cfid_put(cfid);
> }
> /*
> * depending on inode type, check if attribute caching disabled for
> diff --git a/fs/smb/client/readdir.c b/fs/smb/client/readdir.c
> index 50f96259d9ad..1e1768152803 100644
> --- a/fs/smb/client/readdir.c
> +++ b/fs/smb/client/readdir.c
> @@ -1093,7 +1093,7 @@ int cifs_readdir(struct file *file, struct dir_context *ctx)
> * find_cifs_entry in case there will be reconnects during
> * query_directory.
> */
> - close_cached_dir(cfid);
> + cfid_put(cfid);
> cfid = NULL;
>
> cache_not_found:
> @@ -1199,7 +1199,7 @@ int cifs_readdir(struct file *file, struct dir_context *ctx)
>
> rddir2_exit:
> if (cfid)
> - close_cached_dir(cfid);
> + cfid_put(cfid);
> free_dentry_path(page);
> free_xid(xid);
> return rc;
> diff --git a/fs/smb/client/smb2inode.c b/fs/smb/client/smb2inode.c
> index 57d9bfbadd97..f805d71a8d19 100644
> --- a/fs/smb/client/smb2inode.c
> +++ b/fs/smb/client/smb2inode.c
> @@ -975,7 +975,7 @@ int smb2_query_path_info(const unsigned int xid,
> cfid->fid.volatile_fid,
> &data->fi);
> }
> - close_cached_dir(cfid);
> + cfid_put(cfid);
> return rc;
> }
> cmds[num_cmds++] = SMB2_OP_QUERY_INFO;
> diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
> index 2fe8eeb98535..97c4d44c9a69 100644
> --- a/fs/smb/client/smb2ops.c
> +++ b/fs/smb/client/smb2ops.c
> @@ -889,7 +889,7 @@ smb3_qfs_tcon(const unsigned int xid, struct cifs_tcon *tcon,
> if (cfid == NULL)
> SMB2_close(xid, tcon, fid.persistent_fid, fid.volatile_fid);
> else
> - close_cached_dir(cfid);
> + cfid_put(cfid);
> }
>
> static void
> @@ -940,10 +940,10 @@ smb2_is_path_accessible(const unsigned int xid, struct cifs_tcon *tcon,
> rc = open_cached_dir(xid, tcon, full_path, cifs_sb, true, &cfid);
> if (!rc) {
> if (cfid->has_lease) {
> - close_cached_dir(cfid);
> + cfid_put(cfid);
> return 0;
> }
> - close_cached_dir(cfid);
> + cfid_put(cfid);
> }
>
> utf16_path = cifs_convert_path_to_utf16(full_path, cifs_sb);
> @@ -2804,7 +2804,7 @@ smb2_query_info_compound(const unsigned int xid, struct cifs_tcon *tcon,
> free_rsp_buf(resp_buftype[0], rsp_iov[0].iov_base);
> free_rsp_buf(resp_buftype[2], rsp_iov[2].iov_base);
> if (cfid)
> - close_cached_dir(cfid);
> + cfid_put(cfid);
> kfree(vars);
> out_free_path:
> kfree(utf16_path);
> --
> 2.47.0
>
>
--
Thanks,
Steve
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] smb: cached_dir.c: fix race in cfid release
2025-05-02 19:07 ` Steve French
@ 2025-05-02 19:58 ` Enzo Matsumiya
2025-05-02 20:20 ` Henrique Carvalho
0 siblings, 1 reply; 7+ messages in thread
From: Enzo Matsumiya @ 2025-05-02 19:58 UTC (permalink / raw)
To: Steve French
Cc: Henrique Carvalho, pc, ronniesahlberg, sprasad, paul, bharathsm,
linux-cifs, Meetakshi Setiya
On 05/02, Steve French wrote:
>I fixed a minor checkpatch warning but also noticed this compile
>warning - is there a missing lock call?
>
>cached_dir.c:429:20: warning: context imbalance in 'cfid_release' -
>unexpected unlock
The lock is taken (inside kref_put_lock) if count == 0 (i.e. when the
release function is called) and must be released from within the
release function (which is done here).
However, sparse can't recognize this and also there doesn't seem to
exist an annotation to indicate so.
@Henrique do you think you could rework the patch to something like:
cfid_release() {
list_del();
on_list = false;
num_entries--;
}
cfid_put() {
lock();
if (kref_put(..., cfid_release)) {
unlock();
dput();
SMB2_close();
free_cached_dir();
return;
}
unlock();
}
Cheers,
Enzo
>On Fri, May 2, 2025 at 1:04 PM Henrique Carvalho
><henrique.carvalho@suse.com> wrote:
>>
>> find_or_create_cached_dir() could grab a new reference after kref_put()
>> had seen the refcount drop to zero but before cfid_list_lock is acquired
>> in smb2_close_cached_fid(), leading to use-after-free.
>>
>> Switch to kref_put_lock() so cfid_release() runs with cfid_list_lock
>> held, closing that gap.
>>
>> While we are at it:
>> * rename the functions smb2_close_cached_fid() and close_cached_dir()
>> for clarity;
>> * replace the calls to kref_put() for cfid_put().
>>
>> Signed-off-by: Henrique Carvalho <henrique.carvalho@suse.com>
>> ---
>> fs/smb/client/cached_dir.c | 28 ++++++++++++++++------------
>> fs/smb/client/cached_dir.h | 2 +-
>> fs/smb/client/inode.c | 4 ++--
>> fs/smb/client/readdir.c | 4 ++--
>> fs/smb/client/smb2inode.c | 2 +-
>> fs/smb/client/smb2ops.c | 8 ++++----
>> 6 files changed, 26 insertions(+), 22 deletions(-)
>>
>> diff --git a/fs/smb/client/cached_dir.c b/fs/smb/client/cached_dir.c
>> index fe738623cf1b..ff4f9f8150cf 100644
>> --- a/fs/smb/client/cached_dir.c
>> +++ b/fs/smb/client/cached_dir.c
>> @@ -14,7 +14,7 @@
>>
>> static struct cached_fid *init_cached_dir(const char *path);
>> static void free_cached_dir(struct cached_fid *cfid);
>> -static void smb2_close_cached_fid(struct kref *ref);
>> +static void cfid_release(struct kref *ref);
>> static void cfids_laundromat_worker(struct work_struct *work);
>>
>> struct cached_dir_dentry {
>> @@ -370,11 +370,13 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
>> * lease. Release one here, and the second below.
>> */
>> cfid->has_lease = false;
>> - kref_put(&cfid->refcount, smb2_close_cached_fid);
>> +
>> + /* If this cfid_put calls back cfid_release the code is wrong anyway. */
>> + cfid_put(cfid);
>> }
>> spin_unlock(&cfids->cfid_list_lock);
>>
>> - kref_put(&cfid->refcount, smb2_close_cached_fid);
>> + cfid_put(cfid);
>> } else {
>> *ret_cfid = cfid;
>> atomic_inc(&tcon->num_remote_opens);
>> @@ -413,13 +415,12 @@ int open_cached_dir_by_dentry(struct cifs_tcon *tcon,
>> }
>>
>> static void
>> -smb2_close_cached_fid(struct kref *ref)
>> +cfid_release(struct kref *ref)
>> {
>> struct cached_fid *cfid = container_of(ref, struct cached_fid,
>> refcount);
>> int rc;
>>
>> - spin_lock(&cfid->cfids->cfid_list_lock);
>> if (cfid->on_list) {
>> list_del(&cfid->entry);
>> cfid->on_list = false;
>> @@ -454,16 +455,19 @@ void drop_cached_dir_by_name(const unsigned int xid, struct cifs_tcon *tcon,
>> spin_lock(&cfid->cfids->cfid_list_lock);
>> if (cfid->has_lease) {
>> cfid->has_lease = false;
>> - kref_put(&cfid->refcount, smb2_close_cached_fid);
>> +
>> + /* If this cfid_put calls back cfid_release the code is wrong anyway. */
>> + cfid_put(cfid);
>> }
>> spin_unlock(&cfid->cfids->cfid_list_lock);
>> - close_cached_dir(cfid);
>> + cfid_put(cfid);
>> }
>>
>>
>> -void close_cached_dir(struct cached_fid *cfid)
>> +void cfid_put(struct cached_fid *cfid)
>> {
>> - kref_put(&cfid->refcount, smb2_close_cached_fid);
>> + struct cached_fids *cfids = cfid->tcon->cfids;
>> + kref_put_lock(&cfid->refcount, cfid_release, &cfids->cfid_list_lock);
>> }
>>
>> /*
>> @@ -564,7 +568,7 @@ cached_dir_offload_close(struct work_struct *work)
>>
>> WARN_ON(cfid->on_list);
>>
>> - kref_put(&cfid->refcount, smb2_close_cached_fid);
>> + cfid_put(cfid);
>> cifs_put_tcon(tcon, netfs_trace_tcon_ref_put_cached_close);
>> }
>>
>> @@ -688,7 +692,7 @@ static void cfids_invalidation_worker(struct work_struct *work)
>> list_for_each_entry_safe(cfid, q, &entry, entry) {
>> list_del(&cfid->entry);
>> /* Drop the ref-count acquired in invalidate_all_cached_dirs */
>> - kref_put(&cfid->refcount, smb2_close_cached_fid);
>> + cfid_put(cfid);
>> }
>> }
>>
>> @@ -741,7 +745,7 @@ static void cfids_laundromat_worker(struct work_struct *work)
>> * Drop the ref-count from above, either the lease-ref (if there
>> * was one) or the extra one acquired.
>> */
>> - kref_put(&cfid->refcount, smb2_close_cached_fid);
>> + cfid_put(cfid);
>> }
>> queue_delayed_work(cfid_put_wq, &cfids->laundromat_work,
>> dir_cache_timeout * HZ);
>> diff --git a/fs/smb/client/cached_dir.h b/fs/smb/client/cached_dir.h
>> index 1dfe79d947a6..f4fc7818dda5 100644
>> --- a/fs/smb/client/cached_dir.h
>> +++ b/fs/smb/client/cached_dir.h
>> @@ -73,7 +73,7 @@ extern int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
>> extern int open_cached_dir_by_dentry(struct cifs_tcon *tcon,
>> struct dentry *dentry,
>> struct cached_fid **cfid);
>> -extern void close_cached_dir(struct cached_fid *cfid);
>> +extern void cfid_put(struct cached_fid *cfid);
>> extern void drop_cached_dir_by_name(const unsigned int xid,
>> struct cifs_tcon *tcon,
>> const char *name,
>> diff --git a/fs/smb/client/inode.c b/fs/smb/client/inode.c
>> index 75be4b46bc6f..c3ef1f93a80d 100644
>> --- a/fs/smb/client/inode.c
>> +++ b/fs/smb/client/inode.c
>> @@ -2610,10 +2610,10 @@ cifs_dentry_needs_reval(struct dentry *dentry)
>>
>> if (!open_cached_dir_by_dentry(tcon, dentry->d_parent, &cfid)) {
>> if (cfid->time && cifs_i->time > cfid->time) {
>> - close_cached_dir(cfid);
>> + cfid_put(cfid);
>> return false;
>> }
>> - close_cached_dir(cfid);
>> + cfid_put(cfid);
>> }
>> /*
>> * depending on inode type, check if attribute caching disabled for
>> diff --git a/fs/smb/client/readdir.c b/fs/smb/client/readdir.c
>> index 50f96259d9ad..1e1768152803 100644
>> --- a/fs/smb/client/readdir.c
>> +++ b/fs/smb/client/readdir.c
>> @@ -1093,7 +1093,7 @@ int cifs_readdir(struct file *file, struct dir_context *ctx)
>> * find_cifs_entry in case there will be reconnects during
>> * query_directory.
>> */
>> - close_cached_dir(cfid);
>> + cfid_put(cfid);
>> cfid = NULL;
>>
>> cache_not_found:
>> @@ -1199,7 +1199,7 @@ int cifs_readdir(struct file *file, struct dir_context *ctx)
>>
>> rddir2_exit:
>> if (cfid)
>> - close_cached_dir(cfid);
>> + cfid_put(cfid);
>> free_dentry_path(page);
>> free_xid(xid);
>> return rc;
>> diff --git a/fs/smb/client/smb2inode.c b/fs/smb/client/smb2inode.c
>> index 57d9bfbadd97..f805d71a8d19 100644
>> --- a/fs/smb/client/smb2inode.c
>> +++ b/fs/smb/client/smb2inode.c
>> @@ -975,7 +975,7 @@ int smb2_query_path_info(const unsigned int xid,
>> cfid->fid.volatile_fid,
>> &data->fi);
>> }
>> - close_cached_dir(cfid);
>> + cfid_put(cfid);
>> return rc;
>> }
>> cmds[num_cmds++] = SMB2_OP_QUERY_INFO;
>> diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
>> index 2fe8eeb98535..97c4d44c9a69 100644
>> --- a/fs/smb/client/smb2ops.c
>> +++ b/fs/smb/client/smb2ops.c
>> @@ -889,7 +889,7 @@ smb3_qfs_tcon(const unsigned int xid, struct cifs_tcon *tcon,
>> if (cfid == NULL)
>> SMB2_close(xid, tcon, fid.persistent_fid, fid.volatile_fid);
>> else
>> - close_cached_dir(cfid);
>> + cfid_put(cfid);
>> }
>>
>> static void
>> @@ -940,10 +940,10 @@ smb2_is_path_accessible(const unsigned int xid, struct cifs_tcon *tcon,
>> rc = open_cached_dir(xid, tcon, full_path, cifs_sb, true, &cfid);
>> if (!rc) {
>> if (cfid->has_lease) {
>> - close_cached_dir(cfid);
>> + cfid_put(cfid);
>> return 0;
>> }
>> - close_cached_dir(cfid);
>> + cfid_put(cfid);
>> }
>>
>> utf16_path = cifs_convert_path_to_utf16(full_path, cifs_sb);
>> @@ -2804,7 +2804,7 @@ smb2_query_info_compound(const unsigned int xid, struct cifs_tcon *tcon,
>> free_rsp_buf(resp_buftype[0], rsp_iov[0].iov_base);
>> free_rsp_buf(resp_buftype[2], rsp_iov[2].iov_base);
>> if (cfid)
>> - close_cached_dir(cfid);
>> + cfid_put(cfid);
>> kfree(vars);
>> out_free_path:
>> kfree(utf16_path);
>> --
>> 2.47.0
>>
>>
>
>
>--
>Thanks,
>
>Steve
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] smb: cached_dir.c: fix race in cfid release
2025-05-02 19:58 ` Enzo Matsumiya
@ 2025-05-02 20:20 ` Henrique Carvalho
2025-05-02 20:59 ` Henrique Carvalho
0 siblings, 1 reply; 7+ messages in thread
From: Henrique Carvalho @ 2025-05-02 20:20 UTC (permalink / raw)
To: Enzo Matsumiya
Cc: Steve French, pc, ronniesahlberg, sprasad, paul, bharathsm,
linux-cifs, Meetakshi Setiya
On Fri, May 02, 2025 at 04:58:00PM -0300, Enzo Matsumiya wrote:
> On 05/02, Steve French wrote:
> > I fixed a minor checkpatch warning but also noticed this compile
> > warning - is there a missing lock call?
> >
> > cached_dir.c:429:20: warning: context imbalance in 'cfid_release' -
> > unexpected unlock
>
> The lock is taken (inside kref_put_lock) if count == 0 (i.e. when the
> release function is called) and must be released from within the
> release function (which is done here).
>
> However, sparse can't recognize this and also there doesn't seem to
> exist an annotation to indicate so.
>
> @Henrique do you think you could rework the patch to something like:
>
> cfid_release() {
> list_del();
> on_list = false;
> num_entries--;
> }
>
> cfid_put() {
> lock();
> if (kref_put(..., cfid_release)) {
> unlock();
> dput();
> SMB2_close();
> free_cached_dir();
> return;
> }
> unlock();
> }
>
@Enzo, good idea. I will rework the patch.
Henrique
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] smb: cached_dir.c: fix race in cfid release
2025-05-02 20:20 ` Henrique Carvalho
@ 2025-05-02 20:59 ` Henrique Carvalho
2025-05-02 21:54 ` Steve French
2025-05-02 22:41 ` Henrique Carvalho
0 siblings, 2 replies; 7+ messages in thread
From: Henrique Carvalho @ 2025-05-02 20:59 UTC (permalink / raw)
To: Enzo Matsumiya
Cc: Steve French, pc, ronniesahlberg, sprasad, paul, bharathsm,
linux-cifs, Meetakshi Setiya
On Fri, May 02, 2025 at 05:20:31PM -0300, Henrique Carvalho wrote:
> On Fri, May 02, 2025 at 04:58:00PM -0300, Enzo Matsumiya wrote:
> > On 05/02, Steve French wrote:
> > > I fixed a minor checkpatch warning but also noticed this compile
> > > warning - is there a missing lock call?
> > >
> > > cached_dir.c:429:20: warning: context imbalance in 'cfid_release' -
> > > unexpected unlock
> >
> > The lock is taken (inside kref_put_lock) if count == 0 (i.e. when the
> > release function is called) and must be released from within the
> > release function (which is done here).
> >
> > However, sparse can't recognize this and also there doesn't seem to
> > exist an annotation to indicate so.
> >
> > @Henrique do you think you could rework the patch to something like:
> >
> > cfid_release() {
> > list_del();
> > on_list = false;
> > num_entries--;
> > }
> >
> > cfid_put() {
> > lock();
> > if (kref_put(..., cfid_release)) {
> > unlock();
> > dput();
> > SMB2_close();
> > free_cached_dir();
> > return;
> > }
> > unlock();
> > }
> >
>
> @Enzo, good idea. I will rework the patch.
>
Actually, this change would prevent me from calling cfid_put() with the
lock held in cases where the kref does *not* reach 0 and the release
function isn't supposed to run. While it could work, the code won't be
as elegant.
I’m open to suggestions if there's a way to preserve that behavior
while satisfying sparse.
In the meantime, I'm reviewing similar discussions on other mailing
lists to see if there are known solutions.
Henrique
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] smb: cached_dir.c: fix race in cfid release
2025-05-02 20:59 ` Henrique Carvalho
@ 2025-05-02 21:54 ` Steve French
2025-05-02 22:41 ` Henrique Carvalho
1 sibling, 0 replies; 7+ messages in thread
From: Steve French @ 2025-05-02 21:54 UTC (permalink / raw)
To: Henrique Carvalho
Cc: Enzo Matsumiya, pc, ronniesahlberg, sprasad, paul, bharathsm,
linux-cifs, Meetakshi Setiya
The patch does work around the hang/crash after generic/241 but a few
other tests failed unexpectedly (could be unrelated bugs) to windows
multichannel
http://smb311-linux-testing.southcentralus.cloudapp.azure.com/#/builders/12/builds/32
On Fri, May 2, 2025 at 4:01 PM Henrique Carvalho
<henrique.carvalho@suse.com> wrote:
>
> On Fri, May 02, 2025 at 05:20:31PM -0300, Henrique Carvalho wrote:
> > On Fri, May 02, 2025 at 04:58:00PM -0300, Enzo Matsumiya wrote:
> > > On 05/02, Steve French wrote:
> > > > I fixed a minor checkpatch warning but also noticed this compile
> > > > warning - is there a missing lock call?
> > > >
> > > > cached_dir.c:429:20: warning: context imbalance in 'cfid_release' -
> > > > unexpected unlock
> > >
> > > The lock is taken (inside kref_put_lock) if count == 0 (i.e. when the
> > > release function is called) and must be released from within the
> > > release function (which is done here).
> > >
> > > However, sparse can't recognize this and also there doesn't seem to
> > > exist an annotation to indicate so.
> > >
> > > @Henrique do you think you could rework the patch to something like:
> > >
> > > cfid_release() {
> > > list_del();
> > > on_list = false;
> > > num_entries--;
> > > }
> > >
> > > cfid_put() {
> > > lock();
> > > if (kref_put(..., cfid_release)) {
> > > unlock();
> > > dput();
> > > SMB2_close();
> > > free_cached_dir();
> > > return;
> > > }
> > > unlock();
> > > }
> > >
> >
> > @Enzo, good idea. I will rework the patch.
> >
>
> Actually, this change would prevent me from calling cfid_put() with the
> lock held in cases where the kref does *not* reach 0 and the release
> function isn't supposed to run. While it could work, the code won't be
> as elegant.
>
> I’m open to suggestions if there's a way to preserve that behavior
> while satisfying sparse.
>
> In the meantime, I'm reviewing similar discussions on other mailing
> lists to see if there are known solutions.
>
>
> Henrique
--
Thanks,
Steve
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] smb: cached_dir.c: fix race in cfid release
2025-05-02 20:59 ` Henrique Carvalho
2025-05-02 21:54 ` Steve French
@ 2025-05-02 22:41 ` Henrique Carvalho
1 sibling, 0 replies; 7+ messages in thread
From: Henrique Carvalho @ 2025-05-02 22:41 UTC (permalink / raw)
To: Enzo Matsumiya, smfrench
Cc: Steve French, pc, ronniesahlberg, sprasad, paul, bharathsm,
linux-cifs, Meetakshi Setiya
Using __releases() annotation seemed to work, I am sending a second
version of the patch.
Henrique
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2025-05-02 22:43 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-05-02 18:01 [PATCH] smb: cached_dir.c: fix race in cfid release Henrique Carvalho
2025-05-02 19:07 ` Steve French
2025-05-02 19:58 ` Enzo Matsumiya
2025-05-02 20:20 ` Henrique Carvalho
2025-05-02 20:59 ` Henrique Carvalho
2025-05-02 21:54 ` Steve French
2025-05-02 22:41 ` Henrique Carvalho
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox