Re: [PATCH v2 02/16] mm/slab: do not init any kfence objects on allocation

[Harry Yoo]

On 6/11/26 11:47 PM, Vlastimil Babka (SUSE) wrote:
> On 6/11/26 10:34, Vlastimil Babka (SUSE) wrote:
>> On 6/11/26 05:19, Harry Yoo wrote:
>>>
>>>> This potentially adds overhead of the is_kfence_address() check to
>>>> allocation hotpath, but that one is designed to be as small as possible,
>>>> and it's only evaluated if zeroing is about to happen. This means (aside
>>>> from init_on_alloc hardening) only for __GFP_ZERO allocations, and the
>>>> zeroing itself comes with an overhead likely larger than the added
>>>> check.
>>>>
>>>> Signed-off-by: Vlastimil Babka (SUSE) <vbabka@xxxxxxxxxx>
>>>> ---
>>>>  mm/kfence/core.c |  2 +-
>>>>  mm/slub.c        | 23 ++++++++---------------
>>>>  2 files changed, 9 insertions(+), 16 deletions(-)
>>>>
>>>> diff --git a/mm/slub.c b/mm/slub.c
>>>> index e2ee8f1aaccf..8e5264d3ddbf 100644
>>>> --- a/mm/slub.c
>>>> +++ b/mm/slub.c
>>>> @@ -4565,9 +4565,10 @@ struct kmem_cache *slab_pre_alloc_hook(struct kmem_cache *s, gfp_t flags)
>>>>  
>>>>  static __fastpath_inline
>>>>  bool slab_post_alloc_hook(struct kmem_cache *s, struct list_lru *lru,
>>>> -			  gfp_t flags, size_t size, void **p, bool init,
>>>> +			  gfp_t flags, size_t size, void **p,
>>>>  			  unsigned int orig_size)
>>>>  {
>>>> +	bool init = slab_want_init_on_alloc(flags, s);
>>>>  	unsigned int zero_size = s->object_size;
>>>>  	bool kasan_init = init;
>>>>  	size_t i;
>>>> @@ -4608,7 +4609,8 @@ bool slab_post_alloc_hook(struct kmem_cache *s, struct list_lru *lru,
>>>>  	for (i = 0; i < size; i++) {
>>>>  		p[i] = kasan_slab_alloc(s, p[i], init_flags, kasan_init);
>>>>  		if (p[i] && init && (!kasan_init ||
>>>> -				     !kasan_has_integrated_init()))
>>>> +				     !kasan_has_integrated_init())
>>>> +				 && !is_kfence_address(p[i]))
>>>
>>> I hope we could make it bit more verbose and straightforward,
>>> something like:
>>>
>>> diff --git a/mm/slub.c b/mm/slub.c
>>> index 5d7ea72ebebd..29cf4590f9d9 100644
>>> --- a/mm/slub.c
>>> +++ b/mm/slub.c
>>> @@ -4573,7 +4573,6 @@ bool slab_post_alloc_hook(struct kmem_cache *s,
>>> gfp_t flags, size_t size,
>>>  {
>>>  	bool init = slab_want_init_on_alloc(flags, s);
>>>  	unsigned int zero_size = s->object_size;
>>> -	bool kasan_init = init;
>>>  	size_t i;
>>>  	gfp_t init_flags = flags & gfp_allowed_mask;
>>>
>>> @@ -4591,29 +4590,37 @@ bool slab_post_alloc_hook(struct kmem_cache *s,
>>> gfp_t flags, size_t size,
>>>  	if (slub_debug_orig_size(s))
>>>  		zero_size = ac->orig_size;
>>>
>>> -	/*
>>> -	 * When slab_debug is enabled, avoid memory initialization integrated
>>> -	 * into KASAN and instead zero out the memory via the memset below with
>>> -	 * the proper size. Otherwise, KASAN might overwrite SLUB redzones and
>>> -	 * cause false-positive reports. This does not lead to a performance
>>> -	 * penalty on production builds, as slab_debug is not intended to be
>>> -	 * enabled there.
>>> -	 */
>>> -	if (__slub_debug_enabled())
>>> -		kasan_init = false;
>>> -
>>> -	/*
>>> -	 * As memory initialization might be integrated into KASAN,
>>> -	 * kasan_slab_alloc and initialization memset must be
>>> -	 * kept together to avoid discrepancies in behavior.
>>> -	 *
>>> -	 * As p[i] might get tagged, memset and kmemleak hook come after KASAN.
>>> -	 */
>>>  	for (i = 0; i < size; i++) {
>>> -		p[i] = kasan_slab_alloc(s, p[i], init_flags, kasan_init);
>>> -		if (p[i] && init && (!kasan_init ||
>>> -				     !kasan_has_integrated_init())
>>> -				 && !is_kfence_address(p[i]))
>>> +		bool skip_init = false;
>>> +
>>> +		if (is_kfence_address(p[i])) {
>>> +			/*
>>> +			 * kfence zeroes the object instead of SLUB to avoid
>>> +			 * overwriting its own redzone, and zeroing of
>>> +			 * s->object_size will corrupt it.
>>> +			 */
>>> +			skip_init = true;
>>
>> But now we perform this check even if init is false, making it more hot.
>>
>>> +		} else if (__slub_debug_enabled()) {
>>> +			/*
>>> +			 * KASAN never zeroes memory when slab_debug is enabled
>>> +			 * to avoid overwriting SLUB redzones. This does not
>>> +			 * lead to a performance penalty on production builds,
>>> +			 * as slab_debug is not intended to be enabled there.
>>> +			 */
>>> +			skip_init = false;
>>> +		} else if (kasan_has_integrated_init()) {
>>> +			/*
>>> +			 * ARM64 can set memory tags and zero the memory using
>>> +			 * a single instruction. Since HW_TAGS KASAN uses that
>>> +			 * while tagging the object, a separate zeroing is
>>> +			 * unnecessary unless slab_debug is enabled.
>>> +			 */
>>
>> (I like the new/updated comments)
>>
>>> +			skip_init = true;
>>> +		}>
>>
>> And these two are now done in every loop iteration even though they don't
>> depend on the object. Yeah it's a static key and build-time constant but still.
>>
>> But maybe there's some middle ground?
>>
>> Above the loop do (with your comments).
> 
> OK, not so simple, we still need the kasan_init variable too.

Ouch, right.

> I've ended up with this, thoughts?

Much better!

> From 3a1c4398ce9f361a4e6f4d9946eab6237eea89c2 Mon Sep 17 00:00:00 2001
> From: "Vlastimil Babka (SUSE)" <vbabka@xxxxxxxxxx>
> Date: Wed, 10 Jun 2026 17:40:04 +0200
> Subject: [PATCH] mm/slab: do not init any kfence objects on allocation
> 
> When init (zeroing) on allocation is requested, for kmalloc() we
> generally have to zero the full object size even if a smaller size is
> requested, in order to provide krealloc()'s __GFP_ZERO guarantees.
> 
> When we end up allocating a kfence object, kfence perfoms the zeroing on

nit: perfoms -> performs

> its own because has its own redzone beyond the requested size. Thus
> slab_post_alloc_hook() has an 'init' parameter which has to be evaluated
> in all callers (via slab_want_init_on_alloc()) and should be false for
> kfence allocations.
> 
> For kfence allocations in slab_alloc_node() this is achieved by subtly
> skipping over the slab_want_init_on_alloc() call. Other callers (i.e.
> kmem_cache_alloc_bulk_noprof()) however evaluate it unconditionally even
> if they do end up with a kfence allocation. This is only subtly not a
> problem, as those are not kmalloc allocations and thus the "requested
> size" equals s->object_size and thus it cannot interfere with kfence's
> redzone. There's just a unnecessary double zeroing (in both kfence and
> slab_post_alloc_hook()), but it's all very fragile and contradicts the
> comment in kfence_guarded_alloc().
> 
> Remove this subtlety and simplify the code by eliminating the init
> parameter from slab_post_alloc_hook() and make it call
> slab_want_init_on_alloc() itself. Instead add a is_kfence_address()
> check before performing the memset, which will start doing the right
> thing for all callers of slab_post_alloc_hook().
> 
> This potentially adds overhead of the is_kfence_address() check to
> allocation hotpath, but that one is designed to be as small as possible,
> and it's only evaluated if zeroing is about to happen. This means (aside
> from init_on_alloc hardening) only for __GFP_ZERO allocations, and the
> zeroing itself comes with an overhead likely larger than the added
> check.

> While at it, refactor the handling of evaluating when KASAN does the
> init instead of SLUB, with no intended functional changes. A
> non-functional change is that we don't pass kasan_init as true to
> kasan_slab_alloc() if kasan has no integrated init, but then the value
> is ignored anyway, so it's theoretically more correct.

Right.

> Thanks to Harry Yoo for the initial refactoring attempt, and for updated
> comments that are used here.

No problem ;)

> Link: https://patch.msgid.link/20260610-slab_alloc_flags-v2-2-7190909db118@xxxxxxxxxx
> Signed-off-by: Vlastimil Babka (SUSE) <vbabka@xxxxxxxxxx>
> ---

Looks good to me,
Reviewed-by: Harry Yoo (Oracle) <harry@xxxxxxxxxx>

Thanks!

-- 
Cheers,
Harry / Hyeonggon