[PATCH] [RFC] fs: prefer kfree_rcu() in fasync_remove_entry()

[PATCH] [RFC] fs: prefer kfree_rcu() in fasync_remove_entry()

[Dmitry Antipov]

In 'fasync_remove_entry()', prefer 'kfree_rcu()' over 'call_rcu()' with dummy
'fasync_free_rcu()' callback. This is mostly intended in attempt to fix weird
https://syzkaller.appspot.com/bug?id=6a64ad907e361e49e92d1c4c114128a1bda2ed7f,
where kmemleak may consider 'fa' as unreferenced during RCU grace period. See
https://lore.kernel.org/stable/20230930174657.800551-1-joel@joelfernandes.org
as well. Comments are highly appreciated.

Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
 fs/fcntl.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/fs/fcntl.c b/fs/fcntl.c
index c80a6acad742..c3e342eb74af 100644
--- a/fs/fcntl.c
+++ b/fs/fcntl.c
@@ -846,12 +846,6 @@ int send_sigurg(struct fown_struct *fown)
 static DEFINE_SPINLOCK(fasync_lock);
 static struct kmem_cache *fasync_cache __ro_after_init;
 
-static void fasync_free_rcu(struct rcu_head *head)
-{
-	kmem_cache_free(fasync_cache,
-			container_of(head, struct fasync_struct, fa_rcu));
-}
-
 /*
  * Remove a fasync entry. If successfully removed, return
  * positive and clear the FASYNC flag. If no entry exists,
@@ -877,7 +871,7 @@ int fasync_remove_entry(struct file *filp, struct fasync_struct **fapp)
 		write_unlock_irq(&fa->fa_lock);
 
 		*fp = fa->fa_next;
-		call_rcu(&fa->fa_rcu, fasync_free_rcu);
+		kfree_rcu(fa, fa_rcu);
 		filp->f_flags &= ~FASYNC;
 		result = 1;
 		break;
-- 
2.43.0

Re: [PATCH] [RFC] fs: prefer kfree_rcu() in fasync_remove_entry()

[Christian Brauner]

On Fri, 09 Feb 2024 15:52:19 +0300, Dmitry Antipov wrote:
> In 'fasync_remove_entry()', prefer 'kfree_rcu()' over 'call_rcu()' with dummy
> 'fasync_free_rcu()' callback. This is mostly intended in attempt to fix weird
> https://syzkaller.appspot.com/bug?id=6a64ad907e361e49e92d1c4c114128a1bda2ed7f,
> where kmemleak may consider 'fa' as unreferenced during RCU grace period. See
> https://lore.kernel.org/stable/20230930174657.800551-1-joel@joelfernandes.org
> as well. Comments are highly appreciated.
> 
> [...]

Applied to the vfs.misc branch of the vfs/vfs.git tree.
Patches in the vfs.misc branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.misc

[1/1] fs: prefer kfree_rcu() in fasync_remove_entry()
      https://git.kernel.org/vfs/vfs/c/f5f7ac72f4ee

Re: [PATCH] [RFC] fs: prefer kfree_rcu() in fasync_remove_entry()

[Christian Brauner]

On Fri, Feb 09, 2024 at 03:52:19PM +0300, Dmitry Antipov wrote:
> In 'fasync_remove_entry()', prefer 'kfree_rcu()' over 'call_rcu()' with dummy
> 'fasync_free_rcu()' callback. This is mostly intended in attempt to fix weird
> https://syzkaller.appspot.com/bug?id=6a64ad907e361e49e92d1c4c114128a1bda2ed7f,
> where kmemleak may consider 'fa' as unreferenced during RCU grace period. See
> https://lore.kernel.org/stable/20230930174657.800551-1-joel@joelfernandes.org
> as well. Comments are highly appreciated.
> 
> Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
> ---

Yeah, according to commit ae65a5211d90 ("mm/slab: document kfree() as
allowed for kmem_cache_alloc() objects") this is now guaranteed to work
for kmem_cache_alloc() objects since slab is gone. So independent of
syzbot this seems like a decent enough cleanup.

Re: [PATCH] [RFC] fs: prefer kfree_rcu() in fasync_remove_entry()

[Al Viro]

On Fri, Feb 09, 2024 at 03:52:19PM +0300, Dmitry Antipov wrote:
> In 'fasync_remove_entry()', prefer 'kfree_rcu()' over 'call_rcu()' with dummy
> 'fasync_free_rcu()' callback. This is mostly intended in attempt to fix weird
> https://syzkaller.appspot.com/bug?id=6a64ad907e361e49e92d1c4c114128a1bda2ed7f,
> where kmemleak may consider 'fa' as unreferenced during RCU grace period. See
> https://lore.kernel.org/stable/20230930174657.800551-1-joel@joelfernandes.org
> as well. Comments are highly appreciated.

That should go with mentioning that _these_ _days_ kfree() can be paired with
kmem_cache_alloc().  A reference to ae65a5211d90 "mm/slab: document kfree() as
allowed for kmem_cache_alloc() objects" might be a good idea; at the very least
it *must* come with "don't even think of backporting to any kernel that still
has SLOB support (i.e. anything prior to 6.4)".

Not sure if it's a good idea at this point - it doesn't look like it would get
mixed into anything that might need backporting, but...

Re: [PATCH] [RFC] fs: prefer kfree_rcu() in fasync_remove_entry()

[Al Viro]

On Fri, Feb 09, 2024 at 03:22:15PM +0100, Christian Brauner wrote:
> On Fri, Feb 09, 2024 at 03:52:19PM +0300, Dmitry Antipov wrote:
> > In 'fasync_remove_entry()', prefer 'kfree_rcu()' over 'call_rcu()' with dummy
> > 'fasync_free_rcu()' callback. This is mostly intended in attempt to fix weird
> > https://syzkaller.appspot.com/bug?id=6a64ad907e361e49e92d1c4c114128a1bda2ed7f,
> > where kmemleak may consider 'fa' as unreferenced during RCU grace period. See
> > https://lore.kernel.org/stable/20230930174657.800551-1-joel@joelfernandes.org
> > as well. Comments are highly appreciated.
> > 
> > Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
> > ---
> 
> Yeah, according to commit ae65a5211d90 ("mm/slab: document kfree() as
> allowed for kmem_cache_alloc() objects") this is now guaranteed to work
> for kmem_cache_alloc() objects since slab is gone. So independent of
> syzbot this seems like a decent enough cleanup.

Sure, but we'd better make very sure that it does *NOT* get picked by any
-stable prior to 6.4.

Re: [PATCH] [RFC] fs: prefer kfree_rcu() in fasync_remove_entry()

[Christian Brauner]

On Fri, Feb 09, 2024 at 04:36:46PM +0000, Al Viro wrote:
> On Fri, Feb 09, 2024 at 03:22:15PM +0100, Christian Brauner wrote:
> > On Fri, Feb 09, 2024 at 03:52:19PM +0300, Dmitry Antipov wrote:
> > > In 'fasync_remove_entry()', prefer 'kfree_rcu()' over 'call_rcu()' with dummy
> > > 'fasync_free_rcu()' callback. This is mostly intended in attempt to fix weird
> > > https://syzkaller.appspot.com/bug?id=6a64ad907e361e49e92d1c4c114128a1bda2ed7f,
> > > where kmemleak may consider 'fa' as unreferenced during RCU grace period. See
> > > https://lore.kernel.org/stable/20230930174657.800551-1-joel@joelfernandes.org
> > > as well. Comments are highly appreciated.
> > > 
> > > Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
> > > ---
> > 
> > Yeah, according to commit ae65a5211d90 ("mm/slab: document kfree() as
> > allowed for kmem_cache_alloc() objects") this is now guaranteed to work
> > for kmem_cache_alloc() objects since slab is gone. So independent of
> > syzbot this seems like a decent enough cleanup.
> 
> Sure, but we'd better make very sure that it does *NOT* get picked by any
> -stable prior to 6.4.

Yeah, sure. I've not marked it for stable exactly because of that. _But_
we can't exactly control AUTOSEL. Maybe there's a way. In any case, I'll
keep an eye out for it and will reply appropriately.