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

Marco Elver
2021-03-03T16:32:51Z

On Wed, 3 Mar 2021 at 11:39, Christophe Leroy
<christophe.leroy@csgroup.eu> wrote:
>
>
>
> Le 02/03/2021 à 12:39, Marco Elver a écrit :
> > 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?
>
> 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 ?

You're going to have to change all users of stack_trace_print/save
across the kernel, because the assumption is that the current frame is
included.

It is just bad design if we add special code to all users of the
<linux/stacktrace.h> API just so we can print the current frame at the
top of the trace. Therefore, I'm afraid your proposed patch to KFENCE
is not acceptable.

Instead, we have to either extend the <linux/stacktrace.h> API, or
simply accept that all current users of the API expect the current
frame to be included. If you do not want to include the current frame,
that API even provides a way to skip it already (just pass +1 as
skipnr).

<linux/stacktrace.h> writes this about arch_stack_walk():

* task NULL Stack trace from task (can be current)
* current regs Stack trace starting on regs->stackpointer

This is a bit vague, and unfortunately seems outdated, but I'd assume
that when it says "Stack trace from task" would be the stack trace
including the current function (at IP) being executed.

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.

Thanks,
-- Marco