[PATCH] slab: remove store_stackinfo()

[PATCH] slab: remove store_stackinfo()

[Qian Cai]

store_stackinfo() does not seem used in actual SLAB debugging.
Potentially, it could be added to check_poison_obj() to provide more
information, but this seems like an overkill due to the declining
popularity of the SLAB, so just remove it instead.

Signed-off-by: Qian Cai <cai@lca.pw>
---
 mm/slab.c | 48 ++++++------------------------------------------
 1 file changed, 6 insertions(+), 42 deletions(-)

diff --git a/mm/slab.c b/mm/slab.c
index 3e1b7ff0360c..20f318f4f56e 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -1467,53 +1467,17 @@ static bool is_debug_pagealloc_cache(struct kmem_cache *cachep)
 }
 
 #ifdef CONFIG_DEBUG_PAGEALLOC
-static void store_stackinfo(struct kmem_cache *cachep, unsigned long *addr,
-			    unsigned long caller)
-{
-	int size = cachep->object_size;
-
-	addr = (unsigned long *)&((char *)addr)[obj_offset(cachep)];
-
-	if (size < 5 * sizeof(unsigned long))
-		return;
-
-	*addr++ = 0x12345678;
-	*addr++ = caller;
-	*addr++ = smp_processor_id();
-	size -= 3 * sizeof(unsigned long);
-	{
-		unsigned long *sptr = &caller;
-		unsigned long svalue;
-
-		while (!kstack_end(sptr)) {
-			svalue = *sptr++;
-			if (kernel_text_address(svalue)) {
-				*addr++ = svalue;
-				size -= sizeof(unsigned long);
-				if (size <= sizeof(unsigned long))
-					break;
-			}
-		}
-
-	}
-	*addr++ = 0x87654321;
-}
-
-static void slab_kernel_map(struct kmem_cache *cachep, void *objp,
-				int map, unsigned long caller)
+static void slab_kernel_map(struct kmem_cache *cachep, void *objp, int map)
 {
 	if (!is_debug_pagealloc_cache(cachep))
 		return;
 
-	if (caller)
-		store_stackinfo(cachep, objp, caller);
-
 	kernel_map_pages(virt_to_page(objp), cachep->size / PAGE_SIZE, map);
 }
 
 #else
 static inline void slab_kernel_map(struct kmem_cache *cachep, void *objp,
-				int map, unsigned long caller) {}
+				int map) {}
 
 #endif
 
