Messages in this thread | | | From | Marco Elver <> | Date | Tue, 2 Mar 2021 12:39:12 +0100 | Subject | Re: [RFC PATCH v1] powerpc: Enable KFENCE for PPC32 |
| |
On Tue, 2 Mar 2021 at 12:21, Christophe Leroy <christophe.leroy@csgroup.eu> 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?
Thanks, -- Marco
| |