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

[Harry Yoo <harry@kernel.org>]

[-- Attachment #1.1: Type: text/plain, Size: 6048 bytes --]



On 6/11/26 12:40 AM, Vlastimil Babka (SUSE) wrote:
> 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.

Oh, today I learned...

> When we end up allocating a kfence object, kfence perfoms the zeroing on
> 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.

TIL again :D

> For kfence allocations in slab_alloc_node() this is achieved by subtly
> skipping over the slab_want_init_on_alloc() call.

Indeed subtle and I didn't realize this.

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

Right.

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

Right.

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

Great, more straightforward!

> 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@kernel.org>
> ---
>  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;
+		} 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.
+			 */
+			skip_init = true;
+		}
+
+		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,


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 228 bytes --]