@@ -1661,7 +1625,7 @@ static void slab_destroy_debugcheck(struct kmem_cache *cachep,
 
 		if (cachep->flags & SLAB_POISON) {
 			check_poison_obj(cachep, objp);
-			slab_kernel_map(cachep, objp, 1, 0);
+			slab_kernel_map(cachep, objp, 1);
 		}
 		if (cachep->flags & SLAB_RED_ZONE) {
 			if (*dbg_redzone1(cachep, objp) != RED_INACTIVE)
@@ -2433,7 +2397,7 @@ static void cache_init_objs_debug(struct kmem_cache *cachep, struct page *page)
 		/* need to poison the objs? */
 		if (cachep->flags & SLAB_POISON) {
 			poison_obj(cachep, objp, POISON_FREE);
-			slab_kernel_map(cachep, objp, 0, 0);
+			slab_kernel_map(cachep, objp, 0);
 		}
 	}
 #endif
@@ -2812,7 +2776,7 @@ static void *cache_free_debugcheck(struct kmem_cache *cachep, void *objp,
 
 	if (cachep->flags & SLAB_POISON) {
 		poison_obj(cachep, objp, POISON_FREE);
-		slab_kernel_map(cachep, objp, 0, caller);
+		slab_kernel_map(cachep, objp, 0);
 	}
 	return objp;
 }
@@ -3076,7 +3040,7 @@ static void *cache_alloc_debugcheck_after(struct kmem_cache *cachep,
 		return objp;
 	if (cachep->flags & SLAB_POISON) {
 		check_poison_obj(cachep, objp);
-		slab_kernel_map(cachep, objp, 1, 0);
+		slab_kernel_map(cachep, objp, 1);
 		poison_obj(cachep, objp, POISON_INUSE);
 	}
 	if (cachep->flags & SLAB_STORE_USER)
-- 
2.20.1 (Apple Git-117)

Re: [PATCH] slab: remove store_stackinfo()

[Vlastimil Babka]

On 4/16/19 4:22 PM, Qian Cai wrote:
> store_stackinfo() does not seem used in actual SLAB debugging.
> Potentially, it could be added to check_poison_obj() to provide more
> information, but this seems like an overkill due to the declining
> popularity of the SLAB, so just remove it instead.
> 
> Signed-off-by: Qian Cai <cai@lca.pw>

I've acked Thomas' version already which was narrower, but no objection
to remove more stuff on top of that. Linus (and I later in another
thread) already pointed out /proc/slab_allocators. It only takes a look
at add_caller() there to not regret removing that one.

> ---
>  mm/slab.c | 48 ++++++------------------------------------------
>  1 file changed, 6 insertions(+), 42 deletions(-)
> 
> diff --git a/mm/slab.c b/mm/slab.c
> index 3e1b7ff0360c..20f318f4f56e 100644
> --- a/mm/slab.c
> +++ b/mm/slab.c
> @@ -1467,53 +1467,17 @@ static bool is_debug_pagealloc_cache(struct kmem_cache *cachep)
>  }
>  
>  #ifdef CONFIG_DEBUG_PAGEALLOC
> -static void store_stackinfo(struct kmem_cache *cachep, unsigned long *addr,
> -			    unsigned long caller)
> -{
> -	int size = cachep->object_size;
> -
> -	addr = (unsigned long *)&((char *)addr)[obj_offset(cachep)];
> -
> -	if (size < 5 * sizeof(unsigned long))
> -		return;
> -
> -	*addr++ = 0x12345678;
> -	*addr++ = caller;
> -	*addr++ = smp_processor_id();
> -	size -= 3 * sizeof(unsigned long);
> -	{
> -		unsigned long *sptr = &caller;
> -		unsigned long svalue;
> -
> -		while (!kstack_end(sptr)) {
> -			svalue = *sptr++;
> -			if (kernel_text_address(svalue)) {
> -				*addr++ = svalue;
> -				size -= sizeof(unsigned long);
> -				if (size <= sizeof(unsigned long))
> -					break;
> -			}
> -		}
> -
> -	}
> -	*addr++ = 0x87654321;
> -}
> -
> -static void slab_kernel_map(struct kmem_cache *cachep, void *objp,
> -				int map, unsigned long caller)
> +static void slab_kernel_map(struct kmem_cache *cachep, void *objp, int map)
>  {
>  	if (!is_debug_pagealloc_cache(cachep))
>  		return;
>  
> -	if (caller)
> -		store_stackinfo(cachep, objp, caller);
> -
>  	kernel_map_pages(virt_to_page(objp), cachep->size / PAGE_SIZE, map);
>  }
>  
>  #else
>  static inline void slab_kernel_map(struct kmem_cache *cachep, void *objp,
> -				int map, unsigned long caller) {}
> +				int map) {}
>  
>  #endif
>  
> @@ -1661,7 +1625,7 @@ static void slab_destroy_debugcheck(struct kmem_cache *cachep,
>  
>  		if (cachep->flags & SLAB_POISON) {
>  			check_poison_obj(cachep, objp);
> -			slab_kernel_map(cachep, objp, 1, 0);
> +			slab_kernel_map(cachep, objp, 1);
>  		}
>  		if (cachep->flags & SLAB_RED_ZONE) {
>  			if (*dbg_redzone1(cachep, objp) != RED_INACTIVE)
> @@ -2433,7 +2397,7 @@ static void cache_init_objs_debug(struct kmem_cache *cachep, struct page *page)
>  		/* need to poison the objs? */
>  		if (cachep->flags & SLAB_POISON) {
>  			poison_obj(cachep, objp, POISON_FREE);
> -			slab_kernel_map(cachep, objp, 0, 0);
> +			slab_kernel_map(cachep, objp, 0);
>  		}
>  	}
>  #endif
> @@ -2812,7 +2776,7 @@ static void *cache_free_debugcheck(struct kmem_cache *cachep, void *objp,
>  
>  	if (cachep->flags & SLAB_POISON) {
>  		poison_obj(cachep, objp, POISON_FREE);
> -		slab_kernel_map(cachep, objp, 0, caller);
> +		slab_kernel_map(cachep, objp, 0);
>  	}
>  	return objp;
>  }
> @@ -3076,7 +3040,7 @@ static void *cache_alloc_debugcheck_after(struct kmem_cache *cachep,
>  		return objp;
>  	if (cachep->flags & SLAB_POISON) {
>  		check_poison_obj(cachep, objp);
> -		slab_kernel_map(cachep, objp, 1, 0);
> +		slab_kernel_map(cachep, objp, 1);
>  		poison_obj(cachep, objp, POISON_INUSE);
>  	}
>  	if (cachep->flags & SLAB_STORE_USER)
> 

Re: [PATCH] slab: remove store_stackinfo()

[Qian Cai]

On 4/16/19 11:25 AM, Vlastimil Babka wrote:
> On 4/16/19 4:22 PM, Qian Cai wrote:
>> store_stackinfo() does not seem used in actual SLAB debugging.
>> Potentially, it could be added to check_poison_obj() to provide more
>> information, but this seems like an overkill due to the declining
>> popularity of the SLAB, so just remove it instead.
>>
>> Signed-off-by: Qian Cai <cai@lca.pw>
> 
> I've acked Thomas' version already which was narrower, but no objection
> to remove more stuff on top of that. Linus (and I later in another
> thread) already pointed out /proc/slab_allocators. It only takes a look
> at add_caller() there to not regret removing that one.

Well, with the 2 patches I sent a while back, /proc/slab_allocators is back to
life on all arches (arm64, powerpc, and x86) which provides a little information
that may still useful for debugging until SLAB is gone entirely.

# cat /proc/slab_allocators
xfs_ili: 92 kmem_zone_alloc+0x6c/0x100 [xfs]
xfs_ifork: 2539 kmem_zone_alloc+0x6c/0x100 [xfs]
xfs_log_ticket: 3 kmem_zone_alloc+0x6c/0x100 [xfs]
sd_ext_cdb: 2 mempool_alloc_slab+0x1c/0x30
ip_fib_trie: 7 fib_insert_alias+0x11a/0x2b0
ip_fib_alias: 9 fib_table_insert+0x16d/0x510
eventpoll_pwq: 5 ep_ptable_queue_proc+0x3f/0xc0
inotify_inode_mark: 8 __x64_sys_inotify_add_watch+0x225/0x340
khugepaged_mm_slot: 6 __khugepaged_enter+0x36/0x190
file_lock_ctx: 23 locks_get_lock_context+0xf2/0x180
fsnotify_mark_connector: 79 fsnotify_add_mark_locked+0x117/0x460
task_delay_info: 509 __delayacct_tsk_init+0x1e/0x50
sigqueue: 2 __sigqueue_alloc+0xa8/0x130
kernfs_iattrs_cache: 122 __kernfs_iattrs+0x5c/0xf0
kernfs_node_cache: 28234 __kernfs_new_node.constprop.6+0x65/0x200
buffer_head: 1 alloc_buffer_head+0x21/0x70
nsproxy: 4 create_new_namespaces+0x36/0x1c0
vm_area_struct: 17 vm_area_alloc+0x1e/0x60
anon_vma_chain: 16 __anon_vma_prepare+0x3d/0x160
anon_vma: 18 __anon_vma_prepare+0xd2/0x160
Acpi-Operand: 2931 acpi_ut_allocate_object_desc_dbg+0x3e/0x69
Acpi-Namespace: 1618 acpi_ns_create_node+0x37/0x46
numa_policy: 47 __mpol_dup+0x3c/0x170
kmemleak_scan_area: 994 kmemleak_scan_area+0xa0/0x1e0
kmemleak_object: 513426 create_object+0x48/0x2c0
trace_event_file: 1650 trace_create_new_event+0x22/0x90
ftrace_event_field: 3198 __trace_define_field+0x36/0xc0
vmap_area: 890 alloc_vmap_area+0xaf/0x880
vmap_area: 663 alloc_vmap_area+0x2a3/0x880
vmap_area: 6 pcpu_get_vm_areas+0x277/0xbe0
vmap_area: 1 pcpu_get_vm_areas+0x689/0xbe0
vmap_area: 1 vmalloc_init+0x23d/0x26e
debug_objects_cache: 15917 __debug_object_init+0x444/0x4e0
debug_objects_cache: 999 debug_objects_mem_init+0x7b/0x5a2
page->ptl: 1526 ptlock_alloc+0x1e/0x40

Re: [PATCH] slab: remove store_stackinfo()

[Thomas Gleixner]

On Tue, 16 Apr 2019, Vlastimil Babka wrote:

> On 4/16/19 4:22 PM, Qian Cai wrote:
> > store_stackinfo() does not seem used in actual SLAB debugging.
> > Potentially, it could be added to check_poison_obj() to provide more
> > information, but this seems like an overkill due to the declining
> > popularity of the SLAB, so just remove it instead.
> > 
> > Signed-off-by: Qian Cai <cai@lca.pw>
> 
> I've acked Thomas' version already which was narrower, but no objection
> to remove more stuff on top of that. Linus (and I later in another
> thread) already pointed out /proc/slab_allocators. It only takes a look
> at add_caller() there to not regret removing that one.

The issue why I was looking at this was a krobot complaint about the kernel
crashing in that stack store function with my stackguard series applied. It
was broken before the stackguard pages already, it just went unnoticed.

As you explained, nobody is caring about DEBUG_SLAB + DEBUG_PAGEALLOC
anyway, so I'm happy to not care about krobot tripping over it either.

So we have 3 options:

   1) I ignore it and merge the stack guard series w/o it

   2) I can carry the minimal fix or Qian's version in the stackguard
      branch

   3) We ship that minimal fix to Linus right now and then everyone can
      base their stuff on top independently.

