Le 03/03/2021 à 15:38, Marco Elver a écrit :
On Wed, 3 Mar 2021 at 15:09, Christophe Leroy
<christophe.le...@csgroup.eu> wrote:
It seems like all other sane architectures, namely x86 and arm64
at least, include the running function as top entry when saving
stack trace.
Functionnalities like KFENCE expect it.
Do the same on powerpc, it allows KFENCE to properly identify the faulting
function as depicted below. Before the patch KFENCE was identifying
finish_task_switch.isra as the faulting function.
[ 14.937370]
==================================================================
[ 14.948692] BUG: KFENCE: invalid read in test_invalid_access+0x54/0x108
[ 14.948692]
[ 14.956814] Invalid read at 0xdf98800a:
[ 14.960664] test_invalid_access+0x54/0x108
[ 14.964876] finish_task_switch.isra.0+0x54/0x23c
[ 14.969606] kunit_try_run_case+0x5c/0xd0
[ 14.973658] kunit_generic_run_threadfn_adapter+0x24/0x30
[ 14.979079] kthread+0x15c/0x174
[ 14.982342] ret_from_kernel_thread+0x14/0x1c
[ 14.986731]
[ 14.988236] CPU: 0 PID: 111 Comm: kunit_try_catch Tainted: G B
5.12.0-rc1-01537-g95f6e2088d7e-dirty #4682
[ 14.999795] NIP: c016ec2c LR: c02f517c CTR: c016ebd8
[ 15.004851] REGS: e2449d90 TRAP: 0301 Tainted: G B
(5.12.0-rc1-01537-g95f6e2088d7e-dirty)
[ 15.015274] MSR: 00009032 <EE,ME,IR,DR,RI> CR: 22000004 XER: 00000000
[ 15.022043] DAR: df98800a DSISR: 20000000
[ 15.022043] GPR00: c02f517c e2449e50 c1142080 e100dd24 c084b13c 00000008
c084b32b c016ebd8
[ 15.022043] GPR08: c0850000 df988000 c0d10000 e2449eb0 22000288
[ 15.040581] NIP [c016ec2c] test_invalid_access+0x54/0x108
[ 15.046010] LR [c02f517c] kunit_try_run_case+0x5c/0xd0
[ 15.051181] Call Trace:
[ 15.053637] [e2449e50] [c005a68c] finish_task_switch.isra.0+0x54/0x23c
(unreliable)
[ 15.061338] [e2449eb0] [c02f517c] kunit_try_run_case+0x5c/0xd0
[ 15.067215] [e2449ed0] [c02f648c]
kunit_generic_run_threadfn_adapter+0x24/0x30
[ 15.074472] [e2449ef0] [c004e7b0] kthread+0x15c/0x174
[ 15.079571] [e2449f30] [c001317c] ret_from_kernel_thread+0x14/0x1c
[ 15.085798] Instruction dump:
[ 15.088784] 8129d608 38e7ebd8 81020280 911f004c 39000000 995f0024 907f0028
90ff001c
[ 15.096613] 3949000a 915f0020 3d40c0d1 3d00c085 <8929000a> 3908adb0 812a4b98
3d40c02f
[ 15.104612]
==================================================================
Signed-off-by: Christophe Leroy <christophe.le...@csgroup.eu>
Acked-by: Marco Elver <el...@google.com>
Thank you, I think this looks like the right solution. Just a question below:
...
@@ -59,23 +70,26 @@ void save_stack_trace(struct stack_trace *trace)
sp = current_stack_frame();
- save_context_stack(trace, sp, current, 1);
+ save_context_stack(trace, sp, (unsigned long)save_stack_trace, current,
1);
This causes ip == save_stack_trace and also below for
save_stack_trace_tsk. Does this mean save_stack_trace() is included in
the trace? Looking at kernel/stacktrace.c, I think the library wants
to exclude itself from the trace, as it does '.skip = skipnr + 1' (and
'.skip = skipnr + (current == tsk)' for the _tsk variant).
If the arch-helper here is included, should this use _RET_IP_ instead?
Don't really know, I was inspired by arm64 which has:
void arch_stack_walk(stack_trace_consume_fn consume_entry, void *cookie,
struct task_struct *task, struct pt_regs *regs)
{
struct stackframe frame;
if (regs)
start_backtrace(&frame, regs->regs[29], regs->pc);
else if (task == current)
start_backtrace(&frame,
(unsigned long)__builtin_frame_address(0),
(unsigned long)arch_stack_walk);
else
start_backtrace(&frame, thread_saved_fp(task),
thread_saved_pc(task));
walk_stackframe(task, &frame, consume_entry, cookie);
}
But looking at x86 you may be right, so what should be done really ?
Thanks
Christophe
