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

[PATCH v2 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 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

 lib/vsprintf.c |  2 +-
 mm/slub.c      | 15 ++++++++++++---
 2 files changed, 13 insertions(+), 4 deletions(-)


base-commit: d07f6ca923ea0927a1024dfccafc5b53b61cfecc
-- 
https://chromeos.dev

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

Cc: Vlastimil Babka <vbabka@suse.cz>
Fixes: e17f1dfba37b ("mm, slub: extend slub_debug syntax for multiple blocks")
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

Re: [External] [PATCH v2 1/4] slub: Restore slub_debug=- behavior

[Muchun Song]

On Wed, May 26, 2021 at 10:56 AM Stephen Boyd <swboyd@chromium.org> wrote:
>
> 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.
>
> Cc: Vlastimil Babka <vbabka@suse.cz>
> Fixes: e17f1dfba37b ("mm, slub: extend slub_debug syntax for multiple blocks")

Is it caused by the commit 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
>

Re: [External] [PATCH v2 1/4] slub: Restore slub_debug=- behavior

[Vlastimil Babka]

On 5/26/21 6:04 AM, Muchun Song wrote:
> On Wed, May 26, 2021 at 10:56 AM Stephen Boyd <swboyd@chromium.org> wrote:
>>
>> 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.
>>
>> Cc: Vlastimil Babka <vbabka@suse.cz>
>> Fixes: e17f1dfba37b ("mm, slub: extend slub_debug syntax for multiple blocks")

Acked-by: Vlastimil Babka <vbabka@suse.cz>

> 
> Is it caused by the commit ca0cab65ea2b ("mm, slub: introduce static
> key for slub_debug()")?

Yep, looks like a better Fixes: candidate.

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

Re: [External] [PATCH v2 1/4] slub: Restore slub_debug=- behavior

[Stephen Boyd]

Quoting Vlastimil Babka (2021-05-26 03:39:54)
> On 5/26/21 6:04 AM, Muchun Song wrote:
> > On Wed, May 26, 2021 at 10:56 AM Stephen Boyd <swboyd@chromium.org> wrote:
> >>
> >> 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.
> >>
> >> Cc: Vlastimil Babka <vbabka@suse.cz>
> >> Fixes: e17f1dfba37b ("mm, slub: extend slub_debug syntax for multiple blocks")
>
> Acked-by: Vlastimil Babka <vbabka@suse.cz>
>
> >
> > Is it caused by the commit ca0cab65ea2b ("mm, slub: introduce static
> > key for slub_debug()")?
>
> Yep, looks like a better Fixes: candidate.
>

Fixed it. Thanks.

Re: [External] [PATCH v2 1/4] slub: Restore slub_debug=- behavior

[Muchun Song]

On Thu, May 27, 2021 at 4:20 AM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Vlastimil Babka (2021-05-26 03:39:54)
> > On 5/26/21 6:04 AM, Muchun Song wrote:
> > > On Wed, May 26, 2021 at 10:56 AM Stephen Boyd <swboyd@chromium.org> wrote:
> > >>
> > >> 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.
> > >>
> > >> Cc: Vlastimil Babka <vbabka@suse.cz>
> > >> Fixes: e17f1dfba37b ("mm, slub: extend slub_debug syntax for multiple blocks")
> >
> > Acked-by: Vlastimil Babka <vbabka@suse.cz>
> >
> > >
> > > Is it caused by the commit ca0cab65ea2b ("mm, slub: introduce static
> > > key for slub_debug()")?
> >
> > Yep, looks like a better Fixes: candidate.
> >
>
> Fixed it. Thanks.

With that fix. Please feel free to add:

Reviewed-by: Muchun Song <songmuchun@bytedance.com>

Thanks.

[PATCH v2 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>
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

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

[Muchun Song]

On Wed, May 26, 2021 at 10:56 AM Stephen Boyd <swboyd@chromium.org> wrote:
>
> 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>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
>

More convenient. LGTM.

Reviewed-by: Muchun Song <songmuchun@bytedance.com>

[PATCH v2 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>
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 v2 3/4] slub: Indicate slab_fix() uses printf formats

[Vlastimil Babka]

On 5/26/21 4:56 AM, 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>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>

Acked-by: Vlastimil Babka <vbabka@suse.cz>

> ---
>  mm/slub.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/slub.c b/mm/slub.c
> index 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;
>  }
> 

[PATCH v2 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>
---

I opted for extern because I guess we don't want to advertise
no_hash_pointers_enable() in some sort of header file? It can be put in
a header file but I see that the no_hash_pointers variable is also not 
in a header file but exported as symbol.

 lib/vsprintf.c | 2 +-
 mm/slub.c      | 6 ++++++
 2 files changed, 7 insertions(+), 1 deletion(-)

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..1c30436d3e6c 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -4451,6 +4451,8 @@ static struct kmem_cache * __init bootstrap(struct kmem_cache *static_cache)
 	return s;
 }
 
+extern int no_hash_pointers_enable(char *str);
+
 void __init kmem_cache_init(void)
 {
 	static __initdata struct kmem_cache boot_kmem_cache,
@@ -4470,6 +4472,10 @@ void __init kmem_cache_init(void)
 	for_each_node_state(node, N_NORMAL_MEMORY)
 		node_set(node, slab_nodes);
 
+	/* Print slub debugging pointers without hashing */
+	if (static_branch_unlikely(&slub_debug_enabled))
+		no_hash_pointers_enable(NULL);
+
 	create_boot_cache(kmem_cache_node, "kmem_cache_node",
 		sizeof(struct kmem_cache_node), SLAB_HWCACHE_ALIGN, 0, 0);
 
-- 
https://chromeos.dev

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

[kernel test robot]

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

Hi Stephen,

I love your patch! Perhaps something to improve:

[auto build test WARNING on d07f6ca923ea0927a1024dfccafc5b53b61cfecc]

url:    https://github.com/0day-ci/linux/commits/Stephen-Boyd/slub-Print-non-hashed-pointers-in-slub-debugging/20210526-105816
base:   d07f6ca923ea0927a1024dfccafc5b53b61cfecc
config: i386-tinyconfig (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
        # https://github.com/0day-ci/linux/commit/1e3e0117436276faacd0217d89715df61021b4d2
        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/20210526-105816
        git checkout 1e3e0117436276faacd0217d89715df61021b4d2
        # save the attached .config to linux build tree
        make W=1 ARCH=i386 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   lib/vsprintf.c: In function 'va_format':
   lib/vsprintf.c:1663:2: warning: function 'va_format' might be a candidate for 'gnu_printf' format attribute [-Wsuggest-attribute=format]
    1663 |  buf += vsnprintf(buf, end > buf ? end - buf : 0, va_fmt->fmt, va);
         |  ^~~
   lib/vsprintf.c: At top level:
>> lib/vsprintf.c:2189:12: warning: no previous prototype for 'no_hash_pointers_enable' [-Wmissing-prototypes]
    2189 | int __init no_hash_pointers_enable(char *str)
         |            ^~~~~~~~~~~~~~~~~~~~~~~


vim +/no_hash_pointers_enable +2189 lib/vsprintf.c

  2188	
> 2189	int __init no_hash_pointers_enable(char *str)
  2190	{
  2191		if (no_hash_pointers)
  2192			return 0;
  2193	
  2194		no_hash_pointers = true;
  2195	
  2196		pr_warn("**********************************************************\n");
  2197		pr_warn("**   NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE   **\n");
  2198		pr_warn("**                                                      **\n");
  2199		pr_warn("** This system shows unhashed kernel memory addresses   **\n");
  2200		pr_warn("** via the console, logs, and other interfaces. This    **\n");
  2201		pr_warn("** might reduce the security of your system.            **\n");
  2202		pr_warn("**                                                      **\n");
  2203		pr_warn("** If you see this message and you are not debugging    **\n");
  2204		pr_warn("** the kernel, report this immediately to your system   **\n");
  2205		pr_warn("** administrator!                                       **\n");
  2206		pr_warn("**                                                      **\n");
  2207		pr_warn("**   NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE   **\n");
  2208		pr_warn("**********************************************************\n");
  2209	
  2210		return 0;
  2211	}
  2212	early_param("no_hash_pointers", no_hash_pointers_enable);
  2213	

---
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: 7413 bytes --]

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

[kernel test robot]

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

Hi Stephen,

I love your patch! Perhaps something to improve:

[auto build test WARNING on d07f6ca923ea0927a1024dfccafc5b53b61cfecc]

url:    https://github.com/0day-ci/linux/commits/Stephen-Boyd/slub-Print-non-hashed-pointers-in-slub-debugging/20210526-105816
base:   d07f6ca923ea0927a1024dfccafc5b53b61cfecc
config: powerpc-randconfig-r013-20210526 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 99155e913e9bad5f7f8a247f8bb3a3ff3da74af1)
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 powerpc cross compiling tool for clang build
        # apt-get install binutils-powerpc-linux-gnu
        # https://github.com/0day-ci/linux/commit/1e3e0117436276faacd0217d89715df61021b4d2
        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/20210526-105816
        git checkout 1e3e0117436276faacd0217d89715df61021b4d2
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=powerpc 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> lib/vsprintf.c:2189:12: warning: no previous prototype for function 'no_hash_pointers_enable' [-Wmissing-prototypes]
   int __init no_hash_pointers_enable(char *str)
              ^
   lib/vsprintf.c:2189:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   int __init no_hash_pointers_enable(char *str)
   ^
   static 
   1 warning generated.


vim +/no_hash_pointers_enable +2189 lib/vsprintf.c

  2188	
> 2189	int __init no_hash_pointers_enable(char *str)
  2190	{
  2191		if (no_hash_pointers)
  2192			return 0;
  2193	
  2194		no_hash_pointers = true;
  2195	
  2196		pr_warn("**********************************************************\n");
  2197		pr_warn("**   NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE   **\n");
  2198		pr_warn("**                                                      **\n");
  2199		pr_warn("** This system shows unhashed kernel memory addresses   **\n");
  2200		pr_warn("** via the console, logs, and other interfaces. This    **\n");
  2201		pr_warn("** might reduce the security of your system.            **\n");
  2202		pr_warn("**                                                      **\n");
  2203		pr_warn("** If you see this message and you are not debugging    **\n");
  2204		pr_warn("** the kernel, report this immediately to your system   **\n");
  2205		pr_warn("** administrator!                                       **\n");
  2206		pr_warn("**                                                      **\n");
  2207		pr_warn("**   NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE   **\n");
  2208		pr_warn("**********************************************************\n");
  2209	
  2210		return 0;
  2211	}
  2212	early_param("no_hash_pointers", no_hash_pointers_enable);
  2213	

---
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: 29915 bytes --]

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

[Vlastimil Babka]

On 5/26/21 4:56 AM, 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>
> ---
> 
> I opted for extern because I guess we don't want to advertise
> no_hash_pointers_enable() in some sort of header file? It can be put in
> a header file

Hm looks like the bots disagree. I suppose a declaration right above definition
in lib/vsprintf.c would silence them, but I'll leave it to printk maintainers if
they would prefer that way or traditionally
include/linux/kernel.h

> but I see that the no_hash_pointers variable is also not 
> in a header file but exported as symbol.

Yeah it's only used by tests, and a variable doesn't need separate declaration.

>  lib/vsprintf.c | 2 +-
>  mm/slub.c      | 6 ++++++
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> 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..1c30436d3e6c 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -4451,6 +4451,8 @@ static struct kmem_cache * __init bootstrap(struct kmem_cache *static_cache)
>  	return s;
>  }
>  
> +extern int no_hash_pointers_enable(char *str);
> +
>  void __init kmem_cache_init(void)
>  {
>  	static __initdata struct kmem_cache boot_kmem_cache,
> @@ -4470,6 +4472,10 @@ void __init kmem_cache_init(void)
>  	for_each_node_state(node, N_NORMAL_MEMORY)
>  		node_set(node, slab_nodes);
>  
> +	/* Print slub debugging pointers without hashing */
> +	if (static_branch_unlikely(&slub_debug_enabled))
> +		no_hash_pointers_enable(NULL);
> +
>  	create_boot_cache(kmem_cache_node, "kmem_cache_node",
>  		sizeof(struct kmem_cache_node), SLAB_HWCACHE_ALIGN, 0, 0);
>  
> 

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

[Petr Mladek]

On Wed 2021-05-26 12:48:47, Vlastimil Babka wrote:
> On 5/26/21 4:56 AM, Stephen Boyd wrote:
> > Obscuring the pointers that slub shows when debugging makes for some
> > confusing slub debug messages:
> > 
> >  Padding overwritten. 0x0000000079f0674a-0x000000000d4dce17
> > 
> > I opted for extern because I guess we don't want to advertise
> > no_hash_pointers_enable() in some sort of header file? It can be put in
> > a header file
> 
> Hm looks like the bots disagree. I suppose a declaration right above definition
> in lib/vsprintf.c would silence them, but I'll leave it to printk maintainers if
> they would prefer that way or traditionally
> include/linux/kernel.h

I slightly prefer to put it into kernel.h. I expect that some more
debugging facilities would want to enable this in the future.
But I would accept even the "ugly" declaration in vsprintf.c.

Best Regards,
Petr

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

[Stephen Boyd]

Quoting Petr Mladek (2021-05-26 06:47:23)
> On Wed 2021-05-26 12:48:47, Vlastimil Babka wrote:
> > On 5/26/21 4:56 AM, Stephen Boyd wrote:
> > > Obscuring the pointers that slub shows when debugging makes for some
> > > confusing slub debug messages:
> > >
> > >  Padding overwritten. 0x0000000079f0674a-0x000000000d4dce17
> > >
> > > I opted for extern because I guess we don't want to advertise
> > > no_hash_pointers_enable() in some sort of header file? It can be put in
> > > a header file
> >
> > Hm looks like the bots disagree. I suppose a declaration right above definition
> > in lib/vsprintf.c would silence them, but I'll leave it to printk maintainers if
> > they would prefer that way or traditionally
> > include/linux/kernel.h
>
> I slightly prefer to put it into kernel.h. I expect that some more
> debugging facilities would want to enable this in the future.
> But I would accept even the "ugly" declaration in vsprintf.c.

Ok no problem. Would printk.h be more appropriate?

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

[Petr Mladek]

On Wed 2021-05-26 15:27:37, Stephen Boyd wrote:
> Quoting Petr Mladek (2021-05-26 06:47:23)
> > On Wed 2021-05-26 12:48:47, Vlastimil Babka wrote:
> > > On 5/26/21 4:56 AM, Stephen Boyd wrote:
> > > > Obscuring the pointers that slub shows when debugging makes for some
> > > > confusing slub debug messages:
> > > >
> > > >  Padding overwritten. 0x0000000079f0674a-0x000000000d4dce17
> > > >
> > > > I opted for extern because I guess we don't want to advertise
> > > > no_hash_pointers_enable() in some sort of header file? It can be put in
> > > > a header file
> > >
> > > Hm looks like the bots disagree. I suppose a declaration right above definition
> > > in lib/vsprintf.c would silence them, but I'll leave it to printk maintainers if
> > > they would prefer that way or traditionally
> > > include/linux/kernel.h
> >
> > I slightly prefer to put it into kernel.h. I expect that some more
> > debugging facilities would want to enable this in the future.
> > But I would accept even the "ugly" declaration in vsprintf.c.
> 
> Ok no problem. Would printk.h be more appropriate?

kernel.h looks more appropriate to me. vsprintf-related are there and
no_hash_pointers is implemented and handled in vsprintf.c.

Best Regards,
Petr