[PATCH v8 0/3] ARM: Implement MODULE_PLT support in FTRACE

[PATCH v8 0/3] ARM: Implement MODULE_PLT support in FTRACE

[Alexander A Sverdlin]

From: Alexander Sverdlin <alexander.sverdlin@nokia.com>

FTRACE's function tracer currently doesn't always work on ARM with
MODULE_PLT option enabled. If the module is loaded too far, FTRACE's
code modifier cannot cope with introduced veneers and turns the
function tracer off globally.

ARM64 already has a solution for the problem, refer to the following
patches:

arm64: ftrace: emit ftrace-mod.o contents through code
arm64: module-plts: factor out PLT generation code for ftrace
arm64: ftrace: fix !CONFIG_ARM64_MODULE_PLTS kernels
arm64: ftrace: fix building without CONFIG_MODULES
arm64: ftrace: add support for far branches to dynamic ftrace
arm64: ftrace: don't validate branch via PLT in ftrace_make_nop()

But the presented ARM variant has just a half of the footprint in terms of
the changed LoCs. It also retains the code validation-before-modification
instead of switching it off.

Changelog:
v8:
* Add warn suppress parameter to arm_gen_branch_link()
v7:
* rebased
v6:
* rebased
v5:
* BUILD_BUG_ON() ensures fixed_plts[] always fits one PLT block
* use "for" loop instead of "while"
* scripts/recordmcount is filtering reloc types
v4:
* Fixed build without CONFIG_FUNCTION_TRACER
* Reorganized pre-allocated PLTs handling in get_module_plt(),
  now compiler eliminates the whole FTRACE-related handling code
    if ARRAY_SIZE(fixed_plts) == 0
    v3:
    * Only extend struct dyn_arch_ftrace when ARM_MODULE_PLTS is enabled
    v2:
    * As suggested by Steven Rostedt, refrain from tree-wide API modification,
      save module pointer in struct dyn_arch_ftrace instead (PowerPC way)

Alexander Sverdlin (3):
  ARM: PLT: Move struct plt_entries definition to header
  ARM: Add warn suppress parameter to arm_gen_branch_link()
  ARM: ftrace: Add MODULE_PLTS support

 arch/arm/include/asm/ftrace.h |  3 +++
 arch/arm/include/asm/insn.h   |  8 +++----
 arch/arm/include/asm/module.h | 10 +++++++++
 arch/arm/kernel/ftrace.c      | 46 +++++++++++++++++++++++++++++++++-------
 arch/arm/kernel/insn.c        | 19 +++++++++--------
 arch/arm/kernel/module-plts.c | 49 +++++++++++++++++++++++++++++++++----------
 6 files changed, 103 insertions(+), 32 deletions(-)

-- 
2.10.2

[PATCH v8 1/3] ARM: PLT: Move struct plt_entries definition to header

[Alexander A Sverdlin]

From: Alexander Sverdlin <alexander.sverdlin@nokia.com>

No functional change, later it will be re-used in several files.

Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>
---
 arch/arm/include/asm/module.h | 9 +++++++++
 arch/arm/kernel/module-plts.c | 9 ---------
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/arch/arm/include/asm/module.h b/arch/arm/include/asm/module.h
index 4b0df09..09b9ad5 100644
--- a/arch/arm/include/asm/module.h
+++ b/arch/arm/include/asm/module.h
@@ -19,6 +19,15 @@ enum {
 };
 #endif
 
+#define PLT_ENT_STRIDE		L1_CACHE_BYTES
+#define PLT_ENT_COUNT		(PLT_ENT_STRIDE / sizeof(u32))
+#define PLT_ENT_SIZE		(sizeof(struct plt_entries) / PLT_ENT_COUNT)
+
+struct plt_entries {
+	u32	ldr[PLT_ENT_COUNT];
+	u32	lit[PLT_ENT_COUNT];
+};
+
 struct mod_plt_sec {
 	struct elf32_shdr	*plt;
 	int			plt_count;
diff --git a/arch/arm/kernel/module-plts.c b/arch/arm/kernel/module-plts.c
index 6e626ab..d330e9e 100644
--- a/arch/arm/kernel/module-plts.c
+++ b/arch/arm/kernel/module-plts.c
@@ -12,10 +12,6 @@
 #include <asm/cache.h>
 #include <asm/opcodes.h>
 
-#define PLT_ENT_STRIDE		L1_CACHE_BYTES
-#define PLT_ENT_COUNT		(PLT_ENT_STRIDE / sizeof(u32))
-#define PLT_ENT_SIZE		(sizeof(struct plt_entries) / PLT_ENT_COUNT)
-
 #ifdef CONFIG_THUMB2_KERNEL
 #define PLT_ENT_LDR		__opcode_to_mem_thumb32(0xf8dff000 | \
 							(PLT_ENT_STRIDE - 4))
@@ -24,11 +20,6 @@
 						    (PLT_ENT_STRIDE - 8))
 #endif
 
