linux-kernel - Re: [PATCH] cachefiles: fix dentry leak in cachefiles_open

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <11c591fd-221b-4eeb-a0bd-e9e303d391a6@huaweicloud.com>
Date: Wed, 28 Aug 2024 22:05:08 +0800
From: Baokun Li <libaokun@...weicloud.com>
To: David Howells <dhowells@...hat.com>
Cc: Markus Elfring <Markus.Elfring@....de>, netfs@...ts.linux.dev,
 linux-fsdevel@...r.kernel.org, linux-erofs@...ts.ozlabs.org,
 Jeff Layton <jlayton@...nel.org>, stable@...nel.org,
 LKML <linux-kernel@...r.kernel.org>, Christian Brauner <brauner@...nel.org>,
 Gao Xiang <hsiangkao@...ux.alibaba.com>, Hou Tao <houtao1@...wei.com>,
 Jingbo Xu <jefflexu@...ux.alibaba.com>, Yang Erkun <yangerkun@...wei.com>,
 Yu Kuai <yukuai3@...wei.com>, Zizhi Wo <wozizhi@...wei.com>,
 Baokun Li <libaokun1@...wei.com>, Baokun Li <libaokun@...weicloud.com>
Subject: Re: [PATCH] cachefiles: fix dentry leak in cachefiles_open_file()

Hello David,

Thanks for the review.

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