#3 is probably the right thing to do.

Thanks,

	tglx

Re: [PATCH] slab: remove store_stackinfo()

[Vlastimil Babka]

On 4/16/2019 8:50 PM, Thomas Gleixner wrote:
> On Tue, 16 Apr 2019, Vlastimil Babka wrote:
> 
>> On 4/16/19 4:22 PM, Qian Cai wrote:
>>> store_stackinfo() does not seem used in actual SLAB debugging.
>>> Potentially, it could be added to check_poison_obj() to provide more
>>> information, but this seems like an overkill due to the declining
>>> popularity of the SLAB, so just remove it instead.
>>>
>>> Signed-off-by: Qian Cai <cai@lca.pw>
>>
>> I've acked Thomas' version already which was narrower, but no objection
>> to remove more stuff on top of that. Linus (and I later in another
>> thread) already pointed out /proc/slab_allocators. It only takes a look
>> at add_caller() there to not regret removing that one.
> 
> The issue why I was looking at this was a krobot complaint about the kernel
> crashing in that stack store function with my stackguard series applied. It
> was broken before the stackguard pages already, it just went unnoticed.
> 
> As you explained, nobody is caring about DEBUG_SLAB + DEBUG_PAGEALLOC
> anyway, so I'm happy to not care about krobot tripping over it either.
> 
> So we have 3 options:
> 
>    1) I ignore it and merge the stack guard series w/o it
> 
>    2) I can carry the minimal fix or Qian's version in the stackguard
>       branch
> 
>    3) We ship that minimal fix to Linus right now and then everyone can
>       base their stuff on top independently.

