[PATCH] mm: slub: add panic_on_error to the debug facilities

[PATCH] mm: slub: add panic_on_error to the debug facilities

[Rafael Aquini]

Sometimes it is desirable to override SLUB's debug facilities
default behavior upon stumbling on a cache or object error
and just stop the execution in order to grab a coredump, at
the error-spotting time, instead of trying to fix the issue
and report in an attempt to keep the system rolling.

This patch introduces a new debug flag SLAB_PANIC_ON_ERROR,
along with its related SLUB-machinery, in order to extend
current slub_debug facilites and provide the aforementioned
behavior override.

Signed-off-by: Rafael Aquini <aquini@redhat.com>
---
 Documentation/vm/slub.rst |  2 ++
 include/linux/slab.h      |  2 ++
 mm/slab.h                 |  3 ++-
 mm/slub.c                 | 44 ++++++++++++++++++++++++++++++---------
 4 files changed, 40 insertions(+), 11 deletions(-)

diff --git a/Documentation/vm/slub.rst b/Documentation/vm/slub.rst
index 933ada4368ff..51b18c28ec78 100644
--- a/Documentation/vm/slub.rst
+++ b/Documentation/vm/slub.rst
@@ -54,6 +54,8 @@ Possible debug options are::
 			caused higher minimum slab orders
 	-		Switch all debugging off (useful if the kernel is
 			configured with CONFIG_SLUB_DEBUG_ON)
+	C		Toggle panic on error (crash) to allow for post-mortem
+			analysis of a coredump taken at the error-spotting time
 
 F.e. in order to boot just with sanity checks and red zoning one would specify::
 
diff --git a/include/linux/slab.h b/include/linux/slab.h
index 6d454886bcaf..e3496ad7859f 100644
--- a/include/linux/slab.h
+++ b/include/linux/slab.h
@@ -25,6 +25,8 @@
  */
 /* DEBUG: Perform (expensive) checks on alloc/free */
 #define SLAB_CONSISTENCY_CHECKS	((slab_flags_t __force)0x00000100U)
+/* DEBUG: panic on error (forced crash) */
+#define SLAB_PANIC_ON_ERROR	((slab_flags_t __force)0x00000200U)
 /* DEBUG: Red zone objs in a cache */
 #define SLAB_RED_ZONE		((slab_flags_t __force)0x00000400U)
 /* DEBUG: Poison objects */
diff --git a/mm/slab.h b/mm/slab.h
index 207c83ef6e06..27116f8683a1 100644
--- a/mm/slab.h
+++ b/mm/slab.h
@@ -198,7 +198,8 @@ static inline slab_flags_t kmem_cache_flags(unsigned int object_size,
 #define SLAB_DEBUG_FLAGS (SLAB_RED_ZONE | SLAB_POISON | SLAB_STORE_USER)
 #elif defined(CONFIG_SLUB_DEBUG)
 #define SLAB_DEBUG_FLAGS (SLAB_RED_ZONE | SLAB_POISON | SLAB_STORE_USER | \
-			  SLAB_TRACE | SLAB_CONSISTENCY_CHECKS)
+			  SLAB_TRACE | SLAB_CONSISTENCY_CHECKS | \
+			  SLAB_PANIC_ON_ERROR)
 #else
 #define SLAB_DEBUG_FLAGS (0)
 #endif
diff --git a/mm/slub.c b/mm/slub.c
index 9bf44955c4f1..8b4fc002b865 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -700,8 +700,6 @@ static void print_trailer(struct kmem_cache *s, struct page *page, u8 *p)
 		/* Beginning of the filler is the free pointer */
 		print_section(KERN_ERR, "Padding ", p + off,
 			      size_from_object(s) - off);
