[PATCH v4 0/2] ARM: Implement MODULE_PLT support in FTRACE

[PATCH v4 0/2] ARM: Implement MODULE_PLT support in FTRACE

[Alexander Sverdlin]

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:
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 (2):
  ARM: PLT: Move struct plt_entries definition to header
  ARM: ftrace: Add MODULE_PLTS support

 arch/arm/include/asm/ftrace.h |  3 +++
 arch/arm/include/asm/module.h | 10 +++++++
 arch/arm/kernel/ftrace.c      | 62 ++++++++++++++++++++++++++++++++++++-------
 arch/arm/kernel/module-plts.c | 52 ++++++++++++++++++++++++++++--------
 4 files changed, 107 insertions(+), 20 deletions(-)

-- 
2.4.6

[PATCH v4 1/2] ARM: PLT: Move struct plt_entries definition to header

[Alexander Sverdlin]

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 89ad059..6996405 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 3d0c2e4..f272711 100644
--- a/arch/arm/kernel/module-plts.c
+++ b/arch/arm/kernel/module-plts.c
@@ -14,10 +14,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))
@@ -26,11 +22,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.4.6

[PATCH v4 2/2] ARM: ftrace: Add MODULE_PLTS support

[Alexander Sverdlin]

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      | 62 ++++++++++++++++++++++++++++++++++++-------
 arch/arm/kernel/module-plts.c | 47 +++++++++++++++++++++++++++++---
 4 files changed, 100 insertions(+), 13 deletions(-)

diff --git a/arch/arm/include/asm/ftrace.h b/arch/arm/include/asm/ftrace.h
index 9e842ff..faeb6b1 100644
--- a/arch/arm/include/asm/ftrace.h
+++ b/arch/arm/include/asm/ftrace.h
@@ -19,6 +19,9 @@ struct dyn_arch_ftrace {
 #ifdef CONFIG_OLD_MCOUNT
 	bool	old_mcount;
 #endif
+#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 6996405..e3d7a51 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 5617932..b55355f 100644
--- a/arch/arm/kernel/ftrace.c
+++ b/arch/arm/kernel/ftrace.c
@@ -98,6 +98,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;
+
+	if (IS_ENABLED(CONFIG_THUMB2_KERNEL)) {
+		blim = 0xff000004;
+		flim = 0x01000002;
+	}
+
+	if (IS_ENABLED(CONFIG_ARM_MODULE_PLTS) &&
+	    (offset < blim || offset > flim))
+		return 0;
+
 	return arm_gen_branch_link(pc, addr);
 }
 
@@ -166,10 +179,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) {
+			aaddr = get_module_plt(mod, ip, aaddr);
+			new = ftrace_call_replace(ip, aaddr);
+		}
+	}
+#endif
 
 	return ftrace_modify_code(rec->ip, old, new, true);
 }
@@ -199,20 +224,39 @@ int ftrace_make_nop(struct module *mod,
 	unsigned long new;
 	int ret;
 
-	old = ftrace_call_replace(ip, adjust_address(rec, addr));
-	new = ftrace_nop_replace(rec);
-	ret = ftrace_modify_code(ip, old, new, true);
+#ifdef CONFIG_ARM_MODULE_PLTS
+	/* mod is only supplied during module loading */
+	if (!mod)
+		mod = rec->arch.mod;
+	else
+		rec->arch.mod = mod;
+#endif
 
-#ifdef CONFIG_OLD_MCOUNT
-	if (ret == -EINVAL && addr == MCOUNT_ADDR) {
-		rec->arch.old_mcount = true;
+	for (;;) {
+		unsigned long aaddr = adjust_address(rec, addr);
+
+		old = ftrace_call_replace(ip, aaddr);
+
+#ifdef CONFIG_ARM_MODULE_PLTS
+		if (!old && mod) {
+			aaddr = get_module_plt(mod, ip, aaddr);
+			old = ftrace_call_replace(ip, aaddr);
+		}
+#endif
 
-		old = ftrace_call_replace(ip, adjust_address(rec, addr));
 		new = ftrace_nop_replace(rec);
 		ret = ftrace_modify_code(ip, old, new, true);
-	}
+
+#ifdef CONFIG_OLD_MCOUNT
+		if (ret == -EINVAL && !rec->arch.old_mcount) {
+			rec->arch.old_mcount = true;
+			continue;
+		}
 #endif
 
+		break;
+	}
+
 	return ret;
 }
 
