linux-kernel - Re: [PATCH v2] mm: slub: call WARN() instead of pr_err on slab

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <55522d9a-7fd2-1df0-19df-1552644b009e@google.com>
Date: Wed, 5 Feb 2025 09:10:32 -0800 (PST)
From: David Rientjes <rientjes@...gle.com>
To: Hyesoo Yu <hyesoo.yu@...sung.com>
cc: janghyuck.kim@...sung.com, Christoph Lameter <cl@...ux.com>, 
    Pekka Enberg <penberg@...nel.org>, Joonsoo Kim <iamjoonsoo.kim@....com>, 
    Andrew Morton <akpm@...ux-foundation.org>, 
    Vlastimil Babka <vbabka@...e.cz>, 
    Roman Gushchin <roman.gushchin@...ux.dev>, 
    Hyeonggon Yoo <42.hyeyoo@...il.com>, linux-mm@...ck.org, 
    linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] mm: slub: call WARN() instead of pr_err on
 slab_fix.

On Wed, 5 Feb 2025, Hyesoo Yu wrote:

> If a slab object is corrupted or an error occurs in its internal
> value, continuing after restoration may cause other side effects.
> At this point, it is difficult to debug because the problem occurred
> in the past. It is better to use WARN() instead of pr_err to catch
> errors at the point of issue because WARN() could trigger panic for
> system debugging when panic_on_warn is enabled. WARN() should be
> called prior to fixing the value because when a panic is triggered by WARN(),
> it allows us to check corrupted data.
> 

I think this makes sense, but it doesn't document why the other changes 
are being made, like moving the setting of *freelist to NULL.  This is 
presumably something that you want in the crash dump when 
kernel.panic_on_warn is enabled.  Probably best to call that out, but to 
also indicate what you're relying on in the crash dump to make forward 
progress on in diagnosing the issue.

> Changes in v2:
> - Replace direct calling with BUG_ON with the use of WARN in slab_fix.
> 
> Signed-off-by: Hyesoo Yu <hyesoo.yu@...sung.com>
> ---
>  mm/slub.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/mm/slub.c b/mm/slub.c
> index 1f50129dcfb3..ea956cb4b8be 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -1043,7 +1043,7 @@ static void slab_fix(struct kmem_cache *s, char *fmt, ...)
>  	va_start(args, fmt);
>  	vaf.fmt = fmt;
>  	vaf.va = &args;
> -	pr_err("FIX %s: %pV\n", s->name, &vaf);
> +	WARN(1, "FIX %s: %pV\n", s->name, &vaf);
>  	va_end(args);
>  }
>  
> @@ -1106,8 +1106,8 @@ static bool freelist_corrupted(struct kmem_cache *s, struct slab *slab,
>  	if ((s->flags & SLAB_CONSISTENCY_CHECKS) &&
>  	    !check_valid_pointer(s, slab, nextfree) && freelist) {
>  		object_err(s, slab, *freelist, "Freechain corrupt");
> -		*freelist = NULL;
>  		slab_fix(s, "Isolate corrupted freechain");
> +		*freelist = NULL;
>  		return true;
>  	}
>  
> @@ -1445,9 +1445,9 @@ static int on_freelist(struct kmem_cache *s, struct slab *slab, void *search)
>  				set_freepointer(s, object, NULL);
>  			} else {
>  				slab_err(s, slab, "Freepointer corrupt");
> +				slab_fix(s, "Freelist cleared");
>  				slab->freelist = NULL;
>  				slab->inuse = slab->objects;
> -				slab_fix(s, "Freelist cleared");
>  				return 0;
>  			}
>  			break;
> @@ -1464,14 +1464,14 @@ static int on_freelist(struct kmem_cache *s, struct slab *slab, void *search)
>  	if (slab->objects != max_objects) {
>  		slab_err(s, slab, "Wrong number of objects. Found %d but should be %d",
>  			 slab->objects, max_objects);
> -		slab->objects = max_objects;
>  		slab_fix(s, "Number of objects adjusted");
> +		slab->objects = max_objects;
>  	}
>  	if (slab->inuse != slab->objects - nr) {
>  		slab_err(s, slab, "Wrong object count. Counter is %d but counted were %d",
>  			 slab->inuse, slab->objects - nr);
> -		slab->inuse = slab->objects - nr;
>  		slab_fix(s, "Object count adjusted");
> +		slab->inuse = slab->objects - nr;
>  	}
>  	return search == NULL;
>  }
> -- 
> 2.48.0
> 
>