On 03/08/21 08:58, Alexander Sverdlin wrote:

Hi!

On 07/03/2021 18:26, Qais Yousef wrote:

quoted

I tried on 5.12-rc2 and 5.11 but couldn't reproduce the problem using your

I still can't reproduce on 5.12-rc2.

I do have CONFIG_ARM_MODULE_PLTS=y. Do you need to do something else after
loading the module? I tried starting ftrace, but maybe there's a particular
combination required?

quoted

instructions on the other email. But most likely because I'm hitting another
problem that could be masking it. I'm not sure it is related or just randomly
happened to hit it.

Did you see something similar?

[...]

quoted

	[    0.000000] [<c1b01a38>] (ftrace_bug) from [<c046316c>] (ftrace_process_locs+0x2b0/0x518)
	[    0.000000]  r7:c3817ac4 r6:c38040c0 r5:00000a3c r4:000134e4
	[    0.000000] [<c0462ebc>] (ftrace_process_locs) from [<c2b25240>] (ftrace_init+0xc8/0x174)
	[    0.000000]  r10:c2ffa000 r9:c2be8a78 r8:c2c5d1fc r7:c2c0c208 r6:00000001 r5:c2d0908c
	[    0.000000]  r4:c362f518
	[    0.000000] [<c2b25178>] (ftrace_init) from [<c2b00e14>] (start_kernel+0x2f4/0x5b8)
	[    0.000000]  r9:c2be8a78 r8:dbfffec0 r7:00000000 r6:c36385cc r5:c2d08f00 r4:c2ffa000
	[    0.000000] [<c2b00b20>] (start_kernel) from [<00000000>] (0x0)

This means, FTRACE has more problems with your kernel/compiler/platform, I've addressed similar issue
in the past, but my patch should be long merged:

https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1817963.html

Could it be the same problem as here:
https://www.spinics.net/lists/arm-kernel/msg854022.html

Seems that the size check deserves something line BUILD_BUG_ON() with FTRACE...

So I only see this when I convert all modules to be built-in

	sed -i 's/=m/=y/' .config

FWIW, I see the problem with your patch applied too. Trying to dig more into
it..

quoted

quoted

diff --git a/arch/arm/kernel/ftrace.c b/arch/arm/kernel/ftrace.c
index 9a79ef6..fa867a5 100644
--- a/arch/arm/kernel/ftrace.c
+++ b/arch/arm/kernel/ftrace.c

@@ -70,6 +70,19 @@ int ftrace_arch_code_modify_post_process(void)
 
 static unsigned long ftrace_call_replace(unsigned long pc, unsigned long addr)
 {
+	s32 offset = addr - pc;
+	s32 blim = 0xfe000008;
+	s32 flim = 0x02000004;

This look like magic numbers to me..

These magic numbers are most probably the reason for your FTRACE to resign...
Those are backward- and forward-branch limits. I didn't find the matching DEFINEs
in the kernel, but I would be happy to learn them. I can also put some comments,
but I actually thought the purpose would be obvious from the code...

So I did dig more into it. The range is asymmetrical indeed. And the strange
offset is to cater for the pc being incremented by +8 (+4 for thumb2).

You're duplicating the checks in __arm_gen_branch_{thumb2, arm}(). As you noted
__arm_gen_branch() which is called by arm_gen_branch_link() will end up doing
the exact same check and return 0. So why do you need to duplicate the check
here? We can do something about the WARN_ON_ONCE(1).

[...]

quoted

quoted

+
 	return arm_gen_branch_link(pc, addr);
 }
 

@@ -124,10 +137,22 @@ int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
 {
 	unsigned long new, old;
 	unsigned long ip = rec->ip;
+	unsigned long aaddr = adjust_address(rec, addr);
 
 	old = ftrace_nop_replace(rec);
 
-	new = ftrace_call_replace(ip, adjust_address(rec, addr));
+	new = ftrace_call_replace(ip, aaddr);
+
+#ifdef CONFIG_ARM_MODULE_PLTS
+	if (!new) {
+		struct module *mod = rec->arch.mod;
+
+		if (mod) {

What would happen if !new and !mod?

I believe, that's exactly what happens in the dump you experience with your kernel.
This is not covered by this patch, this patch covers the issue with modules in vmalloc area.

quoted

quoted

+			aaddr = get_module_plt(mod, ip, aaddr);
+			new = ftrace_call_replace(ip, aaddr);

I assume we're guaranteed to have a sensible value returned in 'new' here?

Otherwise you'd see the dump you see :)
It relies on the already existing error handling.

I understand from this there are still loose ends to be handled in this area of
the code.

I admit I need to spend more time to understand why I get the failure above and
how this overlaps with your proposal. But as it stands it seems there's more
work to be done here.

Thanks

--
Qais Yousef

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