-
-	dump_stack();
 }
 
 void object_err(struct kmem_cache *s, struct page *page,
@@ -709,6 +707,9 @@ void object_err(struct kmem_cache *s, struct page *page,
 {
 	slab_bug(s, "%s", reason);
 	print_trailer(s, page, object);
+	if (unlikely(s->flags & SLAB_PANIC_ON_ERROR))
+		panic("BUG: %s: %s", s->name, reason);
+	dump_stack();
 }
 
 static __printf(3, 4) void slab_err(struct kmem_cache *s, struct page *page,
@@ -722,6 +723,8 @@ static __printf(3, 4) void slab_err(struct kmem_cache *s, struct page *page,
 	va_end(args);
 	slab_bug(s, "%s", buf);
 	print_page_info(page);
+	if (unlikely(s->flags & SLAB_PANIC_ON_ERROR))
+		panic("BUG: %s: %s", s->name, buf);
 	dump_stack();
 }
 
@@ -771,7 +774,7 @@ static int check_bytes_and_report(struct kmem_cache *s, struct page *page,
 					fault, end - 1, fault - addr,
 					fault[0], value);
 	print_trailer(s, page, object);
-
+	dump_stack();
 	restore_bytes(s, what, value, fault, end);
 	return 0;
 }
@@ -1173,13 +1176,14 @@ static inline int free_consistency_checks(struct kmem_cache *s,
 		if (!PageSlab(page)) {
 			slab_err(s, page, "Attempt to free object(0x%p) outside of slab",
 				 object);
-		} else if (!page->slab_cache) {
-			pr_err("SLUB <none>: no slab for object 0x%p.\n",
-			       object);
-			dump_stack();
-		} else
-			object_err(s, page, object,
-					"page slab pointer corrupt.");
+		} else {
+			char reason[80];
+
+			snprintf(reason, sizeof(reason),
+				 "page slab pointer corruption: 0x%p (0x%p expected)",
+				 page->slab_cache, s);
+			object_err(s, page, object, reason);
+		}
 		return 0;
 	}
 	return 1;
@@ -1291,6 +1295,9 @@ static int __init setup_slub_debug(char *str)
 			 */
 			disable_higher_order_debug = 1;
 			break;
+		case 'c':
+			slub_debug |= SLAB_PANIC_ON_ERROR;
+			break;
 		default:
 			pr_err("slub_debug option '%c' unknown. skipped\n",
 			       *str);
@@ -5312,6 +5319,22 @@ static ssize_t free_calls_show(struct kmem_cache *s, char *buf)
 	return list_locations(s, buf, TRACK_FREE);
 }
 SLAB_ATTR_RO(free_calls);
+
+static ssize_t
+panic_on_error_show(struct kmem_cache *s, char *buf)
+{
+	return sprintf(buf, "%d\n", !!(s->flags & SLAB_PANIC_ON_ERROR));
+}
+
+static ssize_t
+panic_on_error_store(struct kmem_cache *s, const char *buf, size_t length)
+{
+	s->flags &= ~SLAB_PANIC_ON_ERROR;
+	if (buf[0] == '1')
+		s->flags |= SLAB_PANIC_ON_ERROR;
+	return length;
+}
+SLAB_ATTR(panic_on_error);
 #endif /* CONFIG_SLUB_DEBUG */
 
 #ifdef CONFIG_FAILSLAB
