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

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

[Hyesoo Yu]

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

version 2 changes
 - Used WARN() to trigger a panic instead of direct calling of BUG_ON()
 - Print the broken data only once before the restore.

version 3 changes
 - Move WARN() from slab_fix to slab_err and object to call WARN on
 all error reporting paths.
 - Change the parameter t ype of check_bytes_and_report.

Hyesoo Yu (2):
  mm: slub: Print the broken data before restoring slub.
  mm: slub: call WARN() when the slab detect an error

 mm/slub.c | 60 ++++++++++++++++++++++++++++++-------------------------
 1 file changed, 33 insertions(+), 27 deletions(-)

-- 
2.28.0

[PATCH v3 1/2] mm: slub: Print the broken data before restoring slub.

[Hyesoo Yu]

Previously, the restore occured after printing the object in slub.
After commit 47d911b02cbe ("slab: make check_object() more consistent"),
the bytes are printed after the restore. This information about the bytes
before the restore is highly valuable for debugging purpose.
For instance, in a event of cache issue, it displays byte patterns
by breaking them down into 64-bytes units. Without this information,
we can only speculate on how it was broken. Hence the corrupted regions
should be printed prior to the restoration process. However if an object
breaks in multiple places, the same log may be output multiple times.
Therefore the slub log is reported only once to prevent redundant printing,
by sending a parameter indicating whether an error has occurred previously.

Changes in v3:
- Change the parameter type of check_bytes_and_report.

Changes in v2:
- Instead of using print_section every time on check_bytes_and_report,
just print it once for the entire slub object before the restore.

Signed-off-by: Hyesoo Yu <hyesoo.yu@samsung.com>
Change-Id: I73cf76c110eed62506643913517c957c05a29520
---
 mm/slub.c | 29 ++++++++++++++---------------
 1 file changed, 14 insertions(+), 15 deletions(-)

diff --git a/mm/slub.c b/mm/slub.c
index b3969d63cc04..de62fed12236 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -1192,12 +1192,13 @@ 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, unsigned int bytes)
+		       u8 *object, char *what, u8 *start, unsigned int value,
+		       unsigned int bytes, bool slab_obj_print)
 {
 	u8 *fault;
 	u8 *end;
 	u8 *addr = slab_address(slab);
+	char buf[100];
 
 	metadata_access_enable();
 	fault = memchr_inv(kasan_reset_tag(start), value, bytes);
@@ -1212,11 +1213,14 @@ check_bytes_and_report(struct kmem_cache *s, struct slab *slab,
 	if (slab_add_kunit_errors())
 		goto skip_bug_print;
 
-	slab_bug(s, "%s overwritten", what);
 	pr_err("0x%p-0x%p @offset=%tu. First byte 0x%x instead of 0x%x\n",
 					fault, end - 1, fault - addr,
 					fault[0], value);
 
+	scnprintf(buf, 100, "%s overwritten", what);
+	if (slab_obj_print)
+		object_err(s, slab, object, buf);
+
 skip_bug_print:
 	restore_bytes(s, what, value, fault, end);
 	return 0;
@@ -1279,7 +1283,7 @@ static int check_pad_bytes(struct kmem_cache *s, struct slab *slab, u8 *p)
 		return 1;
 
 	return check_bytes_and_report(s, slab, p, "Object padding",
-			p + off, POISON_INUSE, size_from_object(s) - off);
+			p + off, POISON_INUSE, size_from_object(s) - off, true);
 }
 
 /* Check the pad bytes at the end of a slab page */
@@ -1329,11 +1333,11 @@ static int check_object(struct kmem_cache *s, struct slab *slab,
 
 	if (s->flags & SLAB_RED_ZONE) {
 		if (!check_bytes_and_report(s, slab, object, "Left Redzone",
-			object - s->red_left_pad, val, s->red_left_pad))
+			object - s->red_left_pad, val, s->red_left_pad, !!ret))
 			ret = 0;
 
 		if (!check_bytes_and_report(s, slab, object, "Right Redzone",
-			endobject, val, s->inuse - s->object_size))
+			endobject, val, s->inuse - s->object_size, !!ret))
 			ret = 0;
 
 		if (slub_debug_orig_size(s) && val == SLUB_RED_ACTIVE) {
@@ -1342,7 +1346,7 @@ static int check_object(struct kmem_cache *s, struct slab *slab,
 			if (s->object_size > orig_size  &&
 				!check_bytes_and_report(s, slab, object,
 					"kmalloc Redzone", p + orig_size,
-					val, s->object_size - orig_size)) {
+					val, s->object_size - orig_size, !!ret)) {
 				ret = 0;
 			}
 		}
@@ -1350,7 +1354,7 @@ static int check_object(struct kmem_cache *s, struct slab *slab,
 		if ((s->flags & SLAB_POISON) && s->object_size < s->inuse) {
 			if (!check_bytes_and_report(s, slab, p, "Alignment padding",
 				endobject, POISON_INUSE,
-				s->inuse - s->object_size))
+				s->inuse - s->object_size, !!ret))
 				ret = 0;
 		}
 	}