diff --git a/arch/arm/kernel/module-plts.c b/arch/arm/kernel/module-plts.c
index f272711..0951270 100644
--- a/arch/arm/kernel/module-plts.c
+++ b/arch/arm/kernel/module-plts.c
@@ -7,6 +7,7 @@
  */
 
 #include <linux/elf.h>
+#include <linux/ftrace.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/sort.h>
@@ -22,18 +23,54 @@
 						    (PLT_ENT_STRIDE - 8))
 #endif
 
+static const u32 fixed_plts[] = {
+#ifdef CONFIG_FUNCTION_TRACER
+	FTRACE_ADDR,
+	MCOUNT_ADDR,
+#ifdef CONFIG_OLD_MCOUNT
+	(unsigned long)ftrace_caller_old,
+	(unsigned long)mcount,
+#endif
+#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))
+		return;
+
+	for (i = 0; i < ARRAY_SIZE(plt->ldr); ++i)
+		plt->ldr[i] = PLT_ENT_LDR;
+	memcpy(plt->lit, fixed_plts, sizeof(fixed_plts));
+	pltsec->plt_count = ARRAY_SIZE(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;
 
-	struct plt_entries *plt = (struct plt_entries *)pltsec->plt->sh_addr;
-	int idx = 0;
+	/* 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;
+
+	if (!pltsec->plt_count)
+		prealloc_fixed(pltsec, plt);
+
+	idx = ARRAY_SIZE(fixed_plts);
+	while (idx)
+		if (plt->lit[--idx] == val)
+			return (u32)&plt->ldr[idx];
 
 	/*
 	 * Look for an existing entry pointing to 'val'. Given that the
@@ -182,8 +219,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;
 
@@ -238,6 +275,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;
@@ -245,6 +283,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.4.6

Re: [PATCH v4 2/2] ARM: ftrace: Add MODULE_PLTS support

[Ard Biesheuvel]

On 13 March 2018 at 13:53, Alexander Sverdlin
<alexander.sverdlin@nokia.com> wrote:
> 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      | 62 ++++++++++++++++++++++++++++++++++++-------
>  arch/arm/kernel/module-plts.c | 47 +++++++++++++++++++++++++++++---
>  4 files changed, 100 insertions(+), 13 deletions(-)
>
...
> diff --git a/arch/arm/kernel/module-plts.c b/arch/arm/kernel/module-plts.c
> index f272711..0951270 100644
> --- a/arch/arm/kernel/module-plts.c
> +++ b/arch/arm/kernel/module-plts.c
> @@ -7,6 +7,7 @@
>   */
>
>  #include <linux/elf.h>
> +#include <linux/ftrace.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/sort.h>
> @@ -22,18 +23,54 @@
>                                                     (PLT_ENT_STRIDE - 8))
>  #endif
>
> +static const u32 fixed_plts[] = {
> +#ifdef CONFIG_FUNCTION_TRACER
> +       FTRACE_ADDR,
> +       MCOUNT_ADDR,
> +#ifdef CONFIG_OLD_MCOUNT
> +       (unsigned long)ftrace_caller_old,
> +       (unsigned long)mcount,
> +#endif
> +#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))
> +               return;
> +
> +       for (i = 0; i < ARRAY_SIZE(plt->ldr); ++i)
> +               plt->ldr[i] = PLT_ENT_LDR;
> +       memcpy(plt->lit, fixed_plts, sizeof(fixed_plts));

This is slightly dodgy. You are assuming that sizeof(plt->lit) >=
sizeof(fixed_plts), which may change depending on configuration or
future changes.

Could you add a BUILD_BUG_ON() here to ensure that this remains the case?

> +       pltsec->plt_count = ARRAY_SIZE(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;
>
> -       struct plt_entries *plt = (struct plt_entries *)pltsec->plt->sh_addr;
> -       int idx = 0;
> +       /* 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;
> +

Where is plt_ent ever used?

> +       if (!pltsec->plt_count)
> +               prealloc_fixed(pltsec, plt);
> +

Please move the if () check into prealloc_fixed(), and only keep the loop below


> +       idx = ARRAY_SIZE(fixed_plts);
> +       while (idx)
> +               if (plt->lit[--idx] == val)
> +                       return (u32)&plt->ldr[idx];

Please use a normal for loop here and iterate upward starting at 0


>
>         /*
>          * Look for an existing entry pointing to 'val'. Given that the
> @@ -182,8 +219,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;
>
> @@ -238,6 +275,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;
> @@ -245,6 +283,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.4.6
>

Re: [PATCH v4 2/2] ARM: ftrace: Add MODULE_PLTS support

[Alexander Sverdlin]

Hello Ard,

On 13/03/18 17:12, Ard Biesheuvel wrote:
>>  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;
>>
>> -       struct plt_entries *plt = (struct plt_entries *)pltsec->plt->sh_addr;
                                                           ^^^^^^^^^^^ (*)

>> -       int idx = 0;
>> +       /* 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;
>> +
> Where is plt_ent ever used?

Above is exactly the place it's used.
I need to cache it because after the module load is finished the ELF header is freed,
pltsec->plt pointer (*) is not valid any more.
With the above modification it's possible to call the function during the whole life
time of the module.

>> +       if (!pltsec->plt_count)
>> +               prealloc_fixed(pltsec, plt);

I'll prepare v5 based on your other comments.

-- 
Best regards,
Alexander Sverdlin.

Re: [PATCH v4 2/2] ARM: ftrace: Add MODULE_PLTS support

[Ard Biesheuvel]

On 13 March 2018 at 17:13, Alexander Sverdlin
<alexander.sverdlin@nokia.com> wrote:
> Hello Ard,
>
> On 13/03/18 17:12, Ard Biesheuvel wrote:
>>>  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;
>>>
>>> -       struct plt_entries *plt = (struct plt_entries *)pltsec->plt->sh_addr;
>                                                            ^^^^^^^^^^^ (*)
>
>>> -       int idx = 0;
>>> +       /* 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;
>>> +
>> Where is plt_ent ever used?
>
> Above is exactly the place it's used.
> I need to cache it because after the module load is finished the ELF header is freed,
> pltsec->plt pointer (*) is not valid any more.
> With the above modification it's possible to call the function during the whole life
> time of the module.
>

Right, ok. That's a problem.

This means that you are relying on get_module_plt() being called at
least once at module load time, which is not guaranteed.

Instead, you should set this member (and perhaps the entire prealloc
routine) in a module_finalize() implementation.

>>> +       if (!pltsec->plt_count)
>>> +               prealloc_fixed(pltsec, plt);
>
> I'll prepare v5 based on your other comments.
>
> --
> Best regards,
> Alexander Sverdlin.

Re: [PATCH v4 2/2] ARM: ftrace: Add MODULE_PLTS support

[Alexander Sverdlin]

Hi Ard!

On 13/03/18 18:18, Ard Biesheuvel wrote:
>>>>  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;
>>>>
>>>> -       struct plt_entries *plt = (struct plt_entries *)pltsec->plt->sh_addr;
>>                                                            ^^^^^^^^^^^ (*)
>>
>>>> -       int idx = 0;
>>>> +       /* 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;
>>>> +
>>> Where is plt_ent ever used?
>> Above is exactly the place it's used.
>> I need to cache it because after the module load is finished the ELF header is freed,
>> pltsec->plt pointer (*) is not valid any more.
>> With the above modification it's possible to call the function during the whole life
>> time of the module.
>>
> Right, ok. That's a problem.
> 
> This means that you are relying on get_module_plt() being called at
> least once at module load time, which is not guaranteed.

This is indeed guaranteed. For FTRACE use case. If it's being called from FTRACE in
run time, this would mean there were long calls in this module section, which in
turn means, get_module_plt() was called at least once for this module and this
section.

This doesn't hold in general, though.

In any case, if you insist, I can try to rework the whole stuff implementing module_finalize().

-- 
Best regards,
Alexander Sverdlin.

Re: [PATCH v4 2/2] ARM: ftrace: Add MODULE_PLTS support

[Ard Biesheuvel]

On 13 March 2018 at 17:32, Alexander Sverdlin
<alexander.sverdlin@nokia.com> wrote:
> Hi Ard!
>
> On 13/03/18 18:18, Ard Biesheuvel wrote:
>>>>>  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;
>>>>>
>>>>> -       struct plt_entries *plt = (struct plt_entries *)pltsec->plt->sh_addr;
>>>                                                            ^^^^^^^^^^^ (*)
>>>
>>>>> -       int idx = 0;
>>>>> +       /* 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;
>>>>> +
>>>> Where is plt_ent ever used?
>>> Above is exactly the place it's used.
>>> I need to cache it because after the module load is finished the ELF header is freed,
>>> pltsec->plt pointer (*) is not valid any more.
>>> With the above modification it's possible to call the function during the whole life
>>> time of the module.
>>>
>> Right, ok. That's a problem.
>>
>> This means that you are relying on get_module_plt() being called at
>> least once at module load time, which is not guaranteed.
>
> This is indeed guaranteed. For FTRACE use case. If it's being called from FTRACE in
> run time, this would mean there were long calls in this module section, which in
> turn means, get_module_plt() was called at least once for this module and this
> section.
>
> This doesn't hold in general, though.
>
> In any case, if you insist, I can try to rework the whole stuff implementing module_finalize().
>

I think it would be much better to use the module_finalize() hook for
this, given that it is only called once already, and all the data you
need is still available.

Note that ARM already has such a function, so you'll need to add a
call there, i.e.,

if (IS_ENABLED(CONFIG_ARM_MODULE_PLTS))
    module_plt_alloc_fixed();

or something like that. The CONFIG_FTRACE dependency can be kept local
to module-plts.c

Re: [PATCH v4 2/2] ARM: ftrace: Add MODULE_PLTS support

[Alexander Sverdlin]

On 13/03/18 18:39, Ard Biesheuvel wrote:
> if (IS_ENABLED(CONFIG_ARM_MODULE_PLTS))
>     module_plt_alloc_fixed();

Do you consider this a legal C code if without module-plts.o the function would not exist at all?
That's too much relying on optimizer I think...

-- 
Best regards,
Alexander Sverdlin.

Re: [PATCH v4 2/2] ARM: ftrace: Add MODULE_PLTS support

[Ard Biesheuvel]

On 13 March 2018 at 17:49, Alexander Sverdlin
<alexander.sverdlin@nokia.com> wrote:
> On 13/03/18 18:39, Ard Biesheuvel wrote:
>> if (IS_ENABLED(CONFIG_ARM_MODULE_PLTS))
>>     module_plt_alloc_fixed();
>
> Do you consider this a legal C code if without module-plts.o the function would not exist at all?
> That's too much relying on optimizer I think...
>

Yes, we rely on that in many different places in the kernel.

Re: [PATCH v4 2/2] ARM: ftrace: Add MODULE_PLTS support

[Alexander Sverdlin]

Hi!

On 13/03/18 18:51, Ard Biesheuvel wrote:
>>> if (IS_ENABLED(CONFIG_ARM_MODULE_PLTS))
>>>     module_plt_alloc_fixed();
>> Do you consider this a legal C code if without module-plts.o the function would not exist at all?
>> That's too much relying on optimizer I think...
>>
> Yes, we rely on that in many different places in the kernel.

https://www.kernel.org/doc/Documentation/process/coding-style.rst:
"However, this approach still allows the C compiler to see the code
inside the block, and check it for correctness (syntax, types, symbol
references, etc).  Thus, you still have to use an #ifdef if the code inside the
block references symbols that will not exist if the condition is not met."
 
But we can of course ignore it.

-- 
Best regards,
Alexander Sverdlin.

Re: [PATCH v4 2/2] ARM: ftrace: Add MODULE_PLTS support

[Alexander Sverdlin]

Hi!

On 13/03/18 18:39, Ard Biesheuvel wrote:
>>>>>>  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;
>>>>>>
>>>>>> -       struct plt_entries *plt = (struct plt_entries *)pltsec->plt->sh_addr;
>>>>                                                            ^^^^^^^^^^^ (*)
>>>>
>>>>>> -       int idx = 0;
>>>>>> +       /* 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;
>>>>>> +
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
(*)

>>>>> Where is plt_ent ever used?
>>>> Above is exactly the place it's used.
>>>> I need to cache it because after the module load is finished the ELF header is freed,
>>>> pltsec->plt pointer (*) is not valid any more.
>>>> With the above modification it's possible to call the function during the whole life
>>>> time of the module.
>>>>
>>> Right, ok. That's a problem.
>>>
>>> This means that you are relying on get_module_plt() being called at
>>> least once at module load time, which is not guaranteed.
>> This is indeed guaranteed. For FTRACE use case. If it's being called from FTRACE in
>> run time, this would mean there were long calls in this module section, which in
>> turn means, get_module_plt() was called at least once for this module and this
>> section.
>>
>> This doesn't hold in general, though.
>>
>> In any case, if you insist, I can try to rework the whole stuff implementing module_finalize().
>>
> I think it would be much better to use the module_finalize() hook for
> this, given that it is only called once already, and all the data you
> need is still available.

No problem, but some kind of (*) block would still be required, because
get_module_plt() has to work after module_finalize() as well as *before* it.
So before module_finalize() we would have to dereference pltsec->plt->sh_addr conditionally.
 
> Note that ARM already has such a function, so you'll need to add a
> call there, i.e.,
> 
> if (IS_ENABLED(CONFIG_ARM_MODULE_PLTS))
>     module_plt_alloc_fixed();
> 
> or something like that. The CONFIG_FTRACE dependency can be kept local
> to module-plts.c
> 

-- 
Best regards,
Alexander Sverdlin.

Re: [PATCH v4 2/2] ARM: ftrace: Add MODULE_PLTS support

[Ard Biesheuvel]

On 13 March 2018 at 18:24, Alexander Sverdlin
<alexander.sverdlin@nokia.com> wrote:
> Hi!
>
> On 13/03/18 18:51, Ard Biesheuvel wrote:
>>>> if (IS_ENABLED(CONFIG_ARM_MODULE_PLTS))
>>>>     module_plt_alloc_fixed();
>>> Do you consider this a legal C code if without module-plts.o the function would not exist at all?
>>> That's too much relying on optimizer I think...
>>>
>> Yes, we rely on that in many different places in the kernel.
>
> https://www.kernel.org/doc/Documentation/process/coding-style.rst:
> "However, this approach still allows the C compiler to see the code
> inside the block, and check it for correctness (syntax, types, symbol
> references, etc).  Thus, you still have to use an #ifdef if the code inside the
> block references symbols that will not exist if the condition is not met."
>

"will not exist" is ambiguous here. It is rather common to declare
symbols, but only define them conditionally, and use IS_ENABLED() to
refer to them. As the documentation says, this gets rid of #ifdefs,
making the code always visible to the compiler which is a good thing.

Re: [PATCH v4 2/2] ARM: ftrace: Add MODULE_PLTS support

[Alexander Sverdlin]

Hello Ard,

On 13/03/18 18:32, Alexander Sverdlin wrote:
>>>>>  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;
>>>>>
>>>>> -       struct plt_entries *plt = (struct plt_entries *)pltsec->plt->sh_addr;
>>>                                                            ^^^^^^^^^^^ (*)
>>>
>>>>> -       int idx = 0;
>>>>> +       /* 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;
>>>>> +
>>>> Where is plt_ent ever used?
>>> Above is exactly the place it's used.
>>> I need to cache it because after the module load is finished the ELF header is freed,
>>> pltsec->plt pointer (*) is not valid any more.
>>> With the above modification it's possible to call the function during the whole life
>>> time of the module.
>>>
>> Right, ok. That's a problem.
>>
>> This means that you are relying on get_module_plt() being called at
>> least once at module load time, which is not guaranteed.
> This is indeed guaranteed. For FTRACE use case. If it's being called from FTRACE in
> run time, this would mean there were long calls in this module section, which in
> turn means, get_module_plt() was called at least once for this module and this
> section.
> 
> This doesn't hold in general, though.
> 
> In any case, if you insist, I can try to rework the whole stuff implementing module_finalize().

now when I have a new implementation via module_finalize(), I must admit it's not possible to
do it sanely this way.

module_finalize() can only add entries at the end of PLT, which means, they will be different
from the entries module loader/relocator has created before, which means, FTRACE will not
be able to replace these entries with NOPs.
As I don't want to do O(N) search on every dynamic ftrace operation, seems this is not an
option.
Either v4 has to be accepted, or I cannot propose a solution for upstream FTRACE+MODULES_PLT
combination.

-- 
Best regards,
Alexander Sverdlin.