Re: [PATCH v4 0/2] mm: slub: Enhanced debugging in slub error

[Harry Yoo <harry.yoo@oracle.com>]

On Thu, Feb 27, 2025 at 05:26:26PM +0100, Vlastimil Babka wrote:
> On 2/27/25 17:12, Vlastimil Babka wrote:
> > On 2/26/25 09:11, Hyesoo Yu wrote:
> >> Dear Maintainer,
> >> 
> >> The purpose is to improve the debugging capabilities of the slub allocator
> >> when a error occurs. The following improvements have been made:
> >> 
> >>  - Added WARN() calls at specific locations (slab_err, object_err) to detect
> >> errors effectively and to generate a crash dump if panic_on_warn is enabled.
> >> 
> >>  - Additionally, the error printing location in check_object has been adjusted to
> >> display the broken data before the restoration process. This improvement
> >> allows for a better understanding of how the data was corrupted.
> >> 
> >> This series combines two patches that were discussed seperately in the links below.
> >> https://lore.kernel.org/linux-mm/20250120082908.4162780-1-hyesoo.yu@samsung.com/
> >> https://lore.kernel.org/linux-mm/20250120083023.4162932-1-hyesoo.yu@samsung.com/
> >> 
> >> Thanks you.
> > 
> > Thanks. On top of things already mentioned, I added some kunit suppressions
> > in patch 2. Please check the result:
> > 
> > https://web.git.kernel.org/pub/scm/linux/kernel/git/vbabka/slab.git/log/?h=slab/for-6.15/fixes-cleanups
> 
> What do you think about the following patch on top?
> 
> ---8<---
> From c38dadde6293cacdb91f95afc3615c22dec5830a Mon Sep 17 00:00:00 2001
> From: Vlastimil Babka <vbabka@suse.cz>
> Date: Thu, 27 Feb 2025 16:05:46 +0100
> Subject: [PATCH] mm, slab: cleanup slab_bug() parameters
> 
> slab_err() has variadic printf arguments but instead of passing them to
> slab_bug() it does vsnprintf() to a buffer and passes %s, buf.
> 
> To allow passing them directly, turn slab_bug() to __slab_bug() with a
> va_list parameter, and slab_bug() a wrapper with fmt, ... parameters.
> Then slab_err() can call __slab_bug() without the intermediate buffer.
> 
> Also constify fmt everywhere, which also simplifies object_err()'s
> call to slab_bug().
> 
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> ---

Looks good to me.

FWIW,
Reviewed-by: Harry Yoo <harry.yoo@oracle.com>

-- 
Cheers,
Harry

>  mm/slub.c | 28 ++++++++++++++++++----------
>  1 file changed, 18 insertions(+), 10 deletions(-)
> 
> diff --git a/mm/slub.c b/mm/slub.c
> index a9a02b4ae4d6..d94af020b305 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -1017,12 +1017,12 @@ void skip_orig_size_check(struct kmem_cache *s, const void *object)
>  	set_orig_size(s, (void *)object, s->object_size);
>  }
>  
> -static void slab_bug(struct kmem_cache *s, char *fmt, ...)
> +static void __slab_bug(struct kmem_cache *s, const char *fmt, va_list argsp)
>  {
>  	struct va_format vaf;
>  	va_list args;
>  
> -	va_start(args, fmt);
> +	va_copy(args, argsp);
>  	vaf.fmt = fmt;
>  	vaf.va = &args;
>  	pr_err("=============================================================================\n");
> @@ -1031,8 +1031,17 @@ static void slab_bug(struct kmem_cache *s, char *fmt, ...)
>  	va_end(args);
>  }
>  
> +static void slab_bug(struct kmem_cache *s, const char *fmt, ...)
> +{
> +	va_list args;
> +
> +	va_start(args, fmt);
> +	__slab_bug(s, fmt, args);
> +	va_end(args);
> +}
> +
>  __printf(2, 3)
> -static void slab_fix(struct kmem_cache *s, char *fmt, ...)
> +static void slab_fix(struct kmem_cache *s, const char *fmt, ...)
>  {
>  	struct va_format vaf;
>  	va_list args;
> @@ -1088,12 +1097,12 @@ static void print_trailer(struct kmem_cache *s, struct slab *slab, u8 *p)
>  }
>  
>  static void object_err(struct kmem_cache *s, struct slab *slab,
> -			u8 *object, char *reason)
> +			u8 *object, const char *reason)
>  {
>  	if (slab_add_kunit_errors())
>  		return;
>  
> -	slab_bug(s, "%s", reason);
> +	slab_bug(s, reason);
>  	print_trailer(s, slab, object);
>  	add_taint(TAINT_BAD_PAGE, LOCKDEP_NOW_UNRELIABLE);
>  
> @@ -1129,15 +1138,14 @@ static __printf(3, 4) void slab_err(struct kmem_cache *s, struct slab *slab,
>  			const char *fmt, ...)
>  {
>  	va_list args;
> -	char buf[100];
>  
>  	if (slab_add_kunit_errors())
>  		return;
>  
>  	va_start(args, fmt);
> -	vsnprintf(buf, sizeof(buf), fmt, args);
> +	__slab_bug(s, fmt, args);
>  	va_end(args);
> -	slab_bug(s, "%s", buf);
> +
>  	__slab_err(slab);
>  }
>  
> @@ -1175,7 +1183,7 @@ static void init_object(struct kmem_cache *s, void *object, u8 val)
>  					  s->inuse - poison_size);
>  }
>  
> -static void restore_bytes(struct kmem_cache *s, char *message, u8 data,
> +static void restore_bytes(struct kmem_cache *s, const char *message, u8 data,
>  						void *from, void *to)
>  {
>  	slab_fix(s, "Restoring %s 0x%p-0x%p=0x%x", message, from, to - 1, data);
> @@ -1190,7 +1198,7 @@ static void restore_bytes(struct kmem_cache *s, char *message, u8 data,
>  
>  static pad_check_attributes int
>  check_bytes_and_report(struct kmem_cache *s, struct slab *slab,
> -		       u8 *object, char *what, u8 *start, unsigned int value,
> +		       u8 *object, const char *what, u8 *start, unsigned int value,
>  		       unsigned int bytes, bool slab_obj_print)
>  {
>  	u8 *fault;
> -- 
> 2.48.1
> 
>