@@ -1366,11 +1370,11 @@ static int check_object(struct kmem_cache *s, struct slab *slab,
 			if (kasan_meta_size < s->object_size - 1 &&
 			    !check_bytes_and_report(s, slab, p, "Poison",
 					p + kasan_meta_size, POISON_FREE,
-					s->object_size - kasan_meta_size - 1))
+					s->object_size - kasan_meta_size - 1, !!ret))
 				ret = 0;
 			if (kasan_meta_size < s->object_size &&
 			    !check_bytes_and_report(s, slab, p, "End Poison",
-					p + s->object_size - 1, POISON_END, 1))
+					p + s->object_size - 1, POISON_END, 1, !!ret))
 				ret = 0;
 		}
 		/*
@@ -1396,11 +1400,6 @@ static int check_object(struct kmem_cache *s, struct slab *slab,
 		ret = 0;
 	}
 
-	if (!ret && !slab_in_kunit_test()) {
-		print_trailer(s, slab, object);
-		add_taint(TAINT_BAD_PAGE, LOCKDEP_NOW_UNRELIABLE);
-	}
-
 	return ret;
 }
 
-- 
2.28.0

Re: [PATCH v3 1/2] mm: slub: Print the broken data before restoring slub.

[Harry Yoo]

On Thu, Feb 20, 2025 at 12:39:43PM +0900, Hyesoo Yu wrote:
> Previously, the restore occured after printing the object in slub.
> After commit 47d911b02cbe ("slab: make check_object() more consistent"),
> the bytes are printed after the restore. This information about the bytes
> before the restore is highly valuable for debugging purpose.
> For instance, in a event of cache issue, it displays byte patterns
> by breaking them down into 64-bytes units. Without this information,
> we can only speculate on how it was broken. Hence the corrupted regions
> should be printed prior to the restoration process. However if an object
> breaks in multiple places, the same log may be output multiple times.
> Therefore the slub log is reported only once to prevent redundant printing,
> by sending a parameter indicating whether an error has occurred previously.
> 
> Changes in v3:
> - Change the parameter type of check_bytes_and_report.
> 
> Changes in v2:
> - Instead of using print_section every time on check_bytes_and_report,
> just print it once for the entire slub object before the restore.
>
> Signed-off-by: Hyesoo Yu <hyesoo.yu@samsung.com>
> Change-Id: I73cf76c110eed62506643913517c957c05a29520

As previously mentioned by others, Change-Id is not used in Linux
kernel development.

> ---
>  mm/slub.c | 29 ++++++++++++++---------------
>  1 file changed, 14 insertions(+), 15 deletions(-)
> 
> diff --git a/mm/slub.c b/mm/slub.c
> index b3969d63cc04..de62fed12236 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -1192,12 +1192,13 @@ 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, unsigned int bytes)
> +		       u8 *object, char *what, u8 *start, unsigned int value,
> +		       unsigned int bytes, bool slab_obj_print)
>  {
>  	u8 *fault;
>  	u8 *end;
>  	u8 *addr = slab_address(slab);
> +	char buf[100];
>  
>  	metadata_access_enable();
>  	fault = memchr_inv(kasan_reset_tag(start), value, bytes);
> @@ -1212,11 +1213,14 @@ check_bytes_and_report(struct kmem_cache *s, struct slab *slab,
>  	if (slab_add_kunit_errors())
>  		goto skip_bug_print;
>  
> -	slab_bug(s, "%s overwritten", what);
>  	pr_err("0x%p-0x%p @offset=%tu. First byte 0x%x instead of 0x%x\n",
>  					fault, end - 1, fault - addr,
>  					fault[0], value);
>  
> +	scnprintf(buf, 100, "%s overwritten", what);

How about moving this into the if block and changing 100 to sizeof(buf)?

> +	if (slab_obj_print)
> +		object_err(s, slab, object, buf);
> +
>  skip_bug_print:
>  	restore_bytes(s, what, value, fault, end);
>  	return 0;
> @@ -1279,7 +1283,7 @@ static int check_pad_bytes(struct kmem_cache *s, struct slab *slab, u8 *p)
>  		return 1;
>  
>  	return check_bytes_and_report(s, slab, p, "Object padding",
> -			p + off, POISON_INUSE, size_from_object(s) - off);
> +			p + off, POISON_INUSE, size_from_object(s) - off, true);
>  }
>  
>  /* Check the pad bytes at the end of a slab page */
> @@ -1329,11 +1333,11 @@ static int check_object(struct kmem_cache *s, struct slab *slab,
>  
>  	if (s->flags & SLAB_RED_ZONE) {
>  		if (!check_bytes_and_report(s, slab, object, "Left Redzone",
> -			object - s->red_left_pad, val, s->red_left_pad))
> +			object - s->red_left_pad, val, s->red_left_pad, !!ret))

