Messages in this thread | | | Date | Wed, 24 Nov 2021 13:12:49 +0800 | Subject | Re: [PATCH v2] mm: Delay kmemleak object creation of module_alloc() | From | Kefeng Wang <> |
| |
On 2021/11/24 3:44, Catalin Marinas wrote: > On Tue, Nov 23, 2021 at 10:32:20PM +0800, Kefeng Wang wrote: >> Yongqiang reports a kmemleak panic when module insmod/rmmod with KASAN >> enabled on x86[1]. >> >> When the module allocates memory, it's kmemleak_object is created successfully, >> but the KASAN shadow memory of module allocation is not ready, so when kmemleak >> scan the module's pointer, it will panic due to no shadow memory with KASAN. >> >> module_alloc >> __vmalloc_node_range >> kmemleak_vmalloc >> kmemleak_scan >> update_checksum >> kasan_module_alloc >> kmemleak_ignore > Can you share the .config and the stack trace you get on arm64?
My gcc could not support CC_HAS_KASAN_SW_TAGS, so no repoduced on ARM64
> > I have a suspicion there is no problem if KASAN_VMALLOC is enabled.
Yes, if KASAN_VMALLOC enabled, the memory of vmalloc shadow has been populated in arch's kasan_init(),
there is no issue. but x86/arm64/s390 support dynamic allocation of module area per module load by
kasan_module_alloc(), and this leads the above concurrent issue.
>> diff --git a/mm/kasan/shadow.c b/mm/kasan/shadow.c >> index 4a4929b29a23..2ade2f484562 100644 >> --- a/mm/kasan/shadow.c >> +++ b/mm/kasan/shadow.c >> @@ -498,7 +498,7 @@ void kasan_release_vmalloc(unsigned long start, unsigned long end, >> >> #else /* CONFIG_KASAN_VMALLOC */ >> >> -int kasan_module_alloc(void *addr, size_t size) >> +int kasan_module_alloc(void *addr, size_t size, gfp_t gfp_mask) >> { >> void *ret; >> size_t scaled_size; >> @@ -520,9 +520,14 @@ int kasan_module_alloc(void *addr, size_t size) >> __builtin_return_address(0)); >> >> if (ret) { >> + struct vm_struct *vm = find_vm_area(addr); >> __memset(ret, KASAN_SHADOW_INIT, shadow_size); >> - find_vm_area(addr)->flags |= VM_KASAN; >> + vm->flags |= VM_KASAN; >> kmemleak_ignore(ret); >> + >> + if (vm->flags & VM_DELAY_KMEMLEAK) >> + kmemleak_vmalloc(vm, size, gfp_mask); >> + >> return 0; >> } > This function only exists if CONFIG_KASAN_VMALLOC=n. yes. > >> diff --git a/mm/vmalloc.c b/mm/vmalloc.c >> index d2a00ad4e1dd..23c595b15839 100644 >> --- a/mm/vmalloc.c >> +++ b/mm/vmalloc.c >> @@ -3074,7 +3074,8 @@ void *__vmalloc_node_range(unsigned long size, unsigned long align, >> clear_vm_uninitialized_flag(area); >> >> size = PAGE_ALIGN(size); >> - kmemleak_vmalloc(area, size, gfp_mask); >> + if (!(vm_flags & VM_DELAY_KMEMLEAK)) >> + kmemleak_vmalloc(area, size, gfp_mask); > So with KASAN_VMALLOC enabled, we'll miss the kmemleak allocation.
See the definination, if KASAN_VMALLOC enabled, VM_DELAY_KMEMLEAK is 0, so kmemleak allocation
still works.
+#if defined(CONFIG_KASAN) && (defined(CONFIG_KASAN_GENERIC) || \ + defined(CONFIG_KASAN_SW_TAGS)) && !defined(CONFIG_KASAN_VMALLOC) +#define VM_DELAY_KMEMLEAK 0x00000800 /* delay kmemleak object create */ +#else +#define VM_DELAY_KMEMLEAK 0 +#endif +
> > You could add an IS_ENABLED(CONFIG_KASAN_VMALLOC) check but I'm not > particularly fond of the delay approach (also think DEFER is probably a > better name). Will use DEFER instead. > > A quick fix would be to make KMEMLEAK depend on !KASAN || KASAN_VMALLOC. > We'll miss KASAN_SW_TAGS with kmemleak but I think vmalloc support could > be enabled for this as well. > > What does KASAN do with other vmalloc() allocations when !KASAN_VMALLOC? > Can we not have a similar approach. I don't fully understand why the > module vmalloc() is a special case.
Only the shadow of module area is dynamic allocation , this exists on ARM64 too.
if no KASAN_VMALLOC, no shadow area for vmalloc, other vmalloc allocation is
no problem. Correct me if I'm wrong.
Thanks.
>
| |