Re: [PATCH 1/2] riscv: Add memmove string operation.
Hi Paul,
On Tue, Aug 27, 2019 at 05:33:11PM +0800, Andrey Ryabinin wrote:
>
>
> On 8/27/19 12:07 PM, Nick Hu wrote:
> > Hi Andrey
> >
> > On Thu, Aug 22, 2019 at 11:59:02PM +0800, Andrey Ryabinin wrote:
> >> On 8/7/19 10:19 AM, Nick Hu wrote:
> >>> There are some features which need this string operation for compilation,
> >>> like KASAN. So the purpose of this porting is for the features like KASAN
> >>> which cannot be compiled without it.
> >>>
> >>
> >> Compilation error can be fixed by diff bellow (I didn't test it).
> >> If you don't need memmove very early (before kasan_early_init()) than
> >> arch-specific not-instrumented memmove()
> >> isn't necessary to have.
> >>
> >> ---
> >> mm/kasan/common.c | 2 ++
> >> 1 file changed, 2 insertions(+)
> >>
> >> diff --git a/mm/kasan/common.c b/mm/kasan/common.c
> >> index 6814d6d6a023..897f9520bab3 100644
> >> --- a/mm/kasan/common.c
> >> +++ b/mm/kasan/common.c
> >> @@ -107,6 +107,7 @@ void *memset(void *addr, int c, size_t len)
> >>return __memset(addr, c, len);
> >> }
> >>
> >> +#ifdef __HAVE_ARCH_MEMMOVE
> >> #undef memmove
> >> void *memmove(void *dest, const void *src, size_t len)
> >> {
> >> @@ -115,6 +116,7 @@ void *memmove(void *dest, const void *src, size_t len)
> >>
> >>return __memmove(dest, src, len);
> >> }
> >> +#endif
> >>
> >> #undef memcpy
> >> void *memcpy(void *dest, const void *src, size_t len)
> >> --
> >> 2.21.0
> >>
> >>
> >>
> > I have confirmed that the string operations are not used before
> > kasan_early_init().
> > But I can't make sure whether other ARCHs would need it before
> > kasan_early_init().
> > Do you have any idea to check that? Should I cc all other ARCH maintainers?
>
>
> This doesn't affect other ARCHes in any way. If other arches have their own
> not-instrumented
> memmove implementation (and they do), they will continue to be able to use it
> early.
I prefer Andrey's method since porting the generic string operations with
newlib ones should
be a separated patch from KASAN.
Nick
Re: [PATCH 1/2] riscv: Add memmove string operation.
On 8/27/19 12:07 PM, Nick Hu wrote:
> Hi Andrey
>
> On Thu, Aug 22, 2019 at 11:59:02PM +0800, Andrey Ryabinin wrote:
>> On 8/7/19 10:19 AM, Nick Hu wrote:
>>> There are some features which need this string operation for compilation,
>>> like KASAN. So the purpose of this porting is for the features like KASAN
>>> which cannot be compiled without it.
>>>
>>
>> Compilation error can be fixed by diff bellow (I didn't test it).
>> If you don't need memmove very early (before kasan_early_init()) than
>> arch-specific not-instrumented memmove()
>> isn't necessary to have.
>>
>> ---
>> mm/kasan/common.c | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/mm/kasan/common.c b/mm/kasan/common.c
>> index 6814d6d6a023..897f9520bab3 100644
>> --- a/mm/kasan/common.c
>> +++ b/mm/kasan/common.c
>> @@ -107,6 +107,7 @@ void *memset(void *addr, int c, size_t len)
>> return __memset(addr, c, len);
>> }
>>
>> +#ifdef __HAVE_ARCH_MEMMOVE
>> #undef memmove
>> void *memmove(void *dest, const void *src, size_t len)
>> {
>> @@ -115,6 +116,7 @@ void *memmove(void *dest, const void *src, size_t len)
>>
>> return __memmove(dest, src, len);
>> }
>> +#endif
>>
>> #undef memcpy
>> void *memcpy(void *dest, const void *src, size_t len)
>> --
>> 2.21.0
>>
>>
>>
> I have confirmed that the string operations are not used before
> kasan_early_init().
> But I can't make sure whether other ARCHs would need it before
> kasan_early_init().
> Do you have any idea to check that? Should I cc all other ARCH maintainers?
This doesn't affect other ARCHes in any way. If other arches have their own
not-instrumented
memmove implementation (and they do), they will continue to be able to use it
early.
Re: [PATCH 1/2] riscv: Add memmove string operation.
Hi Andrey
On Thu, Aug 22, 2019 at 11:59:02PM +0800, Andrey Ryabinin wrote:
> On 8/7/19 10:19 AM, Nick Hu wrote:
> > There are some features which need this string operation for compilation,
> > like KASAN. So the purpose of this porting is for the features like KASAN
> > which cannot be compiled without it.
> >
>
> Compilation error can be fixed by diff bellow (I didn't test it).
> If you don't need memmove very early (before kasan_early_init()) than
> arch-specific not-instrumented memmove()
> isn't necessary to have.
>
> ---
> mm/kasan/common.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/mm/kasan/common.c b/mm/kasan/common.c
> index 6814d6d6a023..897f9520bab3 100644
> --- a/mm/kasan/common.c
> +++ b/mm/kasan/common.c
> @@ -107,6 +107,7 @@ void *memset(void *addr, int c, size_t len)
> return __memset(addr, c, len);
> }
>
> +#ifdef __HAVE_ARCH_MEMMOVE
> #undef memmove
> void *memmove(void *dest, const void *src, size_t len)
> {
> @@ -115,6 +116,7 @@ void *memmove(void *dest, const void *src, size_t len)
>
> return __memmove(dest, src, len);
> }
> +#endif
>
> #undef memcpy
> void *memcpy(void *dest, const void *src, size_t len)
> --
> 2.21.0
>
>
>
I have confirmed that the string operations are not used before
kasan_early_init().
But I can't make sure whether other ARCHs would need it before
kasan_early_init().
Do you have any idea to check that? Should I cc all other ARCH maintainers?
Nick
Re: [PATCH 1/2] riscv: Add memmove string operation.
On 8/7/19 10:19 AM, Nick Hu wrote:
> There are some features which need this string operation for compilation,
> like KASAN. So the purpose of this porting is for the features like KASAN
> which cannot be compiled without it.
>
Compilation error can be fixed by diff bellow (I didn't test it).
If you don't need memmove very early (before kasan_early_init()) than
arch-specific not-instrumented memmove()
isn't necessary to have.
---
mm/kasan/common.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/mm/kasan/common.c b/mm/kasan/common.c
index 6814d6d6a023..897f9520bab3 100644
--- a/mm/kasan/common.c
+++ b/mm/kasan/common.c
@@ -107,6 +107,7 @@ void *memset(void *addr, int c, size_t len)
return __memset(addr, c, len);
}
+#ifdef __HAVE_ARCH_MEMMOVE
#undef memmove
void *memmove(void *dest, const void *src, size_t len)
{
@@ -115,6 +116,7 @@ void *memmove(void *dest, const void *src, size_t len)
return __memmove(dest, src, len);
}
+#endif
#undef memcpy
void *memcpy(void *dest, const void *src, size_t len)
--
2.21.0
Re: [PATCH 1/2] riscv: Add memmove string operation.
Hi Paul,
On Thu, Aug 15, 2019 at 11:27:51AM -0700, Paul Walmsley wrote:
> On Thu, 15 Aug 2019, Nick Hu wrote:
>
> > On Wed, Aug 14, 2019 at 10:03:39AM -0700, Paul Walmsley wrote:
> >
> > > Thanks for the explanation. What do you think about Palmer's idea to
> > > define a generic C set of KASAN string operations, derived from the
> > > newlib
> > > code?
> >
> > That sounds good to me. But it should be another topic. We need to
> > investigate
> > it further about replacing something generic and fundamental in lib/string.c
> > with newlib C functions. Some blind spots may exist. So I suggest, let's
> > consider KASAN for now.
>
> OK. Here is the problem for us as maintainers. You, Palmer, and I all
> agree that a C-language version would be better. We'd rather not merge a
> pure assembly-language version unless it had significant advantages, and
> right now we're not anticipating that. So that suggests that a C-language
> memmove() is the right way to go.
>
> But if we merge a C-language memmove() into arch/riscv, other kernel
> developers would probably ask us why we're doing that, since there's
> nothing RISC-V-specific about it. So do you think you might reconsider
> sending patches to add a generic C-language memmove()?
>
>
> - Paul
About pushing mem*() generic, let's start with the reason why in the first place
KASAN needs re-implement its own string operations:
In mm/kasan/common.c:
#undef memset
void *memset(void *addr, int c, size_t len)
{
check_memory_region((unsigned long)addr, len, true, _RET_IP_);
return __memset(addr, c, len);
}
KASAN would call the string operations with the prefix '__', which should be
just an alias to the proper one.
In the past, every architecture that supports KASAN does this in assembly.
E.g. ARM64:
In arch/arm64/lib/memset.S:
ENTRY(__memset)
ENTRY(memset)
...
...
EXPORT_SYMBOL(memset)
EXPORT_SYMBOL(__memset) // export this as an alias
In arch/arm64/include/asm/string.h
#define __HAVE_ARCH_MEMSET
extern void *memset(void *, int, __kernel_size_t);
extern void *__memset(void *, int, __kernel_size_t);
Now, if we are going to replace the current string operations with newlib ones
and let KASAN use them, we must provide something like this:
In lib/string.c:
void *___memset(...)
{
...
}
In include/linux/string.h:
#ifndef __HAVE_ARCH_MEMCPY
#ifdef CONFIG_KASAN
static inline void* __memset(...)
{
___memset(...);
}
extern void memset(...); // force those who include this header uses the
memset wrapped by KASAN
#else
static inline void *memset(...)
{
___memset(...);
}
#endif
#endif
Does this look OK to you?
Nick
Re: [PATCH 1/2] riscv: Add memmove string operation.
Hi Paul, On Wed, Aug 14, 2019 at 10:03:39AM -0700, Paul Walmsley wrote: > Hi Nick, > > On Wed, 14 Aug 2019, Nick Hu wrote: > > > On Wed, Aug 14, 2019 at 10:22:15AM +0800, Paul Walmsley wrote: > > > On Tue, 13 Aug 2019, Palmer Dabbelt wrote: > > > > > > > On Mon, 12 Aug 2019 08:04:46 PDT (-0700), Christoph Hellwig wrote: > > > > > On Wed, Aug 07, 2019 at 03:19:14PM +0800, Nick Hu wrote: > > > > > > There are some features which need this string operation for > > > > > > compilation, > > > > > > like KASAN. So the purpose of this porting is for the features like > > > > > > KASAN > > > > > > which cannot be compiled without it. > > > > > > > > > > > > KASAN's string operations would replace the original string > > > > > > operations and > > > > > > call for the architecture defined string operations. Since we don't > > > > > > have > > > > > > this in current kernel, this patch provides the implementation. > > > > > > > > > > > > This porting refers to the 'arch/nds32/lib/memmove.S'. > > > > > > > > > > This looks sensible to me, although my stringop asm is rather rusty, > > > > > so just an ack and not a real review-by: > > > > > > > > FWIW, we just write this in C everywhere else and rely on the compiler > > > > to > > > > unroll the loops. I always prefer C to assembly when possible, so I'd > > > > prefer > > > > if we just adopt the string code from newlib. We have a RISC-V-specific > > > > memcpy in there, but just use the generic memmove. > > > > > > > > Maybe the best bet here would be to adopt the newlib memcpy/memmove as > > > > generic > > > > Linux functions? They're both in C so they should be fine, and they > > > > both look > > > > faster than what's in lib/string.c. Then everyone would benefit and we > > > > don't > > > > need this tricky RISC-V assembly. Also, from the look of it the newlib > > > > code > > > > is faster because the inner loop is unrolled. > > > > > > There's a generic memmove implementation in the kernel already: > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/string.h#n362 > > > > > > Nick, could you tell us more about why the generic memmove() isn't > > > suitable? > > > > KASAN has its own string operations(memcpy/memmove/memset) because it needs > > to > > hook some code to check memory region. It would undefined the original > > string > > operations and called the string operations with the prefix '__'. But the > > generic string operations didn't declare with the prefix. Other archs with > > KASAN support like arm64 and xtensa all have their own string operations and > > defined with the prefix. > > Thanks for the explanation. What do you think about Palmer's idea to > define a generic C set of KASAN string operations, derived from the newlib > code? > > > - Paul That sounds good to me. But it should be another topic. We need to investigate it further about replacing something generic and fundamental in lib/string.c with newlib C functions. Some blind spots may exist. So I suggest, let's consider KASAN for now. Nick
Re: [PATCH 1/2] riscv: Add memmove string operation.
On Tue, 13 Aug 2019 19:22:15 PDT (-0700), Paul Walmsley wrote: On Tue, 13 Aug 2019, Palmer Dabbelt wrote: On Mon, 12 Aug 2019 08:04:46 PDT (-0700), Christoph Hellwig wrote: > On Wed, Aug 07, 2019 at 03:19:14PM +0800, Nick Hu wrote: > > There are some features which need this string operation for compilation, > > like KASAN. So the purpose of this porting is for the features like KASAN > > which cannot be compiled without it. > > > > KASAN's string operations would replace the original string operations and > > call for the architecture defined string operations. Since we don't have > > this in current kernel, this patch provides the implementation. > > > > This porting refers to the 'arch/nds32/lib/memmove.S'. > > This looks sensible to me, although my stringop asm is rather rusty, > so just an ack and not a real review-by: > > Acked-by: Christoph Hellwig FWIW, we just write this in C everywhere else and rely on the compiler to unroll the loops. I always prefer C to assembly when possible, so I'd prefer if we just adopt the string code from newlib. We have a RISC-V-specific memcpy in there, but just use the generic memmove. Maybe the best bet here would be to adopt the newlib memcpy/memmove as generic Linux functions? They're both in C so they should be fine, and they both look faster than what's in lib/string.c. Then everyone would benefit and we don't need this tricky RISC-V assembly. Also, from the look of it the newlib code is faster because the inner loop is unrolled. There's a generic memmove implementation in the kernel already: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/string.h#n362 That ends up at __builtin_memcpy(), which ends up looking for memcpy() for large copies, which is in lib/string.c. The code in there is just byte at a time memcpy()/memmove(), which is way slower than the newlib stuff. Nick, could you tell us more about why the generic memmove() isn't suitable? - Paul
Re: [PATCH 1/2] riscv: Add memmove string operation.
On Wed, Aug 14, 2019 at 10:22:15AM +0800, Paul Walmsley wrote: > On Tue, 13 Aug 2019, Palmer Dabbelt wrote: > > > On Mon, 12 Aug 2019 08:04:46 PDT (-0700), Christoph Hellwig wrote: > > > On Wed, Aug 07, 2019 at 03:19:14PM +0800, Nick Hu wrote: > > > > There are some features which need this string operation for > > > > compilation, > > > > like KASAN. So the purpose of this porting is for the features like > > > > KASAN > > > > which cannot be compiled without it. > > > > > > > > KASAN's string operations would replace the original string operations > > > > and > > > > call for the architecture defined string operations. Since we don't have > > > > this in current kernel, this patch provides the implementation. > > > > > > > > This porting refers to the 'arch/nds32/lib/memmove.S'. > > > > > > This looks sensible to me, although my stringop asm is rather rusty, > > > so just an ack and not a real review-by: > > > > > > Acked-by: Christoph Hellwig > > > > FWIW, we just write this in C everywhere else and rely on the compiler to > > unroll the loops. I always prefer C to assembly when possible, so I'd > > prefer > > if we just adopt the string code from newlib. We have a RISC-V-specific > > memcpy in there, but just use the generic memmove. > > > > Maybe the best bet here would be to adopt the newlib memcpy/memmove as > > generic > > Linux functions? They're both in C so they should be fine, and they both > > look > > faster than what's in lib/string.c. Then everyone would benefit and we > > don't > > need this tricky RISC-V assembly. Also, from the look of it the newlib code > > is faster because the inner loop is unrolled. > > There's a generic memmove implementation in the kernel already: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/string.h#n362 > > Nick, could you tell us more about why the generic memmove() isn't > suitable? > > > - Paul Hi Paul, KASAN has its own string operations(memcpy/memmove/memset) because it needs to hook some code to check memory region. It would undefined the original string operations and called the string operations with the prefix '__'. But the generic string operations didn't declare with the prefix. Other archs with KASAN support like arm64 and xtensa all have their own string operations and defined with the prefix. Nick
Re: [PATCH 1/2] riscv: Add memmove string operation.
On Tue, 13 Aug 2019, Palmer Dabbelt wrote: > On Mon, 12 Aug 2019 08:04:46 PDT (-0700), Christoph Hellwig wrote: > > On Wed, Aug 07, 2019 at 03:19:14PM +0800, Nick Hu wrote: > > > There are some features which need this string operation for compilation, > > > like KASAN. So the purpose of this porting is for the features like KASAN > > > which cannot be compiled without it. > > > > > > KASAN's string operations would replace the original string operations and > > > call for the architecture defined string operations. Since we don't have > > > this in current kernel, this patch provides the implementation. > > > > > > This porting refers to the 'arch/nds32/lib/memmove.S'. > > > > This looks sensible to me, although my stringop asm is rather rusty, > > so just an ack and not a real review-by: > > > > Acked-by: Christoph Hellwig > > FWIW, we just write this in C everywhere else and rely on the compiler to > unroll the loops. I always prefer C to assembly when possible, so I'd prefer > if we just adopt the string code from newlib. We have a RISC-V-specific > memcpy in there, but just use the generic memmove. > > Maybe the best bet here would be to adopt the newlib memcpy/memmove as generic > Linux functions? They're both in C so they should be fine, and they both look > faster than what's in lib/string.c. Then everyone would benefit and we don't > need this tricky RISC-V assembly. Also, from the look of it the newlib code > is faster because the inner loop is unrolled. There's a generic memmove implementation in the kernel already: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/string.h#n362 Nick, could you tell us more about why the generic memmove() isn't suitable? - Paul
Re: [PATCH 1/2] riscv: Add memmove string operation.
On Mon, 12 Aug 2019 08:04:46 PDT (-0700), Christoph Hellwig wrote: On Wed, Aug 07, 2019 at 03:19:14PM +0800, Nick Hu wrote: There are some features which need this string operation for compilation, like KASAN. So the purpose of this porting is for the features like KASAN which cannot be compiled without it. KASAN's string operations would replace the original string operations and call for the architecture defined string operations. Since we don't have this in current kernel, this patch provides the implementation. This porting refers to the 'arch/nds32/lib/memmove.S'. This looks sensible to me, although my stringop asm is rather rusty, so just an ack and not a real review-by: Acked-by: Christoph Hellwig FWIW, we just write this in C everywhere else and rely on the compiler to unroll the loops. I always prefer C to assembly when possible, so I'd prefer if we just adopt the string code from newlib. We have a RISC-V-specific memcpy in there, but just use the generic memmove. Maybe the best bet here would be to adopt the newlib memcpy/memmove as generic Linux functions? They're both in C so they should be fine, and they both look faster than what's in lib/string.c. Then everyone would benefit and we don't need this tricky RISC-V assembly. Also, from the look of it the newlib code is faster because the inner loop is unrolled.
Re: [PATCH 1/2] riscv: Add memmove string operation.
On Wed, Aug 07, 2019 at 03:19:14PM +0800, Nick Hu wrote: > There are some features which need this string operation for compilation, > like KASAN. So the purpose of this porting is for the features like KASAN > which cannot be compiled without it. > > KASAN's string operations would replace the original string operations and > call for the architecture defined string operations. Since we don't have > this in current kernel, this patch provides the implementation. > > This porting refers to the 'arch/nds32/lib/memmove.S'. This looks sensible to me, although my stringop asm is rather rusty, so just an ack and not a real review-by: Acked-by: Christoph Hellwig
