* Re: [PATCH] smb: client: avoid dentry leak by not overwriting cfid->dentry
2025-05-07 6:16 ` Paul Aurich
@ 2025-05-07 14:03 ` Henrique Carvalho
[not found] ` <CAH2r5muyz7zY=+Fgrtc_zOA6GR1ZSGpR-Z4pFzgqmfszhnywWQ@mail.gmail.com>
` (2 subsequent siblings)
3 siblings, 0 replies; 9+ messages in thread
From: Henrique Carvalho @ 2025-05-07 14:03 UTC (permalink / raw)
To: Paul Aurich
Cc: ematsumiya, sfrench, smfrench, pc, ronniesahlberg, sprasad,
bharathsm, linux-cifs
On Tue, May 06, 2025 at 11:16:46PM -0700, Paul Aurich wrote:
> On 2025-05-06 19:31:56 -0300, Henrique Carvalho wrote:
> > A race, likely between lease break and open, can cause cfid->dentry to
> > be valid when open_cached_dir() tries to set it again. This overwrites
> > the old dentry without dput(), leaking it.
> >
> > Skip assignment if cfid->dentry is already set.
> >
> > Signed-off-by: Henrique Carvalho <henrique.carvalho@suse.com>
> > ---
> > fs/smb/client/cached_dir.c | 23 +++++++++++++++--------
> > 1 file changed, 15 insertions(+), 8 deletions(-)
> >
> > diff --git a/fs/smb/client/cached_dir.c b/fs/smb/client/cached_dir.c
> > index 43228ec2424d..8c1f00a3fc29 100644
> > --- a/fs/smb/client/cached_dir.c
> > +++ b/fs/smb/client/cached_dir.c
> > @@ -219,16 +219,23 @@ 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;
> > + /*
> > + * BB: cfid->dentry should be NULL here; if not, we're likely racing with
> > + * a lease break. This is a temporary workaround to avoid overwriting
> > + * a valid dentry. Needs proper fix.
> > + */
>
> Ah ha. I think this is trying to address the same race as Shyam's 'cifs: do
> not return an invalidated cfid' [1].
>
> What about modifying open_cached_dir to hold cfid_list_lock across the call
> to find_or_create_cached_dir through where it tests for validity, and then
> dropping the locking in find_or_create_cached_dir itself (see attached in
> case my text description isn't clear)?
>
> That's the only way I can see that a pre-existing cfid could escape to the
> rest of open_cached_dir. I think.
>
> ~Paul
>
> [1] https://lore.kernel.org/linux-cifs/20250502051517.10449-2-sprasad@microsoft.com/T/#u
>
> > + if (!cfid->dentry) {
> > + 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->dentry = dentry;
> > cfid->tcon = tcon;
> >
> > /*
> > --
> > 2.47.0
> >From 583824153dd901f1f7d63ce9364d32c9c249fd43 Mon Sep 17 00:00:00 2001
> From: Paul Aurich <paul@darkrain42.org>
> Date: Tue, 6 May 2025 22:28:09 -0700
> Subject: [PATCH] smb: client: Avoid race in open_cached_dir with lease breaks
>
> A pre-existing valid cfid returned from find_or_create_cached_dir might
> race with a lease break, meaning open_cached_dir doesn't consider it
> valid, and thinks it's newly-constructed. This leaks a dentry reference
> if the allocation occurs before the queued lease break work runs.
>
> Avoid the race by extending holding the cfid_list_lock across
> find_or_create_cached_dir and when the result is checked.
>
> Signed-off-by: Paul Aurich <paul@darkrain42.org>
> Cc: stable@vger.kernel.org
> ---
> fs/smb/client/cached_dir.c | 10 ++--------
> 1 file changed, 2 insertions(+), 8 deletions(-)
>
> diff --git a/fs/smb/client/cached_dir.c b/fs/smb/client/cached_dir.c
> index 10aad87ce679..cb8477c18905 100644
> --- a/fs/smb/client/cached_dir.c
> +++ b/fs/smb/client/cached_dir.c
> @@ -27,38 +27,32 @@ static struct cached_fid *find_or_create_cached_dir(struct cached_fids *cfids,
> bool lookup_only,
> __u32 max_cached_dirs)
> {
> struct cached_fid *cfid;
>
> - spin_lock(&cfids->cfid_list_lock);
> list_for_each_entry(cfid, &cfids->entries, entry) {
> if (!strcmp(cfid->path, path)) {
> /*
> * If it doesn't have a lease it is either not yet
> * fully cached or it may be in the process of
> * being deleted due to a lease break.
> */
> if (!cfid->time || !cfid->has_lease) {
> - spin_unlock(&cfids->cfid_list_lock);
> return NULL;
> }
> kref_get(&cfid->refcount);
> - spin_unlock(&cfids->cfid_list_lock);
> return cfid;
> }
> }
> if (lookup_only) {
> - spin_unlock(&cfids->cfid_list_lock);
> return NULL;
> }
> if (cfids->num_entries >= max_cached_dirs) {
> - spin_unlock(&cfids->cfid_list_lock);
> return NULL;
> }
> cfid = init_cached_dir(path);
> if (cfid == NULL) {
> - spin_unlock(&cfids->cfid_list_lock);
> return NULL;
> }
> cfid->cfids = cfids;
> cfids->num_entries++;
> list_add(&cfid->entry, &cfids->entries);
> @@ -72,11 +66,10 @@ static struct cached_fid *find_or_create_cached_dir(struct cached_fids *cfids,
> * Concurrent processes won't be to use it yet due to @cfid->time being
> * zero.
> */
> cfid->has_lease = true;
>
> - spin_unlock(&cfids->cfid_list_lock);
> return cfid;
> }
>
> static struct dentry *
> path_to_dentry(struct cifs_sb_info *cifs_sb, const char *path)
> @@ -185,21 +178,22 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
>
> utf16_path = cifs_convert_path_to_utf16(path, cifs_sb);
> if (!utf16_path)
> return -ENOMEM;
>
> + spin_lock(&cfids->cfid_list_lock);
> cfid = find_or_create_cached_dir(cfids, path, lookup_only, tcon->max_cached_dirs);
> if (cfid == NULL) {
> + spin_unlock(&cfids->cfid_list_lock);
> 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);
> if (cfid->has_lease && cfid->time) {
> spin_unlock(&cfids->cfid_list_lock);
> *ret_cfid = cfid;
> kfree(utf16_path);
> return 0;
> --
> 2.47.2
>
Yes!
Closing this gap seems to be the only viable fix.
I'm running the tests, but LGTM.
Best,
Henrique
^ permalink raw reply [flat|nested] 9+ messages in thread[parent not found: <CAH2r5muyz7zY=+Fgrtc_zOA6GR1ZSGpR-Z4pFzgqmfszhnywWQ@mail.gmail.com>]
* Fwd: [PATCH] smb: client: avoid dentry leak by not overwriting cfid->dentry
[not found] ` <CAH2r5muyz7zY=+Fgrtc_zOA6GR1ZSGpR-Z4pFzgqmfszhnywWQ@mail.gmail.com>
@ 2025-05-07 17:01 ` Steve French
[not found] ` <CAH2r5msisxGqZCFpJUu1Bqv5Kgo+-HD2DEO+wzQeSqG6TS2J6Q@mail.gmail.com>
1 sibling, 0 replies; 9+ messages in thread
From: Steve French @ 2025-05-07 17:01 UTC (permalink / raw)
To: CIFS
---------- Forwarded message ---------
From: Steve French <smfrench@gmail.com>
Date: Wed, May 7, 2025 at 11:31 AM
Subject: Re: [PATCH] smb: client: avoid dentry leak by not overwriting
cfid->dentry
To: Henrique Carvalho <henrique.carvalho@suse.com>, Enzo Matsumiya
<ematsumiya@suse.de>, Steve French <smfrench@gmail.com>, Shyam Prasad
N <sprasad@microsoft.com>, Bharath S M <bharathsm@microsoft.com>,
ronnie sahlberg <ronniesahlberg@gmail.com>, CIFS
<linux-cifs@vger.kernel.org>
I can try some test runs with Paul's patch. I wasn't clear on
whether it obsoletes Henrique's patch or if both would still be needed
though.
Is it ok to run with both patches
237d73fd2428 (HEAD -> for-next, origin/for-next, origin/HEAD) smb:
client: Avoid race in open_cached_dir with lease breaks
0b68d50bb6aa smb: client: fix delay on concurrent opens
419408103208 smb: client: avoid dentry leak by not overwriting cfid->dentry
d90b023718a1 smb3 client: warn when parse contexts returns error on
compounded operation
92a09c47464d (tag: v6.15-rc5, linus/master) Linux 6.15-rc5
With Henrique's patch there is a hang it looks like it introduces in
generic/013 to Azure with multichannel (no directory leases) which
seems strange
http://smb311-linux-testing.southcentralus.cloudapp.azure.com/#/builders/5/builds/443
but it does address the hang with directory leases after test
generic/241 with directory leases:
http://smb311-linux-testing.southcentralus.cloudapp.azure.com/#/builders/12/builds/48
and in the main test group, the only test which fails is the expected
one (the netfs regression) in generic/349 (I need to retry David's
updated netfs fixes for this)
http://smb311-linux-testing.southcentralus.cloudapp.azure.com/#/builders/3/builds/466
Thanks,
Steve
On Wed, May 7, 2025, 1:16 AM Paul Aurich <paul@darkrain42.org> wrote:
>
> On 2025-05-06 19:31:56 -0300, Henrique Carvalho wrote:
> >A race, likely between lease break and open, can cause cfid->dentry to
> >be valid when open_cached_dir() tries to set it again. This overwrites
> >the old dentry without dput(), leaking it.
> >
> >Skip assignment if cfid->dentry is already set.
> >
> >Signed-off-by: Henrique Carvalho <henrique.carvalho@suse.com>
> >---
> > fs/smb/client/cached_dir.c | 23 +++++++++++++++--------
> > 1 file changed, 15 insertions(+), 8 deletions(-)
> >
> >diff --git a/fs/smb/client/cached_dir.c b/fs/smb/client/cached_dir.c
> >index 43228ec2424d..8c1f00a3fc29 100644
> >--- a/fs/smb/client/cached_dir.c
> >+++ b/fs/smb/client/cached_dir.c
> >@@ -219,16 +219,23 @@ 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;
> >+ /*
> >+ * BB: cfid->dentry should be NULL here; if not, we're likely racing with
> >+ * a lease break. This is a temporary workaround to avoid overwriting
> >+ * a valid dentry. Needs proper fix.
> >+ */
>
> Ah ha. I think this is trying to address the same race as Shyam's 'cifs: do
> not return an invalidated cfid' [1].
>
> What about modifying open_cached_dir to hold cfid_list_lock across the call to
> find_or_create_cached_dir through where it tests for validity, and then
> dropping the locking in find_or_create_cached_dir itself (see attached in case
> my text description isn't clear)?
>
> That's the only way I can see that a pre-existing cfid could escape to the
> rest of open_cached_dir. I think.
>
> ~Paul
>
> [1] https://lore.kernel.org/linux-cifs/20250502051517.10449-2-sprasad@microsoft.com/T/#u
>
> >+ if (!cfid->dentry) {
> >+ 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->dentry = dentry;
> > cfid->tcon = tcon;
> >
> > /*
> >--
> >2.47.0
--
Thanks,
Steve
^ permalink raw reply [flat|nested] 9+ messages in thread[parent not found: <CAH2r5msisxGqZCFpJUu1Bqv5Kgo+-HD2DEO+wzQeSqG6TS2J6Q@mail.gmail.com>]
* Re: [PATCH] smb: client: avoid dentry leak by not overwriting cfid->dentry
[not found] ` <CAH2r5msisxGqZCFpJUu1Bqv5Kgo+-HD2DEO+wzQeSqG6TS2J6Q@mail.gmail.com>
@ 2025-05-07 19:38 ` Henrique Carvalho
0 siblings, 0 replies; 9+ messages in thread
From: Henrique Carvalho @ 2025-05-07 19:38 UTC (permalink / raw)
To: Steve French
Cc: Enzo Matsumiya, Shyam Prasad N, Bharath S M, ronnie sahlberg,
CIFS
On Wed, May 07, 2025 at 02:01:08PM -0500, Steve French wrote:
>
> With both patches I still see the hang in generic/013 to azure
> multichannel but it does still fix the directory lease crash
>
> [1]http://smb311-linux-testing.southcentralus.cloudapp.azure.com/#/buil
> ders/5/builds/444
I cannot see the mount options for this test, does it mount with
nohandlecache?
>
> Thanks,
>
> Steve
>
> On Wed, May 7, 2025, 11:31 AM Steve French <[2]smfrench@gmail.com>
> wrote:
>
> I can try some test runs with Paul's patch. I wasn't clear on whether
> it obsoletes Henrique's patch or if both would still be needed though.
> Is it ok to run with both patches
I believe Paul's patch makes mine obsolete, unless I'm missing something
-- or unless we want to be extra cautious by keeping the dentry check as
an additional safeguard.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] smb: client: avoid dentry leak by not overwriting cfid->dentry
2025-05-07 6:16 ` Paul Aurich
2025-05-07 14:03 ` Henrique Carvalho
[not found] ` <CAH2r5muyz7zY=+Fgrtc_zOA6GR1ZSGpR-Z4pFzgqmfszhnywWQ@mail.gmail.com>
@ 2025-05-07 23:56 ` Steve French
2025-05-08 15:24 ` Shyam Prasad N
3 siblings, 0 replies; 9+ messages in thread
From: Steve French @ 2025-05-07 23:56 UTC (permalink / raw)
To: Henrique Carvalho, ematsumiya, sfrench, smfrench, pc,
ronniesahlberg, sprasad, bharathsm, linux-cifs
So far Paul's patch is working - it avoided the generic/241 crash (dir
lease related)
http://smb311-linux-testing.southcentralus.cloudapp.azure.com/#/builders/12/builds/50
and the generic/013 multichannel hang
http://smb311-linux-testing.southcentralus.cloudapp.azure.com/#/builders/5/builds/445
Since Paul's patch avoids the most urgent problems and is small
enough, the larger longer term locking improvements should probably
just build on this (but some may have to wait for 6.16-rc)
On Wed, May 7, 2025 at 1:16 AM Paul Aurich <paul@darkrain42.org> wrote:
>
> On 2025-05-06 19:31:56 -0300, Henrique Carvalho wrote:
> >A race, likely between lease break and open, can cause cfid->dentry to
> >be valid when open_cached_dir() tries to set it again. This overwrites
> >the old dentry without dput(), leaking it.
> >
> >Skip assignment if cfid->dentry is already set.
> >
> >Signed-off-by: Henrique Carvalho <henrique.carvalho@suse.com>
> >---
> > fs/smb/client/cached_dir.c | 23 +++++++++++++++--------
> > 1 file changed, 15 insertions(+), 8 deletions(-)
> >
> >diff --git a/fs/smb/client/cached_dir.c b/fs/smb/client/cached_dir.c
> >index 43228ec2424d..8c1f00a3fc29 100644
> >--- a/fs/smb/client/cached_dir.c
> >+++ b/fs/smb/client/cached_dir.c
> >@@ -219,16 +219,23 @@ 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;
> >+ /*
> >+ * BB: cfid->dentry should be NULL here; if not, we're likely racing with
> >+ * a lease break. This is a temporary workaround to avoid overwriting
> >+ * a valid dentry. Needs proper fix.
> >+ */
>
> Ah ha. I think this is trying to address the same race as Shyam's 'cifs: do
> not return an invalidated cfid' [1].
>
> What about modifying open_cached_dir to hold cfid_list_lock across the call to
> find_or_create_cached_dir through where it tests for validity, and then
> dropping the locking in find_or_create_cached_dir itself (see attached in case
> my text description isn't clear)?
>
> That's the only way I can see that a pre-existing cfid could escape to the
> rest of open_cached_dir. I think.
>
> ~Paul
>
> [1] https://lore.kernel.org/linux-cifs/20250502051517.10449-2-sprasad@microsoft.com/T/#u
>
> >+ if (!cfid->dentry) {
> >+ 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->dentry = dentry;
> > cfid->tcon = tcon;
> >
> > /*
> >--
> >2.47.0
--
Thanks,
Steve
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] smb: client: avoid dentry leak by not overwriting cfid->dentry
2025-05-07 6:16 ` Paul Aurich
` (2 preceding siblings ...)
2025-05-07 23:56 ` Steve French
@ 2025-05-08 15:24 ` Shyam Prasad N
2025-05-08 15:53 ` Paul Aurich
3 siblings, 1 reply; 9+ messages in thread
From: Shyam Prasad N @ 2025-05-08 15:24 UTC (permalink / raw)
To: Henrique Carvalho, ematsumiya, sfrench, smfrench, pc,
ronniesahlberg, sprasad, bharathsm, linux-cifs
On Wed, May 7, 2025 at 11:56 AM Paul Aurich <paul@darkrain42.org> wrote:
>
> On 2025-05-06 19:31:56 -0300, Henrique Carvalho wrote:
> >A race, likely between lease break and open, can cause cfid->dentry to
> >be valid when open_cached_dir() tries to set it again. This overwrites
> >the old dentry without dput(), leaking it.
> >
> >Skip assignment if cfid->dentry is already set.
> >
> >Signed-off-by: Henrique Carvalho <henrique.carvalho@suse.com>
> >---
> > fs/smb/client/cached_dir.c | 23 +++++++++++++++--------
> > 1 file changed, 15 insertions(+), 8 deletions(-)
> >
> >diff --git a/fs/smb/client/cached_dir.c b/fs/smb/client/cached_dir.c
> >index 43228ec2424d..8c1f00a3fc29 100644
> >--- a/fs/smb/client/cached_dir.c
> >+++ b/fs/smb/client/cached_dir.c
> >@@ -219,16 +219,23 @@ 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;
> >+ /*
> >+ * BB: cfid->dentry should be NULL here; if not, we're likely racing with
> >+ * a lease break. This is a temporary workaround to avoid overwriting
> >+ * a valid dentry. Needs proper fix.
> >+ */
>
> Ah ha. I think this is trying to address the same race as Shyam's 'cifs: do
> not return an invalidated cfid' [1].
Hi Paul,
Yes. One of my patch did this check to avoid leaking dentry.
However, without serializing threads in this codepath, it is not
possible to rule out all such races.
>
> What about modifying open_cached_dir to hold cfid_list_lock across the call to
> find_or_create_cached_dir through where it tests for validity, and then
> dropping the locking in find_or_create_cached_dir itself (see attached in case
> my text description isn't clear)?
We can do that. But holding a spinlock till the response comes back
from the server is not a good idea.
We could see high CPU utilization if response from the server takes longer.
My other patch introduced a mutex just for this purpose.
>
> That's the only way I can see that a pre-existing cfid could escape to the
> rest of open_cached_dir. I think.
>
> ~Paul
>
> [1] https://lore.kernel.org/linux-cifs/20250502051517.10449-2-sprasad@microsoft.com/T/#u
>
> >+ if (!cfid->dentry) {
> >+ 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->dentry = dentry;
> > cfid->tcon = tcon;
> >
> > /*
> >--
> >2.47.0
--
Regards,
Shyam
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] smb: client: avoid dentry leak by not overwriting cfid->dentry
2025-05-08 15:24 ` Shyam Prasad N
@ 2025-05-08 15:53 ` Paul Aurich
2025-05-08 16:09 ` Steve French
0 siblings, 1 reply; 9+ messages in thread
From: Paul Aurich @ 2025-05-08 15:53 UTC (permalink / raw)
To: Shyam Prasad N
Cc: Henrique Carvalho, ematsumiya, sfrench, smfrench, pc,
ronniesahlberg, sprasad, bharathsm, linux-cifs
On 2025-05-08 20:54:34 +0530, Shyam Prasad N wrote:
>On Wed, May 7, 2025 at 11:56 AM Paul Aurich <paul@darkrain42.org> wrote:
>>
>> On 2025-05-06 19:31:56 -0300, Henrique Carvalho wrote:
>> >A race, likely between lease break and open, can cause cfid->dentry to
>> >be valid when open_cached_dir() tries to set it again. This overwrites
>> >the old dentry without dput(), leaking it.
>> >
>> >Skip assignment if cfid->dentry is already set.
>> >
>> >Signed-off-by: Henrique Carvalho <henrique.carvalho@suse.com>
>> >---
>> > fs/smb/client/cached_dir.c | 23 +++++++++++++++--------
>> > 1 file changed, 15 insertions(+), 8 deletions(-)
>> >
>> >diff --git a/fs/smb/client/cached_dir.c b/fs/smb/client/cached_dir.c
>> >index 43228ec2424d..8c1f00a3fc29 100644
>> >--- a/fs/smb/client/cached_dir.c
>> >+++ b/fs/smb/client/cached_dir.c
>> >@@ -219,16 +219,23 @@ 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;
>> >+ /*
>> >+ * BB: cfid->dentry should be NULL here; if not, we're likely racing with
>> >+ * a lease break. This is a temporary workaround to avoid overwriting
>> >+ * a valid dentry. Needs proper fix.
>> >+ */
>>
>> Ah ha. I think this is trying to address the same race as Shyam's 'cifs: do
>> not return an invalidated cfid' [1].
>
>
>Hi Paul,
>Yes. One of my patch did this check to avoid leaking dentry.
>However, without serializing threads in this codepath, it is not
>possible to rule out all such races.
>
>>
>> What about modifying open_cached_dir to hold cfid_list_lock across the call to
>> find_or_create_cached_dir through where it tests for validity, and then
>> dropping the locking in find_or_create_cached_dir itself (see attached in case
>> my text description isn't clear)?
>
>We can do that. But holding a spinlock till the response comes back
>from the server is not a good idea.
>We could see high CPU utilization if response from the server takes longer.
>My other patch introduced a mutex just for this purpose.
I don't think my proposed patch extends locking over server-side
communication or anything that can sleep/preempt. (To be clear about 'tests
for validity', I just meant the 'if (cfid->has_lease && cfid->time)' test that
occurs a few lines below the call to find_or_create_cached_dir)
Without my patch (i.e. before changes), find_or_create_cached_dir() holds
cfid_list_lock over its entire execution, but drops the lock before returning.
open_cached_dir() (the only caller of find_or_create_cached_dir) then the
spinlock again and checks if the cfid is valid.
The fix just moves the locking out of find_or_create_cached_dir() so it's
acquired _once_ and held across both searching for (or constructing a new)
cfid and then checking the results.
I don't think there are any intervening operations that would sleep?
@@ -185,21 +178,22 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
utf16_path = cifs_convert_path_to_utf16(path, cifs_sb);
if (!utf16_path)
return -ENOMEM;
+ spin_lock(&cfids->cfid_list_lock);
cfid = find_or_create_cached_dir(cfids, path, lookup_only, tcon->max_cached_dirs);
if (cfid == NULL) {
+ spin_unlock(&cfids->cfid_list_lock);
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);
if (cfid->has_lease && cfid->time) {
spin_unlock(&cfids->cfid_list_lock);
*ret_cfid = cfid;
kfree(utf16_path);
return 0;
Full patch:
https://git.samba.org/?p=sfrench/cifs-2.6.git;a=commitdiff;h=3ca02e63edccb78ef3659bebc68579c7224a6ca2
>
>>
>> That's the only way I can see that a pre-existing cfid could escape to the
>> rest of open_cached_dir. I think.
>>
>> ~Paul
>>
>> [1] https://lore.kernel.org/linux-cifs/20250502051517.10449-2-sprasad@microsoft.com/T/#u
>>
>> >+ if (!cfid->dentry) {
>> >+ 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->dentry = dentry;
>> > cfid->tcon = tcon;
>> >
>> > /*
>> >--
>> >2.47.0
>
>--
>Regards,
>Shyam
--
~Paul
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] smb: client: avoid dentry leak by not overwriting cfid->dentry
2025-05-08 15:53 ` Paul Aurich
@ 2025-05-08 16:09 ` Steve French
0 siblings, 0 replies; 9+ messages in thread
From: Steve French @ 2025-05-08 16:09 UTC (permalink / raw)
To: Shyam Prasad N, Henrique Carvalho, ematsumiya, sfrench, smfrench,
pc, ronniesahlberg, sprasad, bharathsm, linux-cifs
On Thu, May 8, 2025 at 10:53 AM Paul Aurich <paul@darkrain42.org> wrote:
>
> On 2025-05-08 20:54:34 +0530, Shyam Prasad N wrote:
> >On Wed, May 7, 2025 at 11:56 AM Paul Aurich <paul@darkrain42.org> wrote:
> >>
> >> On 2025-05-06 19:31:56 -0300, Henrique Carvalho wrote:
> >> >A race, likely between lease break and open, can cause cfid->dentry to
> >> >be valid when open_cached_dir() tries to set it again. This overwrites
> >> >the old dentry without dput(), leaking it.
> >> >
> >> >Skip assignment if cfid->dentry is already set.
> >> >
> >> >Signed-off-by: Henrique Carvalho <henrique.carvalho@suse.com>
> >> >---
> >> > fs/smb/client/cached_dir.c | 23 +++++++++++++++--------
> >> > 1 file changed, 15 insertions(+), 8 deletions(-)
> >> >
> >> >diff --git a/fs/smb/client/cached_dir.c b/fs/smb/client/cached_dir.c
> >> >index 43228ec2424d..8c1f00a3fc29 100644
> >> >--- a/fs/smb/client/cached_dir.c
> >> >+++ b/fs/smb/client/cached_dir.c
> >> >@@ -219,16 +219,23 @@ 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;
> >> >+ /*
> >> >+ * BB: cfid->dentry should be NULL here; if not, we're likely racing with
> >> >+ * a lease break. This is a temporary workaround to avoid overwriting
> >> >+ * a valid dentry. Needs proper fix.
> >> >+ */
> >>
> >> Ah ha. I think this is trying to address the same race as Shyam's 'cifs: do
> >> not return an invalidated cfid' [1].
> >
> >
> >Hi Paul,
> >Yes. One of my patch did this check to avoid leaking dentry.
> >However, without serializing threads in this codepath, it is not
> >possible to rule out all such races.
> >
> >>
> >> What about modifying open_cached_dir to hold cfid_list_lock across the call to
> >> find_or_create_cached_dir through where it tests for validity, and then
> >> dropping the locking in find_or_create_cached_dir itself (see attached in case
> >> my text description isn't clear)?
> >
> >We can do that. But holding a spinlock till the response comes back
> >from the server is not a good idea.
> >We could see high CPU utilization if response from the server takes longer.
> >My other patch introduced a mutex just for this purpose.
>
> I don't think my proposed patch extends locking over server-side
> communication or anything that can sleep/preempt. (To be clear about 'tests
> for validity', I just meant the 'if (cfid->has_lease && cfid->time)' test that
> occurs a few lines below the call to find_or_create_cached_dir)
>
> Without my patch (i.e. before changes), find_or_create_cached_dir() holds
> cfid_list_lock over its entire execution, but drops the lock before returning.
> open_cached_dir() (the only caller of find_or_create_cached_dir) then the
> spinlock again and checks if the cfid is valid.
>
> The fix just moves the locking out of find_or_create_cached_dir() so it's
> acquired _once_ and held across both searching for (or constructing a new)
> cfid and then checking the results.
>
> I don't think there are any intervening operations that would sleep?
I don't remember seeing any obvious slowdowns on individual xfstests
running with Paul's patch,
but we can rescan the test runs with and without his patch to make
sure we aren't slowing down
anything (and there is some perf randomness when running tests in the
cloud) - but I didn't see
anything suspicious from a perf perspective
--
Thanks,
Steve
^ permalink raw reply [flat|nested] 9+ messages in thread