linux-riscv.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
From: Daniel Axtens <dja@axtens.net>
To: Nick Hu <nickhu@andestech.com>, Christoph Hellwig <hch@infradead.org>
Cc: "aou@eecs.berkeley.edu" <aou@eecs.berkeley.edu>,
	"Alan Quey-Liang Kao(高魁良)" <alankao@andestech.com>,
	"atish.patra@wdc.com" <atish.patra@wdc.com>,
	"gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
	"palmer@sifive.com" <palmer@sifive.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"kasan-dev@googlegroups.com" <kasan-dev@googlegroups.com>,
	"離職Zong Zong-Xian Li(李宗憲)" <zong@andestech.com>,
	"alexios.zavras@intel.com" <alexios.zavras@intel.com>,
	"Anup.Patel@wdc.com" <Anup.Patel@wdc.com>,
	"glider@google.com" <glider@google.com>,
	"green.hu@gmail.com" <green.hu@gmail.com>,
	"paul.walmsley@sifive.com" <paul.walmsley@sifive.com>,
	"aryabinin@virtuozzo.com" <aryabinin@virtuozzo.com>,
	"tglx@linutronix.de" <tglx@linutronix.de>,
	"deanbo422@gmail.com" <deanbo422@gmail.com>,
	"linux-riscv@lists.infradead.org"
	<linux-riscv@lists.infradead.org>,
	"dvyukov@google.com" <dvyukov@google.com>
Subject: Re: [PATCH 2/2] riscv: Add KASAN support
Date: Wed, 04 Sep 2019 01:08:51 +1000	[thread overview]
Message-ID: <87k1apto1o.fsf@dja-thinkpad.axtens.net> (raw)
In-Reply-To: <20190814074417.GA21929@andestech.com>

Nick Hu <nickhu@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-dja@axtens.net/

I link this with ppc64 in this series:

https://lore.kernel.org/linuxppc-dev/20190806233827.16454-1-dja@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 <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.
>
> -- 
> 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+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kasan-dev/20190814074417.GA21929%40andestech.com.

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

  parent reply	other threads:[~2019-09-03 15:09 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-07  7:19 [PATCH 0/2] KASAN support for RISC-V Nick Hu
2019-08-07  7:19 ` [PATCH 1/2] riscv: Add memmove string operation Nick Hu
2019-08-12 15:04   ` Christoph Hellwig
2019-08-13 23:50     ` Palmer Dabbelt
2019-08-14  2:22       ` Paul Walmsley
2019-08-14  3:27         ` Nick Hu
2019-08-14 17:03           ` Paul Walmsley
2019-08-15  3:12             ` Nick Hu
2019-08-15 18:27               ` Paul Walmsley
2019-08-19  6:29                 ` Nick Hu
2019-08-14 18:33         ` Palmer Dabbelt
2019-08-22 15:59   ` Andrey Ryabinin
2019-08-27  9:07     ` Nick Hu
2019-08-27  9:33       ` Andrey Ryabinin
2019-08-28  3:06         ` Nick Hu
2019-08-07  7:19 ` [PATCH 2/2] riscv: Add KASAN support Nick Hu
2019-08-12 15:10   ` Christoph Hellwig
2019-08-14  7:44     ` Nick Hu
2019-08-22 17:08       ` Andrey Ryabinin
2019-09-03 15:08       ` Daniel Axtens [this message]
2019-09-04  2:24         ` Nick Hu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87k1apto1o.fsf@dja-thinkpad.axtens.net \
    --to=dja@axtens.net \
    --cc=Anup.Patel@wdc.com \
    --cc=alankao@andestech.com \
    --cc=alexios.zavras@intel.com \
    --cc=aou@eecs.berkeley.edu \
    --cc=aryabinin@virtuozzo.com \
    --cc=atish.patra@wdc.com \
    --cc=deanbo422@gmail.com \
    --cc=dvyukov@google.com \
    --cc=glider@google.com \
    --cc=green.hu@gmail.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=hch@infradead.org \
    --cc=kasan-dev@googlegroups.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-riscv@lists.infradead.org \
    --cc=nickhu@andestech.com \
    --cc=palmer@sifive.com \
    --cc=paul.walmsley@sifive.com \
    --cc=tglx@linutronix.de \
    --cc=zong@andestech.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).