I think you don't have to add !! to ret.
Converting from int to _Bool is legal in C99 and it will work as intended. 

>  			ret = 0;

-- 
Cheers,
Harry

Re: [PATCH v3 1/2] mm: slub: Print the broken data before restoring slub.

[Hyesoo Yu]

[-- Attachment #1: Type: text/plain, Size: 3969 bytes --]

On Thu, Feb 20, 2025 at 08:01:57PM +0900, Harry Yoo wrote:
> On Thu, Feb 20, 2025 at 12:39:43PM +0900, Hyesoo Yu wrote:
> > Previously, the restore occured after printing the object in slub.
> > After commit 47d911b02cbe ("slab: make check_object() more consistent"),
> > the bytes are printed after the restore. This information about the bytes
> > before the restore is highly valuable for debugging purpose.
> > For instance, in a event of cache issue, it displays byte patterns
> > by breaking them down into 64-bytes units. Without this information,
> > we can only speculate on how it was broken. Hence the corrupted regions
> > should be printed prior to the restoration process. However if an object
> > breaks in multiple places, the same log may be output multiple times.
> > Therefore the slub log is reported only once to prevent redundant printing,
> > by sending a parameter indicating whether an error has occurred previously.
> > 
> > Changes in v3:
> > - Change the parameter type of check_bytes_and_report.
> > 
> > Changes in v2:
> > - Instead of using print_section every time on check_bytes_and_report,
> > just print it once for the entire slub object before the restore.
> >
> > Signed-off-by: Hyesoo Yu <hyesoo.yu@samsung.com>
> > Change-Id: I73cf76c110eed62506643913517c957c05a29520
> 
> As previously mentioned by others, Change-Id is not used in Linux
> kernel development.
>

Oops, It is my mistake. I will remove it.

> > ---
> >  mm/slub.c | 29 ++++++++++++++---------------
> >  1 file changed, 14 insertions(+), 15 deletions(-)
> > 
> > diff --git a/mm/slub.c b/mm/slub.c
> > index b3969d63cc04..de62fed12236 100644
> > --- a/mm/slub.c
> > +++ b/mm/slub.c
> > @@ -1192,12 +1192,13 @@ 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, unsigned int bytes)
> > +		       u8 *object, char *what, u8 *start, unsigned int value,
> > +		       unsigned int bytes, bool slab_obj_print)
> >  {
> >  	u8 *fault;
> >  	u8 *end;
> >  	u8 *addr = slab_address(slab);
> > +	char buf[100];
> >  
> >  	metadata_access_enable();
> >  	fault = memchr_inv(kasan_reset_tag(start), value, bytes);
> > @@ -1212,11 +1213,14 @@ check_bytes_and_report(struct kmem_cache *s, struct slab *slab,
> >  	if (slab_add_kunit_errors())
> >  		goto skip_bug_print;
> >  
> > -	slab_bug(s, "%s overwritten", what);
> >  	pr_err("0x%p-0x%p @offset=%tu. First byte 0x%x instead of 0x%x\n",
> >  					fault, end - 1, fault - addr,
> >  					fault[0], value);
> >  
> > +	scnprintf(buf, 100, "%s overwritten", what);
> 
> How about moving this into the if block and changing 100 to sizeof(buf)?
>

That sounds good. I will change it.

> > +	if (slab_obj_print)
> > +		object_err(s, slab, object, buf);
> > +
> >  skip_bug_print:
> >  	restore_bytes(s, what, value, fault, end);
> >  	return 0;
> > @@ -1279,7 +1283,7 @@ static int check_pad_bytes(struct kmem_cache *s, struct slab *slab, u8 *p)
> >  		return 1;
> >  
> >  	return check_bytes_and_report(s, slab, p, "Object padding",
> > -			p + off, POISON_INUSE, size_from_object(s) - off);
> > +			p + off, POISON_INUSE, size_from_object(s) - off, true);
> >  }
> >  
> >  /* Check the pad bytes at the end of a slab page */
> > @@ -1329,11 +1333,11 @@ static int check_object(struct kmem_cache *s, struct slab *slab,
> >  
> >  	if (s->flags & SLAB_RED_ZONE) {
> >  		if (!check_bytes_and_report(s, slab, object, "Left Redzone",
> > -			object - s->red_left_pad, val, s->red_left_pad))
> > +			object - s->red_left_pad, val, s->red_left_pad, !!ret))
> 
> I think you don't have to add !! to ret.
> Converting from int to _Bool is legal in C99 and it will work as intended. 
> 

Thank you for informing me.
I remove !! to next version.

Thanks,
Regards.

> > 			ret = 0;
> 
> -- 
> Cheers,
> Harry
> 





[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v3 1/2] mm: slub: Print the broken data before restoring slub.

