Re: [PATCH RFC 2/4] mm/slab: handle allow_spin in slab_free_hook() instead of open coding

[Harry Yoo <harry@kernel.org>]

Apologies for the late reply...

On Fri, Jun 26, 2026 at 04:41:22PM +0200, Vlastimil Babka (SUSE) wrote:
> On 6/24/26 15:11, Harry Yoo (Oracle) wrote:
> > diff --git a/mm/slub.c b/mm/slub.c
> > index 32672a92581b..85760c8ff2e2 100644
> > --- a/mm/slub.c
> > +++ b/mm/slub.c
> > @@ -2611,41 +2611,52 @@ struct rcu_delayed_free {
> >   * - memcg_slab_post_alloc_hook()
> >   * - alloc_tagging_slab_alloc_hook()
> >   *
> > + * slab_free_hook() assumes that memcg_slab_free_hook() and
> > + * alloc_tagging_slab_free_hook() are already invoked.
> > + *
> >   * Free hooks that must handle missing corresponding alloc hooks:
> >   * - kmemleak_free_recursive()
> >   * - kfence_free()
> >   *
> > - * Free hooks that have no alloc hook counterpart, and thus safe to call:
> > + * Free hooks that have no alloc hook counterpart, and thus safe to call
> > + * when spinning is allowed:
> >   * - debug_check_no_locks_freed()
> >   * - debug_check_no_obj_freed()
> >   * - __kcsan_check_access()
> >   */
> >  static __always_inline
> >  bool slab_free_hook(struct kmem_cache *s, void *x, bool init,
> > -		    bool after_rcu_delay)
> > +		    bool after_rcu_delay, bool allow_spin)
> >  {
> >  	/* Are the object contents still accessible? */
> >  	bool still_accessible = (s->flags & SLAB_TYPESAFE_BY_RCU) && !after_rcu_delay;
> >  
> > -	kmemleak_free_recursive(x, s->flags);
> >  	kmsan_slab_free(s, x);
> >  
> > -	debug_check_no_locks_freed(x, s->object_size);
> > -
> > -	if (!(s->flags & SLAB_DEBUG_OBJECTS))
> > -		debug_check_no_obj_freed(x, s->object_size);
> > -
> > -	/* Use KCSAN to help debug racy use-after-free. */
> > -	if (!still_accessible)
> > -		__kcsan_check_access(x, s->object_size,
> > -				     KCSAN_ACCESS_WRITE | KCSAN_ACCESS_ASSERT);
> > -
> > -	if (kfence_free(x))
> > -		return false;
> > +	if (likely(allow_spin)) {
> > +		/* These hooks acquire spinlocks */
> > +		kmemleak_free_recursive(x, s->flags);
> > +		debug_check_no_locks_freed(x, s->object_size);
> 
> This line shouldn't be here?

You mean because debug_check_no_locks_freed() doesn't acquire a lock
when there is no bug to report?

That's fair, but I didn't want to introduce functional changes
in this patch as kfree_nolock() hasn't been calling it anyway.

So separated this (and KCSAN checks) to patch 3 in v2 as they only
acquire locks when reporting a bug. (Taking risk of deadlock for bug
reporting just like KASAN).

> > +		if (!(s->flags & SLAB_DEBUG_OBJECTS))
> > +			debug_check_no_obj_freed(x, s->object_size);
> > +		/* Use KCSAN to help debug racy use-after-free. */
> > +		if (!still_accessible)
> > +			__kcsan_check_access(x, s->object_size,
> > +					     KCSAN_ACCESS_WRITE | KCSAN_ACCESS_ASSERT);
> > +		if (kfence_free(x))
> > +			return false;
> 
> The whole block now misses free space lines that it had.

Adjusted in v2, thanks!

-- 
Cheers,
Harry / Hyeonggon