Re: [PATCH 2/2] riscv: Add KASAN support

[Nick Hu <nickhu@andestech.com>]

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.

> > @@ -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 <glider@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.

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv