slub: Print non-hashed pointers in slub debugging

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

Quoted 1 line

Quoted 1 line

mm/slub.c:4464:30: error: use of undeclared identifier 'slub_debug_enabled'

           if (static_branch_unlikely(&slub_debug_enabled))
                                       ^

Quoted 4 lines

Quoted 4 lines

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))
               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Quoted 4 lines

Quoted 2 lines

Quoted 2 lines

mm/slub.c:4464:30: error: use of undeclared identifier 'slub_debug_enabled'

           if (static_branch_unlikely(&slub_debug_enabled))
                                       ^

Quoted 6 lines

Quoted 4 lines

Quoted 4 lines

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?

mm/slub.c renamed from mm/slub.c~slub-force-on-no_hash_pointers-when-slub_debug-is-enabled-fix +1 -0

@@ -117,12 +117,26 @@
  */
 
 #ifdef CONFIG_SLUB_DEBUG
+

mm/slub.c renamed from mm/slub.c~slub-force-on-no_hash_pointers-when-slub_debug-is-enabled-fix +10 -0

+}
+
+#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)
 {

mm/slub.c renamed from mm/slub.c~slub-force-on-no_hash_pointers-when-slub_debug-is-enabled-fix +0 -0

                no_hash_pointers_enable(NULL);
 
        kmem_cache_node = &boot_kmem_cache_node;

Quoting Andrew Morton (2021-06-01 17:26:59)

Quoted 5 lines · On Wed, 2 Jun 2021 06:45:55 +0800 kernel test robot <[email protected]> wrote:

Quoted 4 lines

Quoted 2 lines

Quoted 2 lines

mm/slub.c:4464:30: error: use of undeclared identifier 'slub_debug_enabled'

           if (static_branch_unlikely(&slub_debug_enabled))
                                       ^

Quoted 8 lines

Quoted 6 lines

Quoted 4 lines

Quoted 4 lines

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!

Quoted 8 lines

mm/slub.c renamed from mm/slub.c~slub-force-on-no_hash_pointers-when-slub_debug-is-enabled-fix +1 -0

@@ -117,12 +117,26 @@
  */
 
 #ifdef CONFIG_SLUB_DEBUG
+

mm/slub.c line 129

Quoted 10 lines

mm/slub.c renamed from mm/slub.c~slub-force-on-no_hash_pointers-when-slub_debug-is-enabled-fix +4 -1

 #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);

Quoted 11 lines

mm/slub.c renamed from mm/slub.c~slub-force-on-no_hash_pointers-when-slub_debug-is-enabled-fix +10 -0

+}
+
+#else          /* CONFIG_SLUB_DEBUG */
+
+static inline bool __slub_debug_enabled(void)
+{
+       return false;
+}
+
+#endif         /* CONFIG_SLUB_DEBUG */
 

mm/slub.c void __init kmem_cache_init(void) · line 4510

Quoted 8 lines

mm/slub.c renamed from mm/slub.c~slub-force-on-no_hash_pointers-when-slub_debug-is-enabled-fix +1 -1

 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.

Quoted 3 lines

mm/slub.c renamed from mm/slub.c~slub-force-on-no_hash_pointers-when-slub_debug-is-enabled-fix +0 -0

                no_hash_pointers_enable(NULL);
 
        kmem_cache_node = &boot_kmem_cache_node;

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(-)

include/linux/kernel.h +2 -0

index 15d8bad3d2f2..bf950621febf 100644
@@ -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);

lib/vsprintf.c +1 -1

index f0c35d9b65bf..cc281f5895f9 100644
@@ -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;

mm/slub.c +4 -0

index bf4949115412..a722794f1dbd 100644
@@ -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;
 

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

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(-)

include/linux/kernel.h +2 -0

index 15d8bad3d2f2..bf950621febf 100644
@@ -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);

lib/vsprintf.c +1 -1

index f0c35d9b65bf..cc281f5895f9 100644
@@ -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;

mm/slub.c +4 -0

index bf4949115412..a722794f1dbd 100644
@@ -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;
 

Signature · 1 line

https://chromeos.dev

Quoted 14 lines

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.

Quoted 5 lines

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?

Quoting Kees Cook (2021-09-20 07:29:54)

Quoted 21 lines · Stephen Boyd

Quoted 14 lines

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.

Quoted 5 lines

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.

Quoted 8 lines

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]

Quoted 36 lines

Quoting Kees Cook (2021-09-20 07:29:54)

Quoted 21 lines · Stephen Boyd

Quoted 14 lines

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.

Quoted 5 lines

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.

Quoted 8 lines

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?

Quoted 1 line

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