Nick Hu <nic...@andestech.com> writes: > Hi Christoph, > > Thanks for your reply. I will answer one by one. > > Hi Alexander, > > Would you help me for the question about SOFTIRQENTRY_TEXT? > > On Mon, Aug 12, 2019 at 11:10:50PM +0800, Christoph Hellwig wrote: >> > 2. KASAN can't debug the modules since the modules are allocated in VMALLOC >> > area. We mapped the shadow memory, which corresponding to VMALLOC area, >> > to the kasan_early_shadow_page because we don't have enough physical space >> > for all the shadow memory corresponding to VMALLOC area. >> >> How do other architectures solve this problem? >> > Other archs like arm64 and x86 allocate modules in their module region.
I've run in to a similar difficulty in ppc64. My approach has been to add a generic feature to allow kasan to handle vmalloc areas: https://lore.kernel.org/linux-mm/20190903145536.3390-1-...@axtens.net/ I link this with ppc64 in this series: https://lore.kernel.org/linuxppc-dev/20190806233827.16454-1-...@axtens.net/ However, see Christophe Leroy's comments: he thinks I should take a different approach in a number of places, including just adding a separate module area. I haven't had time to think through all of his proposals yet; in particular I'd want to think through what the implication of a separate module area is for KASLR. Regards, Daniel > >> > @@ -54,6 +54,8 @@ config RISCV >> > select EDAC_SUPPORT >> > select ARCH_HAS_GIGANTIC_PAGE >> > select ARCH_WANT_HUGE_PMD_SHARE if 64BIT >> > + select GENERIC_STRNCPY_FROM_USER if KASAN >> >> Is there any reason why we can't always enabled this? Also just >> enabling the generic efficient strncpy_from_user should probably be >> a separate patch. >> > You're right, always enable it would be better. > >> > + select HAVE_ARCH_KASAN if MMU >> >> Based on your cover letter this should be if MMU && 64BIT >> >> > #define __HAVE_ARCH_MEMCPY >> > extern asmlinkage void *memcpy(void *, const void *, size_t); >> > +extern asmlinkage void *__memcpy(void *, const void *, size_t); >> > >> > #define __HAVE_ARCH_MEMMOVE >> > extern asmlinkage void *memmove(void *, const void *, size_t); >> > +extern asmlinkage void *__memmove(void *, const void *, size_t); >> > + >> > +#define memcpy(dst, src, len) __memcpy(dst, src, len) >> > +#define memmove(dst, src, len) __memmove(dst, src, len) >> > +#define memset(s, c, n) __memset(s, c, n) >> >> This looks weird and at least needs a very good comment. Also >> with this we effectively don't need the non-prefixed prototypes >> anymore. Also you probably want to split the renaming of the mem* >> routines into a separate patch with a proper changelog. >> > I made some mistakes on this porting, this would be better: > > #define __HAVE_ARCH_MEMSET > extern asmlinkage void *memset(void *, int, size_t); > extern asmlinkage void *__memset(void *, int, size_t); > > #define __HAVE_ARCH_MEMCPY > extern asmlinkage void *memcpy(void *, const void *, size_t); > extern asmlinkage void *__memcpy(void *, const void *, size_t); > > #define __HAVE_ARCH_MEMMOVE > extern asmlinkage void *memmove(void *, const void *, size_t); > extern asmlinkage void *__memmove(void *, const void *, size_t); > > #if defined(CONFIG_KASAN) && !defined(__SANITIZE_ADDRESS__) > > #define memcpy(dst, src, len) __memcpy(dst, src, len) > #define memmove(dst, src, len) __memmove(dst, src, len) > #define memset(s, c, n) __memset(s, c, n) > > #endif > >> > #include <asm/tlbflush.h> >> > #include <asm/thread_info.h> >> > >> > +#ifdef CONFIG_KASAN >> > +#include <asm/kasan.h> >> > +#endif >> >> Any good reason to not just always include the header? >> > Nope, I would remove the '#ifdef CONFIG_KASAN', and do the logic in the header > instead. > >> > + >> > #ifdef CONFIG_DUMMY_CONSOLE >> > struct screen_info screen_info = { >> > .orig_video_lines = 30, >> > @@ -64,12 +68,17 @@ void __init setup_arch(char **cmdline_p) >> > >> > setup_bootmem(); >> > paging_init(); >> > + >> > unflatten_device_tree(); >> >> spurious whitespace change. >> >> > diff --git a/arch/riscv/kernel/vmlinux.lds.S >> > b/arch/riscv/kernel/vmlinux.lds.S >> > index 23cd1a9..9700980 100644 >> > --- a/arch/riscv/kernel/vmlinux.lds.S >> > +++ b/arch/riscv/kernel/vmlinux.lds.S >> > @@ -46,6 +46,7 @@ SECTIONS >> > KPROBES_TEXT >> > ENTRY_TEXT >> > IRQENTRY_TEXT >> > + SOFTIRQENTRY_TEXT >> >> Hmm. What is the relation to kasan here? Maybe we should add this >> separately with a good changelog? >> > There is a commit for it: > > Author: Alexander Potapenko <gli...@google.com> > Date: Fri Mar 25 14:22:05 2016 -0700 > > arch, ftrace: for KASAN put hard/soft IRQ entries into separate sections > > KASAN needs to know whether the allocation happens in an IRQ handler. > This lets us strip everything below the IRQ entry point to reduce the > number of unique stack traces needed to be stored. > > Move the definition of __irq_entry to <linux/interrupt.h> so that the > users don't need to pull in <linux/ftrace.h>. Also introduce the > __softirq_entry macro which is similar to __irq_entry, but puts the > corresponding functions to the .softirqentry.text section. > > After reading the patch I understand that soft/hard IRQ entries should be > separated for KASAN to work, but why? > > Alexender, do you have any comments on this? > >> > +++ b/arch/riscv/mm/kasan_init.c >> > @@ -0,0 +1,102 @@ >> > +// SPDX-License-Identifier: GPL-2.0 >> >> This probably also wants a copyright statement. >> >> > + // init for swapper_pg_dir >> >> Please use /* */ style comments. > > -- > You received this message because you are subscribed to the Google Groups > "kasan-dev" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to kasan-dev+unsubscr...@googlegroups.com. > To view this discussion on the web visit > https://groups.google.com/d/msgid/kasan-dev/20190814074417.GA21929%40andestech.com.
