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

[Vlastimil Babka (SUSE)]

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.
I've ended up with this, thoughts?