-struct plt_entries {
-	u32	ldr[PLT_ENT_COUNT];
-	u32	lit[PLT_ENT_COUNT];
-};
-
 static bool in_init(const struct module *mod, unsigned long loc)
 {
 	return loc - (u32)mod->init_layout.base < mod->init_layout.size;
-- 
2.10.2

[PATCH v8 2/3] ARM: Add warn suppress parameter to arm_gen_branch_link()

[Alexander A Sverdlin]

From: Alexander Sverdlin <alexander.sverdlin@nokia.com>

Will be used in the following patch. No functional change.

Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>
---
 arch/arm/include/asm/insn.h |  8 ++++----
 arch/arm/kernel/ftrace.c    |  2 +-
 arch/arm/kernel/insn.c      | 19 ++++++++++---------
 3 files changed, 15 insertions(+), 14 deletions(-)

diff --git a/arch/arm/include/asm/insn.h b/arch/arm/include/asm/insn.h
index f20e08a..5475cbf 100644
--- a/arch/arm/include/asm/insn.h
+++ b/arch/arm/include/asm/insn.h
@@ -13,18 +13,18 @@ arm_gen_nop(void)
 }
 
 unsigned long
-__arm_gen_branch(unsigned long pc, unsigned long addr, bool link);
+__arm_gen_branch(unsigned long pc, unsigned long addr, bool link, bool warn);
 
 static inline unsigned long
 arm_gen_branch(unsigned long pc, unsigned long addr)
 {
-	return __arm_gen_branch(pc, addr, false);
+	return __arm_gen_branch(pc, addr, false, true);
 }
 
 static inline unsigned long
-arm_gen_branch_link(unsigned long pc, unsigned long addr)
+arm_gen_branch_link(unsigned long pc, unsigned long addr, bool warn)
 {
-	return __arm_gen_branch(pc, addr, true);
+	return __arm_gen_branch(pc, addr, true, warn);
 }
 
 #endif