I think #3 is overkill for something that was broken for who knows how long and
nobody noticed. I'd go with 2) and perhaps Qian's version as nobody AFAIK uses
the caller+cpu as well as the stack trace.

For Qian's version also:
Acked-by: Vlastimil Babka <vbabka@suse.cz>

> #3 is probably the right thing to do.
> 
> Thanks,
> 
> 	tglx
> 

Re: [PATCH] slab: remove store_stackinfo()

[Thomas Gleixner]

On Tue, 16 Apr 2019, Vlastimil Babka wrote:
> On 4/16/2019 8:50 PM, Thomas Gleixner wrote:
> > On Tue, 16 Apr 2019, Vlastimil Babka wrote:
> > 
> >> On 4/16/19 4:22 PM, Qian Cai wrote:
> >>> store_stackinfo() does not seem used in actual SLAB debugging.
> >>> Potentially, it could be added to check_poison_obj() to provide more
> >>> information, but this seems like an overkill due to the declining
> >>> popularity of the SLAB, so just remove it instead.
> >>>
> >>> Signed-off-by: Qian Cai <cai@lca.pw>
> >>
> >> I've acked Thomas' version already which was narrower, but no objection
> >> to remove more stuff on top of that. Linus (and I later in another
> >> thread) already pointed out /proc/slab_allocators. It only takes a look
> >> at add_caller() there to not regret removing that one.
> > 
> > The issue why I was looking at this was a krobot complaint about the kernel
> > crashing in that stack store function with my stackguard series applied. It
> > was broken before the stackguard pages already, it just went unnoticed.
> > 
> > As you explained, nobody is caring about DEBUG_SLAB + DEBUG_PAGEALLOC
> > anyway, so I'm happy to not care about krobot tripping over it either.
> > 
> > So we have 3 options:
> > 
> >    1) I ignore it and merge the stack guard series w/o it
> > 
> >    2) I can carry the minimal fix or Qian's version in the stackguard
> >       branch
> > 
> >    3) We ship that minimal fix to Linus right now and then everyone can
> >       base their stuff on top independently.
> 
> I think #3 is overkill for something that was broken for who knows how long and
> nobody noticed. I'd go with 2) and perhaps Qian's version as nobody AFAIK uses
> the caller+cpu as well as the stack trace.
> 
> For Qian's version also:
> Acked-by: Vlastimil Babka <vbabka@suse.cz>

Ok. I'll pick it up and base the stackguard stuff on top.

Thanks,

	tglx

Re: [PATCH] slab: remove store_stackinfo()

[Thomas Gleixner]

On Tue, 16 Apr 2019, Qian Cai wrote:

> store_stackinfo() does not seem used in actual SLAB debugging.
> Potentially, it could be added to check_poison_obj() to provide more
> information, but this seems like an overkill due to the declining
> popularity of the SLAB, so just remove it instead.
> 
> Signed-off-by: Qian Cai <cai@lca.pw>

Acked-by: Thomas Gleixner <tglx@linutronix.de>

[tip:x86/irq] mm/slab: Remove store_stackinfo()

[tip-bot for Qian Cai]

Commit-ID:  80552f0f7aebdd8deda8ea455292cbfbf462d655
Gitweb:     https://git.kernel.org/tip/80552f0f7aebdd8deda8ea455292cbfbf462d655
Author:     Qian Cai <cai@lca.pw>
AuthorDate: Tue, 16 Apr 2019 10:22:57 -0400
Committer:  Borislav Petkov <bp@suse.de>
CommitDate: Wed, 17 Apr 2019 11:46:27 +0200

mm/slab: Remove store_stackinfo()

store_stackinfo() does not seem used in actual SLAB debugging.
Potentially, it could be added to check_poison_obj() to provide more
information but this seems like an overkill due to the declining
popularity of SLAB, so just remove it instead.

Signed-off-by: Qian Cai <cai@lca.pw>
Signed-off-by: Borislav Petkov <bp@suse.de>
Acked-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Vlastimil Babka <vbabka@suse.cz>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Christoph Lameter <cl@linux.com>
Cc: David Rientjes <rientjes@google.com>
Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: linux-mm <linux-mm@kvack.org>
Cc: Pekka Enberg <penberg@kernel.org>
Cc: rientjes@google.com
Cc: sean.j.christopherson@intel.com
Link: https://lkml.kernel.org/r/20190416142258.18694-1-cai@lca.pw
---
 mm/slab.c | 48 ++++++------------------------------------------
 1 file changed, 6 insertions(+), 42 deletions(-)

