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

[Baokun Li <libaokun@xxxxxxxxxxxxxxx>]

Hello David,

Thanks for the review.

On 2024/8/28 21:01, David Howells wrote:
> Baokun Li <libaokun@xxxxxxxxxxxxxxx> 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