Messages in this thread Patch in this message | | | Subject | Re: [RFC PATCH v1] powerpc: Enable KFENCE for PPC32 | From | Christophe Leroy <> | Date | Fri, 5 Mar 2021 09:23:00 +0100 |
| |
Le 05/03/2021 à 08:50, Marco Elver a écrit : > On Fri, Mar 05, 2021 at 04:01PM +1100, Michael Ellerman wrote: >> Marco Elver <elver@google.com> 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 >>>>> <christophe.leroy@csgroup.eu> 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 0xc00000003c9c0000 (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 <asm/kfence.h> 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-elver@google.com/ > https://lkml.kernel.org/r/afaec81a551ef15345cb7d7563b3fac3d7041c3a.1614868445.git.christophe.leroy@csgroup.eu > > Since KFENCE requires <asm/kfence.h> 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
| |