Re: [RFC PATCH v1] powerpc: Enable KFENCE for PPC32
From: Christophe Leroy
Date: Wed Mar 03 2021 - 10:48:39 EST
Le 02/03/2021 à 12:39, Marco Elver a écrit :
On Tue, 2 Mar 2021 at 12:21, Christophe Leroy
<christophe.leroy@xxxxxxxxxx> 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: G B
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: G B
(5.12.0-rc1-s3k-dev-01534-g4f14ae75edf0-dirty)
[ 16.911386] MSR: 00009032 <EE,ME,IR,DR,RI> CR: 22000004 XER: 00000000
[ 16.918153] DAR: df98800a DSISR: 20000000
[ 16.918153] GPR00: c02f50dc e2449e50 c1140d00 e100dd24 c084b13c 00000008 c084b32b c016eb38
[ 16.918153] GPR08: c0850000 df988000 c0d10000 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 39000000 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(&expect) 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(&expect));
}
But as I said above, this is fragile. If for some reason one day test_alloc() gets inlined, it may
not work anymore.
Yeah, obviously that's hack, but interesting nevertheless.
Based on what you say above, however, it seems that
stack_trace_save_regs()/arch_stack_walk() don't exactly do what they
should? Can they be fixed for ppc32?
Can we really consider they don't do what they should ?
I have the feeling that excepting entry[0] of the stack trace to match the instruction pointer is
not a valid expectation. That's probably correct on architectures that always have a stack frame for
any function, but for powerpc who can have frameless functions, we can't expect that I think.
I have proposed a change to KFENCE in another response to this mail thread, could it be the solution ?
Thanks
Christophe