[PATCH 0/9] slub_debug fixes and improvements

[PATCH 0/9] slub_debug fixes and improvements

[Vlastimil Babka]

Hi,

this series puts together the recent series "replace runtime slub_debug
toggling with more capable boot parameter" [4] (no longer RFC, thanks for
reviews), with older RFC [5] to introduce a static key for slub_debug. It's
because the removal of runtime toggling makes the static key addition simpler.
The last patch improves cache_from_obj() based on recent debugging effort [6].

We had recently reports [1,2] of troubles with runtime writes to various SLUB
per-cache sysfs files related to slub_debug or tuning leading to crashes. I have
inspected all those writable files and the rather unfortunate result is that
most of them are made read-only by this patchset, as fixing the issues doesn't
seem viable. Details for each are given in each patch (2-5)

The runtime toggles were however necessary for the use case described in [3],
so the first patch extends the slub_debug boot parameter syntax to achieve the
same configuration without runtime toggles. That should hopefully make the
changes more feasible.

Patches 6-9 reduce slub_debug overhead in cases where it's compiled in but not
enabled during boot, with a static key. Patch 9 also improves bug reporting
in cache_from_obj().

[1] https://lkml.kernel.org/r/1580379523-32272-1-git-send-email-vjitta@codeaurora.org
[2] https://lore.kernel.org/r/CAG48ez31PP--h6_FzVyfJ4H86QYczAFPdxtJHUEEan+7VJETAQ@mail.gmail.com
[3] https://lore.kernel.org/r/1383cd32-1ddc-4dac-b5f8-9c42282fa81c@codeaurora.org
[4] https://lore.kernel.org/r/20200602141519.7099-1-vbabka@suse.cz
[5] https://lore.kernel.org/r/20190404091531.9815-1-vbabka@suse.cz
[6] https://lore.kernel.org/r/4dc93ff8-f86e-f4c9-ebeb-6d3153a78d03@oracle.com

Vlastimil Babka (9):
  mm, slub: extend slub_debug syntax for multiple blocks
  mm, slub: make some slub_debug related attributes read-only
  mm, slub: remove runtime allocation order changes
  mm, slub: make remaining slub_debug related attributes read-only
  mm, slub: make reclaim_account attribute read-only
  mm, slub: introduce static key for slub_debug()
  mm, slub: introduce kmem_cache_debug_flags()
  mm, slub: extend checks guarded by slub_debug static key
  mm, slab/slub: move and improve cache_from_obj()

 .../admin-guide/kernel-parameters.txt         |   2 +-
 Documentation/vm/slub.rst                     |  37 +-
 mm/slab.c                                     |   8 +
 mm/slab.h                                     |  23 --
 mm/slub.c                                     | 374 +++++++++---------
 5 files changed, 222 insertions(+), 222 deletions(-)

-- 
2.26.2

[PATCH 1/9] mm, slub: extend slub_debug syntax for multiple blocks

[Vlastimil Babka]

The slub_debug kernel boot parameter can either apply a single set of options
to all caches or a list of caches. There is a use case where debugging is
applied for all caches and then disabled at runtime for specific caches, for
performance and memory consumption reasons [1]. As runtime changes are
dangerous, extend the boot parameter syntax so that multiple blocks of either
global or slab-specific options can be specified, with blocks delimited by ';'.
This will also support the use case of [1] without runtime changes.

For details see the updated Documentation/vm/slub.rst

[1] https://lore.kernel.org/r/1383cd32-1ddc-4dac-b5f8-9c42282fa81c@codeaurora.org

Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
Reviewed-by: Kees Cook <keescook@chromium.org>
---
 .../admin-guide/kernel-parameters.txt         |   2 +-
 Documentation/vm/slub.rst                     |  18 ++
 mm/slub.c                                     | 177 +++++++++++++-----
 3 files changed, 145 insertions(+), 52 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index fb95fad81c79..a408876b64c9 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -4604,7 +4604,7 @@
 			fragmentation.  Defaults to 1 for systems with
 			more than 32MB of RAM, 0 otherwise.
 