[Harry Yoo]

On Thu, Feb 20, 2025 at 12:39:43PM +0900, Hyesoo Yu wrote:
> Previously, the restore occured after printing the object in slub.
> After commit 47d911b02cbe ("slab: make check_object() more consistent"),
> the bytes are printed after the restore. This information about the bytes
> before the restore is highly valuable for debugging purpose.
> For instance, in a event of cache issue, it displays byte patterns
> by breaking them down into 64-bytes units. Without this information,
> we can only speculate on how it was broken. Hence the corrupted regions
> should be printed prior to the restoration process. However if an object
> breaks in multiple places, the same log may be output multiple times.
> Therefore the slub log is reported only once to prevent redundant printing,
> by sending a parameter indicating whether an error has occurred previously.
> 
> Changes in v3:
> - Change the parameter type of check_bytes_and_report.
> 
> Changes in v2:
> - Instead of using print_section every time on check_bytes_and_report,
> just print it once for the entire slub object before the restore.
> 
> Signed-off-by: Hyesoo Yu <hyesoo.yu@samsung.com>
> Change-Id: I73cf76c110eed62506643913517c957c05a29520
> ---
>  mm/slub.c | 29 ++++++++++++++---------------
>  1 file changed, 14 insertions(+), 15 deletions(-)
> 

