Re: [PATCH v2] mm: slub: call WARN() instead of pr_err on slab_fix.

[David Rientjes <rientjes@google.com>]

On Thu, 6 Feb 2025, Hyesoo Yu wrote:

> What do you think of this comment ? I will add it in patch v3.
> 
> /*
>  * slab_fix indicates that the value would be restored even if an error occurs.
>  * Or, it is possible to trigger a panic without restoring using WARN() if panic_on_warn
>  * is enabled. This can obtain a crash dump at the point of issue to debug.
>  * It is advisable not to restore the data before calling slab_fix() to check for corrupted
>  * data in the crash dump.
>  */
> 

Looks good, you might consider it to be a bit more brief, but otherwise 
looks to clearly document the intent here.

Was thinking of something like:
/*
 * If panic_on_warn is enabled to generate a crash dump, be careful to not
 * restore data before this point.
 */
WARN()

But I don't feel strongly.  Thanks!

> Thanks,
> Regards.
> 
> > > >> Changes in v2:
> > > >> - Replace direct calling with BUG_ON with the use of WARN in slab_fix.
> > > >> 
> > > >> Signed-off-by: Hyesoo Yu <hyesoo.yu@samsung.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
> > > >> 
> > > >> 
> > > 
> > > 
> > 
>