diff --git a/arch/arm/kernel/ftrace.c b/arch/arm/kernel/ftrace.c
index 9a79ef6..61de817 100644
--- a/arch/arm/kernel/ftrace.c
+++ b/arch/arm/kernel/ftrace.c
@@ -70,7 +70,7 @@ int ftrace_arch_code_modify_post_process(void)
 
 static unsigned long ftrace_call_replace(unsigned long pc, unsigned long addr)
 {
-	return arm_gen_branch_link(pc, addr);
+	return arm_gen_branch_link(pc, addr, true);
 }
 
 static int ftrace_modify_code(unsigned long pc, unsigned long old,
diff --git a/arch/arm/kernel/insn.c b/arch/arm/kernel/insn.c
index 2e844b7..db0acbb 100644
--- a/arch/arm/kernel/insn.c
+++ b/arch/arm/kernel/insn.c
@@ -3,8 +3,9 @@
 #include <linux/kernel.h>
 #include <asm/opcodes.h>
 
-static unsigned long
-__arm_gen_branch_thumb2(unsigned long pc, unsigned long addr, bool link)
+static unsigned long __arm_gen_branch_thumb2(unsigned long pc,
+					     unsigned long addr, bool link,
+					     bool warn)
 {
 	unsigned long s, j1, j2, i1, i2, imm10, imm11;
 	unsigned long first, second;
@@ -12,7 +13,7 @@ __arm_gen_branch_thumb2(unsigned long pc, unsigned long addr, bool link)
 
 	offset = (long)addr - (long)(pc + 4);
 	if (offset < -16777216 || offset > 16777214) {
-		WARN_ON_ONCE(1);
+		WARN_ON_ONCE(warn);
 		return 0;
 	}
 
@@ -33,8 +34,8 @@ __arm_gen_branch_thumb2(unsigned long pc, unsigned long addr, bool link)
 	return __opcode_thumb32_compose(first, second);
 }
 
-static unsigned long
-__arm_gen_branch_arm(unsigned long pc, unsigned long addr, bool link)
+static unsigned long __arm_gen_branch_arm(unsigned long pc, unsigned long addr,
+					  bool link, bool warn)
 {
 	unsigned long opcode = 0xea000000;
 	long offset;
@@ -44,7 +45,7 @@ __arm_gen_branch_arm(unsigned long pc, unsigned long addr, bool link)
 
 	offset = (long)addr - (long)(pc + 8);
 	if (unlikely(offset < -33554432 || offset > 33554428)) {
-		WARN_ON_ONCE(1);
+		WARN_ON_ONCE(warn);
 		return 0;
 	}
 
@@ -54,10 +55,10 @@ __arm_gen_branch_arm(unsigned long pc, unsigned long addr, bool link)
 }
 
 unsigned long
-__arm_gen_branch(unsigned long pc, unsigned long addr, bool link)
+__arm_gen_branch(unsigned long pc, unsigned long addr, bool link, bool warn)
 {
 	if (IS_ENABLED(CONFIG_THUMB2_KERNEL))
-		return __arm_gen_branch_thumb2(pc, addr, link);
+		return __arm_gen_branch_thumb2(pc, addr, link, warn);
 	else
-		return __arm_gen_branch_arm(pc, addr, link);
+		return __arm_gen_branch_arm(pc, addr, link, warn);
 }
-- 
2.10.2

[PATCH v8 3/3] ARM: ftrace: Add MODULE_PLTS support

[Alexander A Sverdlin]

From: Alexander Sverdlin <alexander.sverdlin@nokia.com>

Teach ftrace_make_call() and ftrace_make_nop() about PLTs.
Teach PLT code about FTRACE and all its callbacks.
Otherwise the following might happen:

------------[ cut here ]------------
WARNING: CPU: 14 PID: 2265 at .../arch/arm/kernel/insn.c:14 __arm_gen_branch+0x83/0x8c()
...
Hardware name: LSI Axxia AXM55XX
[<c0314a49>] (unwind_backtrace) from [<c03115e9>] (show_stack+0x11/0x14)
[<c03115e9>] (show_stack) from [<c0519f51>] (dump_stack+0x81/0xa8)
[<c0519f51>] (dump_stack) from [<c032185d>] (warn_slowpath_common+0x69/0x90)
[<c032185d>] (warn_slowpath_common) from [<c03218f3>] (warn_slowpath_null+0x17/0x1c)
[<c03218f3>] (warn_slowpath_null) from [<c03143cf>] (__arm_gen_branch+0x83/0x8c)
[<c03143cf>] (__arm_gen_branch) from [<c0314337>] (ftrace_make_nop+0xf/0x24)
[<c0314337>] (ftrace_make_nop) from [<c038ebcb>] (ftrace_process_locs+0x27b/0x3e8)
[<c038ebcb>] (ftrace_process_locs) from [<c0378d79>] (load_module+0x11e9/0x1a44)
[<c0378d79>] (load_module) from [<c037974d>] (SyS_finit_module+0x59/0x84)
[<c037974d>] (SyS_finit_module) from [<c030e981>] (ret_fast_syscall+0x1/0x18)
---[ end trace e1b64ced7a89adcc ]---
------------[ cut here ]------------
WARNING: CPU: 14 PID: 2265 at .../kernel/trace/ftrace.c:1979 ftrace_bug+0x1b1/0x234()
...
Hardware name: LSI Axxia AXM55XX
[<c0314a49>] (unwind_backtrace) from [<c03115e9>] (show_stack+0x11/0x14)
[<c03115e9>] (show_stack) from [<c0519f51>] (dump_stack+0x81/0xa8)
[<c0519f51>] (dump_stack) from [<c032185d>] (warn_slowpath_common+0x69/0x90)
[<c032185d>] (warn_slowpath_common) from [<c03218f3>] (warn_slowpath_null+0x17/0x1c)
[<c03218f3>] (warn_slowpath_null) from [<c038e87d>] (ftrace_bug+0x1b1/0x234)
[<c038e87d>] (ftrace_bug) from [<c038ebd5>] (ftrace_process_locs+0x285/0x3e8)
[<c038ebd5>] (ftrace_process_locs) from [<c0378d79>] (load_module+0x11e9/0x1a44)
[<c0378d79>] (load_module) from [<c037974d>] (SyS_finit_module+0x59/0x84)
[<c037974d>] (SyS_finit_module) from [<c030e981>] (ret_fast_syscall+0x1/0x18)
---[ end trace e1b64ced7a89adcd ]---
ftrace failed to modify [<e9ef7006>] 0xe9ef7006
actual: 02:f0:3b:fa
ftrace record flags: 0
(0) expected tramp: c0314265

Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>
---
 arch/arm/include/asm/ftrace.h |  3 +++
 arch/arm/include/asm/module.h |  1 +
 arch/arm/kernel/ftrace.c      | 46 +++++++++++++++++++++++++++++++++++--------
 arch/arm/kernel/module-plts.c | 44 +++++++++++++++++++++++++++++++++++++----
 4 files changed, 82 insertions(+), 12 deletions(-)

diff --git a/arch/arm/include/asm/ftrace.h b/arch/arm/include/asm/ftrace.h
index 48ec1d0..a4dbac0 100644
--- a/arch/arm/include/asm/ftrace.h
+++ b/arch/arm/include/asm/ftrace.h
@@ -15,6 +15,9 @@ extern void __gnu_mcount_nc(void);
 
 #ifdef CONFIG_DYNAMIC_FTRACE
 struct dyn_arch_ftrace {
+#ifdef CONFIG_ARM_MODULE_PLTS
+	struct module *mod;
+#endif
 };
 
 static inline unsigned long ftrace_call_adjust(unsigned long addr)
diff --git a/arch/arm/include/asm/module.h b/arch/arm/include/asm/module.h
index 09b9ad5..cfffae6 100644
--- a/arch/arm/include/asm/module.h
+++ b/arch/arm/include/asm/module.h
@@ -30,6 +30,7 @@ struct plt_entries {
 
 struct mod_plt_sec {
 	struct elf32_shdr	*plt;
+	struct plt_entries	*plt_ent;
 	int			plt_count;
 };
 
diff --git a/arch/arm/kernel/ftrace.c b/arch/arm/kernel/ftrace.c
index 61de817..3c83b5d 100644
--- a/arch/arm/kernel/ftrace.c
+++ b/arch/arm/kernel/ftrace.c
@@ -68,9 +68,10 @@ int ftrace_arch_code_modify_post_process(void)
 	return 0;
 }
 
-static unsigned long ftrace_call_replace(unsigned long pc, unsigned long addr)
+static unsigned long ftrace_call_replace(unsigned long pc, unsigned long addr,
+					 bool warn)
 {
-	return arm_gen_branch_link(pc, addr, true);
+	return arm_gen_branch_link(pc, addr, warn);
 }
 
 static int ftrace_modify_code(unsigned long pc, unsigned long old,
@@ -104,14 +105,14 @@ int ftrace_update_ftrace_func(ftrace_func_t func)
 	int ret;
 
 	pc = (unsigned long)&ftrace_call;
-	new = ftrace_call_replace(pc, (unsigned long)func);
+	new = ftrace_call_replace(pc, (unsigned long)func, true);
 
 	ret = ftrace_modify_code(pc, 0, new, false);
 
 #ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
 	if (!ret) {
 		pc = (unsigned long)&ftrace_regs_call;
-		new = ftrace_call_replace(pc, (unsigned long)func);
+		new = ftrace_call_replace(pc, (unsigned long)func, true);
 
 		ret = ftrace_modify_code(pc, 0, new, false);
 	}
@@ -124,10 +125,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);
+	struct module *mod = NULL;
+
+#ifdef CONFIG_ARM_MODULE_PLTS
+	mod = rec->arch.mod;
+#endif
 
 	old = ftrace_nop_replace(rec);
 
-	new = ftrace_call_replace(ip, adjust_address(rec, addr));
+	new = ftrace_call_replace(ip, aaddr, !mod);
+#ifdef CONFIG_ARM_MODULE_PLTS
+	if (!new && mod) {
+		aaddr = get_module_plt(mod, ip, aaddr);
+		new = ftrace_call_replace(ip, aaddr, true);
+	}
+#endif
 
 	return ftrace_modify_code(rec->ip, old, new, true);
 }