> @@ -1212,11 +1213,14 @@ check_bytes_and_report(struct kmem_cache *s, struct slab *slab,
>  	if (slab_add_kunit_errors())
>  		goto skip_bug_print;
>  
> -	slab_bug(s, "%s overwritten", what);
>  	pr_err("0x%p-0x%p @offset=%tu. First byte 0x%x instead of 0x%x\n",
>  					fault, end - 1, fault - addr,
>  					fault[0], value);
>  
> +	scnprintf(buf, 100, "%s overwritten", what);
> +	if (slab_obj_print)
> +		object_err(s, slab, object, buf);


Wait, I think it's better to keep printing "%s overwritten" regardless
of slab_obj_print and only call __slab_err() if slab_obj_print == true
as discussed here [1]? Becuase in case there are multiple errors,
users should know.

[1] https://lore.kernel.org/all/2ff52c5e-4b6b-4b3d-9047-f00967315d3e@suse.cz

-- 
Cheers,
Harry

Re: [PATCH v3 1/2] mm: slub: Print the broken data before restoring slub.

[Hyesoo Yu]

[-- Attachment #1: Type: text/plain, Size: 2712 bytes --]

On Fri, Feb 21, 2025 at 05:16:01PM +0900, Harry Yoo wrote:
> On Thu, Feb 20, 2025 at 12:39:43PM +0900, Hyesoo Yu wrote:
> > Previously, the restore occured after printing the object in slub.
> > After commit 47d911b02cbe ("slab: make check_object() more consistent"),
> > the bytes are printed after the restore. This information about the bytes
> > before the restore is highly valuable for debugging purpose.
> > For instance, in a event of cache issue, it displays byte patterns
> > by breaking them down into 64-bytes units. Without this information,
> > we can only speculate on how it was broken. Hence the corrupted regions
> > should be printed prior to the restoration process. However if an object
> > breaks in multiple places, the same log may be output multiple times.
> > Therefore the slub log is reported only once to prevent redundant printing,
> > by sending a parameter indicating whether an error has occurred previously.
> > 
> > Changes in v3:
> > - Change the parameter type of check_bytes_and_report.
> > 
> > Changes in v2:
> > - Instead of using print_section every time on check_bytes_and_report,
> > just print it once for the entire slub object before the restore.
> > 
> > Signed-off-by: Hyesoo Yu <hyesoo.yu@samsung.com>
> > Change-Id: I73cf76c110eed62506643913517c957c05a29520
> > ---
> >  mm/slub.c | 29 ++++++++++++++---------------
> >  1 file changed, 14 insertions(+), 15 deletions(-)
> > 
> 
> > @@ -1212,11 +1213,14 @@ check_bytes_and_report(struct kmem_cache *s, struct slab *slab,
> >  	if (slab_add_kunit_errors())
> >  		goto skip_bug_print;
> >  
> > -	slab_bug(s, "%s overwritten", what);
> >  	pr_err("0x%p-0x%p @offset=%tu. First byte 0x%x instead of 0x%x\n",
> >  					fault, end - 1, fault - addr,
> >  					fault[0], value);
> >  
> > +	scnprintf(buf, 100, "%s overwritten", what);
> > +	if (slab_obj_print)
> > +		object_err(s, slab, object, buf);
> 
> 
> Wait, I think it's better to keep printing "%s overwritten" regardless
> of slab_obj_print and only call __slab_err() if slab_obj_print == true
> as discussed here [1]? Becuase in case there are multiple errors,
> users should know.
> 
> [1] https://lore.kernel.org/all/2ff52c5e-4b6b-4b3d-9047-f00967315d3e@suse.cz
> 

Hi,

__slab_err() doesn't include print_trainer(). It needs object_err().

How about including the specific error name 'what' to pr_err ?
And then object_err would print "Object corrupt" at the beginning once
without buf like below.

	if (slab_obj_print)
		object_err(s, slab, object, "Object corrupt");

	pr_err("[%s] 0x%p-0x%p @offset=%tu. First byte 0x%x instead of 0x%x\n",
 	       what, fault, end - 1, fault - addr, fault[0], value);

Thanks,
Regards.
> -- 
> Cheers,
> Harry
> 

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v3 1/2] mm: slub: Print the broken data before restoring slub.

[Vlastimil Babka]

On 2/24/25 03:43, Hyesoo Yu wrote:
> On Fri, Feb 21, 2025 at 05:16:01PM +0900, Harry Yoo wrote:
>> On Thu, Feb 20, 2025 at 12:39:43PM +0900, Hyesoo Yu wrote:
>> > Previously, the restore occured after printing the object in slub.
>> > After commit 47d911b02cbe ("slab: make check_object() more consistent"),
>> > the bytes are printed after the restore. This information about the bytes
>> > before the restore is highly valuable for debugging purpose.
>> > For instance, in a event of cache issue, it displays byte patterns
>> > by breaking them down into 64-bytes units. Without this information,
>> > we can only speculate on how it was broken. Hence the corrupted regions
>> > should be printed prior to the restoration process. However if an object
>> > breaks in multiple places, the same log may be output multiple times.
>> > Therefore the slub log is reported only once to prevent redundant printing,
>> > by sending a parameter indicating whether an error has occurred previously.
>> > 
>> > Changes in v3:
>> > - Change the parameter type of check_bytes_and_report.
>> > 
>> > Changes in v2:
>> > - Instead of using print_section every time on check_bytes_and_report,
>> > just print it once for the entire slub object before the restore.
>> > 
>> > Signed-off-by: Hyesoo Yu <hyesoo.yu@samsung.com>
>> > Change-Id: I73cf76c110eed62506643913517c957c05a29520
>> > ---
>> >  mm/slub.c | 29 ++++++++++++++---------------
>> >  1 file changed, 14 insertions(+), 15 deletions(-)
>> > 
>> 
>> > @@ -1212,11 +1213,14 @@ check_bytes_and_report(struct kmem_cache *s, struct slab *slab,
>> >  	if (slab_add_kunit_errors())
>> >  		goto skip_bug_print;
>> >  
>> > -	slab_bug(s, "%s overwritten", what);
>> >  	pr_err("0x%p-0x%p @offset=%tu. First byte 0x%x instead of 0x%x\n",
>> >  					fault, end - 1, fault - addr,
>> >  					fault[0], value);
>> >  
>> > +	scnprintf(buf, 100, "%s overwritten", what);
>> > +	if (slab_obj_print)
>> > +		object_err(s, slab, object, buf);
>> 
>> 
>> Wait, I think it's better to keep printing "%s overwritten" regardless
>> of slab_obj_print and only call __slab_err() if slab_obj_print == true
>> as discussed here [1]? Becuase in case there are multiple errors,
>> users should know.
>> 
>> [1] https://lore.kernel.org/all/2ff52c5e-4b6b-4b3d-9047-f00967315d3e@suse.cz
>> 
> 
> Hi,
> 
> __slab_err() doesn't include print_trainer(). It needs object_err().

print_trailer() could be used directly?

> How about including the specific error name 'what' to pr_err ?
> And then object_err would print "Object corrupt" at the beginning once
> without buf like below.

Could also work.

> 	if (slab_obj_print)
> 		object_err(s, slab, object, "Object corrupt");
> 
> 	pr_err("[%s] 0x%p-0x%p @offset=%tu. First byte 0x%x instead of 0x%x\n",
>  	       what, fault, end - 1, fault - addr, fault[0], value);

Probably in opposite order so object_err doesn't panic_on_warn before the
pr_err?

> Thanks,
> Regards.
>> -- 
>> Cheers,
>> Harry
>> 
> 
> 

Re: [PATCH v3 1/2] mm: slub: Print the broken data before restoring slub.

[Hyesoo Yu]

[-- Attachment #1: Type: text/plain, Size: 3462 bytes --]

On Mon, Feb 24, 2025 at 03:08:55PM +0100, Vlastimil Babka wrote:
> On 2/24/25 03:43, Hyesoo Yu wrote:
> > On Fri, Feb 21, 2025 at 05:16:01PM +0900, Harry Yoo wrote:
> >> On Thu, Feb 20, 2025 at 12:39:43PM +0900, Hyesoo Yu wrote:
> >> > Previously, the restore occured after printing the object in slub.
> >> > After commit 47d911b02cbe ("slab: make check_object() more consistent"),
> >> > the bytes are printed after the restore. This information about the bytes
> >> > before the restore is highly valuable for debugging purpose.
> >> > For instance, in a event of cache issue, it displays byte patterns
> >> > by breaking them down into 64-bytes units. Without this information,
> >> > we can only speculate on how it was broken. Hence the corrupted regions
> >> > should be printed prior to the restoration process. However if an object
> >> > breaks in multiple places, the same log may be output multiple times.
> >> > Therefore the slub log is reported only once to prevent redundant printing,
> >> > by sending a parameter indicating whether an error has occurred previously.
> >> > 
> >> > Changes in v3:
> >> > - Change the parameter type of check_bytes_and_report.
> >> > 
> >> > Changes in v2:
> >> > - Instead of using print_section every time on check_bytes_and_report,
> >> > just print it once for the entire slub object before the restore.
> >> > 
> >> > Signed-off-by: Hyesoo Yu <hyesoo.yu@samsung.com>
> >> > Change-Id: I73cf76c110eed62506643913517c957c05a29520
> >> > ---
> >> >  mm/slub.c | 29 ++++++++++++++---------------
> >> >  1 file changed, 14 insertions(+), 15 deletions(-)
> >> > 
> >> 
> >> > @@ -1212,11 +1213,14 @@ check_bytes_and_report(struct kmem_cache *s, struct slab *slab,
> >> >  	if (slab_add_kunit_errors())
> >> >  		goto skip_bug_print;
> >> >  
> >> > -	slab_bug(s, "%s overwritten", what);
> >> >  	pr_err("0x%p-0x%p @offset=%tu. First byte 0x%x instead of 0x%x\n",
> >> >  					fault, end - 1, fault - addr,
> >> >  					fault[0], value);
> >> >  
> >> > +	scnprintf(buf, 100, "%s overwritten", what);
> >> > +	if (slab_obj_print)
> >> > +		object_err(s, slab, object, buf);
> >> 
> >> 
> >> Wait, I think it's better to keep printing "%s overwritten" regardless
> >> of slab_obj_print and only call __slab_err() if slab_obj_print == true
> >> as discussed here [1]? Becuase in case there are multiple errors,
> >> users should know.
> >> 
> >> [1] https://lore.kernel.org/all/2ff52c5e-4b6b-4b3d-9047-f00967315d3e@suse.cz
> >> 
> > 
> > Hi,
> > 
> > __slab_err() doesn't include print_trainer(). It needs object_err().
> 
> print_trailer() could be used directly?
>

object_err calls print_trailer, add_taint and WARN_ON that we need to call here.
I think direct calling is just redundant.

> > How about including the specific error name 'what' to pr_err ?
> > And then object_err would print "Object corrupt" at the beginning once
> > without buf like below.
> 
> Could also work.
> 
> > 	if (slab_obj_print)
> > 		object_err(s, slab, object, "Object corrupt");
> > 
> > 	pr_err("[%s] 0x%p-0x%p @offset=%tu. First byte 0x%x instead of 0x%x\n",
> >  	       what, fault, end - 1, fault - addr, fault[0], value);
> 
> Probably in opposite order so object_err doesn't panic_on_warn before the
> pr_err?
> 

Yes, I tested and found that logs are not printed when panic_on_warn is enabled.
we first call pr_err and then call object_err.

> > Thanks,
> > Regards.
> >> -- 
> >> Cheers,
> >> Harry
> >> 
> > 
> > 
> 
> 

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v3 1/2] mm: slub: Print the broken data before restoring slub.

[Christoph Lameter (Ampere)]

On Thu, 20 Feb 2025, Hyesoo Yu wrote:

> @@ -1396,11 +1400,6 @@ static int check_object(struct kmem_cache *s, struct slab *slab,
>  		ret = 0;
>  	}
>
> -	if (!ret && !slab_in_kunit_test()) {


> -		print_trailer(s, slab, object);
> -		add_taint(TAINT_BAD_PAGE, LOCKDEP_NOW_UNRELIABLE);

No tainting and printing of the trailer anymore?

Re: [PATCH v3 1/2] mm: slub: Print the broken data before restoring slub.

[Hyesoo Yu]

[-- Attachment #1: Type: text/plain, Size: 591 bytes --]

On Mon, Feb 24, 2025 at 09:24:42AM -0800, Christoph Lameter (Ampere) wrote:
> On Thu, 20 Feb 2025, Hyesoo Yu wrote:
> 
> > @@ -1396,11 +1400,6 @@ static int check_object(struct kmem_cache *s, struct slab *slab,
> >  		ret = 0;
> >  	}
> >
> > -	if (!ret && !slab_in_kunit_test()) {
> 
> 
> > -		print_trailer(s, slab, object);
> > -		add_taint(TAINT_BAD_PAGE, LOCKDEP_NOW_UNRELIABLE);
> 
> No tainting and printing of the trailer anymore?
> 

If there is an error before on check_object, the check_bytes_and_report
calls object_err that calls print_trailer and add_traint.

Thanks,
Regards.

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

[PATCH v3 2/2] mm: slub: call WARN() when the slab detect an error

[Hyesoo Yu]

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 useful to use WARN() to catch errors at the point
of issue because WARN() could trigger panic for system debugging when
panic_on_warn is enabled. WARN() is added where to detect the error
on slab_err and object_err.

There are cases where slab_err is called before meaningful logs are
printed. If the WARN() in slab_err cause a panic, these logs will not
be printed. WARN() should called after these logs are printed. Thus
slab_err() is splited to __slab_err that calls the WARN() and it is
called after printing logs.

Changes in v3:
- move the WARN from slab_fix to slab_err, object_err to use WARN on
all error reporting paths.

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>
Change-Id: I90b2ea9ffc58e3826f7ae9f1a774bb48c2d43bf4
---
 mm/slub.c | 31 +++++++++++++++++++------------
 1 file changed, 19 insertions(+), 12 deletions(-)

diff --git a/mm/slub.c b/mm/slub.c
index de62fed12236..7f0583a71cda 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -1096,8 +1096,6 @@ static void print_trailer(struct kmem_cache *s, struct slab *slab, u8 *p)
 		/* Beginning of the filler is the free pointer */
 		print_section(KERN_ERR, "Padding  ", p + off,
 			      size_from_object(s) - off);
-
-	dump_stack();
 }
 
 static void object_err(struct kmem_cache *s, struct slab *slab,
@@ -1109,6 +1107,8 @@ static void object_err(struct kmem_cache *s, struct slab *slab,
 	slab_bug(s, "%s", reason);
 	print_trailer(s, slab, object);
 	add_taint(TAINT_BAD_PAGE, LOCKDEP_NOW_UNRELIABLE);
+
+	WARN_ON(1);
 }
 
 static bool freelist_corrupted(struct kmem_cache *s, struct slab *slab,
@@ -1125,6 +1125,14 @@ static bool freelist_corrupted(struct kmem_cache *s, struct slab *slab,
 	return false;
 }
 
+static void __slab_err(struct slab *slab)
+{
+	print_slab_info(slab);
+	add_taint(TAINT_BAD_PAGE, LOCKDEP_NOW_UNRELIABLE);
+
+	WARN_ON(1);
+}
+
 static __printf(3, 4) void slab_err(struct kmem_cache *s, struct slab *slab,
 			const char *fmt, ...)
 {
@@ -1138,9 +1146,7 @@ static __printf(3, 4) void slab_err(struct kmem_cache *s, struct slab *slab,
 	vsnprintf(buf, sizeof(buf), fmt, args);
 	va_end(args);
 	slab_bug(s, "%s", buf);
-	print_slab_info(slab);
-	dump_stack();
-	add_taint(TAINT_BAD_PAGE, LOCKDEP_NOW_UNRELIABLE);
+	__slab_err(slab);
 }
 
 static void init_object(struct kmem_cache *s, void *object, u8 val)
@@ -1316,9 +1322,10 @@ slab_pad_check(struct kmem_cache *s, struct slab *slab)
 	while (end > fault && end[-1] == POISON_INUSE)
 		end--;
 
-	slab_err(s, slab, "Padding overwritten. 0x%p-0x%p @offset=%tu",
-			fault, end - 1, fault - start);
+	slab_bug(s, "Padding overwritten. 0x%p-0x%p @offset=%tu",
+		 fault, end - 1, fault - start);
 	print_section(KERN_ERR, "Padding ", pad, remainder);
+	__slab_err(slab);
 
 	restore_bytes(s, "slab padding", POISON_INUSE, fault, end);
 }
@@ -5431,14 +5438,13 @@ static int calculate_sizes(struct kmem_cache_args *args, struct kmem_cache *s)
 	return !!oo_objects(s->oo);
 }
 
-static void list_slab_objects(struct kmem_cache *s, struct slab *slab,
-			      const char *text)
+static void list_slab_objects(struct kmem_cache *s, struct slab *slab)
 {
 #ifdef CONFIG_SLUB_DEBUG
 	void *addr = slab_address(slab);
 	void *p;
 
-	slab_err(s, slab, text, s->name);
+	slab_bug(s, "Objects remaining on __kmem_cache_shutdown()");
 
 	spin_lock(&object_map_lock);
 	__fill_map(object_map, s, slab);
@@ -5453,6 +5459,8 @@ static void list_slab_objects(struct kmem_cache *s, struct slab *slab,
 		}
 	}
 	spin_unlock(&object_map_lock);
+
+	__slab_err(slab);
 #endif
 }
 
@@ -5473,8 +5481,7 @@ static void free_partial(struct kmem_cache *s, struct kmem_cache_node *n)
 			remove_partial(n, slab);
 			list_add(&slab->slab_list, &discard);
 		} else {
-			list_slab_objects(s, slab,
-			  "Objects remaining in %s on __kmem_cache_shutdown()");
+			list_slab_objects(s, slab);
 		}
 	}
 	spin_unlock_irq(&n->list_lock);
-- 
2.28.0

Re: [PATCH v3 2/2] mm: slub: call WARN() when the slab detect an error

[Harry Yoo]

On Thu, Feb 20, 2025 at 12:39:44PM +0900, 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 useful to use WARN() to catch errors at the point
> of issue because WARN() could trigger panic for system debugging when
> panic_on_warn is enabled. WARN() is added where to detect the error
> on slab_err and object_err.
>
> There are cases where slab_err is called before meaningful logs are
> printed. If the WARN() in slab_err cause a panic, these logs will not
> be printed. WARN() should called after these logs are printed. Thus
> slab_err() is splited to __slab_err that calls the WARN() and it is
> called after printing logs.
> 
> Changes in v3:
> - move the WARN from slab_fix to slab_err, object_err to use WARN on
> all error reporting paths.
> 
> 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>
> Change-Id: I90b2ea9ffc58e3826f7ae9f1a774bb48c2d43bf4
> ---
>  mm/slub.c | 31 +++++++++++++++++++------------
>  1 file changed, 19 insertions(+), 12 deletions(-)
> 
> diff --git a/mm/slub.c b/mm/slub.c
> index de62fed12236..7f0583a71cda 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -5473,8 +5481,7 @@ static void free_partial(struct kmem_cache *s, struct kmem_cache_node *n)
>  			remove_partial(n, slab);
>  			list_add(&slab->slab_list, &discard);
>  		} else {
> -			list_slab_objects(s, slab,
> -			  "Objects remaining in %s on __kmem_cache_shutdown()");
> +			list_slab_objects(s, slab);

Could you remove WARN() in kmem_cache_destroy()?

When a cache is destroyed with remaining objects, two WARNINGs being are
printed, one from list_slab_objects() and another from kmem_cache_destroy().
The latter becomes redundant with this patch.

The WARN() is added there because it's good to catch such an error.
At that time, slab_err() and object_err() did not call WARN().

>  		}
>  	}
>  	spin_unlock_irq(&n->list_lock);
> -- 
> 2.28.0
> 

-- 
Cheers,
Harry

Re: [PATCH v3 2/2] mm: slub: call WARN() when the slab detect an error

[Hyesoo Yu]

[-- Attachment #1: Type: text/plain, Size: 2445 bytes --]

On Fri, Feb 21, 2025 at 05:30:59PM +0900, Harry Yoo wrote:
> On Thu, Feb 20, 2025 at 12:39:44PM +0900, 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 useful to use WARN() to catch errors at the point
> > of issue because WARN() could trigger panic for system debugging when
> > panic_on_warn is enabled. WARN() is added where to detect the error
> > on slab_err and object_err.
> >
> > There are cases where slab_err is called before meaningful logs are
> > printed. If the WARN() in slab_err cause a panic, these logs will not
> > be printed. WARN() should called after these logs are printed. Thus
> > slab_err() is splited to __slab_err that calls the WARN() and it is
> > called after printing logs.
> > 
> > Changes in v3:
> > - move the WARN from slab_fix to slab_err, object_err to use WARN on
> > all error reporting paths.
> > 
> > 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>
> > Change-Id: I90b2ea9ffc58e3826f7ae9f1a774bb48c2d43bf4
> > ---
> >  mm/slub.c | 31 +++++++++++++++++++------------
> >  1 file changed, 19 insertions(+), 12 deletions(-)
> > 
> > diff --git a/mm/slub.c b/mm/slub.c
> > index de62fed12236..7f0583a71cda 100644
> > --- a/mm/slub.c
> > +++ b/mm/slub.c
> > @@ -5473,8 +5481,7 @@ static void free_partial(struct kmem_cache *s, struct kmem_cache_node *n)
> >  			remove_partial(n, slab);
> >  			list_add(&slab->slab_list, &discard);
> >  		} else {
> > -			list_slab_objects(s, slab,
> > -			  "Objects remaining in %s on __kmem_cache_shutdown()");
> > +			list_slab_objects(s, slab);
> 
> Could you remove WARN() in kmem_cache_destroy()?
> 
> When a cache is destroyed with remaining objects, two WARNINGs being are
> printed, one from list_slab_objects() and another from kmem_cache_destroy().
> The latter becomes redundant with this patch.
> 
> The WARN() is added there because it's good to catch such an error.
> At that time, slab_err() and object_err() did not call WARN().
> 

Thank you for reviewing the part I missed.
I will remove the WARN in kmem_cache_destory().

Thanks,
Regards.

> >  		}
> >  	}
> >  	spin_unlock_irq(&n->list_lock);
> > -- 
> > 2.28.0
> > 
> 
> -- 
> Cheers,
> Harry
> 

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]