Re: [PATCH 3/3] arm64: module/ftrace: Fix mcount-based ftrace initialization failure

[Li Huafei <lihuafei1@huawei.com>]

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.

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);
 }

Thanks,
Huafei

> Thanks,
> Mark.
> 
>>
>> Thanks,
>> Mark.
>>
>> ---->8----
>> diff --git a/arch/arm64/kernel/ftrace.c b/arch/arm64/kernel/ftrace.c
>> index ea5dc7c90f46..ba9b76ea5e68 100644
>> --- a/arch/arm64/kernel/ftrace.c
>> +++ b/arch/arm64/kernel/ftrace.c
>> @@ -216,6 +216,17 @@ 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;
>>  
>>  	if (!ftrace_find_callable_addr(rec, mod, &addr))
>>  		return -EINVAL;
>> @@ -223,7 +234,7 @@ int ftrace_make_nop(struct module *mod, struct dyn_ftrace *rec,
>>  	old = aarch64_insn_gen_branch_imm(pc, addr, AARCH64_INSN_BRANCH_LINK);
>>  	new = aarch64_insn_gen_nop();
>>  
>> -	return ftrace_modify_code(pc, old, new, true);
>> +	return ftrace_modify_code(pc, old, new, validate);
>>  }
>>  
>>  void arch_ftrace_update_code(int command)
> 
> .
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel