Message ID | 20190416142258.18694-1-cai@lca.pw (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | slab: remove store_stackinfo() | expand |
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) >
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
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
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 >
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
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>
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)
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(-)