| Message ID | 20210601182202.3011020-5-swboyd@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | slub: Print non-hashed pointers in slub debugging | expand |
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 <lkp@intel.com>
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/kbuild-all@lists.01.org
On Wed, 2 Jun 2021 06:45:55 +0800 kernel test robot <lkp@intel.com> 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 <lkp@intel.com> 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/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 <lkp@intel.com> 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 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 <swboyd@chromium.org> Acked-by: Vlastimil Babka <vbabka@suse.cz> > --- > 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 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 <swboyd@chromium.org> Acked-by: Petr Mladek <pmladek@suse.com> Best Regards, Petr
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 <swboyd@chromium.org> > --- > 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 >
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/20210520013539.3733631-1-swboyd@chromium.org
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/20210520013539.3733631-1-swboyd@chromium.org 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
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/20210520013539.3733631-1-swboyd@chromium.org > > 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.
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;
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 <swboyd@chromium.org> --- include/linux/kernel.h | 2 ++ lib/vsprintf.c | 2 +- mm/slub.c | 4 ++++ 3 files changed, 7 insertions(+), 1 deletion(-)