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:

quoted

Marco Elver [off-list ref] writes:

quoted

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++;
+

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/ (local)
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