diff --git a/mm/slab.c b/mm/slab.c
index 47a380a486ee..e79ef28396e2 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -1467,53 +1467,17 @@ static bool is_debug_pagealloc_cache(struct kmem_cache *cachep)
 }
 
 #ifdef CONFIG_DEBUG_PAGEALLOC
-static void store_stackinfo(struct kmem_cache *cachep, unsigned long *addr,
-			    unsigned long caller)
-{
-	int size = cachep->object_size;
-
-	addr = (unsigned long *)&((char *)addr)[obj_offset(cachep)];
-
-	if (size < 5 * sizeof(unsigned long))
-		return;
-
-	*addr++ = 0x12345678;
-	*addr++ = caller;
-	*addr++ = smp_processor_id();
-	size -= 3 * sizeof(unsigned long);
-	{
-		unsigned long *sptr = &caller;
-		unsigned long svalue;
-
-		while (!kstack_end(sptr)) {
-			svalue = *sptr++;
-			if (kernel_text_address(svalue)) {
-				*addr++ = svalue;
-				size -= sizeof(unsigned long);
-				if (size <= sizeof(unsigned long))
-					break;
-			}
-		}
-
-	}
-	*addr++ = 0x87654321;
-}
-
-static void slab_kernel_map(struct kmem_cache *cachep, void *objp,
-				int map, unsigned long caller)
+static void slab_kernel_map(struct kmem_cache *cachep, void *objp, int map)
 {
 	if (!is_debug_pagealloc_cache(cachep))
 		return;
 
-	if (caller)
-		store_stackinfo(cachep, objp, caller);
-
 	kernel_map_pages(virt_to_page(objp), cachep->size / PAGE_SIZE, map);
 }
 
 #else
 static inline void slab_kernel_map(struct kmem_cache *cachep, void *objp,
-				int map, unsigned long caller) {}
+				int map) {}
 
 #endif
 
@@ -1661,7 +1625,7 @@ static void slab_destroy_debugcheck(struct kmem_cache *cachep,
 
 		if (cachep->flags & SLAB_POISON) {
 			check_poison_obj(cachep, objp);
-			slab_kernel_map(cachep, objp, 1, 0);
+			slab_kernel_map(cachep, objp, 1);
 		}
 		if (cachep->flags & SLAB_RED_ZONE) {
 			if (*dbg_redzone1(cachep, objp) != RED_INACTIVE)
@@ -2434,7 +2398,7 @@ static void cache_init_objs_debug(struct kmem_cache *cachep, struct page *page)
 		/* need to poison the objs? */
 		if (cachep->flags & SLAB_POISON) {
 			poison_obj(cachep, objp, POISON_FREE);
-			slab_kernel_map(cachep, objp, 0, 0);
+			slab_kernel_map(cachep, objp, 0);
 		}
 	}
 #endif
@@ -2813,7 +2777,7 @@ static void *cache_free_debugcheck(struct kmem_cache *cachep, void *objp,
 
 	if (cachep->flags & SLAB_POISON) {
 		poison_obj(cachep, objp, POISON_FREE);
-		slab_kernel_map(cachep, objp, 0, caller);
+		slab_kernel_map(cachep, objp, 0);
 	}
 	return objp;
 }
@@ -3077,7 +3041,7 @@ static void *cache_alloc_debugcheck_after(struct kmem_cache *cachep,
 		return objp;
 	if (cachep->flags & SLAB_POISON) {
 		check_poison_obj(cachep, objp);
-		slab_kernel_map(cachep, objp, 1, 0);
+		slab_kernel_map(cachep, objp, 1);
 		poison_obj(cachep, objp, POISON_INUSE);
 	}
 	if (cachep->flags & SLAB_STORE_USER)