-	slub_debug[=options[,slabs]]	[MM, SLUB]
+	slub_debug[=options[,slabs][;[options[,slabs]]...]	[MM, SLUB]
 			Enabling slub_debug allows one to determine the
 			culprit if slab objects become corrupted. Enabling
 			slub_debug can create guard zones around objects and
diff --git a/Documentation/vm/slub.rst b/Documentation/vm/slub.rst
index 4eee598555c9..cfccb258cf42 100644
--- a/Documentation/vm/slub.rst
+++ b/Documentation/vm/slub.rst
@@ -41,6 +41,11 @@ slub_debug=<Debug-Options>,<slab name1>,<slab name2>,...
 	Enable options only for select slabs (no spaces
 	after a comma)
 
+Multiple blocks of options for all slabs or selected slabs can be given, with
+blocks of options delimited by ';'. The last of "all slabs" blocks is applied
+to all slabs except those that match one of the "select slabs" block. Options
+of the first "select slabs" blocks that matches the slab's name are applied.
+
 Possible debug options are::
 
 	F		Sanity checks on (enables SLAB_DEBUG_CONSISTENCY_CHECKS
@@ -83,6 +88,19 @@ in low memory situations or if there's high fragmentation of memory.  To
 
 	slub_debug=O
 
+You can apply different options to different list of slab names, using blocks
+of options. This will enable red zoning for dentry and user tracking for
+kmalloc. All other slabs will not get any debugging enabled::
+
+	slub_debug=Z,dentry;U,kmalloc-*
+
+You can also enable options (e.g. sanity checks and poisoning) for all caches
+except some that are deemed too performance critical and don't need to be
+debugged by specifying global debug options followed by a list of slab names
+with "-" as options::
+
+	slub_debug=FZ;-,zs_handle,zspage
+
 In case you forgot to enable debugging on the kernel command line: It is
 possible to enable debugging manually when the kernel is up. Look at the
 contents of::
diff --git a/mm/slub.c b/mm/slub.c
index b8f798b50d44..a53371426e06 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -499,7 +499,7 @@ static slab_flags_t slub_debug = DEBUG_DEFAULT_FLAGS;
 static slab_flags_t slub_debug;
 #endif
 
-static char *slub_debug_slabs;
+static char *slub_debug_string;
 static int disable_higher_order_debug;
 
 /*
@@ -1262,68 +1262,132 @@ static noinline int free_debug_processing(
 	return ret;
 }
 
-static int __init setup_slub_debug(char *str)
+/*
+ * Parse a block of slub_debug options. Blocks are delimited by ';'
+ *
+ * @str:    start of block
+ * @flags:  returns parsed flags, or DEBUG_DEFAULT_FLAGS if none specified
+ * @slabs:  return start of list of slabs, or NULL when there's no list
+ * @init:   assume this is initial parsing and not per-kmem-create parsing
+ *
+ * returns the start of next block if there's any, or NULL
+ */
+char *
+parse_slub_debug_flags(char *str, slab_flags_t *flags, char **slabs, bool init)
 {
-	slub_debug = DEBUG_DEFAULT_FLAGS;
-	if (*str++ != '=' || !*str)
-		/*
-		 * No options specified. Switch on full debugging.
-		 */
-		goto out;
+	bool higher_order_disable = false;
 
-	if (*str == ',')
+	/* Skip any completely empty blocks */
+	while (*str && *str == ';')
+		str++;
+
+	if (*str == ',') {
 		/*
 		 * No options but restriction on slabs. This means full
 		 * debugging for slabs matching a pattern.
 		 */
+		*flags = DEBUG_DEFAULT_FLAGS;
 		goto check_slabs;
+	}
+	*flags = 0;
 
-	slub_debug = 0;
-	if (*str == '-')
-		/*
-		 * Switch off all debugging measures.
-		 */
-		goto out;
-
-	/*
-	 * Determine which debug features should be switched on
-	 */
-	for (; *str && *str != ','; str++) {
+	/* Determine which debug features should be switched on */
+	for (; *str && *str != ',' && *str != ';'; str++) {
 		switch (tolower(*str)) {
+		case '-':
+			*flags = 0;
+			break;
 		case 'f':
-			slub_debug |= SLAB_CONSISTENCY_CHECKS;
+			*flags |= SLAB_CONSISTENCY_CHECKS;
 			break;
 		case 'z':
-			slub_debug |= SLAB_RED_ZONE;
+			*flags |= SLAB_RED_ZONE;
 			break;
 		case 'p':
-			slub_debug |= SLAB_POISON;
+			*flags |= SLAB_POISON;
 			break;
 		case 'u':
-			slub_debug |= SLAB_STORE_USER;
+			*flags |= SLAB_STORE_USER;
 			break;
 		case 't':
-			slub_debug |= SLAB_TRACE;
+			*flags |= SLAB_TRACE;
 			break;
 		case 'a':
-			slub_debug |= SLAB_FAILSLAB;
+			*flags |= SLAB_FAILSLAB;
 			break;
 		case 'o':
 			/*
 			 * Avoid enabling debugging on caches if its minimum
 			 * order would increase as a result.
 			 */
-			disable_higher_order_debug = 1;
+			higher_order_disable = true;
 			break;
 		default:
-			pr_err("slub_debug option '%c' unknown. skipped\n",
-			       *str);
+			if (init)
+				pr_err("slub_debug option '%c' unknown. skipped\n", *str);
 		}
 	}
-
 check_slabs:
 	if (*str == ',')
-		slub_debug_slabs = str + 1;
+		*slabs = ++str;
+	else
+		*slabs = NULL;
+
+	/* Skip over the slab list */
+	while (*str && *str != ';')
+		str++;
+
+	/* Skip any completely empty blocks */
+	while (*str && *str == ';')
+		str++;
+
+	if (init && higher_order_disable)
+		disable_higher_order_debug = 1;
+
+	if (*str)
+		return str;
+	else
+		return NULL;
+}
+
+static int __init setup_slub_debug(char *str)
+{
+	slab_flags_t flags;
+	char *saved_str;
+	char *slab_list;
+	bool global_slub_debug_changed = false;
+	bool slab_list_specified = false;
+
+	slub_debug = DEBUG_DEFAULT_FLAGS;
+	if (*str++ != '=' || !*str)
+		/*
+		 * No options specified. Switch on full debugging.
+		 */
+		goto out;
+
+	saved_str = str;
+	while (str) {
+		str = parse_slub_debug_flags(str, &flags, &slab_list, true);
+
+		if (!slab_list) {
+			slub_debug = flags;
+			global_slub_debug_changed = true;
+		} else {
+			slab_list_specified = true;
+		}
+	}
+
+	/*
+	 * For backwards compatibility, a single list of flags with list of
+	 * slabs means debugging is only enabled for those slabs, so the global
+	 * slub_debug should be 0. We can extended that to multiple lists as
+	 * long as there is no option specifying flags without a slab list.
+	 */
+	if (slab_list_specified) {
+		if (!global_slub_debug_changed)
+			slub_debug = 0;
+		slub_debug_string = saved_str;
+	}
 out:
 	if ((static_branch_unlikely(&init_on_alloc) ||
 	     static_branch_unlikely(&init_on_free)) &&
@@ -1352,36 +1416,47 @@ slab_flags_t kmem_cache_flags(unsigned int object_size,
 {
 	char *iter;
 	size_t len;
+	char *next_block;
+	slab_flags_t block_flags;
 
 	/* If slub_debug = 0, it folds into the if conditional. */
-	if (!slub_debug_slabs)
+	if (!slub_debug_string)
 		return flags | slub_debug;
 
 	len = strlen(name);
-	iter = slub_debug_slabs;
-	while (*iter) {
-		char *end, *glob;
-		size_t cmplen;
-
-		end = strchrnul(iter, ',');
+	next_block = slub_debug_string;
+	/* Go through all blocks of debug options, see if any matches our slab's name */
+	while (next_block) {
+		next_block = parse_slub_debug_flags(next_block, &block_flags, &iter, false);
+		if (!iter)
+			continue;
+		/* Found a block that has a slab list, search it */
+		while (*iter) {
+			char *end, *glob;
+			size_t cmplen;
+
+			end = strchrnul(iter, ',');
+			if (next_block && next_block < end)
+				end = next_block - 1;
+
+			glob = strnchr(iter, end - iter, '*');
+			if (glob)
+				cmplen = glob - iter;
+			else
+				cmplen = max_t(size_t, len, (end - iter));
 
-		glob = strnchr(iter, end - iter, '*');
-		if (glob)
-			cmplen = glob - iter;
-		else
-			cmplen = max_t(size_t, len, (end - iter));
+			if (!strncmp(name, iter, cmplen)) {
+				flags |= block_flags;
+				return flags;
+			}
 
-		if (!strncmp(name, iter, cmplen)) {
-			flags |= slub_debug;
-			break;
+			if (!*end || *end == ';')
+				break;
+			iter = end + 1;
 		}
-
-		if (!*end)
-			break;
-		iter = end + 1;
 	}
 
-	return flags;
+	return slub_debug;
 }
 #else /* !CONFIG_SLUB_DEBUG */
 static inline void setup_object_debug(struct kmem_cache *s,
-- 
2.26.2

[PATCH 2/9] mm, slub: make some slub_debug related attributes read-only

[Vlastimil Babka]

SLUB_DEBUG creates several files under /sys/kernel/slab/<cache>/ that can be
read to check if the respective debugging options are enabled for given cache.
The options can be also toggled at runtime by writing into the files. Some of
those, namely red_zone, poison, and store_user can be toggled only when no
objects yet exist in the cache.

Vijayanand reports [1] that there is a problem with freelist randomization if
changing the debugging option's state results in different number of objects
per page, and the random sequence cache needs thus needs to be recomputed.

However, another problem is that the check for "no objects yet exist in the
cache" is racy, as noted by Jann [2] and fixing that would add overhead or
otherwise complicate the allocation/freeing paths. Thus it would be much
simpler just to remove the runtime toggling support. The documentation
describes it's "In case you forgot to enable debugging on the kernel command
line", but the neccessity of having no objects limits its usefulness anyway for
many caches.

Vijayanand describes an use case [3] where debugging is enabled for all but
zram caches for memory overhead reasons, and using the runtime toggles was the
only way to achieve such configuration. After the previous patch it's now
possible to do that directly from the kernel boot option, so we can remove the
dangerous runtime toggles by making the /sys attribute files read-only.

While updating it, also improve the documentation of the debugging /sys files.

[1] https://lkml.kernel.org/r/1580379523-32272-1-git-send-email-vjitta@codeaurora.org
[2] https://lore.kernel.org/r/CAG48ez31PP--h6_FzVyfJ4H86QYczAFPdxtJHUEEan+7VJETAQ@mail.gmail.com
[3] https://lore.kernel.org/r/1383cd32-1ddc-4dac-b5f8-9c42282fa81c@codeaurora.org

Reported-by: Vijayanand Jitta <vjitta@codeaurora.org>
Reported-by: Jann Horn <jannh@google.com>
Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
Reviewed-by: Kees Cook <keescook@chromium.org>
Acked-by: Roman Gushchin <guro@fb.com>
---
 Documentation/vm/slub.rst | 28 ++++++++++++++----------
 mm/slub.c                 | 46 +++------------------------------------
 2 files changed, 20 insertions(+), 54 deletions(-)

diff --git a/Documentation/vm/slub.rst b/Documentation/vm/slub.rst
index cfccb258cf42..36241dfba024 100644
--- a/Documentation/vm/slub.rst
+++ b/Documentation/vm/slub.rst
@@ -101,20 +101,26 @@ debugged by specifying global debug options followed by a list of slab names
 
 	slub_debug=FZ;-,zs_handle,zspage
 
-In case you forgot to enable debugging on the kernel command line: It is
-possible to enable debugging manually when the kernel is up. Look at the
-contents of::
+The state of each debug option for a slab can be found in the respective files
+under::
 
 	/sys/kernel/slab/<slab name>/
 
-Look at the writable files. Writing 1 to them will enable the
-corresponding debug option. All options can be set on a slab that does
-not contain objects. If the slab already contains objects then sanity checks
-and tracing may only be enabled. The other options may cause the realignment
-of objects.
-
-Careful with tracing: It may spew out lots of information and never stop if
-used on the wrong slab.
+If the file contains 1, the option is enabled, 0 means disabled. The debug
+options from the ``slub_debug`` parameter translate to the following files::
+
+	F	sanity_checks
+	Z	red_zone
+	P	poison
+	U	store_user
+	T	trace
+	A	failslab
+
+The sanity_checks, trace and failslab files are writable, so writing 1 or 0
+will enable or disable the option at runtime. The writes to trace and failslab
+may return -EINVAL if the cache is subject to slab merging. Careful with
+tracing: It may spew out lots of information and never stop if used on the
+wrong slab.
 
 Slab merging
 ============
diff --git a/mm/slub.c b/mm/slub.c
index a53371426e06..e254164d6cae 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -5351,61 +5351,21 @@ static ssize_t red_zone_show(struct kmem_cache *s, char *buf)
 	return sprintf(buf, "%d\n", !!(s->flags & SLAB_RED_ZONE));
 }
 
-static ssize_t red_zone_store(struct kmem_cache *s,
-				const char *buf, size_t length)
-{
-	if (any_slab_objects(s))
-		return -EBUSY;
-
-	s->flags &= ~SLAB_RED_ZONE;
-	if (buf[0] == '1') {
-		s->flags |= SLAB_RED_ZONE;
-	}
-	calculate_sizes(s, -1);
-	return length;
-}
-SLAB_ATTR(red_zone);
+SLAB_ATTR_RO(red_zone);
 
 static ssize_t poison_show(struct kmem_cache *s, char *buf)
 {
 	return sprintf(buf, "%d\n", !!(s->flags & SLAB_POISON));
 }
 
-static ssize_t poison_store(struct kmem_cache *s,
-				const char *buf, size_t length)
-{
-	if (any_slab_objects(s))
-		return -EBUSY;
-
-	s->flags &= ~SLAB_POISON;
-	if (buf[0] == '1') {
-		s->flags |= SLAB_POISON;
-	}
-	calculate_sizes(s, -1);
-	return length;
-}
-SLAB_ATTR(poison);
+SLAB_ATTR_RO(poison);
 
 static ssize_t store_user_show(struct kmem_cache *s, char *buf)
 {
 	return sprintf(buf, "%d\n", !!(s->flags & SLAB_STORE_USER));
 }
 
-static ssize_t store_user_store(struct kmem_cache *s,
-				const char *buf, size_t length)
-{
-	if (any_slab_objects(s))
-		return -EBUSY;
-
-	s->flags &= ~SLAB_STORE_USER;
-	if (buf[0] == '1') {
-		s->flags &= ~__CMPXCHG_DOUBLE;
-		s->flags |= SLAB_STORE_USER;
-	}
-	calculate_sizes(s, -1);
-	return length;
-}
-SLAB_ATTR(store_user);
+SLAB_ATTR_RO(store_user);
 
 static ssize_t validate_show(struct kmem_cache *s, char *buf)
 {
-- 
2.26.2

[PATCH 3/9] mm, slub: remove runtime allocation order changes

[Vlastimil Babka]

SLUB allows runtime changing of page allocation order by writing into the
/sys/kernel/slab/<cache>/order file. Jann has reported [1] that this interface
allows the order to be set too small, leading to crashes.

While it's possible to fix the immediate issue, closer inspection reveals
potential races. Storing the new order calls calculate_sizes() which
non-atomically updates a lot of kmem_cache fields while the cache is still in
use. Unexpected behavior might occur even if the fields are set to the same
value as they were.

This could be fixed by splitting out the part of calculate_sizes() that depends
on forced_order, so that we only update kmem_cache.oo field. This could still
race with init_cache_random_seq(), shuffle_freelist(), allocate_slab(). Perhaps
it's possible to audit and e.g. add some READ_ONCE/WRITE_ONCE accesses, it
might be easier just to remove the runtime order changes, which is what this
patch does. If there are valid usecases for per-cache order setting, we could
e.g. extend the boot parameters to do that.

[1] https://lore.kernel.org/r/CAG48ez31PP--h6_FzVyfJ4H86QYczAFPdxtJHUEEan+7VJETAQ@mail.gmail.com

Reported-by: Jann Horn <jannh@google.com>
Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
Reviewed-by: Kees Cook <keescook@chromium.org>
Acked-by: Roman Gushchin <guro@fb.com>
---
 mm/slub.c | 19 +------------------
 1 file changed, 1 insertion(+), 18 deletions(-)

diff --git a/mm/slub.c b/mm/slub.c
index e254164d6cae..c5f3f2424392 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -5111,28 +5111,11 @@ static ssize_t objs_per_slab_show(struct kmem_cache *s, char *buf)
 }
 SLAB_ATTR_RO(objs_per_slab);
 
-static ssize_t order_store(struct kmem_cache *s,
-				const char *buf, size_t length)
-{
-	unsigned int order;
-	int err;
-
-	err = kstrtouint(buf, 10, &order);
-	if (err)
-		return err;
-
-	if (order > slub_max_order || order < slub_min_order)
-		return -EINVAL;
-
-	calculate_sizes(s, order);
-	return length;
-}
-
 static ssize_t order_show(struct kmem_cache *s, char *buf)
 {
 	return sprintf(buf, "%u\n", oo_order(s->oo));
 }
-SLAB_ATTR(order);
+SLAB_ATTR_RO(order);
 
 static ssize_t min_partial_show(struct kmem_cache *s, char *buf)
 {
-- 
2.26.2

[PATCH 4/9] mm, slub: make remaining slub_debug related attributes read-only

[Vlastimil Babka]

SLUB_DEBUG creates several files under /sys/kernel/slab/<cache>/ that can be
read to check if the respective debugging options are enabled for given cache.
Some options, namely sanity_checks, trace, and failslab can be also enabled and
disabled at runtime by writing into the files.

The runtime toggling is racy. Some options disable __CMPXCHG_DOUBLE when
enabled, which means that in case of concurrent allocations, some can still use
__CMPXCHG_DOUBLE and some not, leading to potential corruption. The s->flags
field is also not updated or checked atomically. The simplest solution is to
remove the runtime toggling. The extended slub_debug boot parameter syntax
introduced by earlier patch should allow to fine-tune the debugging
configuration during boot with same granularity.

Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
Reviewed-by: Kees Cook <keescook@chromium.org>
Acked-by: Roman Gushchin <guro@fb.com>
---
 Documentation/vm/slub.rst |  7 ++---
 mm/slub.c                 | 62 ++-------------------------------------
 2 files changed, 5 insertions(+), 64 deletions(-)

diff --git a/Documentation/vm/slub.rst b/Documentation/vm/slub.rst
index 36241dfba024..289d231cee97 100644
--- a/Documentation/vm/slub.rst
+++ b/Documentation/vm/slub.rst
@@ -116,11 +116,8 @@ If the file contains 1, the option is enabled, 0 means disabled. The debug
 	T	trace
 	A	failslab
 
-The sanity_checks, trace and failslab files are writable, so writing 1 or 0
-will enable or disable the option at runtime. The writes to trace and failslab
-may return -EINVAL if the cache is subject to slab merging. Careful with
-tracing: It may spew out lots of information and never stop if used on the
-wrong slab.
+Careful with tracing: It may spew out lots of information and never stop if
+used on the wrong slab.
 
 Slab merging
 ============
diff --git a/mm/slub.c b/mm/slub.c
index c5f3f2424392..0108829838e7 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -5056,20 +5056,6 @@ static ssize_t show_slab_objects(struct kmem_cache *s,
 	return x + sprintf(buf + x, "\n");
 }
 
-#ifdef CONFIG_SLUB_DEBUG
-static int any_slab_objects(struct kmem_cache *s)
-{
-	int node;
-	struct kmem_cache_node *n;
-
-	for_each_kmem_cache_node(s, node, n)
-		if (atomic_long_read(&n->total_objects))
-			return 1;
-
-	return 0;
-}
-#endif
-
 #define to_slab_attr(n) container_of(n, struct slab_attribute, attr)
 #define to_slab(n) container_of(n, struct kmem_cache, kobj)
 
@@ -5291,43 +5277,13 @@ static ssize_t sanity_checks_show(struct kmem_cache *s, char *buf)
 {
 	return sprintf(buf, "%d\n", !!(s->flags & SLAB_CONSISTENCY_CHECKS));
 }
-
-static ssize_t sanity_checks_store(struct kmem_cache *s,
-				const char *buf, size_t length)
-{
-	s->flags &= ~SLAB_CONSISTENCY_CHECKS;
-	if (buf[0] == '1') {
-		s->flags &= ~__CMPXCHG_DOUBLE;
-		s->flags |= SLAB_CONSISTENCY_CHECKS;
-	}
-	return length;
-}
-SLAB_ATTR(sanity_checks);
+SLAB_ATTR_RO(sanity_checks);
 
 static ssize_t trace_show(struct kmem_cache *s, char *buf)
 {
 	return sprintf(buf, "%d\n", !!(s->flags & SLAB_TRACE));
 }
-
-static ssize_t trace_store(struct kmem_cache *s, const char *buf,
-							size_t length)
-{
-	/*
-	 * Tracing a merged cache is going to give confusing results
-	 * as well as cause other issues like converting a mergeable
-	 * cache into an umergeable one.
-	 */
-	if (s->refcount > 1)
-		return -EINVAL;
-
-	s->flags &= ~SLAB_TRACE;
-	if (buf[0] == '1') {
-		s->flags &= ~__CMPXCHG_DOUBLE;
-		s->flags |= SLAB_TRACE;
-	}
-	return length;
-}
-SLAB_ATTR(trace);
+SLAB_ATTR_RO(trace);
 
 static ssize_t red_zone_show(struct kmem_cache *s, char *buf)
 {
@@ -5391,19 +5347,7 @@ static ssize_t failslab_show(struct kmem_cache *s, char *buf)
 {
 	return sprintf(buf, "%d\n", !!(s->flags & SLAB_FAILSLAB));
 }
-
-static ssize_t failslab_store(struct kmem_cache *s, const char *buf,
-							size_t length)
-{
-	if (s->refcount > 1)
-		return -EINVAL;
-
-	s->flags &= ~SLAB_FAILSLAB;
-	if (buf[0] == '1')
-		s->flags |= SLAB_FAILSLAB;
-	return length;
-}
-SLAB_ATTR(failslab);
+SLAB_ATTR_RO(failslab);
 #endif
 
 static ssize_t shrink_show(struct kmem_cache *s, char *buf)
-- 
2.26.2

[PATCH 5/9] mm, slub: make reclaim_account attribute read-only

[Vlastimil Babka]

The attribute reflects the SLAB_RECLAIM_ACCOUNT cache flag. It's not clear why
this attribute was writable in the first place, as it's tied to how the cache
is used by its creator, it's not a user tunable. Furthermore:

- it affects slab merging, but that's not being checked while toggled
- if affects whether __GFP_RECLAIMABLE flag is used to allocate page, but
  the runtime toggle doesn't update allocflags
- it affects cache_vmstat_idx() so runtime toggling might lead to incosistency
  of NR_SLAB_RECLAIMABLE and NR_SLAB_UNRECLAIMABLE

Thus make it read-only.

Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
Reviewed-by: Kees Cook <keescook@chromium.org>
Acked-by: Roman Gushchin <guro@fb.com>
---
 mm/slub.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/mm/slub.c b/mm/slub.c
index 0108829838e7..8dd2925448ae 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -5223,16 +5223,7 @@ static ssize_t reclaim_account_show(struct kmem_cache *s, char *buf)
 {
 	return sprintf(buf, "%d\n", !!(s->flags & SLAB_RECLAIM_ACCOUNT));
 }
-
-static ssize_t reclaim_account_store(struct kmem_cache *s,
-				const char *buf, size_t length)
-{
-	s->flags &= ~SLAB_RECLAIM_ACCOUNT;
-	if (buf[0] == '1')
-		s->flags |= SLAB_RECLAIM_ACCOUNT;
-	return length;
-}
-SLAB_ATTR(reclaim_account);
+SLAB_ATTR_RO(reclaim_account);
 
 static ssize_t hwcache_align_show(struct kmem_cache *s, char *buf)
 {
-- 
2.26.2

[PATCH 6/9] mm, slub: introduce static key for slub_debug()

[Vlastimil Babka]

One advantage of CONFIG_SLUB_DEBUG is that a generic distro kernel can be built
with the option enabled, but it's inactive until simply enabled on boot,
without rebuilding the kernel. With a static key, we can further eliminate the
overhead of checking whether a cache has a particular debug flag enabled if we
know that there are no such caches (slub_debug was not enabled during boot). We
use the same mechanism also for e.g.  page_owner, debug_pagealloc or kmemcg
functionality.

This patch introduces the static key and makes the general check for per-cache
debug flags kmem_cache_debug() use it. This benefits several call sites,
including (slow path but still rather frequent) __slab_free(). The next patches
will add more uses.

Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
---
 mm/slub.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/mm/slub.c b/mm/slub.c
index 8dd2925448ae..24d3e5f832aa 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -114,13 +114,21 @@
  * 			the fast path and disables lockless freelists.
  */
 
+#ifdef CONFIG_SLUB_DEBUG
+#ifdef CONFIG_SLUB_DEBUG_ON
+DEFINE_STATIC_KEY_TRUE(slub_debug_enabled);
+#else
+DEFINE_STATIC_KEY_FALSE(slub_debug_enabled);
+#endif
+#endif
+
 static inline int kmem_cache_debug(struct kmem_cache *s)
 {
 #ifdef CONFIG_SLUB_DEBUG
-	return unlikely(s->flags & SLAB_DEBUG_FLAGS);
-#else
-	return 0;
+	if (static_branch_unlikely(&slub_debug_enabled))
+		return s->flags & SLAB_DEBUG_FLAGS;
 #endif
+	return 0;
 }
 
 void *fixup_red_left(struct kmem_cache *s, void *p)
@@ -1389,6 +1397,8 @@ static int __init setup_slub_debug(char *str)
 		slub_debug_string = saved_str;
 	}
 out:
+	if (slub_debug != 0 || slub_debug_string)
+		static_branch_enable(&slub_debug_enabled);
 	if ((static_branch_unlikely(&init_on_alloc) ||
 	     static_branch_unlikely(&init_on_free)) &&
 	    (slub_debug & SLAB_POISON))
-- 
2.26.2

[PATCH 7/9] mm, slub: introduce kmem_cache_debug_flags()

[Vlastimil Babka]

There are few places that call kmem_cache_debug(s) (which tests if any of debug
flags are enabled for a cache) immediatelly followed by a test for a specific
flag. The compiler can probably eliminate the extra check, but we can make the
code nicer by introducing kmem_cache_debug_flags() that works like
kmem_cache_debug() (including the static key check) but tests for specifig
flag(s). The next patches will add more users.

Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
---
 mm/slub.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/mm/slub.c b/mm/slub.c
index 24d3e5f832aa..c8e8b4ae2451 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -122,18 +122,28 @@ DEFINE_STATIC_KEY_FALSE(slub_debug_enabled);
 #endif
 #endif
 
-static inline int kmem_cache_debug(struct kmem_cache *s)
+/*
+ * Returns true if any of the specified slub_debug flags is enabled for the
+ * cache. Use only for flags parsed by setup_slub_debug() as it also enables
+ * the static key.
+ */
+static inline int kmem_cache_debug_flags(struct kmem_cache *s, slab_flags_t flags)
 {
 #ifdef CONFIG_SLUB_DEBUG
 	if (static_branch_unlikely(&slub_debug_enabled))
-		return s->flags & SLAB_DEBUG_FLAGS;
+		return s->flags & flags;
 #endif
 	return 0;
 }
 
+static inline int kmem_cache_debug(struct kmem_cache *s)
+{
+	return kmem_cache_debug_flags(s, SLAB_DEBUG_FLAGS);
+}
+
 void *fixup_red_left(struct kmem_cache *s, void *p)
 {
-	if (kmem_cache_debug(s) && s->flags & SLAB_RED_ZONE)
+	if (kmem_cache_debug_flags(s, SLAB_RED_ZONE))
 		p += s->red_left_pad;
 
 	return p;
@@ -4076,7 +4086,7 @@ void __check_heap_object(const void *ptr, unsigned long n, struct page *page,
 	offset = (ptr - page_address(page)) % s->size;
 
 	/* Adjust for redzone and reject if within the redzone. */
-	if (kmem_cache_debug(s) && s->flags & SLAB_RED_ZONE) {
+	if (kmem_cache_debug_flags(s, SLAB_RED_ZONE)) {
 		if (offset < s->red_left_pad)
 			usercopy_abort("SLUB object in left red zone",
 				       s->name, to_user, offset, n);
-- 
2.26.2

[PATCH 8/9] mm, slub: extend checks guarded by slub_debug static key

[Vlastimil Babka]

There are few more places in SLUB that could benefit from reduced overhead of
the static key introduced by a previous patch:

- setup_object_debug() called on each object in newly allocated slab page
- setup_page_debug() called on newly allocated slab page
- __free_slab() called on freed slab page

Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
---
 mm/slub.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/mm/slub.c b/mm/slub.c
index c8e8b4ae2451..efb08f2e9c66 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -1130,7 +1130,7 @@ static inline void dec_slabs_node(struct kmem_cache *s, int node, int objects)
 static void setup_object_debug(struct kmem_cache *s, struct page *page,
 								void *object)
 {
-	if (!(s->flags & (SLAB_STORE_USER|SLAB_RED_ZONE|__OBJECT_POISON)))
+	if (!kmem_cache_debug_flags(s, SLAB_STORE_USER|SLAB_RED_ZONE|__OBJECT_POISON))
 		return;
 
 	init_object(s, object, SLUB_RED_INACTIVE);
@@ -1140,7 +1140,7 @@ static void setup_object_debug(struct kmem_cache *s, struct page *page,
 static
 void setup_page_debug(struct kmem_cache *s, struct page *page, void *addr)
 {
-	if (!(s->flags & SLAB_POISON))
+	if (!kmem_cache_debug_flags(s, SLAB_POISON))
 		return;
 
 	metadata_access_enable();
@@ -1857,7 +1857,7 @@ static void __free_slab(struct kmem_cache *s, struct page *page)
 	int order = compound_order(page);
 	int pages = 1 << order;
 
-	if (s->flags & SLAB_CONSISTENCY_CHECKS) {
+	if (kmem_cache_debug_flags(s, SLAB_CONSISTENCY_CHECKS)) {
 		void *p;
 
 		slab_pad_check(s, page);
-- 
2.26.2

[PATCH 9/9] mm, slab/slub: move and improve cache_from_obj()

[Vlastimil Babka]

The function cache_from_obj() was added by commit b9ce5ef49f00 ("sl[au]b:
always get the cache from its page in kmem_cache_free()") to support kmemcg,
where per-memcg cache can be different from the root one, so we can't use
the kmem_cache pointer given to kmem_cache_free().

Prior to that commit, SLUB already had debugging check+warning that could be
enabled to compare the given kmem_cache pointer to one referenced by the slab
page where the object-to-be-freed resides. This check was moved to
cache_from_obj(). Later the check was also enabled for SLAB_FREELIST_HARDENED
configs by commit 598a0717a816 ("mm/slab: validate cache membership under
freelist hardening").

These checks and warnings can be useful especially for the debugging, which can
be improved. Commit 598a0717a816 changed the pr_err() with WARN_ON_ONCE() to
WARN_ONCE() so only the first hit is now reported, others are silent. This
patch changes it to WARN() so that all errors are reported.

It's also useful to print SLUB allocation/free tracking info for the offending
object, if tracking is enabled. We could export the SLUB print_tracking()
function and provide an empty one for SLAB, or realize that both the debugging
and hardening cases in cache_from_obj() are only supported by SLUB anyway. So
this patch moves cache_from_obj() from slab.h to separate instances in slab.c
and slub.c, where the SLAB version only does the kmemcg lookup and even could
be completely removed once the kmemcg rework [1] is merged. The SLUB version
can thus easily use the print_tracking() function. It can also use the
kmem_cache_debug_flags() static key check for improved performance in kernels
without the hardening and with debugging not enabled on boot.

[1] https://lore.kernel.org/r/20200608230654.828134-18-guro@fb.com

Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
---
 mm/slab.c |  8 ++++++++
 mm/slab.h | 23 -----------------------
 mm/slub.c | 21 +++++++++++++++++++++
 3 files changed, 29 insertions(+), 23 deletions(-)

diff --git a/mm/slab.c b/mm/slab.c
index 9350062ffc1a..6134c4c36d4c 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -3672,6 +3672,14 @@ void *__kmalloc_track_caller(size_t size, gfp_t flags, unsigned long caller)
 }
 EXPORT_SYMBOL(__kmalloc_track_caller);
 
+static inline struct kmem_cache *cache_from_obj(struct kmem_cache *s, void *x)
+{
+	if (memcg_kmem_enabled())
+		return virt_to_cache(x);
+	else
+		return s;
+}
+
 /**
  * kmem_cache_free - Deallocate an object
  * @cachep: The cache the allocation was from.
diff --git a/mm/slab.h b/mm/slab.h
index 815e4e9a94cd..c0c4244f75da 100644
--- a/mm/slab.h
+++ b/mm/slab.h
@@ -503,29 +503,6 @@ static __always_inline void uncharge_slab_page(struct page *page, int order,
 	memcg_uncharge_slab(page, order, s);
 }
 
-static inline struct kmem_cache *cache_from_obj(struct kmem_cache *s, void *x)
-{
-	struct kmem_cache *cachep;
-
-	/*
-	 * When kmemcg is not being used, both assignments should return the
-	 * same value. but we don't want to pay the assignment price in that
-	 * case. If it is not compiled in, the compiler should be smart enough
-	 * to not do even the assignment. In that case, slab_equal_or_root
-	 * will also be a constant.
-	 */
-	if (!memcg_kmem_enabled() &&
-	    !IS_ENABLED(CONFIG_SLAB_FREELIST_HARDENED) &&
-	    !unlikely(s->flags & SLAB_CONSISTENCY_CHECKS))
-		return s;
-
-	cachep = virt_to_cache(x);
-	WARN_ONCE(cachep && !slab_equal_or_root(cachep, s),
-		  "%s: Wrong slab cache. %s but object is from %s\n",
-		  __func__, s->name, cachep->name);
-	return cachep;
-}
-
 static inline size_t slab_ksize(const struct kmem_cache *s)
 {
 #ifndef CONFIG_SLUB
diff --git a/mm/slub.c b/mm/slub.c
index efb08f2e9c66..f7a1d8537674 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -1524,6 +1524,10 @@ static bool freelist_corrupted(struct kmem_cache *s, struct page *page,
 {
 	return false;
 }
+
+static void print_tracking(struct kmem_cache *s, void *object)
+{
+}
 #endif /* CONFIG_SLUB_DEBUG */
 
 /*
@@ -3175,6 +3179,23 @@ void ___cache_free(struct kmem_cache *cache, void *x, unsigned long addr)
 }
 #endif
 
+static inline struct kmem_cache *cache_from_obj(struct kmem_cache *s, void *x)
+{
+	struct kmem_cache *cachep;
+
+	if (!IS_ENABLED(CONFIG_SLAB_FREELIST_HARDENED) &&
+	    !memcg_kmem_enabled() &&
+	    !kmem_cache_debug_flags(s, SLAB_CONSISTENCY_CHECKS))
+		return s;
+
+	cachep = virt_to_cache(x);
+	if (WARN(cachep && !slab_equal_or_root(cachep, s),
+		  "%s: Wrong slab cache. %s but object is from %s\n",
+		  __func__, s->name, cachep->name))
+		print_tracking(cachep, x);
+	return cachep;
+}
+
 void kmem_cache_free(struct kmem_cache *s, void *x)
 {
 	s = cache_from_obj(s, x);
-- 
2.26.2

Re: [PATCH 6/9] mm, slub: introduce static key for slub_debug()

[Roman Gushchin]

On Wed, Jun 10, 2020 at 06:31:32PM +0200, Vlastimil Babka wrote:
> One advantage of CONFIG_SLUB_DEBUG is that a generic distro kernel can be built
> with the option enabled, but it's inactive until simply enabled on boot,
> without rebuilding the kernel. With a static key, we can further eliminate the
> overhead of checking whether a cache has a particular debug flag enabled if we
> know that there are no such caches (slub_debug was not enabled during boot). We
> use the same mechanism also for e.g.  page_owner, debug_pagealloc or kmemcg
> functionality.
> 
> This patch introduces the static key and makes the general check for per-cache
> debug flags kmem_cache_debug() use it. This benefits several call sites,
> including (slow path but still rather frequent) __slab_free(). The next patches
> will add more uses.
> 
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>

Acked-by: Roman Gushchin <guro@fb.com>

Thanks!

> ---
>  mm/slub.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/slub.c b/mm/slub.c
> index 8dd2925448ae..24d3e5f832aa 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -114,13 +114,21 @@
>   * 			the fast path and disables lockless freelists.
>   */
>  
> +#ifdef CONFIG_SLUB_DEBUG
> +#ifdef CONFIG_SLUB_DEBUG_ON
> +DEFINE_STATIC_KEY_TRUE(slub_debug_enabled);
> +#else
> +DEFINE_STATIC_KEY_FALSE(slub_debug_enabled);
> +#endif
> +#endif
> +
>  static inline int kmem_cache_debug(struct kmem_cache *s)
>  {
>  #ifdef CONFIG_SLUB_DEBUG
> -	return unlikely(s->flags & SLAB_DEBUG_FLAGS);
> -#else
> -	return 0;
> +	if (static_branch_unlikely(&slub_debug_enabled))
> +		return s->flags & SLAB_DEBUG_FLAGS;
>  #endif
> +	return 0;
>  }
>  
>  void *fixup_red_left(struct kmem_cache *s, void *p)
> @@ -1389,6 +1397,8 @@ static int __init setup_slub_debug(char *str)
>  		slub_debug_string = saved_str;
>  	}
>  out:
> +	if (slub_debug != 0 || slub_debug_string)
> +		static_branch_enable(&slub_debug_enabled);
>  	if ((static_branch_unlikely(&init_on_alloc) ||
>  	     static_branch_unlikely(&init_on_free)) &&
>  	    (slub_debug & SLAB_POISON))
> -- 
> 2.26.2
> 

Re: [PATCH 7/9] mm, slub: introduce kmem_cache_debug_flags()

[Roman Gushchin]

On Wed, Jun 10, 2020 at 06:31:33PM +0200, Vlastimil Babka wrote:
> There are few places that call kmem_cache_debug(s) (which tests if any of debug
> flags are enabled for a cache) immediatelly followed by a test for a specific
> flag. The compiler can probably eliminate the extra check, but we can make the
> code nicer by introducing kmem_cache_debug_flags() that works like
> kmem_cache_debug() (including the static key check) but tests for specifig
> flag(s). The next patches will add more users.
> 
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> ---
>  mm/slub.c | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)

Acked-by: Roman Gushchin <guro@fb.com>

One small nit below.

> 
> diff --git a/mm/slub.c b/mm/slub.c
> index 24d3e5f832aa..c8e8b4ae2451 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -122,18 +122,28 @@ DEFINE_STATIC_KEY_FALSE(slub_debug_enabled);
>  #endif
>  #endif
>  
> -static inline int kmem_cache_debug(struct kmem_cache *s)
> +/*
> + * Returns true if any of the specified slub_debug flags is enabled for the
> + * cache. Use only for flags parsed by setup_slub_debug() as it also enables
> + * the static key.

Maybe adding VM_WARN_ON_ONCE(!(flags & SLAB_DEBUG_FLAGS)) to make sure the flags
argument is valid?

> + */
> +static inline int kmem_cache_debug_flags(struct kmem_cache *s, slab_flags_t flags)
>  {
>  #ifdef CONFIG_SLUB_DEBUG
>  	if (static_branch_unlikely(&slub_debug_enabled))
> -		return s->flags & SLAB_DEBUG_FLAGS;
> +		return s->flags & flags;
>  #endif
>  	return 0;
>  }
>  
> +static inline int kmem_cache_debug(struct kmem_cache *s)
> +{
> +	return kmem_cache_debug_flags(s, SLAB_DEBUG_FLAGS);
> +}
> +
>  void *fixup_red_left(struct kmem_cache *s, void *p)
>  {
> -	if (kmem_cache_debug(s) && s->flags & SLAB_RED_ZONE)
> +	if (kmem_cache_debug_flags(s, SLAB_RED_ZONE))
>  		p += s->red_left_pad;
>  
>  	return p;
> @@ -4076,7 +4086,7 @@ void __check_heap_object(const void *ptr, unsigned long n, struct page *page,
>  	offset = (ptr - page_address(page)) % s->size;
>  
>  	/* Adjust for redzone and reject if within the redzone. */
> -	if (kmem_cache_debug(s) && s->flags & SLAB_RED_ZONE) {
> +	if (kmem_cache_debug_flags(s, SLAB_RED_ZONE)) {
>  		if (offset < s->red_left_pad)
>  			usercopy_abort("SLUB object in left red zone",
>  				       s->name, to_user, offset, n);
> -- 
> 2.26.2
> 

Re: [PATCH 8/9] mm, slub: extend checks guarded by slub_debug static key

[Roman Gushchin]

On Wed, Jun 10, 2020 at 06:31:34PM +0200, Vlastimil Babka wrote:
> There are few more places in SLUB that could benefit from reduced overhead of
> the static key introduced by a previous patch:
> 
> - setup_object_debug() called on each object in newly allocated slab page
> - setup_page_debug() called on newly allocated slab page
> - __free_slab() called on freed slab page
> 
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>

Acked-by: Roman Gushchin <guro@fb.com>

Thanks!

> ---
>  mm/slub.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/slub.c b/mm/slub.c
> index c8e8b4ae2451..efb08f2e9c66 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -1130,7 +1130,7 @@ static inline void dec_slabs_node(struct kmem_cache *s, int node, int objects)
>  static void setup_object_debug(struct kmem_cache *s, struct page *page,
>  								void *object)
>  {
> -	if (!(s->flags & (SLAB_STORE_USER|SLAB_RED_ZONE|__OBJECT_POISON)))
> +	if (!kmem_cache_debug_flags(s, SLAB_STORE_USER|SLAB_RED_ZONE|__OBJECT_POISON))
>  		return;

This part is nice! It might bring some perf improvements.

>  
>  	init_object(s, object, SLUB_RED_INACTIVE);
> @@ -1140,7 +1140,7 @@ static void setup_object_debug(struct kmem_cache *s, struct page *page,
>  static
>  void setup_page_debug(struct kmem_cache *s, struct page *page, void *addr)
>  {
> -	if (!(s->flags & SLAB_POISON))
> +	if (!kmem_cache_debug_flags(s, SLAB_POISON))
>  		return;
>  
>  	metadata_access_enable();
> @@ -1857,7 +1857,7 @@ static void __free_slab(struct kmem_cache *s, struct page *page)
>  	int order = compound_order(page);
>  	int pages = 1 << order;
>  
> -	if (s->flags & SLAB_CONSISTENCY_CHECKS) {
> +	if (kmem_cache_debug_flags(s, SLAB_CONSISTENCY_CHECKS)) {
>  		void *p;
>  
>  		slab_pad_check(s, page);
> -- 
> 2.26.2
> 

Re: [PATCH 9/9] mm, slab/slub: move and improve cache_from_obj()

[Roman Gushchin]

On Wed, Jun 10, 2020 at 06:31:35PM +0200, Vlastimil Babka wrote:
> The function cache_from_obj() was added by commit b9ce5ef49f00 ("sl[au]b:
> always get the cache from its page in kmem_cache_free()") to support kmemcg,
> where per-memcg cache can be different from the root one, so we can't use
> the kmem_cache pointer given to kmem_cache_free().
> 
> Prior to that commit, SLUB already had debugging check+warning that could be
> enabled to compare the given kmem_cache pointer to one referenced by the slab
> page where the object-to-be-freed resides. This check was moved to
> cache_from_obj(). Later the check was also enabled for SLAB_FREELIST_HARDENED
> configs by commit 598a0717a816 ("mm/slab: validate cache membership under
> freelist hardening").
> 
> These checks and warnings can be useful especially for the debugging, which can
> be improved. Commit 598a0717a816 changed the pr_err() with WARN_ON_ONCE() to
> WARN_ONCE() so only the first hit is now reported, others are silent. This
> patch changes it to WARN() so that all errors are reported.
> 
> It's also useful to print SLUB allocation/free tracking info for the offending
> object, if tracking is enabled. We could export the SLUB print_tracking()
> function and provide an empty one for SLAB, or realize that both the debugging
> and hardening cases in cache_from_obj() are only supported by SLUB anyway. So
> this patch moves cache_from_obj() from slab.h to separate instances in slab.c
> and slub.c, where the SLAB version only does the kmemcg lookup and even could
> be completely removed once the kmemcg rework [1] is merged. The SLUB version
> can thus easily use the print_tracking() function. It can also use the
> kmem_cache_debug_flags() static key check for improved performance in kernels
> without the hardening and with debugging not enabled on boot.
> 
> [1] https://lore.kernel.org/r/20200608230654.828134-18-guro@fb.com
> 
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> ---
>  mm/slab.c |  8 ++++++++
>  mm/slab.h | 23 -----------------------
>  mm/slub.c | 21 +++++++++++++++++++++
>  3 files changed, 29 insertions(+), 23 deletions(-)
> 
> diff --git a/mm/slab.c b/mm/slab.c
> index 9350062ffc1a..6134c4c36d4c 100644
> --- a/mm/slab.c
> +++ b/mm/slab.c
> @@ -3672,6 +3672,14 @@ void *__kmalloc_track_caller(size_t size, gfp_t flags, unsigned long caller)
>  }
>  EXPORT_SYMBOL(__kmalloc_track_caller);
>  
> +static inline struct kmem_cache *cache_from_obj(struct kmem_cache *s, void *x)
> +{
> +	if (memcg_kmem_enabled())
> +		return virt_to_cache(x);
> +	else
> +		return s;
> +}

Hm, it looks like all the SLAB version doesn't perform any sanity checks anymore.
Is it intended?

Also, Is it ever possible that s != virt_to_cache(x) if there are no bugs?

kmem_cache_free_bulk() in slab.c does contain the following:
	if (!orig_s) /* called via kfree_bulk */
		s = virt_to_cache(objp);
	else
		s = cache_from_obj(orig_s, objp);
which looks a bit strange with the version above.

> +
>  /**
>   * kmem_cache_free - Deallocate an object
>   * @cachep: The cache the allocation was from.
> diff --git a/mm/slab.h b/mm/slab.h
> index 815e4e9a94cd..c0c4244f75da 100644
> --- a/mm/slab.h
> +++ b/mm/slab.h
> @@ -503,29 +503,6 @@ static __always_inline void uncharge_slab_page(struct page *page, int order,
>  	memcg_uncharge_slab(page, order, s);
>  }
>  
> -static inline struct kmem_cache *cache_from_obj(struct kmem_cache *s, void *x)
> -{
> -	struct kmem_cache *cachep;
> -
> -	/*
> -	 * When kmemcg is not being used, both assignments should return the
> -	 * same value. but we don't want to pay the assignment price in that
> -	 * case. If it is not compiled in, the compiler should be smart enough
> -	 * to not do even the assignment. In that case, slab_equal_or_root
> -	 * will also be a constant.
> -	 */
> -	if (!memcg_kmem_enabled() &&
> -	    !IS_ENABLED(CONFIG_SLAB_FREELIST_HARDENED) &&
> -	    !unlikely(s->flags & SLAB_CONSISTENCY_CHECKS))
> -		return s;
> -
> -	cachep = virt_to_cache(x);
> -	WARN_ONCE(cachep && !slab_equal_or_root(cachep, s),
> -		  "%s: Wrong slab cache. %s but object is from %s\n",
> -		  __func__, s->name, cachep->name);
> -	return cachep;
> -}
> -
>  static inline size_t slab_ksize(const struct kmem_cache *s)
>  {
>  #ifndef CONFIG_SLUB
> diff --git a/mm/slub.c b/mm/slub.c
> index efb08f2e9c66..f7a1d8537674 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -1524,6 +1524,10 @@ static bool freelist_corrupted(struct kmem_cache *s, struct page *page,
>  {
>  	return false;
>  }
> +
> +static void print_tracking(struct kmem_cache *s, void *object)
> +{
> +}
>  #endif /* CONFIG_SLUB_DEBUG */
>  
>  /*
> @@ -3175,6 +3179,23 @@ void ___cache_free(struct kmem_cache *cache, void *x, unsigned long addr)
>  }
>  #endif
>  
> +static inline struct kmem_cache *cache_from_obj(struct kmem_cache *s, void *x)
> +{
> +	struct kmem_cache *cachep;
> +
> +	if (!IS_ENABLED(CONFIG_SLAB_FREELIST_HARDENED) &&
> +	    !memcg_kmem_enabled() &&
> +	    !kmem_cache_debug_flags(s, SLAB_CONSISTENCY_CHECKS))
> +		return s;
> +
> +	cachep = virt_to_cache(x);
> +	if (WARN(cachep && !slab_equal_or_root(cachep, s),
> +		  "%s: Wrong slab cache. %s but object is from %s\n",
> +		  __func__, s->name, cachep->name))
> +		print_tracking(cachep, x);
> +	return cachep;
> +}

Maybe we can define a trivial SLAB version of kmem_cache_debug_flags()
and keep a single version of cache_from_obj()?

Thank you!

Re: [PATCH 9/9] mm, slab/slub: move and improve cache_from_obj()

[Vlastimil Babka]

On 6/11/20 12:46 AM, Roman Gushchin wrote:
> On Wed, Jun 10, 2020 at 06:31:35PM +0200, Vlastimil Babka wrote:
>> @@ -3672,6 +3672,14 @@ void *__kmalloc_track_caller(size_t size, gfp_t flags, unsigned long caller)
>>  }
>>  EXPORT_SYMBOL(__kmalloc_track_caller);
>>  
>> +static inline struct kmem_cache *cache_from_obj(struct kmem_cache *s, void *x)
>> +{
>> +	if (memcg_kmem_enabled())
>> +		return virt_to_cache(x);
>> +	else
>> +		return s;
>> +}
> 
> Hm, it looks like all the SLAB version doesn't perform any sanity checks anymore.
> Is it intended?

Yes, it was the same before commit b9ce5ef49f00. The commit could have been more
precise - kmemcg needs virt_to_cache(), but not the sanity check. The SLUB
version also shouldn't really be doing the sanity check if only
memcg_kmem_enabled() is true (and not the debugging/hardening), but the code
then looks ugly and I hope this will just fix itself with your kmemcg slab rework.

> Also, Is it ever possible that s != virt_to_cache(x) if there are no bugs?

Well, only in the kmemcg case it should be possible.

> kmem_cache_free_bulk() in slab.c does contain the following:
> 	if (!orig_s) /* called via kfree_bulk */
> 		s = virt_to_cache(objp);
> 	else
> 		s = cache_from_obj(orig_s, objp);
> which looks a bit strange with the version above.

Looks fine to me. If we are called with non-NULL s, and kmemcg is not enabled,
we can just trust s. If we are called with NULL s (via kfree_bulk()) we need to
get cache from the object, even if kmemcg is not enabled, so we do
virt_to_cache() unconditionally.
Once your series is fully accepted, we can remove SLAB's cache_from_obj() and
the whole 'else' part in the hunk above. Or am I missing something?


>> @@ -3175,6 +3179,23 @@ void ___cache_free(struct kmem_cache *cache, void *x, unsigned long addr)
>>  }
>>  #endif
>>  
>> +static inline struct kmem_cache *cache_from_obj(struct kmem_cache *s, void *x)
>> +{
>> +	struct kmem_cache *cachep;
>> +
>> +	if (!IS_ENABLED(CONFIG_SLAB_FREELIST_HARDENED) &&
>> +	    !memcg_kmem_enabled() &&
>> +	    !kmem_cache_debug_flags(s, SLAB_CONSISTENCY_CHECKS))
>> +		return s;
>> +
>> +	cachep = virt_to_cache(x);
>> +	if (WARN(cachep && !slab_equal_or_root(cachep, s),
>> +		  "%s: Wrong slab cache. %s but object is from %s\n",
>> +		  __func__, s->name, cachep->name))
>> +		print_tracking(cachep, x);
>> +	return cachep;
>> +}
> 
> Maybe we can define a trivial SLAB version of kmem_cache_debug_flags()
> and keep a single version of cache_from_obj()?

I think the result would be more obfuscated than just making it plain that SLAB
doesn't have those SLUB features. And I still hope SLAB's version will go away
completely. If your series is accepted first, then this patch based in that will
not introduce slab.c cache_from_obj() at all.

> Thank you!
> 

Re: [PATCH 9/9] mm, slab/slub: move and improve cache_from_obj()

[Roman Gushchin]

On Thu, Jun 11, 2020 at 11:56:53AM +0200, Vlastimil Babka wrote:
> On 6/11/20 12:46 AM, Roman Gushchin wrote:
> > On Wed, Jun 10, 2020 at 06:31:35PM +0200, Vlastimil Babka wrote:
> >> @@ -3672,6 +3672,14 @@ void *__kmalloc_track_caller(size_t size, gfp_t flags, unsigned long caller)
> >>  }
> >>  EXPORT_SYMBOL(__kmalloc_track_caller);
> >>  
> >> +static inline struct kmem_cache *cache_from_obj(struct kmem_cache *s, void *x)
> >> +{
> >> +	if (memcg_kmem_enabled())
> >> +		return virt_to_cache(x);
> >> +	else
> >> +		return s;
> >> +}
> > 
> > Hm, it looks like all the SLAB version doesn't perform any sanity checks anymore.
> > Is it intended?
> 
> Yes, it was the same before commit b9ce5ef49f00. The commit could have been more
> precise - kmemcg needs virt_to_cache(), but not the sanity check. The SLUB
> version also shouldn't really be doing the sanity check if only
> memcg_kmem_enabled() is true (and not the debugging/hardening), but the code
> then looks ugly and I hope this will just fix itself with your kmemcg slab rework.

Got it.

> 
> > Also, Is it ever possible that s != virt_to_cache(x) if there are no bugs?
> 
> Well, only in the kmemcg case it should be possible.
> 
> > kmem_cache_free_bulk() in slab.c does contain the following:
> > 	if (!orig_s) /* called via kfree_bulk */
> > 		s = virt_to_cache(objp);
> > 	else
> > 		s = cache_from_obj(orig_s, objp);
> > which looks a bit strange with the version above.
> 
> Looks fine to me. If we are called with non-NULL s, and kmemcg is not enabled,
> we can just trust s. If we are called with NULL s (via kfree_bulk()) we need to
> get cache from the object, even if kmemcg is not enabled, so we do
> virt_to_cache() unconditionally.
> Once your series is fully accepted, we can remove SLAB's cache_from_obj() and
> the whole 'else' part in the hunk above. Or am I missing something?

Right. I guess there will be even more cleanups possible, let's see where we'll end up.
It looks like nothing prevents it from being queued for 5.9 after 5.8-rc1 will be out,
right?

> 
> 
> >> @@ -3175,6 +3179,23 @@ void ___cache_free(struct kmem_cache *cache, void *x, unsigned long addr)
> >>  }
> >>  #endif
> >>  
> >> +static inline struct kmem_cache *cache_from_obj(struct kmem_cache *s, void *x)
> >> +{
> >> +	struct kmem_cache *cachep;
> >> +
> >> +	if (!IS_ENABLED(CONFIG_SLAB_FREELIST_HARDENED) &&
> >> +	    !memcg_kmem_enabled() &&
> >> +	    !kmem_cache_debug_flags(s, SLAB_CONSISTENCY_CHECKS))
> >> +		return s;
> >> +
> >> +	cachep = virt_to_cache(x);
> >> +	if (WARN(cachep && !slab_equal_or_root(cachep, s),
> >> +		  "%s: Wrong slab cache. %s but object is from %s\n",
> >> +		  __func__, s->name, cachep->name))
> >> +		print_tracking(cachep, x);
> >> +	return cachep;
> >> +}
> > 
> > Maybe we can define a trivial SLAB version of kmem_cache_debug_flags()
> > and keep a single version of cache_from_obj()?
> 
> I think the result would be more obfuscated than just making it plain that SLAB
> doesn't have those SLUB features. And I still hope SLAB's version will go away
> completely. If your series is accepted first, then this patch based in that will
> not introduce slab.c cache_from_obj() at all.

Ok, makes sense to me.

Thank you!

Re: [PATCH 9/9] mm, slab/slub: move and improve cache_from_obj()

[Kees Cook]

On Wed, Jun 10, 2020 at 06:31:35PM +0200, Vlastimil Babka wrote:
> The function cache_from_obj() was added by commit b9ce5ef49f00 ("sl[au]b:
> always get the cache from its page in kmem_cache_free()") to support kmemcg,
> where per-memcg cache can be different from the root one, so we can't use
> the kmem_cache pointer given to kmem_cache_free().
> 
> Prior to that commit, SLUB already had debugging check+warning that could be
> enabled to compare the given kmem_cache pointer to one referenced by the slab
> page where the object-to-be-freed resides. This check was moved to
> cache_from_obj(). Later the check was also enabled for SLAB_FREELIST_HARDENED
> configs by commit 598a0717a816 ("mm/slab: validate cache membership under
> freelist hardening").
> 
> These checks and warnings can be useful especially for the debugging, which can
> be improved. Commit 598a0717a816 changed the pr_err() with WARN_ON_ONCE() to
> WARN_ONCE() so only the first hit is now reported, others are silent. This
> patch changes it to WARN() so that all errors are reported.
> 
> It's also useful to print SLUB allocation/free tracking info for the offending
> object, if tracking is enabled. We could export the SLUB print_tracking()
> function and provide an empty one for SLAB, or realize that both the debugging
> and hardening cases in cache_from_obj() are only supported by SLUB anyway. So
> this patch moves cache_from_obj() from slab.h to separate instances in slab.c
> and slub.c, where the SLAB version only does the kmemcg lookup and even could

Oops. I made a mistake when I applied CONFIG_SLAB_FREELIST_HARDENED
here, I was thinking of SLAB_FREELIST_RANDOM's coverage (SLUB and SLAB),
and I see now that I never updated CONFIG_SLAB_FREELIST_HARDENED to
cover SLAB and SLOB.

The point being: I still want the sanity check for the SLAB case under
hardening. This needs to stay a common function. The whole point is
to catch corruption from the wrong kmem_cache * being associated with
an object, and that's agnostic of slab/slub/slob.

So, I'll send a follow-up to this patch to actually do what I had
originally intended for 598a0717a816 ("mm/slab: validate cache membership
under freelist hardening"), which wasn't intended to be SLUB-specific.

-- 
Kees Cook

Re: [PATCH 6/9] mm, slub: introduce static key for slub_debug()

[Kees Cook]

On Wed, Jun 10, 2020 at 06:31:32PM +0200, Vlastimil Babka wrote:
> One advantage of CONFIG_SLUB_DEBUG is that a generic distro kernel can be built
> with the option enabled, but it's inactive until simply enabled on boot,
> without rebuilding the kernel. With a static key, we can further eliminate the
> overhead of checking whether a cache has a particular debug flag enabled if we
> know that there are no such caches (slub_debug was not enabled during boot). We
> use the same mechanism also for e.g.  page_owner, debug_pagealloc or kmemcg
> functionality.
> 
> This patch introduces the static key and makes the general check for per-cache
> debug flags kmem_cache_debug() use it. This benefits several call sites,
> including (slow path but still rather frequent) __slab_free(). The next patches
> will add more uses.
> 
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> ---
>  mm/slub.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/slub.c b/mm/slub.c
> index 8dd2925448ae..24d3e5f832aa 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -114,13 +114,21 @@
>   * 			the fast path and disables lockless freelists.
>   */
>  
> +#ifdef CONFIG_SLUB_DEBUG
> +#ifdef CONFIG_SLUB_DEBUG_ON
> +DEFINE_STATIC_KEY_TRUE(slub_debug_enabled);
> +#else
> +DEFINE_STATIC_KEY_FALSE(slub_debug_enabled);
> +#endif
> +#endif

Yet another potential future user of DEFINE_STATIC_KEY_MAYBE()[1], once
I get it landed. :)

Reviewed-by: Kees Cook <keescook@chromium.org>

[1] https://lore.kernel.org/lkml/20200324203231.64324-2-keescook@chromium.org/

-- 
Kees Cook

Re: [PATCH 7/9] mm, slub: introduce kmem_cache_debug_flags()

[Kees Cook]

On Wed, Jun 10, 2020 at 06:31:33PM +0200, Vlastimil Babka wrote:
> There are few places that call kmem_cache_debug(s) (which tests if any of debug
> flags are enabled for a cache) immediatelly followed by a test for a specific
> flag. The compiler can probably eliminate the extra check, but we can make the
> code nicer by introducing kmem_cache_debug_flags() that works like
> kmem_cache_debug() (including the static key check) but tests for specifig
> flag(s). The next patches will add more users.
> 
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> ---
>  mm/slub.c | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/mm/slub.c b/mm/slub.c
> index 24d3e5f832aa..c8e8b4ae2451 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -122,18 +122,28 @@ DEFINE_STATIC_KEY_FALSE(slub_debug_enabled);
>  #endif
>  #endif
>  
> -static inline int kmem_cache_debug(struct kmem_cache *s)
> +/*
> + * Returns true if any of the specified slub_debug flags is enabled for the
> + * cache. Use only for flags parsed by setup_slub_debug() as it also enables
> + * the static key.
> + */
> +static inline int kmem_cache_debug_flags(struct kmem_cache *s, slab_flags_t flags)

This should return slab_flag_t, yes?

>  {
>  #ifdef CONFIG_SLUB_DEBUG
>  	if (static_branch_unlikely(&slub_debug_enabled))
> -		return s->flags & SLAB_DEBUG_FLAGS;
> +		return s->flags & flags;
>  #endif
>  	return 0;
>  }
>  
> +static inline int kmem_cache_debug(struct kmem_cache *s)

And this too, as long as we're making changes here.

-- 
Kees Cook

Re: [PATCH 7/9] mm, slub: introduce kmem_cache_debug_flags()

[Vlastimil Babka]

On 6/17/20 7:56 PM, Kees Cook wrote:
> On Wed, Jun 10, 2020 at 06:31:33PM +0200, Vlastimil Babka wrote:
>> There are few places that call kmem_cache_debug(s) (which tests if any of debug
>> flags are enabled for a cache) immediatelly followed by a test for a specific
>> flag. The compiler can probably eliminate the extra check, but we can make the
>> code nicer by introducing kmem_cache_debug_flags() that works like
>> kmem_cache_debug() (including the static key check) but tests for specifig
>> flag(s). The next patches will add more users.
>> 
>> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
>> ---
>>  mm/slub.c | 18 ++++++++++++++----
>>  1 file changed, 14 insertions(+), 4 deletions(-)
>> 
>> diff --git a/mm/slub.c b/mm/slub.c
>> index 24d3e5f832aa..c8e8b4ae2451 100644
>> --- a/mm/slub.c
>> +++ b/mm/slub.c
>> @@ -122,18 +122,28 @@ DEFINE_STATIC_KEY_FALSE(slub_debug_enabled);
>>  #endif
>>  #endif
>>  
>> -static inline int kmem_cache_debug(struct kmem_cache *s)
>> +/*
>> + * Returns true if any of the specified slub_debug flags is enabled for the
>> + * cache. Use only for flags parsed by setup_slub_debug() as it also enables
>> + * the static key.
>> + */
>> +static inline int kmem_cache_debug_flags(struct kmem_cache *s, slab_flags_t flags)
> 
> This should return slab_flag_t, yes?

bool, actually

>>  {
>>  #ifdef CONFIG_SLUB_DEBUG
>>  	if (static_branch_unlikely(&slub_debug_enabled))
>> -		return s->flags & SLAB_DEBUG_FLAGS;
>> +		return s->flags & flags;
>>  #endif
>>  	return 0;
>>  }
>>  
>> +static inline int kmem_cache_debug(struct kmem_cache *s)
> 
> And this too, as long as we're making changes here.

OK

Re: [PATCH 7/9] mm, slub: introduce kmem_cache_debug_flags()

[Vlastimil Babka]

On 6/10/20 6:31 PM, Vlastimil Babka wrote:
> There are few places that call kmem_cache_debug(s) (which tests if any of debug
> flags are enabled for a cache) immediatelly followed by a test for a specific
> flag. The compiler can probably eliminate the extra check, but we can make the
> code nicer by introducing kmem_cache_debug_flags() that works like
> kmem_cache_debug() (including the static key check) but tests for specifig
> flag(s). The next patches will add more users.
> 
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>

Please add this fixup per reviews.
----8<----
From 25793252a31a36f8d1d4ccf0f27ed3e43fba54d8 Mon Sep 17 00:00:00 2001
From: Vlastimil Babka <vbabka@suse.cz>
Date: Thu, 18 Jun 2020 10:34:53 +0200
Subject: [PATCH] mm, slub: introduce kmem_cache_debug_flags()-fix

Change return from int to bool, per Kees.
Add VM_WARN_ON_ONCE() for invalid flags, per Roman.

Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
---
 mm/slub.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/mm/slub.c b/mm/slub.c
index c8e8b4ae2451..59d19c64069e 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -127,16 +127,17 @@ DEFINE_STATIC_KEY_FALSE(slub_debug_enabled);
  * cache. Use only for flags parsed by setup_slub_debug() as it also enables
  * the static key.
  */
-static inline int kmem_cache_debug_flags(struct kmem_cache *s, slab_flags_t flags)
+static inline bool kmem_cache_debug_flags(struct kmem_cache *s, slab_flags_t flags)
 {
+	VM_WARN_ON_ONCE(!(flags & SLAB_DEBUG_FLAGS));
 #ifdef CONFIG_SLUB_DEBUG
 	if (static_branch_unlikely(&slub_debug_enabled))
 		return s->flags & flags;
 #endif
-	return 0;
+	return false;
 }
 
-static inline int kmem_cache_debug(struct kmem_cache *s)
+static inline bool kmem_cache_debug(struct kmem_cache *s)
 {
 	return kmem_cache_debug_flags(s, SLAB_DEBUG_FLAGS);
 }
-- 
2.27.0

Re: [PATCH 9/9] mm, slab/slub: move and improve cache_from_obj()

[Vlastimil Babka]

On 6/17/20 7:49 PM, Kees Cook wrote:
> On Wed, Jun 10, 2020 at 06:31:35PM +0200, Vlastimil Babka wrote:
>> The function cache_from_obj() was added by commit b9ce5ef49f00 ("sl[au]b:
>> always get the cache from its page in kmem_cache_free()") to support kmemcg,
>> where per-memcg cache can be different from the root one, so we can't use
>> the kmem_cache pointer given to kmem_cache_free().
>> 
>> Prior to that commit, SLUB already had debugging check+warning that could be
>> enabled to compare the given kmem_cache pointer to one referenced by the slab
>> page where the object-to-be-freed resides. This check was moved to
>> cache_from_obj(). Later the check was also enabled for SLAB_FREELIST_HARDENED
>> configs by commit 598a0717a816 ("mm/slab: validate cache membership under
>> freelist hardening").
>> 
>> These checks and warnings can be useful especially for the debugging, which can
>> be improved. Commit 598a0717a816 changed the pr_err() with WARN_ON_ONCE() to
>> WARN_ONCE() so only the first hit is now reported, others are silent. This
>> patch changes it to WARN() so that all errors are reported.
>> 
>> It's also useful to print SLUB allocation/free tracking info for the offending
>> object, if tracking is enabled. We could export the SLUB print_tracking()
>> function and provide an empty one for SLAB, or realize that both the debugging
>> and hardening cases in cache_from_obj() are only supported by SLUB anyway. So
>> this patch moves cache_from_obj() from slab.h to separate instances in slab.c
>> and slub.c, where the SLAB version only does the kmemcg lookup and even could
> 
> Oops. I made a mistake when I applied CONFIG_SLAB_FREELIST_HARDENED
> here, I was thinking of SLAB_FREELIST_RANDOM's coverage (SLUB and SLAB),
> and I see now that I never updated CONFIG_SLAB_FREELIST_HARDENED to
> cover SLAB and SLOB.
> 
> The point being: I still want the sanity check for the SLAB case under
> hardening. This needs to stay a common function. The whole point is
> to catch corruption from the wrong kmem_cache * being associated with
> an object, and that's agnostic of slab/slub/slob.
> 
> So, I'll send a follow-up to this patch to actually do what I had
> originally intended for 598a0717a816 ("mm/slab: validate cache membership
> under freelist hardening"), which wasn't intended to be SLUB-specific.

To prvent the churn of your patch moving the cache_from_obj() back to slab.h, I
think it's best if we modify my patch. The patch below should be squashed into
the current version in mmots, with the commit log used for the whole result.

This will cause conflicts while reapplying Roman's
mm-memcg-slab-use-a-single-set-of-kmem_caches-for-all-allocations.patch which
can be fixed by
a) throwing away the conflicting hunks for cache_from_obj() in slab.c and slub.c
b) applying this hunk instead:

--- a/mm/slab.h
+++ b/mm/slab.h
@@ -455,12 +455,11 @@ static inline struct kmem_cache *cache_from_obj(struct kmem_cache *s, void *x)
 	struct kmem_cache *cachep;
 
 	if (!IS_ENABLED(CONFIG_SLAB_FREELIST_HARDENED) &&
-	    !memcg_kmem_enabled() &&
 	    !kmem_cache_debug_flags(s, SLAB_CONSISTENCY_CHECKS))
 		return s;
 
 	cachep = virt_to_cache(x);
-	if (WARN(cachep && !slab_equal_or_root(cachep, s),
+	if (WARN(cachep && cachep != s,
 		  "%s: Wrong slab cache. %s but object is from %s\n",
 		  __func__, s->name, cachep->name))
 		print_tracking(cachep, x);

The fixup patch itself:
----8<----
From b8df607d92b37e5329ce7bda62b2b364cc249893 Mon Sep 17 00:00:00 2001
From: Vlastimil Babka <vbabka@suse.cz>
Date: Thu, 18 Jun 2020 11:52:03 +0200
Subject: [PATCH] mm, slab/slub: improve error reporting and overhead of
 cache_from_obj()

The function cache_from_obj() was added by commit b9ce5ef49f00 ("sl[au]b:
always get the cache from its page in kmem_cache_free()") to support
kmemcg, where per-memcg cache can be different from the root one, so we
can't use the kmem_cache pointer given to kmem_cache_free().

Prior to that commit, SLUB already had debugging check+warning that could
be enabled to compare the given kmem_cache pointer to one referenced by
the slab page where the object-to-be-freed resides.  This check was moved
to cache_from_obj().  Later the check was also enabled for
SLAB_FREELIST_HARDENED configs by commit 598a0717a816 ("mm/slab: validate
cache membership under freelist hardening").

These checks and warnings can be useful especially for the debugging,
which can be improved.  Commit 598a0717a816 changed the pr_err() with
WARN_ON_ONCE() to WARN_ONCE() so only the first hit is now reported,
others are silent.  This patch changes it to WARN() so that all errors are
reported.

It's also useful to print SLUB allocation/free tracking info for the offending
object, if tracking is enabled. Thus, export the SLUB print_tracking() function
and provide an empty one for SLAB.

For SLUB we can also benefit from the static key check in
kmem_cache_debug_flags(), but we need to move this function to slab.h and
declare the static key there.

[1] https://lore.kernel.org/r/20200608230654.828134-18-guro@fb.com

Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
---
 mm/slab.c |  8 --------
 mm/slab.h | 45 +++++++++++++++++++++++++++++++++++++++++++++
 mm/slub.c | 38 +-------------------------------------
 3 files changed, 46 insertions(+), 45 deletions(-)

diff --git a/mm/slab.c b/mm/slab.c
index 6134c4c36d4c..9350062ffc1a 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -3672,14 +3672,6 @@ void *__kmalloc_track_caller(size_t size, gfp_t flags, unsigned long caller)
 }
 EXPORT_SYMBOL(__kmalloc_track_caller);
 
-static inline struct kmem_cache *cache_from_obj(struct kmem_cache *s, void *x)
-{
-	if (memcg_kmem_enabled())
-		return virt_to_cache(x);
-	else
-		return s;
-}
-
 /**
  * kmem_cache_free - Deallocate an object
  * @cachep: The cache the allocation was from.
diff --git a/mm/slab.h b/mm/slab.h
index a2696d306b62..a9f5ba9ce9a7 100644
--- a/mm/slab.h
+++ b/mm/slab.h
@@ -275,6 +275,34 @@ static inline int cache_vmstat_idx(struct kmem_cache *s)
 		NR_SLAB_RECLAIMABLE : NR_SLAB_UNRECLAIMABLE;
 }
 
+#ifdef CONFIG_SLUB_DEBUG
+#ifdef CONFIG_SLUB_DEBUG_ON
+DECLARE_STATIC_KEY_TRUE(slub_debug_enabled);
+#else
+DECLARE_STATIC_KEY_FALSE(slub_debug_enabled);
+#endif
+extern void print_tracking(struct kmem_cache *s, void *object);
+#else
+static inline void print_tracking(struct kmem_cache *s, void *object)
+{
+}
+#endif
+
+/*
+ * Returns true if any of the specified slub_debug flags is enabled for the
+ * cache. Use only for flags parsed by setup_slub_debug() as it also enables
+ * the static key.
+ */
+static inline bool kmem_cache_debug_flags(struct kmem_cache *s, slab_flags_t flags)
+{
+	VM_WARN_ON_ONCE(!(flags & SLAB_DEBUG_FLAGS));
+#ifdef CONFIG_SLUB_DEBUG
+	if (static_branch_unlikely(&slub_debug_enabled))
+		return s->flags & flags;
+#endif
+	return false;
+}
+
 #ifdef CONFIG_MEMCG_KMEM
 
 /* List of all root caches. */
@@ -503,6 +531,23 @@ static __always_inline void uncharge_slab_page(struct page *page, int order,
 	memcg_uncharge_slab(page, order, s);
 }
 
+static inline struct kmem_cache *cache_from_obj(struct kmem_cache *s, void *x)
+{
+	struct kmem_cache *cachep;
+
+	if (!IS_ENABLED(CONFIG_SLAB_FREELIST_HARDENED) &&
+	    !memcg_kmem_enabled() &&
+	    !kmem_cache_debug_flags(s, SLAB_CONSISTENCY_CHECKS))
+		return s;
+
+	cachep = virt_to_cache(x);
+	if (WARN(cachep && !slab_equal_or_root(cachep, s),
+		  "%s: Wrong slab cache. %s but object is from %s\n",
+		  __func__, s->name, cachep->name))
+		print_tracking(cachep, x);
+	return cachep;
+}
+
 static inline size_t slab_ksize(const struct kmem_cache *s)
 {
 #ifndef CONFIG_SLUB
diff --git a/mm/slub.c b/mm/slub.c
index 202fb423d195..0e635a8aa340 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -122,21 +122,6 @@ DEFINE_STATIC_KEY_FALSE(slub_debug_enabled);
 #endif
 #endif
 
-/*
- * Returns true if any of the specified slub_debug flags is enabled for the
- * cache. Use only for flags parsed by setup_slub_debug() as it also enables
- * the static key.
- */
-static inline bool kmem_cache_debug_flags(struct kmem_cache *s, slab_flags_t flags)
-{
-	VM_WARN_ON_ONCE(!(flags & SLAB_DEBUG_FLAGS));
-#ifdef CONFIG_SLUB_DEBUG
-	if (static_branch_unlikely(&slub_debug_enabled))
-		return s->flags & flags;
-#endif
-	return false;
-}
-
 static inline bool kmem_cache_debug(struct kmem_cache *s)
 {
 	return kmem_cache_debug_flags(s, SLAB_DEBUG_FLAGS);
@@ -653,7 +638,7 @@ static void print_track(const char *s, struct track *t, unsigned long pr_time)
 #endif
 }
 
-static void print_tracking(struct kmem_cache *s, void *object)
+void print_tracking(struct kmem_cache *s, void *object)
 {
 	unsigned long pr_time = jiffies;
 	if (!(s->flags & SLAB_STORE_USER))
@@ -1525,10 +1510,6 @@ static bool freelist_corrupted(struct kmem_cache *s, struct page *page,
 {
 	return false;
 }
-
-static void print_tracking(struct kmem_cache *s, void *object)
-{
-}
 #endif /* CONFIG_SLUB_DEBUG */
 
 /*
@@ -3180,23 +3161,6 @@ void ___cache_free(struct kmem_cache *cache, void *x, unsigned long addr)
 }
 #endif
 
-static inline struct kmem_cache *cache_from_obj(struct kmem_cache *s, void *x)
-{
-	struct kmem_cache *cachep;
-
-	if (!IS_ENABLED(CONFIG_SLAB_FREELIST_HARDENED) &&
-	    !memcg_kmem_enabled() &&
-	    !kmem_cache_debug_flags(s, SLAB_CONSISTENCY_CHECKS))
-		return s;
-
-	cachep = virt_to_cache(x);
-	if (WARN(cachep && !slab_equal_or_root(cachep, s),
-		  "%s: Wrong slab cache. %s but object is from %s\n",
-		  __func__, s->name, cachep->name))
-		print_tracking(cachep, x);
-	return cachep;
-}
-
 void kmem_cache_free(struct kmem_cache *s, void *x)
 {
 	s = cache_from_obj(s, x);
-- 
2.27.0

Re: [PATCH 7/9] mm, slub: introduce kmem_cache_debug_flags()

[Roman Gushchin]

On Thu, Jun 18, 2020 at 10:37:07AM +0200, Vlastimil Babka wrote:
> On 6/10/20 6:31 PM, Vlastimil Babka wrote:
> > There are few places that call kmem_cache_debug(s) (which tests if any of debug
> > flags are enabled for a cache) immediatelly followed by a test for a specific
> > flag. The compiler can probably eliminate the extra check, but we can make the
> > code nicer by introducing kmem_cache_debug_flags() that works like
> > kmem_cache_debug() (including the static key check) but tests for specifig
> > flag(s). The next patches will add more users.
> > 
> > Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> 
> Please add this fixup per reviews.
> ----8<----
> From 25793252a31a36f8d1d4ccf0f27ed3e43fba54d8 Mon Sep 17 00:00:00 2001
> From: Vlastimil Babka <vbabka@suse.cz>
> Date: Thu, 18 Jun 2020 10:34:53 +0200
> Subject: [PATCH] mm, slub: introduce kmem_cache_debug_flags()-fix
> 
> Change return from int to bool, per Kees.
> Add VM_WARN_ON_ONCE() for invalid flags, per Roman.
> 
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>

Acked-by: Roman Gushchin <guro@fb.com>

Thanks!

> ---
>  mm/slub.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/slub.c b/mm/slub.c
> index c8e8b4ae2451..59d19c64069e 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -127,16 +127,17 @@ DEFINE_STATIC_KEY_FALSE(slub_debug_enabled);
>   * cache. Use only for flags parsed by setup_slub_debug() as it also enables
>   * the static key.
>   */
> -static inline int kmem_cache_debug_flags(struct kmem_cache *s, slab_flags_t flags)
> +static inline bool kmem_cache_debug_flags(struct kmem_cache *s, slab_flags_t flags)
>  {
> +	VM_WARN_ON_ONCE(!(flags & SLAB_DEBUG_FLAGS));
>  #ifdef CONFIG_SLUB_DEBUG
>  	if (static_branch_unlikely(&slub_debug_enabled))
>  		return s->flags & flags;
>  #endif
> -	return 0;
> +	return false;
>  }
>  
> -static inline int kmem_cache_debug(struct kmem_cache *s)
> +static inline bool kmem_cache_debug(struct kmem_cache *s)
>  {
>  	return kmem_cache_debug_flags(s, SLAB_DEBUG_FLAGS);
>  }
> -- 
> 2.27.0
> 
> 

Re: [PATCH 7/9] mm, slub: introduce kmem_cache_debug_flags()

[Kees Cook]

On Thu, Jun 18, 2020 at 10:37:07AM +0200, Vlastimil Babka wrote:
> On 6/10/20 6:31 PM, Vlastimil Babka wrote:
> > There are few places that call kmem_cache_debug(s) (which tests if any of debug
> > flags are enabled for a cache) immediatelly followed by a test for a specific
> > flag. The compiler can probably eliminate the extra check, but we can make the
> > code nicer by introducing kmem_cache_debug_flags() that works like
> > kmem_cache_debug() (including the static key check) but tests for specifig
> > flag(s). The next patches will add more users.
> > 
> > Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> 
> Please add this fixup per reviews.
> ----8<----
> From 25793252a31a36f8d1d4ccf0f27ed3e43fba54d8 Mon Sep 17 00:00:00 2001
> From: Vlastimil Babka <vbabka@suse.cz>
> Date: Thu, 18 Jun 2020 10:34:53 +0200
> Subject: [PATCH] mm, slub: introduce kmem_cache_debug_flags()-fix
> 
> Change return from int to bool, per Kees.
> Add VM_WARN_ON_ONCE() for invalid flags, per Roman.
> 
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>

Acked-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

Re: [PATCH 9/9] mm, slab/slub: move and improve cache_from_obj()

[Kees Cook]

On Thu, Jun 18, 2020 at 12:10:38PM +0200, Vlastimil Babka wrote:
> 
> On 6/17/20 7:49 PM, Kees Cook wrote:
> > On Wed, Jun 10, 2020 at 06:31:35PM +0200, Vlastimil Babka wrote:
> >> The function cache_from_obj() was added by commit b9ce5ef49f00 ("sl[au]b:
> >> always get the cache from its page in kmem_cache_free()") to support kmemcg,
> >> where per-memcg cache can be different from the root one, so we can't use
> >> the kmem_cache pointer given to kmem_cache_free().
> >> 
> >> Prior to that commit, SLUB already had debugging check+warning that could be
> >> enabled to compare the given kmem_cache pointer to one referenced by the slab
> >> page where the object-to-be-freed resides. This check was moved to
> >> cache_from_obj(). Later the check was also enabled for SLAB_FREELIST_HARDENED
> >> configs by commit 598a0717a816 ("mm/slab: validate cache membership under
> >> freelist hardening").
> >> 
> >> These checks and warnings can be useful especially for the debugging, which can
> >> be improved. Commit 598a0717a816 changed the pr_err() with WARN_ON_ONCE() to
> >> WARN_ONCE() so only the first hit is now reported, others are silent. This
> >> patch changes it to WARN() so that all errors are reported.
> >> 
> >> It's also useful to print SLUB allocation/free tracking info for the offending
> >> object, if tracking is enabled. We could export the SLUB print_tracking()
> >> function and provide an empty one for SLAB, or realize that both the debugging
> >> and hardening cases in cache_from_obj() are only supported by SLUB anyway. So
> >> this patch moves cache_from_obj() from slab.h to separate instances in slab.c
> >> and slub.c, where the SLAB version only does the kmemcg lookup and even could
> > 
> > Oops. I made a mistake when I applied CONFIG_SLAB_FREELIST_HARDENED
> > here, I was thinking of SLAB_FREELIST_RANDOM's coverage (SLUB and SLAB),
> > and I see now that I never updated CONFIG_SLAB_FREELIST_HARDENED to
> > cover SLAB and SLOB.
> > 
> > The point being: I still want the sanity check for the SLAB case under
> > hardening. This needs to stay a common function. The whole point is
> > to catch corruption from the wrong kmem_cache * being associated with
> > an object, and that's agnostic of slab/slub/slob.
> > 
> > So, I'll send a follow-up to this patch to actually do what I had
> > originally intended for 598a0717a816 ("mm/slab: validate cache membership
> > under freelist hardening"), which wasn't intended to be SLUB-specific.
> 
> To prvent the churn of your patch moving the cache_from_obj() back to slab.h, I
> think it's best if we modify my patch. The patch below should be squashed into
> the current version in mmots, with the commit log used for the whole result.
> 
> This will cause conflicts while reapplying Roman's
> mm-memcg-slab-use-a-single-set-of-kmem_caches-for-all-allocations.patch which
> can be fixed by
> a) throwing away the conflicting hunks for cache_from_obj() in slab.c and slub.c
> b) applying this hunk instead:
> 
> --- a/mm/slab.h
> +++ b/mm/slab.h
> @@ -455,12 +455,11 @@ static inline struct kmem_cache *cache_from_obj(struct kmem_cache *s, void *x)
>  	struct kmem_cache *cachep;
>  
>  	if (!IS_ENABLED(CONFIG_SLAB_FREELIST_HARDENED) &&
> -	    !memcg_kmem_enabled() &&
>  	    !kmem_cache_debug_flags(s, SLAB_CONSISTENCY_CHECKS))
>  		return s;
>  
>  	cachep = virt_to_cache(x);
> -	if (WARN(cachep && !slab_equal_or_root(cachep, s),
> +	if (WARN(cachep && cachep != s,
>  		  "%s: Wrong slab cache. %s but object is from %s\n",
>  		  __func__, s->name, cachep->name))
>  		print_tracking(cachep, x);
> 
> The fixup patch itself:
> ----8<----
> From b8df607d92b37e5329ce7bda62b2b364cc249893 Mon Sep 17 00:00:00 2001
> From: Vlastimil Babka <vbabka@suse.cz>
> Date: Thu, 18 Jun 2020 11:52:03 +0200
> Subject: [PATCH] mm, slab/slub: improve error reporting and overhead of
>  cache_from_obj()
> 
> The function cache_from_obj() was added by commit b9ce5ef49f00 ("sl[au]b:
> always get the cache from its page in kmem_cache_free()") to support
> kmemcg, where per-memcg cache can be different from the root one, so we
> can't use the kmem_cache pointer given to kmem_cache_free().
> 
> Prior to that commit, SLUB already had debugging check+warning that could
> be enabled to compare the given kmem_cache pointer to one referenced by
> the slab page where the object-to-be-freed resides.  This check was moved
> to cache_from_obj().  Later the check was also enabled for
> SLAB_FREELIST_HARDENED configs by commit 598a0717a816 ("mm/slab: validate
> cache membership under freelist hardening").
> 
> These checks and warnings can be useful especially for the debugging,
> which can be improved.  Commit 598a0717a816 changed the pr_err() with
> WARN_ON_ONCE() to WARN_ONCE() so only the first hit is now reported,
> others are silent.  This patch changes it to WARN() so that all errors are
> reported.
> 
> It's also useful to print SLUB allocation/free tracking info for the offending
> object, if tracking is enabled. Thus, export the SLUB print_tracking() function
> and provide an empty one for SLAB.
> 
> For SLUB we can also benefit from the static key check in
> kmem_cache_debug_flags(), but we need to move this function to slab.h and
> declare the static key there.
> 
> [1] https://lore.kernel.org/r/20200608230654.828134-18-guro@fb.com
> 
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>

Acked-by: Kees Cook <keescook@chromium.org>

I will rebase my fix for SLAB_FREELIST_HARDENED coverage on this.

Thanks!

-- 
Kees Cook

Re: [PATCH 9/9] mm, slab/slub: move and improve cache_from_obj()

[Roman Gushchin]

On Thu, Jun 18, 2020 at 12:10:38PM +0200, Vlastimil Babka wrote:
> 
> On 6/17/20 7:49 PM, Kees Cook wrote:
> > On Wed, Jun 10, 2020 at 06:31:35PM +0200, Vlastimil Babka wrote:
> >> The function cache_from_obj() was added by commit b9ce5ef49f00 ("sl[au]b:
> >> always get the cache from its page in kmem_cache_free()") to support kmemcg,
> >> where per-memcg cache can be different from the root one, so we can't use
> >> the kmem_cache pointer given to kmem_cache_free().
> >> 
> >> Prior to that commit, SLUB already had debugging check+warning that could be
> >> enabled to compare the given kmem_cache pointer to one referenced by the slab
> >> page where the object-to-be-freed resides. This check was moved to
> >> cache_from_obj(). Later the check was also enabled for SLAB_FREELIST_HARDENED
> >> configs by commit 598a0717a816 ("mm/slab: validate cache membership under
> >> freelist hardening").
> >> 
> >> These checks and warnings can be useful especially for the debugging, which can
> >> be improved. Commit 598a0717a816 changed the pr_err() with WARN_ON_ONCE() to
> >> WARN_ONCE() so only the first hit is now reported, others are silent. This
> >> patch changes it to WARN() so that all errors are reported.
> >> 
> >> It's also useful to print SLUB allocation/free tracking info for the offending
> >> object, if tracking is enabled. We could export the SLUB print_tracking()
> >> function and provide an empty one for SLAB, or realize that both the debugging
> >> and hardening cases in cache_from_obj() are only supported by SLUB anyway. So
> >> this patch moves cache_from_obj() from slab.h to separate instances in slab.c
> >> and slub.c, where the SLAB version only does the kmemcg lookup and even could
> > 
> > Oops. I made a mistake when I applied CONFIG_SLAB_FREELIST_HARDENED
> > here, I was thinking of SLAB_FREELIST_RANDOM's coverage (SLUB and SLAB),
> > and I see now that I never updated CONFIG_SLAB_FREELIST_HARDENED to
> > cover SLAB and SLOB.
> > 
> > The point being: I still want the sanity check for the SLAB case under
> > hardening. This needs to stay a common function. The whole point is
> > to catch corruption from the wrong kmem_cache * being associated with
> > an object, and that's agnostic of slab/slub/slob.
> > 
> > So, I'll send a follow-up to this patch to actually do what I had
> > originally intended for 598a0717a816 ("mm/slab: validate cache membership
> > under freelist hardening"), which wasn't intended to be SLUB-specific.
> 
> To prvent the churn of your patch moving the cache_from_obj() back to slab.h, I
> think it's best if we modify my patch. The patch below should be squashed into
> the current version in mmots, with the commit log used for the whole result.
> 
> This will cause conflicts while reapplying Roman's
> mm-memcg-slab-use-a-single-set-of-kmem_caches-for-all-allocations.patch which
> can be fixed by
> a) throwing away the conflicting hunks for cache_from_obj() in slab.c and slub.c
> b) applying this hunk instead:
> 
> --- a/mm/slab.h
> +++ b/mm/slab.h
> @@ -455,12 +455,11 @@ static inline struct kmem_cache *cache_from_obj(struct kmem_cache *s, void *x)
>  	struct kmem_cache *cachep;
>  
>  	if (!IS_ENABLED(CONFIG_SLAB_FREELIST_HARDENED) &&
> -	    !memcg_kmem_enabled() &&
>  	    !kmem_cache_debug_flags(s, SLAB_CONSISTENCY_CHECKS))
>  		return s;
>  
>  	cachep = virt_to_cache(x);
> -	if (WARN(cachep && !slab_equal_or_root(cachep, s),
> +	if (WARN(cachep && cachep != s,
>  		  "%s: Wrong slab cache. %s but object is from %s\n",
>  		  __func__, s->name, cachep->name))
>  		print_tracking(cachep, x);
> 
> The fixup patch itself:
> ----8<----
> From b8df607d92b37e5329ce7bda62b2b364cc249893 Mon Sep 17 00:00:00 2001
> From: Vlastimil Babka <vbabka@suse.cz>
> Date: Thu, 18 Jun 2020 11:52:03 +0200
> Subject: [PATCH] mm, slab/slub: improve error reporting and overhead of
>  cache_from_obj()
> 
> The function cache_from_obj() was added by commit b9ce5ef49f00 ("sl[au]b:
> always get the cache from its page in kmem_cache_free()") to support
> kmemcg, where per-memcg cache can be different from the root one, so we
> can't use the kmem_cache pointer given to kmem_cache_free().
> 
> Prior to that commit, SLUB already had debugging check+warning that could
> be enabled to compare the given kmem_cache pointer to one referenced by
> the slab page where the object-to-be-freed resides.  This check was moved
> to cache_from_obj().  Later the check was also enabled for
> SLAB_FREELIST_HARDENED configs by commit 598a0717a816 ("mm/slab: validate
> cache membership under freelist hardening").
> 
> These checks and warnings can be useful especially for the debugging,
> which can be improved.  Commit 598a0717a816 changed the pr_err() with
> WARN_ON_ONCE() to WARN_ONCE() so only the first hit is now reported,
> others are silent.  This patch changes it to WARN() so that all errors are
> reported.
> 
> It's also useful to print SLUB allocation/free tracking info for the offending
> object, if tracking is enabled. Thus, export the SLUB print_tracking() function
> and provide an empty one for SLAB.
> 
> For SLUB we can also benefit from the static key check in
> kmem_cache_debug_flags(), but we need to move this function to slab.h and
> declare the static key there.
> 
> [1] https://lore.kernel.org/r/20200608230654.828134-18-guro@fb.com
> 
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>

Acked-by: Roman Gushchin <guro@fb.com>

Thanks!

> ---
>  mm/slab.c |  8 --------
>  mm/slab.h | 45 +++++++++++++++++++++++++++++++++++++++++++++
>  mm/slub.c | 38 +-------------------------------------
>  3 files changed, 46 insertions(+), 45 deletions(-)
> 
> diff --git a/mm/slab.c b/mm/slab.c
> index 6134c4c36d4c..9350062ffc1a 100644
> --- a/mm/slab.c
> +++ b/mm/slab.c
> @@ -3672,14 +3672,6 @@ void *__kmalloc_track_caller(size_t size, gfp_t flags, unsigned long caller)
>  }
>  EXPORT_SYMBOL(__kmalloc_track_caller);
>  
> -static inline struct kmem_cache *cache_from_obj(struct kmem_cache *s, void *x)
> -{
> -	if (memcg_kmem_enabled())
> -		return virt_to_cache(x);
> -	else
> -		return s;
> -}
> -
>  /**
>   * kmem_cache_free - Deallocate an object
>   * @cachep: The cache the allocation was from.
> diff --git a/mm/slab.h b/mm/slab.h
> index a2696d306b62..a9f5ba9ce9a7 100644
> --- a/mm/slab.h
> +++ b/mm/slab.h
> @@ -275,6 +275,34 @@ static inline int cache_vmstat_idx(struct kmem_cache *s)
>  		NR_SLAB_RECLAIMABLE : NR_SLAB_UNRECLAIMABLE;
>  }
>  
> +#ifdef CONFIG_SLUB_DEBUG
> +#ifdef CONFIG_SLUB_DEBUG_ON
> +DECLARE_STATIC_KEY_TRUE(slub_debug_enabled);
> +#else
> +DECLARE_STATIC_KEY_FALSE(slub_debug_enabled);
> +#endif
> +extern void print_tracking(struct kmem_cache *s, void *object);
> +#else
> +static inline void print_tracking(struct kmem_cache *s, void *object)
> +{
> +}
> +#endif
> +
> +/*
> + * Returns true if any of the specified slub_debug flags is enabled for the
> + * cache. Use only for flags parsed by setup_slub_debug() as it also enables
> + * the static key.
> + */
> +static inline bool kmem_cache_debug_flags(struct kmem_cache *s, slab_flags_t flags)
> +{
> +	VM_WARN_ON_ONCE(!(flags & SLAB_DEBUG_FLAGS));
> +#ifdef CONFIG_SLUB_DEBUG
> +	if (static_branch_unlikely(&slub_debug_enabled))
> +		return s->flags & flags;
> +#endif
> +	return false;
> +}
> +
>  #ifdef CONFIG_MEMCG_KMEM
>  
>  /* List of all root caches. */
> @@ -503,6 +531,23 @@ static __always_inline void uncharge_slab_page(struct page *page, int order,
>  	memcg_uncharge_slab(page, order, s);
>  }
>  
> +static inline struct kmem_cache *cache_from_obj(struct kmem_cache *s, void *x)
> +{
> +	struct kmem_cache *cachep;
> +
> +	if (!IS_ENABLED(CONFIG_SLAB_FREELIST_HARDENED) &&
> +	    !memcg_kmem_enabled() &&
> +	    !kmem_cache_debug_flags(s, SLAB_CONSISTENCY_CHECKS))
> +		return s;
> +
> +	cachep = virt_to_cache(x);
> +	if (WARN(cachep && !slab_equal_or_root(cachep, s),
> +		  "%s: Wrong slab cache. %s but object is from %s\n",
> +		  __func__, s->name, cachep->name))
> +		print_tracking(cachep, x);
> +	return cachep;
> +}
> +
>  static inline size_t slab_ksize(const struct kmem_cache *s)
>  {
>  #ifndef CONFIG_SLUB
> diff --git a/mm/slub.c b/mm/slub.c
> index 202fb423d195..0e635a8aa340 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -122,21 +122,6 @@ DEFINE_STATIC_KEY_FALSE(slub_debug_enabled);
>  #endif
>  #endif
>  
> -/*
> - * Returns true if any of the specified slub_debug flags is enabled for the
> - * cache. Use only for flags parsed by setup_slub_debug() as it also enables
> - * the static key.
> - */
> -static inline bool kmem_cache_debug_flags(struct kmem_cache *s, slab_flags_t flags)
> -{
> -	VM_WARN_ON_ONCE(!(flags & SLAB_DEBUG_FLAGS));
> -#ifdef CONFIG_SLUB_DEBUG
> -	if (static_branch_unlikely(&slub_debug_enabled))
> -		return s->flags & flags;
> -#endif
> -	return false;
> -}
> -
>  static inline bool kmem_cache_debug(struct kmem_cache *s)
>  {
>  	return kmem_cache_debug_flags(s, SLAB_DEBUG_FLAGS);
> @@ -653,7 +638,7 @@ static void print_track(const char *s, struct track *t, unsigned long pr_time)
>  #endif
>  }
>  
> -static void print_tracking(struct kmem_cache *s, void *object)
> +void print_tracking(struct kmem_cache *s, void *object)
>  {
>  	unsigned long pr_time = jiffies;
>  	if (!(s->flags & SLAB_STORE_USER))
> @@ -1525,10 +1510,6 @@ static bool freelist_corrupted(struct kmem_cache *s, struct page *page,
>  {
>  	return false;
>  }
> -
> -static void print_tracking(struct kmem_cache *s, void *object)
> -{
> -}
>  #endif /* CONFIG_SLUB_DEBUG */
>  
>  /*
> @@ -3180,23 +3161,6 @@ void ___cache_free(struct kmem_cache *cache, void *x, unsigned long addr)
>  }
>  #endif
>  
> -static inline struct kmem_cache *cache_from_obj(struct kmem_cache *s, void *x)
> -{
> -	struct kmem_cache *cachep;
> -
> -	if (!IS_ENABLED(CONFIG_SLAB_FREELIST_HARDENED) &&
> -	    !memcg_kmem_enabled() &&
> -	    !kmem_cache_debug_flags(s, SLAB_CONSISTENCY_CHECKS))
> -		return s;
> -
> -	cachep = virt_to_cache(x);
> -	if (WARN(cachep && !slab_equal_or_root(cachep, s),
> -		  "%s: Wrong slab cache. %s but object is from %s\n",
> -		  __func__, s->name, cachep->name))
> -		print_tracking(cachep, x);
> -	return cachep;
> -}
> -
>  void kmem_cache_free(struct kmem_cache *s, void *x)
>  {
>  	s = cache_from_obj(s, x);
> -- 
> 2.27.0
> 
> 

Re: [PATCH 9/9] mm, slab/slub: move and improve cache_from_obj()

[Kees Cook]

On Thu, Jun 18, 2020 at 01:05:53PM -0700, Roman Gushchin wrote:
> On Thu, Jun 18, 2020 at 12:10:38PM +0200, Vlastimil Babka wrote:
> > To prvent the churn of your patch moving the cache_from_obj() back to slab.h, I
> > think it's best if we modify my patch. The patch below should be squashed into
> > the current version in mmots, with the commit log used for the whole result.
> > 
> > This will cause conflicts while reapplying Roman's
> > mm-memcg-slab-use-a-single-set-of-kmem_caches-for-all-allocations.patch which
> > can be fixed by
> > a) throwing away the conflicting hunks for cache_from_obj() in slab.c and slub.c
> > b) applying this hunk instead:
> > 
> > --- a/mm/slab.h
> > +++ b/mm/slab.h
> > @@ -455,12 +455,11 @@ static inline struct kmem_cache *cache_from_obj(struct kmem_cache *s, void *x)
> >  	struct kmem_cache *cachep;
> >  
> >  	if (!IS_ENABLED(CONFIG_SLAB_FREELIST_HARDENED) &&
> > -	    !memcg_kmem_enabled() &&
> >  	    !kmem_cache_debug_flags(s, SLAB_CONSISTENCY_CHECKS))
> >  		return s;
> >  
> >  	cachep = virt_to_cache(x);
> > -	if (WARN(cachep && !slab_equal_or_root(cachep, s),
> > +	if (WARN(cachep && cachep != s,
> >  		  "%s: Wrong slab cache. %s but object is from %s\n",
> >  		  __func__, s->name, cachep->name))
> >  		print_tracking(cachep, x);
> > 
> > The fixup patch itself:
> > ----8<----
> > From b8df607d92b37e5329ce7bda62b2b364cc249893 Mon Sep 17 00:00:00 2001
> > From: Vlastimil Babka <vbabka@suse.cz>
> > Date: Thu, 18 Jun 2020 11:52:03 +0200
> > Subject: [PATCH] mm, slab/slub: improve error reporting and overhead of
> >  cache_from_obj()

Andrew, do you need this separately, or can you extract this fixup from
this thread?

-Kees

> > 
> > The function cache_from_obj() was added by commit b9ce5ef49f00 ("sl[au]b:
> > always get the cache from its page in kmem_cache_free()") to support
> > kmemcg, where per-memcg cache can be different from the root one, so we
> > can't use the kmem_cache pointer given to kmem_cache_free().
> > 
> > Prior to that commit, SLUB already had debugging check+warning that could
> > be enabled to compare the given kmem_cache pointer to one referenced by
> > the slab page where the object-to-be-freed resides.  This check was moved
> > to cache_from_obj().  Later the check was also enabled for
> > SLAB_FREELIST_HARDENED configs by commit 598a0717a816 ("mm/slab: validate
> > cache membership under freelist hardening").
> > 
> > These checks and warnings can be useful especially for the debugging,
> > which can be improved.  Commit 598a0717a816 changed the pr_err() with
> > WARN_ON_ONCE() to WARN_ONCE() so only the first hit is now reported,
> > others are silent.  This patch changes it to WARN() so that all errors are
> > reported.
> > 
> > It's also useful to print SLUB allocation/free tracking info for the offending
> > object, if tracking is enabled. Thus, export the SLUB print_tracking() function
> > and provide an empty one for SLAB.
> > 
> > For SLUB we can also benefit from the static key check in
> > kmem_cache_debug_flags(), but we need to move this function to slab.h and
> > declare the static key there.
> > 
> > [1] https://lore.kernel.org/r/20200608230654.828134-18-guro@fb.com
> > 
> > Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> 
> Acked-by: Roman Gushchin <guro@fb.com>
> 
> Thanks!
> 
> > ---
> >  mm/slab.c |  8 --------
> >  mm/slab.h | 45 +++++++++++++++++++++++++++++++++++++++++++++
> >  mm/slub.c | 38 +-------------------------------------
> >  3 files changed, 46 insertions(+), 45 deletions(-)
> > 
> > diff --git a/mm/slab.c b/mm/slab.c
> > index 6134c4c36d4c..9350062ffc1a 100644
> > --- a/mm/slab.c
> > +++ b/mm/slab.c
> > @@ -3672,14 +3672,6 @@ void *__kmalloc_track_caller(size_t size, gfp_t flags, unsigned long caller)
> >  }
> >  EXPORT_SYMBOL(__kmalloc_track_caller);
> >  
> > -static inline struct kmem_cache *cache_from_obj(struct kmem_cache *s, void *x)
> > -{
> > -	if (memcg_kmem_enabled())
> > -		return virt_to_cache(x);
> > -	else
> > -		return s;
> > -}
> > -
> >  /**
> >   * kmem_cache_free - Deallocate an object
> >   * @cachep: The cache the allocation was from.
> > diff --git a/mm/slab.h b/mm/slab.h
> > index a2696d306b62..a9f5ba9ce9a7 100644
> > --- a/mm/slab.h
> > +++ b/mm/slab.h
> > @@ -275,6 +275,34 @@ static inline int cache_vmstat_idx(struct kmem_cache *s)
> >  		NR_SLAB_RECLAIMABLE : NR_SLAB_UNRECLAIMABLE;
> >  }
> >  
> > +#ifdef CONFIG_SLUB_DEBUG
> > +#ifdef CONFIG_SLUB_DEBUG_ON
> > +DECLARE_STATIC_KEY_TRUE(slub_debug_enabled);
> > +#else
> > +DECLARE_STATIC_KEY_FALSE(slub_debug_enabled);
> > +#endif
> > +extern void print_tracking(struct kmem_cache *s, void *object);
> > +#else
> > +static inline void print_tracking(struct kmem_cache *s, void *object)
> > +{
> > +}
> > +#endif
> > +
> > +/*
> > + * Returns true if any of the specified slub_debug flags is enabled for the
> > + * cache. Use only for flags parsed by setup_slub_debug() as it also enables
> > + * the static key.
> > + */
> > +static inline bool kmem_cache_debug_flags(struct kmem_cache *s, slab_flags_t flags)
> > +{
> > +	VM_WARN_ON_ONCE(!(flags & SLAB_DEBUG_FLAGS));
> > +#ifdef CONFIG_SLUB_DEBUG
> > +	if (static_branch_unlikely(&slub_debug_enabled))
> > +		return s->flags & flags;
> > +#endif
> > +	return false;
> > +}
> > +
> >  #ifdef CONFIG_MEMCG_KMEM
> >  
> >  /* List of all root caches. */
> > @@ -503,6 +531,23 @@ static __always_inline void uncharge_slab_page(struct page *page, int order,
> >  	memcg_uncharge_slab(page, order, s);
> >  }
> >  
> > +static inline struct kmem_cache *cache_from_obj(struct kmem_cache *s, void *x)
> > +{
> > +	struct kmem_cache *cachep;
> > +
> > +	if (!IS_ENABLED(CONFIG_SLAB_FREELIST_HARDENED) &&
> > +	    !memcg_kmem_enabled() &&
> > +	    !kmem_cache_debug_flags(s, SLAB_CONSISTENCY_CHECKS))
> > +		return s;
> > +
> > +	cachep = virt_to_cache(x);
> > +	if (WARN(cachep && !slab_equal_or_root(cachep, s),
> > +		  "%s: Wrong slab cache. %s but object is from %s\n",
> > +		  __func__, s->name, cachep->name))
> > +		print_tracking(cachep, x);
> > +	return cachep;
> > +}
> > +
> >  static inline size_t slab_ksize(const struct kmem_cache *s)
> >  {
> >  #ifndef CONFIG_SLUB
> > diff --git a/mm/slub.c b/mm/slub.c
> > index 202fb423d195..0e635a8aa340 100644
> > --- a/mm/slub.c
> > +++ b/mm/slub.c
> > @@ -122,21 +122,6 @@ DEFINE_STATIC_KEY_FALSE(slub_debug_enabled);
> >  #endif
> >  #endif
> >  
> > -/*
> > - * Returns true if any of the specified slub_debug flags is enabled for the
> > - * cache. Use only for flags parsed by setup_slub_debug() as it also enables
> > - * the static key.
> > - */
> > -static inline bool kmem_cache_debug_flags(struct kmem_cache *s, slab_flags_t flags)
> > -{
> > -	VM_WARN_ON_ONCE(!(flags & SLAB_DEBUG_FLAGS));
> > -#ifdef CONFIG_SLUB_DEBUG
> > -	if (static_branch_unlikely(&slub_debug_enabled))
> > -		return s->flags & flags;
> > -#endif
> > -	return false;
> > -}
> > -
> >  static inline bool kmem_cache_debug(struct kmem_cache *s)
> >  {
> >  	return kmem_cache_debug_flags(s, SLAB_DEBUG_FLAGS);
> > @@ -653,7 +638,7 @@ static void print_track(const char *s, struct track *t, unsigned long pr_time)
> >  #endif
> >  }
> >  
> > -static void print_tracking(struct kmem_cache *s, void *object)
> > +void print_tracking(struct kmem_cache *s, void *object)
> >  {
> >  	unsigned long pr_time = jiffies;
> >  	if (!(s->flags & SLAB_STORE_USER))
> > @@ -1525,10 +1510,6 @@ static bool freelist_corrupted(struct kmem_cache *s, struct page *page,
> >  {
> >  	return false;
> >  }
> > -
> > -static void print_tracking(struct kmem_cache *s, void *object)
> > -{
> > -}
> >  #endif /* CONFIG_SLUB_DEBUG */
> >  
> >  /*
> > @@ -3180,23 +3161,6 @@ void ___cache_free(struct kmem_cache *cache, void *x, unsigned long addr)
> >  }
> >  #endif
> >  
> > -static inline struct kmem_cache *cache_from_obj(struct kmem_cache *s, void *x)
> > -{
> > -	struct kmem_cache *cachep;
> > -
> > -	if (!IS_ENABLED(CONFIG_SLAB_FREELIST_HARDENED) &&
> > -	    !memcg_kmem_enabled() &&
> > -	    !kmem_cache_debug_flags(s, SLAB_CONSISTENCY_CHECKS))
> > -		return s;
> > -
> > -	cachep = virt_to_cache(x);
> > -	if (WARN(cachep && !slab_equal_or_root(cachep, s),
> > -		  "%s: Wrong slab cache. %s but object is from %s\n",
> > -		  __func__, s->name, cachep->name))
> > -		print_tracking(cachep, x);
> > -	return cachep;
> > -}
> > -
> >  void kmem_cache_free(struct kmem_cache *s, void *x)
> >  {
> >  	s = cache_from_obj(s, x);
> > -- 
> > 2.27.0
> > 
> > 

-- 
Kees Cook

Re: [PATCH 9/9] mm, slab/slub: move and improve cache_from_obj()

[Vlastimil Babka]

On 6/18/20 12:10 PM, Vlastimil Babka wrote:
> ----8<----
> From b8df607d92b37e5329ce7bda62b2b364cc249893 Mon Sep 17 00:00:00 2001
> From: Vlastimil Babka <vbabka@suse.cz>
> Date: Thu, 18 Jun 2020 11:52:03 +0200
> Subject: [PATCH] mm, slab/slub: improve error reporting and overhead of
>  cache_from_obj()
> 

Another small fixup, please.

----8<----
From 2cd24408828a22a3cd4301afbaf98767b1a14eb1 Mon Sep 17 00:00:00 2001
From: Vlastimil Babka <vbabka@suse.cz>
Date: Wed, 24 Jun 2020 09:49:15 +0200
Subject: [PATCH] mm, slab/slub: improve error reporting and overhead of
 cache_from_obj()-fix

The added VM_WARN_ON_ONCE triggers [1] with CONFIG_SLAB, as SLAB_DEBUG_FLAGS
doesn't include SLAB_CONSISTENCY_CHECKS. Move the check under #ifdef
SLUB_DEBUG.

[1] https://lore.kernel.org/r/20200623090213.GW5535@shao2-debian

Reported-by: kernel test robot <rong.a.chen@intel.com>
Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
---
 mm/slab.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/slab.h b/mm/slab.h
index 525260217013..e829d9f5e6ef 100644
--- a/mm/slab.h
+++ b/mm/slab.h
@@ -229,8 +229,8 @@ static inline void print_tracking(struct kmem_cache *s, void *object)
  */
 static inline bool kmem_cache_debug_flags(struct kmem_cache *s, slab_flags_t flags)
 {
-	VM_WARN_ON_ONCE(!(flags & SLAB_DEBUG_FLAGS));
 #ifdef CONFIG_SLUB_DEBUG
+	VM_WARN_ON_ONCE(!(flags & SLAB_DEBUG_FLAGS));
 	if (static_branch_unlikely(&slub_debug_enabled))
 		return s->flags & flags;
 #endif
-- 
2.27.0