Messages in this thread | | | Date | Thu, 29 Sep 2022 14:41:51 +0100 | From | Mark Rutland <> | Subject | Re: [PATCH 3/3] arm64: module/ftrace: Fix mcount-based ftrace initialization failure |
| |
On Thu, Sep 29, 2022 at 08:26:17PM +0800, Li Huafei wrote: > > > On 2022/9/29 19:59, Mark Rutland wrote: > > On Thu, Sep 29, 2022 at 12:26:52PM +0100, Mark Rutland wrote: > >> On Thu, Sep 29, 2022 at 05:41:34PM +0800, Li Huafei wrote: > >>> The commit a6253579977e ("arm64: ftrace: consistently handle PLTs.") > >>> makes ftrace_make_nop() always validate the 'old' instruction that will > >>> be replaced. However, in the mcount-based implementation, > >>> ftrace_init_nop() also calls ftrace_make_nop() to do the initialization, > >>> and the 'old' target address is MCOUNT_ADDR at this time. with > >>> CONFIG_MODULE_PLT support, the distance between MCOUNT_ADDR and callsite > >>> may exceed 128M, at which point ftrace_find_callable_addr() will fail > >>> because it cannot find an available PLT. > >> > >> Ah, sorry about this. > >> > >>> We can reproduce this problem by forcing the module to alloc memory away > >>> from the kernel: > >>> > >>> ftrace_test: loading out-of-tree module taints kernel. > >>> ftrace: no module PLT for _mcount > >>> ------------[ ftrace bug ]------------ > >>> ftrace failed to modify > >>> [<ffff800029180014>] 0xffff800029180014 > >>> actual: 44:00:00:94 > >>> Initializing ftrace call sites > >>> ftrace record flags: 2000000 > >>> (0) > >>> expected tramp: ffff80000802eb3c > >>> ------------[ cut here ]------------ > >>> WARNING: CPU: 3 PID: 157 at kernel/trace/ftrace.c:2120 ftrace_bug+0x94/0x270 > >>> Modules linked in: > >>> CPU: 3 PID: 157 Comm: insmod Tainted: G O 6.0.0-rc6-00151-gcd722513a189-dirty #22 > >>> Hardware name: linux,dummy-virt (DT) > >>> pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--) > >>> pc : ftrace_bug+0x94/0x270 > >>> lr : ftrace_bug+0x21c/0x270 > >>> sp : ffff80000b2bbaf0 > >>> x29: ffff80000b2bbaf0 x28: 0000000000000000 x27: ffff0000c4d38000 > >>> x26: 0000000000000001 x25: ffff800009d7e000 x24: ffff0000c4d86e00 > >>> x23: 0000000002000000 x22: ffff80000a62b000 x21: ffff8000098ebea8 > >>> x20: ffff0000c4d38000 x19: ffff80000aa24158 x18: ffffffffffffffff > >>> x17: 0000000000000000 x16: 0a0d2d2d2d2d2d2d x15: ffff800009aa9118 > >>> x14: 0000000000000000 x13: 6333626532303830 x12: 3030303866666666 > >>> x11: 203a706d61727420 x10: 6465746365707865 x9 : 3362653230383030 > >>> x8 : c0000000ffffefff x7 : 0000000000017fe8 x6 : 000000000000bff4 > >>> x5 : 0000000000057fa8 x4 : 0000000000000000 x3 : 0000000000000001 > >>> x2 : ad2cb14bb5438900 x1 : 0000000000000000 x0 : 0000000000000022 > >>> Call trace: > >>> ftrace_bug+0x94/0x270 > >>> ftrace_process_locs+0x308/0x430 > >>> ftrace_module_init+0x44/0x60 > >>> load_module+0x15b4/0x1ce8 > >>> __do_sys_init_module+0x1ec/0x238 > >>> __arm64_sys_init_module+0x24/0x30 > >>> invoke_syscall+0x54/0x118 > >>> el0_svc_common.constprop.4+0x84/0x100 > >>> do_el0_svc+0x3c/0xd0 > >>> el0_svc+0x1c/0x50 > >>> el0t_64_sync_handler+0x90/0xb8 > >>> el0t_64_sync+0x15c/0x160 > >>> ---[ end trace 0000000000000000 ]--- > >>> ---------test_init----------- > >>> > >>> In fact, in .init.plt or .plt or both of them, we have the mcount PLT. > >>> If we save the mcount PLT entry address, we can determine what the 'old' > >>> instruction should be when initializing the nop instruction. > >>> > >>> Fixes: a6253579977e ("arm64: ftrace: consistently handle PLTs.") > >>> Signed-off-by: Li Huafei <lihuafei1@huawei.com> > >>> --- > >>> arch/arm64/include/asm/module.h | 7 +++++++ > >>> arch/arm64/kernel/ftrace.c | 29 ++++++++++++++++++++++++++++- > >>> arch/arm64/kernel/module-plts.c | 16 ++++++++++++++++ > >>> arch/arm64/kernel/module.c | 11 +++++++++++ > >>> 4 files changed, 62 insertions(+), 1 deletion(-) > >> > >> Since this only matters for the initalization of a module callsite, I'd rather > >> we simply didn't check in this case, so that we don't have to go scanning for > >> the PLTs and keep that information around forever. > >> > >> To be honest, I'd rather we simply didn't check when initializing an mcount > >> call-site for a module, as we used to do prior to commit a6253579977e. > > Yes, I agree. If it's just for the initialization phase validation, my patch does make a bit of a fuss. > > >> > >> Does the below work for you? > > > > Thinking some more, that's probably going to warn in the insn code when > > unconditionally generating the 'old' branch; I'll spin a new version after some > > testing. > > > > I see it. And ftrace_find_callable_addr() would still fail.
Ah, yes, since that points to the `_mcount` stub, but we'll generate the address of the module's ftrace PLT.
> > With a slight modification, it worked for me: > > diff --git a/arch/arm64/kernel/ftrace.c b/arch/arm64/kernel/ftrace.c > index ea5dc7c90f46..621c62238d96 100644 > --- a/arch/arm64/kernel/ftrace.c > +++ b/arch/arm64/kernel/ftrace.c > @@ -216,14 +216,28 @@ int ftrace_make_nop(struct module *mod, struct dyn_ftrace *rec, > { > unsigned long pc = rec->ip; > u32 old = 0, new; > + bool validate = true; > + > + /* > + * When using mcount, calls can be indirected via a PLT generated by > + * the toolchain. Ignore this when initializing the callsite. > + * > + * Note: `mod` is only set at module load time. > + */ > + if (!IS_ENABLED(CONFIG_DYNAMIC_FTRACE_WITH_REGS) && > + IS_ENABLED(CONFIG_ARM64_MODULE_PLTS) && mod) { > + validate = false; > + goto make_nop; > + } > > if (!ftrace_find_callable_addr(rec, mod, &addr)) > return -EINVAL; > > old = aarch64_insn_gen_branch_imm(pc, addr, AARCH64_INSN_BRANCH_LINK); > +make_nop: > new = aarch64_insn_gen_nop(); > > - return ftrace_modify_code(pc, old, new, true); > + return ftrace_modify_code(pc, old, new, validate); > }
Great; I'll clean this up a bit and post as a patch shortly.
Thanks, Mark.
| |