| Message ID | 20240209125220.330383-1-dmantipov@yandex.ru (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] fs: prefer kfree_rcu() in fasync_remove_entry() | expand |
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
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.
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...
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.
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.
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;
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(-)