linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] mm/slub: initialize stack depot in boot process
@ 2022-03-01  3:36 Hyeonggon Yoo
  2022-03-01  3:53 ` Hyeonggon Yoo
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Hyeonggon Yoo @ 2022-03-01  3:36 UTC (permalink / raw)
  To: Vlastimil Babka
  Cc: David Rientjes, Christoph Lameter, Joonsoo Kim, Pekka Enberg,
	Roman Gushchin, Andrew Morton, linux-mm, patches, linux-kernel,
	Oliver Glitta, Faiyaz Mohammed, Dmitry Vyukov, Eric Dumazet,
	Jarkko Sakkinen, Johannes Berg, Yury Norov, Arnd Bergmann,
	James Bottomley, Matteo Croce, Marco Elver, Andrey Konovalov,
	Imran Khan, Zqiang

commit ba10d4b46655 ("mm/slub: use stackdepot to save stack trace in
objects") initializes stack depot in cache creation if SLAB_STORE_USER
flag is set.

This can make kernel crash because a cache can be crashed in various
contexts. For example if user sets slub_debug=U, kernel crashes
because create_boot_cache() calls stack_depot_init(), which tries to
allocate hash table using memblock_alloc() if slab is not available.
But memblock is also not available at that time.

This patch solves the problem by initializing stack depot early
in boot process if SLAB_STORE_USER debug flag is set globally
or the flag is set for at least one cache.

[ elver@google.com: initialize stack depot depending on slub_debug
  parameter instead of allowing stack_depot_init() to be called
  during kmem_cache_init() for simplicity. ]

[ vbabka@suse.cz: parse slub_debug parameter in setup_slub_debug()
  and initialize stack depot in stack_depot_early_init(). ]

Link: https://lore.kernel.org/all/YhyeaP8lrzKgKm5A@ip-172-31-19-208.ap-northeast-1.compute.internal/
Fixes: ba10d4b46655 ("mm/slub: use stackdepot to save stack trace in objects")
Signed-off-by: Hyeonggon Yoo <42.hyeyoo@gmail.com>
---
 include/linux/slab.h       |  6 ++++++
 include/linux/stackdepot.h |  3 ++-
 mm/slub.c                  | 18 +++++++++++++++---
 3 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/include/linux/slab.h b/include/linux/slab.h
index 37bde99b74af..062128e0db10 100644
--- a/include/linux/slab.h
+++ b/include/linux/slab.h
@@ -763,6 +763,12 @@ extern void kvfree_sensitive(const void *addr, size_t len);
 unsigned int kmem_cache_size(struct kmem_cache *s);
 void __init kmem_cache_init_late(void);
 
+#if defined(CONFIG_SLUB_DEBUG) && defined(CONFIG_STACKDEPOT)
+int slab_stack_depot_init(void);
+#else
+int slab_stack_depot_init(void) { return 0; }
+#endif
+
 #if defined(CONFIG_SMP) && defined(CONFIG_SLAB)
 int slab_prepare_cpu(unsigned int cpu);
 int slab_dead_cpu(unsigned int cpu);
diff --git a/include/linux/stackdepot.h b/include/linux/stackdepot.h
index 17f992fe6355..a813a2673c48 100644
--- a/include/linux/stackdepot.h
+++ b/include/linux/stackdepot.h
@@ -12,6 +12,7 @@
 #define _LINUX_STACKDEPOT_H
 
 #include <linux/gfp.h>
+#include <linux/slab.h>
 
 typedef u32 depot_stack_handle_t;
 
@@ -32,7 +33,7 @@ int stack_depot_init(void);
 #ifdef CONFIG_STACKDEPOT_ALWAYS_INIT
 static inline int stack_depot_early_init(void)	{ return stack_depot_init(); }
 #else
-static inline int stack_depot_early_init(void)	{ return 0; }
+static inline int stack_depot_early_init(void)	{ return slab_stack_depot_init(); }
 #endif
 
 depot_stack_handle_t stack_depot_save(unsigned long *entries,
diff --git a/mm/slub.c b/mm/slub.c
index a74afe59a403..2419fc3cc9f3 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -646,6 +646,14 @@ static slab_flags_t slub_debug;
 
 static char *slub_debug_string;
 static int disable_higher_order_debug;
+static bool init_stack_depot;
+
+int slab_stack_depot_init(void)
+{
+	if (init_stack_depot)
+		stack_depot_init();
+	return 0;
+}
 
 /*
  * slub is about to manipulate internal object metadata.  This memory lies
@@ -1531,6 +1539,8 @@ static int __init setup_slub_debug(char *str)
 			global_slub_debug_changed = true;
 		} else {
 			slab_list_specified = true;
+			if (flags & SLAB_STORE_USER)
+				init_stack_depot = true;
 		}
 	}
 
@@ -1546,6 +1556,10 @@ static int __init setup_slub_debug(char *str)
 			global_flags = slub_debug;
 		slub_debug_string = saved_str;
 	}
+
+	if (global_flags & SLAB_STORE_USER)
+		init_stack_depot = true;
+
 out:
 	slub_debug = global_flags;
 	if (slub_debug != 0 || slub_debug_string)
@@ -1556,6 +1570,7 @@ static int __init setup_slub_debug(char *str)
 	     static_branch_unlikely(&init_on_free)) &&
 	    (slub_debug & SLAB_POISON))
 		pr_info("mem auto-init: SLAB_POISON will take precedence over init_on_alloc/init_on_free\n");
+
 	return 1;
 }
 
@@ -4221,9 +4236,6 @@ static int kmem_cache_open(struct kmem_cache *s, slab_flags_t flags)
 	s->remote_node_defrag_ratio = 1000;
 #endif
 
-	if (s->flags & SLAB_STORE_USER && IS_ENABLED(CONFIG_STACKDEPOT))
-		stack_depot_init();
-
 	/* Initialize the pre-computed randomized freelist if slab is up */
 	if (slab_state >= UP) {
 		if (init_cache_random_seq(s))
-- 
2.33.1

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH v2] mm/slub: initialize stack depot in boot process
  2022-03-01  3:36 [PATCH v2] mm/slub: initialize stack depot in boot process Hyeonggon Yoo
@ 2022-03-01  3:53 ` Hyeonggon Yoo
  2022-03-01  7:00 ` kernel test robot
  2022-03-01  7:11 ` kernel test robot
  2 siblings, 0 replies; 7+ messages in thread
From: Hyeonggon Yoo @ 2022-03-01  3:53 UTC (permalink / raw)
  To: Vlastimil Babka
  Cc: David Rientjes, Christoph Lameter, Joonsoo Kim, Pekka Enberg,
	Roman Gushchin, Andrew Morton, linux-mm, patches, linux-kernel,
	Oliver Glitta, Faiyaz Mohammed, Dmitry Vyukov, Eric Dumazet,
	Jarkko Sakkinen, Johannes Berg, Yury Norov, Arnd Bergmann,
	James Bottomley, Matteo Croce, Marco Elver, Andrey Konovalov,
	Imran Khan, Zqiang

On Tue, Mar 01, 2022 at 03:36:38AM +0000, Hyeonggon Yoo wrote:
> commit ba10d4b46655 ("mm/slub: use stackdepot to save stack trace in
> objects") initializes stack depot in cache creation if SLAB_STORE_USER
> flag is set.
> 
> This can make kernel crash because a cache can be crashed in various
> contexts. For example if user sets slub_debug=U, kernel crashes
> because create_boot_cache() calls stack_depot_init(), which tries to
> allocate hash table using memblock_alloc() if slab is not available.
> But memblock is also not available at that time.
> 
> This patch solves the problem by initializing stack depot early
> in boot process if SLAB_STORE_USER debug flag is set globally
> or the flag is set for at least one cache.
> 

Hello Vlastimil, would you pick this up into slub-stackdepot-v1,
or fold it into original patch (2/5)?

Thanks!

-- 
Thank you, You are awesome!
Hyeonggon :-)

