fs/cachefiles/namei.c | 1 + 1 file changed, 1 insertion(+)
From: Baokun Li <libaokun1@huawei.com>
In ondemand mode, dentry leaks may be caused when the mount has the
following concurrency with cull:
P1 | P2
-----------------------------------------------------------
cachefiles_lookup_cookie
cachefiles_look_up_object
lookup_one_positive_unlocked
// get dentry
cachefiles_cull
inode->i_flags |= S_KERNEL_FILE;
cachefiles_open_file
cachefiles_mark_inode_in_use
__cachefiles_mark_inode_in_use
can_use = false
if (!(inode->i_flags & S_KERNEL_FILE))
can_use = true
return false
return false
// Returns an error but doesn't put dentry
After that the following WARNING will be triggered when the backend folder
is umounted:
==================================================================
BUG: Dentry 000000008ad87947{i=7a,n=Dx_1_1.img} still in use (1) [unmount of ext4 sda]
WARNING: CPU: 4 PID: 359261 at fs/dcache.c:1767 umount_check+0x5d/0x70
CPU: 4 PID: 359261 Comm: umount Not tainted 6.6.0-dirty #25
RIP: 0010:umount_check+0x5d/0x70
Call Trace:
<TASK>
d_walk+0xda/0x2b0
do_one_tree+0x20/0x40
shrink_dcache_for_umount+0x2c/0x90
generic_shutdown_super+0x20/0x160
kill_block_super+0x1a/0x40
ext4_kill_sb+0x22/0x40
deactivate_locked_super+0x35/0x80
cleanup_mnt+0x104/0x160
==================================================================
Add the missing dput() to cachefiles_open_file() for a quick fix.
Fixes: 1f08c925e7a3 ("cachefiles: Implement backing file wrangling")
Cc: stable@kernel.org
Signed-off-by: Baokun Li <libaokun1@huawei.com>
---
fs/cachefiles/namei.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/fs/cachefiles/namei.c b/fs/cachefiles/namei.c
index f53977169db4..0bd2f367c3ff 100644
--- a/fs/cachefiles/namei.c
+++ b/fs/cachefiles/namei.c
@@ -554,6 +554,7 @@ static bool cachefiles_open_file(struct cachefiles_object *object,
if (!cachefiles_mark_inode_in_use(object, d_inode(dentry))) {
pr_notice("cachefiles: Inode already in use: %pd (B=%lx)\n",
dentry, d_inode(dentry)->i_ino);
+ dput(dentry);
return false;
}
--
2.39.2…
> Add the missing dput() to cachefiles_open_file() for a quick fix.
I suggest to use a goto chain accordingly.
…
> +++ b/fs/cachefiles/namei.c
> @@ -554,6 +554,7 @@ static bool cachefiles_open_file(struct cachefiles_object *object,
> if (!cachefiles_mark_inode_in_use(object, d_inode(dentry))) {
> pr_notice("cachefiles: Inode already in use: %pd (B=%lx)\n",
> dentry, d_inode(dentry)->i_ino);
> + dput(dentry);
> return false;
Please replace two statements by the statement “goto put_dentry;”.
…
> error:
> cachefiles_do_unmark_inode_in_use(object, d_inode(dentry));
+put_dentry:
> dput(dentry);
> return false;
> }
Regards,
Markus
On 2024/8/26 21:55, Markus Elfring wrote:
> …
>> Add the missing dput() to cachefiles_open_file() for a quick fix.
> I suggest to use a goto chain accordingly.
>
>
> …
Hi Markus,
Thanks for the suggestion, but I think the current solution is simple
enough that we don't need to add a label to it.
Actually, at first I was going to release the reference count of the
dentry uniformly in cachefiles_look_up_object() and delete all dput()
in cachefiles_open_file(), but this may conflict when backporting
the code to stable. So just keep it simple to facilitate backporting
to stable.
Thanks,
Baokun
>> +++ b/fs/cachefiles/namei.c
>> @@ -554,6 +554,7 @@ static bool cachefiles_open_file(struct cachefiles_object *object,
>> if (!cachefiles_mark_inode_in_use(object, d_inode(dentry))) {
>> pr_notice("cachefiles: Inode already in use: %pd (B=%lx)\n",
>> dentry, d_inode(dentry)->i_ino);
>> + dput(dentry);
>> return false;
> Please replace two statements by the statement “goto put_dentry;”.
>
>
> …
>> error:
>> cachefiles_do_unmark_inode_in_use(object, d_inode(dentry));
> +put_dentry:
>> dput(dentry);
>> return false;
>> }
> Regards,
> Markus
--
With Best Regards,
Baokun Li
Baokun Li <libaokun@huaweicloud.com> wrote: > Actually, at first I was going to release the reference count of the > dentry uniformly in cachefiles_look_up_object() and delete all dput() > in cachefiles_open_file(), You couldn't do that anyway, since kernel_file_open() steals the caller's ref if successful. > but this may conflict when backporting the code to stable. So just keep it > simple to facilitate backporting to stable. Prioritise upstream, please. I think Markus's suggestion of inserting a label and switching to a goto is better. Thanks, David
Hello David,
Thanks for the review.
On 2024/8/28 21:01, David Howells wrote:
> Baokun Li <libaokun@huaweicloud.com> wrote:
>
>> Actually, at first I was going to release the reference count of the
>> dentry uniformly in cachefiles_look_up_object() and delete all dput()
>> in cachefiles_open_file(),
> You couldn't do that anyway, since kernel_file_open() steals the caller's ref
> if successful.
Ignoring kernel_file_open(), we now put a reference count of the dentry
whether cachefiles_open_file() returns true or false.
And cachefiles_open_file() doesn't modify the dentry, so I'm thinking it's
releasing the reference count of the dentry that was got by
lookup_positive_unlocked() in cachefiles_look_up_object().
I'm not sure how kernel_file_open() steals the reference count,
am I missing something?
The code is as follows:
diff --git a/fs/cachefiles/namei.c b/fs/cachefiles/namei.c
index f53977169db4..2b3f9935dbb4 100644
--- a/fs/cachefiles/namei.c
+++ b/fs/cachefiles/namei.c
@@ -595,14 +595,12 @@ static bool cachefiles_open_file(struct
cachefiles_object *object,
* write and readdir but not lookup or open).
*/
touch_atime(&file->f_path);
- dput(dentry);
return true;
check_failed:
fscache_cookie_lookup_negative(object->cookie);
cachefiles_unmark_inode_in_use(object, file);
fput(file);
- dput(dentry);
if (ret == -ESTALE)
return cachefiles_create_file(object);
return false;
@@ -611,7 +609,6 @@ static bool cachefiles_open_file(struct
cachefiles_object *object,
fput(file);
error:
cachefiles_do_unmark_inode_in_use(object, d_inode(dentry));
- dput(dentry);
return false;
}
@@ -654,7 +651,9 @@ bool cachefiles_look_up_object(struct
cachefiles_object *object)
goto new_file;
}
- if (!cachefiles_open_file(object, dentry))
+ ret = cachefiles_open_file(object, dentry);
+ dput(dentry);
+ if (!ret)
return false;
_leave(" = t [%lu]", file_inode(object->file)->i_ino);
Regards,
Baokun
>> but this may conflict when backporting the code to stable. So just keep it
>> simple to facilitate backporting to stable.
> Prioritise upstream, please.
>
> I think Markus's suggestion of inserting a label and switching to a goto is
> better.
>
> Thanks,
> David
Baokun Li <libaokun@huaweicloud.com> wrote: > > You couldn't do that anyway, since kernel_file_open() steals the caller's ref > > if successful. > Ignoring kernel_file_open(), we now put a reference count of the dentry > whether cachefiles_open_file() returns true or false. Actually, I'm wrong kernel_file_open() doesn't steal a ref. David
Hi David, On 2024/8/29 0:14, David Howells wrote: > Baokun Li <libaokun@huaweicloud.com> wrote: > >>> You couldn't do that anyway, since kernel_file_open() steals the caller's ref >>> if successful. >> Ignoring kernel_file_open(), we now put a reference count of the dentry >> whether cachefiles_open_file() returns true or false. > Actually, I'm wrong kernel_file_open() doesn't steal a ref. > > David > > Thanks for confirming this. I will send a new version using the new solution. Cheers, Baokun
>> … >>> Add the missing dput() to cachefiles_open_file() for a quick fix. >> I suggest to use a goto chain accordingly. … > Thanks for the suggestion, but I think the current solution is simple enough Yes, of course (in principle). > that we don't need to add a label to it. I came along other software design expectations. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?h=v6.11-rc5#n526 Regards, Markus
© 2016 - 2025 Red Hat, Inc.