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

[Vlastimil Babka (SUSE)]

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).

bool init;

/* ARM64 can ...
 * ...
 * But KASAN never zeroes ...
 */
if (kasan_has_integrated_init() && !__slub_debug_enabled())
	init = false;
else
	init = slab_want_init_on_alloc(flags, s);

In the loop:

		if (p[i] && init && !is_kfence_address(p[i]))
			memset(p[i], 0, zero_size);

> +		p[i] = kasan_slab_alloc(s, p[i], init_flags, init && skip_init);
> +		/* memset and hooks come after KASAN as p[i] might get tagged */
> +		if (p[i] && init && !skip_init)
>  			memset(p[i], 0, zero_size);
>  		if (alloc_flags_allow_spinning(ac->alloc_flags))
>  			kmemleak_alloc_recursive(p[i], s->object_size, 1,
>