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

[Vlastimil Babka]

On 2/5/25 18:10, David Rientjes wrote:
> 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.

Well the last sentence of the changelog above says exactly that, no?

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