Re: [RFC PATCH v1] powerpc: Enable KFENCE for PPC32

Marco Elver [off-list ref] writes:

On Thu, Mar 04, 2021 at 12:48PM +0100, Christophe Leroy wrote:

quoted

Le 04/03/2021 à 12:31, Marco Elver a écrit :

quoted

On Thu, 4 Mar 2021 at 12:23, Christophe Leroy
[off-list ref] wrote:

quoted

Le 03/03/2021 à 11:56, Marco Elver a écrit :

quoted

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]

[...]

quoted

quoted

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++;
+
 		if (str_has_prefix(buf, "kfence_") || str_has_prefix(buf, "__kfence_") ||
 		    !strncmp(buf, "__slab_free", len)) {
 			/*

cheers