Re: [RFC PATCH v1] powerpc: Enable KFENCE for PPC32
On Fri, 5 Mar 2021 at 12:49, Michael Ellerman wrote: > Marco Elver writes: > ... > > > > The choice is between: > > > > 1. ARCH_FUNC_PREFIX (as a matter of fact, the ARCH_FUNC_PREFIX patch > > is already in -mm). Perhaps we could optimize it further, by checking > > ARCH_FUNC_PREFIX in buf, and advancing buf like you propose, but I'm > > not sure it's worth worrying about. > > > > 2. The dynamic solution that I proposed that does not use a hard-coded > > '.' (or some variation thereof). > > > > Please tell me which solution you prefer, 1 or 2 -- I'd like to stop > > bikeshedding here. If there's a compelling argument for hard-coding > > the '.' in non-arch code, please clarify, but otherwise I'd like to > > keep arch-specific things out of generic code. > > It's your choice, I was just trying to minimise the size of the wart you > have to carry in kfence code to deal with it. > > The ARCH_FUNC_PREFIX solution is fine by me. Thank you -- the ARCH_FUNC_PREFIX version is already in -mm, so let's keep it. It's purely static vs the other options. Should another debugging tool need something similar we can revisit whether to change or move it. Thanks, -- Marco
Re: [RFC PATCH v1] powerpc: Enable KFENCE for PPC32
Marco Elver writes: ... > > The choice is between: > > 1. ARCH_FUNC_PREFIX (as a matter of fact, the ARCH_FUNC_PREFIX patch > is already in -mm). Perhaps we could optimize it further, by checking > ARCH_FUNC_PREFIX in buf, and advancing buf like you propose, but I'm > not sure it's worth worrying about. > > 2. The dynamic solution that I proposed that does not use a hard-coded > '.' (or some variation thereof). > > Please tell me which solution you prefer, 1 or 2 -- I'd like to stop > bikeshedding here. If there's a compelling argument for hard-coding > the '.' in non-arch code, please clarify, but otherwise I'd like to > keep arch-specific things out of generic code. It's your choice, I was just trying to minimise the size of the wart you have to carry in kfence code to deal with it. The ARCH_FUNC_PREFIX solution is fine by me. cheers
Re: [RFC PATCH v1] powerpc: Enable KFENCE for PPC32
On Fri, 5 Mar 2021 at 09:23, Christophe Leroy
wrote:
>
>
>
> Le 05/03/2021 à 08:50, Marco Elver a écrit :
> > On Fri, Mar 05, 2021 at 04:01PM +1100, Michael Ellerman wrote:
> >> Marco Elver writes:
> >>> On Thu, Mar 04, 2021 at 12:48PM +0100, Christophe Leroy wrote:
> Le 04/03/2021 à 12:31, Marco Elver a écrit :
> > On Thu, 4 Mar 2021 at 12:23, Christophe Leroy
> > wrote:
> >> Le 03/03/2021 à 11:56, Marco Elver a écrit :
> >>>
> >>> Somewhat tangentially, I also note that e.g. show_regs(regs) (which
> >>> was printed along the KFENCE report above) didn't include the top
> >>> frame in the "Call Trace", so this assumption is definitely not
> >>> isolated to KFENCE.
> >>>
> >>
> >> Now, I have tested PPC64 (with the patch I sent yesterday to modify
> >> save_stack_trace_regs()
> >> applied), and I get many failures. Any idea ?
> >>
> >> [ 17.653751][ T58]
> >> ==
> >> [ 17.654379][ T58] BUG: KFENCE: invalid free in
> >> .kfence_guarded_free+0x2e4/0x530
> >> [ 17.654379][ T58]
> >> [ 17.654831][ T58] Invalid free of 0xc0003c9c (in
> >> kfence-#77):
> >> [ 17.655358][ T58] .kfence_guarded_free+0x2e4/0x530
> >> [ 17.655775][ T58] .__slab_free+0x320/0x5a0
> >> [ 17.656039][ T58] .test_double_free+0xe0/0x198
> >> [ 17.656308][ T58] .kunit_try_run_case+0x80/0x110
> >> [ 17.656523][ T58] .kunit_generic_run_threadfn_adapter+0x38/0x50
> >> [ 17.657161][ T58] .kthread+0x18c/0x1a0
> >> [ 17.659148][ T58] .ret_from_kernel_thread+0x58/0x70
> >> [ 17.659869][ T58]
> >>> [...]
> >
> > Looks like something is prepending '.' to function names. We expect
> > the function name to appear as-is, e.g. "kfence_guarded_free",
> > "test_double_free", etc.
> >
> > Is there something special on ppc64, where the '.' is some convention?
> >
>
> I think so, see
> https://refspecs.linuxfoundation.org/ELF/ppc64/PPC-elf64abi.html#FUNC-DES
>
> Also see commit https://github.com/linuxppc/linux/commit/02424d896
> >>>
> >>> Thanks -- could you try the below patch? You'll need to define
> >>> ARCH_FUNC_PREFIX accordingly.
> >>>
> >>> We think, since there are only very few architectures that add a prefix,
> >>> requiring to define something like ARCH_FUNC_PREFIX is
> >>> the simplest option. Let me know if this works for you.
> >>>
> >>> There an alternative option, which is to dynamically figure out the
> >>> prefix, but if this simpler option is fine with you, we'd prefer it.
> >>
> >> We have rediscovered this problem in basically every tracing / debugging
> >> feature added in the last 20 years :)
> >>
> >> I think the simplest solution is the one tools/perf/util/symbol.c uses,
> >> which is to just skip a leading '.'.
> >>
> >> Does that work?
> >>
> >> diff --git a/mm/kfence/report.c b/mm/kfence/report.c
> >> index ab83d5a59bb1..67b49dc54b38 100644
> >> --- a/mm/kfence/report.c
> >> +++ b/mm/kfence/report.c
> >> @@ -67,6 +67,9 @@ static int get_stack_skipnr(const unsigned long
> >> stack_entries[], int num_entries
> >> for (skipnr = 0; skipnr < num_entries; skipnr++) {
> >> int len = scnprintf(buf, sizeof(buf), "%ps", (void
> >> *)stack_entries[skipnr]);
> >>
> >> +if (buf[0] == '.')
> >> +buf++;
> >> +
> >
> > Unfortunately this does not work, since buf is an array. We'd need an
> > offset, and it should be determined outside the loop. I had a solution
> > like this, but it turned out quite complex (see below). And since most
> > architectures do not require this, decided that the safest option is to
> > use the macro approach with ARCH_FUNC_PREFIX, for which Christophe
> > already prepared a patch and tested:
> > https://lore.kernel.org/linux-mm/20210304144000.1148590-1-el...@google.com/
> > https://lkml.kernel.org/r/afaec81a551ef15345cb7d7563b3fac3d7041c3a.1614868445.git.christophe.le...@csgroup.eu
> >
> > Since KFENCE requires anyway, we'd prefer this approach
> > (vs. dynamically detecting).
> >
> > Thanks,
> > -- Marco
> >
>
> What about
Sure something like that would work. But I explicitly did *not* want
to hard-code the '.' in non-arch code.
The choice is between:
1. ARCH_FUNC_PREFIX (as a matter of fact, the ARCH_FUNC_PREFIX patch
is already in -mm). Perhaps we could optimize it further, by checking
ARCH_FUNC_PREFIX in buf, and advancing buf like you propose, but I'm
not sure it's worth worrying about.
2. The dynamic solution that I proposed that does not use a hard-coded
'.' (or some variation thereof).
Please tell me which solution you prefer, 1 or 2 -- I'd like to stop
bikeshedding here. If there's a compelling argument for hard-coding
the '.' in non-arch code, please clarify, but otherwise I'd like to
keep arch-specific things out of generic code.
Thanks.
> diff
Re: [RFC PATCH v1] powerpc: Enable KFENCE for PPC32
Le 05/03/2021 à 08:50, Marco Elver a écrit :
On Fri, Mar 05, 2021 at 04:01PM +1100, Michael Ellerman wrote:
Marco Elver writes:
On Thu, Mar 04, 2021 at 12:48PM +0100, Christophe Leroy wrote:
Le 04/03/2021 à 12:31, Marco Elver a écrit :
On Thu, 4 Mar 2021 at 12:23, Christophe Leroy
wrote:
Le 03/03/2021 à 11:56, Marco Elver a écrit :
Somewhat tangentially, I also note that e.g. show_regs(regs) (which
was printed along the KFENCE report above) didn't include the top
frame in the "Call Trace", so this assumption is definitely not
isolated to KFENCE.
Now, I have tested PPC64 (with the patch I sent yesterday to modify
save_stack_trace_regs()
applied), and I get many failures. Any idea ?
[ 17.653751][ T58]
==
[ 17.654379][ T58] BUG: KFENCE: invalid free in
.kfence_guarded_free+0x2e4/0x530
[ 17.654379][ T58]
[ 17.654831][ T58] Invalid free of 0xc0003c9c (in kfence-#77):
[ 17.655358][ T58] .kfence_guarded_free+0x2e4/0x530
[ 17.655775][ T58] .__slab_free+0x320/0x5a0
[ 17.656039][ T58] .test_double_free+0xe0/0x198
[ 17.656308][ T58] .kunit_try_run_case+0x80/0x110
[ 17.656523][ T58] .kunit_generic_run_threadfn_adapter+0x38/0x50
[ 17.657161][ T58] .kthread+0x18c/0x1a0
[ 17.659148][ T58] .ret_from_kernel_thread+0x58/0x70
[ 17.659869][ T58]
[...]
Looks like something is prepending '.' to function names. We expect
the function name to appear as-is, e.g. "kfence_guarded_free",
"test_double_free", etc.
Is there something special on ppc64, where the '.' is some convention?
I think so, see
https://refspecs.linuxfoundation.org/ELF/ppc64/PPC-elf64abi.html#FUNC-DES
Also see commit https://github.com/linuxppc/linux/commit/02424d896
Thanks -- could you try the below patch? You'll need to define
ARCH_FUNC_PREFIX accordingly.
We think, since there are only very few architectures that add a prefix,
requiring to define something like ARCH_FUNC_PREFIX is
the simplest option. Let me know if this works for you.
There an alternative option, which is to dynamically figure out the
prefix, but if this simpler option is fine with you, we'd prefer it.
We have rediscovered this problem in basically every tracing / debugging
feature added in the last 20 years :)
I think the simplest solution is the one tools/perf/util/symbol.c uses,
which is to just skip a leading '.'.
Does that work?
diff --git a/mm/kfence/report.c b/mm/kfence/report.c
index ab83d5a59bb1..67b49dc54b38 100644
--- a/mm/kfence/report.c
+++ b/mm/kfence/report.c
@@ -67,6 +67,9 @@ static int get_stack_skipnr(const unsigned long
stack_entries[], int num_entries
for (skipnr = 0; skipnr < num_entries; skipnr++) {
int len = scnprintf(buf, sizeof(buf), "%ps", (void
*)stack_entries[skipnr]);
+ if (buf[0] == '.')
+ buf++;
+
Unfortunately this does not work, since buf is an array. We'd need an
offset, and it should be determined outside the loop. I had a solution
like this, but it turned out quite complex (see below). And since most
architectures do not require this, decided that the safest option is to
use the macro approach with ARCH_FUNC_PREFIX, for which Christophe
already prepared a patch and tested:
https://lore.kernel.org/linux-mm/20210304144000.1148590-1-el...@google.com/
https://lkml.kernel.org/r/afaec81a551ef15345cb7d7563b3fac3d7041c3a.1614868445.git.christophe.le...@csgroup.eu
Since KFENCE requires anyway, we'd prefer this approach
(vs. dynamically detecting).
Thanks,
-- Marco
What about
diff --git a/mm/kfence/report.c b/mm/kfence/report.c
index 519f037720f5..5e196625fb34 100644
--- a/mm/kfence/report.c
+++ b/mm/kfence/report.c
@@ -43,7 +43,7 @@ static void seq_con_printf(struct seq_file *seq, const char
*fmt, ...)
static int get_stack_skipnr(const unsigned long stack_entries[], int
num_entries,
const enum kfence_error_type *type)
{
- char buf[64];
+ char _buf[64];
int skipnr, fallback = 0;
if (type) {
@@ -65,7 +65,11 @@ static int get_stack_skipnr(const unsigned long
stack_entries[], int num_entries
}
for (skipnr = 0; skipnr < num_entries; skipnr++) {
- int len = scnprintf(buf, sizeof(buf), "%ps", (void
*)stack_entries[skipnr]);
+ char *buf = _buf;
+ int len = scnprintf(_buf, sizeof(_buf), "%ps", (void
*)stack_entries[skipnr]);
+
+ if (_buf[0] == '.')
+ buf++, len--;
if (str_has_prefix(buf, "kfence_") || str_has_prefix(buf,
"__kfence_") ||
!strncmp(buf, "__slab_free", len)) {
---
Christophe
Re: [RFC PATCH v1] powerpc: Enable KFENCE for PPC32
On Fri, Mar 05, 2021 at 04:01PM +1100, Michael Ellerman wrote:
> Marco Elver writes:
> > On Thu, Mar 04, 2021 at 12:48PM +0100, Christophe Leroy wrote:
> >> Le 04/03/2021 à 12:31, Marco Elver a écrit :
> >> > On Thu, 4 Mar 2021 at 12:23, Christophe Leroy
> >> > wrote:
> >> > > Le 03/03/2021 à 11:56, Marco Elver a écrit :
> >> > > >
> >> > > > Somewhat tangentially, I also note that e.g. show_regs(regs) (which
> >> > > > was printed along the KFENCE report above) didn't include the top
> >> > > > frame in the "Call Trace", so this assumption is definitely not
> >> > > > isolated to KFENCE.
> >> > > >
> >> > >
> >> > > Now, I have tested PPC64 (with the patch I sent yesterday to modify
> >> > > save_stack_trace_regs()
> >> > > applied), and I get many failures. Any idea ?
> >> > >
> >> > > [ 17.653751][ T58]
> >> > > ==
> >> > > [ 17.654379][ T58] BUG: KFENCE: invalid free in
> >> > > .kfence_guarded_free+0x2e4/0x530
> >> > > [ 17.654379][ T58]
> >> > > [ 17.654831][ T58] Invalid free of 0xc0003c9c (in
> >> > > kfence-#77):
> >> > > [ 17.655358][ T58] .kfence_guarded_free+0x2e4/0x530
> >> > > [ 17.655775][ T58] .__slab_free+0x320/0x5a0
> >> > > [ 17.656039][ T58] .test_double_free+0xe0/0x198
> >> > > [ 17.656308][ T58] .kunit_try_run_case+0x80/0x110
> >> > > [ 17.656523][ T58] .kunit_generic_run_threadfn_adapter+0x38/0x50
> >> > > [ 17.657161][ T58] .kthread+0x18c/0x1a0
> >> > > [ 17.659148][ T58] .ret_from_kernel_thread+0x58/0x70
> >> > > [ 17.659869][ T58]
> > [...]
> >> >
> >> > Looks like something is prepending '.' to function names. We expect
> >> > the function name to appear as-is, e.g. "kfence_guarded_free",
> >> > "test_double_free", etc.
> >> >
> >> > Is there something special on ppc64, where the '.' is some convention?
> >> >
> >>
> >> I think so, see
> >> https://refspecs.linuxfoundation.org/ELF/ppc64/PPC-elf64abi.html#FUNC-DES
> >>
> >> Also see commit https://github.com/linuxppc/linux/commit/02424d896
> >
> > Thanks -- could you try the below patch? You'll need to define
> > ARCH_FUNC_PREFIX accordingly.
> >
> > We think, since there are only very few architectures that add a prefix,
> > requiring to define something like ARCH_FUNC_PREFIX is
> > the simplest option. Let me know if this works for you.
> >
> > There an alternative option, which is to dynamically figure out the
> > prefix, but if this simpler option is fine with you, we'd prefer it.
>
> We have rediscovered this problem in basically every tracing / debugging
> feature added in the last 20 years :)
>
> I think the simplest solution is the one tools/perf/util/symbol.c uses,
> which is to just skip a leading '.'.
>
> Does that work?
>
> diff --git a/mm/kfence/report.c b/mm/kfence/report.c
> index ab83d5a59bb1..67b49dc54b38 100644
> --- a/mm/kfence/report.c
> +++ b/mm/kfence/report.c
> @@ -67,6 +67,9 @@ static int get_stack_skipnr(const unsigned long
> stack_entries[], int num_entries
> for (skipnr = 0; skipnr < num_entries; skipnr++) {
> int len = scnprintf(buf, sizeof(buf), "%ps", (void
> *)stack_entries[skipnr]);
>
> + if (buf[0] == '.')
> + buf++;
> +
Unfortunately this does not work, since buf is an array. We'd need an
offset, and it should be determined outside the loop. I had a solution
like this, but it turned out quite complex (see below). And since most
architectures do not require this, decided that the safest option is to
use the macro approach with ARCH_FUNC_PREFIX, for which Christophe
already prepared a patch and tested:
https://lore.kernel.org/linux-mm/20210304144000.1148590-1-el...@google.com/
https://lkml.kernel.org/r/afaec81a551ef15345cb7d7563b3fac3d7041c3a.1614868445.git.christophe.le...@csgroup.eu
Since KFENCE requires anyway, we'd prefer this approach
(vs. dynamically detecting).
Thanks,
-- Marco
-- >8 --
diff --git a/mm/kfence/report.c b/mm/kfence/report.c
index 519f037720f5..b0590199b039 100644
--- a/mm/kfence/report.c
+++ b/mm/kfence/report.c
@@ -43,8 +43,8 @@ static void seq_con_printf(struct seq_file *seq, const char
*fmt, ...)
static int get_stack_skipnr(const unsigned long stack_entries[], int
num_entries,
const enum kfence_error_type *type)
{
+ int skipnr, fallback = 0, fprefix_chars = 0;
char buf[64];
- int skipnr, fallback = 0;
if (type) {
/* Depending on error type, find different stack entries. */
@@ -64,11 +64,24 @@ static int get_stack_skipnr(const unsigned long
stack_entries[], int num_entries
}
}
+ if (scnprintf(buf, sizeof(buf), "%ps", (void *)kfree)) {
+ /*
+* Some architectures (e.g. ppc64) add a constant prefix to
+* function names. Determine if such a prefix exists.
+*/
+ const char *str =
Re: [RFC PATCH v1] powerpc: Enable KFENCE for PPC32
Marco Elver writes:
> On Thu, Mar 04, 2021 at 12:48PM +0100, Christophe Leroy wrote:
>> Le 04/03/2021 à 12:31, Marco Elver a écrit :
>> > On Thu, 4 Mar 2021 at 12:23, Christophe Leroy
>> > wrote:
>> > > Le 03/03/2021 à 11:56, Marco Elver a écrit :
>> > > >
>> > > > Somewhat tangentially, I also note that e.g. show_regs(regs) (which
>> > > > was printed along the KFENCE report above) didn't include the top
>> > > > frame in the "Call Trace", so this assumption is definitely not
>> > > > isolated to KFENCE.
>> > > >
>> > >
>> > > Now, I have tested PPC64 (with the patch I sent yesterday to modify
>> > > save_stack_trace_regs()
>> > > applied), and I get many failures. Any idea ?
>> > >
>> > > [ 17.653751][ T58]
>> > > ==
>> > > [ 17.654379][ T58] BUG: KFENCE: invalid free in
>> > > .kfence_guarded_free+0x2e4/0x530
>> > > [ 17.654379][ T58]
>> > > [ 17.654831][ T58] Invalid free of 0xc0003c9c (in
>> > > kfence-#77):
>> > > [ 17.655358][ T58] .kfence_guarded_free+0x2e4/0x530
>> > > [ 17.655775][ T58] .__slab_free+0x320/0x5a0
>> > > [ 17.656039][ T58] .test_double_free+0xe0/0x198
>> > > [ 17.656308][ T58] .kunit_try_run_case+0x80/0x110
>> > > [ 17.656523][ T58] .kunit_generic_run_threadfn_adapter+0x38/0x50
>> > > [ 17.657161][ T58] .kthread+0x18c/0x1a0
>> > > [ 17.659148][ T58] .ret_from_kernel_thread+0x58/0x70
>> > > [ 17.659869][ T58]
> [...]
>> >
>> > Looks like something is prepending '.' to function names. We expect
>> > the function name to appear as-is, e.g. "kfence_guarded_free",
>> > "test_double_free", etc.
>> >
>> > Is there something special on ppc64, where the '.' is some convention?
>> >
>>
>> I think so, see
>> https://refspecs.linuxfoundation.org/ELF/ppc64/PPC-elf64abi.html#FUNC-DES
>>
>> Also see commit https://github.com/linuxppc/linux/commit/02424d896
>
> Thanks -- could you try the below patch? You'll need to define
> ARCH_FUNC_PREFIX accordingly.
>
> We think, since there are only very few architectures that add a prefix,
> requiring to define something like ARCH_FUNC_PREFIX is
> the simplest option. Let me know if this works for you.
>
> There an alternative option, which is to dynamically figure out the
> prefix, but if this simpler option is fine with you, we'd prefer it.
We have rediscovered this problem in basically every tracing / debugging
feature added in the last 20 years :)
I think the simplest solution is the one tools/perf/util/symbol.c uses,
which is to just skip a leading '.'.
Does that work?
diff --git a/mm/kfence/report.c b/mm/kfence/report.c
index ab83d5a59bb1..67b49dc54b38 100644
--- a/mm/kfence/report.c
+++ b/mm/kfence/report.c
@@ -67,6 +67,9 @@ static int get_stack_skipnr(const unsigned long
stack_entries[], int num_entries
for (skipnr = 0; skipnr < num_entries; skipnr++) {
int len = scnprintf(buf, sizeof(buf), "%ps", (void
*)stack_entries[skipnr]);
+ if (buf[0] == '.')
+ buf++;
+
if (str_has_prefix(buf, "kfence_") || str_has_prefix(buf,
"__kfence_") ||
!strncmp(buf, "__slab_free", len)) {
/*
cheers
Re: [RFC PATCH v1] powerpc: Enable KFENCE for PPC32
On Thu, 4 Mar 2021 at 15:08, Christophe Leroy
wrote:
>
>
>
> Le 04/03/2021 à 13:48, Marco Elver a écrit :
> > From d118080eb9552073f5dcf1f86198f3d86d5ea850 Mon Sep 17 00:00:00 2001
> > From: Marco Elver
> > Date: Thu, 4 Mar 2021 13:15:51 +0100
> > Subject: [PATCH] kfence: fix reports if constant function prefixes exist
> >
> > Some architectures prefix all functions with a constant string ('.' on
> > ppc64). Add ARCH_FUNC_PREFIX, which may optionally be defined in
> > , so that get_stack_skipnr() can work properly.
>
>
> It works, thanks.
>
> >
> > Link:
> > https://lkml.kernel.org/r/f036c53d-7e81-763c-47f4-6024c6c5f...@csgroup.eu
> > Reported-by: Christophe Leroy
> > Signed-off-by: Marco Elver
>
> Tested-by: Christophe Leroy
Thanks, I'll send this to Andrew for inclusion in -mm, since this is
not a strict dependency (it'll work without the patch, just the stack
traces aren't that pretty but still useful). If the ppc patches and
this make it into the next merge window, everything should be good for
5.13.
> > ---
> > mm/kfence/report.c | 18 --
> > 1 file changed, 12 insertions(+), 6 deletions(-)
> >
> > diff --git a/mm/kfence/report.c b/mm/kfence/report.c
> > index 519f037720f5..e3f71451ad9e 100644
> > --- a/mm/kfence/report.c
> > +++ b/mm/kfence/report.c
> > @@ -20,6 +20,11 @@
> >
> > #include "kfence.h"
> >
> > +/* May be overridden by . */
> > +#ifndef ARCH_FUNC_PREFIX
> > +#define ARCH_FUNC_PREFIX ""
> > +#endif
> > +
> > extern bool no_hash_pointers;
> >
> > /* Helper function to either print to a seq_file or to console. */
> > @@ -67,8 +72,9 @@ static int get_stack_skipnr(const unsigned long
> > stack_entries[], int num_entries
> > for (skipnr = 0; skipnr < num_entries; skipnr++) {
> > int len = scnprintf(buf, sizeof(buf), "%ps", (void
> > *)stack_entries[skipnr]);
> >
> > - if (str_has_prefix(buf, "kfence_") || str_has_prefix(buf,
> > "__kfence_") ||
> > - !strncmp(buf, "__slab_free", len)) {
> > + if (str_has_prefix(buf, ARCH_FUNC_PREFIX "kfence_") ||
> > + str_has_prefix(buf, ARCH_FUNC_PREFIX "__kfence_") ||
> > + !strncmp(buf, ARCH_FUNC_PREFIX "__slab_free", len)) {
> > /*
> >* In case of tail calls from any of the below
> >* to any of the above.
> > @@ -77,10 +83,10 @@ static int get_stack_skipnr(const unsigned long
> > stack_entries[], int num_entries
> > }
> >
> > /* Also the *_bulk() variants by only checking prefixes. */
> > - if (str_has_prefix(buf, "kfree") ||
> > - str_has_prefix(buf, "kmem_cache_free") ||
> > - str_has_prefix(buf, "__kmalloc") ||
> > - str_has_prefix(buf, "kmem_cache_alloc"))
> > + if (str_has_prefix(buf, ARCH_FUNC_PREFIX "kfree") ||
> > + str_has_prefix(buf, ARCH_FUNC_PREFIX "kmem_cache_free") ||
> > + str_has_prefix(buf, ARCH_FUNC_PREFIX "__kmalloc") ||
> > + str_has_prefix(buf, ARCH_FUNC_PREFIX "kmem_cache_alloc"))
> > goto found;
> > }
> > if (fallback < num_entries)
> >
Re: [RFC PATCH v1] powerpc: Enable KFENCE for PPC32
Le 04/03/2021 à 13:48, Marco Elver a écrit :
From d118080eb9552073f5dcf1f86198f3d86d5ea850 Mon Sep 17 00:00:00 2001
From: Marco Elver
Date: Thu, 4 Mar 2021 13:15:51 +0100
Subject: [PATCH] kfence: fix reports if constant function prefixes exist
Some architectures prefix all functions with a constant string ('.' on
ppc64). Add ARCH_FUNC_PREFIX, which may optionally be defined in
, so that get_stack_skipnr() can work properly.
It works, thanks.
Link: https://lkml.kernel.org/r/f036c53d-7e81-763c-47f4-6024c6c5f...@csgroup.eu
Reported-by: Christophe Leroy
Signed-off-by: Marco Elver
Tested-by: Christophe Leroy
---
mm/kfence/report.c | 18 --
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/mm/kfence/report.c b/mm/kfence/report.c
index 519f037720f5..e3f71451ad9e 100644
--- a/mm/kfence/report.c
+++ b/mm/kfence/report.c
@@ -20,6 +20,11 @@
#include "kfence.h"
+/* May be overridden by . */
+#ifndef ARCH_FUNC_PREFIX
+#define ARCH_FUNC_PREFIX ""
+#endif
+
extern bool no_hash_pointers;
/* Helper function to either print to a seq_file or to console. */
@@ -67,8 +72,9 @@ static int get_stack_skipnr(const unsigned long
stack_entries[], int num_entries
for (skipnr = 0; skipnr < num_entries; skipnr++) {
int len = scnprintf(buf, sizeof(buf), "%ps", (void
*)stack_entries[skipnr]);
- if (str_has_prefix(buf, "kfence_") || str_has_prefix(buf, "__kfence_") ||
- !strncmp(buf, "__slab_free", len)) {
+ if (str_has_prefix(buf, ARCH_FUNC_PREFIX "kfence_") ||
+ str_has_prefix(buf, ARCH_FUNC_PREFIX "__kfence_") ||
+ !strncmp(buf, ARCH_FUNC_PREFIX "__slab_free", len)) {
/*
* In case of tail calls from any of the below
* to any of the above.
@@ -77,10 +83,10 @@ static int get_stack_skipnr(const unsigned long
stack_entries[], int num_entries
}
/* Also the *_bulk() variants by only checking prefixes. */
- if (str_has_prefix(buf, "kfree") ||
- str_has_prefix(buf, "kmem_cache_free") ||
- str_has_prefix(buf, "__kmalloc") ||
- str_has_prefix(buf, "kmem_cache_alloc"))
+ if (str_has_prefix(buf, ARCH_FUNC_PREFIX "kfree") ||
+ str_has_prefix(buf, ARCH_FUNC_PREFIX "kmem_cache_free") ||
+ str_has_prefix(buf, ARCH_FUNC_PREFIX "__kmalloc") ||
+ str_has_prefix(buf, ARCH_FUNC_PREFIX "kmem_cache_alloc"))
goto found;
}
if (fallback < num_entries)
Re: [RFC PATCH v1] powerpc: Enable KFENCE for PPC32
On Thu, Mar 04, 2021 at 12:48PM +0100, Christophe Leroy wrote:
> Le 04/03/2021 à 12:31, Marco Elver a écrit :
> > On Thu, 4 Mar 2021 at 12:23, Christophe Leroy
> > wrote:
> > > Le 03/03/2021 à 11:56, Marco Elver a écrit :
> > > >
> > > > Somewhat tangentially, I also note that e.g. show_regs(regs) (which
> > > > was printed along the KFENCE report above) didn't include the top
> > > > frame in the "Call Trace", so this assumption is definitely not
> > > > isolated to KFENCE.
> > > >
> > >
> > > Now, I have tested PPC64 (with the patch I sent yesterday to modify
> > > save_stack_trace_regs()
> > > applied), and I get many failures. Any idea ?
> > >
> > > [ 17.653751][ T58]
> > > ==
> > > [ 17.654379][ T58] BUG: KFENCE: invalid free in
> > > .kfence_guarded_free+0x2e4/0x530
> > > [ 17.654379][ T58]
> > > [ 17.654831][ T58] Invalid free of 0xc0003c9c (in kfence-#77):
> > > [ 17.655358][ T58] .kfence_guarded_free+0x2e4/0x530
> > > [ 17.655775][ T58] .__slab_free+0x320/0x5a0
> > > [ 17.656039][ T58] .test_double_free+0xe0/0x198
> > > [ 17.656308][ T58] .kunit_try_run_case+0x80/0x110
> > > [ 17.656523][ T58] .kunit_generic_run_threadfn_adapter+0x38/0x50
> > > [ 17.657161][ T58] .kthread+0x18c/0x1a0
> > > [ 17.659148][ T58] .ret_from_kernel_thread+0x58/0x70
> > > [ 17.659869][ T58]
[...]
> >
> > Looks like something is prepending '.' to function names. We expect
> > the function name to appear as-is, e.g. "kfence_guarded_free",
> > "test_double_free", etc.
> >
> > Is there something special on ppc64, where the '.' is some convention?
> >
>
> I think so, see
> https://refspecs.linuxfoundation.org/ELF/ppc64/PPC-elf64abi.html#FUNC-DES
>
> Also see commit https://github.com/linuxppc/linux/commit/02424d896
Thanks -- could you try the below patch? You'll need to define
ARCH_FUNC_PREFIX accordingly.
We think, since there are only very few architectures that add a prefix,
requiring to define something like ARCH_FUNC_PREFIX is
the simplest option. Let me know if this works for you.
There an alternative option, which is to dynamically figure out the
prefix, but if this simpler option is fine with you, we'd prefer it.
Thanks,
-- Marco
-- >8 --
>From d118080eb9552073f5dcf1f86198f3d86d5ea850 Mon Sep 17 00:00:00 2001
From: Marco Elver
Date: Thu, 4 Mar 2021 13:15:51 +0100
Subject: [PATCH] kfence: fix reports if constant function prefixes exist
Some architectures prefix all functions with a constant string ('.' on
ppc64). Add ARCH_FUNC_PREFIX, which may optionally be defined in
, so that get_stack_skipnr() can work properly.
Link: https://lkml.kernel.org/r/f036c53d-7e81-763c-47f4-6024c6c5f...@csgroup.eu
Reported-by: Christophe Leroy
Signed-off-by: Marco Elver
---
mm/kfence/report.c | 18 --
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/mm/kfence/report.c b/mm/kfence/report.c
index 519f037720f5..e3f71451ad9e 100644
--- a/mm/kfence/report.c
+++ b/mm/kfence/report.c
@@ -20,6 +20,11 @@
#include "kfence.h"
+/* May be overridden by . */
+#ifndef ARCH_FUNC_PREFIX
+#define ARCH_FUNC_PREFIX ""
+#endif
+
extern bool no_hash_pointers;
/* Helper function to either print to a seq_file or to console. */
@@ -67,8 +72,9 @@ static int get_stack_skipnr(const unsigned long
stack_entries[], int num_entries
for (skipnr = 0; skipnr < num_entries; skipnr++) {
int len = scnprintf(buf, sizeof(buf), "%ps", (void
*)stack_entries[skipnr]);
- if (str_has_prefix(buf, "kfence_") || str_has_prefix(buf,
"__kfence_") ||
- !strncmp(buf, "__slab_free", len)) {
+ if (str_has_prefix(buf, ARCH_FUNC_PREFIX "kfence_") ||
+ str_has_prefix(buf, ARCH_FUNC_PREFIX "__kfence_") ||
+ !strncmp(buf, ARCH_FUNC_PREFIX "__slab_free", len)) {
/*
* In case of tail calls from any of the below
* to any of the above.
@@ -77,10 +83,10 @@ static int get_stack_skipnr(const unsigned long
stack_entries[], int num_entries
}
/* Also the *_bulk() variants by only checking prefixes. */
- if (str_has_prefix(buf, "kfree") ||
- str_has_prefix(buf, "kmem_cache_free") ||
- str_has_prefix(buf, "__kmalloc") ||
- str_has_prefix(buf, "kmem_cache_alloc"))
+ if (str_has_prefix(buf, ARCH_FUNC_PREFIX "kfree") ||
+ str_has_prefix(buf, ARCH_FUNC_PREFIX "kmem_cache_free") ||
+ str_has_prefix(buf, ARCH_FUNC_PREFIX "__kmalloc") ||
+ str_has_prefix(buf, ARCH_FUNC_PREFIX "kmem_cache_alloc"))
goto found;
}
if (fallback < num_entries)
--
2.30.1.766.gb4fecdf3b7-goog
Re: [RFC PATCH v1] powerpc: Enable KFENCE for PPC32
On Thu, 4 Mar 2021 at 13:00, Christophe Leroy wrote: > > > > Le 04/03/2021 à 12:48, Christophe Leroy a écrit : > > > > > > Le 04/03/2021 à 12:31, Marco Elver a écrit : > >> On Thu, 4 Mar 2021 at 12:23, Christophe Leroy > >> wrote: > >>> Le 03/03/2021 à 11:56, Marco Elver a écrit : > > Somewhat tangentially, I also note that e.g. show_regs(regs) (which > was printed along the KFENCE report above) didn't include the top > frame in the "Call Trace", so this assumption is definitely not > isolated to KFENCE. > > >>> > >>> Now, I have tested PPC64 (with the patch I sent yesterday to modify > >>> save_stack_trace_regs() > >>> applied), and I get many failures. Any idea ? > >>> > >>> [ 17.653751][ T58] > >>> == > >>> [ 17.654379][ T58] BUG: KFENCE: invalid free in > >>> .kfence_guarded_free+0x2e4/0x530 > >>> [ 17.654379][ T58] > >>> [ 17.654831][ T58] Invalid free of 0xc0003c9c (in kfence-#77): > >>> [ 17.655358][ T58] .kfence_guarded_free+0x2e4/0x530 > >>> [ 17.655775][ T58] .__slab_free+0x320/0x5a0 > >>> [ 17.656039][ T58] .test_double_free+0xe0/0x198 > >>> [ 17.656308][ T58] .kunit_try_run_case+0x80/0x110 > >>> [ 17.656523][ T58] .kunit_generic_run_threadfn_adapter+0x38/0x50 > >>> [ 17.657161][ T58] .kthread+0x18c/0x1a0 > >>> [ 17.659148][ T58] .ret_from_kernel_thread+0x58/0x70 > >>> [ 17.659869][ T58] > >>> [ 17.663954][ T58] kfence-#77 [0xc0003c9c-0xc0003c9c001f, > >>> size=32, cache=kmalloc-32] > >>> allocated by task 58: > >>> [ 17.666113][ T58] .__kfence_alloc+0x1bc/0x510 > >>> [ 17.667069][ T58] .__kmalloc+0x280/0x4f0 > >>> [ 17.667452][ T58] .test_alloc+0x19c/0x430 > >>> [ 17.667732][ T58] .test_double_free+0x88/0x198 > >>> [ 17.667971][ T58] .kunit_try_run_case+0x80/0x110 > >>> [ 17.668283][ T58] .kunit_generic_run_threadfn_adapter+0x38/0x50 > >>> [ 17.668553][ T58] .kthread+0x18c/0x1a0 > >>> [ 17.669315][ T58] .ret_from_kernel_thread+0x58/0x70 > >>> [ 17.669711][ T58] > >>> [ 17.669711][ T58] freed by task 58: > >>> [ 17.670116][ T58] .kfence_guarded_free+0x3d0/0x530 > >>> [ 17.670421][ T58] .__slab_free+0x320/0x5a0 > >>> [ 17.670603][ T58] .test_double_free+0xb4/0x198 > >>> [ 17.670827][ T58] .kunit_try_run_case+0x80/0x110 > >>> [ 17.671073][ T58] .kunit_generic_run_threadfn_adapter+0x38/0x50 > >>> [ 17.671410][ T58] .kthread+0x18c/0x1a0 > >>> [ 17.671618][ T58] .ret_from_kernel_thread+0x58/0x70 > >>> [ 17.671972][ T58] > >>> [ 17.672638][ T58] CPU: 0 PID: 58 Comm: kunit_try_catch Tainted: G > >>> B > >>> 5.12.0-rc1-01540-g0783285cc1b8-dirty #4685 > >>> [ 17.673768][ T58] > >>> == > >>> [ 17.677031][ T58] # test_double_free: EXPECTATION FAILED at > >>> mm/kfence/kfence_test.c:380 > >>> [ 17.677031][ T58] Expected report_matches() to be true, > >>> but is false > >>> [ 17.684397][T1] not ok 7 - test_double_free > >>> [ 17.686463][ T59] # test_double_free-memcache: setup_test_cache: > >>> size=32, ctor=0x0 > >>> [ 17.688403][ T59] # test_double_free-memcache: test_alloc: > >>> size=32, gfp=cc0, policy=any, > >>> cache=1 > >> > >> Looks like something is prepending '.' to function names. We expect > >> the function name to appear as-is, e.g. "kfence_guarded_free", > >> "test_double_free", etc. > >> > >> Is there something special on ppc64, where the '.' is some convention? > >> > > > > I think so, see > > https://refspecs.linuxfoundation.org/ELF/ppc64/PPC-elf64abi.html#FUNC-DES > > > > Also see commit https://github.com/linuxppc/linux/commit/02424d896 > > > > But I'm wondering, if the dot is the problem, how so is the following one ok ? > > [ 79.574457][ T75] # test_krealloc: test_alloc: size=32, gfp=cc0, > policy=any, cache=0 > [ 79.682728][ T75] > == > [ 79.684017][ T75] BUG: KFENCE: use-after-free read in > .test_krealloc+0x4fc/0x5b8 > [ 79.684017][ T75] > [ 79.684955][ T75] Use-after-free read at 0xc0003d06 (in > kfence-#130): > [ 79.687581][ T75] .test_krealloc+0x4fc/0x5b8 > [ 79.688216][ T75] .test_krealloc+0x4e4/0x5b8 > [ 79.688824][ T75] .kunit_try_run_case+0x80/0x110 > [ 79.689737][ T75] .kunit_generic_run_threadfn_adapter+0x38/0x50 > [ 79.690335][ T75] .kthread+0x18c/0x1a0 > [ 79.691092][ T75] .ret_from_kernel_thread+0x58/0x70 > [ 79.692081][ T75] > [ 79.692671][ T75] kfence-#130 [0xc0003d06-0xc0003d06001f, > size=32, > cache=kmalloc-32] allocated by task 75: > [ 79.700977][ T75] .__kfence_alloc+0x1bc/0x510 > [ 79.701812][ T75] .__kmalloc+0x280/0x4f0 > [ 79.702695][ T75] .test_alloc+0x19c/0x430 > [ 79.703051][ T75] .test_krealloc+0xa8/0x5b8 >
Re: [RFC PATCH v1] powerpc: Enable KFENCE for PPC32
Le 04/03/2021 à 12:48, Christophe Leroy a écrit : Le 04/03/2021 à 12:31, Marco Elver a écrit : On Thu, 4 Mar 2021 at 12:23, Christophe Leroy wrote: Le 03/03/2021 à 11:56, Marco Elver a écrit : Somewhat tangentially, I also note that e.g. show_regs(regs) (which was printed along the KFENCE report above) didn't include the top frame in the "Call Trace", so this assumption is definitely not isolated to KFENCE. Now, I have tested PPC64 (with the patch I sent yesterday to modify save_stack_trace_regs() applied), and I get many failures. Any idea ? [ 17.653751][ T58] == [ 17.654379][ T58] BUG: KFENCE: invalid free in .kfence_guarded_free+0x2e4/0x530 [ 17.654379][ T58] [ 17.654831][ T58] Invalid free of 0xc0003c9c (in kfence-#77): [ 17.655358][ T58] .kfence_guarded_free+0x2e4/0x530 [ 17.655775][ T58] .__slab_free+0x320/0x5a0 [ 17.656039][ T58] .test_double_free+0xe0/0x198 [ 17.656308][ T58] .kunit_try_run_case+0x80/0x110 [ 17.656523][ T58] .kunit_generic_run_threadfn_adapter+0x38/0x50 [ 17.657161][ T58] .kthread+0x18c/0x1a0 [ 17.659148][ T58] .ret_from_kernel_thread+0x58/0x70 [ 17.659869][ T58] [ 17.663954][ T58] kfence-#77 [0xc0003c9c-0xc0003c9c001f, size=32, cache=kmalloc-32] allocated by task 58: [ 17.666113][ T58] .__kfence_alloc+0x1bc/0x510 [ 17.667069][ T58] .__kmalloc+0x280/0x4f0 [ 17.667452][ T58] .test_alloc+0x19c/0x430 [ 17.667732][ T58] .test_double_free+0x88/0x198 [ 17.667971][ T58] .kunit_try_run_case+0x80/0x110 [ 17.668283][ T58] .kunit_generic_run_threadfn_adapter+0x38/0x50 [ 17.668553][ T58] .kthread+0x18c/0x1a0 [ 17.669315][ T58] .ret_from_kernel_thread+0x58/0x70 [ 17.669711][ T58] [ 17.669711][ T58] freed by task 58: [ 17.670116][ T58] .kfence_guarded_free+0x3d0/0x530 [ 17.670421][ T58] .__slab_free+0x320/0x5a0 [ 17.670603][ T58] .test_double_free+0xb4/0x198 [ 17.670827][ T58] .kunit_try_run_case+0x80/0x110 [ 17.671073][ T58] .kunit_generic_run_threadfn_adapter+0x38/0x50 [ 17.671410][ T58] .kthread+0x18c/0x1a0 [ 17.671618][ T58] .ret_from_kernel_thread+0x58/0x70 [ 17.671972][ T58] [ 17.672638][ T58] CPU: 0 PID: 58 Comm: kunit_try_catch Tainted: G B 5.12.0-rc1-01540-g0783285cc1b8-dirty #4685 [ 17.673768][ T58] == [ 17.677031][ T58] # test_double_free: EXPECTATION FAILED at mm/kfence/kfence_test.c:380 [ 17.677031][ T58] Expected report_matches() to be true, but is false [ 17.684397][ T1] not ok 7 - test_double_free [ 17.686463][ T59] # test_double_free-memcache: setup_test_cache: size=32, ctor=0x0 [ 17.688403][ T59] # test_double_free-memcache: test_alloc: size=32, gfp=cc0, policy=any, cache=1 Looks like something is prepending '.' to function names. We expect the function name to appear as-is, e.g. "kfence_guarded_free", "test_double_free", etc. Is there something special on ppc64, where the '.' is some convention? I think so, see https://refspecs.linuxfoundation.org/ELF/ppc64/PPC-elf64abi.html#FUNC-DES Also see commit https://github.com/linuxppc/linux/commit/02424d896 But I'm wondering, if the dot is the problem, how so is the following one ok ? [ 79.574457][ T75] # test_krealloc: test_alloc: size=32, gfp=cc0, policy=any, cache=0 [ 79.682728][ T75] == [ 79.684017][ T75] BUG: KFENCE: use-after-free read in .test_krealloc+0x4fc/0x5b8 [ 79.684017][ T75] [ 79.684955][ T75] Use-after-free read at 0xc0003d06 (in kfence-#130): [ 79.687581][ T75] .test_krealloc+0x4fc/0x5b8 [ 79.688216][ T75] .test_krealloc+0x4e4/0x5b8 [ 79.688824][ T75] .kunit_try_run_case+0x80/0x110 [ 79.689737][ T75] .kunit_generic_run_threadfn_adapter+0x38/0x50 [ 79.690335][ T75] .kthread+0x18c/0x1a0 [ 79.691092][ T75] .ret_from_kernel_thread+0x58/0x70 [ 79.692081][ T75] [ 79.692671][ T75] kfence-#130 [0xc0003d06-0xc0003d06001f, size=32, cache=kmalloc-32] allocated by task 75: [ 79.700977][ T75] .__kfence_alloc+0x1bc/0x510 [ 79.701812][ T75] .__kmalloc+0x280/0x4f0 [ 79.702695][ T75] .test_alloc+0x19c/0x430 [ 79.703051][ T75] .test_krealloc+0xa8/0x5b8 [ 79.703276][ T75] .kunit_try_run_case+0x80/0x110 [ 79.703693][ T75] .kunit_generic_run_threadfn_adapter+0x38/0x50 [ 79.704223][ T75] .kthread+0x18c/0x1a0 [ 79.704586][ T75] .ret_from_kernel_thread+0x58/0x70 [ 79.704968][ T75] [ 79.704968][ T75] freed by task 75: [ 79.705756][ T75] .kfence_guarded_free+0x3d0/0x530 [ 79.706754][ T75] .__slab_free+0x320/0x5a0 [ 79.708575][ T75] .krealloc+0xe8/0x180 [ 79.708970][ T75] .test_krealloc+0x1c8/0x5b8 [ 79.709606][ T75] .kunit_try_run_case+0x80/0x110
Re: [RFC PATCH v1] powerpc: Enable KFENCE for PPC32
Le 04/03/2021 à 12:31, Marco Elver a écrit : On Thu, 4 Mar 2021 at 12:23, Christophe Leroy wrote: Le 03/03/2021 à 11:56, Marco Elver a écrit : Somewhat tangentially, I also note that e.g. show_regs(regs) (which was printed along the KFENCE report above) didn't include the top frame in the "Call Trace", so this assumption is definitely not isolated to KFENCE. Now, I have tested PPC64 (with the patch I sent yesterday to modify save_stack_trace_regs() applied), and I get many failures. Any idea ? [ 17.653751][ T58] == [ 17.654379][ T58] BUG: KFENCE: invalid free in .kfence_guarded_free+0x2e4/0x530 [ 17.654379][ T58] [ 17.654831][ T58] Invalid free of 0xc0003c9c (in kfence-#77): [ 17.655358][ T58] .kfence_guarded_free+0x2e4/0x530 [ 17.655775][ T58] .__slab_free+0x320/0x5a0 [ 17.656039][ T58] .test_double_free+0xe0/0x198 [ 17.656308][ T58] .kunit_try_run_case+0x80/0x110 [ 17.656523][ T58] .kunit_generic_run_threadfn_adapter+0x38/0x50 [ 17.657161][ T58] .kthread+0x18c/0x1a0 [ 17.659148][ T58] .ret_from_kernel_thread+0x58/0x70 [ 17.659869][ T58] [ 17.663954][ T58] kfence-#77 [0xc0003c9c-0xc0003c9c001f, size=32, cache=kmalloc-32] allocated by task 58: [ 17.666113][ T58] .__kfence_alloc+0x1bc/0x510 [ 17.667069][ T58] .__kmalloc+0x280/0x4f0 [ 17.667452][ T58] .test_alloc+0x19c/0x430 [ 17.667732][ T58] .test_double_free+0x88/0x198 [ 17.667971][ T58] .kunit_try_run_case+0x80/0x110 [ 17.668283][ T58] .kunit_generic_run_threadfn_adapter+0x38/0x50 [ 17.668553][ T58] .kthread+0x18c/0x1a0 [ 17.669315][ T58] .ret_from_kernel_thread+0x58/0x70 [ 17.669711][ T58] [ 17.669711][ T58] freed by task 58: [ 17.670116][ T58] .kfence_guarded_free+0x3d0/0x530 [ 17.670421][ T58] .__slab_free+0x320/0x5a0 [ 17.670603][ T58] .test_double_free+0xb4/0x198 [ 17.670827][ T58] .kunit_try_run_case+0x80/0x110 [ 17.671073][ T58] .kunit_generic_run_threadfn_adapter+0x38/0x50 [ 17.671410][ T58] .kthread+0x18c/0x1a0 [ 17.671618][ T58] .ret_from_kernel_thread+0x58/0x70 [ 17.671972][ T58] [ 17.672638][ T58] CPU: 0 PID: 58 Comm: kunit_try_catch Tainted: GB 5.12.0-rc1-01540-g0783285cc1b8-dirty #4685 [ 17.673768][ T58] == [ 17.677031][ T58] # test_double_free: EXPECTATION FAILED at mm/kfence/kfence_test.c:380 [ 17.677031][ T58] Expected report_matches() to be true, but is false [ 17.684397][T1] not ok 7 - test_double_free [ 17.686463][ T59] # test_double_free-memcache: setup_test_cache: size=32, ctor=0x0 [ 17.688403][ T59] # test_double_free-memcache: test_alloc: size=32, gfp=cc0, policy=any, cache=1 Looks like something is prepending '.' to function names. We expect the function name to appear as-is, e.g. "kfence_guarded_free", "test_double_free", etc. Is there something special on ppc64, where the '.' is some convention? I think so, see https://refspecs.linuxfoundation.org/ELF/ppc64/PPC-elf64abi.html#FUNC-DES Also see commit https://github.com/linuxppc/linux/commit/02424d896 Christophe
Re: [RFC PATCH v1] powerpc: Enable KFENCE for PPC32
On Thu, 4 Mar 2021 at 12:23, Christophe Leroy wrote: > Le 03/03/2021 à 11:56, Marco Elver a écrit : > > > > Somewhat tangentially, I also note that e.g. show_regs(regs) (which > > was printed along the KFENCE report above) didn't include the top > > frame in the "Call Trace", so this assumption is definitely not > > isolated to KFENCE. > > > > Now, I have tested PPC64 (with the patch I sent yesterday to modify > save_stack_trace_regs() > applied), and I get many failures. Any idea ? > > [ 17.653751][ T58] > == > [ 17.654379][ T58] BUG: KFENCE: invalid free in > .kfence_guarded_free+0x2e4/0x530 > [ 17.654379][ T58] > [ 17.654831][ T58] Invalid free of 0xc0003c9c (in kfence-#77): > [ 17.655358][ T58] .kfence_guarded_free+0x2e4/0x530 > [ 17.655775][ T58] .__slab_free+0x320/0x5a0 > [ 17.656039][ T58] .test_double_free+0xe0/0x198 > [ 17.656308][ T58] .kunit_try_run_case+0x80/0x110 > [ 17.656523][ T58] .kunit_generic_run_threadfn_adapter+0x38/0x50 > [ 17.657161][ T58] .kthread+0x18c/0x1a0 > [ 17.659148][ T58] .ret_from_kernel_thread+0x58/0x70 > [ 17.659869][ T58] > [ 17.663954][ T58] kfence-#77 [0xc0003c9c-0xc0003c9c001f, > size=32, cache=kmalloc-32] > allocated by task 58: > [ 17.666113][ T58] .__kfence_alloc+0x1bc/0x510 > [ 17.667069][ T58] .__kmalloc+0x280/0x4f0 > [ 17.667452][ T58] .test_alloc+0x19c/0x430 > [ 17.667732][ T58] .test_double_free+0x88/0x198 > [ 17.667971][ T58] .kunit_try_run_case+0x80/0x110 > [ 17.668283][ T58] .kunit_generic_run_threadfn_adapter+0x38/0x50 > [ 17.668553][ T58] .kthread+0x18c/0x1a0 > [ 17.669315][ T58] .ret_from_kernel_thread+0x58/0x70 > [ 17.669711][ T58] > [ 17.669711][ T58] freed by task 58: > [ 17.670116][ T58] .kfence_guarded_free+0x3d0/0x530 > [ 17.670421][ T58] .__slab_free+0x320/0x5a0 > [ 17.670603][ T58] .test_double_free+0xb4/0x198 > [ 17.670827][ T58] .kunit_try_run_case+0x80/0x110 > [ 17.671073][ T58] .kunit_generic_run_threadfn_adapter+0x38/0x50 > [ 17.671410][ T58] .kthread+0x18c/0x1a0 > [ 17.671618][ T58] .ret_from_kernel_thread+0x58/0x70 > [ 17.671972][ T58] > [ 17.672638][ T58] CPU: 0 PID: 58 Comm: kunit_try_catch Tainted: GB > 5.12.0-rc1-01540-g0783285cc1b8-dirty #4685 > [ 17.673768][ T58] > == > [ 17.677031][ T58] # test_double_free: EXPECTATION FAILED at > mm/kfence/kfence_test.c:380 > [ 17.677031][ T58] Expected report_matches() to be true, but > is false > [ 17.684397][T1] not ok 7 - test_double_free > [ 17.686463][ T59] # test_double_free-memcache: setup_test_cache: > size=32, ctor=0x0 > [ 17.688403][ T59] # test_double_free-memcache: test_alloc: size=32, > gfp=cc0, policy=any, > cache=1 Looks like something is prepending '.' to function names. We expect the function name to appear as-is, e.g. "kfence_guarded_free", "test_double_free", etc. Is there something special on ppc64, where the '.' is some convention? Thanks, -- Marco
Re: [RFC PATCH v1] powerpc: Enable KFENCE for PPC32
Le 03/03/2021 à 11:56, Marco Elver a écrit : Somewhat tangentially, I also note that e.g. show_regs(regs) (which was printed along the KFENCE report above) didn't include the top frame in the "Call Trace", so this assumption is definitely not isolated to KFENCE. Now, I have tested PPC64 (with the patch I sent yesterday to modify save_stack_trace_regs() applied), and I get many failures. Any idea ? [ 17.653751][ T58] == [ 17.654379][ T58] BUG: KFENCE: invalid free in .kfence_guarded_free+0x2e4/0x530 [ 17.654379][ T58] [ 17.654831][ T58] Invalid free of 0xc0003c9c (in kfence-#77): [ 17.655358][ T58] .kfence_guarded_free+0x2e4/0x530 [ 17.655775][ T58] .__slab_free+0x320/0x5a0 [ 17.656039][ T58] .test_double_free+0xe0/0x198 [ 17.656308][ T58] .kunit_try_run_case+0x80/0x110 [ 17.656523][ T58] .kunit_generic_run_threadfn_adapter+0x38/0x50 [ 17.657161][ T58] .kthread+0x18c/0x1a0 [ 17.659148][ T58] .ret_from_kernel_thread+0x58/0x70 [ 17.659869][ T58] [ 17.663954][ T58] kfence-#77 [0xc0003c9c-0xc0003c9c001f, size=32, cache=kmalloc-32] allocated by task 58: [ 17.666113][ T58] .__kfence_alloc+0x1bc/0x510 [ 17.667069][ T58] .__kmalloc+0x280/0x4f0 [ 17.667452][ T58] .test_alloc+0x19c/0x430 [ 17.667732][ T58] .test_double_free+0x88/0x198 [ 17.667971][ T58] .kunit_try_run_case+0x80/0x110 [ 17.668283][ T58] .kunit_generic_run_threadfn_adapter+0x38/0x50 [ 17.668553][ T58] .kthread+0x18c/0x1a0 [ 17.669315][ T58] .ret_from_kernel_thread+0x58/0x70 [ 17.669711][ T58] [ 17.669711][ T58] freed by task 58: [ 17.670116][ T58] .kfence_guarded_free+0x3d0/0x530 [ 17.670421][ T58] .__slab_free+0x320/0x5a0 [ 17.670603][ T58] .test_double_free+0xb4/0x198 [ 17.670827][ T58] .kunit_try_run_case+0x80/0x110 [ 17.671073][ T58] .kunit_generic_run_threadfn_adapter+0x38/0x50 [ 17.671410][ T58] .kthread+0x18c/0x1a0 [ 17.671618][ T58] .ret_from_kernel_thread+0x58/0x70 [ 17.671972][ T58] [ 17.672638][ T58] CPU: 0 PID: 58 Comm: kunit_try_catch Tainted: GB 5.12.0-rc1-01540-g0783285cc1b8-dirty #4685 [ 17.673768][ T58] == [ 17.677031][ T58] # test_double_free: EXPECTATION FAILED at mm/kfence/kfence_test.c:380 [ 17.677031][ T58] Expected report_matches() to be true, but is false [ 17.684397][T1] not ok 7 - test_double_free [ 17.686463][ T59] # test_double_free-memcache: setup_test_cache: size=32, ctor=0x0 [ 17.688403][ T59] # test_double_free-memcache: test_alloc: size=32, gfp=cc0, policy=any, cache=1 [ 17.797584][ T59] == [ 17.801260][ T59] BUG: KFENCE: invalid free in .kfence_guarded_free+0x2e4/0x530 [ 17.801260][ T59] [ 17.801512][ T59] Invalid free of 0xc0003c9effe0 (in kfence-#78): [ 17.801668][ T59] .kfence_guarded_free+0x2e4/0x530 [ 17.801849][ T59] .__slab_free+0x320/0x5a0 [ 17.801983][ T59] .kmem_cache_free+0x31c/0x5c0 [ 17.802109][ T59] .test_double_free+0xd0/0x198 [ 17.802227][ T59] .kunit_try_run_case+0x80/0x110 [ 17.802494][ T59] .kunit_generic_run_threadfn_adapter+0x38/0x50 [ 17.802641][ T59] .kthread+0x18c/0x1a0 [ 17.802821][ T59] .ret_from_kernel_thread+0x58/0x70 [ 17.802989][ T59] [ 17.803303][ T59] kfence-#78 [0xc0003c9effe0-0xc0003c9e, size=32, cache=test] allocated by task 59: [ 17.803666][ T59] .__kfence_alloc+0x1bc/0x510 [ 17.803898][ T59] .kmem_cache_alloc+0x290/0x440 [ 17.804036][ T59] .test_alloc+0x188/0x430 [ 17.804151][ T59] .test_double_free+0x88/0x198 [ 17.804363][ T59] .kunit_try_run_case+0x80/0x110 [ 17.804637][ T59] .kunit_generic_run_threadfn_adapter+0x38/0x50 [ 17.805099][ T59] .kthread+0x18c/0x1a0 [ 17.805313][ T59] .ret_from_kernel_thread+0x58/0x70 [ 17.806035][ T59] [ 17.806035][ T59] freed by task 59: [ 17.806495][ T59] .kfence_guarded_free+0x3d0/0x530 [ 17.806689][ T59] .__slab_free+0x320/0x5a0 [ 17.806941][ T59] .kmem_cache_free+0x31c/0x5c0 [ 17.807122][ T59] .test_double_free+0xa8/0x198 [ 17.807360][ T59] .kunit_try_run_case+0x80/0x110 [ 17.807538][ T59] .kunit_generic_run_threadfn_adapter+0x38/0x50 [ 17.807703][ T59] .kthread+0x18c/0x1a0 [ 17.808015][ T59] .ret_from_kernel_thread+0x58/0x70 [ 17.808220][ T59] [ 17.808406][ T59] CPU: 0 PID: 59 Comm: kunit_try_catch Tainted: GB 5.12.0-rc1-01540-g0783285cc1b8-dirty #4685 [ 17.808670][ T59] == [ 17.809882][ T59] # test_double_free-memcache: EXPECTATION FAILED at mm/kfence/kfence_test.c:380 [ 17.809882][ T59] Expected report_matches() to be true, but is false [
Re: [RFC PATCH v1] powerpc: Enable KFENCE for PPC32
Le 03/03/2021 à 11:39, Marco Elver a écrit : On Wed, 3 Mar 2021 at 11:32, Christophe Leroy wrote: Le 02/03/2021 à 10:53, Marco Elver a écrit : On Tue, 2 Mar 2021 at 10:27, Christophe Leroy wrote: Le 02/03/2021 à 10:21, Alexander Potapenko a écrit : [ 14.998426] BUG: KFENCE: invalid read in finish_task_switch.isra.0+0x54/0x23c [ 14.998426] [ 15.007061] Invalid read at 0x(ptrval): [ 15.010906] finish_task_switch.isra.0+0x54/0x23c [ 15.015633] kunit_try_run_case+0x5c/0xd0 [ 15.019682] kunit_generic_run_threadfn_adapter+0x24/0x30 [ 15.025099] kthread+0x15c/0x174 [ 15.028359] ret_from_kernel_thread+0x14/0x1c [ 15.032747] [ 15.034251] CPU: 0 PID: 111 Comm: kunit_try_catch Tainted: GB 5.12.0-rc1-s3k-dev-01534-g4f14ae75edf0-dirty #4674 [ 15.045811] == [ 15.053324] # test_invalid_access: EXPECTATION FAILED at mm/kfence/kfence_test.c:636 [ 15.053324] Expected report_matches() to be true, but is false [ 15.068359] not ok 21 - test_invalid_access The test expects the function name to be test_invalid_access, i. e. the first line should be "BUG: KFENCE: invalid read in test_invalid_access". The error reporting function unwinds the stack, skips a couple of "uninteresting" frames (https://elixir.bootlin.com/linux/v5.12-rc1/source/mm/kfence/report.c#L43) and uses the first "interesting" one frame to print the report header (https://elixir.bootlin.com/linux/v5.12-rc1/source/mm/kfence/report.c#L226). It's strange that test_invalid_access is missing altogether from the stack trace - is that expected? Can you try printing the whole stacktrace without skipping any frames to see if that function is there? Booting with 'no_hash_pointers" I get the following. Does it helps ? [ 16.837198] == [ 16.848521] BUG: KFENCE: invalid read in finish_task_switch.isra.0+0x54/0x23c [ 16.848521] [ 16.857158] Invalid read at 0xdf98800a: [ 16.861004] finish_task_switch.isra.0+0x54/0x23c [ 16.865731] kunit_try_run_case+0x5c/0xd0 [ 16.869780] kunit_generic_run_threadfn_adapter+0x24/0x30 [ 16.875199] kthread+0x15c/0x174 [ 16.878460] ret_from_kernel_thread+0x14/0x1c [ 16.882847] [ 16.884351] CPU: 0 PID: 111 Comm: kunit_try_catch Tainted: GB 5.12.0-rc1-s3k-dev-01534-g4f14ae75edf0-dirty #4674 [ 16.895908] NIP: c016eb8c LR: c02f50dc CTR: c016eb38 [ 16.900963] REGS: e2449d90 TRAP: 0301 Tainted: GB (5.12.0-rc1-s3k-dev-01534-g4f14ae75edf0-dirty) [ 16.911386] MSR: 9032 CR: 2204 XER: [ 16.918153] DAR: df98800a DSISR: 2000 [ 16.918153] GPR00: c02f50dc e2449e50 c1140d00 e100dd24 c084b13c 0008 c084b32b c016eb38 [ 16.918153] GPR08: c085 df988000 c0d1 e2449eb0 22000288 [ 16.936695] NIP [c016eb8c] test_invalid_access+0x54/0x108 [ 16.942125] LR [c02f50dc] kunit_try_run_case+0x5c/0xd0 [ 16.947292] Call Trace: [ 16.949746] [e2449e50] [c005a5ec] finish_task_switch.isra.0+0x54/0x23c (unreliable) The "(unreliable)" might be a clue that it's related to ppc32 stack unwinding. Any ppc expert know what this is about? [ 16.957443] [e2449eb0] [c02f50dc] kunit_try_run_case+0x5c/0xd0 [ 16.963319] [e2449ed0] [c02f63ec] kunit_generic_run_threadfn_adapter+0x24/0x30 [ 16.970574] [e2449ef0] [c004e710] kthread+0x15c/0x174 [ 16.975670] [e2449f30] [c001317c] ret_from_kernel_thread+0x14/0x1c [ 16.981896] Instruction dump: [ 16.984879] 8129d608 38e7eb38 81020280 911f004c 3900 995f0024 907f0028 90ff001c [ 16.992710] 3949000a 915f0020 3d40c0d1 3d00c085 <8929000a> 3908adb0 812a4b98 3d40c02f [ 17.000711] == [ 17.008223] # test_invalid_access: EXPECTATION FAILED at mm/kfence/kfence_test.c:636 [ 17.008223] Expected report_matches() to be true, but is false [ 17.023243] not ok 21 - test_invalid_access On a fault in test_invalid_access, KFENCE prints the stack trace based on the information in pt_regs. So we do not think there's anything we can do to improve stack printing pe-se. What's confusing is that it's only this test, and none of the others. Given that, it might be code-gen related, which results in some subtle issue with stack unwinding. There are a few things to try, if you feel like it: -- Change the unwinder, if it's possible for ppc32. -- Add code to test_invalid_access(), to get the compiler to emit different code. E.g. add a bunch (unnecessary) function calls, or add barriers, etc. -- Play with compiler options. We already pass -fno-optimize-sibling-calls for kfence_test.o to avoid tail-call optimizations that'd hide stack trace entries. But perhaps there's something ppc-specific we missed? Well, the good thing is that KFENCE detects the bad access just fine. Since, according to the test, everything works from KFENCE's side, I'd be happy to give my Ack: Acked-by: Marco
Re: [RFC PATCH v1] powerpc: Enable KFENCE for PPC32
On Wed, 3 Mar 2021 at 11:39, Christophe Leroy wrote: > > > > Le 02/03/2021 à 12:39, Marco Elver a écrit : > > On Tue, 2 Mar 2021 at 12:21, Christophe Leroy > > wrote: > > [...] > Booting with 'no_hash_pointers" I get the following. Does it helps ? > > [ 16.837198] > == > [ 16.848521] BUG: KFENCE: invalid read in > finish_task_switch.isra.0+0x54/0x23c > [ 16.848521] > [ 16.857158] Invalid read at 0xdf98800a: > [ 16.861004] finish_task_switch.isra.0+0x54/0x23c > [ 16.865731] kunit_try_run_case+0x5c/0xd0 > [ 16.869780] kunit_generic_run_threadfn_adapter+0x24/0x30 > [ 16.875199] kthread+0x15c/0x174 > [ 16.878460] ret_from_kernel_thread+0x14/0x1c > [ 16.882847] > [ 16.884351] CPU: 0 PID: 111 Comm: kunit_try_catch Tainted: GB > 5.12.0-rc1-s3k-dev-01534-g4f14ae75edf0-dirty #4674 > [ 16.895908] NIP: c016eb8c LR: c02f50dc CTR: c016eb38 > [ 16.900963] REGS: e2449d90 TRAP: 0301 Tainted: GB > (5.12.0-rc1-s3k-dev-01534-g4f14ae75edf0-dirty) > [ 16.911386] MSR: 9032 CR: 2204 XER: > > [ 16.918153] DAR: df98800a DSISR: 2000 > [ 16.918153] GPR00: c02f50dc e2449e50 c1140d00 e100dd24 c084b13c > 0008 c084b32b c016eb38 > [ 16.918153] GPR08: c085 df988000 c0d1 e2449eb0 22000288 > [ 16.936695] NIP [c016eb8c] test_invalid_access+0x54/0x108 > [ 16.942125] LR [c02f50dc] kunit_try_run_case+0x5c/0xd0 > [ 16.947292] Call Trace: > [ 16.949746] [e2449e50] [c005a5ec] > finish_task_switch.isra.0+0x54/0x23c (unreliable) > >>> > >>> The "(unreliable)" might be a clue that it's related to ppc32 stack > >>> unwinding. Any ppc expert know what this is about? > >>> > [ 16.957443] [e2449eb0] [c02f50dc] kunit_try_run_case+0x5c/0xd0 > [ 16.963319] [e2449ed0] [c02f63ec] > kunit_generic_run_threadfn_adapter+0x24/0x30 > [ 16.970574] [e2449ef0] [c004e710] kthread+0x15c/0x174 > [ 16.975670] [e2449f30] [c001317c] ret_from_kernel_thread+0x14/0x1c > [ 16.981896] Instruction dump: > [ 16.984879] 8129d608 38e7eb38 81020280 911f004c 3900 995f0024 > 907f0028 90ff001c > [ 16.992710] 3949000a 915f0020 3d40c0d1 3d00c085 <8929000a> 3908adb0 > 812a4b98 3d40c02f > [ 17.000711] > == > [ 17.008223] # test_invalid_access: EXPECTATION FAILED at > mm/kfence/kfence_test.c:636 > [ 17.008223] Expected report_matches() to be true, but is > false > [ 17.023243] not ok 21 - test_invalid_access > >>> > >>> On a fault in test_invalid_access, KFENCE prints the stack trace based > >>> on the information in pt_regs. So we do not think there's anything we > >>> can do to improve stack printing pe-se. > >> > >> stack printing, probably not. Would be good anyway to mark the last level > >> [unreliable] as the ppc does. > > > > We use stack_trace_save_regs() + stack_trace_print(). > > > >> IIUC, on ppc the address in the stack frame of the caller is written by > >> the caller. In most tests, > >> there is some function call being done before the fault, for instance > >> test_kmalloc_aligned_oob_read() does a call to kunit_do_assertion which > >> populates the address of the > >> call in the stack. However this is fragile. > > > > Interesting, this might explain it. > > > >> This works for function calls because in order to call a subfunction, a > >> function has to set up a > >> stack frame in order to same the value in the Link Register, which > >> contains the address of the > >> function's parent and that will be clobbered by the sub-function call. > >> > >> However, it cannot be done by exceptions, because exceptions can happen in > >> a function that has no > >> stack frame (because that function has no need to call a subfunction and > >> doesn't need to same > >> anything on the stack). If the exception handler was writting the caller's > >> address in the stack > >> frame, it would in fact write it in the parent's frame, leading to a mess. > >> > >> But in fact the information is in pt_regs, it is in regs->nip so KFENCE > >> should be able to use that > >> instead of the stack. > > > > Perhaps stack_trace_save_regs() needs fixing for ppc32? Although that > > seems to use arch_stack_walk(). > > > >>> What's confusing is that it's only this test, and none of the others. > >>> Given that, it might be code-gen related, which results in some subtle > >>> issue with stack unwinding. There are a few things to try, if you feel > >>> like it: > >>> > >>> -- Change the unwinder, if it's possible for ppc32. > >> > >> I don't think it is possible. > >> > >>> > >>> -- Add code to test_invalid_access(), to get the compiler to emit > >>> different code. E.g. add a bunch (unnecessary)
Re: [RFC PATCH v1] powerpc: Enable KFENCE for PPC32
On Wed, 3 Mar 2021 at 11:32, Christophe Leroy wrote: > > > > Le 02/03/2021 à 10:53, Marco Elver a écrit : > > On Tue, 2 Mar 2021 at 10:27, Christophe Leroy > > wrote: > >> Le 02/03/2021 à 10:21, Alexander Potapenko a écrit : > [ 14.998426] BUG: KFENCE: invalid read in > finish_task_switch.isra.0+0x54/0x23c > [ 14.998426] > [ 15.007061] Invalid read at 0x(ptrval): > [ 15.010906] finish_task_switch.isra.0+0x54/0x23c > [ 15.015633] kunit_try_run_case+0x5c/0xd0 > [ 15.019682] kunit_generic_run_threadfn_adapter+0x24/0x30 > [ 15.025099] kthread+0x15c/0x174 > [ 15.028359] ret_from_kernel_thread+0x14/0x1c > [ 15.032747] > [ 15.034251] CPU: 0 PID: 111 Comm: kunit_try_catch Tainted: GB > 5.12.0-rc1-s3k-dev-01534-g4f14ae75edf0-dirty #4674 > [ 15.045811] > == > [ 15.053324] # test_invalid_access: EXPECTATION FAILED at > mm/kfence/kfence_test.c:636 > [ 15.053324] Expected report_matches() to be true, but is > false > [ 15.068359] not ok 21 - test_invalid_access > >>> > >>> The test expects the function name to be test_invalid_access, i. e. > >>> the first line should be "BUG: KFENCE: invalid read in > >>> test_invalid_access". > >>> The error reporting function unwinds the stack, skips a couple of > >>> "uninteresting" frames > >>> (https://elixir.bootlin.com/linux/v5.12-rc1/source/mm/kfence/report.c#L43) > >>> and uses the first "interesting" one frame to print the report header > >>> (https://elixir.bootlin.com/linux/v5.12-rc1/source/mm/kfence/report.c#L226). > >>> > >>> It's strange that test_invalid_access is missing altogether from the > >>> stack trace - is that expected? > >>> Can you try printing the whole stacktrace without skipping any frames > >>> to see if that function is there? > >>> > >> > >> Booting with 'no_hash_pointers" I get the following. Does it helps ? > >> > >> [ 16.837198] > >> == > >> [ 16.848521] BUG: KFENCE: invalid read in > >> finish_task_switch.isra.0+0x54/0x23c > >> [ 16.848521] > >> [ 16.857158] Invalid read at 0xdf98800a: > >> [ 16.861004] finish_task_switch.isra.0+0x54/0x23c > >> [ 16.865731] kunit_try_run_case+0x5c/0xd0 > >> [ 16.869780] kunit_generic_run_threadfn_adapter+0x24/0x30 > >> [ 16.875199] kthread+0x15c/0x174 > >> [ 16.878460] ret_from_kernel_thread+0x14/0x1c > >> [ 16.882847] > >> [ 16.884351] CPU: 0 PID: 111 Comm: kunit_try_catch Tainted: GB > >> 5.12.0-rc1-s3k-dev-01534-g4f14ae75edf0-dirty #4674 > >> [ 16.895908] NIP: c016eb8c LR: c02f50dc CTR: c016eb38 > >> [ 16.900963] REGS: e2449d90 TRAP: 0301 Tainted: GB > >> (5.12.0-rc1-s3k-dev-01534-g4f14ae75edf0-dirty) > >> [ 16.911386] MSR: 9032 CR: 2204 XER: > >> [ 16.918153] DAR: df98800a DSISR: 2000 > >> [ 16.918153] GPR00: c02f50dc e2449e50 c1140d00 e100dd24 c084b13c > >> 0008 c084b32b c016eb38 > >> [ 16.918153] GPR08: c085 df988000 c0d1 e2449eb0 22000288 > >> [ 16.936695] NIP [c016eb8c] test_invalid_access+0x54/0x108 > >> [ 16.942125] LR [c02f50dc] kunit_try_run_case+0x5c/0xd0 > >> [ 16.947292] Call Trace: > >> [ 16.949746] [e2449e50] [c005a5ec] finish_task_switch.isra.0+0x54/0x23c > >> (unreliable) > > > > The "(unreliable)" might be a clue that it's related to ppc32 stack > > unwinding. Any ppc expert know what this is about? > > > >> [ 16.957443] [e2449eb0] [c02f50dc] kunit_try_run_case+0x5c/0xd0 > >> [ 16.963319] [e2449ed0] [c02f63ec] > >> kunit_generic_run_threadfn_adapter+0x24/0x30 > >> [ 16.970574] [e2449ef0] [c004e710] kthread+0x15c/0x174 > >> [ 16.975670] [e2449f30] [c001317c] ret_from_kernel_thread+0x14/0x1c > >> [ 16.981896] Instruction dump: > >> [ 16.984879] 8129d608 38e7eb38 81020280 911f004c 3900 995f0024 > >> 907f0028 90ff001c > >> [ 16.992710] 3949000a 915f0020 3d40c0d1 3d00c085 <8929000a> 3908adb0 > >> 812a4b98 3d40c02f > >> [ 17.000711] > >> == > >> [ 17.008223] # test_invalid_access: EXPECTATION FAILED at > >> mm/kfence/kfence_test.c:636 > >> [ 17.008223] Expected report_matches() to be true, but is > >> false > >> [ 17.023243] not ok 21 - test_invalid_access > > > > On a fault in test_invalid_access, KFENCE prints the stack trace based > > on the information in pt_regs. So we do not think there's anything we > > can do to improve stack printing pe-se. > > > > What's confusing is that it's only this test, and none of the others. > > Given that, it might be code-gen related, which results in some subtle > > issue with stack unwinding. There are a few things to try, if you feel > > like it: > > > > -- Change the unwinder, if it's possible for ppc32. > > > > -- Add code to test_invalid_access(), to get the compiler to emit > >
Re: [RFC PATCH v1] powerpc: Enable KFENCE for PPC32
Le 02/03/2021 à 12:39, Marco Elver a écrit : On Tue, 2 Mar 2021 at 12:21, Christophe Leroy wrote: [...] Booting with 'no_hash_pointers" I get the following. Does it helps ? [ 16.837198] == [ 16.848521] BUG: KFENCE: invalid read in finish_task_switch.isra.0+0x54/0x23c [ 16.848521] [ 16.857158] Invalid read at 0xdf98800a: [ 16.861004] finish_task_switch.isra.0+0x54/0x23c [ 16.865731] kunit_try_run_case+0x5c/0xd0 [ 16.869780] kunit_generic_run_threadfn_adapter+0x24/0x30 [ 16.875199] kthread+0x15c/0x174 [ 16.878460] ret_from_kernel_thread+0x14/0x1c [ 16.882847] [ 16.884351] CPU: 0 PID: 111 Comm: kunit_try_catch Tainted: GB 5.12.0-rc1-s3k-dev-01534-g4f14ae75edf0-dirty #4674 [ 16.895908] NIP: c016eb8c LR: c02f50dc CTR: c016eb38 [ 16.900963] REGS: e2449d90 TRAP: 0301 Tainted: GB (5.12.0-rc1-s3k-dev-01534-g4f14ae75edf0-dirty) [ 16.911386] MSR: 9032 CR: 2204 XER: [ 16.918153] DAR: df98800a DSISR: 2000 [ 16.918153] GPR00: c02f50dc e2449e50 c1140d00 e100dd24 c084b13c 0008 c084b32b c016eb38 [ 16.918153] GPR08: c085 df988000 c0d1 e2449eb0 22000288 [ 16.936695] NIP [c016eb8c] test_invalid_access+0x54/0x108 [ 16.942125] LR [c02f50dc] kunit_try_run_case+0x5c/0xd0 [ 16.947292] Call Trace: [ 16.949746] [e2449e50] [c005a5ec] finish_task_switch.isra.0+0x54/0x23c (unreliable) The "(unreliable)" might be a clue that it's related to ppc32 stack unwinding. Any ppc expert know what this is about? [ 16.957443] [e2449eb0] [c02f50dc] kunit_try_run_case+0x5c/0xd0 [ 16.963319] [e2449ed0] [c02f63ec] kunit_generic_run_threadfn_adapter+0x24/0x30 [ 16.970574] [e2449ef0] [c004e710] kthread+0x15c/0x174 [ 16.975670] [e2449f30] [c001317c] ret_from_kernel_thread+0x14/0x1c [ 16.981896] Instruction dump: [ 16.984879] 8129d608 38e7eb38 81020280 911f004c 3900 995f0024 907f0028 90ff001c [ 16.992710] 3949000a 915f0020 3d40c0d1 3d00c085 <8929000a> 3908adb0 812a4b98 3d40c02f [ 17.000711] == [ 17.008223] # test_invalid_access: EXPECTATION FAILED at mm/kfence/kfence_test.c:636 [ 17.008223] Expected report_matches() to be true, but is false [ 17.023243] not ok 21 - test_invalid_access On a fault in test_invalid_access, KFENCE prints the stack trace based on the information in pt_regs. So we do not think there's anything we can do to improve stack printing pe-se. stack printing, probably not. Would be good anyway to mark the last level [unreliable] as the ppc does. We use stack_trace_save_regs() + stack_trace_print(). IIUC, on ppc the address in the stack frame of the caller is written by the caller. In most tests, there is some function call being done before the fault, for instance test_kmalloc_aligned_oob_read() does a call to kunit_do_assertion which populates the address of the call in the stack. However this is fragile. Interesting, this might explain it. This works for function calls because in order to call a subfunction, a function has to set up a stack frame in order to same the value in the Link Register, which contains the address of the function's parent and that will be clobbered by the sub-function call. However, it cannot be done by exceptions, because exceptions can happen in a function that has no stack frame (because that function has no need to call a subfunction and doesn't need to same anything on the stack). If the exception handler was writting the caller's address in the stack frame, it would in fact write it in the parent's frame, leading to a mess. But in fact the information is in pt_regs, it is in regs->nip so KFENCE should be able to use that instead of the stack. Perhaps stack_trace_save_regs() needs fixing for ppc32? Although that seems to use arch_stack_walk(). What's confusing is that it's only this test, and none of the others. Given that, it might be code-gen related, which results in some subtle issue with stack unwinding. There are a few things to try, if you feel like it: -- Change the unwinder, if it's possible for ppc32. I don't think it is possible. -- Add code to test_invalid_access(), to get the compiler to emit different code. E.g. add a bunch (unnecessary) function calls, or add barriers, etc. The following does the trick diff --git a/mm/kfence/kfence_test.c b/mm/kfence/kfence_test.c index 4acf4251ee04..22550676cd1f 100644 --- a/mm/kfence/kfence_test.c +++ b/mm/kfence/kfence_test.c @@ -631,8 +631,11 @@ static void test_invalid_access(struct kunit *test) .addr = &__kfence_pool[10], .is_write = false, }; + char *buf; + buf = test_alloc(test, 4, GFP_KERNEL, ALLOCATE_RIGHT); READ_ONCE(__kfence_pool[10]); + test_free(buf); KUNIT_EXPECT_TRUE(test, report_matches()); } But as I said above, this is fragile. If for some
Re: [RFC PATCH v1] powerpc: Enable KFENCE for PPC32
Le 02/03/2021 à 10:53, Marco Elver a écrit : On Tue, 2 Mar 2021 at 10:27, Christophe Leroy wrote: Le 02/03/2021 à 10:21, Alexander Potapenko a écrit : [ 14.998426] BUG: KFENCE: invalid read in finish_task_switch.isra.0+0x54/0x23c [ 14.998426] [ 15.007061] Invalid read at 0x(ptrval): [ 15.010906] finish_task_switch.isra.0+0x54/0x23c [ 15.015633] kunit_try_run_case+0x5c/0xd0 [ 15.019682] kunit_generic_run_threadfn_adapter+0x24/0x30 [ 15.025099] kthread+0x15c/0x174 [ 15.028359] ret_from_kernel_thread+0x14/0x1c [ 15.032747] [ 15.034251] CPU: 0 PID: 111 Comm: kunit_try_catch Tainted: GB 5.12.0-rc1-s3k-dev-01534-g4f14ae75edf0-dirty #4674 [ 15.045811] == [ 15.053324] # test_invalid_access: EXPECTATION FAILED at mm/kfence/kfence_test.c:636 [ 15.053324] Expected report_matches() to be true, but is false [ 15.068359] not ok 21 - test_invalid_access The test expects the function name to be test_invalid_access, i. e. the first line should be "BUG: KFENCE: invalid read in test_invalid_access". The error reporting function unwinds the stack, skips a couple of "uninteresting" frames (https://elixir.bootlin.com/linux/v5.12-rc1/source/mm/kfence/report.c#L43) and uses the first "interesting" one frame to print the report header (https://elixir.bootlin.com/linux/v5.12-rc1/source/mm/kfence/report.c#L226). It's strange that test_invalid_access is missing altogether from the stack trace - is that expected? Can you try printing the whole stacktrace without skipping any frames to see if that function is there? Booting with 'no_hash_pointers" I get the following. Does it helps ? [ 16.837198] == [ 16.848521] BUG: KFENCE: invalid read in finish_task_switch.isra.0+0x54/0x23c [ 16.848521] [ 16.857158] Invalid read at 0xdf98800a: [ 16.861004] finish_task_switch.isra.0+0x54/0x23c [ 16.865731] kunit_try_run_case+0x5c/0xd0 [ 16.869780] kunit_generic_run_threadfn_adapter+0x24/0x30 [ 16.875199] kthread+0x15c/0x174 [ 16.878460] ret_from_kernel_thread+0x14/0x1c [ 16.882847] [ 16.884351] CPU: 0 PID: 111 Comm: kunit_try_catch Tainted: GB 5.12.0-rc1-s3k-dev-01534-g4f14ae75edf0-dirty #4674 [ 16.895908] NIP: c016eb8c LR: c02f50dc CTR: c016eb38 [ 16.900963] REGS: e2449d90 TRAP: 0301 Tainted: GB (5.12.0-rc1-s3k-dev-01534-g4f14ae75edf0-dirty) [ 16.911386] MSR: 9032 CR: 2204 XER: [ 16.918153] DAR: df98800a DSISR: 2000 [ 16.918153] GPR00: c02f50dc e2449e50 c1140d00 e100dd24 c084b13c 0008 c084b32b c016eb38 [ 16.918153] GPR08: c085 df988000 c0d1 e2449eb0 22000288 [ 16.936695] NIP [c016eb8c] test_invalid_access+0x54/0x108 [ 16.942125] LR [c02f50dc] kunit_try_run_case+0x5c/0xd0 [ 16.947292] Call Trace: [ 16.949746] [e2449e50] [c005a5ec] finish_task_switch.isra.0+0x54/0x23c (unreliable) The "(unreliable)" might be a clue that it's related to ppc32 stack unwinding. Any ppc expert know what this is about? [ 16.957443] [e2449eb0] [c02f50dc] kunit_try_run_case+0x5c/0xd0 [ 16.963319] [e2449ed0] [c02f63ec] kunit_generic_run_threadfn_adapter+0x24/0x30 [ 16.970574] [e2449ef0] [c004e710] kthread+0x15c/0x174 [ 16.975670] [e2449f30] [c001317c] ret_from_kernel_thread+0x14/0x1c [ 16.981896] Instruction dump: [ 16.984879] 8129d608 38e7eb38 81020280 911f004c 3900 995f0024 907f0028 90ff001c [ 16.992710] 3949000a 915f0020 3d40c0d1 3d00c085 <8929000a> 3908adb0 812a4b98 3d40c02f [ 17.000711] == [ 17.008223] # test_invalid_access: EXPECTATION FAILED at mm/kfence/kfence_test.c:636 [ 17.008223] Expected report_matches() to be true, but is false [ 17.023243] not ok 21 - test_invalid_access On a fault in test_invalid_access, KFENCE prints the stack trace based on the information in pt_regs. So we do not think there's anything we can do to improve stack printing pe-se. What's confusing is that it's only this test, and none of the others. Given that, it might be code-gen related, which results in some subtle issue with stack unwinding. There are a few things to try, if you feel like it: -- Change the unwinder, if it's possible for ppc32. -- Add code to test_invalid_access(), to get the compiler to emit different code. E.g. add a bunch (unnecessary) function calls, or add barriers, etc. -- Play with compiler options. We already pass -fno-optimize-sibling-calls for kfence_test.o to avoid tail-call optimizations that'd hide stack trace entries. But perhaps there's something ppc-specific we missed? Well, the good thing is that KFENCE detects the bad access just fine. Since, according to the test, everything works from KFENCE's side, I'd be happy to give my Ack: Acked-by: Marco Elver Thanks. For you information, I've got a pile of warnings from mm/kfence/report.o . Is that
Re: [RFC PATCH v1] powerpc: Enable KFENCE for PPC32
On Mär 03 2021, Marco Elver wrote:
> On Wed, 3 Mar 2021 at 11:32, Christophe Leroy
> wrote:
>> ./include/linux/kern_levels.h:5:18: warning: format '%zd' expects argument
>> of type 'signed size_t',
>> but argument 3 has type 'ptrdiff_t' {aka 'const long int'} [-Wformat=]
>> 5 | #define KERN_SOH "\001" /* ASCII Start Of Header */
>>| ^~
>> ./include/linux/kern_levels.h:11:18: note: in expansion of macro 'KERN_SOH'
>> 11 | #define KERN_ERR KERN_SOH "3" /* error conditions */
>>| ^~~~
>> ./include/linux/printk.h:343:9: note: in expansion of macro 'KERN_ERR'
>>343 | printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
>>| ^~~~
>> mm/kfence/report.c:233:3: note: in expansion of macro 'pr_err'
>>233 | pr_err("Invalid free of 0x%p (in kfence-#%zd):\n", (void
>> *)address,
>>| ^~
>>
>> Christophe
>
> No this is not expected. Is 'signed size_t' != 'long int' on ppc32?
If you want to format a ptrdiff_t you should use %td.
Andreas.
--
Andreas Schwab, sch...@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1
"And now for something completely different."
Re: [RFC PATCH v1] powerpc: Enable KFENCE for PPC32
Le 02/03/2021 à 12:40, Michael Ellerman a écrit : Christophe Leroy writes: Le 02/03/2021 à 10:53, Marco Elver a écrit : On Tue, 2 Mar 2021 at 10:27, Christophe Leroy wrote: Le 02/03/2021 à 10:21, Alexander Potapenko a écrit : [ 14.998426] BUG: KFENCE: invalid read in finish_task_switch.isra.0+0x54/0x23c [ 14.998426] [ 15.007061] Invalid read at 0x(ptrval): [ 15.010906] finish_task_switch.isra.0+0x54/0x23c [ 15.015633] kunit_try_run_case+0x5c/0xd0 [ 15.019682] kunit_generic_run_threadfn_adapter+0x24/0x30 [ 15.025099] kthread+0x15c/0x174 [ 15.028359] ret_from_kernel_thread+0x14/0x1c [ 15.032747] [ 15.034251] CPU: 0 PID: 111 Comm: kunit_try_catch Tainted: GB 5.12.0-rc1-s3k-dev-01534-g4f14ae75edf0-dirty #4674 [ 15.045811] == [ 15.053324] # test_invalid_access: EXPECTATION FAILED at mm/kfence/kfence_test.c:636 [ 15.053324] Expected report_matches() to be true, but is false [ 15.068359] not ok 21 - test_invalid_access The test expects the function name to be test_invalid_access, i. e. the first line should be "BUG: KFENCE: invalid read in test_invalid_access". The error reporting function unwinds the stack, skips a couple of "uninteresting" frames (https://elixir.bootlin.com/linux/v5.12-rc1/source/mm/kfence/report.c#L43) and uses the first "interesting" one frame to print the report header (https://elixir.bootlin.com/linux/v5.12-rc1/source/mm/kfence/report.c#L226). It's strange that test_invalid_access is missing altogether from the stack trace - is that expected? Can you try printing the whole stacktrace without skipping any frames to see if that function is there? Booting with 'no_hash_pointers" I get the following. Does it helps ? [ 16.837198] == [ 16.848521] BUG: KFENCE: invalid read in finish_task_switch.isra.0+0x54/0x23c [ 16.848521] [ 16.857158] Invalid read at 0xdf98800a: [ 16.861004] finish_task_switch.isra.0+0x54/0x23c [ 16.865731] kunit_try_run_case+0x5c/0xd0 [ 16.869780] kunit_generic_run_threadfn_adapter+0x24/0x30 [ 16.875199] kthread+0x15c/0x174 [ 16.878460] ret_from_kernel_thread+0x14/0x1c [ 16.882847] [ 16.884351] CPU: 0 PID: 111 Comm: kunit_try_catch Tainted: GB 5.12.0-rc1-s3k-dev-01534-g4f14ae75edf0-dirty #4674 [ 16.895908] NIP: c016eb8c LR: c02f50dc CTR: c016eb38 [ 16.900963] REGS: e2449d90 TRAP: 0301 Tainted: GB (5.12.0-rc1-s3k-dev-01534-g4f14ae75edf0-dirty) [ 16.911386] MSR: 9032 CR: 2204 XER: [ 16.918153] DAR: df98800a DSISR: 2000 [ 16.918153] GPR00: c02f50dc e2449e50 c1140d00 e100dd24 c084b13c 0008 c084b32b c016eb38 [ 16.918153] GPR08: c085 df988000 c0d1 e2449eb0 22000288 [ 16.936695] NIP [c016eb8c] test_invalid_access+0x54/0x108 [ 16.942125] LR [c02f50dc] kunit_try_run_case+0x5c/0xd0 [ 16.947292] Call Trace: [ 16.949746] [e2449e50] [c005a5ec] finish_task_switch.isra.0+0x54/0x23c (unreliable) The "(unreliable)" might be a clue that it's related to ppc32 stack unwinding. Any ppc expert know what this is about? [ 16.957443] [e2449eb0] [c02f50dc] kunit_try_run_case+0x5c/0xd0 [ 16.963319] [e2449ed0] [c02f63ec] kunit_generic_run_threadfn_adapter+0x24/0x30 [ 16.970574] [e2449ef0] [c004e710] kthread+0x15c/0x174 [ 16.975670] [e2449f30] [c001317c] ret_from_kernel_thread+0x14/0x1c [ 16.981896] Instruction dump: [ 16.984879] 8129d608 38e7eb38 81020280 911f004c 3900 995f0024 907f0028 90ff001c [ 16.992710] 3949000a 915f0020 3d40c0d1 3d00c085 <8929000a> 3908adb0 812a4b98 3d40c02f [ 17.000711] == [ 17.008223] # test_invalid_access: EXPECTATION FAILED at mm/kfence/kfence_test.c:636 [ 17.008223] Expected report_matches() to be true, but is false [ 17.023243] not ok 21 - test_invalid_access On a fault in test_invalid_access, KFENCE prints the stack trace based on the information in pt_regs. So we do not think there's anything we can do to improve stack printing pe-se. stack printing, probably not. Would be good anyway to mark the last level [unreliable] as the ppc does. IIUC, on ppc the address in the stack frame of the caller is written by the caller. In most tests, there is some function call being done before the fault, for instance test_kmalloc_aligned_oob_read() does a call to kunit_do_assertion which populates the address of the call in the stack. However this is fragile. This works for function calls because in order to call a subfunction, a function has to set up a stack frame in order to same the value in the Link Register, which contains the address of the function's parent and that will be clobbered by the sub-function call. However, it cannot be done by exceptions, because exceptions can happen in a function that has no stack frame (because that function has no need to call a
Re: [RFC PATCH v1] powerpc: Enable KFENCE for PPC32
On Tue, Mar 02, 2021 at 10:40:03PM +1100, Michael Ellerman wrote: > >> -- Change the unwinder, if it's possible for ppc32. > > > > I don't think it is possible. > > I think this actually is the solution. > > It seems the good architectures have all added support for > arch_stack_walk(), and we have not. I have no idea what arch_stack_walk does, but some background info: PowerPC functions that do save the LR (== the return address), and/or that set up a new stack frame, do not do this at the start of the function necessarily (it is a lot faster to postpone this, even if you always have to do it). So, in a leaf function it isn't always known if this has been done (in all callers further up it is always done, of course). If you have DWARF unwind info all is fine of course, but you do not have that in the kernel. > So I think it's probably on us to update to that new API. Or at least > update our save_stack_trace() to fabricate an entry using the NIP, as it > seems that's what callers expect. This sounds very expensive? If it is only a debug feature that won't be used in production that does not matter, but it worries me. Segher
Re: [RFC PATCH v1] powerpc: Enable KFENCE for PPC32
Christophe Leroy writes: > Le 02/03/2021 à 10:53, Marco Elver a écrit : >> On Tue, 2 Mar 2021 at 10:27, Christophe Leroy >> wrote: >>> Le 02/03/2021 à 10:21, Alexander Potapenko a écrit : > [ 14.998426] BUG: KFENCE: invalid read in > finish_task_switch.isra.0+0x54/0x23c > [ 14.998426] > [ 15.007061] Invalid read at 0x(ptrval): > [ 15.010906] finish_task_switch.isra.0+0x54/0x23c > [ 15.015633] kunit_try_run_case+0x5c/0xd0 > [ 15.019682] kunit_generic_run_threadfn_adapter+0x24/0x30 > [ 15.025099] kthread+0x15c/0x174 > [ 15.028359] ret_from_kernel_thread+0x14/0x1c > [ 15.032747] > [ 15.034251] CPU: 0 PID: 111 Comm: kunit_try_catch Tainted: GB > 5.12.0-rc1-s3k-dev-01534-g4f14ae75edf0-dirty #4674 > [ 15.045811] > == > [ 15.053324] # test_invalid_access: EXPECTATION FAILED at > mm/kfence/kfence_test.c:636 > [ 15.053324] Expected report_matches() to be true, but is > false > [ 15.068359] not ok 21 - test_invalid_access The test expects the function name to be test_invalid_access, i. e. the first line should be "BUG: KFENCE: invalid read in test_invalid_access". The error reporting function unwinds the stack, skips a couple of "uninteresting" frames (https://elixir.bootlin.com/linux/v5.12-rc1/source/mm/kfence/report.c#L43) and uses the first "interesting" one frame to print the report header (https://elixir.bootlin.com/linux/v5.12-rc1/source/mm/kfence/report.c#L226). It's strange that test_invalid_access is missing altogether from the stack trace - is that expected? Can you try printing the whole stacktrace without skipping any frames to see if that function is there? >>> >>> Booting with 'no_hash_pointers" I get the following. Does it helps ? >>> >>> [ 16.837198] >>> == >>> [ 16.848521] BUG: KFENCE: invalid read in >>> finish_task_switch.isra.0+0x54/0x23c >>> [ 16.848521] >>> [ 16.857158] Invalid read at 0xdf98800a: >>> [ 16.861004] finish_task_switch.isra.0+0x54/0x23c >>> [ 16.865731] kunit_try_run_case+0x5c/0xd0 >>> [ 16.869780] kunit_generic_run_threadfn_adapter+0x24/0x30 >>> [ 16.875199] kthread+0x15c/0x174 >>> [ 16.878460] ret_from_kernel_thread+0x14/0x1c >>> [ 16.882847] >>> [ 16.884351] CPU: 0 PID: 111 Comm: kunit_try_catch Tainted: GB >>> 5.12.0-rc1-s3k-dev-01534-g4f14ae75edf0-dirty #4674 >>> [ 16.895908] NIP: c016eb8c LR: c02f50dc CTR: c016eb38 >>> [ 16.900963] REGS: e2449d90 TRAP: 0301 Tainted: GB >>> (5.12.0-rc1-s3k-dev-01534-g4f14ae75edf0-dirty) >>> [ 16.911386] MSR: 9032 CR: 2204 XER: >>> [ 16.918153] DAR: df98800a DSISR: 2000 >>> [ 16.918153] GPR00: c02f50dc e2449e50 c1140d00 e100dd24 c084b13c 0008 >>> c084b32b c016eb38 >>> [ 16.918153] GPR08: c085 df988000 c0d1 e2449eb0 22000288 >>> [ 16.936695] NIP [c016eb8c] test_invalid_access+0x54/0x108 >>> [ 16.942125] LR [c02f50dc] kunit_try_run_case+0x5c/0xd0 >>> [ 16.947292] Call Trace: >>> [ 16.949746] [e2449e50] [c005a5ec] finish_task_switch.isra.0+0x54/0x23c >>> (unreliable) >> >> The "(unreliable)" might be a clue that it's related to ppc32 stack >> unwinding. Any ppc expert know what this is about? >> >>> [ 16.957443] [e2449eb0] [c02f50dc] kunit_try_run_case+0x5c/0xd0 >>> [ 16.963319] [e2449ed0] [c02f63ec] >>> kunit_generic_run_threadfn_adapter+0x24/0x30 >>> [ 16.970574] [e2449ef0] [c004e710] kthread+0x15c/0x174 >>> [ 16.975670] [e2449f30] [c001317c] ret_from_kernel_thread+0x14/0x1c >>> [ 16.981896] Instruction dump: >>> [ 16.984879] 8129d608 38e7eb38 81020280 911f004c 3900 995f0024 >>> 907f0028 90ff001c >>> [ 16.992710] 3949000a 915f0020 3d40c0d1 3d00c085 <8929000a> 3908adb0 >>> 812a4b98 3d40c02f >>> [ 17.000711] >>> == >>> [ 17.008223] # test_invalid_access: EXPECTATION FAILED at >>> mm/kfence/kfence_test.c:636 >>> [ 17.008223] Expected report_matches() to be true, but is false >>> [ 17.023243] not ok 21 - test_invalid_access >> >> On a fault in test_invalid_access, KFENCE prints the stack trace based >> on the information in pt_regs. So we do not think there's anything we >> can do to improve stack printing pe-se. > > stack printing, probably not. Would be good anyway to mark the last level > [unreliable] as the ppc does. > > IIUC, on ppc the address in the stack frame of the caller is written by the > caller. In most tests, > there is some function call being done before the fault, for instance > test_kmalloc_aligned_oob_read() does a call to kunit_do_assertion which > populates the address of the > call in the stack. However this is fragile. > > This works for function calls because in order to call a
Re: [RFC PATCH v1] powerpc: Enable KFENCE for PPC32
On Tue, 2 Mar 2021 at 12:21, Christophe Leroy wrote: [...] > >> Booting with 'no_hash_pointers" I get the following. Does it helps ? > >> > >> [ 16.837198] > >> == > >> [ 16.848521] BUG: KFENCE: invalid read in > >> finish_task_switch.isra.0+0x54/0x23c > >> [ 16.848521] > >> [ 16.857158] Invalid read at 0xdf98800a: > >> [ 16.861004] finish_task_switch.isra.0+0x54/0x23c > >> [ 16.865731] kunit_try_run_case+0x5c/0xd0 > >> [ 16.869780] kunit_generic_run_threadfn_adapter+0x24/0x30 > >> [ 16.875199] kthread+0x15c/0x174 > >> [ 16.878460] ret_from_kernel_thread+0x14/0x1c > >> [ 16.882847] > >> [ 16.884351] CPU: 0 PID: 111 Comm: kunit_try_catch Tainted: GB > >> 5.12.0-rc1-s3k-dev-01534-g4f14ae75edf0-dirty #4674 > >> [ 16.895908] NIP: c016eb8c LR: c02f50dc CTR: c016eb38 > >> [ 16.900963] REGS: e2449d90 TRAP: 0301 Tainted: GB > >> (5.12.0-rc1-s3k-dev-01534-g4f14ae75edf0-dirty) > >> [ 16.911386] MSR: 9032 CR: 2204 XER: > >> [ 16.918153] DAR: df98800a DSISR: 2000 > >> [ 16.918153] GPR00: c02f50dc e2449e50 c1140d00 e100dd24 c084b13c > >> 0008 c084b32b c016eb38 > >> [ 16.918153] GPR08: c085 df988000 c0d1 e2449eb0 22000288 > >> [ 16.936695] NIP [c016eb8c] test_invalid_access+0x54/0x108 > >> [ 16.942125] LR [c02f50dc] kunit_try_run_case+0x5c/0xd0 > >> [ 16.947292] Call Trace: > >> [ 16.949746] [e2449e50] [c005a5ec] finish_task_switch.isra.0+0x54/0x23c > >> (unreliable) > > > > The "(unreliable)" might be a clue that it's related to ppc32 stack > > unwinding. Any ppc expert know what this is about? > > > >> [ 16.957443] [e2449eb0] [c02f50dc] kunit_try_run_case+0x5c/0xd0 > >> [ 16.963319] [e2449ed0] [c02f63ec] > >> kunit_generic_run_threadfn_adapter+0x24/0x30 > >> [ 16.970574] [e2449ef0] [c004e710] kthread+0x15c/0x174 > >> [ 16.975670] [e2449f30] [c001317c] ret_from_kernel_thread+0x14/0x1c > >> [ 16.981896] Instruction dump: > >> [ 16.984879] 8129d608 38e7eb38 81020280 911f004c 3900 995f0024 > >> 907f0028 90ff001c > >> [ 16.992710] 3949000a 915f0020 3d40c0d1 3d00c085 <8929000a> 3908adb0 > >> 812a4b98 3d40c02f > >> [ 17.000711] > >> == > >> [ 17.008223] # test_invalid_access: EXPECTATION FAILED at > >> mm/kfence/kfence_test.c:636 > >> [ 17.008223] Expected report_matches() to be true, but is > >> false > >> [ 17.023243] not ok 21 - test_invalid_access > > > > On a fault in test_invalid_access, KFENCE prints the stack trace based > > on the information in pt_regs. So we do not think there's anything we > > can do to improve stack printing pe-se. > > stack printing, probably not. Would be good anyway to mark the last level > [unreliable] as the ppc does. We use stack_trace_save_regs() + stack_trace_print(). > IIUC, on ppc the address in the stack frame of the caller is written by the > caller. In most tests, > there is some function call being done before the fault, for instance > test_kmalloc_aligned_oob_read() does a call to kunit_do_assertion which > populates the address of the > call in the stack. However this is fragile. Interesting, this might explain it. > This works for function calls because in order to call a subfunction, a > function has to set up a > stack frame in order to same the value in the Link Register, which contains > the address of the > function's parent and that will be clobbered by the sub-function call. > > However, it cannot be done by exceptions, because exceptions can happen in a > function that has no > stack frame (because that function has no need to call a subfunction and > doesn't need to same > anything on the stack). If the exception handler was writting the caller's > address in the stack > frame, it would in fact write it in the parent's frame, leading to a mess. > > But in fact the information is in pt_regs, it is in regs->nip so KFENCE > should be able to use that > instead of the stack. Perhaps stack_trace_save_regs() needs fixing for ppc32? Although that seems to use arch_stack_walk(). > > What's confusing is that it's only this test, and none of the others. > > Given that, it might be code-gen related, which results in some subtle > > issue with stack unwinding. There are a few things to try, if you feel > > like it: > > > > -- Change the unwinder, if it's possible for ppc32. > > I don't think it is possible. > > > > > -- Add code to test_invalid_access(), to get the compiler to emit > > different code. E.g. add a bunch (unnecessary) function calls, or add > > barriers, etc. > > The following does the trick > > diff --git a/mm/kfence/kfence_test.c b/mm/kfence/kfence_test.c > index 4acf4251ee04..22550676cd1f 100644 > --- a/mm/kfence/kfence_test.c > +++ b/mm/kfence/kfence_test.c > @@ -631,8 +631,11 @@ static void test_invalid_access(struct kunit *test) > .addr = &__kfence_pool[10], >
Re: [RFC PATCH v1] powerpc: Enable KFENCE for PPC32
Le 02/03/2021 à 10:53, Marco Elver a écrit : On Tue, 2 Mar 2021 at 10:27, Christophe Leroy wrote: Le 02/03/2021 à 10:21, Alexander Potapenko a écrit : [ 14.998426] BUG: KFENCE: invalid read in finish_task_switch.isra.0+0x54/0x23c [ 14.998426] [ 15.007061] Invalid read at 0x(ptrval): [ 15.010906] finish_task_switch.isra.0+0x54/0x23c [ 15.015633] kunit_try_run_case+0x5c/0xd0 [ 15.019682] kunit_generic_run_threadfn_adapter+0x24/0x30 [ 15.025099] kthread+0x15c/0x174 [ 15.028359] ret_from_kernel_thread+0x14/0x1c [ 15.032747] [ 15.034251] CPU: 0 PID: 111 Comm: kunit_try_catch Tainted: GB 5.12.0-rc1-s3k-dev-01534-g4f14ae75edf0-dirty #4674 [ 15.045811] == [ 15.053324] # test_invalid_access: EXPECTATION FAILED at mm/kfence/kfence_test.c:636 [ 15.053324] Expected report_matches() to be true, but is false [ 15.068359] not ok 21 - test_invalid_access The test expects the function name to be test_invalid_access, i. e. the first line should be "BUG: KFENCE: invalid read in test_invalid_access". The error reporting function unwinds the stack, skips a couple of "uninteresting" frames (https://elixir.bootlin.com/linux/v5.12-rc1/source/mm/kfence/report.c#L43) and uses the first "interesting" one frame to print the report header (https://elixir.bootlin.com/linux/v5.12-rc1/source/mm/kfence/report.c#L226). It's strange that test_invalid_access is missing altogether from the stack trace - is that expected? Can you try printing the whole stacktrace without skipping any frames to see if that function is there? Booting with 'no_hash_pointers" I get the following. Does it helps ? [ 16.837198] == [ 16.848521] BUG: KFENCE: invalid read in finish_task_switch.isra.0+0x54/0x23c [ 16.848521] [ 16.857158] Invalid read at 0xdf98800a: [ 16.861004] finish_task_switch.isra.0+0x54/0x23c [ 16.865731] kunit_try_run_case+0x5c/0xd0 [ 16.869780] kunit_generic_run_threadfn_adapter+0x24/0x30 [ 16.875199] kthread+0x15c/0x174 [ 16.878460] ret_from_kernel_thread+0x14/0x1c [ 16.882847] [ 16.884351] CPU: 0 PID: 111 Comm: kunit_try_catch Tainted: GB 5.12.0-rc1-s3k-dev-01534-g4f14ae75edf0-dirty #4674 [ 16.895908] NIP: c016eb8c LR: c02f50dc CTR: c016eb38 [ 16.900963] REGS: e2449d90 TRAP: 0301 Tainted: GB (5.12.0-rc1-s3k-dev-01534-g4f14ae75edf0-dirty) [ 16.911386] MSR: 9032 CR: 2204 XER: [ 16.918153] DAR: df98800a DSISR: 2000 [ 16.918153] GPR00: c02f50dc e2449e50 c1140d00 e100dd24 c084b13c 0008 c084b32b c016eb38 [ 16.918153] GPR08: c085 df988000 c0d1 e2449eb0 22000288 [ 16.936695] NIP [c016eb8c] test_invalid_access+0x54/0x108 [ 16.942125] LR [c02f50dc] kunit_try_run_case+0x5c/0xd0 [ 16.947292] Call Trace: [ 16.949746] [e2449e50] [c005a5ec] finish_task_switch.isra.0+0x54/0x23c (unreliable) The "(unreliable)" might be a clue that it's related to ppc32 stack unwinding. Any ppc expert know what this is about? [ 16.957443] [e2449eb0] [c02f50dc] kunit_try_run_case+0x5c/0xd0 [ 16.963319] [e2449ed0] [c02f63ec] kunit_generic_run_threadfn_adapter+0x24/0x30 [ 16.970574] [e2449ef0] [c004e710] kthread+0x15c/0x174 [ 16.975670] [e2449f30] [c001317c] ret_from_kernel_thread+0x14/0x1c [ 16.981896] Instruction dump: [ 16.984879] 8129d608 38e7eb38 81020280 911f004c 3900 995f0024 907f0028 90ff001c [ 16.992710] 3949000a 915f0020 3d40c0d1 3d00c085 <8929000a> 3908adb0 812a4b98 3d40c02f [ 17.000711] == [ 17.008223] # test_invalid_access: EXPECTATION FAILED at mm/kfence/kfence_test.c:636 [ 17.008223] Expected report_matches() to be true, but is false [ 17.023243] not ok 21 - test_invalid_access On a fault in test_invalid_access, KFENCE prints the stack trace based on the information in pt_regs. So we do not think there's anything we can do to improve stack printing pe-se. stack printing, probably not. Would be good anyway to mark the last level [unreliable] as the ppc does. IIUC, on ppc the address in the stack frame of the caller is written by the caller. In most tests, there is some function call being done before the fault, for instance test_kmalloc_aligned_oob_read() does a call to kunit_do_assertion which populates the address of the call in the stack. However this is fragile. This works for function calls because in order to call a subfunction, a function has to set up a stack frame in order to same the value in the Link Register, which contains the address of the function's parent and that will be clobbered by the sub-function call. However, it cannot be done by exceptions, because exceptions can happen in a function that has no stack frame (because that function has no need to call a subfunction and doesn't need to same anything on the stack). If the exception
Re: [RFC PATCH v1] powerpc: Enable KFENCE for PPC32
On Tue, 2 Mar 2021 at 10:27, Christophe Leroy wrote: > Le 02/03/2021 à 10:21, Alexander Potapenko a écrit : > >> [ 14.998426] BUG: KFENCE: invalid read in > >> finish_task_switch.isra.0+0x54/0x23c > >> [ 14.998426] > >> [ 15.007061] Invalid read at 0x(ptrval): > >> [ 15.010906] finish_task_switch.isra.0+0x54/0x23c > >> [ 15.015633] kunit_try_run_case+0x5c/0xd0 > >> [ 15.019682] kunit_generic_run_threadfn_adapter+0x24/0x30 > >> [ 15.025099] kthread+0x15c/0x174 > >> [ 15.028359] ret_from_kernel_thread+0x14/0x1c > >> [ 15.032747] > >> [ 15.034251] CPU: 0 PID: 111 Comm: kunit_try_catch Tainted: GB > >> 5.12.0-rc1-s3k-dev-01534-g4f14ae75edf0-dirty #4674 > >> [ 15.045811] > >> == > >> [ 15.053324] # test_invalid_access: EXPECTATION FAILED at > >> mm/kfence/kfence_test.c:636 > >> [ 15.053324] Expected report_matches() to be true, but is > >> false > >> [ 15.068359] not ok 21 - test_invalid_access > > > > The test expects the function name to be test_invalid_access, i. e. > > the first line should be "BUG: KFENCE: invalid read in > > test_invalid_access". > > The error reporting function unwinds the stack, skips a couple of > > "uninteresting" frames > > (https://elixir.bootlin.com/linux/v5.12-rc1/source/mm/kfence/report.c#L43) > > and uses the first "interesting" one frame to print the report header > > (https://elixir.bootlin.com/linux/v5.12-rc1/source/mm/kfence/report.c#L226). > > > > It's strange that test_invalid_access is missing altogether from the > > stack trace - is that expected? > > Can you try printing the whole stacktrace without skipping any frames > > to see if that function is there? > > > > Booting with 'no_hash_pointers" I get the following. Does it helps ? > > [ 16.837198] > == > [ 16.848521] BUG: KFENCE: invalid read in > finish_task_switch.isra.0+0x54/0x23c > [ 16.848521] > [ 16.857158] Invalid read at 0xdf98800a: > [ 16.861004] finish_task_switch.isra.0+0x54/0x23c > [ 16.865731] kunit_try_run_case+0x5c/0xd0 > [ 16.869780] kunit_generic_run_threadfn_adapter+0x24/0x30 > [ 16.875199] kthread+0x15c/0x174 > [ 16.878460] ret_from_kernel_thread+0x14/0x1c > [ 16.882847] > [ 16.884351] CPU: 0 PID: 111 Comm: kunit_try_catch Tainted: GB > 5.12.0-rc1-s3k-dev-01534-g4f14ae75edf0-dirty #4674 > [ 16.895908] NIP: c016eb8c LR: c02f50dc CTR: c016eb38 > [ 16.900963] REGS: e2449d90 TRAP: 0301 Tainted: GB > (5.12.0-rc1-s3k-dev-01534-g4f14ae75edf0-dirty) > [ 16.911386] MSR: 9032 CR: 2204 XER: > [ 16.918153] DAR: df98800a DSISR: 2000 > [ 16.918153] GPR00: c02f50dc e2449e50 c1140d00 e100dd24 c084b13c 0008 > c084b32b c016eb38 > [ 16.918153] GPR08: c085 df988000 c0d1 e2449eb0 22000288 > [ 16.936695] NIP [c016eb8c] test_invalid_access+0x54/0x108 > [ 16.942125] LR [c02f50dc] kunit_try_run_case+0x5c/0xd0 > [ 16.947292] Call Trace: > [ 16.949746] [e2449e50] [c005a5ec] finish_task_switch.isra.0+0x54/0x23c > (unreliable) The "(unreliable)" might be a clue that it's related to ppc32 stack unwinding. Any ppc expert know what this is about? > [ 16.957443] [e2449eb0] [c02f50dc] kunit_try_run_case+0x5c/0xd0 > [ 16.963319] [e2449ed0] [c02f63ec] > kunit_generic_run_threadfn_adapter+0x24/0x30 > [ 16.970574] [e2449ef0] [c004e710] kthread+0x15c/0x174 > [ 16.975670] [e2449f30] [c001317c] ret_from_kernel_thread+0x14/0x1c > [ 16.981896] Instruction dump: > [ 16.984879] 8129d608 38e7eb38 81020280 911f004c 3900 995f0024 907f0028 > 90ff001c > [ 16.992710] 3949000a 915f0020 3d40c0d1 3d00c085 <8929000a> 3908adb0 > 812a4b98 3d40c02f > [ 17.000711] > == > [ 17.008223] # test_invalid_access: EXPECTATION FAILED at > mm/kfence/kfence_test.c:636 > [ 17.008223] Expected report_matches() to be true, but is false > [ 17.023243] not ok 21 - test_invalid_access On a fault in test_invalid_access, KFENCE prints the stack trace based on the information in pt_regs. So we do not think there's anything we can do to improve stack printing pe-se. What's confusing is that it's only this test, and none of the others. Given that, it might be code-gen related, which results in some subtle issue with stack unwinding. There are a few things to try, if you feel like it: -- Change the unwinder, if it's possible for ppc32. -- Add code to test_invalid_access(), to get the compiler to emit different code. E.g. add a bunch (unnecessary) function calls, or add barriers, etc. -- Play with compiler options. We already pass -fno-optimize-sibling-calls for kfence_test.o to avoid tail-call optimizations that'd hide stack trace entries. But perhaps there's something ppc-specific we missed? Well, the good thing is that KFENCE detects the bad access just fine. Since, according to the test,
Re: [RFC PATCH v1] powerpc: Enable KFENCE for PPC32
> [ 14.998426] BUG: KFENCE: invalid read in > finish_task_switch.isra.0+0x54/0x23c > [ 14.998426] > [ 15.007061] Invalid read at 0x(ptrval): > [ 15.010906] finish_task_switch.isra.0+0x54/0x23c > [ 15.015633] kunit_try_run_case+0x5c/0xd0 > [ 15.019682] kunit_generic_run_threadfn_adapter+0x24/0x30 > [ 15.025099] kthread+0x15c/0x174 > [ 15.028359] ret_from_kernel_thread+0x14/0x1c > [ 15.032747] > [ 15.034251] CPU: 0 PID: 111 Comm: kunit_try_catch Tainted: GB > 5.12.0-rc1-s3k-dev-01534-g4f14ae75edf0-dirty #4674 > [ 15.045811] > == > [ 15.053324] # test_invalid_access: EXPECTATION FAILED at > mm/kfence/kfence_test.c:636 > [ 15.053324] Expected report_matches() to be true, but is false > [ 15.068359] not ok 21 - test_invalid_access The test expects the function name to be test_invalid_access, i. e. the first line should be "BUG: KFENCE: invalid read in test_invalid_access". The error reporting function unwinds the stack, skips a couple of "uninteresting" frames (https://elixir.bootlin.com/linux/v5.12-rc1/source/mm/kfence/report.c#L43) and uses the first "interesting" one frame to print the report header (https://elixir.bootlin.com/linux/v5.12-rc1/source/mm/kfence/report.c#L226). It's strange that test_invalid_access is missing altogether from the stack trace - is that expected? Can you try printing the whole stacktrace without skipping any frames to see if that function is there?
Re: [RFC PATCH v1] powerpc: Enable KFENCE for PPC32
Le 02/03/2021 à 10:21, Alexander Potapenko a écrit : [ 14.998426] BUG: KFENCE: invalid read in finish_task_switch.isra.0+0x54/0x23c [ 14.998426] [ 15.007061] Invalid read at 0x(ptrval): [ 15.010906] finish_task_switch.isra.0+0x54/0x23c [ 15.015633] kunit_try_run_case+0x5c/0xd0 [ 15.019682] kunit_generic_run_threadfn_adapter+0x24/0x30 [ 15.025099] kthread+0x15c/0x174 [ 15.028359] ret_from_kernel_thread+0x14/0x1c [ 15.032747] [ 15.034251] CPU: 0 PID: 111 Comm: kunit_try_catch Tainted: GB 5.12.0-rc1-s3k-dev-01534-g4f14ae75edf0-dirty #4674 [ 15.045811] == [ 15.053324] # test_invalid_access: EXPECTATION FAILED at mm/kfence/kfence_test.c:636 [ 15.053324] Expected report_matches() to be true, but is false [ 15.068359] not ok 21 - test_invalid_access The test expects the function name to be test_invalid_access, i. e. the first line should be "BUG: KFENCE: invalid read in test_invalid_access". The error reporting function unwinds the stack, skips a couple of "uninteresting" frames (https://elixir.bootlin.com/linux/v5.12-rc1/source/mm/kfence/report.c#L43) and uses the first "interesting" one frame to print the report header (https://elixir.bootlin.com/linux/v5.12-rc1/source/mm/kfence/report.c#L226). It's strange that test_invalid_access is missing altogether from the stack trace - is that expected? Can you try printing the whole stacktrace without skipping any frames to see if that function is there? Booting with 'no_hash_pointers" I get the following. Does it helps ? [ 16.837198] == [ 16.848521] BUG: KFENCE: invalid read in finish_task_switch.isra.0+0x54/0x23c [ 16.848521] [ 16.857158] Invalid read at 0xdf98800a: [ 16.861004] finish_task_switch.isra.0+0x54/0x23c [ 16.865731] kunit_try_run_case+0x5c/0xd0 [ 16.869780] kunit_generic_run_threadfn_adapter+0x24/0x30 [ 16.875199] kthread+0x15c/0x174 [ 16.878460] ret_from_kernel_thread+0x14/0x1c [ 16.882847] [ 16.884351] CPU: 0 PID: 111 Comm: kunit_try_catch Tainted: GB 5.12.0-rc1-s3k-dev-01534-g4f14ae75edf0-dirty #4674 [ 16.895908] NIP: c016eb8c LR: c02f50dc CTR: c016eb38 [ 16.900963] REGS: e2449d90 TRAP: 0301 Tainted: GB (5.12.0-rc1-s3k-dev-01534-g4f14ae75edf0-dirty) [ 16.911386] MSR: 9032 CR: 2204 XER: [ 16.918153] DAR: df98800a DSISR: 2000 [ 16.918153] GPR00: c02f50dc e2449e50 c1140d00 e100dd24 c084b13c 0008 c084b32b c016eb38 [ 16.918153] GPR08: c085 df988000 c0d1 e2449eb0 22000288 [ 16.936695] NIP [c016eb8c] test_invalid_access+0x54/0x108 [ 16.942125] LR [c02f50dc] kunit_try_run_case+0x5c/0xd0 [ 16.947292] Call Trace: [ 16.949746] [e2449e50] [c005a5ec] finish_task_switch.isra.0+0x54/0x23c (unreliable) [ 16.957443] [e2449eb0] [c02f50dc] kunit_try_run_case+0x5c/0xd0 [ 16.963319] [e2449ed0] [c02f63ec] kunit_generic_run_threadfn_adapter+0x24/0x30 [ 16.970574] [e2449ef0] [c004e710] kthread+0x15c/0x174 [ 16.975670] [e2449f30] [c001317c] ret_from_kernel_thread+0x14/0x1c [ 16.981896] Instruction dump: [ 16.984879] 8129d608 38e7eb38 81020280 911f004c 3900 995f0024 907f0028 90ff001c [ 16.992710] 3949000a 915f0020 3d40c0d1 3d00c085 <8929000a> 3908adb0 812a4b98 3d40c02f [ 17.000711] == [ 17.008223] # test_invalid_access: EXPECTATION FAILED at mm/kfence/kfence_test.c:636 [ 17.008223] Expected report_matches() to be true, but is false [ 17.023243] not ok 21 - test_invalid_access
Re: [RFC PATCH v1] powerpc: Enable KFENCE for PPC32
Le 02/03/2021 à 09:58, Marco Elver a écrit : On Tue, 2 Mar 2021 at 09:37, Christophe Leroy wrote: Add architecture specific implementation details for KFENCE and enable KFENCE for the ppc32 architecture. In particular, this implements the required interface in . Nice! KFENCE requires that attributes for pages from its memory pool can individually be set. Therefore, force the Read/Write linear map to be mapped at page granularity. Unit tests succeed on all tests but one: [ 15.053324] # test_invalid_access: EXPECTATION FAILED at mm/kfence/kfence_test.c:636 [ 15.053324] Expected report_matches() to be true, but is false [ 15.068359] not ok 21 - test_invalid_access This is strange, given all the other tests passed. Do you mind sharing the full test log? [0.00] Linux version 5.12.0-rc1-s3k-dev-01534-g4f14ae75edf0-dirty (root@localhost.localdomain) (powerpc64-linux-gcc (GCC) 10.1.0, GNU ld (GNU Binutils) 2.34) #4674 PREEMPT Tue Mar 2 08:18:49 UTC 2021 [0.00] Using CMPCPRO machine description [0.00] Found legacy serial port 0 for /soc8321@b000/serial@4500 [0.00] mem=b0004500, taddr=b0004500, irq=0, clk=13334, speed=0 [0.00] Found legacy serial port 1 for /soc8321@b000/serial@4600 [0.00] mem=b0004600, taddr=b0004600, irq=0, clk=13334, speed=0 [0.00] ioremap() called early from find_legacy_serial_ports+0x3e4/0x4d8. Use early_ioremap() instead [0.00] printk: bootconsole [udbg0] enabled [0.00] - [0.00] phys_mem_size = 0x2000 [0.00] dcache_bsize = 0x20 [0.00] icache_bsize = 0x20 [0.00] cpu_features = 0x01000140 [0.00] possible= 0x277ce140 [0.00] always = 0x0100 [0.00] cpu_user_features = 0x8400 0x [0.00] mmu_features = 0x0021 [0.00] Hash_size = 0x0 [0.00] - [0.00] Top of RAM: 0x2000, Total RAM: 0x2000 [0.00] Memory hole size: 0MB [0.00] Zone ranges: [0.00] Normal [mem 0x-0x1fff] [0.00] Movable zone start for each node [0.00] Early memory node ranges [0.00] node 0: [mem 0x-0x1fff] [0.00] Initmem setup node 0 [mem 0x-0x1fff] [0.00] On node 0 totalpages: 131072 [0.00] Normal zone: 1024 pages used for memmap [0.00] Normal zone: 0 pages reserved [0.00] Normal zone: 131072 pages, LIFO batch:31 [0.00] pcpu-alloc: s0 r0 d32768 u32768 alloc=1*32768 [0.00] pcpu-alloc: [0] 0 [0.00] Built 1 zonelists, mobility grouping on. Total pages: 130048 [0.00] Kernel command line: ip=192.168.0.3:192.168.0.1::255.0.0.0:vgoippro:eth0:off console=ttyS0,115200 [0.00] Dentry cache hash table entries: 65536 (order: 6, 262144 bytes, linear) [0.00] Inode-cache hash table entries: 32768 (order: 5, 131072 bytes, linear) [0.00] mem auto-init: stack:off, heap alloc:off, heap free:off [0.00] Memory: 503516K/524288K available (7532K kernel code, 2236K rwdata, 1328K rodata, 1500K init, 931K bss, 20772K reserved, 0K cma-reserved) [0.00] Kernel virtual memory layout: [0.00] * 0xff7ff000..0xf000 : fixmap [0.00] * 0xff7fd000..0xff7ff000 : early ioremap [0.00] * 0xe100..0xff7fd000 : vmalloc & ioremap [0.00] SLUB: HWalign=32, Order=0-3, MinObjects=0, CPUs=1, Nodes=1 [0.00] rcu: Preemptible hierarchical RCU implementation. [0.00] rcu: RCU event tracing is enabled. [0.00] Trampoline variant of Tasks RCU enabled. [0.00] rcu: RCU calculated value of scheduler-enlistment delay is 10 jiffies. [0.00] NR_IRQS: 512, nr_irqs: 512, preallocated irqs: 16 [0.00] IPIC (128 IRQ sources) at (ptrval) [0.00] kfence: initialized - using 2097152 bytes for 255 objects at 0x(ptrval)-0x(ptrval) ... [4.472455] # Subtest: kfence [4.472490] 1..25 [4.476069] # test_out_of_bounds_read: test_alloc: size=32, gfp=cc0, policy=left, cache=0 [4.946420] == [4.953667] BUG: KFENCE: out-of-bounds read in test_out_of_bounds_read+0x90/0x228 [4.953667] [4.962657] Out-of-bounds read at 0x(ptrval) (1B left of kfence-#23): [4.969109] test_out_of_bounds_read+0x90/0x228 [4.973663] kunit_try_run_case+0x5c/0xd0 [4.977712] kunit_generic_run_threadfn_adapter+0x24/0x30 [4.983128] kthread+0x15c/0x174 [4.986387] ret_from_kernel_thread+0x14/0x1c [4.990774] [4.992274] kfence-#23 [0x(ptrval)-0x(ptrval), size=32, cache=kmalloc-32] allocated by task 91: [
Re: [RFC PATCH v1] powerpc: Enable KFENCE for PPC32
On Tue, 2 Mar 2021 at 09:37, Christophe Leroy wrote: > Add architecture specific implementation details for KFENCE and enable > KFENCE for the ppc32 architecture. In particular, this implements the > required interface in . Nice! > KFENCE requires that attributes for pages from its memory pool can > individually be set. Therefore, force the Read/Write linear map to be > mapped at page granularity. > > Unit tests succeed on all tests but one: > > [ 15.053324] # test_invalid_access: EXPECTATION FAILED at > mm/kfence/kfence_test.c:636 > [ 15.053324] Expected report_matches() to be true, but > is false > [ 15.068359] not ok 21 - test_invalid_access This is strange, given all the other tests passed. Do you mind sharing the full test log? Thanks, -- Marco
[RFC PATCH v1] powerpc: Enable KFENCE for PPC32
Add architecture specific implementation details for KFENCE and enable
KFENCE for the ppc32 architecture. In particular, this implements the
required interface in .
KFENCE requires that attributes for pages from its memory pool can
individually be set. Therefore, force the Read/Write linear map to be
mapped at page granularity.
Unit tests succeed on all tests but one:
[ 15.053324] # test_invalid_access: EXPECTATION FAILED at
mm/kfence/kfence_test.c:636
[ 15.053324] Expected report_matches() to be true, but is
false
[ 15.068359] not ok 21 - test_invalid_access
Signed-off-by: Christophe Leroy
---
arch/powerpc/Kconfig | 13 +++--
arch/powerpc/include/asm/kfence.h | 32 +++
arch/powerpc/mm/book3s32/mmu.c| 2 +-
arch/powerpc/mm/fault.c | 7 ++-
arch/powerpc/mm/init_32.c | 3 +++
arch/powerpc/mm/nohash/8xx.c | 5 +++--
6 files changed, 52 insertions(+), 10 deletions(-)
create mode 100644 arch/powerpc/include/asm/kfence.h
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 386ae12d8523..d46db0bfb998 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -185,6 +185,7 @@ config PPC
select HAVE_ARCH_KASAN if PPC32 && PPC_PAGE_SHIFT <= 14
select HAVE_ARCH_KASAN_VMALLOC if PPC32 && PPC_PAGE_SHIFT <= 14
select HAVE_ARCH_KGDB
+ select HAVE_ARCH_KFENCE if PPC32
select HAVE_ARCH_MMAP_RND_BITS
select HAVE_ARCH_MMAP_RND_COMPAT_BITS if COMPAT
select HAVE_ARCH_NVRAM_OPS
@@ -786,7 +787,7 @@ config THREAD_SHIFT
config DATA_SHIFT_BOOL
bool "Set custom data alignment"
depends on ADVANCED_OPTIONS
- depends on STRICT_KERNEL_RWX || DEBUG_PAGEALLOC
+ depends on STRICT_KERNEL_RWX || DEBUG_PAGEALLOC || KFENCE
depends on PPC_BOOK3S_32 || (PPC_8xx && !PIN_TLB_DATA &&
!STRICT_KERNEL_RWX)
help
This option allows you to set the kernel data alignment. When
@@ -798,13 +799,13 @@ config DATA_SHIFT_BOOL
config DATA_SHIFT
int "Data shift" if DATA_SHIFT_BOOL
default 24 if STRICT_KERNEL_RWX && PPC64
- range 17 28 if (STRICT_KERNEL_RWX || DEBUG_PAGEALLOC) && PPC_BOOK3S_32
- range 19 23 if (STRICT_KERNEL_RWX || DEBUG_PAGEALLOC) && PPC_8xx
+ range 17 28 if (STRICT_KERNEL_RWX || DEBUG_PAGEALLOC || KFENCE) &&
PPC_BOOK3S_32
+ range 19 23 if (STRICT_KERNEL_RWX || DEBUG_PAGEALLOC || KFENCE) &&
PPC_8xx
default 22 if STRICT_KERNEL_RWX && PPC_BOOK3S_32
- default 18 if DEBUG_PAGEALLOC && PPC_BOOK3S_32
+ default 18 if (DEBUG_PAGEALLOC || KFENCE) && PPC_BOOK3S_32
default 23 if STRICT_KERNEL_RWX && PPC_8xx
- default 23 if DEBUG_PAGEALLOC && PPC_8xx && PIN_TLB_DATA
- default 19 if DEBUG_PAGEALLOC && PPC_8xx
+ default 23 if (DEBUG_PAGEALLOC || KFENCE) && PPC_8xx && PIN_TLB_DATA
+ default 19 if (DEBUG_PAGEALLOC || KFENCE) && PPC_8xx
default PPC_PAGE_SHIFT
help
On Book3S 32 (603+), DBATs are used to map kernel text and rodata RO.
diff --git a/arch/powerpc/include/asm/kfence.h
b/arch/powerpc/include/asm/kfence.h
new file mode 100644
index ..c229ee6a48f0
--- /dev/null
+++ b/arch/powerpc/include/asm/kfence.h
@@ -0,0 +1,32 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * powerpc KFENCE support.
+ *
+ * Copyright (C) 2020 CS GROUP France
+ */
+
+#ifndef __ASM_POWERPC_KFENCE_H
+#define __ASM_POWERPC_KFENCE_H
+
+#include
+
+static inline bool arch_kfence_init_pool(void)
+{
+ return true;
+}
+
+static inline bool kfence_protect_page(unsigned long addr, bool protect)
+{
+ pte_t *kpte = virt_to_kpte(addr);
+
+ if (protect) {
+ pte_update(_mm, addr, kpte, _PAGE_PRESENT, 0, 0);
+ flush_tlb_kernel_range(addr, addr + PAGE_SIZE);
+ } else {
+ pte_update(_mm, addr, kpte, 0, _PAGE_PRESENT, 0);
+ }
+
+ return true;
+}
+
+#endif /* __ASM_POWERPC_KFENCE_H */
diff --git a/arch/powerpc/mm/book3s32/mmu.c b/arch/powerpc/mm/book3s32/mmu.c
index d7eb266a3f7a..4548aec95561 100644
--- a/arch/powerpc/mm/book3s32/mmu.c
+++ b/arch/powerpc/mm/book3s32/mmu.c
@@ -162,7 +162,7 @@ unsigned long __init mmu_mapin_ram(unsigned long base,
unsigned long top)
unsigned long border = (unsigned long)__init_begin - PAGE_OFFSET;
- if (debug_pagealloc_enabled() || __map_without_bats) {
+ if (debug_pagealloc_enabled() || __map_without_bats ||
IS_ENABLED(CONFIG_KFENCE)) {
pr_debug_once("Read-Write memory mapped without BATs\n");
if (base >= border)
return base;
diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
index bb368257b55c..bea13682c909 100644
--- a/arch/powerpc/mm/fault.c
+++ b/arch/powerpc/mm/fault.c
@@ -32,6 +32,7 @@
#include
#include
#include
+#include
#include
#include
@@ -418,8
