I was doing some debugging recently and noticed that my pointers were
being hashed while slub_debug was on the kernel commandline. Let's force
on the no hash pointer option when slub_debug is on the kernel
commandline so that the prints are more meaningful.
The first two patches are something else I noticed while looking at the
code. The message argument is never used so the debugging messages are
not as clear as they could be and the slub_debug=- behavior seems to be
busted. Then there's a printf fixup from Joe and the final patch is the
one that force disables pointer hashing.
Changes from v2 (https://lore.kernel.org/r/[email protected]):
* Fixed up Fixes tag on first first patch
* Picked up acks
* Moved decl of no_hash_pointers() to kernel.h
Changes from v1:
* Dropped the hexdump printing format
* Forced on the no_hash_pointers option instead of pushing %px
Joe Perches (1):
slub: Indicate slab_fix() uses printf formats
Stephen Boyd (3):
slub: Restore slub_debug=- behavior
slub: Actually use 'message' in restore_bytes()
slub: Force on no_hash_pointers when slub_debug is enabled
include/linux/kernel.h | 2 ++
lib/vsprintf.c | 2 +-
mm/slub.c | 13 ++++++++++---
3 files changed, 13 insertions(+), 4 deletions(-)
base-commit: d07f6ca923ea0927a1024dfccafc5b53b61cfecc
--
https://chromeos.dev
Passing slub_debug=- on the kernel commandline is supposed to disable
slub debugging. This is especially useful with CONFIG_SLUB_DEBUG_ON
where the default is to have slub debugging enabled in the build. Due to
some code reorganization this behavior was dropped, but the code to make
it work mostly stuck around. Restore the previous behavior by disabling
the static key when we parse the commandline and see that we're trying
to disable slub debugging.
Acked-by: Vlastimil Babka <[email protected]>
Reviewed-by: Muchun Song <[email protected]>
Fixes: ca0cab65ea2b ("mm, slub: introduce static key for slub_debug()")
Signed-off-by: Stephen Boyd <[email protected]>
---
mm/slub.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/mm/slub.c b/mm/slub.c
index 438fa8d4c970..2f53e8a9c28e 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -1396,6 +1396,8 @@ static int __init setup_slub_debug(char *str)
out:
if (slub_debug != 0 || slub_debug_string)
static_branch_enable(&slub_debug_enabled);
+ else
+ static_branch_disable(&slub_debug_enabled);
if ((static_branch_unlikely(&init_on_alloc) ||
static_branch_unlikely(&init_on_free)) &&
(slub_debug & SLAB_POISON))
--
https://chromeos.dev
From: Joe Perches <[email protected]>
Ideally, slab_fix() would be marked with __printf and the format here
would not use \n as that's emitted by the slab_fix(). Make these
changes.
Signed-off-by: Joe Perches <[email protected]>
Acked-by: Vlastimil Babka <[email protected]>
Signed-off-by: Stephen Boyd <[email protected]>
---
mm/slub.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/mm/slub.c b/mm/slub.c
index 6168b3ce1b3e..bf4949115412 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -672,6 +672,7 @@ static void slab_bug(struct kmem_cache *s, char *fmt, ...)
va_end(args);
}
+__printf(2, 3)
static void slab_fix(struct kmem_cache *s, char *fmt, ...)
{
struct va_format vaf;
@@ -777,7 +778,7 @@ static void init_object(struct kmem_cache *s, void *object, u8 val)
static void restore_bytes(struct kmem_cache *s, char *message, u8 data,
void *from, void *to)
{
- slab_fix(s, "Restoring %s 0x%p-0x%p=0x%x\n", message, from, to - 1, data);
+ slab_fix(s, "Restoring %s 0x%p-0x%p=0x%x", message, from, to - 1, data);
memset(from, data, to - from);
}
@@ -1026,13 +1027,13 @@ static int on_freelist(struct kmem_cache *s, struct page *page, void *search)
slab_err(s, page, "Wrong number of objects. Found %d but should be %d",
page->objects, max_objects);
page->objects = max_objects;
- slab_fix(s, "Number of objects adjusted.");
+ slab_fix(s, "Number of objects adjusted");
}
if (page->inuse != page->objects - nr) {
slab_err(s, page, "Wrong object count. Counter is %d but counted were %d",
page->inuse, page->objects - nr);
page->inuse = page->objects - nr;
- slab_fix(s, "Object count adjusted.");
+ slab_fix(s, "Object count adjusted");
}
return search == NULL;
}
--
https://chromeos.dev
The message argument isn't used here. Let's pass the string to the
printk message so that the developer can figure out what's happening,
instead of guessing that a redzone is being restored, etc.
Reviewed-by: Vlastimil Babka <[email protected]>
Acked-by: David Rientjes <[email protected]>
Reviewed-by: Muchun Song <[email protected]>
Signed-off-by: Stephen Boyd <[email protected]>
---
mm/slub.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/mm/slub.c b/mm/slub.c
index 2f53e8a9c28e..6168b3ce1b3e 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -777,7 +777,7 @@ static void init_object(struct kmem_cache *s, void *object, u8 val)
static void restore_bytes(struct kmem_cache *s, char *message, u8 data,
void *from, void *to)
{
- slab_fix(s, "Restoring 0x%p-0x%p=0x%x\n", from, to - 1, data);
+ slab_fix(s, "Restoring %s 0x%p-0x%p=0x%x\n", message, from, to - 1, data);
memset(from, data, to - from);
}
--
https://chromeos.dev
Obscuring the pointers that slub shows when debugging makes for some
confusing slub debug messages:
Padding overwritten. 0x0000000079f0674a-0x000000000d4dce17
Those addresses are hashed for kernel security reasons. If we're trying
to be secure with slub_debug on the commandline we have some big
problems given that we dump whole chunks of kernel memory to the kernel
logs. Let's force on the no_hash_pointers commandline flag when
slub_debug is on the commandline. This makes slub debug messages more
meaningful and if by chance a kernel address is in some slub debug
object dump we will have a better chance of figuring out what went
wrong.
Note that we don't use %px in the slub code because we want to reduce
the number of places that %px is used in the kernel. This also nicely
prints a big fat warning at kernel boot if slub_debug is on the
commandline so that we know that this kernel shouldn't be used on
production systems.
Signed-off-by: Stephen Boyd <[email protected]>
---
include/linux/kernel.h | 2 ++
lib/vsprintf.c | 2 +-
mm/slub.c | 4 ++++
3 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 15d8bad3d2f2..bf950621febf 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -357,6 +357,8 @@ int sscanf(const char *, const char *, ...);
extern __scanf(2, 0)
int vsscanf(const char *, const char *, va_list);
+extern int no_hash_pointers_enable(char *str);
+
extern int get_option(char **str, int *pint);
extern char *get_options(const char *str, int nints, int *ints);
extern unsigned long long memparse(const char *ptr, char **retptr);
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index f0c35d9b65bf..cc281f5895f9 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -2186,7 +2186,7 @@ char *fwnode_string(char *buf, char *end, struct fwnode_handle *fwnode,
bool no_hash_pointers __ro_after_init;
EXPORT_SYMBOL_GPL(no_hash_pointers);
-static int __init no_hash_pointers_enable(char *str)
+int __init no_hash_pointers_enable(char *str)
{
if (no_hash_pointers)
return 0;
diff --git a/mm/slub.c b/mm/slub.c
index bf4949115412..a722794f1dbd 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -4460,6 +4460,10 @@ void __init kmem_cache_init(void)
if (debug_guardpage_minorder())
slub_max_order = 0;
+ /* Print slub debugging pointers without hashing */
+ if (static_branch_unlikely(&slub_debug_enabled))
+ no_hash_pointers_enable(NULL);
+
kmem_cache_node = &boot_kmem_cache_node;
kmem_cache = &boot_kmem_cache;
--
https://chromeos.dev
Hi Stephen,
I love your patch! Yet something to improve:
[auto build test ERROR on d07f6ca923ea0927a1024dfccafc5b53b61cfecc]
url: https://github.com/0day-ci/linux/commits/Stephen-Boyd/slub-Print-non-hashed-pointers-in-slub-debugging/20210602-022255
base: d07f6ca923ea0927a1024dfccafc5b53b61cfecc
config: s390-randconfig-r036-20210601 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project db26cd30b6dd65e88d786e97a1e453af5cd48966)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install s390 cross compiling tool for clang build
# apt-get install binutils-s390x-linux-gnu
# https://github.com/0day-ci/linux/commit/ab6ede356a6f366690b4a4e9dd597b63be241ad0
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Stephen-Boyd/slub-Print-non-hashed-pointers-in-slub-debugging/20210602-022255
git checkout ab6ede356a6f366690b4a4e9dd597b63be241ad0
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=s390
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>):
In file included from mm/slub.c:14:
In file included from include/linux/swap.h:9:
In file included from include/linux/memcontrol.h:22:
In file included from include/linux/writeback.h:14:
In file included from include/linux/blk-cgroup.h:23:
In file included from include/linux/blkdev.h:25:
In file included from include/linux/scatterlist.h:9:
In file included from arch/s390/include/asm/io.h:75:
include/asm-generic/io.h:464:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __raw_readb(PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:477:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/big_endian.h:36:59: note: expanded from macro '__le16_to_cpu'
#define __le16_to_cpu(x) __swab16((__force __u16)(__le16)(x))
^
include/uapi/linux/swab.h:102:54: note: expanded from macro '__swab16'
#define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
^
In file included from mm/slub.c:14:
In file included from include/linux/swap.h:9:
In file included from include/linux/memcontrol.h:22:
In file included from include/linux/writeback.h:14:
In file included from include/linux/blk-cgroup.h:23:
In file included from include/linux/blkdev.h:25:
In file included from include/linux/scatterlist.h:9:
In file included from arch/s390/include/asm/io.h:75:
include/asm-generic/io.h:490:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/big_endian.h:34:59: note: expanded from macro '__le32_to_cpu'
#define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
^
include/uapi/linux/swab.h:115:54: note: expanded from macro '__swab32'
#define __swab32(x) (__u32)__builtin_bswap32((__u32)(x))
^
In file included from mm/slub.c:14:
In file included from include/linux/swap.h:9:
In file included from include/linux/memcontrol.h:22:
In file included from include/linux/writeback.h:14:
In file included from include/linux/blk-cgroup.h:23:
In file included from include/linux/blkdev.h:25:
In file included from include/linux/scatterlist.h:9:
In file included from arch/s390/include/asm/io.h:75:
include/asm-generic/io.h:501:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writeb(value, PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:511:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:521:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:609:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
readsb(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:617:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
readsw(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:625:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
readsl(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:634:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
writesb(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:643:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
writesw(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:652:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
writesl(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
>> mm/slub.c:4464:30: error: use of undeclared identifier 'slub_debug_enabled'
if (static_branch_unlikely(&slub_debug_enabled))
^
>> mm/slub.c:4464:30: error: use of undeclared identifier 'slub_debug_enabled'
>> mm/slub.c:4464:30: error: use of undeclared identifier 'slub_debug_enabled'
>> mm/slub.c:4464:30: error: use of undeclared identifier 'slub_debug_enabled'
>> mm/slub.c:4464:6: error: invalid argument type 'void' to unary expression
if (static_branch_unlikely(&slub_debug_enabled))
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/jump_label.h:496:35: note: expanded from macro 'static_branch_unlikely'
#define static_branch_unlikely(x) unlikely_notrace(static_key_enabled(&(x)->key))
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/compiler.h:80:30: note: expanded from macro 'unlikely_notrace'
# define unlikely_notrace(x) unlikely(x)
^~~~~~~~~~~
include/linux/compiler.h:78:40: note: expanded from macro 'unlikely'
# define unlikely(x) __builtin_expect(!!(x), 0)
^~~~
12 warnings and 5 errors generated.
vim +/slub_debug_enabled +4464 mm/slub.c
4453
4454 void __init kmem_cache_init(void)
4455 {
4456 static __initdata struct kmem_cache boot_kmem_cache,
4457 boot_kmem_cache_node;
4458 int node;
4459
4460 if (debug_guardpage_minorder())
4461 slub_max_order = 0;
4462
4463 /* Print slub debugging pointers without hashing */
> 4464 if (static_branch_unlikely(&slub_debug_enabled))
4465 no_hash_pointers_enable(NULL);
4466
4467 kmem_cache_node = &boot_kmem_cache_node;
4468 kmem_cache = &boot_kmem_cache;
4469
4470 /*
4471 * Initialize the nodemask for which we will allocate per node
4472 * structures. Here we don't need taking slab_mutex yet.
4473 */
4474 for_each_node_state(node, N_NORMAL_MEMORY)
4475 node_set(node, slab_nodes);
4476
4477 create_boot_cache(kmem_cache_node, "kmem_cache_node",
4478 sizeof(struct kmem_cache_node), SLAB_HWCACHE_ALIGN, 0, 0);
4479
4480 register_hotmemory_notifier(&slab_memory_callback_nb);
4481
4482 /* Able to allocate the per node structures */
4483 slab_state = PARTIAL;
4484
4485 create_boot_cache(kmem_cache, "kmem_cache",
4486 offsetof(struct kmem_cache, node) +
4487 nr_node_ids * sizeof(struct kmem_cache_node *),
4488 SLAB_HWCACHE_ALIGN, 0, 0);
4489
4490 kmem_cache = bootstrap(&boot_kmem_cache);
4491 kmem_cache_node = bootstrap(&boot_kmem_cache_node);
4492
4493 /* Now we can use the kmem_cache to allocate kmalloc slabs */
4494 setup_kmalloc_cache_index_table();
4495 create_kmalloc_caches(0);
4496
4497 /* Setup random freelists for each cache */
4498 init_freelist_randomization();
4499
4500 cpuhp_setup_state_nocalls(CPUHP_SLUB_DEAD, "slub:dead", NULL,
4501 slub_cpu_dead);
4502
4503 pr_info("SLUB: HWalign=%d, Order=%u-%u, MinObjects=%u, CPUs=%u, Nodes=%u\n",
4504 cache_line_size(),
4505 slub_min_order, slub_max_order, slub_min_objects,
4506 nr_cpu_ids, nr_node_ids);
4507 }
4508
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
On Wed, 2 Jun 2021 06:45:55 +0800 kernel test robot <[email protected]> wrote:
> >> mm/slub.c:4464:30: error: use of undeclared identifier 'slub_debug_enabled'
> if (static_branch_unlikely(&slub_debug_enabled))
> ^
> >> mm/slub.c:4464:30: error: use of undeclared identifier 'slub_debug_enabled'
> >> mm/slub.c:4464:30: error: use of undeclared identifier 'slub_debug_enabled'
> >> mm/slub.c:4464:30: error: use of undeclared identifier 'slub_debug_enabled'
> >> mm/slub.c:4464:6: error: invalid argument type 'void' to unary expression
> if (static_branch_unlikely(&slub_debug_enabled))
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Thanks. Stephen, how about this?
--- a/mm/slub.c~slub-force-on-no_hash_pointers-when-slub_debug-is-enabled-fix
+++ a/mm/slub.c
@@ -117,12 +117,26 @@
*/
#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 bool __slub_debug_enabled(void)
+{
+ return static_branch_unlikely(&slub_debug_enabled);
+}
+
+#else /* CONFIG_SLUB_DEBUG */
+
+static inline bool __slub_debug_enabled(void)
+{
+ return false;
+}
+
+#endif /* CONFIG_SLUB_DEBUG */
static inline bool kmem_cache_debug(struct kmem_cache *s)
{
@@ -4493,7 +4507,7 @@ void __init kmem_cache_init(void)
slub_max_order = 0;
/* Print slub debugging pointers without hashing */
- if (static_branch_unlikely(&slub_debug_enabled))
+ if (__slub_debug_enabled())
no_hash_pointers_enable(NULL);
kmem_cache_node = &boot_kmem_cache_node;
_
Quoting Andrew Morton (2021-06-01 17:26:59)
> On Wed, 2 Jun 2021 06:45:55 +0800 kernel test robot <[email protected]> wrote:
>
> > >> mm/slub.c:4464:30: error: use of undeclared identifier 'slub_debug_enabled'
> > if (static_branch_unlikely(&slub_debug_enabled))
> > ^
> > >> mm/slub.c:4464:30: error: use of undeclared identifier 'slub_debug_enabled'
> > >> mm/slub.c:4464:30: error: use of undeclared identifier 'slub_debug_enabled'
> > >> mm/slub.c:4464:30: error: use of undeclared identifier 'slub_debug_enabled'
> > >> mm/slub.c:4464:6: error: invalid argument type 'void' to unary expression
> > if (static_branch_unlikely(&slub_debug_enabled))
> > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> Thanks. Stephen, how about this?
Looks good to me. Thanks for the quick fix!
>
> --- a/mm/slub.c~slub-force-on-no_hash_pointers-when-slub_debug-is-enabled-fix
> +++ a/mm/slub.c
> @@ -117,12 +117,26 @@
> */
>
> #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 bool __slub_debug_enabled(void)
> +{
> + return static_branch_unlikely(&slub_debug_enabled);
To make this even better it could be
return static_branch_maybe(CONFIG_SLUB_DEBUG_ON, &slub_debug_enabled);
> +}
> +
> +#else /* CONFIG_SLUB_DEBUG */
> +
> +static inline bool __slub_debug_enabled(void)
> +{
> + return false;
> +}
> +
> +#endif /* CONFIG_SLUB_DEBUG */
>
> static inline bool kmem_cache_debug(struct kmem_cache *s)
> {
> @@ -4493,7 +4507,7 @@ void __init kmem_cache_init(void)
> slub_max_order = 0;
>
> /* Print slub debugging pointers without hashing */
> - if (static_branch_unlikely(&slub_debug_enabled))
> + if (__slub_debug_enabled())
It would be super cool if static branches could be optimized out when
they're never changed by any code, nor exported to code, just tested in
conditions. I've no idea if that is the case though.
> no_hash_pointers_enable(NULL);
>
> kmem_cache_node = &boot_kmem_cache_node;
On 6/1/21 8:22 PM, Stephen Boyd wrote:
> Obscuring the pointers that slub shows when debugging makes for some
> confusing slub debug messages:
>
> Padding overwritten. 0x0000000079f0674a-0x000000000d4dce17
>
> Those addresses are hashed for kernel security reasons. If we're trying
> to be secure with slub_debug on the commandline we have some big
> problems given that we dump whole chunks of kernel memory to the kernel
> logs. Let's force on the no_hash_pointers commandline flag when
> slub_debug is on the commandline. This makes slub debug messages more
> meaningful and if by chance a kernel address is in some slub debug
> object dump we will have a better chance of figuring out what went
> wrong.
>
> Note that we don't use %px in the slub code because we want to reduce
> the number of places that %px is used in the kernel. This also nicely
> prints a big fat warning at kernel boot if slub_debug is on the
> commandline so that we know that this kernel shouldn't be used on
> production systems.
>
> Signed-off-by: Stephen Boyd <[email protected]>
Acked-by: Vlastimil Babka <[email protected]>
> ---
> include/linux/kernel.h | 2 ++
> lib/vsprintf.c | 2 +-
> mm/slub.c | 4 ++++
> 3 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index 15d8bad3d2f2..bf950621febf 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -357,6 +357,8 @@ int sscanf(const char *, const char *, ...);
> extern __scanf(2, 0)
> int vsscanf(const char *, const char *, va_list);
>
> +extern int no_hash_pointers_enable(char *str);
> +
> extern int get_option(char **str, int *pint);
> extern char *get_options(const char *str, int nints, int *ints);
> extern unsigned long long memparse(const char *ptr, char **retptr);
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index f0c35d9b65bf..cc281f5895f9 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -2186,7 +2186,7 @@ char *fwnode_string(char *buf, char *end, struct fwnode_handle *fwnode,
> bool no_hash_pointers __ro_after_init;
> EXPORT_SYMBOL_GPL(no_hash_pointers);
>
> -static int __init no_hash_pointers_enable(char *str)
> +int __init no_hash_pointers_enable(char *str)
> {
> if (no_hash_pointers)
> return 0;
> diff --git a/mm/slub.c b/mm/slub.c
> index bf4949115412..a722794f1dbd 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -4460,6 +4460,10 @@ void __init kmem_cache_init(void)
> if (debug_guardpage_minorder())
> slub_max_order = 0;
>
> + /* Print slub debugging pointers without hashing */
> + if (static_branch_unlikely(&slub_debug_enabled))
> + no_hash_pointers_enable(NULL);
> +
> kmem_cache_node = &boot_kmem_cache_node;
> kmem_cache = &boot_kmem_cache;
>
>
On 6/2/21 3:03 AM, Stephen Boyd wrote:
> Quoting Andrew Morton (2021-06-01 17:26:59)
>> On Wed, 2 Jun 2021 06:45:55 +0800 kernel test robot <[email protected]> wrote:
>>
>> > >> mm/slub.c:4464:30: error: use of undeclared identifier 'slub_debug_enabled'
>> > if (static_branch_unlikely(&slub_debug_enabled))
>> > ^
>> > >> mm/slub.c:4464:30: error: use of undeclared identifier 'slub_debug_enabled'
>> > >> mm/slub.c:4464:30: error: use of undeclared identifier 'slub_debug_enabled'
>> > >> mm/slub.c:4464:30: error: use of undeclared identifier 'slub_debug_enabled'
>> > >> mm/slub.c:4464:6: error: invalid argument type 'void' to unary expression
>> > if (static_branch_unlikely(&slub_debug_enabled))
>> > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>
>> Thanks. Stephen, how about this?
>
> Looks good to me. Thanks for the quick fix!
>
>>
>> --- a/mm/slub.c~slub-force-on-no_hash_pointers-when-slub_debug-is-enabled-fix
>> +++ a/mm/slub.c
>> @@ -117,12 +117,26 @@
>> */
>>
>> #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 bool __slub_debug_enabled(void)
>> +{
>> + return static_branch_unlikely(&slub_debug_enabled);
>
> To make this even better it could be
>
> return static_branch_maybe(CONFIG_SLUB_DEBUG_ON, &slub_debug_enabled);
>
>> +}
>> +
>> +#else /* CONFIG_SLUB_DEBUG */
>> +
>> +static inline bool __slub_debug_enabled(void)
>> +{
>> + return false;
>> +}
>> +
>> +#endif /* CONFIG_SLUB_DEBUG */
>>
>> static inline bool kmem_cache_debug(struct kmem_cache *s)
>> {
>> @@ -4493,7 +4507,7 @@ void __init kmem_cache_init(void)
>> slub_max_order = 0;
>>
>> /* Print slub debugging pointers without hashing */
>> - if (static_branch_unlikely(&slub_debug_enabled))
>> + if (__slub_debug_enabled())
A minimal fix would be to put this under #ifdef CONFIG_SLUB_DEBUG
and use static_key_enabled() as we don't need the jump label optimization for
init code. But the current fix works.
>
> It would be super cool if static branches could be optimized out when
> they're never changed by any code, nor exported to code, just tested in
> conditions. I've no idea if that is the case though.
>
>> no_hash_pointers_enable(NULL);
>>
>> kmem_cache_node = &boot_kmem_cache_node;
>
On Tue 2021-06-01 11:22:02, Stephen Boyd wrote:
> Obscuring the pointers that slub shows when debugging makes for some
> confusing slub debug messages:
>
> Padding overwritten. 0x0000000079f0674a-0x000000000d4dce17
>
> Those addresses are hashed for kernel security reasons. If we're trying
> to be secure with slub_debug on the commandline we have some big
> problems given that we dump whole chunks of kernel memory to the kernel
> logs. Let's force on the no_hash_pointers commandline flag when
> slub_debug is on the commandline. This makes slub debug messages more
> meaningful and if by chance a kernel address is in some slub debug
> object dump we will have a better chance of figuring out what went
> wrong.
>
> Note that we don't use %px in the slub code because we want to reduce
> the number of places that %px is used in the kernel. This also nicely
> prints a big fat warning at kernel boot if slub_debug is on the
> commandline so that we know that this kernel shouldn't be used on
> production systems.
>
> Signed-off-by: Stephen Boyd <[email protected]>
Acked-by: Petr Mladek <[email protected]>
Best Regards,
Petr
On Tue, 1 Jun 2021, Stephen Boyd wrote:
> From: Joe Perches <[email protected]>
>
> Ideally, slab_fix() would be marked with __printf and the format here
> would not use \n as that's emitted by the slab_fix(). Make these
> changes.
>
> Signed-off-by: Joe Perches <[email protected]>
> Acked-by: Vlastimil Babka <[email protected]>
> Signed-off-by: Stephen Boyd <[email protected]>
Acked-by: David Rientjes <[email protected]>
On Tue, Jun 01, 2021 at 11:22:02AM -0700, Stephen Boyd wrote:
> Obscuring the pointers that slub shows when debugging makes for some
> confusing slub debug messages:
>
> Padding overwritten. 0x0000000079f0674a-0x000000000d4dce17
>
> Those addresses are hashed for kernel security reasons. If we're trying
> to be secure with slub_debug on the commandline we have some big
> problems given that we dump whole chunks of kernel memory to the kernel
> logs. Let's force on the no_hash_pointers commandline flag when
> slub_debug is on the commandline. This makes slub debug messages more
> meaningful and if by chance a kernel address is in some slub debug
> object dump we will have a better chance of figuring out what went
> wrong.
>
> Note that we don't use %px in the slub code because we want to reduce
> the number of places that %px is used in the kernel. This also nicely
> prints a big fat warning at kernel boot if slub_debug is on the
> commandline so that we know that this kernel shouldn't be used on
> production systems.
Eeeek. I missed this patch. NAK NAK. People use slub_debug for
production systems to gain redzoning, etc, as a layer of defense, and
they absolutely do not want %p-hashing disabled. %p hashing is
controlled by the no_hash_pointers boot param (since v5.12), and needs to stay
separate from slub_debug.
Can we please revert this in Linus's tree and in v5.14?
-Kees
>
> Signed-off-by: Stephen Boyd <[email protected]>
> ---
> include/linux/kernel.h | 2 ++
> lib/vsprintf.c | 2 +-
> mm/slub.c | 4 ++++
> 3 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index 15d8bad3d2f2..bf950621febf 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -357,6 +357,8 @@ int sscanf(const char *, const char *, ...);
> extern __scanf(2, 0)
> int vsscanf(const char *, const char *, va_list);
>
> +extern int no_hash_pointers_enable(char *str);
> +
> extern int get_option(char **str, int *pint);
> extern char *get_options(const char *str, int nints, int *ints);
> extern unsigned long long memparse(const char *ptr, char **retptr);
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index f0c35d9b65bf..cc281f5895f9 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -2186,7 +2186,7 @@ char *fwnode_string(char *buf, char *end, struct fwnode_handle *fwnode,
> bool no_hash_pointers __ro_after_init;
> EXPORT_SYMBOL_GPL(no_hash_pointers);
>
> -static int __init no_hash_pointers_enable(char *str)
> +int __init no_hash_pointers_enable(char *str)
> {
> if (no_hash_pointers)
> return 0;
> diff --git a/mm/slub.c b/mm/slub.c
> index bf4949115412..a722794f1dbd 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -4460,6 +4460,10 @@ void __init kmem_cache_init(void)
> if (debug_guardpage_minorder())
> slub_max_order = 0;
>
> + /* Print slub debugging pointers without hashing */
> + if (static_branch_unlikely(&slub_debug_enabled))
> + no_hash_pointers_enable(NULL);
> +
> kmem_cache_node = &boot_kmem_cache_node;
> kmem_cache = &boot_kmem_cache;
>
> --
> https://chromeos.dev
>
--
Kees Cook
On Mon, Sep 20, 2021 at 11:23:01AM -0700, Stephen Boyd wrote:
> Quoting Kees Cook (2021-09-20 07:29:54)
> > On Tue, Jun 01, 2021 at 11:22:02AM -0700, Stephen Boyd wrote:
> > > Obscuring the pointers that slub shows when debugging makes for some
> > > confusing slub debug messages:
> > >
> > > Padding overwritten. 0x0000000079f0674a-0x000000000d4dce17
> > >
> > > Those addresses are hashed for kernel security reasons. If we're trying
> > > to be secure with slub_debug on the commandline we have some big
> > > problems given that we dump whole chunks of kernel memory to the kernel
> > > logs. Let's force on the no_hash_pointers commandline flag when
> > > slub_debug is on the commandline. This makes slub debug messages more
> > > meaningful and if by chance a kernel address is in some slub debug
> > > object dump we will have a better chance of figuring out what went
> > > wrong.
> > >
> > > Note that we don't use %px in the slub code because we want to reduce
> > > the number of places that %px is used in the kernel. This also nicely
> > > prints a big fat warning at kernel boot if slub_debug is on the
> > > commandline so that we know that this kernel shouldn't be used on
> > > production systems.
> >
> > Eeeek. I missed this patch. NAK NAK. People use slub_debug for
> > production systems to gain redzoning, etc, as a layer of defense, and
> > they absolutely do not want %p-hashing disabled. %p hashing is
> > controlled by the no_hash_pointers boot param (since v5.12), and needs to stay
> > separate from slub_debug.
> >
> > Can we please revert this in Linus's tree and in v5.14?
> >
>
> This is fine with me as long as debugging with slub_debug on the
> commandline is possible. Would you prefer v1 of this patch series[1]
> that uses the printk format to print unhashed pointers in slub debugging
> messages?
>
> [1] https://lore.kernel.org/r/[email protected]
I'd like to keep %px use in the kernel minimized. Seeing full pointers (%p
hashing disabled) can be done with the no_hash_pointers boot param, and
that's used in other debug cases as well. I'd rather keep it a global
knob.
Thanks!
-Kees
--
Kees Cook
Quoting Kees Cook (2021-09-20 07:29:54)
> On Tue, Jun 01, 2021 at 11:22:02AM -0700, Stephen Boyd wrote:
> > Obscuring the pointers that slub shows when debugging makes for some
> > confusing slub debug messages:
> >
> > Padding overwritten. 0x0000000079f0674a-0x000000000d4dce17
> >
> > Those addresses are hashed for kernel security reasons. If we're trying
> > to be secure with slub_debug on the commandline we have some big
> > problems given that we dump whole chunks of kernel memory to the kernel
> > logs. Let's force on the no_hash_pointers commandline flag when
> > slub_debug is on the commandline. This makes slub debug messages more
> > meaningful and if by chance a kernel address is in some slub debug
> > object dump we will have a better chance of figuring out what went
> > wrong.
> >
> > Note that we don't use %px in the slub code because we want to reduce
> > the number of places that %px is used in the kernel. This also nicely
> > prints a big fat warning at kernel boot if slub_debug is on the
> > commandline so that we know that this kernel shouldn't be used on
> > production systems.
>
> Eeeek. I missed this patch. NAK NAK. People use slub_debug for
> production systems to gain redzoning, etc, as a layer of defense, and
> they absolutely do not want %p-hashing disabled. %p hashing is
> controlled by the no_hash_pointers boot param (since v5.12), and needs to stay
> separate from slub_debug.
>
> Can we please revert this in Linus's tree and in v5.14?
>
This is fine with me as long as debugging with slub_debug on the
commandline is possible. Would you prefer v1 of this patch series[1]
that uses the printk format to print unhashed pointers in slub debugging
messages?
[1] https://lore.kernel.org/r/[email protected]
Quoting Kees Cook (2021-09-20 11:28:50)
> On Mon, Sep 20, 2021 at 11:23:01AM -0700, Stephen Boyd wrote:
> > Quoting Kees Cook (2021-09-20 07:29:54)
> > > On Tue, Jun 01, 2021 at 11:22:02AM -0700, Stephen Boyd wrote:
> > > > Obscuring the pointers that slub shows when debugging makes for some
> > > > confusing slub debug messages:
> > > >
> > > > Padding overwritten. 0x0000000079f0674a-0x000000000d4dce17
> > > >
> > > > Those addresses are hashed for kernel security reasons. If we're trying
> > > > to be secure with slub_debug on the commandline we have some big
> > > > problems given that we dump whole chunks of kernel memory to the kernel
> > > > logs. Let's force on the no_hash_pointers commandline flag when
> > > > slub_debug is on the commandline. This makes slub debug messages more
> > > > meaningful and if by chance a kernel address is in some slub debug
> > > > object dump we will have a better chance of figuring out what went
> > > > wrong.
> > > >
> > > > Note that we don't use %px in the slub code because we want to reduce
> > > > the number of places that %px is used in the kernel. This also nicely
> > > > prints a big fat warning at kernel boot if slub_debug is on the
> > > > commandline so that we know that this kernel shouldn't be used on
> > > > production systems.
> > >
> > > Eeeek. I missed this patch. NAK NAK. People use slub_debug for
> > > production systems to gain redzoning, etc, as a layer of defense, and
> > > they absolutely do not want %p-hashing disabled. %p hashing is
> > > controlled by the no_hash_pointers boot param (since v5.12), and needs to stay
> > > separate from slub_debug.
> > >
> > > Can we please revert this in Linus's tree and in v5.14?
> > >
> >
> > This is fine with me as long as debugging with slub_debug on the
> > commandline is possible. Would you prefer v1 of this patch series[1]
> > that uses the printk format to print unhashed pointers in slub debugging
> > messages?
> >
> > [1] https://lore.kernel.org/r/[email protected]
>
> I'd like to keep %px use in the kernel minimized. Seeing full pointers (%p
> hashing disabled) can be done with the no_hash_pointers boot param, and
> that's used in other debug cases as well. I'd rather keep it a global
> knob.
>
Can you elaborate on your use case where slub debugging is used for
security in production systems via redzoning? Maybe that redzoning logic
in slub debug should be moved out of CONFIG_SLUB_DEBUG into slub proper?
Or maybe the config symbol should be changed to something that doesn't
have 'debug' in the name?
The main goal of this series was to make slub debug messages I saw
useful instead of confusing given that all the pointers were hashed. If
some part of slub debugging is production critical then I wonder why
it's behind a debug named config knob and why it prints so much pointer
information on production systems.