[PATCH v3 0/4] slub: Print non-hashed pointers in slub debugging

[PATCH v3 0/4] slub: Print non-hashed pointers in slub debugging

[Stephen Boyd]

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/20210526025625.601023-1-swboyd@chromium.org):
 * 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

[PATCH v3 1/4] slub: Restore slub_debug=- behavior

[Stephen Boyd]

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 <vbabka@suse.cz>
Reviewed-by: Muchun Song <songmuchun@bytedance.com>
Fixes: ca0cab65ea2b ("mm, slub: introduce static key for slub_debug()")
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 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

[PATCH v3 2/4] slub: Actually use 'message' in restore_bytes()

[Stephen Boyd]

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 <vbabka@suse.cz>
Acked-by: David Rientjes <rientjes@google.com>
Reviewed-by: Muchun Song <songmuchun@bytedance.com>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 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

[PATCH v3 3/4] slub: Indicate slab_fix() uses printf formats

[Stephen Boyd]

From: Joe Perches <joe@perches.com>

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 <joe@perches.com>
Acked-by: Vlastimil Babka <vbabka@suse.cz>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 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

Re: [PATCH v3 3/4] slub: Indicate slab_fix() uses printf formats

[David Rientjes]

On Tue, 1 Jun 2021, Stephen Boyd wrote:

> From: Joe Perches <joe@perches.com>
> 
> 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 <joe@perches.com>
> Acked-by: Vlastimil Babka <vbabka@suse.cz>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>

Acked-by: David Rientjes <rientjes@google.com>

Re: [PATCH v3 3/4] slub: Indicate slab_fix() uses printf formats

[David Rientjes]

On Tue, 1 Jun 2021, Stephen Boyd wrote:

> From: Joe Perches <joe@perches.com>
> 
> 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 <joe@perches.com>
> Acked-by: Vlastimil Babka <vbabka@suse.cz>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>

Acked-by: David Rientjes <rientjes@google.com>

[PATCH v3 4/4] slub: Force on no_hash_pointers when slub_debug is enabled

[Stephen Boyd]

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

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

Re: [PATCH v3 4/4] slub: Force on no_hash_pointers when slub_debug is enabled

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 9904 bytes --]

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 15782 bytes --]

Re: [PATCH v3 4/4] slub: Force on no_hash_pointers when slub_debug is enabled

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 10088 bytes --]

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(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 15782 bytes --]

Re: [PATCH v3 4/4] slub: Force on no_hash_pointers when slub_debug is enabled

[Andrew Morton]

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;
_

Re: [PATCH v3 4/4] slub: Force on no_hash_pointers when slub_debug is enabled

[Andrew Morton]

[-- Attachment #1: Type: text/plain, Size: 1701 bytes --]

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;
_

Re: [PATCH v3 4/4] slub: Force on no_hash_pointers when slub_debug is enabled

[Stephen Boyd]

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;

Re: [PATCH v3 4/4] slub: Force on no_hash_pointers when slub_debug is enabled

[Stephen Boyd]

[-- Attachment #1: Type: text/plain, Size: 2281 bytes --]

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;

Re: [PATCH v3 4/4] slub: Force on no_hash_pointers when slub_debug is enabled

[Stephen Boyd]

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;

Re: [PATCH v3 4/4] slub: Force on no_hash_pointers when slub_debug is enabled

[Vlastimil Babka]

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

Re: [PATCH v3 4/4] slub: Force on no_hash_pointers when slub_debug is enabled

[Vlastimil Babka]

[-- Attachment #1: Type: text/plain, Size: 2597 bytes --]

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

Re: [PATCH v3 4/4] slub: Force on no_hash_pointers when slub_debug is enabled

[Vlastimil Babka]

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

Re: [PATCH v3 4/4] slub: Force on no_hash_pointers when slub_debug is enabled

[Petr Mladek]

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

Re: [PATCH v3 4/4] slub: Force on no_hash_pointers when slub_debug is enabled

[Kees Cook]

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
> 

-- 
Kees Cook

Re: [PATCH v3 4/4] slub: Force on no_hash_pointers when slub_debug is enabled

[Stephen Boyd]

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

Re: [PATCH v3 4/4] slub: Force on no_hash_pointers when slub_debug is enabled

[Stephen Boyd]

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

Re: [PATCH v3 4/4] slub: Force on no_hash_pointers when slub_debug is enabled

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

-- 
Kees Cook

Re: [PATCH v3 4/4] slub: Force on no_hash_pointers when slub_debug is enabled

[Stephen Boyd]

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.

Re: [PATCH v3 4/4] slub: Force on no_hash_pointers when slub_debug is enabled

[Stephen Boyd]

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.