[PATCH] cachefiles: fix dentry leak in cachefiles_open_file()

[PATCH] cachefiles: fix dentry leak in cachefiles_open_file()

[libaokun]

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

Re: [PATCH] cachefiles: fix dentry leak in cachefiles_open_file()

[Markus Elfring]

…
> 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

Re: [PATCH] cachefiles: fix dentry leak in cachefiles_open_file()

[Baokun Li]

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

Re: cachefiles: fix dentry leak in cachefiles_open_file()

[Markus Elfring]

>> …
>>> 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

Re: [PATCH] cachefiles: fix dentry leak in cachefiles_open_file()

[David Howells]

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

Re: [PATCH] cachefiles: fix dentry leak in cachefiles_open_file()

[Baokun Li]

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

Re: [PATCH] cachefiles: fix dentry leak in cachefiles_open_file()

[David Howells]

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

Re: [PATCH] cachefiles: fix dentry leak in cachefiles_open_file()

[Baokun Li]

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