@@ -140,9 +153,9 @@ int ftrace_modify_call(struct dyn_ftrace *rec, unsigned long old_addr,
 	unsigned long new, old;
 	unsigned long ip = rec->ip;
 
-	old = ftrace_call_replace(ip, adjust_address(rec, old_addr));
+	old = ftrace_call_replace(ip, adjust_address(rec, old_addr), true);
 
-	new = ftrace_call_replace(ip, adjust_address(rec, addr));
+	new = ftrace_call_replace(ip, adjust_address(rec, addr), true);
 
 	return ftrace_modify_code(rec->ip, old, new, true);
 }
@@ -152,12 +165,29 @@ int ftrace_modify_call(struct dyn_ftrace *rec, unsigned long old_addr,
 int ftrace_make_nop(struct module *mod,
 		    struct dyn_ftrace *rec, unsigned long addr)
 {
+	unsigned long aaddr = adjust_address(rec, addr);
 	unsigned long ip = rec->ip;
 	unsigned long old;
 	unsigned long new;
 	int ret;
 
-	old = ftrace_call_replace(ip, adjust_address(rec, addr));
+#ifdef CONFIG_ARM_MODULE_PLTS
+	/* mod is only supplied during module loading */
+	if (!mod)
+		mod = rec->arch.mod;
+	else
+		rec->arch.mod = mod;
+#endif
+
+	old = ftrace_call_replace(ip, aaddr,
+				  !IS_ENABLED(CONFIG_ARM_MODULE_PLTS) || !mod);
+#ifdef CONFIG_ARM_MODULE_PLTS
+	if (!old && mod) {
+		aaddr = get_module_plt(mod, ip, aaddr);
+		old = ftrace_call_replace(ip, aaddr, true);
+	}
+#endif
+
 	new = ftrace_nop_replace(rec);
 	ret = ftrace_modify_code(ip, old, new, true);
 
diff --git a/arch/arm/kernel/module-plts.c b/arch/arm/kernel/module-plts.c
index d330e9e..a0524ad 100644
--- a/arch/arm/kernel/module-plts.c
+++ b/arch/arm/kernel/module-plts.c
@@ -4,6 +4,7 @@
  */
 
 #include <linux/elf.h>
+#include <linux/ftrace.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/sort.h>
@@ -20,19 +21,52 @@
 						    (PLT_ENT_STRIDE - 8))
 #endif
 
+static const u32 fixed_plts[] = {
+#ifdef CONFIG_FUNCTION_TRACER
+	FTRACE_ADDR,
+	MCOUNT_ADDR,
+#endif
+};
+
 static bool in_init(const struct module *mod, unsigned long loc)
 {
 	return loc - (u32)mod->init_layout.base < mod->init_layout.size;
 }
 
+static void prealloc_fixed(struct mod_plt_sec *pltsec, struct plt_entries *plt)
+{
+	int i;
+
+	if (!ARRAY_SIZE(fixed_plts) || pltsec->plt_count)
+		return;
+	pltsec->plt_count = ARRAY_SIZE(fixed_plts);
+
+	for (i = 0; i < ARRAY_SIZE(plt->ldr); ++i)
+		plt->ldr[i] = PLT_ENT_LDR;
+
+	BUILD_BUG_ON(sizeof(fixed_plts) > sizeof(plt->lit));
+	memcpy(plt->lit, fixed_plts, sizeof(fixed_plts));
+}
+
 u32 get_module_plt(struct module *mod, unsigned long loc, Elf32_Addr val)
 {
 	struct mod_plt_sec *pltsec = !in_init(mod, loc) ? &mod->arch.core :
 							  &mod->arch.init;
+	struct plt_entries *plt;
+	int idx;
+
+	/* cache the address, ELF header is available only during module load */
+	if (!pltsec->plt_ent)
+		pltsec->plt_ent = (struct plt_entries *)pltsec->plt->sh_addr;
+	plt = pltsec->plt_ent;
 
-	struct plt_entries *plt = (struct plt_entries *)pltsec->plt->sh_addr;
-	int idx = 0;
+	prealloc_fixed(pltsec, plt);
+
+	for (idx = 0; idx < ARRAY_SIZE(fixed_plts); ++idx)
+		if (plt->lit[idx] == val)
+			return (u32)&plt->ldr[idx];
 
+	idx = 0;
 	/*
 	 * Look for an existing entry pointing to 'val'. Given that the
 	 * relocations are sorted, this will be the last entry we allocated.
@@ -180,8 +214,8 @@ static unsigned int count_plts(const Elf32_Sym *syms, Elf32_Addr base,
 int module_frob_arch_sections(Elf_Ehdr *ehdr, Elf_Shdr *sechdrs,
 			      char *secstrings, struct module *mod)
 {
-	unsigned long core_plts = 0;
-	unsigned long init_plts = 0;
+	unsigned long core_plts = ARRAY_SIZE(fixed_plts);
+	unsigned long init_plts = ARRAY_SIZE(fixed_plts);
 	Elf32_Shdr *s, *sechdrs_end = sechdrs + ehdr->e_shnum;
 	Elf32_Sym *syms = NULL;
 
@@ -236,6 +270,7 @@ int module_frob_arch_sections(Elf_Ehdr *ehdr, Elf_Shdr *sechdrs,
 	mod->arch.core.plt->sh_size = round_up(core_plts * PLT_ENT_SIZE,
 					       sizeof(struct plt_entries));
 	mod->arch.core.plt_count = 0;
+	mod->arch.core.plt_ent = NULL;
 
 	mod->arch.init.plt->sh_type = SHT_NOBITS;
 	mod->arch.init.plt->sh_flags = SHF_EXECINSTR | SHF_ALLOC;
@@ -243,6 +278,7 @@ int module_frob_arch_sections(Elf_Ehdr *ehdr, Elf_Shdr *sechdrs,
 	mod->arch.init.plt->sh_size = round_up(init_plts * PLT_ENT_SIZE,
 					       sizeof(struct plt_entries));
 	mod->arch.init.plt_count = 0;
+	mod->arch.init.plt_ent = NULL;
 
 	pr_debug("%s: plt=%x, init.plt=%x\n", __func__,
 		 mod->arch.core.plt->sh_size, mod->arch.init.plt->sh_size);
-- 
2.10.2

Re: [PATCH v8 0/3] ARM: Implement MODULE_PLT support in FTRACE

[Florian Fainelli]

On 3/30/21 4:40 AM, Alexander A Sverdlin wrote:
> From: Alexander Sverdlin <alexander.sverdlin@nokia.com>
> 
> FTRACE's function tracer currently doesn't always work on ARM with
> MODULE_PLT option enabled. If the module is loaded too far, FTRACE's
> code modifier cannot cope with introduced veneers and turns the
> function tracer off globally.
> 
> ARM64 already has a solution for the problem, refer to the following
> patches:
> 
> arm64: ftrace: emit ftrace-mod.o contents through code
> arm64: module-plts: factor out PLT generation code for ftrace
> arm64: ftrace: fix !CONFIG_ARM64_MODULE_PLTS kernels
> arm64: ftrace: fix building without CONFIG_MODULES
> arm64: ftrace: add support for far branches to dynamic ftrace
> arm64: ftrace: don't validate branch via PLT in ftrace_make_nop()
> 
> But the presented ARM variant has just a half of the footprint in terms of
> the changed LoCs. It also retains the code validation-before-modification
> instead of switching it off.
> 
> Changelog:
> v8:
> * Add warn suppress parameter to arm_gen_branch_link()
> v7:
> * rebased
> v6:
> * rebased
> v5:
> * BUILD_BUG_ON() ensures fixed_plts[] always fits one PLT block
> * use "for" loop instead of "while"
> * scripts/recordmcount is filtering reloc types
> v4:
> * Fixed build without CONFIG_FUNCTION_TRACER
> * Reorganized pre-allocated PLTs handling in get_module_plt(),
>   now compiler eliminates the whole FTRACE-related handling code
>     if ARRAY_SIZE(fixed_plts) == 0
>     v3:
>     * Only extend struct dyn_arch_ftrace when ARM_MODULE_PLTS is enabled
>     v2:
>     * As suggested by Steven Rostedt, refrain from tree-wide API modification,
>       save module pointer in struct dyn_arch_ftrace instead (PowerPC way)
> 
> Alexander Sverdlin (3):
>   ARM: PLT: Move struct plt_entries definition to header
>   ARM: Add warn suppress parameter to arm_gen_branch_link()
>   ARM: ftrace: Add MODULE_PLTS support

Tested-by: Florian Fainelli <f.fainelli@gmail.com>

Thanks a lot!
-- 
Florian

Re: [PATCH v8 0/3] ARM: Implement MODULE_PLT support in FTRACE

[Florian Fainelli]

On 3/30/21 4:40 AM, Alexander A Sverdlin wrote:
> From: Alexander Sverdlin <alexander.sverdlin@nokia.com>
> 
> FTRACE's function tracer currently doesn't always work on ARM with
> MODULE_PLT option enabled. If the module is loaded too far, FTRACE's
> code modifier cannot cope with introduced veneers and turns the
> function tracer off globally.
> 
> ARM64 already has a solution for the problem, refer to the following
> patches:
> 
> arm64: ftrace: emit ftrace-mod.o contents through code
> arm64: module-plts: factor out PLT generation code for ftrace
> arm64: ftrace: fix !CONFIG_ARM64_MODULE_PLTS kernels
> arm64: ftrace: fix building without CONFIG_MODULES
> arm64: ftrace: add support for far branches to dynamic ftrace
> arm64: ftrace: don't validate branch via PLT in ftrace_make_nop()
> 
> But the presented ARM variant has just a half of the footprint in terms of
> the changed LoCs. It also retains the code validation-before-modification
> instead of switching it off.
> 
> Changelog:
> v8:
> * Add warn suppress parameter to arm_gen_branch_link()
> v7:
> * rebased
> v6:
> * rebased
> v5:
> * BUILD_BUG_ON() ensures fixed_plts[] always fits one PLT block
> * use "for" loop instead of "while"
> * scripts/recordmcount is filtering reloc types
> v4:
> * Fixed build without CONFIG_FUNCTION_TRACER
> * Reorganized pre-allocated PLTs handling in get_module_plt(),
>   now compiler eliminates the whole FTRACE-related handling code
>     if ARRAY_SIZE(fixed_plts) == 0
>     v3:
>     * Only extend struct dyn_arch_ftrace when ARM_MODULE_PLTS is enabled
>     v2:
>     * As suggested by Steven Rostedt, refrain from tree-wide API modification,
>       save module pointer in struct dyn_arch_ftrace instead (PowerPC way)

FWIW, ftracetest did not pick up new failures (nor were new tests fixed)
with this patch series.
-- 
Florian

Re: [PATCH v8 0/3] ARM: Implement MODULE_PLT support in FTRACE

[Qais Yousef]

Adding Linus Walleij back to CC

On 03/30/21 13:40, Alexander A Sverdlin wrote:
> From: Alexander Sverdlin <alexander.sverdlin@nokia.com>
> 
> FTRACE's function tracer currently doesn't always work on ARM with
> MODULE_PLT option enabled. If the module is loaded too far, FTRACE's
> code modifier cannot cope with introduced veneers and turns the
> function tracer off globally.
> 
> ARM64 already has a solution for the problem, refer to the following
> patches:
> 
> arm64: ftrace: emit ftrace-mod.o contents through code
> arm64: module-plts: factor out PLT generation code for ftrace
> arm64: ftrace: fix !CONFIG_ARM64_MODULE_PLTS kernels
> arm64: ftrace: fix building without CONFIG_MODULES
> arm64: ftrace: add support for far branches to dynamic ftrace
> arm64: ftrace: don't validate branch via PLT in ftrace_make_nop()
> 
> But the presented ARM variant has just a half of the footprint in terms of
> the changed LoCs. It also retains the code validation-before-modification
> instead of switching it off.
> 
> Changelog:
> v8:
> * Add warn suppress parameter to arm_gen_branch_link()

I still think the ifdefery in patch 3 is ugly. Any reason my suggestion didn't
work out for you? I struggle to see how this is better and why it was hard to
incorporate my suggestion.

For example

	-       old = ftrace_call_replace(ip, adjust_address(rec, addr));
	+#ifdef CONFIG_ARM_MODULE_PLTS
	+       /* mod is only supplied during module loading */
	+       if (!mod)
	+               mod = rec->arch.mod;
	+       else
	+               rec->arch.mod = mod;
	+#endif
	+
	+       old = ftrace_call_replace(ip, aaddr,
	+                                 !IS_ENABLED(CONFIG_ARM_MODULE_PLTS) || !mod);
	+#ifdef CONFIG_ARM_MODULE_PLTS
	+       if (!old && mod) {
	+               aaddr = get_module_plt(mod, ip, aaddr);
	+               old = ftrace_call_replace(ip, aaddr, true);
	+       }
	+#endif
	+

There's an ifdef, followed by a code that embeds
!IS_ENABLED(CONFIG_ARM_MODULE_PLTS) followed by another ifdef :-/

And there was no need to make the new warn arg visible all the way to
ftrace_call_repalce() and all of its users.

FWIW

Tested-by: Qais Yousef <qais.yousef@arm.com>

If this gets accepted as-is, I'll send a patch to improve on this.


Thanks

--
Qais Yousef

Re: [PATCH v8 0/3] ARM: Implement MODULE_PLT support in FTRACE

[Alexander Sverdlin]

Hi!

On 09/04/2021 17:33, Qais Yousef wrote:
> I still think the ifdefery in patch 3 is ugly. Any reason my suggestion didn't
> work out for you? I struggle to see how this is better and why it was hard to
> incorporate my suggestion.
> 
> For example
> 
> 	-       old = ftrace_call_replace(ip, adjust_address(rec, addr));
> 	+#ifdef CONFIG_ARM_MODULE_PLTS
> 	+       /* mod is only supplied during module loading */
> 	+       if (!mod)
> 	+               mod = rec->arch.mod;
> 	+       else
> 	+               rec->arch.mod = mod;
> 	+#endif
> 	+
> 	+       old = ftrace_call_replace(ip, aaddr,
> 	+                                 !IS_ENABLED(CONFIG_ARM_MODULE_PLTS) || !mod);
> 	+#ifdef CONFIG_ARM_MODULE_PLTS
> 	+       if (!old && mod) {
> 	+               aaddr = get_module_plt(mod, ip, aaddr);
> 	+               old = ftrace_call_replace(ip, aaddr, true);
> 	+       }
> 	+#endif
> 	+
> 
> There's an ifdef, followed by a code that embeds
> !IS_ENABLED(CONFIG_ARM_MODULE_PLTS) followed by another ifdef :-/

No, it's actually two small ifdefed blocks added before and after an original call,
which parameters have been modified as well. The issue with arch.mod was explained
by Steven Rostedt, maybe you've missed his email.
 
> And there was no need to make the new warn arg visible all the way to
> ftrace_call_repalce() and all of its users.
> 
> FWIW
> 
> Tested-by: Qais Yousef <qais.yousef@arm.com>
-- 
Best regards,
Alexander Sverdlin.

Re: [PATCH v8 0/3] ARM: Implement MODULE_PLT support in FTRACE

[Qais Yousef]

Hi Alexander

Fixing Ard's email as the Linaro one keeps bouncing back. Please fix that in
your future postings.

On 04/12/21 08:28, Alexander Sverdlin wrote:
> Hi!
> 
> On 09/04/2021 17:33, Qais Yousef wrote:
> > I still think the ifdefery in patch 3 is ugly. Any reason my suggestion didn't
> > work out for you? I struggle to see how this is better and why it was hard to
> > incorporate my suggestion.
> > 
> > For example
> > 
> > 	-       old = ftrace_call_replace(ip, adjust_address(rec, addr));
> > 	+#ifdef CONFIG_ARM_MODULE_PLTS
> > 	+       /* mod is only supplied during module loading */
> > 	+       if (!mod)
> > 	+               mod = rec->arch.mod;
> > 	+       else
> > 	+               rec->arch.mod = mod;
> > 	+#endif
> > 	+
> > 	+       old = ftrace_call_replace(ip, aaddr,
> > 	+                                 !IS_ENABLED(CONFIG_ARM_MODULE_PLTS) || !mod);
> > 	+#ifdef CONFIG_ARM_MODULE_PLTS
> > 	+       if (!old && mod) {
> > 	+               aaddr = get_module_plt(mod, ip, aaddr);
> > 	+               old = ftrace_call_replace(ip, aaddr, true);
> > 	+       }
> > 	+#endif
> > 	+
> > 
> > There's an ifdef, followed by a code that embeds
> > !IS_ENABLED(CONFIG_ARM_MODULE_PLTS) followed by another ifdef :-/
> 
> No, it's actually two small ifdefed blocks added before and after an original call,
> which parameters have been modified as well. The issue with arch.mod was explained
> by Steven Rostedt, maybe you've missed his email.

If you're referring to arch.mod having to be protected by the ifdef I did
address that. Please look at my patch.

My comment here refers to the ugliness of this ifdefery. Introducing 2 simple
wrapper functions would address that as I've demonstrated in my
suggestion/patch.

Thanks

--
Qais Yousef

Re: [PATCH v8 0/3] ARM: Implement MODULE_PLT support in FTRACE

[Florian Fainelli]

On 4/12/2021 4:08 AM, Qais Yousef wrote:
> Hi Alexander
> 
> Fixing Ard's email as the Linaro one keeps bouncing back. Please fix that in
> your future postings.
> 
> On 04/12/21 08:28, Alexander Sverdlin wrote:
>> Hi!
>>
>> On 09/04/2021 17:33, Qais Yousef wrote:
>>> I still think the ifdefery in patch 3 is ugly. Any reason my suggestion didn't
>>> work out for you? I struggle to see how this is better and why it was hard to
>>> incorporate my suggestion.
>>>
>>> For example
>>>
>>> 	-       old = ftrace_call_replace(ip, adjust_address(rec, addr));
>>> 	+#ifdef CONFIG_ARM_MODULE_PLTS
>>> 	+       /* mod is only supplied during module loading */
>>> 	+       if (!mod)
>>> 	+               mod = rec->arch.mod;
>>> 	+       else
>>> 	+               rec->arch.mod = mod;
>>> 	+#endif
>>> 	+
>>> 	+       old = ftrace_call_replace(ip, aaddr,
>>> 	+                                 !IS_ENABLED(CONFIG_ARM_MODULE_PLTS) || !mod);
>>> 	+#ifdef CONFIG_ARM_MODULE_PLTS
>>> 	+       if (!old && mod) {
>>> 	+               aaddr = get_module_plt(mod, ip, aaddr);
>>> 	+               old = ftrace_call_replace(ip, aaddr, true);
>>> 	+       }
>>> 	+#endif
>>> 	+
>>>
>>> There's an ifdef, followed by a code that embeds
>>> !IS_ENABLED(CONFIG_ARM_MODULE_PLTS) followed by another ifdef :-/
>>
>> No, it's actually two small ifdefed blocks added before and after an original call,
>> which parameters have been modified as well. The issue with arch.mod was explained
>> by Steven Rostedt, maybe you've missed his email.
> 
> If you're referring to arch.mod having to be protected by the ifdef I did
> address that. Please look at my patch.
> 
> My comment here refers to the ugliness of this ifdefery. Introducing 2 simple
> wrapper functions would address that as I've demonstrated in my
> suggestion/patch.

What is the plan to move forward with this patch series, should v8 be
submitted into RMK's patch tracker and improved upon from there, or do
you feel like your suggestion needs to be addressed right away?
-- 
Florian

Re: [PATCH v8 0/3] ARM: Implement MODULE_PLT support in FTRACE

[Qais Yousef]

On 04/19/21 14:54, Florian Fainelli wrote:
> 
> 
> On 4/12/2021 4:08 AM, Qais Yousef wrote:
> > Hi Alexander
> > 
> > Fixing Ard's email as the Linaro one keeps bouncing back. Please fix that in
> > your future postings.
> > 
> > On 04/12/21 08:28, Alexander Sverdlin wrote:
> >> Hi!
> >>
> >> On 09/04/2021 17:33, Qais Yousef wrote:
> >>> I still think the ifdefery in patch 3 is ugly. Any reason my suggestion didn't
> >>> work out for you? I struggle to see how this is better and why it was hard to
> >>> incorporate my suggestion.
> >>>
> >>> For example
> >>>
> >>> 	-       old = ftrace_call_replace(ip, adjust_address(rec, addr));
> >>> 	+#ifdef CONFIG_ARM_MODULE_PLTS
> >>> 	+       /* mod is only supplied during module loading */
> >>> 	+       if (!mod)
> >>> 	+               mod = rec->arch.mod;
> >>> 	+       else
> >>> 	+               rec->arch.mod = mod;
> >>> 	+#endif
> >>> 	+
> >>> 	+       old = ftrace_call_replace(ip, aaddr,
> >>> 	+                                 !IS_ENABLED(CONFIG_ARM_MODULE_PLTS) || !mod);
> >>> 	+#ifdef CONFIG_ARM_MODULE_PLTS
> >>> 	+       if (!old && mod) {
> >>> 	+               aaddr = get_module_plt(mod, ip, aaddr);
> >>> 	+               old = ftrace_call_replace(ip, aaddr, true);
> >>> 	+       }
> >>> 	+#endif
> >>> 	+
> >>>
> >>> There's an ifdef, followed by a code that embeds
> >>> !IS_ENABLED(CONFIG_ARM_MODULE_PLTS) followed by another ifdef :-/
> >>
> >> No, it's actually two small ifdefed blocks added before and after an original call,
> >> which parameters have been modified as well. The issue with arch.mod was explained
> >> by Steven Rostedt, maybe you've missed his email.
> > 
> > If you're referring to arch.mod having to be protected by the ifdef I did
> > address that. Please look at my patch.
> > 
> > My comment here refers to the ugliness of this ifdefery. Introducing 2 simple
> > wrapper functions would address that as I've demonstrated in my
> > suggestion/patch.
> 
> What is the plan to move forward with this patch series, should v8 be
> submitted into RMK's patch tracker and improved upon from there, or do
> you feel like your suggestion needs to be addressed right away?

There's no objection from my side to submitting this and improve later.

Thanks

--
Qais Yousef

Re: [PATCH v8 0/3] ARM: Implement MODULE_PLT support in FTRACE

[Florian Fainelli]

On 4/19/21 3:34 PM, Qais Yousef wrote:
> On 04/19/21 14:54, Florian Fainelli wrote:
>>
>>
>> On 4/12/2021 4:08 AM, Qais Yousef wrote:
>>> Hi Alexander
>>>
>>> Fixing Ard's email as the Linaro one keeps bouncing back. Please fix that in
>>> your future postings.
>>>
>>> On 04/12/21 08:28, Alexander Sverdlin wrote:
>>>> Hi!
>>>>
>>>> On 09/04/2021 17:33, Qais Yousef wrote:
>>>>> I still think the ifdefery in patch 3 is ugly. Any reason my suggestion didn't
>>>>> work out for you? I struggle to see how this is better and why it was hard to
>>>>> incorporate my suggestion.
>>>>>
>>>>> For example
>>>>>
>>>>> 	-       old = ftrace_call_replace(ip, adjust_address(rec, addr));
>>>>> 	+#ifdef CONFIG_ARM_MODULE_PLTS
>>>>> 	+       /* mod is only supplied during module loading */
>>>>> 	+       if (!mod)
>>>>> 	+               mod = rec->arch.mod;
>>>>> 	+       else
>>>>> 	+               rec->arch.mod = mod;
>>>>> 	+#endif
>>>>> 	+
>>>>> 	+       old = ftrace_call_replace(ip, aaddr,
>>>>> 	+                                 !IS_ENABLED(CONFIG_ARM_MODULE_PLTS) || !mod);
>>>>> 	+#ifdef CONFIG_ARM_MODULE_PLTS
>>>>> 	+       if (!old && mod) {
>>>>> 	+               aaddr = get_module_plt(mod, ip, aaddr);
>>>>> 	+               old = ftrace_call_replace(ip, aaddr, true);
>>>>> 	+       }
>>>>> 	+#endif
>>>>> 	+
>>>>>
>>>>> There's an ifdef, followed by a code that embeds
>>>>> !IS_ENABLED(CONFIG_ARM_MODULE_PLTS) followed by another ifdef :-/
>>>>
>>>> No, it's actually two small ifdefed blocks added before and after an original call,
>>>> which parameters have been modified as well. The issue with arch.mod was explained
>>>> by Steven Rostedt, maybe you've missed his email.
>>>
>>> If you're referring to arch.mod having to be protected by the ifdef I did
>>> address that. Please look at my patch.
>>>
>>> My comment here refers to the ugliness of this ifdefery. Introducing 2 simple
>>> wrapper functions would address that as I've demonstrated in my
>>> suggestion/patch.
>>
>> What is the plan to move forward with this patch series, should v8 be
>> submitted into RMK's patch tracker and improved upon from there, or do
>> you feel like your suggestion needs to be addressed right away?
> 
> There's no objection from my side to submitting this and improve later.

OK, thanks! Alexander, do you mind sending these patches to RMK's patch
tracker: https://www.armlinux.org.uk/developer/patches/?

Thank you!
-- 
Florian

Re: [PATCH v8 0/3] ARM: Implement MODULE_PLT support in FTRACE

[Alexander Sverdlin]

Hi Florian!

On 04/05/2021 21:11, Florian Fainelli wrote:
>>> What is the plan to move forward with this patch series, should v8 be
>>> submitted into RMK's patch tracker and improved upon from there, or do
>>> you feel like your suggestion needs to be addressed right away?
>> There's no objection from my side to submitting this and improve later.
> OK, thanks! Alexander, do you mind sending these patches to RMK's patch
> tracker: https://www.armlinux.org.uk/developer/patches/?

Done!

-- 
Best regards,
Alexander Sverdlin.