> [ elver@google.com: initialize stack depot depending on slub_debug
>   parameter instead of allowing stack_depot_init() to be called
>   during kmem_cache_init() for simplicity. ]
> 
> [ vbabka@suse.cz: parse slub_debug parameter in setup_slub_debug()
>   and initialize stack depot in stack_depot_early_init(). ]
> 
> Link: https://lore.kernel.org/all/YhyeaP8lrzKgKm5A@ip-172-31-19-208.ap-northeast-1.compute.internal/
> Fixes: ba10d4b46655 ("mm/slub: use stackdepot to save stack trace in objects")
> Signed-off-by: Hyeonggon Yoo <42.hyeyoo@gmail.com>
> ---
>  include/linux/slab.h       |  6 ++++++
>  include/linux/stackdepot.h |  3 ++-
>  mm/slub.c                  | 18 +++++++++++++++---
>  3 files changed, 23 insertions(+), 4 deletions(-)
> 
> diff --git a/include/linux/slab.h b/include/linux/slab.h
> index 37bde99b74af..062128e0db10 100644
> --- a/include/linux/slab.h
> +++ b/include/linux/slab.h
> @@ -763,6 +763,12 @@ extern void kvfree_sensitive(const void *addr, size_t len);
>  unsigned int kmem_cache_size(struct kmem_cache *s);
>  void __init kmem_cache_init_late(void);
>  
> +#if defined(CONFIG_SLUB_DEBUG) && defined(CONFIG_STACKDEPOT)
> +int slab_stack_depot_init(void);
> +#else
> +int slab_stack_depot_init(void) { return 0; }
> +#endif
> +
>  #if defined(CONFIG_SMP) && defined(CONFIG_SLAB)
>  int slab_prepare_cpu(unsigned int cpu);
>  int slab_dead_cpu(unsigned int cpu);
> diff --git a/include/linux/stackdepot.h b/include/linux/stackdepot.h
> index 17f992fe6355..a813a2673c48 100644
> --- a/include/linux/stackdepot.h
> +++ b/include/linux/stackdepot.h
> @@ -12,6 +12,7 @@
>  #define _LINUX_STACKDEPOT_H
>  
>  #include <linux/gfp.h>
> +#include <linux/slab.h>
>  
>  typedef u32 depot_stack_handle_t;
>  
> @@ -32,7 +33,7 @@ int stack_depot_init(void);
>  #ifdef CONFIG_STACKDEPOT_ALWAYS_INIT
>  static inline int stack_depot_early_init(void)	{ return stack_depot_init(); }
>  #else
> -static inline int stack_depot_early_init(void)	{ return 0; }
> +static inline int stack_depot_early_init(void)	{ return slab_stack_depot_init(); }
>  #endif
>  
>  depot_stack_handle_t stack_depot_save(unsigned long *entries,
> diff --git a/mm/slub.c b/mm/slub.c
> index a74afe59a403..2419fc3cc9f3 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -646,6 +646,14 @@ static slab_flags_t slub_debug;
>  
>  static char *slub_debug_string;
>  static int disable_higher_order_debug;
> +static bool init_stack_depot;
> +
> +int slab_stack_depot_init(void)
> +{
> +	if (init_stack_depot)
> +		stack_depot_init();
> +	return 0;
> +}
>  
>  /*
>   * slub is about to manipulate internal object metadata.  This memory lies
> @@ -1531,6 +1539,8 @@ static int __init setup_slub_debug(char *str)
>  			global_slub_debug_changed = true;
>  		} else {
>  			slab_list_specified = true;
> +			if (flags & SLAB_STORE_USER)
> +				init_stack_depot = true;
>  		}
>  	}
>  
> @@ -1546,6 +1556,10 @@ static int __init setup_slub_debug(char *str)
>  			global_flags = slub_debug;
>  		slub_debug_string = saved_str;
>  	}
> +
> +	if (global_flags & SLAB_STORE_USER)
> +		init_stack_depot = true;
> +
>  out:
>  	slub_debug = global_flags;
>  	if (slub_debug != 0 || slub_debug_string)
> @@ -1556,6 +1570,7 @@ static int __init setup_slub_debug(char *str)
>  	     static_branch_unlikely(&init_on_free)) &&
>  	    (slub_debug & SLAB_POISON))
>  		pr_info("mem auto-init: SLAB_POISON will take precedence over init_on_alloc/init_on_free\n");
> +
>  	return 1;
>  }
>  
> @@ -4221,9 +4236,6 @@ static int kmem_cache_open(struct kmem_cache *s, slab_flags_t flags)
>  	s->remote_node_defrag_ratio = 1000;
>  #endif
>  
> -	if (s->flags & SLAB_STORE_USER && IS_ENABLED(CONFIG_STACKDEPOT))
> -		stack_depot_init();
> -
>  	/* Initialize the pre-computed randomized freelist if slab is up */
>  	if (slab_state >= UP) {
>  		if (init_cache_random_seq(s))
> -- 
> 2.33.1

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2] mm/slub: initialize stack depot in boot process
  2022-03-01  3:36 [PATCH v2] mm/slub: initialize stack depot in boot process Hyeonggon Yoo
  2022-03-01  3:53 ` Hyeonggon Yoo
@ 2022-03-01  7:00 ` kernel test robot
  2022-03-01  7:11 ` kernel test robot
  2 siblings, 0 replies; 7+ messages in thread
From: kernel test robot @ 2022-03-01  7:00 UTC (permalink / raw)
  To: Hyeonggon Yoo, Vlastimil Babka
  Cc: kbuild-all, David Rientjes, Christoph Lameter, Joonsoo Kim,
	Pekka Enberg, Roman Gushchin, Andrew Morton,
	Linux Memory Management List, patches, linux-kernel,
	Oliver Glitta, Faiyaz Mohammed, Dmitry Vyukov, Eric Dumazet,
	Jarkko Sakkinen, Johannes Berg, Yury Norov, Arnd Bergmann,
	James Bottomley, Matteo Croce, Marco Elver, Andrey Konovalov,
	Imran Khan, Zqiang

Hi Hyeonggon,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on vbabka/slub-stackdepot-v1]
[also build test ERROR on linus/master v5.17-rc6 next-20220228]
[cannot apply to hnaz-mm/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Hyeonggon-Yoo/mm-slub-initialize-stack-depot-in-boot-process/20220301-113825
base:   https://git.kernel.org/pub/scm/linux/kernel/git/vbabka/linux.git slub-stackdepot-v1
config: m68k-randconfig-r014-20220301 (https://download.01.org/0day-ci/archive/20220301/202203011429.a87fKdU7-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 11.2.0
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
        # https://github.com/0day-ci/linux/commit/dd9dbeec7444b13b510dc4a863e9593d1799f965
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Hyeonggon-Yoo/mm-slub-initialize-stack-depot-in-boot-process/20220301-113825
        git checkout dd9dbeec7444b13b510dc4a863e9593d1799f965
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=m68k SHELL=/bin/bash arch/m68k/coldfire/ mm/

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

All error/warnings (new ones prefixed by >>):

   In file included from include/linux/irq.h:21,
                    from include/asm-generic/hardirq.h:17,
                    from ./arch/m68k/include/generated/asm/hardirq.h:1,
                    from include/linux/hardirq.h:11,
                    from include/linux/interrupt.h:11,
                    from include/linux/kernel_stat.h:9,
                    from arch/m68k/kernel/asm-offsets.c:16:
>> include/linux/slab.h:769:5: warning: no previous prototype for 'slab_stack_depot_init' [-Wmissing-prototypes]
     769 | int slab_stack_depot_init(void) { return 0; }
         |     ^~~~~~~~~~~~~~~~~~~~~
--
   In file included from include/linux/irq.h:21,
                    from arch/m68k/coldfire/vectors.c:14:
>> include/linux/slab.h:769:5: warning: no previous prototype for 'slab_stack_depot_init' [-Wmissing-prototypes]
     769 | int slab_stack_depot_init(void) { return 0; }
         |     ^~~~~~~~~~~~~~~~~~~~~
   arch/m68k/coldfire/vectors.c:43:13: warning: no previous prototype for 'trap_init' [-Wmissing-prototypes]
      43 | void __init trap_init(void)
         |             ^~~~~~~~~
--
   In file included from include/linux/irq.h:21,
                    from include/asm-generic/hardirq.h:17,
                    from ./arch/m68k/include/generated/asm/hardirq.h:1,
                    from include/linux/hardirq.h:11,
                    from include/linux/interrupt.h:11,
                    from arch/m68k/coldfire/intc-simr.c:16:
>> include/linux/slab.h:769:5: warning: no previous prototype for 'slab_stack_depot_init' [-Wmissing-prototypes]
     769 | int slab_stack_depot_init(void) { return 0; }
         |     ^~~~~~~~~~~~~~~~~~~~~
   arch/m68k/coldfire/intc-simr.c:177:13: warning: no previous prototype for 'init_IRQ' [-Wmissing-prototypes]
     177 | void __init init_IRQ(void)
         |             ^~~~~~~~
--
   In file included from include/linux/irq.h:21,
                    from include/linux/gpio/driver.h:7,
                    from arch/m68k/coldfire/gpio.c:12:
>> include/linux/slab.h:769:5: warning: no previous prototype for 'slab_stack_depot_init' [-Wmissing-prototypes]
     769 | int slab_stack_depot_init(void) { return 0; }
         |     ^~~~~~~~~~~~~~~~~~~~~
   arch/m68k/coldfire/gpio.c:19:5: warning: no previous prototype for '__mcfgpio_get_value' [-Wmissing-prototypes]
      19 | int __mcfgpio_get_value(unsigned gpio)
         |     ^~~~~~~~~~~~~~~~~~~
   arch/m68k/coldfire/gpio.c:25:6: warning: no previous prototype for '__mcfgpio_set_value' [-Wmissing-prototypes]
      25 | void __mcfgpio_set_value(unsigned gpio, int value)
         |      ^~~~~~~~~~~~~~~~~~~
   arch/m68k/coldfire/gpio.c:50:5: warning: no previous prototype for '__mcfgpio_direction_input' [-Wmissing-prototypes]
      50 | int __mcfgpio_direction_input(unsigned gpio)
         |     ^~~~~~~~~~~~~~~~~~~~~~~~~
   arch/m68k/coldfire/gpio.c:65:5: warning: no previous prototype for '__mcfgpio_direction_output' [-Wmissing-prototypes]
      65 | int __mcfgpio_direction_output(unsigned gpio, int value)
         |     ^~~~~~~~~~~~~~~~~~~~~~~~~~
   arch/m68k/coldfire/gpio.c:96:5: warning: no previous prototype for '__mcfgpio_request' [-Wmissing-prototypes]
      96 | int __mcfgpio_request(unsigned gpio)
         |     ^~~~~~~~~~~~~~~~~
   arch/m68k/coldfire/gpio.c:102:6: warning: no previous prototype for '__mcfgpio_free' [-Wmissing-prototypes]
     102 | void __mcfgpio_free(unsigned gpio)
         |      ^~~~~~~~~~~~~~
--
   In file included from include/linux/stackdepot.h:15,
                    from include/linux/page_ext.h:7,
                    from include/linux/mm.h:25,
                    from mm/page_alloc.c:19:
>> include/linux/slab.h:769:5: warning: no previous prototype for 'slab_stack_depot_init' [-Wmissing-prototypes]
     769 | int slab_stack_depot_init(void) { return 0; }
         |     ^~~~~~~~~~~~~~~~~~~~~
   mm/page_alloc.c:3820:15: warning: no previous prototype for 'should_fail_alloc_page' [-Wmissing-prototypes]
    3820 | noinline bool should_fail_alloc_page(gfp_t gfp_mask, unsigned int order)
         |               ^~~~~~~~~~~~~~~~~~~~~~
--
   In file included from include/linux/stackdepot.h:15,
                    from include/linux/page_ext.h:7,
                    from include/linux/mm.h:25,
                    from mm/page_poison.c:4:
>> include/linux/slab.h:769:5: warning: no previous prototype for 'slab_stack_depot_init' [-Wmissing-prototypes]
     769 | int slab_stack_depot_init(void) { return 0; }
         |     ^~~~~~~~~~~~~~~~~~~~~
   mm/page_poison.c:102:6: warning: no previous prototype for '__kernel_map_pages' [-Wmissing-prototypes]
     102 | void __kernel_map_pages(struct page *page, int numpages, int enable)
         |      ^~~~~~~~~~~~~~~~~~
--
   In file included from include/linux/stackdepot.h:15,
                    from include/linux/page_ext.h:7,
                    from include/linux/mm.h:25,
                    from mm/slub.c:13:
>> include/linux/slab.h:769:5: warning: no previous prototype for 'slab_stack_depot_init' [-Wmissing-prototypes]
     769 | int slab_stack_depot_init(void) { return 0; }
         |     ^~~~~~~~~~~~~~~~~~~~~
>> mm/slub.c:651:5: error: redefinition of 'slab_stack_depot_init'
     651 | int slab_stack_depot_init(void)
         |     ^~~~~~~~~~~~~~~~~~~~~
   In file included from include/linux/stackdepot.h:15,
                    from include/linux/page_ext.h:7,
                    from include/linux/mm.h:25,
                    from mm/slub.c:13:
   include/linux/slab.h:769:5: note: previous definition of 'slab_stack_depot_init' with type 'int(void)'
     769 | int slab_stack_depot_init(void) { return 0; }
         |     ^~~~~~~~~~~~~~~~~~~~~
--
   In file included from include/linux/irq.h:21,
                    from include/asm-generic/hardirq.h:17,
                    from ./arch/m68k/include/generated/asm/hardirq.h:1,
                    from include/linux/hardirq.h:11,
                    from include/linux/interrupt.h:11,
                    from include/linux/kernel_stat.h:9,
                    from arch/m68k/kernel/asm-offsets.c:16:
>> include/linux/slab.h:769:5: warning: no previous prototype for 'slab_stack_depot_init' [-Wmissing-prototypes]
     769 | int slab_stack_depot_init(void) { return 0; }
         |     ^~~~~~~~~~~~~~~~~~~~~


vim +/slab_stack_depot_init +651 mm/slub.c

   650	
 > 651	int slab_stack_depot_init(void)
   652	{
   653		if (init_stack_depot)
   654			stack_depot_init();
   655		return 0;
   656	}
   657	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2] mm/slub: initialize stack depot in boot process
  2022-03-01  3:36 [PATCH v2] mm/slub: initialize stack depot in boot process Hyeonggon Yoo
  2022-03-01  3:53 ` Hyeonggon Yoo
  2022-03-01  7:00 ` kernel test robot
@ 2022-03-01  7:11 ` kernel test robot
  2022-03-01  8:51   ` [PATCH v3] " Hyeonggon Yoo
  2 siblings, 1 reply; 7+ messages in thread
From: kernel test robot @ 2022-03-01  7:11 UTC (permalink / raw)
  To: Hyeonggon Yoo, Vlastimil Babka
  Cc: kbuild-all, David Rientjes, Christoph Lameter, Joonsoo Kim,
	Pekka Enberg, Roman Gushchin, Andrew Morton,
	Linux Memory Management List, patches, linux-kernel,
	Oliver Glitta, Faiyaz Mohammed, Dmitry Vyukov, Eric Dumazet,
	Jarkko Sakkinen, Johannes Berg, Yury Norov, Arnd Bergmann,
	James Bottomley, Matteo Croce, Marco Elver, Andrey Konovalov,
	Imran Khan, Zqiang

Hi Hyeonggon,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on vbabka/slub-stackdepot-v1]
[also build test ERROR on linus/master v5.17-rc6 next-20220228]
[cannot apply to hnaz-mm/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Hyeonggon-Yoo/mm-slub-initialize-stack-depot-in-boot-process/20220301-113825
base:   https://git.kernel.org/pub/scm/linux/kernel/git/vbabka/linux.git slub-stackdepot-v1
config: i386-tinyconfig (https://download.01.org/0day-ci/archive/20220301/202203011512.U0o5cAx4-lkp@intel.com/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/dd9dbeec7444b13b510dc4a863e9593d1799f965
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Hyeonggon-Yoo/mm-slub-initialize-stack-depot-in-boot-process/20220301-113825
        git checkout dd9dbeec7444b13b510dc4a863e9593d1799f965
        # save the config file to linux build tree
        mkdir build_dir
        make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash

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

   ld: arch/x86/kernel/ebda.o: in function `slab_stack_depot_init':
>> ebda.c:(.text+0x0): multiple definition of `slab_stack_depot_init'; arch/x86/kernel/head32.o:head32.c:(.text+0x0): first defined here
   ld: init/main.o: in function `slab_stack_depot_init':
   main.c:(.text+0x32): multiple definition of `slab_stack_depot_init'; arch/x86/kernel/head32.o:head32.c:(.text+0x0): first defined here
   ld: init/do_mounts.o: in function `slab_stack_depot_init':
   do_mounts.c:(.text+0x5): multiple definition of `slab_stack_depot_init'; arch/x86/kernel/head32.o:head32.c:(.text+0x0): first defined here
   ld: init/noinitramfs.o: in function `slab_stack_depot_init':
   noinitramfs.c:(.text+0x0): multiple definition of `slab_stack_depot_init'; arch/x86/kernel/head32.o:head32.c:(.text+0x0): first defined here
   ld: init/init_task.o: in function `slab_stack_depot_init':
   init_task.c:(.text+0x0): multiple definition of `slab_stack_depot_init'; arch/x86/kernel/head32.o:head32.c:(.text+0x0): first defined here
   ld: arch/x86/entry/syscall_32.o: in function `slab_stack_depot_init':
   syscall_32.c:(.text+0x0): multiple definition of `slab_stack_depot_init'; arch/x86/kernel/head32.o:head32.c:(.text+0x0): first defined here
   ld: arch/x86/entry/common.o: in function `slab_stack_depot_init':
   common.c:(.text+0x0): multiple definition of `slab_stack_depot_init'; arch/x86/kernel/head32.o:head32.c:(.text+0x0): first defined here
   ld: arch/x86/entry/vdso/vma.o: in function `slab_stack_depot_init':
   vma.c:(.text+0x1d0): multiple definition of `slab_stack_depot_init'; arch/x86/kernel/head32.o:head32.c:(.text+0x0): first defined here
   ld: arch/x86/entry/vdso/extable.o: in function `slab_stack_depot_init':
   extable.c:(.text+0x0): multiple definition of `slab_stack_depot_init'; arch/x86/kernel/head32.o:head32.c:(.text+0x0): first defined here
   ld: arch/x86/events/core.o: in function `slab_stack_depot_init':
   core.c:(.text+0x903): multiple definition of `slab_stack_depot_init'; arch/x86/kernel/head32.o:head32.c:(.text+0x0): first defined here
   ld: arch/x86/events/amd/core.o: in function `slab_stack_depot_init':
   core.c:(.text+0x70b): multiple definition of `slab_stack_depot_init'; arch/x86/kernel/head32.o:head32.c:(.text+0x0): first defined here
   ld: arch/x86/events/intel/core.o: in function `slab_stack_depot_init':
   core.c:(.text+0x1cbc): multiple definition of `slab_stack_depot_init'; arch/x86/kernel/head32.o:head32.c:(.text+0x0): first defined here
   ld: arch/x86/events/intel/bts.o: in function `slab_stack_depot_init':
   bts.c:(.text+0x6b2): multiple definition of `slab_stack_depot_init'; arch/x86/kernel/head32.o:head32.c:(.text+0x0): first defined here
   ld: arch/x86/events/intel/ds.o: in function `slab_stack_depot_init':
   ds.c:(.text+0x14ad): multiple definition of `slab_stack_depot_init'; arch/x86/kernel/head32.o:head32.c:(.text+0x0): first defined here
   ld: arch/x86/events/intel/knc.o: in function `slab_stack_depot_init':
   knc.c:(.text+0x2b1): multiple definition of `slab_stack_depot_init'; arch/x86/kernel/head32.o:head32.c:(.text+0x0): first defined here
   ld: arch/x86/events/intel/lbr.o: in function `slab_stack_depot_init':
   lbr.c:(.text+0x6c8): multiple definition of `slab_stack_depot_init'; arch/x86/kernel/head32.o:head32.c:(.text+0x0): first defined here
   ld: arch/x86/events/intel/p4.o: in function `slab_stack_depot_init':
   p4.c:(.text+0x5f8): multiple definition of `slab_stack_depot_init'; arch/x86/kernel/head32.o:head32.c:(.text+0x0): first defined here
   ld: arch/x86/events/intel/p6.o: in function `slab_stack_depot_init':
   p6.c:(.text+0xc4): multiple definition of `slab_stack_depot_init'; arch/x86/kernel/head32.o:head32.c:(.text+0x0): first defined here
   ld: arch/x86/events/intel/pt.o: in function `slab_stack_depot_init':
   pt.c:(.text+0x641): multiple definition of `slab_stack_depot_init'; arch/x86/kernel/head32.o:head32.c:(.text+0x0): first defined here
   ld: arch/x86/events/zhaoxin/core.o: in function `slab_stack_depot_init':
   core.c:(.text+0x460): multiple definition of `slab_stack_depot_init'; arch/x86/kernel/head32.o:head32.c:(.text+0x0): first defined here
   ld: arch/x86/realmode/init.o: in function `slab_stack_depot_init':
   init.c:(.text+0x0): multiple definition of `slab_stack_depot_init'; arch/x86/kernel/head32.o:head32.c:(.text+0x0): first defined here
   ld: arch/x86/kernel/process_32.o: in function `slab_stack_depot_init':
   process_32.c:(.text+0x0): multiple definition of `slab_stack_depot_init'; arch/x86/kernel/head32.o:head32.c:(.text+0x0): first defined here
   ld: arch/x86/kernel/signal.o: in function `slab_stack_depot_init':
   signal.c:(.text+0x5b4): multiple definition of `slab_stack_depot_init'; arch/x86/kernel/head32.o:head32.c:(.text+0x0): first defined here
   ld: arch/x86/kernel/traps.o: in function `slab_stack_depot_init':
   traps.c:(.text+0x288): multiple definition of `slab_stack_depot_init'; arch/x86/kernel/head32.o:head32.c:(.text+0x0): first defined here
   ld: arch/x86/kernel/idt.o: in function `slab_stack_depot_init':
   idt.c:(.text+0x0): multiple definition of `slab_stack_depot_init'; arch/x86/kernel/head32.o:head32.c:(.text+0x0): first defined here
   ld: arch/x86/kernel/irq.o: in function `slab_stack_depot_init':
   irq.c:(.text+0x4f): multiple definition of `slab_stack_depot_init'; arch/x86/kernel/head32.o:head32.c:(.text+0x0): first defined here
   ld: arch/x86/kernel/irq_32.o: in function `slab_stack_depot_init':
   irq_32.c:(.text+0x1): multiple definition of `slab_stack_depot_init'; arch/x86/kernel/head32.o:head32.c:(.text+0x0): first defined here
   ld: arch/x86/kernel/dumpstack_32.o: in function `slab_stack_depot_init':
   dumpstack_32.c:(.text+0x0): multiple definition of `slab_stack_depot_init'; arch/x86/kernel/head32.o:head32.c:(.text+0x0): first defined here
   ld: arch/x86/kernel/time.o: in function `slab_stack_depot_init':
   time.c:(.text+0xd): multiple definition of `slab_stack_depot_init'; arch/x86/kernel/head32.o:head32.c:(.text+0x0): first defined here
   ld: arch/x86/kernel/ioport.o: in function `slab_stack_depot_init':
   ioport.c:(.text+0x0): multiple definition of `slab_stack_depot_init'; arch/x86/kernel/head32.o:head32.c:(.text+0x0): first defined here
   ld: arch/x86/kernel/dumpstack.o: in function `slab_stack_depot_init':
   dumpstack.c:(.text+0x11e): multiple definition of `slab_stack_depot_init'; arch/x86/kernel/head32.o:head32.c:(.text+0x0): first defined here
   ld: arch/x86/kernel/nmi.o: in function `slab_stack_depot_init':
   nmi.c:(.text+0x74): multiple definition of `slab_stack_depot_init'; arch/x86/kernel/head32.o:head32.c:(.text+0x0): first defined here
   ld: arch/x86/kernel/setup.o: in function `slab_stack_depot_init':
   setup.c:(.text+0x3): multiple definition of `slab_stack_depot_init'; arch/x86/kernel/head32.o:head32.c:(.text+0x0): first defined here
   ld: arch/x86/kernel/x86_init.o: in function `slab_stack_depot_init':
   x86_init.c:(.text+0x39): multiple definition of `slab_stack_depot_init'; arch/x86/kernel/head32.o:head32.c:(.text+0x0): first defined here
   ld: arch/x86/kernel/i8259.o: in function `slab_stack_depot_init':
   i8259.c:(.text+0x2c4): multiple definition of `slab_stack_depot_init'; arch/x86/kernel/head32.o:head32.c:(.text+0x0): first defined here
   ld: arch/x86/kernel/irqinit.o: in function `slab_stack_depot_init':
   irqinit.c:(.text+0x0): multiple definition of `slab_stack_depot_init'; arch/x86/kernel/head32.o:head32.c:(.text+0x0): first defined here
   ld: arch/x86/kernel/irq_work.o: in function `slab_stack_depot_init':
   irq_work.c:(.text+0x0): multiple definition of `slab_stack_depot_init'; arch/x86/kernel/head32.o:head32.c:(.text+0x0): first defined here
   ld: arch/x86/kernel/probe_roms.o: in function `slab_stack_depot_init':
   probe_roms.c:(.text+0x1a3): multiple definition of `slab_stack_depot_init'; arch/x86/kernel/head32.o:head32.c:(.text+0x0): first defined here
   ld: arch/x86/kernel/sys_ia32.o: in function `slab_stack_depot_init':
   sys_ia32.c:(.text+0x0): multiple definition of `slab_stack_depot_init'; arch/x86/kernel/head32.o:head32.c:(.text+0x0): first defined here
   ld: arch/x86/kernel/bootflag.o: in function `slab_stack_depot_init':
   bootflag.c:(.text+0x0): multiple definition of `slab_stack_depot_init'; arch/x86/kernel/head32.o:head32.c:(.text+0x0): first defined here
   ld: arch/x86/kernel/e820.o: in function `slab_stack_depot_init':
   e820.c:(.text+0xbf): multiple definition of `slab_stack_depot_init'; arch/x86/kernel/head32.o:head32.c:(.text+0x0): first defined here
   ld: arch/x86/kernel/pci-dma.o: in function `slab_stack_depot_init':
   pci-dma.c:(.text+0x0): multiple definition of `slab_stack_depot_init'; arch/x86/kernel/head32.o:head32.c:(.text+0x0): first defined here
   ld: arch/x86/kernel/quirks.o: in function `slab_stack_depot_init':
   quirks.c:(.text+0x0): multiple definition of `slab_stack_depot_init'; arch/x86/kernel/head32.o:head32.c:(.text+0x0): first defined here
   ld: arch/x86/kernel/topology.o: in function `slab_stack_depot_init':
   topology.c:(.text+0x0): multiple definition of `slab_stack_depot_init'; arch/x86/kernel/head32.o:head32.c:(.text+0x0): first defined here
   ld: arch/x86/kernel/kdebugfs.o: in function `slab_stack_depot_init':
   kdebugfs.c:(.text+0x0): multiple definition of `slab_stack_depot_init'; arch/x86/kernel/head32.o:head32.c:(.text+0x0): first defined here
   ld: arch/x86/kernel/alternative.o: in function `slab_stack_depot_init':
   alternative.c:(.text+0x320): multiple definition of `slab_stack_depot_init'; arch/x86/kernel/head32.o:head32.c:(.text+0x0): first defined here
   ld: arch/x86/kernel/hw_breakpoint.o: in function `slab_stack_depot_init':
   hw_breakpoint.c:(.text+0x0): multiple definition of `slab_stack_depot_init'; arch/x86/kernel/head32.o:head32.c:(.text+0x0): first defined here
   ld: arch/x86/kernel/rtc.o: in function `slab_stack_depot_init':
   rtc.c:(.text+0x41): multiple definition of `slab_stack_depot_init'; arch/x86/kernel/head32.o:head32.c:(.text+0x0): first defined here
   ld: arch/x86/kernel/pci-iommu_table.o: in function `slab_stack_depot_init':
   pci-iommu_table.c:(.text+0x0): multiple definition of `slab_stack_depot_init'; arch/x86/kernel/head32.o:head32.c:(.text+0x0): first defined here
   ld: arch/x86/kernel/process.o: in function `slab_stack_depot_init':
   process.c:(.text+0xcf): multiple definition of `slab_stack_depot_init'; arch/x86/kernel/head32.o:head32.c:(.text+0x0): first defined here
   ld: arch/x86/kernel/fpu/init.o: in function `slab_stack_depot_init':
   init.c:(.text+0x0): multiple definition of `slab_stack_depot_init'; arch/x86/kernel/head32.o:head32.c:(.text+0x0): first defined here

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v3] mm/slub: initialize stack depot in boot process
  2022-03-01  7:11 ` kernel test robot
@ 2022-03-01  8:51   ` Hyeonggon Yoo
  2022-03-01  9:14     ` Vlastimil Babka
  0 siblings, 1 reply; 7+ messages in thread
From: Hyeonggon Yoo @ 2022-03-01  8:51 UTC (permalink / raw)
  To: kernel test robot
  Cc: Vlastimil Babka, kbuild-all, David Rientjes, Christoph Lameter,
	Joonsoo Kim, Pekka Enberg, Roman Gushchin, Andrew Morton,
	Linux Memory Management List, patches, linux-kernel,
	Oliver Glitta, Faiyaz Mohammed, Dmitry Vyukov, Eric Dumazet,
	Jarkko Sakkinen, Johannes Berg, Yury Norov, Arnd Bergmann,
	James Bottomley, Matteo Croce, Marco Elver, Andrey Konovalov,
	Imran Khan, Zqiang

commit ba10d4b46655 ("mm/slub: use stackdepot to save stack trace in
objects") initializes stack depot in cache creation if SLAB_STORE_USER
flag is set.

This can make kernel crash because a cache can be crashed in various
contexts. For example if user sets slub_debug=U, kernel crashes
because create_boot_cache() calls stack_depot_init(), which tries to
allocate hash table using memblock_alloc() if slab is not available.
But memblock is also not available at that time.

This patch solves the problem by initializing stack depot early
in boot process if SLAB_STORE_USER debug flag is set globally
or the flag is set for at least one cache.

[ elver@google.com: initialize stack depot depending on slub_debug
  parameter instead of allowing stack_depot_init() to be called
  during kmem_cache_init() for simplicity. ]

[ vbabka@suse.cz: parse slub_debug parameter in setup_slub_debug()
  and initialize stack depot in stack_depot_early_init(). ]

[ lkp@intel.com: Fix build error. ]

Link: https://lore.kernel.org/all/YhyeaP8lrzKgKm5A@ip-172-31-19-208.ap-northeast-1.compute.internal/
Fixes: ba10d4b46655 ("mm/slub: use stackdepot to save stack trace in objects")
Signed-off-by: Hyeonggon Yoo <42.hyeyoo@gmail.com>
---
 include/linux/slab.h       |  1 +
 include/linux/stackdepot.h |  3 ++-
 mm/slab.c                  |  5 +++++
 mm/slob.c                  |  5 +++++
 mm/slub.c                  | 19 ++++++++++++++++---
 5 files changed, 29 insertions(+), 4 deletions(-)

diff --git a/include/linux/slab.h b/include/linux/slab.h
index 37bde99b74af..d2b0f8f9e5e6 100644
--- a/include/linux/slab.h
+++ b/include/linux/slab.h
@@ -762,6 +762,7 @@ extern void kvfree_sensitive(const void *addr, size_t len);
 
 unsigned int kmem_cache_size(struct kmem_cache *s);
 void __init kmem_cache_init_late(void);
+int __init slab_stack_depot_init(void);
 
 #if defined(CONFIG_SMP) && defined(CONFIG_SLAB)
 int slab_prepare_cpu(unsigned int cpu);
diff --git a/include/linux/stackdepot.h b/include/linux/stackdepot.h
index 17f992fe6355..a813a2673c48 100644
--- a/include/linux/stackdepot.h
+++ b/include/linux/stackdepot.h
@@ -12,6 +12,7 @@
 #define _LINUX_STACKDEPOT_H
 
 #include <linux/gfp.h>
+#include <linux/slab.h>
 
 typedef u32 depot_stack_handle_t;
 
@@ -32,7 +33,7 @@ int stack_depot_init(void);
 #ifdef CONFIG_STACKDEPOT_ALWAYS_INIT
 static inline int stack_depot_early_init(void)	{ return stack_depot_init(); }
 #else
-static inline int stack_depot_early_init(void)	{ return 0; }
+static inline int stack_depot_early_init(void)	{ return slab_stack_depot_init(); }
 #endif
 
 depot_stack_handle_t stack_depot_save(unsigned long *entries,
diff --git a/mm/slab.c b/mm/slab.c
index ddf5737c63d9..c7f929665fbe 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -1196,6 +1196,11 @@ static void __init set_up_node(struct kmem_cache *cachep, int index)
 	}
 }
 
+int __init slab_stack_depot_init(void)
+{
+	return 0;
+}
+
 /*
  * Initialisation.  Called after the page allocator have been initialised and
  * before smp_init().
diff --git a/mm/slob.c b/mm/slob.c
index 60c5842215f1..7597c219f061 100644
--- a/mm/slob.c
+++ b/mm/slob.c
@@ -725,3 +725,8 @@ void __init kmem_cache_init_late(void)
 {
 	slab_state = FULL;
 }
+
+int __init slab_stack_depot_init(void)
+{
+	return 0;
+}
diff --git a/mm/slub.c b/mm/slub.c
index a74afe59a403..8f130f917977 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -646,6 +646,16 @@ static slab_flags_t slub_debug;
 
 static char *slub_debug_string;
 static int disable_higher_order_debug;
+static bool __initdata init_stack_depot;
+
+int __init slab_stack_depot_init(void)
+{
+#ifdef CONFIG_STACKDEPOT
+	if (init_stack_depot)
+		stack_depot_init();
+#endif
+	return 0;
+}
 
 /*
  * slub is about to manipulate internal object metadata.  This memory lies
@@ -1531,6 +1541,8 @@ static int __init setup_slub_debug(char *str)
 			global_slub_debug_changed = true;
 		} else {
 			slab_list_specified = true;
+			if (flags & SLAB_STORE_USER)
+				init_stack_depot = true;
 		}
 	}
 
@@ -1546,6 +1558,10 @@ static int __init setup_slub_debug(char *str)
 			global_flags = slub_debug;
 		slub_debug_string = saved_str;
 	}
+
+	if (global_flags & SLAB_STORE_USER)
+		init_stack_depot = true;
+
 out:
 	slub_debug = global_flags;
 	if (slub_debug != 0 || slub_debug_string)
@@ -4221,9 +4237,6 @@ static int kmem_cache_open(struct kmem_cache *s, slab_flags_t flags)
 	s->remote_node_defrag_ratio = 1000;
 #endif
 
-	if (s->flags & SLAB_STORE_USER && IS_ENABLED(CONFIG_STACKDEPOT))
-		stack_depot_init();
-
 	/* Initialize the pre-computed randomized freelist if slab is up */
 	if (slab_state >= UP) {
 		if (init_cache_random_seq(s))
-- 
2.33.1

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH v3] mm/slub: initialize stack depot in boot process
  2022-03-01  8:51   ` [PATCH v3] " Hyeonggon Yoo
@ 2022-03-01  9:14     ` Vlastimil Babka
  2022-03-01  9:29       ` Hyeonggon Yoo
  0 siblings, 1 reply; 7+ messages in thread
From: Vlastimil Babka @ 2022-03-01  9:14 UTC (permalink / raw)
  To: Hyeonggon Yoo, kernel test robot
  Cc: kbuild-all, David Rientjes, Christoph Lameter, Joonsoo Kim,
	Pekka Enberg, Roman Gushchin, Andrew Morton,
	Linux Memory Management List, patches, linux-kernel,
	Oliver Glitta, Faiyaz Mohammed, Dmitry Vyukov, Eric Dumazet,
	Jarkko Sakkinen, Johannes Berg, Yury Norov, Arnd Bergmann,
	James Bottomley, Matteo Croce, Marco Elver, Andrey Konovalov,
	Imran Khan, Zqiang

On 3/1/22 09:51, Hyeonggon Yoo wrote:
> commit ba10d4b46655 ("mm/slub: use stackdepot to save stack trace in
> objects") initializes stack depot in cache creation if SLAB_STORE_USER
> flag is set.

As pointed out, this is not a stable commit, the series was just posted for
review and there will be v2. So instead of "this fixes the commit..." I
suggest writing the patch assuming it's a preparation for the patch
"mm/slub: use stackdepot"... and I can then make it part of the series.
So it should instead explain that for slub_debug we will need a way to
trigger stack_depot_early_init() based on boot options and so this patch
introduces it...

> This can make kernel crash because a cache can be crashed in various
> contexts. For example if user sets slub_debug=U, kernel crashes
> because create_boot_cache() calls stack_depot_init(), which tries to
> allocate hash table using memblock_alloc() if slab is not available.
> But memblock is also not available at that time.
> 
> This patch solves the problem by initializing stack depot early
> in boot process if SLAB_STORE_USER debug flag is set globally
> or the flag is set for at least one cache.
> 
> [ elver@google.com: initialize stack depot depending on slub_debug
>   parameter instead of allowing stack_depot_init() to be called
>   during kmem_cache_init() for simplicity. ]
> 
> [ vbabka@suse.cz: parse slub_debug parameter in setup_slub_debug()
>   and initialize stack depot in stack_depot_early_init(). ]
> 
> [ lkp@intel.com: Fix build error. ]
> 
> Link: https://lore.kernel.org/all/YhyeaP8lrzKgKm5A@ip-172-31-19-208.ap-northeast-1.compute.internal/
> Fixes: ba10d4b46655 ("mm/slub: use stackdepot to save stack trace in objects")
> Signed-off-by: Hyeonggon Yoo <42.hyeyoo@gmail.com>
> ---
>  include/linux/slab.h       |  1 +
>  include/linux/stackdepot.h |  3 ++-
>  mm/slab.c                  |  5 +++++
>  mm/slob.c                  |  5 +++++
>  mm/slub.c                  | 19 ++++++++++++++++---
>  5 files changed, 29 insertions(+), 4 deletions(-)
> 
> diff --git a/include/linux/slab.h b/include/linux/slab.h
> index 37bde99b74af..d2b0f8f9e5e6 100644
> --- a/include/linux/slab.h
> +++ b/include/linux/slab.h
> @@ -762,6 +762,7 @@ extern void kvfree_sensitive(const void *addr, size_t len);
>  
>  unsigned int kmem_cache_size(struct kmem_cache *s);
>  void __init kmem_cache_init_late(void);
> +int __init slab_stack_depot_init(void);
>  
>  #if defined(CONFIG_SMP) && defined(CONFIG_SLAB)
>  int slab_prepare_cpu(unsigned int cpu);
> diff --git a/include/linux/stackdepot.h b/include/linux/stackdepot.h
> index 17f992fe6355..a813a2673c48 100644
> --- a/include/linux/stackdepot.h
> +++ b/include/linux/stackdepot.h
> @@ -12,6 +12,7 @@
>  #define _LINUX_STACKDEPOT_H
>  
>  #include <linux/gfp.h>
> +#include <linux/slab.h>
>  
>  typedef u32 depot_stack_handle_t;
>  
> @@ -32,7 +33,7 @@ int stack_depot_init(void);
>  #ifdef CONFIG_STACKDEPOT_ALWAYS_INIT
>  static inline int stack_depot_early_init(void)	{ return stack_depot_init(); }
>  #else
> -static inline int stack_depot_early_init(void)	{ return 0; }
> +static inline int stack_depot_early_init(void)	{ return slab_stack_depot_init(); }
>  #endif

I think the approach should be generic for stackdepot, not tied to a
function that belongs to slab with 3 different implementations.
E.g. in stackdepot.h declare a variable e.g. "stack_depot_want_early_init"
that is checked in stack_depot_early_init() above to call stack_depot_init().

>  depot_stack_handle_t stack_depot_save(unsigned long *entries,
> diff --git a/mm/slab.c b/mm/slab.c
> index ddf5737c63d9..c7f929665fbe 100644
> --- a/mm/slab.c
> +++ b/mm/slab.c
> @@ -1196,6 +1196,11 @@ static void __init set_up_node(struct kmem_cache *cachep, int index)
>  	}
>  }
>  
> +int __init slab_stack_depot_init(void)
> +{
> +	return 0;
> +}
> +
>  /*
>   * Initialisation.  Called after the page allocator have been initialised and
>   * before smp_init().
> diff --git a/mm/slob.c b/mm/slob.c
> index 60c5842215f1..7597c219f061 100644
> --- a/mm/slob.c
> +++ b/mm/slob.c
> @@ -725,3 +725,8 @@ void __init kmem_cache_init_late(void)
>  {
>  	slab_state = FULL;
>  }
> +
> +int __init slab_stack_depot_init(void)
> +{
> +	return 0;
> +}
> diff --git a/mm/slub.c b/mm/slub.c
> index a74afe59a403..8f130f917977 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -646,6 +646,16 @@ static slab_flags_t slub_debug;
>  
>  static char *slub_debug_string;
>  static int disable_higher_order_debug;
> +static bool __initdata init_stack_depot;
> +
> +int __init slab_stack_depot_init(void)
> +{
> +#ifdef CONFIG_STACKDEPOT
> +	if (init_stack_depot)
> +		stack_depot_init();
> +#endif
> +	return 0;
> +}



>  /*
>   * slub is about to manipulate internal object metadata.  This memory lies
> @@ -1531,6 +1541,8 @@ static int __init setup_slub_debug(char *str)
>  			global_slub_debug_changed = true;
>  		} else {
>  			slab_list_specified = true;
> +			if (flags & SLAB_STORE_USER)
> +				init_stack_depot = true;
>  		}
>  	}
>  
> @@ -1546,6 +1558,10 @@ static int __init setup_slub_debug(char *str)
>  			global_flags = slub_debug;
>  		slub_debug_string = saved_str;
>  	}
> +
> +	if (global_flags & SLAB_STORE_USER)
> +		init_stack_depot = true;

This looks good, it would just set the "stack_depot_want_early_init"
variable instead. But logically should be part of "mm/slub: use
stackdepot...", not part of patch that introduces the variable. That so if
you don't mind I would move it there with credit.

>  out:
>  	slub_debug = global_flags;
>  	if (slub_debug != 0 || slub_debug_string)
> @@ -4221,9 +4237,6 @@ static int kmem_cache_open(struct kmem_cache *s, slab_flags_t flags)
>  	s->remote_node_defrag_ratio = 1000;
>  #endif
>  
> -	if (s->flags & SLAB_STORE_USER && IS_ENABLED(CONFIG_STACKDEPOT))
> -		stack_depot_init();
> -
>  	/* Initialize the pre-computed randomized freelist if slab is up */
>  	if (slab_state >= UP) {
>  		if (init_cache_random_seq(s))


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v3] mm/slub: initialize stack depot in boot process
  2022-03-01  9:14     ` Vlastimil Babka
@ 2022-03-01  9:29       ` Hyeonggon Yoo
  0 siblings, 0 replies; 7+ messages in thread
From: Hyeonggon Yoo @ 2022-03-01  9:29 UTC (permalink / raw)
  To: Vlastimil Babka
  Cc: kernel test robot, kbuild-all, David Rientjes, Christoph Lameter,
	Joonsoo Kim, Pekka Enberg, Roman Gushchin, Andrew Morton,
	Linux Memory Management List, patches, linux-kernel,
	Oliver Glitta, Faiyaz Mohammed, Dmitry Vyukov, Eric Dumazet,
	Jarkko Sakkinen, Johannes Berg, Yury Norov, Arnd Bergmann,
	James Bottomley, Matteo Croce, Marco Elver, Andrey Konovalov,
	Imran Khan, Zqiang

On Tue, Mar 01, 2022 at 10:14:30AM +0100, Vlastimil Babka wrote:
> On 3/1/22 09:51, Hyeonggon Yoo wrote:
> > commit ba10d4b46655 ("mm/slub: use stackdepot to save stack trace in
> > objects") initializes stack depot in cache creation if SLAB_STORE_USER
> > flag is set.
> 
> As pointed out, this is not a stable commit, the series was just posted for
> review and there will be v2. So instead of "this fixes the commit..." I
> suggest writing the patch assuming it's a preparation for the patch
> "mm/slub: use stackdepot"... and I can then make it part of the series.
> So it should instead explain that for slub_debug we will need a way to
> trigger stack_depot_early_init() based on boot options and so this patch
> introduces it...
>

Agreed.

> > This can make kernel crash because a cache can be crashed in various
> > contexts. For example if user sets slub_debug=U, kernel crashes
> > because create_boot_cache() calls stack_depot_init(), which tries to
> > allocate hash table using memblock_alloc() if slab is not available.
> > But memblock is also not available at that time.
> > 
> > This patch solves the problem by initializing stack depot early
> > in boot process if SLAB_STORE_USER debug flag is set globally
> > or the flag is set for at least one cache.
> > 
> > [ elver@google.com: initialize stack depot depending on slub_debug
> >   parameter instead of allowing stack_depot_init() to be called
> >   during kmem_cache_init() for simplicity. ]
> > 
> > [ vbabka@suse.cz: parse slub_debug parameter in setup_slub_debug()
> >   and initialize stack depot in stack_depot_early_init(). ]
> > 
> > [ lkp@intel.com: Fix build error. ]
> > 
> > Link: https://lore.kernel.org/all/YhyeaP8lrzKgKm5A@ip-172-31-19-208.ap-northeast-1.compute.internal/
> > Fixes: ba10d4b46655 ("mm/slub: use stackdepot to save stack trace in objects")
> > Signed-off-by: Hyeonggon Yoo <42.hyeyoo@gmail.com>
> > ---
> >  include/linux/slab.h       |  1 +
> >  include/linux/stackdepot.h |  3 ++-
> >  mm/slab.c                  |  5 +++++
> >  mm/slob.c                  |  5 +++++
> >  mm/slub.c                  | 19 ++++++++++++++++---
> >  5 files changed, 29 insertions(+), 4 deletions(-)
> > 
> > diff --git a/include/linux/slab.h b/include/linux/slab.h
> > index 37bde99b74af..d2b0f8f9e5e6 100644
> > --- a/include/linux/slab.h
> > +++ b/include/linux/slab.h
> > @@ -762,6 +762,7 @@ extern void kvfree_sensitive(const void *addr, size_t len);
> >  
> >  unsigned int kmem_cache_size(struct kmem_cache *s);
> >  void __init kmem_cache_init_late(void);
> > +int __init slab_stack_depot_init(void);
> >  
> >  #if defined(CONFIG_SMP) && defined(CONFIG_SLAB)
> >  int slab_prepare_cpu(unsigned int cpu);
> > diff --git a/include/linux/stackdepot.h b/include/linux/stackdepot.h
> > index 17f992fe6355..a813a2673c48 100644
> > --- a/include/linux/stackdepot.h
> > +++ b/include/linux/stackdepot.h
> > @@ -12,6 +12,7 @@
> >  #define _LINUX_STACKDEPOT_H
> >  
> >  #include <linux/gfp.h>
> > +#include <linux/slab.h>
> >  
> >  typedef u32 depot_stack_handle_t;
> >  
> > @@ -32,7 +33,7 @@ int stack_depot_init(void);
> >  #ifdef CONFIG_STACKDEPOT_ALWAYS_INIT
> >  static inline int stack_depot_early_init(void)	{ return stack_depot_init(); }
> >  #else
> > -static inline int stack_depot_early_init(void)	{ return 0; }
> > +static inline int stack_depot_early_init(void)	{ return slab_stack_depot_init(); }
> >  #endif
> 
> I think the approach should be generic for stackdepot, not tied to a
> function that belongs to slab with 3 different implementations.
> E.g. in stackdepot.h declare a variable e.g. "stack_depot_want_early_init"
> that is checked in stack_depot_early_init() above to call stack_depot_init().

Hmm yeah if we define it in stack depot, that would be nice.
I just didn't want to expose global variable that is specific to slub.

> >  depot_stack_handle_t stack_depot_save(unsigned long *entries,
> > diff --git a/mm/slab.c b/mm/slab.c
> > index ddf5737c63d9..c7f929665fbe 100644
> > --- a/mm/slab.c
> > +++ b/mm/slab.c
> > @@ -1196,6 +1196,11 @@ static void __init set_up_node(struct kmem_cache *cachep, int index)
> >  	}
> >  }
> >  
> > +int __init slab_stack_depot_init(void)
> > +{
> > +	return 0;
> > +}
> > +
> >  /*
> >   * Initialisation.  Called after the page allocator have been initialised and
> >   * before smp_init().
> > diff --git a/mm/slob.c b/mm/slob.c
> > index 60c5842215f1..7597c219f061 100644
> > --- a/mm/slob.c
> > +++ b/mm/slob.c
> > @@ -725,3 +725,8 @@ void __init kmem_cache_init_late(void)
> >  {
> >  	slab_state = FULL;
> >  }
> > +
> > +int __init slab_stack_depot_init(void)
> > +{
> > +	return 0;
> > +}
> > diff --git a/mm/slub.c b/mm/slub.c
> > index a74afe59a403..8f130f917977 100644
> > --- a/mm/slub.c
> > +++ b/mm/slub.c
> > @@ -646,6 +646,16 @@ static slab_flags_t slub_debug;
> >  
> >  static char *slub_debug_string;
> >  static int disable_higher_order_debug;
> > +static bool __initdata init_stack_depot;
> > +
> > +int __init slab_stack_depot_init(void)
> > +{
> > +#ifdef CONFIG_STACKDEPOT
> > +	if (init_stack_depot)
> > +		stack_depot_init();
> > +#endif
> > +	return 0;
> > +}
> 
> 
> 
> >  /*
> >   * slub is about to manipulate internal object metadata.  This memory lies
> > @@ -1531,6 +1541,8 @@ static int __init setup_slub_debug(char *str)
> >  			global_slub_debug_changed = true;
> >  		} else {
> >  			slab_list_specified = true;
> > +			if (flags & SLAB_STORE_USER)
> > +				init_stack_depot = true;
> >  		}
> >  	}
> >  
> > @@ -1546,6 +1558,10 @@ static int __init setup_slub_debug(char *str)
> >  			global_flags = slub_debug;
> >  		slub_debug_string = saved_str;
> >  	}
> > +
> > +	if (global_flags & SLAB_STORE_USER)
> > +		init_stack_depot = true;
> 
> This looks good, it would just set the "stack_depot_want_early_init"
> variable instead. But logically should be part of "mm/slub: use
> stackdepot...", not part of patch that introduces the variable. That so if
> you don't mind I would move it there with credit.
>

Oh I don't mind that.
Then I will expect this to be solved in next series...

I'm looking forward to next version. Thanks!
-- 
Thank you, You are awesome!
Hyeonggon :-)

> >  out:
> >  	slub_debug = global_flags;
> >  	if (slub_debug != 0 || slub_debug_string)
> > @@ -4221,9 +4237,6 @@ static int kmem_cache_open(struct kmem_cache *s, slab_flags_t flags)
> >  	s->remote_node_defrag_ratio = 1000;
> >  #endif
> >  
> > -	if (s->flags & SLAB_STORE_USER && IS_ENABLED(CONFIG_STACKDEPOT))
> > -		stack_depot_init();
> > -
> >  	/* Initialize the pre-computed randomized freelist if slab is up */
> >  	if (slab_state >= UP) {
> >  		if (init_cache_random_seq(s))
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2022-03-01  9:30 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-01  3:36 [PATCH v2] mm/slub: initialize stack depot in boot process Hyeonggon Yoo
2022-03-01  3:53 ` Hyeonggon Yoo
2022-03-01  7:00 ` kernel test robot
2022-03-01  7:11 ` kernel test robot
2022-03-01  8:51   ` [PATCH v3] " Hyeonggon Yoo
2022-03-01  9:14     ` Vlastimil Babka
2022-03-01  9:29       ` Hyeonggon Yoo

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).