| Message ID | 20231023025125.90972-1-liushixin2@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [-next] mm/kmemleak: move the initialisation of object to __link_object | expand |
On Mon, Oct 23, 2023 at 10:51:25AM +0800, Liu Shixin wrote: > Leave __alloc_object() just do the actual allocation and __link_object() > do the full initialisation. > > Suggested-by: Catalin Marinas <catalin.marinas@arm.com> > Signed-off-by: Liu Shixin <liushixin2@huawei.com> It looks fine to me. Thanks. Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
On Mon, 23 Oct 2023 10:51:25 +0800 Liu Shixin <liushixin2@huawei.com> wrote: > Leave __alloc_object() just do the actual allocation and __link_object() > do the full initialisation. Changelog doesn't describe why we're making this change?
On 2023/10/24 2:16, Andrew Morton wrote: > On Mon, 23 Oct 2023 10:51:25 +0800 Liu Shixin <liushixin2@huawei.com> wrote: > >> Leave __alloc_object() just do the actual allocation and __link_object() >> do the full initialisation. > Changelog doesn't describe why we're making this change? > . In patch (“mm: kmemleak: split __create_object into two functions”), the initialisation of object has been splited in two places. Catalin said it feels a bit weird and error prone. So leave __alloc_object() just do the actual allocation and __link_object() do the full initialisation. thanks, >
Hi Liu, On Mon, Oct 23, 2023 at 3:52 AM Liu Shixin <liushixin2@huawei.com> wrote: > Leave __alloc_object() just do the actual allocation and __link_object() > do the full initialisation. > > Suggested-by: Catalin Marinas <catalin.marinas@arm.com> > Signed-off-by: Liu Shixin <liushixin2@huawei.com> Thanks for your patch, which is now commit 245245c2fffd0050 ("mm/kmemleak: move the initialisation of object to __link_object") in v6.7-rc1. I have bisected to this commit the BUG splat below (seen on various platforms). Reverting this commit fixes the issue. Memory: 7923468K/8257536K available (9024K kernel code, 5144K rwdata, 4088K rodata, 3072K init, 18331K bss, 268532K reserved, 65536K cma-reserved) SLUB: HWalign=64, Order=0-3, MinObjects=0, CPUs=4, Nodes=1 + +============================= +[ BUG: Invalid wait context ] +6.6.0-rc4-white-hawk-00387-g245245c2fffd #192 Not tainted +----------------------------- +swapper/0 is trying to lock: +ffffffc0814bbed8 (&zone->lock){....}-{3:3}, at: __rmqueue_pcplist+0x4ac/0x53c +other info that might help us debug this: +context-{5:5} +3 locks held by swapper/0: + #0: ffffffc0813cd720 (slab_mutex){....}-{4:4}, at: kmem_cache_create_usercopy+0xac/0x2e0 + #1: ffffffc0813d93e8 (kmemleak_lock){....}-{2:2}, at: __create_object+0x48/0x98 + #2: ffffff86bef6cc98 (&pcp->lock){....}-{3:3}, at: get_page_from_freelist+0x184/0x7c0 +stack backtrace: +CPU: 0 PID: 0 Comm: swapper Not tainted 6.6.0-rc4-white-hawk-00387-g245245c2fffd #192 +Hardware name: Renesas White Hawk CPU and Breakout boards based on r8a779g0 (DT) +Call trace: + dump_backtrace+0xac/0xe4 + show_stack+0x14/0x20 + dump_stack_lvl+0x68/0x94 + dump_stack+0x14/0x1c + __lock_acquire+0x390/0xffc + lock_acquire+0x230/0x28c + _raw_spin_lock_irqsave+0x54/0x70 + __rmqueue_pcplist+0x4ac/0x53c + get_page_from_freelist+0x2a8/0x7c0 + __alloc_pages+0xf4/0x9f8 + __stack_depot_save+0x178/0x3c8 + stack_depot_save+0x10/0x18 + set_track_prepare+0x44/0x70 + __link_object+0xd0/0x220 + __create_object+0x64/0x98 + kmemleak_alloc+0x28/0x34 + slab_post_alloc_hook.constprop.0+0xbc/0xc4 + kmem_cache_alloc+0xd4/0x158 + kmem_cache_create_usercopy+0x1c8/0x2e0 + kmem_cache_create+0x18/0x20 + kmemleak_init+0x74/0xfc + mm_core_init+0x214/0x250 + start_kernel+0x2cc/0x4ec + __primary_switched+0xb4/0xbc trace event string verifier disabled Running RCU self tests Gr{oetje,eeting}s, Geert
On 2023/11/15 0:21, Geert Uytterhoeven wrote: > Hi Liu, > > On Mon, Oct 23, 2023 at 3:52 AM Liu Shixin <liushixin2@huawei.com> wrote: >> Leave __alloc_object() just do the actual allocation and __link_object() >> do the full initialisation. >> >> Suggested-by: Catalin Marinas <catalin.marinas@arm.com> >> Signed-off-by: Liu Shixin <liushixin2@huawei.com> > Thanks for your patch, which is now commit 245245c2fffd0050 > ("mm/kmemleak: move the initialisation of object to __link_object") > in v6.7-rc1. > > I have bisected to this commit the BUG splat below (seen on various > platforms). Reverting this commit fixes the issue. Thanks for the discovery. It looks like that we can't call set_track_prepare() inside kmemleak_lock. I will revert this patch. Thanks. > > Memory: 7923468K/8257536K available (9024K kernel code, 5144K rwdata, > 4088K rodata, 3072K init, 18331K bss, 268532K reserved, 65536K > cma-reserved) > SLUB: HWalign=64, Order=0-3, MinObjects=0, CPUs=4, Nodes=1 > + > +============================= > +[ BUG: Invalid wait context ] > +6.6.0-rc4-white-hawk-00387-g245245c2fffd #192 Not tainted > +----------------------------- > +swapper/0 is trying to lock: > +ffffffc0814bbed8 (&zone->lock){....}-{3:3}, at: __rmqueue_pcplist+0x4ac/0x53c > +other info that might help us debug this: > +context-{5:5} > +3 locks held by swapper/0: > + #0: ffffffc0813cd720 (slab_mutex){....}-{4:4}, at: > kmem_cache_create_usercopy+0xac/0x2e0 > + #1: ffffffc0813d93e8 (kmemleak_lock){....}-{2:2}, at: > __create_object+0x48/0x98 > + #2: ffffff86bef6cc98 (&pcp->lock){....}-{3:3}, at: > get_page_from_freelist+0x184/0x7c0 > +stack backtrace: > +CPU: 0 PID: 0 Comm: swapper Not tainted > 6.6.0-rc4-white-hawk-00387-g245245c2fffd #192 > +Hardware name: Renesas White Hawk CPU and Breakout boards based on > r8a779g0 (DT) > +Call trace: > + dump_backtrace+0xac/0xe4 > + show_stack+0x14/0x20 > + dump_stack_lvl+0x68/0x94 > + dump_stack+0x14/0x1c > + __lock_acquire+0x390/0xffc > + lock_acquire+0x230/0x28c > + _raw_spin_lock_irqsave+0x54/0x70 > + __rmqueue_pcplist+0x4ac/0x53c > + get_page_from_freelist+0x2a8/0x7c0 > + __alloc_pages+0xf4/0x9f8 > + __stack_depot_save+0x178/0x3c8 > + stack_depot_save+0x10/0x18 > + set_track_prepare+0x44/0x70 > + __link_object+0xd0/0x220 > + __create_object+0x64/0x98 > + kmemleak_alloc+0x28/0x34 > + slab_post_alloc_hook.constprop.0+0xbc/0xc4 > + kmem_cache_alloc+0xd4/0x158 > + kmem_cache_create_usercopy+0x1c8/0x2e0 > + kmem_cache_create+0x18/0x20 > + kmemleak_init+0x74/0xfc > + mm_core_init+0x214/0x250 > + start_kernel+0x2cc/0x4ec > + __primary_switched+0xb4/0xbc > trace event string verifier disabled > Running RCU self tests > > Gr{oetje,eeting}s, > > Geert >
diff --git a/mm/kmemleak.c b/mm/kmemleak.c index a956b2734324..f51544a682de 100644 --- a/mm/kmemleak.c +++ b/mm/kmemleak.c @@ -642,16 +642,32 @@ static struct kmemleak_object * __alloc_object(gfp_t gfp) if (!object) { pr_warn("Cannot allocate a kmemleak_object structure\n"); kmemleak_disable(); - return NULL; } + return object; +} + +static int __link_object(struct kmemleak_object *object, unsigned long ptr, + size_t size, int min_count, bool is_phys) +{ + + struct kmemleak_object *parent; + struct rb_node **link, *rb_parent; + unsigned long untagged_ptr; + unsigned long untagged_objp; + INIT_LIST_HEAD(&object->object_list); INIT_LIST_HEAD(&object->gray_list); INIT_HLIST_HEAD(&object->area_list); raw_spin_lock_init(&object->lock); atomic_set(&object->use_count, 1); + object->flags = OBJECT_ALLOCATED | (is_phys ? OBJECT_PHYS : 0); + object->pointer = ptr; + object->size = kfence_ksize((void *)ptr) ?: size; object->excess_ref = 0; + object->min_count = min_count; object->count = 0; /* white color initially */ + object->jiffies = jiffies; object->checksum = 0; object->del_state = 0; @@ -676,24 +692,6 @@ static struct kmemleak_object * __alloc_object(gfp_t gfp) /* kernel backtrace */ object->trace_handle = set_track_prepare(); - return object; -} - -static int __link_object(struct kmemleak_object *object, unsigned long ptr, - size_t size, int min_count, bool is_phys) -{ - - struct kmemleak_object *parent; - struct rb_node **link, *rb_parent; - unsigned long untagged_ptr; - unsigned long untagged_objp; - - object->flags = OBJECT_ALLOCATED | (is_phys ? OBJECT_PHYS : 0); - object->pointer = ptr; - object->size = kfence_ksize((void *)ptr) ?: size; - object->min_count = min_count; - object->jiffies = jiffies; - untagged_ptr = (unsigned long)kasan_reset_tag((void *)ptr); /* * Only update min_addr and max_addr with object
Leave __alloc_object() just do the actual allocation and __link_object() do the full initialisation. Suggested-by: Catalin Marinas <catalin.marinas@arm.com> Signed-off-by: Liu Shixin <liushixin2@huawei.com> --- mm/kmemleak.c | 36 +++++++++++++++++------------------- 1 file changed, 17 insertions(+), 19 deletions(-)