| Message ID | 20240826040018.2990763-1-libaokun@huaweicloud.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | cachefiles: fix dentry leak in cachefiles_open_file() | expand |
… > 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
>> … >>> 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
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
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; }