Re: [RFC PATCH v3 04/11] powerpc/ftrace: Remove pointer to struct module from dyn_arch_ftrace
From: "Nicholas Piggin" <npiggin@gmail.com>
Date: 2024-07-01 09:28:04
Also in:
bpf, linuxppc-dev
On Fri Jun 21, 2024 at 4:54 AM AEST, Naveen N Rao wrote:
Pointer to struct module is only relevant for ftrace records belonging to kernel modules. Having this field in dyn_arch_ftrace wastes memory for all ftrace records belonging to the kernel. Remove the same in favour of looking up the module from the ftrace record address, similar to other architectures.
arm is the only one left that requires dyn_arch_ftrace after this.
Signed-off-by: Naveen N Rao <naveen@kernel.org> --- arch/powerpc/include/asm/ftrace.h | 1 - arch/powerpc/kernel/trace/ftrace.c | 54 +++++++++++------- arch/powerpc/kernel/trace/ftrace_64_pg.c | 73 +++++++++++------------- 3 files changed, 65 insertions(+), 63 deletions(-)
[snip]
quoted hunk ↗ jump to hunk
@@ -106,28 +106,48 @@ static unsigned long find_ftrace_tramp(unsigned long ip) return 0; } +#ifdef CONFIG_MODULES +static unsigned long ftrace_lookup_module_stub(unsigned long ip, unsigned long addr) +{ + struct module *mod = NULL; + + /* + * NOTE: __module_text_address() must be called with preemption + * disabled, but we can rely on ftrace_lock to ensure that 'mod' + * retains its validity throughout the remainder of this code. + */ + preempt_disable(); + mod = __module_text_address(ip); + preempt_enable();
If 'mod' was guaranteed to exist before your patch, then it should do afterward too. But is it always ftrace_lock that protects it, or do dyn_ftrace entries pin a module in some cases?
quoted hunk ↗ jump to hunk
@@ -555,7 +551,10 @@ __ftrace_modify_call(struct dyn_ftrace *rec, unsigned long old_addr, ppc_inst_t op; unsigned long ip = rec->ip; unsigned long entry, ptr, tramp; - struct module *mod = rec->arch.mod; + struct module *mod = ftrace_lookup_module(rec); + + if (!mod) + return -EINVAL; /* If we never set up ftrace trampolines, then bail */ if (!mod->arch.tramp || !mod->arch.tramp_regs) {@@ -668,14 +667,6 @@ int ftrace_modify_call(struct dyn_ftrace *rec, unsigned long old_addr, return -EINVAL; } - /* - * Out of range jumps are called from modules. - */ - if (!rec->arch.mod) { - pr_err("No module loaded\n"); - return -EINVAL; - } -
A couple of these conversions are not _exactly_ the same (lost the pr_err here), maybe that's deliberate because the messages don't look too useful. Looks okay though Reviewed-by: Nicholas Piggin <npiggin@gmail.com>