@@ -5486,6 +5509,7 @@ static struct attribute *slab_attrs[] = {
 	&validate_attr.attr,
 	&alloc_calls_attr.attr,
 	&free_calls_attr.attr,
+	&panic_on_error_attr.attr,
 #endif
 #ifdef CONFIG_ZONE_DMA
 	&cache_dma_attr.attr,
-- 
2.25.4

Re: [PATCH] mm: slub: add panic_on_error to the debug facilities

[Qian Cai]

> On May 1, 2020, at 5:15 PM, Rafael Aquini <aquini@redhat.com> wrote:
> 
> Sometimes it is desirable to override SLUB's debug facilities
> default behavior upon stumbling on a cache or object error
> and just stop the execution in order to grab a coredump, at
> the error-spotting time, instead of trying to fix the issue
> and report in an attempt to keep the system rolling.
> 
> This patch introduces a new debug flag SLAB_PANIC_ON_ERROR,
> along with its related SLUB-machinery, in order to extend
> current slub_debug facilites and provide the aforementioned
> behavior override.

Instead of adding those things everywhere. How about adding something like panic_on_taint? Then, you could write specific taint flags you are interested in to that file because slab_bug() will taint it TAINT_BAD_PAGE.

Re: [PATCH] mm: slub: add panic_on_error to the debug facilities

[Andrew Morton]

On Fri,  1 May 2020 17:15:40 -0400 Rafael Aquini <aquini@redhat.com> wrote:

> Sometimes it is desirable to override SLUB's debug facilities
> default behavior upon stumbling on a cache or object error
> and just stop the execution in order to grab a coredump, at
> the error-spotting time, instead of trying to fix the issue
> and report in an attempt to keep the system rolling.
> 
> This patch introduces a new debug flag SLAB_PANIC_ON_ERROR,
> along with its related SLUB-machinery, in order to extend
> current slub_debug facilites and provide the aforementioned
> behavior override.
> 

Sees reasonable.

> --- a/Documentation/vm/slub.rst
> +++ b/Documentation/vm/slub.rst
> @@ -54,6 +54,8 @@ Possible debug options are::
>  			caused higher minimum slab orders
>  	-		Switch all debugging off (useful if the kernel is
>  			configured with CONFIG_SLUB_DEBUG_ON)
> +	C		Toggle panic on error (crash) to allow for post-mortem
> +			analysis of a coredump taken at the error-spotting time

nit: "toggle" means to switch to the other state.  But what we're doing
here is to set to the "on" state.  This:

--- a/Documentation/vm/slub.rst~mm-slub-add-panic_on_error-to-the-debug-facilities-fix
+++ a/Documentation/vm/slub.rst
@@ -49,12 +49,12 @@ Possible debug options are::
 	P		Poisoning (object and padding)
 	U		User tracking (free and alloc)
 	T		Trace (please only use on single slabs)
-	A		Toggle failslab filter mark for the cache
+	A		Enable failslab filter mark for the cache
 	O		Switch debugging off for caches that would have
 			caused higher minimum slab orders
 	-		Switch all debugging off (useful if the kernel is
 			configured with CONFIG_SLUB_DEBUG_ON)
-	C		Toggle panic on error (crash) to allow for post-mortem
+	C		Enable panic on error (crash) to allow for post-mortem
 			analysis of a coredump taken at the error-spotting time
 
 F.e. in order to boot just with sanity checks and red zoning one would specify::
_

Re: [PATCH] mm: slub: add panic_on_error to the debug facilities

[Rafael Aquini]

On Fri, May 01, 2020 at 02:37:51PM -0700, Andrew Morton wrote:
> On Fri,  1 May 2020 17:15:40 -0400 Rafael Aquini <aquini@redhat.com> wrote:
> 
> > Sometimes it is desirable to override SLUB's debug facilities
> > default behavior upon stumbling on a cache or object error
> > and just stop the execution in order to grab a coredump, at
> > the error-spotting time, instead of trying to fix the issue
> > and report in an attempt to keep the system rolling.
> > 
> > This patch introduces a new debug flag SLAB_PANIC_ON_ERROR,
> > along with its related SLUB-machinery, in order to extend
> > current slub_debug facilites and provide the aforementioned
> > behavior override.
> > 
> 
> Sees reasonable.
> 
> > --- a/Documentation/vm/slub.rst
> > +++ b/Documentation/vm/slub.rst
> > @@ -54,6 +54,8 @@ Possible debug options are::
> >  			caused higher minimum slab orders
> >  	-		Switch all debugging off (useful if the kernel is
> >  			configured with CONFIG_SLUB_DEBUG_ON)
> > +	C		Toggle panic on error (crash) to allow for post-mortem
> > +			analysis of a coredump taken at the error-spotting time
> 
> nit: "toggle" means to switch to the other state.  But what we're doing
> here is to set to the "on" state.  This:
>

Thanks Andrew, that's indeed much better.
 
> --- a/Documentation/vm/slub.rst~mm-slub-add-panic_on_error-to-the-debug-facilities-fix
> +++ a/Documentation/vm/slub.rst
> @@ -49,12 +49,12 @@ Possible debug options are::
>  	P		Poisoning (object and padding)
>  	U		User tracking (free and alloc)
>  	T		Trace (please only use on single slabs)
> -	A		Toggle failslab filter mark for the cache
> +	A		Enable failslab filter mark for the cache
>  	O		Switch debugging off for caches that would have
>  			caused higher minimum slab orders
>  	-		Switch all debugging off (useful if the kernel is
>  			configured with CONFIG_SLUB_DEBUG_ON)
> -	C		Toggle panic on error (crash) to allow for post-mortem
> +	C		Enable panic on error (crash) to allow for post-mortem
>  			analysis of a coredump taken at the error-spotting time
>  
>  F.e. in order to boot just with sanity checks and red zoning one would specify::
> _
> 
> 

Re: [PATCH] mm: slub: add panic_on_error to the debug facilities

[Rafael Aquini]

On Fri, May 01, 2020 at 05:29:19PM -0400, Qian Cai wrote:
> 
> 
> > On May 1, 2020, at 5:15 PM, Rafael Aquini <aquini@redhat.com> wrote:
> > 
> > Sometimes it is desirable to override SLUB's debug facilities
> > default behavior upon stumbling on a cache or object error
> > and just stop the execution in order to grab a coredump, at
> > the error-spotting time, instead of trying to fix the issue
> > and report in an attempt to keep the system rolling.
> > 
> > This patch introduces a new debug flag SLAB_PANIC_ON_ERROR,
> > along with its related SLUB-machinery, in order to extend
> > current slub_debug facilites and provide the aforementioned
> > behavior override.
> 
> Instead of adding those things everywhere. How about adding something like panic_on_taint? Then, you could write specific taint flags you are interested in to that file because slab_bug() will taint it TAINT_BAD_PAGE.
>
It seems like a good idea which also would required "adding things"
elsewhere, but doesn't look mutually exclusive with the approach here.

Thanks
-- Rafael  

Re: [PATCH] mm: slub: add panic_on_error to the debug facilities

[Qian Cai]

> On May 1, 2020, at 5:54 PM, Rafael Aquini <aquini@redhat.com> wrote:
> 
> On Fri, May 01, 2020 at 05:29:19PM -0400, Qian Cai wrote:
>> 
>> 
>>> On May 1, 2020, at 5:15 PM, Rafael Aquini <aquini@redhat.com> wrote:
>>> 
>>> Sometimes it is desirable to override SLUB's debug facilities
>>> default behavior upon stumbling on a cache or object error
>>> and just stop the execution in order to grab a coredump, at
>>> the error-spotting time, instead of trying to fix the issue
>>> and report in an attempt to keep the system rolling.
>>> 
>>> This patch introduces a new debug flag SLAB_PANIC_ON_ERROR,
>>> along with its related SLUB-machinery, in order to extend
>>> current slub_debug facilites and provide the aforementioned
>>> behavior override.
>> 
>> Instead of adding those things everywhere. How about adding something like panic_on_taint? Then, you could write specific taint flags you are interested in to that file because slab_bug() will taint it TAINT_BAD_PAGE.
>> 
> It seems like a good idea which also would required "adding things"
> elsewhere, but doesn't look mutually exclusive with the approach here.

No, it is mutually exclusive because panic_on_taint would do this same thing but saner.

The thing is that this request came up over and over again where people may want to panic the kernel because of TAINT_BAD_PAGE or some other places due to tainted.

Re: [PATCH] mm: slub: add panic_on_error to the debug facilities

[Qian Cai]

> On May 1, 2020, at 5:54 PM, Rafael Aquini <aquini@redhat.com> wrote:
> 
> It seems like a good idea which also would required "adding things"
> elsewhere, but doesn't look mutually exclusive with the approach here.

Also, what’s so special about these bad pages here that deserve a crash dump, but not other TAINT_BAD_PAGE places?

Re: [PATCH] mm: slub: add panic_on_error to the debug facilities

[Christopher Lameter]

On Fri, 1 May 2020, Rafael Aquini wrote:

> Sometimes it is desirable to override SLUB's debug facilities
> default behavior upon stumbling on a cache or object error
> and just stop the execution in order to grab a coredump, at
> the error-spotting time, instead of trying to fix the issue
> and report in an attempt to keep the system rolling.

The stopping of execution on an error is the default behavior. Usually
you get some OOPS somewhere when data is corrupted and that causes a core
dump.

SLUB can fix the issue and continue if enabled by specifying special
options on boot. That is *not* the default.

Re: [PATCH] mm: slub: add panic_on_error to the debug facilities

[Rafael Aquini]

On Fri, May 01, 2020 at 07:17:24PM -0400, Qian Cai wrote:
> 
> 
> > On May 1, 2020, at 5:54 PM, Rafael Aquini <aquini@redhat.com> wrote:
> > 
> > It seems like a good idea which also would required "adding things"
> > elsewhere, but doesn't look mutually exclusive with the approach here.
> 
> Also, what’s so special about these bad pages here that deserve a crash dump, but not other TAINT_BAD_PAGE places?
>

In that light, yes they're not as different and we could better leverage
a panic_on_taint mechanism (similar to panic_on_warn).

I'll get that posted soon.

Thanks.
-- Rafael 

Re: [PATCH] mm: slub: add panic_on_error to the debug facilities

[Rafael Aquini]

On Sat, May 02, 2020 at 11:16:30PM +0000, Christopher Lameter wrote:
> On Fri, 1 May 2020, Rafael Aquini wrote:
> 
> > Sometimes it is desirable to override SLUB's debug facilities
> > default behavior upon stumbling on a cache or object error
> > and just stop the execution in order to grab a coredump, at
> > the error-spotting time, instead of trying to fix the issue
> > and report in an attempt to keep the system rolling.
> 
> The stopping of execution on an error is the default behavior. Usually
> you get some OOPS somewhere when data is corrupted and that causes a core
> dump.
> 
> SLUB can fix the issue and continue if enabled by specifying special
> options on boot. That is *not* the default.
>
It is the default behavior when slub_debug is turned on, which is what
this patch is trying to override, when needed. We've been seeing the
need for such feature as, most often than not, by letting the system
running to crash somewhere else after hitting occurrences reported by 
slub_debug ends up clobbering clues to the original issue.

-- Rafael

Re: [PATCH] mm: slub: add panic_on_error to the debug facilities

[Christopher Lameter]

On Sun, 3 May 2020, Rafael Aquini wrote:

> On Sat, May 02, 2020 at 11:16:30PM +0000, Christopher Lameter wrote:
> > On Fri, 1 May 2020, Rafael Aquini wrote:
> >
> > > Sometimes it is desirable to override SLUB's debug facilities
> > > default behavior upon stumbling on a cache or object error
> > > and just stop the execution in order to grab a coredump, at
> > > the error-spotting time, instead of trying to fix the issue
> > > and report in an attempt to keep the system rolling.
> >
> > The stopping of execution on an error is the default behavior. Usually
> > you get some OOPS somewhere when data is corrupted and that causes a core
> > dump.
> >
> > SLUB can fix the issue and continue if enabled by specifying special
> > options on boot. That is *not* the default.
> >
> It is the default behavior when slub_debug is turned on, which is what
> this patch is trying to override, when needed. We've been seeing the
> need for such feature as, most often than not, by letting the system
> running to crash somewhere else after hitting occurrences reported by
> slub_debug ends up clobbering clues to the original issue.

Ok. The backtrace is not sufficient in that case?