Hi,
This series is a reworked version of Torsten's FTRACE_WITH_REGS series
[1]. I've tried to rework the existing code in preparatory patches so
that the patchable-function-entry bits slot in with fewer surprises.
This version is based on v5.4-rc3, and can be found in my
arm64/ftrace-with-regs branch [2].
Patch 1 adds an (optional) ftrace_init_nop(), which the core code uses
to initialize callsites. This allows us to avoid a synthetic MCOUNT_ADDR
symbol, and more cleanly separates the one-time initialization of the
callsite from dynamic NOP<->CALL modification. Architectures which don't
implement this get the existing ftrace_make_nop() with MCOUNT_ADDR.
Recently parisc gained ftrace support using patchable-function-entry.
Patch 2 makes the handling of module callsite locations common in
kernel/module.c with a new FTRACE_CALLSITE_SECTION definition, and
removed the newly redundant bits from arch/parisc.
Patches 3 and 4 move the module PLT initialization to module load time,
which simplifies runtime callsite modification. This also means that we
don't transitently mark the module text RW, and will allow for the
removal of module_disable_ro().
Patches 5 and 6 add some trivial infrastructure, with patch 7 finally
adding FTRACE_WITH_REGS support. Additional work will be required for
livepatching (e.g. implementing reliable stack trace), which is
commented as part of patch 7.
Patch 8 is a trivial cleanup atop of the rest of the series, making the
code easier to read and less susceptible to config-specific breakage.
Since v1 [3]:
* Add a couple of people to Cc
* Fold in Ard's Reviewed-by tag
* Rename ftrace_code_init_disabled() to ftrace_nop_initialize()
* Move ftrace_init_nop() to <linux/ftrace.h>, with kerneldoc
* Update kerneldoc for rec parameters
Thanks,
Mark.
[1] https://lore.kernel.org/r/[email protected]
[2] git://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git arm64/ftrace-with-regs
[3] https://lore.kernel.org/r/[email protected]
Mark Rutland (7):
ftrace: add ftrace_init_nop()
module/ftrace: handle patchable-function-entry
arm64: module: rework special section handling
arm64: module/ftrace: intialize PLT at load time
arm64: insn: add encoder for MOV (register)
arm64: asm-offsets: add S_FP
arm64: ftrace: minimize ifdeffery
Torsten Duwe (1):
arm64: implement ftrace with regs
arch/arm64/Kconfig | 2 +
arch/arm64/Makefile | 5 ++
arch/arm64/include/asm/ftrace.h | 23 +++++++
arch/arm64/include/asm/insn.h | 3 +
arch/arm64/include/asm/module.h | 2 +-
arch/arm64/kernel/asm-offsets.c | 1 +
arch/arm64/kernel/entry-ftrace.S | 140 +++++++++++++++++++++++++++++++++++++--
arch/arm64/kernel/ftrace.c | 123 ++++++++++++++++++++--------------
arch/arm64/kernel/insn.c | 13 ++++
arch/arm64/kernel/module-plts.c | 3 +-
arch/arm64/kernel/module.c | 57 +++++++++++++---
arch/parisc/Makefile | 1 -
arch/parisc/kernel/module.c | 10 ++-
arch/parisc/kernel/module.lds | 7 --
include/linux/ftrace.h | 40 ++++++++++-
kernel/module.c | 2 +-
kernel/trace/ftrace.c | 6 +-
17 files changed, 355 insertions(+), 83 deletions(-)
delete mode 100644 arch/parisc/kernel/module.lds
--
2.11.0
When we load a module, we have to perform some special work for a couple
of named sections. To do this, we iterate over all of the module's
sections, and perform work for each section we recognize.
To make it easier to handle the unexpected absence of a section, and to
make the section-specific logic easer to read, let's factor the section
search into a helper. Similar is already done in the core module loader,
and other architectures (and ideally we'd unify these in future).
If we expect a module to have an ftrace trampoline section, but it
doesn't have one, we'll now reject loading the module. When
ARM64_MODULE_PLTS is selected, any correctly built module should have
one (and this is assumed by arm64's ftrace PLT code) and the absence of
such a section implies something has gone wrong at build time.
Subsequent patches will make use of the new helper.
Signed-off-by: Mark Rutland <[email protected]>
Reviewed-by: Ard Biesheuvel <[email protected]>
Cc: Catalin Marinas <[email protected]>
Cc: James Morse <[email protected]>
Cc: Will Deacon <[email protected]>
---
arch/arm64/kernel/module.c | 35 ++++++++++++++++++++++++++---------
1 file changed, 26 insertions(+), 9 deletions(-)
diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c
index 03ff15bffbb6..763a86d52fef 100644
--- a/arch/arm64/kernel/module.c
+++ b/arch/arm64/kernel/module.c
@@ -470,22 +470,39 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
return -ENOEXEC;
}
-int module_finalize(const Elf_Ehdr *hdr,
- const Elf_Shdr *sechdrs,
- struct module *me)
+static const Elf_Shdr *find_section(const Elf_Ehdr *hdr,
+ const Elf_Shdr *sechdrs,
+ const char *name)
{
const Elf_Shdr *s, *se;
const char *secstrs = (void *)hdr + sechdrs[hdr->e_shstrndx].sh_offset;
for (s = sechdrs, se = sechdrs + hdr->e_shnum; s < se; s++) {
- if (strcmp(".altinstructions", secstrs + s->sh_name) == 0)
- apply_alternatives_module((void *)s->sh_addr, s->sh_size);
+ if (strcmp(name, secstrs + s->sh_name) == 0)
+ return s;
+ }
+
+ return NULL;
+}
+
+int module_finalize(const Elf_Ehdr *hdr,
+ const Elf_Shdr *sechdrs,
+ struct module *me)
+{
+ const Elf_Shdr *s;
+
+ s = find_section(hdr, sechdrs, ".altinstructions");
+ if (s)
+ apply_alternatives_module((void *)s->sh_addr, s->sh_size);
+
#ifdef CONFIG_ARM64_MODULE_PLTS
- if (IS_ENABLED(CONFIG_DYNAMIC_FTRACE) &&
- !strcmp(".text.ftrace_trampoline", secstrs + s->sh_name))
- me->arch.ftrace_trampoline = (void *)s->sh_addr;
-#endif
+ if (IS_ENABLED(CONFIG_DYNAMIC_FTRACE)) {
+ s = find_section(hdr, sechdrs, ".text.ftrace_trampoline");
+ if (!s)
+ return -ENOEXEC;
+ me->arch.ftrace_trampoline = (void *)s->sh_addr;
}
+#endif
return 0;
}
--
2.11.0
So that assembly code can more easily manipulate the FP (x29) within a
pt_regs, add an S_FP asm-offsets definition.
Signed-off-by: Mark Rutland <[email protected]>
Reviewed-by: Ard Biesheuvel <[email protected]>
Cc: Catalin Marinas <[email protected]>
Cc: Will Deacon <[email protected]>
---
arch/arm64/kernel/asm-offsets.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c
index 214685760e1c..a5bdce8af65b 100644
--- a/arch/arm64/kernel/asm-offsets.c
+++ b/arch/arm64/kernel/asm-offsets.c
@@ -56,6 +56,7 @@ int main(void)
DEFINE(S_X24, offsetof(struct pt_regs, regs[24]));
DEFINE(S_X26, offsetof(struct pt_regs, regs[26]));
DEFINE(S_X28, offsetof(struct pt_regs, regs[28]));
+ DEFINE(S_FP, offsetof(struct pt_regs, regs[29]));
DEFINE(S_LR, offsetof(struct pt_regs, regs[30]));
DEFINE(S_SP, offsetof(struct pt_regs, sp));
DEFINE(S_PSTATE, offsetof(struct pt_regs, pstate));
--
2.11.0
Now that we no longer refer to mod->arch.ftrace_trampolines in the body
of ftrace_make_call(), we can use IS_ENABLED() rather than ifdeffery,
and make the code easier to follow. Likewise in ftrace_make_nop().
Let's do so.
Signed-off-by: Mark Rutland <[email protected]>
Reviewed-by: Ard Biesheuvel <[email protected]>
Cc: Catalin Marinas <[email protected]>
Cc: Will Deacon <[email protected]>
---
arch/arm64/kernel/ftrace.c | 18 ++++++++----------
1 file changed, 8 insertions(+), 10 deletions(-)
diff --git a/arch/arm64/kernel/ftrace.c b/arch/arm64/kernel/ftrace.c
index aea652c33a38..8618faa82e6d 100644
--- a/arch/arm64/kernel/ftrace.c
+++ b/arch/arm64/kernel/ftrace.c
@@ -62,18 +62,18 @@ int ftrace_update_ftrace_func(ftrace_func_t func)
return ftrace_modify_code(pc, 0, new, false);
}
-#ifdef CONFIG_ARM64_MODULE_PLTS
static struct plt_entry *get_ftrace_plt(struct module *mod, unsigned long addr)
{
+#ifdef CONFIG_ARM64_MODULE_PLTS
struct plt_entry *plt = mod->arch.ftrace_trampolines;
if (addr == FTRACE_ADDR)
return &plt[FTRACE_PLT_IDX];
if (addr == FTRACE_REGS_ADDR && IS_ENABLED(CONFIG_FTRACE_WITH_REGS))
return &plt[FTRACE_REGS_PLT_IDX];
+#endif
return NULL;
}
-#endif
/*
* Turn on the call to ftrace_caller() in instrumented function
@@ -85,10 +85,12 @@ int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
long offset = (long)pc - (long)addr;
if (offset < -SZ_128M || offset >= SZ_128M) {
-#ifdef CONFIG_ARM64_MODULE_PLTS
struct module *mod;
struct plt_entry *plt;
+ if (!IS_ENABLED(CONFIG_ARM64_MODULE_PLTS))
+ return -EINVAL;
+
/*
* On kernels that support module PLTs, the offset between the
* branch instruction and its target may legally exceed the
@@ -113,9 +115,6 @@ int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
}
addr = (unsigned long)plt;
-#else /* CONFIG_ARM64_MODULE_PLTS */
- return -EINVAL;
-#endif /* CONFIG_ARM64_MODULE_PLTS */
}
old = aarch64_insn_gen_nop();
@@ -185,9 +184,11 @@ int ftrace_make_nop(struct module *mod, struct dyn_ftrace *rec,
long offset = (long)pc - (long)addr;
if (offset < -SZ_128M || offset >= SZ_128M) {
-#ifdef CONFIG_ARM64_MODULE_PLTS
u32 replaced;
+ if (!IS_ENABLED(CONFIG_ARM64_MODULE_PLTS))
+ return -EINVAL;
+
/*
* 'mod' is only set at module load time, but if we end up
* dealing with an out-of-range condition, we can assume it
@@ -218,9 +219,6 @@ int ftrace_make_nop(struct module *mod, struct dyn_ftrace *rec,
return -EINVAL;
validate = false;
-#else /* CONFIG_ARM64_MODULE_PLTS */
- return -EINVAL;
-#endif /* CONFIG_ARM64_MODULE_PLTS */
} else {
old = aarch64_insn_gen_branch_imm(pc, addr,
AARCH64_INSN_BRANCH_LINK);
--
2.11.0
Architectures may need to perform special initialization of ftrace
callsites, and today they do so by special-casing ftrace_make_nop() when
the expected branch address is MCOUNT_ADDR. In some cases (e.g. for
patchable-function-entry), we don't have an mcount-like symbol and don't
want a synthetic MCOUNT_ADDR, but we may need to perform some
initialization of callsites.
To make it possible to separate initialization from runtime
modification, and to handle cases without an mcount-like symbol, this
patch adds an optional ftrace_init_nop() function that architectures can
implement, which does not pass a branch address.
Where an architecture does not provide ftrace_init_nop(), we will fall
back to the existing behaviour of calling ftrace_make_nop() with
MCOUNT_ADDR.
At the same time, ftrace_code_disable() is renamed to
ftrace_nop_initialize() to make it clearer that it is intended to
intialize a callsite into a disabled state, and is not for disabling a
callsite that has been runtime enabled. The kerneldoc description of rec
arguments is updated to cover non-mcount callsites.
Signed-off-by: Mark Rutland <[email protected]>
Reviewed-by: Ard Biesheuvel <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Steven Rostedt <[email protected]>
Cc: Torsten Duwe <[email protected]>
---
include/linux/ftrace.h | 35 ++++++++++++++++++++++++++++++++---
kernel/trace/ftrace.c | 6 +++---
2 files changed, 35 insertions(+), 6 deletions(-)
diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index 8a8cb3c401b2..9867d90d635e 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -499,7 +499,7 @@ static inline int ftrace_disable_ftrace_graph_caller(void) { return 0; }
/**
* ftrace_make_nop - convert code into nop
* @mod: module structure if called by module load initialization
- * @rec: the mcount call site record
+ * @rec: the call site record (e.g. mcount/fentry)
* @addr: the address that the call site should be calling
*
* This is a very sensitive operation and great care needs
@@ -520,9 +520,38 @@ static inline int ftrace_disable_ftrace_graph_caller(void) { return 0; }
extern int ftrace_make_nop(struct module *mod,
struct dyn_ftrace *rec, unsigned long addr);
+
+/**
+ * ftrace_init_nop - initialize a nop call site
+ * @mod: module structure if called by module load initialization
+ * @rec: the call site record (e.g. mcount/fentry)
+ *
+ * This is a very sensitive operation and great care needs
+ * to be taken by the arch. The operation should carefully
+ * read the location, check to see if what is read is indeed
+ * what we expect it to be, and then on success of the compare,
+ * it should write to the location.
+ *
+ * The code segment at @rec->ip should contain the contents created by
+ * the compiler
+ *
+ * Return must be:
+ * 0 on success
+ * -EFAULT on error reading the location
+ * -EINVAL on a failed compare of the contents
+ * -EPERM on error writing to the location
+ * Any other value will be considered a failure.
+ */
+#ifndef ftrace_init_nop
+static inline int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec)
+{
+ return ftrace_make_nop(mod, rec, MCOUNT_ADDR);
+}
+#endif
+
/**
* ftrace_make_call - convert a nop call site into a call to addr
- * @rec: the mcount call site record
+ * @rec: the call site record (e.g. mcount/fentry)
* @addr: the address that the call site should call
*
* This is a very sensitive operation and great care needs
@@ -545,7 +574,7 @@ extern int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr);
#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
/**
* ftrace_modify_call - convert from one addr to another (no nop)
- * @rec: the mcount call site record
+ * @rec: the call site record (e.g. mcount/fentry)
* @old_addr: the address expected to be currently called to
* @addr: the address to change to
*
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index f296d89be757..5259d4dea675 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -2494,14 +2494,14 @@ struct dyn_ftrace *ftrace_rec_iter_record(struct ftrace_rec_iter *iter)
}
static int
-ftrace_code_disable(struct module *mod, struct dyn_ftrace *rec)
+ftrace_nop_initialize(struct module *mod, struct dyn_ftrace *rec)
{
int ret;
if (unlikely(ftrace_disabled))
return 0;
- ret = ftrace_make_nop(mod, rec, MCOUNT_ADDR);
+ ret = ftrace_init_nop(mod, rec);
if (ret) {
ftrace_bug_type = FTRACE_BUG_INIT;
ftrace_bug(ret, rec);
@@ -2943,7 +2943,7 @@ static int ftrace_update_code(struct module *mod, struct ftrace_page *new_pgs)
* to the NOP instructions.
*/
if (!__is_defined(CC_USING_NOP_MCOUNT) &&
- !ftrace_code_disable(mod, p))
+ !ftrace_nop_initialize(mod, p))
break;
update_cnt++;
--
2.11.0
Currently we lazily-initialize a module's ftrace PLT at runtime when we
install the first ftrace call. To do so we have to apply a number of
sanity checks, transiently mark the module text as RW, and perform an
IPI as part of handling Neoverse-N1 erratum #1542419.
We only expect the ftrace trampoline to point at ftrace_caller() (AKA
FTRACE_ADDR), so let's simplify all of this by intializing the PLT at
module load time, before the module loader marks the module RO and
performs the intial I-cache maintenance for the module.
Thus we can rely on the module having been correctly intialized, and can
simplify the runtime work necessary to install an ftrace call in a
module. This will also allow for the removal of module_disable_ro().
Tested by forcing ftrace_make_call() to use the module PLT, and then
loading up a module after setting up ftrace with:
| echo ":mod:<module-name>" > set_ftrace_filter;
| echo function > current_tracer;
| modprobe <module-name>
Since FTRACE_ADDR is only defined when CONFIG_DYNAMIC_FTRACE is
selected, we wrap its use along with most of module_init_ftrace_plt()
with ifdeffery rather than using IS_ENABLED().
Signed-off-by: Mark Rutland <[email protected]>
Reviewed-by: Ard Biesheuvel <[email protected]>
Cc: Catalin Marinas <[email protected]>
Cc: James Morse <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Will Deacon <[email protected]>
---
arch/arm64/kernel/ftrace.c | 55 ++++++++++++----------------------------------
arch/arm64/kernel/module.c | 32 +++++++++++++++++----------
2 files changed, 35 insertions(+), 52 deletions(-)
diff --git a/arch/arm64/kernel/ftrace.c b/arch/arm64/kernel/ftrace.c
index 06e56b470315..822718eafdb4 100644
--- a/arch/arm64/kernel/ftrace.c
+++ b/arch/arm64/kernel/ftrace.c
@@ -73,10 +73,22 @@ int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
if (offset < -SZ_128M || offset >= SZ_128M) {
#ifdef CONFIG_ARM64_MODULE_PLTS
- struct plt_entry trampoline, *dst;
struct module *mod;
/*
+ * There is only one ftrace trampoline per module. For now,
+ * this is not a problem since on arm64, all dynamic ftrace
+ * invocations are routed via ftrace_caller(). This will need
+ * to be revisited if support for multiple ftrace entry points
+ * is added in the future, but for now, the pr_err() below
+ * deals with a theoretical issue only.
+ */
+ if (addr != FTRACE_ADDR) {
+ pr_err("ftrace: far branches to multiple entry points unsupported inside a single module\n");
+ return -EINVAL;
+ }
+
+ /*
* On kernels that support module PLTs, the offset between the
* branch instruction and its target may legally exceed the
* range of an ordinary relative 'bl' opcode. In this case, we
@@ -93,46 +105,7 @@ int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
if (WARN_ON(!mod))
return -EINVAL;
- /*
- * There is only one ftrace trampoline per module. For now,
- * this is not a problem since on arm64, all dynamic ftrace
- * invocations are routed via ftrace_caller(). This will need
- * to be revisited if support for multiple ftrace entry points
- * is added in the future, but for now, the pr_err() below
- * deals with a theoretical issue only.
- *
- * Note that PLTs are place relative, and plt_entries_equal()
- * checks whether they point to the same target. Here, we need
- * to check if the actual opcodes are in fact identical,
- * regardless of the offset in memory so use memcmp() instead.
- */
- dst = mod->arch.ftrace_trampoline;
- trampoline = get_plt_entry(addr, dst);
- if (memcmp(dst, &trampoline, sizeof(trampoline))) {
- if (plt_entry_is_initialized(dst)) {
- pr_err("ftrace: far branches to multiple entry points unsupported inside a single module\n");
- return -EINVAL;
- }
-
- /* point the trampoline to our ftrace entry point */
- module_disable_ro(mod);
- *dst = trampoline;
- module_enable_ro(mod, true);
-
- /*
- * Ensure updated trampoline is visible to instruction
- * fetch before we patch in the branch. Although the
- * architecture doesn't require an IPI in this case,
- * Neoverse-N1 erratum #1542419 does require one
- * if the TLB maintenance in module_enable_ro() is
- * skipped due to rodata_enabled. It doesn't seem worth
- * it to make it conditional given that this is
- * certainly not a fast-path.
- */
- flush_icache_range((unsigned long)&dst[0],
- (unsigned long)&dst[1]);
- }
- addr = (unsigned long)dst;
+ addr = (unsigned long)mod->arch.ftrace_trampoline;
#else /* CONFIG_ARM64_MODULE_PLTS */
return -EINVAL;
#endif /* CONFIG_ARM64_MODULE_PLTS */
diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c
index 763a86d52fef..5f5bc3b94da7 100644
--- a/arch/arm64/kernel/module.c
+++ b/arch/arm64/kernel/module.c
@@ -9,6 +9,7 @@
#include <linux/bitops.h>
#include <linux/elf.h>
+#include <linux/ftrace.h>
#include <linux/gfp.h>
#include <linux/kasan.h>
#include <linux/kernel.h>
@@ -485,24 +486,33 @@ static const Elf_Shdr *find_section(const Elf_Ehdr *hdr,
return NULL;
}
+int module_init_ftrace_plt(const Elf_Ehdr *hdr,
+ const Elf_Shdr *sechdrs,
+ struct module *mod)
+{
+#if defined(CONFIG_ARM64_MODULE_PLTS) && defined(CONFIG_DYNAMIC_FTRACE)
+ const Elf_Shdr *s;
+ struct plt_entry *plt;
+
+ s = find_section(hdr, sechdrs, ".text.ftrace_trampoline");
+ if (!s)
+ return -ENOEXEC;
+
+ plt = (void *)s->sh_addr;
+ *plt = get_plt_entry(FTRACE_ADDR, plt);
+ mod->arch.ftrace_trampoline = plt;
+#endif
+ return 0;
+}
+
int module_finalize(const Elf_Ehdr *hdr,
const Elf_Shdr *sechdrs,
struct module *me)
{
const Elf_Shdr *s;
-
s = find_section(hdr, sechdrs, ".altinstructions");
if (s)
apply_alternatives_module((void *)s->sh_addr, s->sh_size);
-#ifdef CONFIG_ARM64_MODULE_PLTS
- if (IS_ENABLED(CONFIG_DYNAMIC_FTRACE)) {
- s = find_section(hdr, sechdrs, ".text.ftrace_trampoline");
- if (!s)
- return -ENOEXEC;
- me->arch.ftrace_trampoline = (void *)s->sh_addr;
- }
-#endif
-
- return 0;
+ return module_init_ftrace_plt(hdr, sechdrs, me);
}
--
2.11.0
From: Torsten Duwe <[email protected]>
This patch implements FTRACE_WITH_REGS for arm64, which allows a traced
function's arguments (and some other registers) to be captured into a
struct pt_regs, allowing these to be inspected and/or modified. This is
a building block for live-patching, where a function's arguments may be
forwarded to another function. This is also necessary to enable ftrace
and in-kernel pointer authentication at the same time, as it allows the
LR value to be captured and adjusted prior to signing.
Using GCC's -fpatchable-function-entry=N option, we can have the
compiler insert a configurable number of NOPs between the function entry
point and the usual prologue. This also ensures functions are AAPCS
compliant (e.g. disabling inter-procedural register allocation).
For example, with -fpatchable-function-entry=2, GCC 8.1.0 compiles the
following:
| unsigned long bar(void);
|
| unsigned long foo(void)
| {
| return bar() + 1;
| }
... to:
| <foo>:
| nop
| nop
| stp x29, x30, [sp, #-16]!
| mov x29, sp
| bl 0 <bar>
| add x0, x0, #0x1
| ldp x29, x30, [sp], #16
| ret
This patch builds the kernel with -fpatchable-function-entry=2,
prefixing each function with two NOPs. To trace a function, we replace
these NOPs with a sequence that saves the LR into a GPR, then calls an
ftrace entry assembly function which saves this and other relevant
registers:
| mov x9, x30
| bl <ftrace-entry>
Since patchable functions are AAPCS compliant (and the kernel does not
use x18 as a platform register), x9-x18 can be safely clobbered in the
patched sequence and the ftrace entry code.
There are now two ftrace entry functions, ftrace_regs_entry (which saves
all GPRs), and ftrace_entry (which saves the bare minimum). A PLT is
allocated for each within modules.
Signed-off-by: Torsten Duwe <[email protected]>
[Mark: rework asm, comments, PLTs, initialization, commit message]
Signed-off-by: Mark Rutland <[email protected]>
Reviewed-by: Ard Biesheuvel <[email protected]>
Cc: AKASHI Takahiro <[email protected]>
Cc: Amit Daniel Kachhap <[email protected]>
Cc: Catalin Marinas <[email protected]>
Cc: Josh Poimboeuf <[email protected]>
Cc: Julien Thierry <[email protected]>
Cc: Will Deacon <[email protected]>
---
arch/arm64/Kconfig | 2 +
arch/arm64/Makefile | 5 ++
arch/arm64/include/asm/ftrace.h | 23 +++++++
arch/arm64/include/asm/module.h | 2 +-
arch/arm64/kernel/entry-ftrace.S | 140 +++++++++++++++++++++++++++++++++++++--
arch/arm64/kernel/ftrace.c | 84 +++++++++++++++++++----
arch/arm64/kernel/module-plts.c | 3 +-
arch/arm64/kernel/module.c | 18 +++--
8 files changed, 252 insertions(+), 25 deletions(-)
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 950a56b71ff0..0ffb8596b8a1 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -143,6 +143,8 @@ config ARM64
select HAVE_DEBUG_KMEMLEAK
select HAVE_DMA_CONTIGUOUS
select HAVE_DYNAMIC_FTRACE
+ select HAVE_DYNAMIC_FTRACE_WITH_REGS \
+ if $(cc-option,-fpatchable-function-entry=2)
select HAVE_EFFICIENT_UNALIGNED_ACCESS
select HAVE_FAST_GUP
select HAVE_FTRACE_MCOUNT_RECORD
diff --git a/arch/arm64/Makefile b/arch/arm64/Makefile
index 2c0238ce0551..1fbe24d4fdb6 100644
--- a/arch/arm64/Makefile
+++ b/arch/arm64/Makefile
@@ -95,6 +95,11 @@ ifeq ($(CONFIG_ARM64_MODULE_PLTS),y)
KBUILD_LDS_MODULE += $(srctree)/arch/arm64/kernel/module.lds
endif
+ifeq ($(CONFIG_DYNAMIC_FTRACE_WITH_REGS),y)
+ KBUILD_CPPFLAGS += -DCC_USING_PATCHABLE_FUNCTION_ENTRY
+ CC_FLAGS_FTRACE := -fpatchable-function-entry=2
+endif
+
# Default value
head-y := arch/arm64/kernel/head.o
diff --git a/arch/arm64/include/asm/ftrace.h b/arch/arm64/include/asm/ftrace.h
index d48667b04c41..91fa4baa1a93 100644
--- a/arch/arm64/include/asm/ftrace.h
+++ b/arch/arm64/include/asm/ftrace.h
@@ -11,9 +11,20 @@
#include <asm/insn.h>
#define HAVE_FUNCTION_GRAPH_FP_TEST
+
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
+#define ARCH_SUPPORTS_FTRACE_OPS 1
+#else
#define MCOUNT_ADDR ((unsigned long)_mcount)
+#endif
+
+/* The BL at the callsite's adjusted rec->ip */
#define MCOUNT_INSN_SIZE AARCH64_INSN_SIZE
+#define FTRACE_PLT_IDX 0
+#define FTRACE_REGS_PLT_IDX 1
+#define NR_FTRACE_PLTS 2
+
/*
* Currently, gcc tends to save the link register after the local variables
* on the stack. This causes the max stack tracer to report the function
@@ -44,12 +55,24 @@ extern void return_to_handler(void);
static inline unsigned long ftrace_call_adjust(unsigned long addr)
{
/*
+ * Adjust addr to point at the BL in the callsite.
+ * See ftrace_init_nop() for the callsite sequence.
+ */
+ if (IS_ENABLED(CONFIG_DYNAMIC_FTRACE_WITH_REGS))
+ return addr + AARCH64_INSN_SIZE;
+ /*
* addr is the address of the mcount call instruction.
* recordmcount does the necessary offset calculation.
*/
return addr;
}
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
+struct dyn_ftrace;
+int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec);
+#define ftrace_init_nop ftrace_init_nop
+#endif
+
#define ftrace_return_address(n) return_address(n)
/*
diff --git a/arch/arm64/include/asm/module.h b/arch/arm64/include/asm/module.h
index f80e13cbf8ec..1e93de68c044 100644
--- a/arch/arm64/include/asm/module.h
+++ b/arch/arm64/include/asm/module.h
@@ -21,7 +21,7 @@ struct mod_arch_specific {
struct mod_plt_sec init;
/* for CONFIG_DYNAMIC_FTRACE */
- struct plt_entry *ftrace_trampoline;
+ struct plt_entry *ftrace_trampolines;
};
#endif
diff --git a/arch/arm64/kernel/entry-ftrace.S b/arch/arm64/kernel/entry-ftrace.S
index 33d003d80121..94720388957f 100644
--- a/arch/arm64/kernel/entry-ftrace.S
+++ b/arch/arm64/kernel/entry-ftrace.S
@@ -7,10 +7,137 @@
*/
#include <linux/linkage.h>
+#include <asm/asm-offsets.h>
#include <asm/assembler.h>
#include <asm/ftrace.h>
#include <asm/insn.h>
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
+/*
+ * Due to -fpatchable-function-entry=2, the compiler has placed two NOPs before
+ * the regular function prologue. For an enabled callsite, ftrace_init_nop() and
+ * ftrace_make_call() have patched those NOPs to:
+ *
+ * MOV X9, LR
+ * BL <entry>
+ *
+ * ... where <entry> is either ftrace_caller or ftrace_regs_caller.
+ *
+ * Each instrumented function follows the AAPCS, so here x0-x8 and x19-x30 are
+ * live, and x9-x18 are safe to clobber.
+ *
+ * We save the callsite's context into a pt_regs before invoking and ftrace
+ * callbacks. So that we can get a sensible backtrace, we create a stack record
+ * for the callsite and the ftrace entry assembly. This is not sufficient for
+ * reliable stacktrace: until we create the callsite stack record, its caller
+ * is missing from the LR and existing chain of frame records.
+ */
+ .macro ftrace_regs_entry, allregs=0
+ /* Make room for pt_regs, plus a callee frame */
+ sub sp, sp, #(S_FRAME_SIZE + 16)
+
+ /* Save function arguments (and x9 for simplicity) */
+ stp x0, x1, [sp, #S_X0]
+ stp x2, x3, [sp, #S_X2]
+ stp x4, x5, [sp, #S_X4]
+ stp x6, x7, [sp, #S_X6]
+ stp x8, x9, [sp, #S_X8]
+
+ /* Optionally save the callee-saved registers, always save the FP */
+ .if \allregs == 1
+ stp x10, x11, [sp, #S_X10]
+ stp x12, x13, [sp, #S_X12]
+ stp x14, x15, [sp, #S_X14]
+ stp x16, x17, [sp, #S_X16]
+ stp x18, x19, [sp, #S_X18]
+ stp x20, x21, [sp, #S_X20]
+ stp x22, x23, [sp, #S_X22]
+ stp x24, x25, [sp, #S_X24]
+ stp x26, x27, [sp, #S_X26]
+ stp x28, x29, [sp, #S_X28]
+ .else
+ str x29, [sp, #S_FP]
+ .endif
+
+ /* Save the callsite's SP and LR */
+ add x10, sp, #(S_FRAME_SIZE + 16)
+ stp x9, x10, [sp, #S_LR]
+
+ /* Save the PC after the ftrace callsite */
+ str x30, [sp, #S_PC]
+
+ /* Create a frame record for the callsite above pt_regs */
+ stp x29, x9, [sp, #S_FRAME_SIZE]
+ add x29, sp, #S_FRAME_SIZE
+
+ /* Create our frame record within pt_regs. */
+ stp x29, x30, [sp, #S_STACKFRAME]
+ add x29, sp, #S_STACKFRAME
+ .endm
+
+ENTRY(ftrace_regs_caller)
+ ftrace_regs_entry 1
+ b ftrace_common
+ENDPROC(ftrace_regs_caller)
+
+ENTRY(ftrace_caller)
+ ftrace_regs_entry 0
+ b ftrace_common
+ENDPROC(ftrace_caller)
+
+ENTRY(ftrace_common)
+ sub x0, x30, #AARCH64_INSN_SIZE // ip (callsite's BL insn)
+ mov x1, x9 // parent_ip (callsite's LR)
+ ldr_l x2, function_trace_op // op
+ mov x3, sp // regs
+
+GLOBAL(ftrace_call)
+ bl ftrace_stub
+
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+GLOBAL(ftrace_graph_call) // ftrace_graph_caller();
+ nop // If enabled, this will be replaced
+ // "b ftrace_graph_caller"
+#endif
+
+/*
+ * At the callsite x0-x8 and x19-x30 were live. Any C code will have preserved
+ * x19-x29 per the AAPCS, and we created frame records upon entry, so we need
+ * to restore x0-x8, x29, and x30.
+ */
+ftrace_common_return:
+ /* Restore function arguments */
+ ldp x0, x1, [sp]
+ ldp x2, x3, [sp, #S_X2]
+ ldp x4, x5, [sp, #S_X4]
+ ldp x6, x7, [sp, #S_X6]
+ ldr x8, [sp, #S_X8]
+
+ /* Restore the callsite's FP, LR, PC */
+ ldr x29, [sp, #S_FP]
+ ldr x30, [sp, #S_LR]
+ ldr x9, [sp, #S_PC]
+
+ /* Restore the callsite's SP */
+ add sp, sp, #S_FRAME_SIZE + 16
+
+ ret x9
+ENDPROC(ftrace_common)
+
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+ENTRY(ftrace_graph_caller)
+ ldr x0, [sp, #S_PC]
+ sub x0, x0, #AARCH64_INSN_SIZE // ip (callsite's BL insn)
+ add x1, sp, #S_LR // parent_ip (callsite's LR)
+ ldr x2, [sp, #S_FRAME_SIZE] // parent fp (callsite's FP)
+ bl prepare_ftrace_return
+ b ftrace_common_return
+ENDPROC(ftrace_graph_caller)
+#else
+#endif
+
+#else /* CONFIG_DYNAMIC_FTRACE_WITH_REGS */
+
/*
* Gcc with -pg will put the following code in the beginning of each function:
* mov x0, x30
@@ -160,11 +287,6 @@ GLOBAL(ftrace_graph_call) // ftrace_graph_caller();
mcount_exit
ENDPROC(ftrace_caller)
-#endif /* CONFIG_DYNAMIC_FTRACE */
-
-ENTRY(ftrace_stub)
- ret
-ENDPROC(ftrace_stub)
#ifdef CONFIG_FUNCTION_GRAPH_TRACER
/*
@@ -184,7 +306,15 @@ ENTRY(ftrace_graph_caller)
mcount_exit
ENDPROC(ftrace_graph_caller)
+#endif /* CONFIG_FUNCTION_GRAPH_TRACER */
+#endif /* CONFIG_DYNAMIC_FTRACE */
+#endif /* CONFIG_DYNAMIC_FTRACE_WITH_REGS */
+
+ENTRY(ftrace_stub)
+ ret
+ENDPROC(ftrace_stub)
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
/*
* void return_to_handler(void)
*
diff --git a/arch/arm64/kernel/ftrace.c b/arch/arm64/kernel/ftrace.c
index 822718eafdb4..aea652c33a38 100644
--- a/arch/arm64/kernel/ftrace.c
+++ b/arch/arm64/kernel/ftrace.c
@@ -62,6 +62,19 @@ int ftrace_update_ftrace_func(ftrace_func_t func)
return ftrace_modify_code(pc, 0, new, false);
}
+#ifdef CONFIG_ARM64_MODULE_PLTS
+static struct plt_entry *get_ftrace_plt(struct module *mod, unsigned long addr)
+{
+ struct plt_entry *plt = mod->arch.ftrace_trampolines;
+
+ if (addr == FTRACE_ADDR)
+ return &plt[FTRACE_PLT_IDX];
+ if (addr == FTRACE_REGS_ADDR && IS_ENABLED(CONFIG_FTRACE_WITH_REGS))
+ return &plt[FTRACE_REGS_PLT_IDX];
+ return NULL;
+}
+#endif
+
/*
* Turn on the call to ftrace_caller() in instrumented function
*/
@@ -74,19 +87,7 @@ int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
if (offset < -SZ_128M || offset >= SZ_128M) {
#ifdef CONFIG_ARM64_MODULE_PLTS
struct module *mod;
-
- /*
- * There is only one ftrace trampoline per module. For now,
- * this is not a problem since on arm64, all dynamic ftrace
- * invocations are routed via ftrace_caller(). This will need
- * to be revisited if support for multiple ftrace entry points
- * is added in the future, but for now, the pr_err() below
- * deals with a theoretical issue only.
- */
- if (addr != FTRACE_ADDR) {
- pr_err("ftrace: far branches to multiple entry points unsupported inside a single module\n");
- return -EINVAL;
- }
+ struct plt_entry *plt;
/*
* On kernels that support module PLTs, the offset between the
@@ -105,7 +106,13 @@ int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
if (WARN_ON(!mod))
return -EINVAL;
- addr = (unsigned long)mod->arch.ftrace_trampoline;
+ plt = get_ftrace_plt(mod, addr);
+ if (!plt) {
+ pr_err("ftrace: no module PLT for %ps\n", (void *)addr);
+ return -EINVAL;
+ }
+
+ addr = (unsigned long)plt;
#else /* CONFIG_ARM64_MODULE_PLTS */
return -EINVAL;
#endif /* CONFIG_ARM64_MODULE_PLTS */
@@ -117,6 +124,55 @@ int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
return ftrace_modify_code(pc, old, new, true);
}
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
+int ftrace_modify_call(struct dyn_ftrace *rec, unsigned long old_addr,
+ unsigned long addr)
+{
+ unsigned long pc = rec->ip;
+ u32 old, new;
+
+ old = aarch64_insn_gen_branch_imm(pc, old_addr,
+ AARCH64_INSN_BRANCH_LINK);
+ new = aarch64_insn_gen_branch_imm(pc, addr, AARCH64_INSN_BRANCH_LINK);
+
+ return ftrace_modify_code(pc, old, new, true);
+}
+
+/*
+ * The compiler has inserted two NOPs before the regular function prologue.
+ * All instrumented functions follow the AAPCS, so x0-x8 and x19-x30 are live,
+ * and x9-x18 are free for our use.
+ *
+ * At runtime we want to be able to swing a single NOP <-> BL to enable or
+ * disable the ftrace call. The BL requires us to save the original LR value,
+ * so here we insert a <MOV X9, LR> over the first NOP so the instructions
+ * before the regular prologue are:
+ *
+ * | Compiled | Disabled | Enabled |
+ * +----------+------------+------------+
+ * | NOP | MOV X9, LR | MOV X9, LR |
+ * | NOP | NOP | BL <entry> |
+ *
+ * The LR value will be recovered by ftrace_regs_entry, and restored into LR
+ * before returning to the regular function prologue. When a function is not
+ * being traced, the MOV is not harmful given x9 is not live per the AAPCS.
+ *
+ * Note: ftrace_process_locs() has pre-adjusted rec->ip to be the address of
+ * the BL.
+ */
+int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec)
+{
+ unsigned long pc = rec->ip - AARCH64_INSN_SIZE;
+ u32 old, new;
+
+ old = aarch64_insn_gen_nop();
+ new = aarch64_insn_gen_move_reg(AARCH64_INSN_REG_9,
+ AARCH64_INSN_REG_LR,
+ AARCH64_INSN_VARIANT_64BIT);
+ return ftrace_modify_code(pc, old, new, true);
+}
+#endif
+
/*
* Turn off the call to ftrace_caller() in instrumented function
*/
diff --git a/arch/arm64/kernel/module-plts.c b/arch/arm64/kernel/module-plts.c
index b182442b87a3..65b08a74aec6 100644
--- a/arch/arm64/kernel/module-plts.c
+++ b/arch/arm64/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>
@@ -330,7 +331,7 @@ int module_frob_arch_sections(Elf_Ehdr *ehdr, Elf_Shdr *sechdrs,
tramp->sh_type = SHT_NOBITS;
tramp->sh_flags = SHF_EXECINSTR | SHF_ALLOC;
tramp->sh_addralign = __alignof__(struct plt_entry);
- tramp->sh_size = sizeof(struct plt_entry);
+ tramp->sh_size = NR_FTRACE_PLTS * sizeof(struct plt_entry);
}
return 0;
diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c
index 5f5bc3b94da7..00d21a420c60 100644
--- a/arch/arm64/kernel/module.c
+++ b/arch/arm64/kernel/module.c
@@ -486,21 +486,31 @@ static const Elf_Shdr *find_section(const Elf_Ehdr *hdr,
return NULL;
}
+static inline void __init_plt(struct plt_entry *plt, unsigned long addr)
+{
+ *plt = get_plt_entry(addr, plt);
+}
+
int module_init_ftrace_plt(const Elf_Ehdr *hdr,
const Elf_Shdr *sechdrs,
struct module *mod)
{
#if defined(CONFIG_ARM64_MODULE_PLTS) && defined(CONFIG_DYNAMIC_FTRACE)
const Elf_Shdr *s;
- struct plt_entry *plt;
+ struct plt_entry *plts;
s = find_section(hdr, sechdrs, ".text.ftrace_trampoline");
if (!s)
return -ENOEXEC;
- plt = (void *)s->sh_addr;
- *plt = get_plt_entry(FTRACE_ADDR, plt);
- mod->arch.ftrace_trampoline = plt;
+ plts = (void *)s->sh_addr;
+
+ __init_plt(&plts[FTRACE_PLT_IDX], FTRACE_ADDR);
+
+ if (IS_ENABLED(CONFIG_DYNAMIC_FTRACE_WITH_REGS))
+ __init_plt(&plts[FTRACE_REGS_PLT_IDX], FTRACE_REGS_ADDR);
+
+ mod->arch.ftrace_trampolines = plts;
#endif
return 0;
}
--
2.11.0
For FTRACE_WITH_REGS, we're going to want to generate a MOV (register)
instruction as part of the callsite intialization. As MOV (register) is
an alias for ORR (shifted register), we can generate this with
aarch64_insn_gen_logical_shifted_reg(), but it's somewhat verbose and
difficult to read in-context.
Add a aarch64_insn_gen_move_reg() wrapper for this case so that we can
write callers in a more straightforward way.
Signed-off-by: Mark Rutland <[email protected]>
Reviewed-by: Ard Biesheuvel <[email protected]>
Cc: Catalin Marinas <[email protected]>
Cc: Will Deacon <[email protected]>
---
arch/arm64/include/asm/insn.h | 3 +++
arch/arm64/kernel/insn.c | 13 +++++++++++++
2 files changed, 16 insertions(+)
diff --git a/arch/arm64/include/asm/insn.h b/arch/arm64/include/asm/insn.h
index 39e7780bedd6..bb313dde58a4 100644
--- a/arch/arm64/include/asm/insn.h
+++ b/arch/arm64/include/asm/insn.h
@@ -440,6 +440,9 @@ u32 aarch64_insn_gen_logical_shifted_reg(enum aarch64_insn_register dst,
int shift,
enum aarch64_insn_variant variant,
enum aarch64_insn_logic_type type);
+u32 aarch64_insn_gen_move_reg(enum aarch64_insn_register dst,
+ enum aarch64_insn_register src,
+ enum aarch64_insn_variant variant);
u32 aarch64_insn_gen_logical_immediate(enum aarch64_insn_logic_type type,
enum aarch64_insn_variant variant,
enum aarch64_insn_register Rn,
diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c
index d801a7094076..513b29c3e735 100644
--- a/arch/arm64/kernel/insn.c
+++ b/arch/arm64/kernel/insn.c
@@ -1268,6 +1268,19 @@ u32 aarch64_insn_gen_logical_shifted_reg(enum aarch64_insn_register dst,
return aarch64_insn_encode_immediate(AARCH64_INSN_IMM_6, insn, shift);
}
+/*
+ * MOV (register) is architecturally an alias of ORR (shifted register) where
+ * MOV <*d>, <*m> is equivalent to ORR <*d>, <*ZR>, <*m>
+ */
+u32 aarch64_insn_gen_move_reg(enum aarch64_insn_register dst,
+ enum aarch64_insn_register src,
+ enum aarch64_insn_variant variant)
+{
+ return aarch64_insn_gen_logical_shifted_reg(dst, AARCH64_INSN_REG_ZR,
+ src, 0, variant,
+ AARCH64_INSN_LOGIC_ORR);
+}
+
u32 aarch64_insn_gen_adr(unsigned long pc, unsigned long addr,
enum aarch64_insn_register reg,
enum aarch64_insn_adr_type type)
--
2.11.0
When using patchable-function-entry, the compiler will record the
callsites into a section named "__patchable_function_entries" rather
than "__mcount_loc". Let's abstract this difference behind a new
FTRACE_CALLSITE_SECTION, so that architectures don't have to handle this
explicitly (e.g. with custom module linker scripts).
As parisc currently handles this explicitly, it is fixed up accordingly,
with its custom linker script removed. Since FTRACE_CALLSITE_SECTION is
only defined when DYNAMIC_FTRACE is selected, the parisc module loading
code is updated to only use the definition in that case. When
DYNAMIC_FTRACE is not selected, modules shouldn't have this section, so
this removes some redundant work in that case.
I built parisc generic-{32,64}bit_defconfig with DYNAMIC_FTRACE enabled,
and verified that the section made it into the .ko files for modules.
Signed-off-by: Mark Rutland <[email protected]>
Reviewed-by: Ard Biesheuvel <[email protected]>
Cc: Helge Deller <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: James E.J. Bottomley <[email protected]>
Cc: Jessica Yu <[email protected]>
Cc: Steven Rostedt <[email protected]>
Cc: Sven Schnelle <[email protected]>
Cc: [email protected]
---
arch/parisc/Makefile | 1 -
arch/parisc/kernel/module.c | 10 +++++++---
arch/parisc/kernel/module.lds | 7 -------
include/linux/ftrace.h | 5 +++++
kernel/module.c | 2 +-
5 files changed, 13 insertions(+), 12 deletions(-)
delete mode 100644 arch/parisc/kernel/module.lds
diff --git a/arch/parisc/Makefile b/arch/parisc/Makefile
index 36b834f1c933..dca8f2de8cf5 100644
--- a/arch/parisc/Makefile
+++ b/arch/parisc/Makefile
@@ -60,7 +60,6 @@ KBUILD_CFLAGS += -DCC_USING_PATCHABLE_FUNCTION_ENTRY=1 \
-DFTRACE_PATCHABLE_FUNCTION_SIZE=$(NOP_COUNT)
CC_FLAGS_FTRACE := -fpatchable-function-entry=$(NOP_COUNT),$(shell echo $$(($(NOP_COUNT)-1)))
-KBUILD_LDS_MODULE += $(srctree)/arch/parisc/kernel/module.lds
endif
OBJCOPY_FLAGS =-O binary -R .note -R .comment -S
diff --git a/arch/parisc/kernel/module.c b/arch/parisc/kernel/module.c
index ac5f34993b53..1c50093e2ebe 100644
--- a/arch/parisc/kernel/module.c
+++ b/arch/parisc/kernel/module.c
@@ -43,6 +43,7 @@
#include <linux/elf.h>
#include <linux/vmalloc.h>
#include <linux/fs.h>
+#include <linux/ftrace.h>
#include <linux/string.h>
#include <linux/kernel.h>
#include <linux/bug.h>
@@ -862,7 +863,7 @@ int module_finalize(const Elf_Ehdr *hdr,
const char *strtab = NULL;
const Elf_Shdr *s;
char *secstrings;
- int err, symindex = -1;
+ int symindex = -1;
Elf_Sym *newptr, *oldptr;
Elf_Shdr *symhdr = NULL;
#ifdef DEBUG
@@ -946,11 +947,13 @@ int module_finalize(const Elf_Ehdr *hdr,
/* patch .altinstructions */
apply_alternatives(aseg, aseg + s->sh_size, me->name);
+#ifdef CONFIG_DYNAMIC_FTRACE
/* For 32 bit kernels we're compiling modules with
* -ffunction-sections so we must relocate the addresses in the
- *__mcount_loc section.
+ * ftrace callsite section.
*/
- if (symindex != -1 && !strcmp(secname, "__mcount_loc")) {
+ if (symindex != -1 && !strcmp(secname, FTRACE_CALLSITE_SECTION)) {
+ int err;
if (s->sh_type == SHT_REL)
err = apply_relocate((Elf_Shdr *)sechdrs,
strtab, symindex,
@@ -962,6 +965,7 @@ int module_finalize(const Elf_Ehdr *hdr,
if (err)
return err;
}
+#endif
}
return 0;
}
diff --git a/arch/parisc/kernel/module.lds b/arch/parisc/kernel/module.lds
deleted file mode 100644
index 1a9a92aca5c8..000000000000
--- a/arch/parisc/kernel/module.lds
+++ /dev/null
@@ -1,7 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0 */
-
-SECTIONS {
- __mcount_loc : {
- *(__patchable_function_entries)
- }
-}
diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index 9867d90d635e..9141f2263286 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -738,6 +738,11 @@ static inline unsigned long get_lock_parent_ip(void)
#ifdef CONFIG_FTRACE_MCOUNT_RECORD
extern void ftrace_init(void);
+#ifdef CC_USING_PATCHABLE_FUNCTION_ENTRY
+#define FTRACE_CALLSITE_SECTION "__patchable_function_entries"
+#else
+#define FTRACE_CALLSITE_SECTION "__mcount_loc"
+#endif
#else
static inline void ftrace_init(void) { }
#endif
diff --git a/kernel/module.c b/kernel/module.c
index ff2d7359a418..acf7962936c4 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3222,7 +3222,7 @@ static int find_module_sections(struct module *mod, struct load_info *info)
#endif
#ifdef CONFIG_FTRACE_MCOUNT_RECORD
/* sechdrs[0].sh_size is always zero */
- mod->ftrace_callsites = section_objs(info, "__mcount_loc",
+ mod->ftrace_callsites = section_objs(info, FTRACE_CALLSITE_SECTION,
sizeof(*mod->ftrace_callsites),
&mod->num_ftrace_callsites);
#endif
--
2.11.0
On Tue, Oct 29, 2019 at 04:58:26PM +0000, Mark Rutland wrote:
> When using patchable-function-entry, the compiler will record the
> callsites into a section named "__patchable_function_entries" rather
> than "__mcount_loc". Let's abstract this difference behind a new
> FTRACE_CALLSITE_SECTION, so that architectures don't have to handle this
> explicitly (e.g. with custom module linker scripts).
>
> As parisc currently handles this explicitly, it is fixed up accordingly,
> with its custom linker script removed. Since FTRACE_CALLSITE_SECTION is
> only defined when DYNAMIC_FTRACE is selected, the parisc module loading
> code is updated to only use the definition in that case. When
> DYNAMIC_FTRACE is not selected, modules shouldn't have this section, so
> this removes some redundant work in that case.
>
> I built parisc generic-{32,64}bit_defconfig with DYNAMIC_FTRACE enabled,
> and verified that the section made it into the .ko files for modules.
This is because of remaining #ifdeffery in include/asm-generic/vmlinux.lds.h:
#ifdef CC_USING_PATCHABLE_FUNCTION_ENTRY
#define MCOUNT_REC() . = ALIGN(8); \
__start_mcount_loc = .; \
KEEP(*(__patchable_function_entries)) \
__stop_mcount_loc = .;
#else
#define MCOUNT_REC() . = ALIGN(8); \
__start_mcount_loc = .; \
KEEP(*(__mcount_loc)) \
__stop_mcount_loc = .;
#endif
Maybe you want to tackle that as well? I suggest to have at least one
FTRACE_CALLSITE_SECTION definition without double quotes. Alternatively, my
earlier solution just kept both sections, in case either one or both are
present.
KEEP(*(__patchable_function_entries)) \
KEEP(*(__mcount_loc)) \
Torsten
On Tue, 29 Oct 2019, Mark Rutland wrote:
> When we load a module, we have to perform some special work for a couple
> of named sections. To do this, we iterate over all of the module's
> sections, and perform work for each section we recognize.
>
> To make it easier to handle the unexpected absence of a section, and to
> make the section-specific logic easer to read, let's factor the section
s/easer/easier/
> search into a helper. Similar is already done in the core module loader,
> and other architectures (and ideally we'd unify these in future).
>
> If we expect a module to have an ftrace trampoline section, but it
> doesn't have one, we'll now reject loading the module. When
> ARM64_MODULE_PLTS is selected, any correctly built module should have
> one (and this is assumed by arm64's ftrace PLT code) and the absence of
> such a section implies something has gone wrong at build time.
>
> Subsequent patches will make use of the new helper.
>
> Signed-off-by: Mark Rutland <[email protected]>
> Reviewed-by: Ard Biesheuvel <[email protected]>
> Cc: Catalin Marinas <[email protected]>
> Cc: James Morse <[email protected]>
> Cc: Will Deacon <[email protected]>
Reviewed-by: Miroslav Benes <[email protected]>
M
Whole series, on top of Linus' HEAD 320000e72ec0613e164ce9 (5.4.0-rc5)
[ 0.418079] Testing dynamic ftrace: PASSED
[ 0.670416] Testing dynamic ftrace ops #1:
[ 0.751367] (1 0 1 0 0)
[ 0.751374] (1 1 2 0 0)
[ 0.857303] (2 1 3 0 281230)
[ 0.857327] (2 2 4 0 281332) PASSED
[ 0.930124] Testing dynamic ftrace ops #2:
[ 1.110333] (1 0 1 281189 0)
[ 1.110360] (1 1 2 281329 0)
[ 1.110815] (2 1 3 1 2)
[ 1.110841] (2 2 4 113 114) PASSED
[ 1.170653] Testing ftrace recursion: PASSED
[ 1.192250] Testing ftrace recursion safe: PASSED
[ 1.213819] Testing ftrace regs: PASSED
[ 1.235397] Testing tracer nop: PASSED
[ 1.235404] Testing tracer wakeup: PASSED
[ 1.363921] Testing tracer wakeup_rt: PASSED
[ 1.494283] Testing tracer wakeup_dl: PASSED
[ 1.623948] Testing tracer function_graph: PASSED
# tracer: function_graph
#
# CPU DURATION FUNCTION CALLS
# | | | | | | |
0) | wake_up_process() {
0) | try_to_wake_up() {
0) | select_task_rq_fair() {
0) | select_idle_sibling() {
0) 3.360 us | available_idle_cpu();
0) + 10.940 us | }
[...]
The graph tracer is the trickiest part to get working correctly, from my
experience. IOW: everything looks fine.
Whole series,
Tested-by: Torsten Duwe <[email protected]>
Torsten
On Tue, 29 Oct 2019, Mark Rutland wrote:
> Architectures may need to perform special initialization of ftrace
> callsites, and today they do so by special-casing ftrace_make_nop() when
> the expected branch address is MCOUNT_ADDR. In some cases (e.g. for
> patchable-function-entry), we don't have an mcount-like symbol and don't
> want a synthetic MCOUNT_ADDR, but we may need to perform some
> initialization of callsites.
>
> To make it possible to separate initialization from runtime
> modification, and to handle cases without an mcount-like symbol, this
> patch adds an optional ftrace_init_nop() function that architectures can
> implement, which does not pass a branch address.
>
> Where an architecture does not provide ftrace_init_nop(), we will fall
> back to the existing behaviour of calling ftrace_make_nop() with
> MCOUNT_ADDR.
>
> At the same time, ftrace_code_disable() is renamed to
> ftrace_nop_initialize() to make it clearer that it is intended to
> intialize a callsite into a disabled state, and is not for disabling a
> callsite that has been runtime enabled. The kerneldoc description of rec
> arguments is updated to cover non-mcount callsites.
>
> Signed-off-by: Mark Rutland <[email protected]>
> Reviewed-by: Ard Biesheuvel <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: Steven Rostedt <[email protected]>
> Cc: Torsten Duwe <[email protected]>
Reviewed-by: Miroslav Benes <[email protected]>
M
On Wed, Oct 30, 2019 at 04:03:02PM +0100, Torsten Duwe wrote:
> On Tue, Oct 29, 2019 at 04:58:26PM +0000, Mark Rutland wrote:
> > When using patchable-function-entry, the compiler will record the
> > callsites into a section named "__patchable_function_entries" rather
> > than "__mcount_loc". Let's abstract this difference behind a new
> > FTRACE_CALLSITE_SECTION, so that architectures don't have to handle this
> > explicitly (e.g. with custom module linker scripts).
> >
> > As parisc currently handles this explicitly, it is fixed up accordingly,
> > with its custom linker script removed. Since FTRACE_CALLSITE_SECTION is
> > only defined when DYNAMIC_FTRACE is selected, the parisc module loading
> > code is updated to only use the definition in that case. When
> > DYNAMIC_FTRACE is not selected, modules shouldn't have this section, so
> > this removes some redundant work in that case.
> >
> > I built parisc generic-{32,64}bit_defconfig with DYNAMIC_FTRACE enabled,
> > and verified that the section made it into the .ko files for modules.
>
> This is because of remaining #ifdeffery in include/asm-generic/vmlinux.lds.h:
>
> #ifdef CC_USING_PATCHABLE_FUNCTION_ENTRY
> #define MCOUNT_REC() . = ALIGN(8); \
> __start_mcount_loc = .; \
> KEEP(*(__patchable_function_entries)) \
> __stop_mcount_loc = .;
> #else
> #define MCOUNT_REC() . = ALIGN(8); \
> __start_mcount_loc = .; \
> KEEP(*(__mcount_loc)) \
> __stop_mcount_loc = .;
> #endif
For modules we use a combination of scripts/module-common.lds and an
architecture's own module.lds, not vmlinux.lds.h. So I don't think the above is
relevant for modules.
For modules the kernel's ELF loader looks for the ELF ection, not the
__{start,stop}_mcount_loc symbols that we use for the main kernel image.
FWIW, when building a module, I see the following linker operations:
| [mark@blommer:~/src/linux]% toolchain korg gcc-8.1.0-nolibc make V=1 ARCH=arm64 CROSS_COMPILE=aarch64-linux- arch/arm64/crypto/sha512-ce.ko | grep aarch64-linux-ld
| aarch64-linux-ld -EL -maarch64elf -r -o arch/arm64/crypto/sha512-ce.o arch/arm64/crypto/sha512-ce-glue.o arch/arm64/crypto/sha512-ce-core.o
| aarch64-linux-ld -r -EL -maarch64elf --build-id -T ./scripts/module-common.lds -T ./arch/arm64/kernel/module.lds -o arch/arm64/crypto/sha512-ce.ko arch/arm64/crypto/sha512-ce.o arch/arm64/crypto/sha512-ce.mod.o; true
> Maybe you want to tackle that as well? I suggest to have at least one
> FTRACE_CALLSITE_SECTION definition without double quotes. Alternatively, my
> earlier solution just kept both sections, in case either one or both are
> present.
>
> KEEP(*(__patchable_function_entries)) \
> KEEP(*(__mcount_loc)) \
I agree that the CC_USING_PATCHABLE_FUNCTION_ENTRY ifdeffery could be
simplified, and that it would be nice to consistently use
FTRACE_CALLSITE_SECTION if we can. However, the generic linker script doesn't
include anything, and I don't see a good location for that to live.
What I could do is add an explicit comment:
/*
* The ftrace call sites are logged to a section whose name depends on the
* compiler option used. A given kernel image will only use one, AKA
* FTRACE_CALLSITE_SECTION. We capture all of them here to avoid header
* dependencies.
*/
#define MCOUNT_REC() \
. = ALIGN(8); \
__start_mcount_loc = .; \
KEEP(*(__patchable_function_entries)) \
KEEP(*(__mcount_loc)) \
__stop_mcount_loc = .;
... which should make the dependency clear. Does that sound good to you?
Thanks,
Mark.
On Thu, Oct 31, 2019 at 09:02:32AM +0000, Mark Rutland wrote:
> On Wed, Oct 30, 2019 at 04:03:02PM +0100, Torsten Duwe wrote:
> > On Tue, Oct 29, 2019 at 04:58:26PM +0000, Mark Rutland wrote:
> > >
> > > I built parisc generic-{32,64}bit_defconfig with DYNAMIC_FTRACE enabled,
> > > and verified that the section made it into the .ko files for modules.
> >
> > This is because of remaining #ifdeffery in include/asm-generic/vmlinux.lds.h:
> >
> > #ifdef CC_USING_PATCHABLE_FUNCTION_ENTRY
> > #define MCOUNT_REC() . = ALIGN(8); \
> > __start_mcount_loc = .; \
> > KEEP(*(__patchable_function_entries)) \
> > __stop_mcount_loc = .;
> > #else
> > #define MCOUNT_REC() . = ALIGN(8); \
> > __start_mcount_loc = .; \
> > KEEP(*(__mcount_loc)) \
> > __stop_mcount_loc = .;
> > #endif
>
> For modules we use a combination of scripts/module-common.lds and an
> architecture's own module.lds, not vmlinux.lds.h. So I don't think the above is
> relevant for modules.
Sure, this is only loosely related,...
> I agree that the CC_USING_PATCHABLE_FUNCTION_ENTRY ifdeffery could be
> simplified, and that it would be nice to consistently use
> FTRACE_CALLSITE_SECTION if we can. However, the generic linker script doesn't
> include anything, and I don't see a good location for that to live.
>
> What I could do is add an explicit comment:
>
> /*
> * The ftrace call sites are logged to a section whose name depends on the
> * compiler option used. A given kernel image will only use one, AKA
> * FTRACE_CALLSITE_SECTION. We capture all of them here to avoid header
> * dependencies.
> */
> #define MCOUNT_REC() \
> . = ALIGN(8); \
> __start_mcount_loc = .; \
> KEEP(*(__patchable_function_entries)) \
> KEEP(*(__mcount_loc)) \
> __stop_mcount_loc = .;
>
> ... which should make the dependency clear. Does that sound good to you?
Beautiful. I just didn't want to miss the opportunity to have this cleaned
up as well, and deemed this patch "closest" because of the definition of
FTRACE_CALLSITE_SECTION. Put it where you see it fit best.
Thanks,
Torsten
On Thu, Oct 31, 2019 at 12:42:23PM +0100, Torsten Duwe wrote:
> On Thu, Oct 31, 2019 at 09:02:32AM +0000, Mark Rutland wrote:
> > I agree that the CC_USING_PATCHABLE_FUNCTION_ENTRY ifdeffery could be
> > simplified, and that it would be nice to consistently use
> > FTRACE_CALLSITE_SECTION if we can. However, the generic linker script doesn't
> > include anything, and I don't see a good location for that to live.
> >
> > What I could do is add an explicit comment:
> >
> > /*
> > * The ftrace call sites are logged to a section whose name depends on the
> > * compiler option used. A given kernel image will only use one, AKA
> > * FTRACE_CALLSITE_SECTION. We capture all of them here to avoid header
> > * dependencies.
> > */
> > #define MCOUNT_REC() \
> > . = ALIGN(8); \
> > __start_mcount_loc = .; \
> > KEEP(*(__patchable_function_entries)) \
> > KEEP(*(__mcount_loc)) \
> > __stop_mcount_loc = .;
> >
> > ... which should make the dependency clear. Does that sound good to you?
>
> Beautiful. I just didn't want to miss the opportunity to have this cleaned
> up as well, and deemed this patch "closest" because of the definition of
> FTRACE_CALLSITE_SECTION. Put it where you see it fit best.
Sure. I've folded the above into this patch, and pushed out an updated branch:
https://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git/log/?h=arm64/ftrace-with-regs
Thanks,
Mark.
After yesterday's testing, now
Reviewed-by: Torsten Duwe <[email protected]>
for the series.
Executive summary: where I used x28 as scratch register in ftrace_regs_caller
which I had to save for that reason, you switched to x10, which is so obvious
that I failed to see it. Then the PLT initialisation on module load, and
finally the ftrace_init_nop() hook that got you started initially. The rest
I'd call more or less cosmetic deviations from my v8. IOW: fine with me.
Torsten
On Thu, Oct 31, 2019 at 06:16:41PM +0100, Torsten Duwe wrote:
> After yesterday's testing, now
>
> Reviewed-by: Torsten Duwe <[email protected]>
>
> for the series.
Thanks! I've folded that in and pushed out the updated branch.
Since the only change this time around was only a trivial change in the linker
script, I'll hold off sending a v3. I'm hoping I can get acks for the ftrace,
module, and parisc bits soon...
> Executive summary: where I used x28 as scratch register in ftrace_regs_caller
> which I had to save for that reason, you switched to x10, which is so obvious
> that I failed to see it. Then the PLT initialisation on module load, and
> finally the ftrace_init_nop() hook that got you started initially. The rest
> I'd call more or less cosmetic deviations from my v8. IOW: fine with me.
Yup, that sounds about right. The other thing I did was expand the comments on
the ABI details, as that can be quite subtle, but I guess that's arguably
cosmetic, too.
Thanks for working on this, and for bearing with me on this rework -- I hadn't
intended that to drag on for so long.
Thanks,
Mark.
Hi Mark,
On Tue, Oct 29, 2019 at 04:58:24PM +0000, Mark Rutland wrote:
> Hi,
>
> This series is a reworked version of Torsten's FTRACE_WITH_REGS series
> [1]. I've tried to rework the existing code in preparatory patches so
> that the patchable-function-entry bits slot in with fewer surprises.
> This version is based on v5.4-rc3, and can be found in my
> arm64/ftrace-with-regs branch [2].
>
> Patch 1 adds an (optional) ftrace_init_nop(), which the core code uses
> to initialize callsites. This allows us to avoid a synthetic MCOUNT_ADDR
> symbol, and more cleanly separates the one-time initialization of the
> callsite from dynamic NOP<->CALL modification. Architectures which don't
> implement this get the existing ftrace_make_nop() with MCOUNT_ADDR.
>
> Recently parisc gained ftrace support using patchable-function-entry.
> Patch 2 makes the handling of module callsite locations common in
> kernel/module.c with a new FTRACE_CALLSITE_SECTION definition, and
> removed the newly redundant bits from arch/parisc.
>
> Patches 3 and 4 move the module PLT initialization to module load time,
> which simplifies runtime callsite modification. This also means that we
> don't transitently mark the module text RW, and will allow for the
> removal of module_disable_ro().
>
> Patches 5 and 6 add some trivial infrastructure, with patch 7 finally
> adding FTRACE_WITH_REGS support. Additional work will be required for
> livepatching (e.g. implementing reliable stack trace), which is
> commented as part of patch 7.
>
> Patch 8 is a trivial cleanup atop of the rest of the series, making the
> code easier to read and less susceptible to config-specific breakage.
>
> Since v1 [3]:
> * Add a couple of people to Cc
> * Fold in Ard's Reviewed-by tag
> * Rename ftrace_code_init_disabled() to ftrace_nop_initialize()
> * Move ftrace_init_nop() to <linux/ftrace.h>, with kerneldoc
> * Update kerneldoc for rec parameters
[..]
I tested this series on parisc both with ftracing kernel internal functions and
module functions. Both are working fine, so feel free to add my
Tested-by: Sven Schnelle <[email protected]>
Regards
Sven
On Fri, Nov 01, 2019 at 04:39:30PM +0100, Sven Schnelle wrote:
> On Tue, Oct 29, 2019 at 04:58:24PM +0000, Mark Rutland wrote:
> > This series is a reworked version of Torsten's FTRACE_WITH_REGS series
> > [1]. I've tried to rework the existing code in preparatory patches so
> > that the patchable-function-entry bits slot in with fewer surprises.
> > This version is based on v5.4-rc3, and can be found in my
> > arm64/ftrace-with-regs branch [2].
> >
> > Patch 1 adds an (optional) ftrace_init_nop(), which the core code uses
> > to initialize callsites. This allows us to avoid a synthetic MCOUNT_ADDR
> > symbol, and more cleanly separates the one-time initialization of the
> > callsite from dynamic NOP<->CALL modification. Architectures which don't
> > implement this get the existing ftrace_make_nop() with MCOUNT_ADDR.
> >
> > Recently parisc gained ftrace support using patchable-function-entry.
> > Patch 2 makes the handling of module callsite locations common in
> > kernel/module.c with a new FTRACE_CALLSITE_SECTION definition, and
> > removed the newly redundant bits from arch/parisc.
> > Since v1 [3]:
> > * Add a couple of people to Cc
> > * Fold in Ard's Reviewed-by tag
> > * Rename ftrace_code_init_disabled() to ftrace_nop_initialize()
> > * Move ftrace_init_nop() to <linux/ftrace.h>, with kerneldoc
> > * Update kerneldoc for rec parameters
> [..]
>
> I tested this series on parisc both with ftracing kernel internal functions and
> module functions. Both are working fine, so feel free to add my
>
> Tested-by: Sven Schnelle <[email protected]>
Thanks! That's much appreciated.
I've applied that to patches 1 and 2, since the remainder of the series was
confined to arch/arm64/.
Mark.
Hi Mark,
On 10/29/19 10:28 PM, Mark Rutland wrote:
> Hi,
>
> This series is a reworked version of Torsten's FTRACE_WITH_REGS series
> [1]. I've tried to rework the existing code in preparatory patches so
> that the patchable-function-entry bits slot in with fewer surprises.
> This version is based on v5.4-rc3, and can be found in my
> arm64/ftrace-with-regs branch [2].
>
> Patch 1 adds an (optional) ftrace_init_nop(), which the core code uses
> to initialize callsites. This allows us to avoid a synthetic MCOUNT_ADDR
> symbol, and more cleanly separates the one-time initialization of the
> callsite from dynamic NOP<->CALL modification. Architectures which don't
> implement this get the existing ftrace_make_nop() with MCOUNT_ADDR.
>
> Recently parisc gained ftrace support using patchable-function-entry.
> Patch 2 makes the handling of module callsite locations common in
> kernel/module.c with a new FTRACE_CALLSITE_SECTION definition, and
> removed the newly redundant bits from arch/parisc.
>
> Patches 3 and 4 move the module PLT initialization to module load time,
> which simplifies runtime callsite modification. This also means that we
> don't transitently mark the module text RW, and will allow for the
> removal of module_disable_ro().
>
> Patches 5 and 6 add some trivial infrastructure, with patch 7 finally
> adding FTRACE_WITH_REGS support. Additional work will be required for
> livepatching (e.g. implementing reliable stack trace), which is
> commented as part of patch 7.
>
> Patch 8 is a trivial cleanup atop of the rest of the series, making the
> code easier to read and less susceptible to config-specific breakage.
I tested the whole series with my latest in-kernel ptrauth patches [1]
and graph_tracer/function_graph_tracer works fine, So for the whole series,
Tested-by: Amit Daniel Kachhap <[email protected]>
Also I gave few minor comments in the individual patches. With those
comments,
Signed-off-by: Amit Daniel Kachhap <[email protected]>
Thanks,
Amit Daniel
[1]: https://patchwork.kernel.org/cover/11195085/
>
> Since v1 [3]:
> * Add a couple of people to Cc
> * Fold in Ard's Reviewed-by tag
> * Rename ftrace_code_init_disabled() to ftrace_nop_initialize()
> * Move ftrace_init_nop() to <linux/ftrace.h>, with kerneldoc
> * Update kerneldoc for rec parameters
>
> Thanks,
> Mark.
>
> [1] https://lore.kernel.org/r/[email protected]
> [2] git://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git arm64/ftrace-with-regs
> [3] https://lore.kernel.org/r/[email protected]
>
> Mark Rutland (7):
> ftrace: add ftrace_init_nop()
> module/ftrace: handle patchable-function-entry
> arm64: module: rework special section handling
> arm64: module/ftrace: intialize PLT at load time
> arm64: insn: add encoder for MOV (register)
> arm64: asm-offsets: add S_FP
> arm64: ftrace: minimize ifdeffery
>
> Torsten Duwe (1):
> arm64: implement ftrace with regs
>
> arch/arm64/Kconfig | 2 +
> arch/arm64/Makefile | 5 ++
> arch/arm64/include/asm/ftrace.h | 23 +++++++
> arch/arm64/include/asm/insn.h | 3 +
> arch/arm64/include/asm/module.h | 2 +-
> arch/arm64/kernel/asm-offsets.c | 1 +
> arch/arm64/kernel/entry-ftrace.S | 140 +++++++++++++++++++++++++++++++++++++--
> arch/arm64/kernel/ftrace.c | 123 ++++++++++++++++++++--------------
> arch/arm64/kernel/insn.c | 13 ++++
> arch/arm64/kernel/module-plts.c | 3 +-
> arch/arm64/kernel/module.c | 57 +++++++++++++---
> arch/parisc/Makefile | 1 -
> arch/parisc/kernel/module.c | 10 ++-
> arch/parisc/kernel/module.lds | 7 --
> include/linux/ftrace.h | 40 ++++++++++-
> kernel/module.c | 2 +-
> kernel/trace/ftrace.c | 6 +-
> 17 files changed, 355 insertions(+), 83 deletions(-)
> delete mode 100644 arch/parisc/kernel/module.lds
>
Hi Mark,
On 10/29/19 10:28 PM, Mark Rutland wrote:
> Architectures may need to perform special initialization of ftrace
> callsites, and today they do so by special-casing ftrace_make_nop() when
> the expected branch address is MCOUNT_ADDR. In some cases (e.g. for
> patchable-function-entry), we don't have an mcount-like symbol and don't
s/an mcount/a mcount.
> want a synthetic MCOUNT_ADDR, but we may need to perform some
> initialization of callsites.
>
> To make it possible to separate initialization from runtime
> modification, and to handle cases without an mcount-like symbol, this
Same as above.
> patch adds an optional ftrace_init_nop() function that architectures can
> implement, which does not pass a branch address.
>
> Where an architecture does not provide ftrace_init_nop(), we will fall
> back to the existing behaviour of calling ftrace_make_nop() with
> MCOUNT_ADDR.
>
> At the same time, ftrace_code_disable() is renamed to
> ftrace_nop_initialize() to make it clearer that it is intended to
> intialize a callsite into a disabled state, and is not for disabling a
> callsite that has been runtime enabled. The kerneldoc description of rec
> arguments is updated to cover non-mcount callsites.
>
> Signed-off-by: Mark Rutland <[email protected]>
> Reviewed-by: Ard Biesheuvel <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: Steven Rostedt <[email protected]>
> Cc: Torsten Duwe <[email protected]>
> ---
> include/linux/ftrace.h | 35 ++++++++++++++++++++++++++++++++---
> kernel/trace/ftrace.c | 6 +++---
> 2 files changed, 35 insertions(+), 6 deletions(-)
>
> diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> index 8a8cb3c401b2..9867d90d635e 100644
> --- a/include/linux/ftrace.h
> +++ b/include/linux/ftrace.h
> @@ -499,7 +499,7 @@ static inline int ftrace_disable_ftrace_graph_caller(void) { return 0; }
> /**
> * ftrace_make_nop - convert code into nop
> * @mod: module structure if called by module load initialization
> - * @rec: the mcount call site record
> + * @rec: the call site record (e.g. mcount/fentry)
> * @addr: the address that the call site should be calling
> *
> * This is a very sensitive operation and great care needs
> @@ -520,9 +520,38 @@ static inline int ftrace_disable_ftrace_graph_caller(void) { return 0; }
> extern int ftrace_make_nop(struct module *mod,
> struct dyn_ftrace *rec, unsigned long addr);
>
> +
> +/**
> + * ftrace_init_nop - initialize a nop call site
> + * @mod: module structure if called by module load initialization
> + * @rec: the call site record (e.g. mcount/fentry)
> + *
> + * This is a very sensitive operation and great care needs
> + * to be taken by the arch. The operation should carefully
> + * read the location, check to see if what is read is indeed
> + * what we expect it to be, and then on success of the compare,
> + * it should write to the location.
> + *
> + * The code segment at @rec->ip should contain the contents created by
> + * the compiler
Nit: Will it be better to write it as "@rec->ip should store the
adjusted ftrace entry address of the call site" or something like that.
> + *
> + * Return must be:
> + * 0 on success
> + * -EFAULT on error reading the location
> + * -EINVAL on a failed compare of the contents
> + * -EPERM on error writing to the location
> + * Any other value will be considered a failure.
> + */
> +#ifndef ftrace_init_nop
> +static inline int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec)
> +{
> + return ftrace_make_nop(mod, rec, MCOUNT_ADDR);
> +}
> +#endif
> +
Now that ftrace_init_nop is also an arch implemented function so it may
be added in Documentation/trace/ftrace-design.rst along with
ftrace_make_nop.
In general also, adding some description about patchable-function-entry
in kernel Documentation will be useful.
Thanks,
Amit Daniel
> /**
> * ftrace_make_call - convert a nop call site into a call to addr
> - * @rec: the mcount call site record
> + * @rec: the call site record (e.g. mcount/fentry)
> * @addr: the address that the call site should call
> *
> * This is a very sensitive operation and great care needs
> @@ -545,7 +574,7 @@ extern int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr);
> #ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
> /**
> * ftrace_modify_call - convert from one addr to another (no nop)
> - * @rec: the mcount call site record
> + * @rec: the call site record (e.g. mcount/fentry)
> * @old_addr: the address expected to be currently called to
> * @addr: the address to change to
> *
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index f296d89be757..5259d4dea675 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -2494,14 +2494,14 @@ struct dyn_ftrace *ftrace_rec_iter_record(struct ftrace_rec_iter *iter)
> }
>
> static int
> -ftrace_code_disable(struct module *mod, struct dyn_ftrace *rec)
> +ftrace_nop_initialize(struct module *mod, struct dyn_ftrace *rec)
> {
> int ret;
>
> if (unlikely(ftrace_disabled))
> return 0;
>
> - ret = ftrace_make_nop(mod, rec, MCOUNT_ADDR);
> + ret = ftrace_init_nop(mod, rec);
> if (ret) {
> ftrace_bug_type = FTRACE_BUG_INIT;
> ftrace_bug(ret, rec);
> @@ -2943,7 +2943,7 @@ static int ftrace_update_code(struct module *mod, struct ftrace_page *new_pgs)
> * to the NOP instructions.
> */
> if (!__is_defined(CC_USING_NOP_MCOUNT) &&
> - !ftrace_code_disable(mod, p))
> + !ftrace_nop_initialize(mod, p))
> break;
>
> update_cnt++;
>
Hi,
On 10/29/19 10:28 PM, Mark Rutland wrote:
> Currently we lazily-initialize a module's ftrace PLT at runtime when we
> install the first ftrace call. To do so we have to apply a number of
> sanity checks, transiently mark the module text as RW, and perform an
> IPI as part of handling Neoverse-N1 erratum #1542419.
>
> We only expect the ftrace trampoline to point at ftrace_caller() (AKA
> FTRACE_ADDR), so let's simplify all of this by intializing the PLT at
> module load time, before the module loader marks the module RO and
> performs the intial I-cache maintenance for the module.
>
> Thus we can rely on the module having been correctly intialized, and can
> simplify the runtime work necessary to install an ftrace call in a
> module. This will also allow for the removal of module_disable_ro().
>
> Tested by forcing ftrace_make_call() to use the module PLT, and then
> loading up a module after setting up ftrace with:
>
> | echo ":mod:<module-name>" > set_ftrace_filter;
> | echo function > current_tracer;
> | modprobe <module-name>
>
> Since FTRACE_ADDR is only defined when CONFIG_DYNAMIC_FTRACE is
> selected, we wrap its use along with most of module_init_ftrace_plt()
> with ifdeffery rather than using IS_ENABLED().
>
> Signed-off-by: Mark Rutland <[email protected]>
> Reviewed-by: Ard Biesheuvel <[email protected]>
> Cc: Catalin Marinas <[email protected]>
> Cc: James Morse <[email protected]>
> Cc: Peter Zijlstra <[email protected]>
> Cc: Will Deacon <[email protected]>
> ---
> arch/arm64/kernel/ftrace.c | 55 ++++++++++++----------------------------------
> arch/arm64/kernel/module.c | 32 +++++++++++++++++----------
> 2 files changed, 35 insertions(+), 52 deletions(-)
>
> diff --git a/arch/arm64/kernel/ftrace.c b/arch/arm64/kernel/ftrace.c
> index 06e56b470315..822718eafdb4 100644
> --- a/arch/arm64/kernel/ftrace.c
> +++ b/arch/arm64/kernel/ftrace.c
> @@ -73,10 +73,22 @@ int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
>
> if (offset < -SZ_128M || offset >= SZ_128M) {
> #ifdef CONFIG_ARM64_MODULE_PLTS
> - struct plt_entry trampoline, *dst;
> struct module *mod;
>
> /*
> + * There is only one ftrace trampoline per module. For now,
> + * this is not a problem since on arm64, all dynamic ftrace
> + * invocations are routed via ftrace_caller(). This will need
> + * to be revisited if support for multiple ftrace entry points
> + * is added in the future, but for now, the pr_err() below
> + * deals with a theoretical issue only.
> + */
> + if (addr != FTRACE_ADDR) {
> + pr_err("ftrace: far branches to multiple entry points unsupported inside a single module\n");
> + return -EINVAL;
> + }
> +
> + /*
> * On kernels that support module PLTs, the offset between the
> * branch instruction and its target may legally exceed the
> * range of an ordinary relative 'bl' opcode. In this case, we
> @@ -93,46 +105,7 @@ int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
> if (WARN_ON(!mod))
> return -EINVAL;
>
> - /*
> - * There is only one ftrace trampoline per module. For now,
> - * this is not a problem since on arm64, all dynamic ftrace
> - * invocations are routed via ftrace_caller(). This will need
> - * to be revisited if support for multiple ftrace entry points
> - * is added in the future, but for now, the pr_err() below
> - * deals with a theoretical issue only.
> - *
> - * Note that PLTs are place relative, and plt_entries_equal()
> - * checks whether they point to the same target. Here, we need
> - * to check if the actual opcodes are in fact identical,
> - * regardless of the offset in memory so use memcmp() instead.
> - */
> - dst = mod->arch.ftrace_trampoline;
> - trampoline = get_plt_entry(addr, dst);
> - if (memcmp(dst, &trampoline, sizeof(trampoline))) {
> - if (plt_entry_is_initialized(dst)) {
> - pr_err("ftrace: far branches to multiple entry points unsupported inside a single module\n");
> - return -EINVAL;
> - }
> -
> - /* point the trampoline to our ftrace entry point */
> - module_disable_ro(mod);
> - *dst = trampoline;
> - module_enable_ro(mod, true);
> -
> - /*
> - * Ensure updated trampoline is visible to instruction
> - * fetch before we patch in the branch. Although the
> - * architecture doesn't require an IPI in this case,
> - * Neoverse-N1 erratum #1542419 does require one
> - * if the TLB maintenance in module_enable_ro() is
> - * skipped due to rodata_enabled. It doesn't seem worth
> - * it to make it conditional given that this is
> - * certainly not a fast-path.
> - */
> - flush_icache_range((unsigned long)&dst[0],
> - (unsigned long)&dst[1]);
> - }
> - addr = (unsigned long)dst;
> + addr = (unsigned long)mod->arch.ftrace_trampoline;
> #else /* CONFIG_ARM64_MODULE_PLTS */
> return -EINVAL;
> #endif /* CONFIG_ARM64_MODULE_PLTS */
> diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c
> index 763a86d52fef..5f5bc3b94da7 100644
> --- a/arch/arm64/kernel/module.c
> +++ b/arch/arm64/kernel/module.c
> @@ -9,6 +9,7 @@
>
> #include <linux/bitops.h>
> #include <linux/elf.h>
> +#include <linux/ftrace.h>
> #include <linux/gfp.h>
> #include <linux/kasan.h>
> #include <linux/kernel.h>
> @@ -485,24 +486,33 @@ static const Elf_Shdr *find_section(const Elf_Ehdr *hdr,
> return NULL;
> }
>
> +int module_init_ftrace_plt(const Elf_Ehdr *hdr,
> + const Elf_Shdr *sechdrs,
> + struct module *mod)
I think this function can be made static as it is not used anywhere.
Thanks,
Amit Daniel
> +{
> +#if defined(CONFIG_ARM64_MODULE_PLTS) && defined(CONFIG_DYNAMIC_FTRACE)
> + const Elf_Shdr *s;
> + struct plt_entry *plt;
> +
> + s = find_section(hdr, sechdrs, ".text.ftrace_trampoline");
> + if (!s)
> + return -ENOEXEC;
> +
> + plt = (void *)s->sh_addr;
> + *plt = get_plt_entry(FTRACE_ADDR, plt);
> + mod->arch.ftrace_trampoline = plt;
> +#endif
> + return 0;
> +}
> +
> int module_finalize(const Elf_Ehdr *hdr,
> const Elf_Shdr *sechdrs,
> struct module *me)
> {
> const Elf_Shdr *s;
> -
> s = find_section(hdr, sechdrs, ".altinstructions");
> if (s)
> apply_alternatives_module((void *)s->sh_addr, s->sh_size);
>
> -#ifdef CONFIG_ARM64_MODULE_PLTS
> - if (IS_ENABLED(CONFIG_DYNAMIC_FTRACE)) {
> - s = find_section(hdr, sechdrs, ".text.ftrace_trampoline");
> - if (!s)
> - return -ENOEXEC;
> - me->arch.ftrace_trampoline = (void *)s->sh_addr;
> - }
> -#endif
> -
> - return 0;
> + return module_init_ftrace_plt(hdr, sechdrs, me);
> }
>
On 10/29/19 10:28 PM, Mark Rutland wrote:
> From: Torsten Duwe <[email protected]>
>
> This patch implements FTRACE_WITH_REGS for arm64, which allows a traced
> function's arguments (and some other registers) to be captured into a
> struct pt_regs, allowing these to be inspected and/or modified. This is
> a building block for live-patching, where a function's arguments may be
> forwarded to another function. This is also necessary to enable ftrace
> and in-kernel pointer authentication at the same time, as it allows the
> LR value to be captured and adjusted prior to signing.
>
> Using GCC's -fpatchable-function-entry=N option, we can have the
> compiler insert a configurable number of NOPs between the function entry
> point and the usual prologue. This also ensures functions are AAPCS
> compliant (e.g. disabling inter-procedural register allocation).
>
> For example, with -fpatchable-function-entry=2, GCC 8.1.0 compiles the
> following:
>
> | unsigned long bar(void);
> |
> | unsigned long foo(void)
> | {
> | return bar() + 1;
> | }
>
> ... to:
>
> | <foo>:
> | nop
> | nop
> | stp x29, x30, [sp, #-16]!
> | mov x29, sp
> | bl 0 <bar>
> | add x0, x0, #0x1
> | ldp x29, x30, [sp], #16
> | ret
>
> This patch builds the kernel with -fpatchable-function-entry=2,
> prefixing each function with two NOPs. To trace a function, we replace
> these NOPs with a sequence that saves the LR into a GPR, then calls an
> ftrace entry assembly function which saves this and other relevant
> registers:
>
> | mov x9, x30
> | bl <ftrace-entry>
>
> Since patchable functions are AAPCS compliant (and the kernel does not
> use x18 as a platform register), x9-x18 can be safely clobbered in the
> patched sequence and the ftrace entry code.
>
> There are now two ftrace entry functions, ftrace_regs_entry (which saves
> all GPRs), and ftrace_entry (which saves the bare minimum). A PLT is
> allocated for each within modules.
>
> Signed-off-by: Torsten Duwe <[email protected]>
> [Mark: rework asm, comments, PLTs, initialization, commit message]
> Signed-off-by: Mark Rutland <[email protected]>
> Reviewed-by: Ard Biesheuvel <[email protected]>
> Cc: AKASHI Takahiro <[email protected]>
> Cc: Amit Daniel Kachhap <[email protected]>
> Cc: Catalin Marinas <[email protected]>
> Cc: Josh Poimboeuf <[email protected]>
> Cc: Julien Thierry <[email protected]>
> Cc: Will Deacon <[email protected]>
> ---
> arch/arm64/Kconfig | 2 +
> arch/arm64/Makefile | 5 ++
> arch/arm64/include/asm/ftrace.h | 23 +++++++
> arch/arm64/include/asm/module.h | 2 +-
> arch/arm64/kernel/entry-ftrace.S | 140 +++++++++++++++++++++++++++++++++++++--
> arch/arm64/kernel/ftrace.c | 84 +++++++++++++++++++----
> arch/arm64/kernel/module-plts.c | 3 +-
> arch/arm64/kernel/module.c | 18 +++--
> 8 files changed, 252 insertions(+), 25 deletions(-)
>
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 950a56b71ff0..0ffb8596b8a1 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -143,6 +143,8 @@ config ARM64
> select HAVE_DEBUG_KMEMLEAK
> select HAVE_DMA_CONTIGUOUS
> select HAVE_DYNAMIC_FTRACE
> + select HAVE_DYNAMIC_FTRACE_WITH_REGS \
> + if $(cc-option,-fpatchable-function-entry=2)
> select HAVE_EFFICIENT_UNALIGNED_ACCESS
> select HAVE_FAST_GUP
> select HAVE_FTRACE_MCOUNT_RECORD
> diff --git a/arch/arm64/Makefile b/arch/arm64/Makefile
> index 2c0238ce0551..1fbe24d4fdb6 100644
> --- a/arch/arm64/Makefile
> +++ b/arch/arm64/Makefile
> @@ -95,6 +95,11 @@ ifeq ($(CONFIG_ARM64_MODULE_PLTS),y)
> KBUILD_LDS_MODULE += $(srctree)/arch/arm64/kernel/module.lds
> endif
>
> +ifeq ($(CONFIG_DYNAMIC_FTRACE_WITH_REGS),y)
> + KBUILD_CPPFLAGS += -DCC_USING_PATCHABLE_FUNCTION_ENTRY
> + CC_FLAGS_FTRACE := -fpatchable-function-entry=2
> +endif
> +
> # Default value
> head-y := arch/arm64/kernel/head.o
>
> diff --git a/arch/arm64/include/asm/ftrace.h b/arch/arm64/include/asm/ftrace.h
> index d48667b04c41..91fa4baa1a93 100644
> --- a/arch/arm64/include/asm/ftrace.h
> +++ b/arch/arm64/include/asm/ftrace.h
> @@ -11,9 +11,20 @@
> #include <asm/insn.h>
>
> #define HAVE_FUNCTION_GRAPH_FP_TEST
> +
> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
> +#define ARCH_SUPPORTS_FTRACE_OPS 1
> +#else
> #define MCOUNT_ADDR ((unsigned long)_mcount)
> +#endif
> +
> +/* The BL at the callsite's adjusted rec->ip */
> #define MCOUNT_INSN_SIZE AARCH64_INSN_SIZE
>
> +#define FTRACE_PLT_IDX 0
> +#define FTRACE_REGS_PLT_IDX 1
> +#define NR_FTRACE_PLTS 2
> +
> /*
> * Currently, gcc tends to save the link register after the local variables
> * on the stack. This causes the max stack tracer to report the function
> @@ -44,12 +55,24 @@ extern void return_to_handler(void);
> static inline unsigned long ftrace_call_adjust(unsigned long addr)
> {
> /*
> + * Adjust addr to point at the BL in the callsite.
> + * See ftrace_init_nop() for the callsite sequence.
> + */
> + if (IS_ENABLED(CONFIG_DYNAMIC_FTRACE_WITH_REGS))
> + return addr + AARCH64_INSN_SIZE;
> + /*
> * addr is the address of the mcount call instruction.
> * recordmcount does the necessary offset calculation.
> */
> return addr;
> }
>
> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
> +struct dyn_ftrace;
> +int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec);
> +#define ftrace_init_nop ftrace_init_nop
> +#endif
> +
> #define ftrace_return_address(n) return_address(n)
>
> /*
> diff --git a/arch/arm64/include/asm/module.h b/arch/arm64/include/asm/module.h
> index f80e13cbf8ec..1e93de68c044 100644
> --- a/arch/arm64/include/asm/module.h
> +++ b/arch/arm64/include/asm/module.h
> @@ -21,7 +21,7 @@ struct mod_arch_specific {
> struct mod_plt_sec init;
>
> /* for CONFIG_DYNAMIC_FTRACE */
> - struct plt_entry *ftrace_trampoline;
> + struct plt_entry *ftrace_trampolines;
> };
> #endif
>
> diff --git a/arch/arm64/kernel/entry-ftrace.S b/arch/arm64/kernel/entry-ftrace.S
> index 33d003d80121..94720388957f 100644
> --- a/arch/arm64/kernel/entry-ftrace.S
> +++ b/arch/arm64/kernel/entry-ftrace.S
> @@ -7,10 +7,137 @@
> */
>
> #include <linux/linkage.h>
> +#include <asm/asm-offsets.h>
> #include <asm/assembler.h>
> #include <asm/ftrace.h>
> #include <asm/insn.h>
>
> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
> +/*
> + * Due to -fpatchable-function-entry=2, the compiler has placed two NOPs before
> + * the regular function prologue. For an enabled callsite, ftrace_init_nop() and
> + * ftrace_make_call() have patched those NOPs to:
> + *
> + * MOV X9, LR
> + * BL <entry>
> + *
> + * ... where <entry> is either ftrace_caller or ftrace_regs_caller.
> + *
> + * Each instrumented function follows the AAPCS, so here x0-x8 and x19-x30 are
> + * live, and x9-x18 are safe to clobber.
> + *
> + * We save the callsite's context into a pt_regs before invoking and ftrace
s/invoking and ftrace callbacks/invoking the ftrace callbacks
> + * callbacks. So that we can get a sensible backtrace, we create a stack record
> + * for the callsite and the ftrace entry assembly. This is not sufficient for
> + * reliable stacktrace: until we create the callsite stack record, its caller
> + * is missing from the LR and existing chain of frame records.
> + */
> + .macro ftrace_regs_entry, allregs=0
> + /* Make room for pt_regs, plus a callee frame */
> + sub sp, sp, #(S_FRAME_SIZE + 16)
> +
> + /* Save function arguments (and x9 for simplicity) */
> + stp x0, x1, [sp, #S_X0]
> + stp x2, x3, [sp, #S_X2]
> + stp x4, x5, [sp, #S_X4]
> + stp x6, x7, [sp, #S_X6]
> + stp x8, x9, [sp, #S_X8]
> +
> + /* Optionally save the callee-saved registers, always save the FP */
> + .if \allregs == 1
> + stp x10, x11, [sp, #S_X10]
> + stp x12, x13, [sp, #S_X12]
> + stp x14, x15, [sp, #S_X14]
> + stp x16, x17, [sp, #S_X16]
> + stp x18, x19, [sp, #S_X18]
> + stp x20, x21, [sp, #S_X20]
> + stp x22, x23, [sp, #S_X22]
> + stp x24, x25, [sp, #S_X24]
> + stp x26, x27, [sp, #S_X26]
> + stp x28, x29, [sp, #S_X28]
> + .else
> + str x29, [sp, #S_FP]
> + .endif
> +
> + /* Save the callsite's SP and LR */
> + add x10, sp, #(S_FRAME_SIZE + 16)
> + stp x9, x10, [sp, #S_LR]
> +
> + /* Save the PC after the ftrace callsite */
> + str x30, [sp, #S_PC]
> +
> + /* Create a frame record for the callsite above pt_regs */
> + stp x29, x9, [sp, #S_FRAME_SIZE]
> + add x29, sp, #S_FRAME_SIZE
> +
> + /* Create our frame record within pt_regs. */
> + stp x29, x30, [sp, #S_STACKFRAME]
> + add x29, sp, #S_STACKFRAME
> + .endm
> +
> +ENTRY(ftrace_regs_caller)
> + ftrace_regs_entry 1
> + b ftrace_common
> +ENDPROC(ftrace_regs_caller)
> +
> +ENTRY(ftrace_caller)
> + ftrace_regs_entry 0
> + b ftrace_common
> +ENDPROC(ftrace_caller)
> +
> +ENTRY(ftrace_common)
> + sub x0, x30, #AARCH64_INSN_SIZE // ip (callsite's BL insn)
> + mov x1, x9 // parent_ip (callsite's LR)
> + ldr_l x2, function_trace_op // op
> + mov x3, sp // regs
> +
> +GLOBAL(ftrace_call)
> + bl ftrace_stub
> +
> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> +GLOBAL(ftrace_graph_call) // ftrace_graph_caller();
> + nop // If enabled, this will be replaced
> + // "b ftrace_graph_caller"
> +#endif
> +
> +/*
> + * At the callsite x0-x8 and x19-x30 were live. Any C code will have preserved
> + * x19-x29 per the AAPCS, and we created frame records upon entry, so we need
> + * to restore x0-x8, x29, and x30.
> + */
> +ftrace_common_return:
> + /* Restore function arguments */
> + ldp x0, x1, [sp]
> + ldp x2, x3, [sp, #S_X2]
> + ldp x4, x5, [sp, #S_X4]
> + ldp x6, x7, [sp, #S_X6]
> + ldr x8, [sp, #S_X8]
> +
> + /* Restore the callsite's FP, LR, PC */
> + ldr x29, [sp, #S_FP]
> + ldr x30, [sp, #S_LR]
> + ldr x9, [sp, #S_PC]
> +
> + /* Restore the callsite's SP */
> + add sp, sp, #S_FRAME_SIZE + 16
> +
> + ret x9
> +ENDPROC(ftrace_common)
> +
> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> +ENTRY(ftrace_graph_caller)
> + ldr x0, [sp, #S_PC]
> + sub x0, x0, #AARCH64_INSN_SIZE // ip (callsite's BL insn)
> + add x1, sp, #S_LR // parent_ip (callsite's LR)
> + ldr x2, [sp, #S_FRAME_SIZE] // parent fp (callsite's FP)
> + bl prepare_ftrace_return
> + b ftrace_common_return
> +ENDPROC(ftrace_graph_caller)
> +#else
> +#endif
> +
> +#else /* CONFIG_DYNAMIC_FTRACE_WITH_REGS */
> +
> /*
> * Gcc with -pg will put the following code in the beginning of each function:
> * mov x0, x30
> @@ -160,11 +287,6 @@ GLOBAL(ftrace_graph_call) // ftrace_graph_caller();
>
> mcount_exit
> ENDPROC(ftrace_caller)
> -#endif /* CONFIG_DYNAMIC_FTRACE */
> -
> -ENTRY(ftrace_stub)
> - ret
> -ENDPROC(ftrace_stub)
>
> #ifdef CONFIG_FUNCTION_GRAPH_TRACER
> /*
> @@ -184,7 +306,15 @@ ENTRY(ftrace_graph_caller)
>
> mcount_exit
> ENDPROC(ftrace_graph_caller)
> +#endif /* CONFIG_FUNCTION_GRAPH_TRACER */
> +#endif /* CONFIG_DYNAMIC_FTRACE */
> +#endif /* CONFIG_DYNAMIC_FTRACE_WITH_REGS */
> +
> +ENTRY(ftrace_stub)
> + ret
> +ENDPROC(ftrace_stub)
>
> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> /*
> * void return_to_handler(void)
> *
> diff --git a/arch/arm64/kernel/ftrace.c b/arch/arm64/kernel/ftrace.c
> index 822718eafdb4..aea652c33a38 100644
> --- a/arch/arm64/kernel/ftrace.c
> +++ b/arch/arm64/kernel/ftrace.c
> @@ -62,6 +62,19 @@ int ftrace_update_ftrace_func(ftrace_func_t func)
> return ftrace_modify_code(pc, 0, new, false);
> }
>
> +#ifdef CONFIG_ARM64_MODULE_PLTS
> +static struct plt_entry *get_ftrace_plt(struct module *mod, unsigned long addr)
> +{
> + struct plt_entry *plt = mod->arch.ftrace_trampolines;
> +
> + if (addr == FTRACE_ADDR)
> + return &plt[FTRACE_PLT_IDX];
> + if (addr == FTRACE_REGS_ADDR && IS_ENABLED(CONFIG_FTRACE_WITH_REGS))
> + return &plt[FTRACE_REGS_PLT_IDX];
> + return NULL;
> +}
> +#endif
> +
> /*
> * Turn on the call to ftrace_caller() in instrumented function
> */
> @@ -74,19 +87,7 @@ int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
> if (offset < -SZ_128M || offset >= SZ_128M) {
> #ifdef CONFIG_ARM64_MODULE_PLTS
> struct module *mod;
> -
> - /*
> - * There is only one ftrace trampoline per module. For now,
> - * this is not a problem since on arm64, all dynamic ftrace
> - * invocations are routed via ftrace_caller(). This will need
> - * to be revisited if support for multiple ftrace entry points
> - * is added in the future, but for now, the pr_err() below
> - * deals with a theoretical issue only.
> - */
> - if (addr != FTRACE_ADDR) {
> - pr_err("ftrace: far branches to multiple entry points unsupported inside a single module\n");
> - return -EINVAL;
> - }
> + struct plt_entry *plt;
>
> /*
> * On kernels that support module PLTs, the offset between the
> @@ -105,7 +106,13 @@ int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
> if (WARN_ON(!mod))
> return -EINVAL;
>
> - addr = (unsigned long)mod->arch.ftrace_trampoline;
> + plt = get_ftrace_plt(mod, addr);
> + if (!plt) {
> + pr_err("ftrace: no module PLT for %ps\n", (void *)addr);
> + return -EINVAL;
> + }
> +
> + addr = (unsigned long)plt;
> #else /* CONFIG_ARM64_MODULE_PLTS */
> return -EINVAL;
> #endif /* CONFIG_ARM64_MODULE_PLTS */
> @@ -117,6 +124,55 @@ int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
> return ftrace_modify_code(pc, old, new, true);
> }
>
> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
> +int ftrace_modify_call(struct dyn_ftrace *rec, unsigned long old_addr,
> + unsigned long addr)
> +{
> + unsigned long pc = rec->ip;
> + u32 old, new;
> +
> + old = aarch64_insn_gen_branch_imm(pc, old_addr,
> + AARCH64_INSN_BRANCH_LINK);
> + new = aarch64_insn_gen_branch_imm(pc, addr, AARCH64_INSN_BRANCH_LINK);
> +
> + return ftrace_modify_code(pc, old, new, true);
> +}
> +
> +/*
> + * The compiler has inserted two NOPs before the regular function prologue.
> + * All instrumented functions follow the AAPCS, so x0-x8 and x19-x30 are live,
> + * and x9-x18 are free for our use.
> + *
> + * At runtime we want to be able to swing a single NOP <-> BL to enable or
> + * disable the ftrace call. The BL requires us to save the original LR value,
> + * so here we insert a <MOV X9, LR> over the first NOP so the instructions
> + * before the regular prologue are:
> + *
> + * | Compiled | Disabled | Enabled |
> + * +----------+------------+------------+
> + * | NOP | MOV X9, LR | MOV X9, LR |
> + * | NOP | NOP | BL <entry> |
> + *
> + * The LR value will be recovered by ftrace_regs_entry, and restored into LR
> + * before returning to the regular function prologue. When a function is not
> + * being traced, the MOV is not harmful given x9 is not live per the AAPCS.
> + *
> + * Note: ftrace_process_locs() has pre-adjusted rec->ip to be the address of
> + * the BL.
> + */
> +int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec)
> +{
> + unsigned long pc = rec->ip - AARCH64_INSN_SIZE;
> + u32 old, new;
> +
> + old = aarch64_insn_gen_nop();
> + new = aarch64_insn_gen_move_reg(AARCH64_INSN_REG_9,
> + AARCH64_INSN_REG_LR,
> + AARCH64_INSN_VARIANT_64BIT);
> + return ftrace_modify_code(pc, old, new, true);
> +}
> +#endif
> +
> /*
> * Turn off the call to ftrace_caller() in instrumented function
> */
> diff --git a/arch/arm64/kernel/module-plts.c b/arch/arm64/kernel/module-plts.c
> index b182442b87a3..65b08a74aec6 100644
> --- a/arch/arm64/kernel/module-plts.c
> +++ b/arch/arm64/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>
> @@ -330,7 +331,7 @@ int module_frob_arch_sections(Elf_Ehdr *ehdr, Elf_Shdr *sechdrs,
> tramp->sh_type = SHT_NOBITS;
> tramp->sh_flags = SHF_EXECINSTR | SHF_ALLOC;
> tramp->sh_addralign = __alignof__(struct plt_entry);
> - tramp->sh_size = sizeof(struct plt_entry);
> + tramp->sh_size = NR_FTRACE_PLTS * sizeof(struct plt_entry);
> }
>
> return 0;
> diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c
> index 5f5bc3b94da7..00d21a420c60 100644
> --- a/arch/arm64/kernel/module.c
> +++ b/arch/arm64/kernel/module.c
> @@ -486,21 +486,31 @@ static const Elf_Shdr *find_section(const Elf_Ehdr *hdr,
> return NULL;
> }
>
> +static inline void __init_plt(struct plt_entry *plt, unsigned long addr)
> +{
> + *plt = get_plt_entry(addr, plt);
> +}
> +
> int module_init_ftrace_plt(const Elf_Ehdr *hdr,
> const Elf_Shdr *sechdrs,
> struct module *mod)
> {
> #if defined(CONFIG_ARM64_MODULE_PLTS) && defined(CONFIG_DYNAMIC_FTRACE)
> const Elf_Shdr *s;
> - struct plt_entry *plt;
> + struct plt_entry *plts;
>
> s = find_section(hdr, sechdrs, ".text.ftrace_trampoline");
> if (!s)
> return -ENOEXEC;
>
> - plt = (void *)s->sh_addr;
> - *plt = get_plt_entry(FTRACE_ADDR, plt);
> - mod->arch.ftrace_trampoline = plt;
> + plts = (void *)s->sh_addr;
> +
> + __init_plt(&plts[FTRACE_PLT_IDX], FTRACE_ADDR);
> +
> + if (IS_ENABLED(CONFIG_DYNAMIC_FTRACE_WITH_REGS))
> + __init_plt(&plts[FTRACE_REGS_PLT_IDX], FTRACE_REGS_ADDR);
> +
> + mod->arch.ftrace_trampolines = plts;
> #endif
> return 0;
> }
>
On Sat, Nov 02, 2019 at 05:42:25PM +0530, Amit Daniel Kachhap wrote:
> On 10/29/19 10:28 PM, Mark Rutland wrote:
> > This series is a reworked version of Torsten's FTRACE_WITH_REGS series
> > [1]. I've tried to rework the existing code in preparatory patches so
> > that the patchable-function-entry bits slot in with fewer surprises.
> > This version is based on v5.4-rc3, and can be found in my
> > arm64/ftrace-with-regs branch [2].
> >
> > Patch 1 adds an (optional) ftrace_init_nop(), which the core code uses
> > to initialize callsites. This allows us to avoid a synthetic MCOUNT_ADDR
> > symbol, and more cleanly separates the one-time initialization of the
> > callsite from dynamic NOP<->CALL modification. Architectures which don't
> > implement this get the existing ftrace_make_nop() with MCOUNT_ADDR.
> >
> > Recently parisc gained ftrace support using patchable-function-entry.
> > Patch 2 makes the handling of module callsite locations common in
> > kernel/module.c with a new FTRACE_CALLSITE_SECTION definition, and
> > removed the newly redundant bits from arch/parisc.
> >
> > Patches 3 and 4 move the module PLT initialization to module load time,
> > which simplifies runtime callsite modification. This also means that we
> > don't transitently mark the module text RW, and will allow for the
> > removal of module_disable_ro().
> >
> > Patches 5 and 6 add some trivial infrastructure, with patch 7 finally
> > adding FTRACE_WITH_REGS support. Additional work will be required for
> > livepatching (e.g. implementing reliable stack trace), which is
> > commented as part of patch 7.
> >
> > Patch 8 is a trivial cleanup atop of the rest of the series, making the
> > code easier to read and less susceptible to config-specific breakage.
> I tested the whole series with my latest in-kernel ptrauth patches [1]
> and graph_tracer/function_graph_tracer works fine, So for the whole series,
> Tested-by: Amit Daniel Kachhap <[email protected]>
>
> Also I gave few minor comments in the individual patches. With those
> comments,
> Signed-off-by: Amit Daniel Kachhap <[email protected]>
I don't think this means what you think it means. Please read:
Documentation/process/submitting-patches.rst
Will
On 11/4/19 6:26 PM, Will Deacon wrote:
> On Sat, Nov 02, 2019 at 05:42:25PM +0530, Amit Daniel Kachhap wrote:
>> On 10/29/19 10:28 PM, Mark Rutland wrote:
>>> This series is a reworked version of Torsten's FTRACE_WITH_REGS series
>>> [1]. I've tried to rework the existing code in preparatory patches so
>>> that the patchable-function-entry bits slot in with fewer surprises.
>>> This version is based on v5.4-rc3, and can be found in my
>>> arm64/ftrace-with-regs branch [2].
>>>
>>> Patch 1 adds an (optional) ftrace_init_nop(), which the core code uses
>>> to initialize callsites. This allows us to avoid a synthetic MCOUNT_ADDR
>>> symbol, and more cleanly separates the one-time initialization of the
>>> callsite from dynamic NOP<->CALL modification. Architectures which don't
>>> implement this get the existing ftrace_make_nop() with MCOUNT_ADDR.
>>>
>>> Recently parisc gained ftrace support using patchable-function-entry.
>>> Patch 2 makes the handling of module callsite locations common in
>>> kernel/module.c with a new FTRACE_CALLSITE_SECTION definition, and
>>> removed the newly redundant bits from arch/parisc.
>>>
>>> Patches 3 and 4 move the module PLT initialization to module load time,
>>> which simplifies runtime callsite modification. This also means that we
>>> don't transitently mark the module text RW, and will allow for the
>>> removal of module_disable_ro().
>>>
>>> Patches 5 and 6 add some trivial infrastructure, with patch 7 finally
>>> adding FTRACE_WITH_REGS support. Additional work will be required for
>>> livepatching (e.g. implementing reliable stack trace), which is
>>> commented as part of patch 7.
>>>
>>> Patch 8 is a trivial cleanup atop of the rest of the series, making the
>>> code easier to read and less susceptible to config-specific breakage.
>> I tested the whole series with my latest in-kernel ptrauth patches [1]
>> and graph_tracer/function_graph_tracer works fine, So for the whole series,
>> Tested-by: Amit Daniel Kachhap <[email protected]>
>>
>> Also I gave few minor comments in the individual patches. With those
>> comments,
>> Signed-off-by: Amit Daniel Kachhap <[email protected]>
Oops sorry I meant,
Reviewed-off-by: Amit Daniel Kachhap <[email protected]>
>
> I don't think this means what you think it means. Please read:
Thanks for pointing it.
>
> Documentation/process/submitting-patches.rst
>
> Will
>
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
On Sat, 2 Nov 2019 17:49:00 +0530
Amit Daniel Kachhap <[email protected]> wrote:
> Now that ftrace_init_nop is also an arch implemented function so it may
> be added in Documentation/trace/ftrace-design.rst along with
> ftrace_make_nop.
> In general also, adding some description about patchable-function-entry
> in kernel Documentation will be useful.
I think this part is outside the scope of this patch set. Honestly, I
need to chisel out some time to rewrite the ftrace-design document, as
that's been long needed. But that can come at a later time. I'm
currently rewriting some of it now, so it will be best to not waste
effort to update a document that will soon become stale. ;-)
-- Steve
On Tue, 29 Oct 2019 16:58:25 +0000
Mark Rutland <[email protected]> wrote:
> Architectures may need to perform special initialization of ftrace
> callsites, and today they do so by special-casing ftrace_make_nop() when
> the expected branch address is MCOUNT_ADDR. In some cases (e.g. for
> patchable-function-entry), we don't have an mcount-like symbol and don't
> want a synthetic MCOUNT_ADDR, but we may need to perform some
> initialization of callsites.
>
> To make it possible to separate initialization from runtime
> modification, and to handle cases without an mcount-like symbol, this
> patch adds an optional ftrace_init_nop() function that architectures can
> implement, which does not pass a branch address.
>
> Where an architecture does not provide ftrace_init_nop(), we will fall
> back to the existing behaviour of calling ftrace_make_nop() with
> MCOUNT_ADDR.
>
> At the same time, ftrace_code_disable() is renamed to
> ftrace_nop_initialize() to make it clearer that it is intended to
> intialize a callsite into a disabled state, and is not for disabling a
> callsite that has been runtime enabled. The kerneldoc description of rec
> arguments is updated to cover non-mcount callsites.
>
> Signed-off-by: Mark Rutland <[email protected]>
> Reviewed-by: Ard Biesheuvel <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: Steven Rostedt <[email protected]>
Reviewed-by: Steven Rostedt (VMware) <[email protected]>
-- Steve
> Cc: Torsten Duwe <[email protected]>
> ---
> include/linux/ftrace.h | 35 ++++++++++++++++++++++++++++++++---
> kernel/trace/ftrace.c | 6 +++---
> 2 files changed, 35 insertions(+), 6 deletions(-)
>
> diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> index 8a8cb3c401b2..9867d90d635e 100644
> --- a/include/linux/ftrace.h
> +++ b/include/linux/ftrace.h
> @@ -499,7 +499,7 @@ static inline int ftrace_disable_ftrace_graph_caller(void) { return 0; }
> /**
> * ftrace_make_nop - convert code into nop
> * @mod: module structure if called by module load initialization
> - * @rec: the mcount call site record
> + * @rec: the call site record (e.g. mcount/fentry)
> * @addr: the address that the call site should be calling
> *
> * This is a very sensitive operation and great care needs
> @@ -520,9 +520,38 @@ static inline int ftrace_disable_ftrace_graph_caller(void) { return 0; }
> extern int ftrace_make_nop(struct module *mod,
> struct dyn_ftrace *rec, unsigned long addr);
>
> +
> +/**
> + * ftrace_init_nop - initialize a nop call site
> + * @mod: module structure if called by module load initialization
> + * @rec: the call site record (e.g. mcount/fentry)
> + *
> + * This is a very sensitive operation and great care needs
> + * to be taken by the arch. The operation should carefully
> + * read the location, check to see if what is read is indeed
> + * what we expect it to be, and then on success of the compare,
> + * it should write to the location.
> + *
> + * The code segment at @rec->ip should contain the contents created by
> + * the compiler
> + *
> + * Return must be:
> + * 0 on success
> + * -EFAULT on error reading the location
> + * -EINVAL on a failed compare of the contents
> + * -EPERM on error writing to the location
> + * Any other value will be considered a failure.
> + */
> +#ifndef ftrace_init_nop
> +static inline int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec)
> +{
> + return ftrace_make_nop(mod, rec, MCOUNT_ADDR);
> +}
> +#endif
> +
> /**
> * ftrace_make_call - convert a nop call site into a call to addr
> - * @rec: the mcount call site record
> + * @rec: the call site record (e.g. mcount/fentry)
> * @addr: the address that the call site should call
> *
> * This is a very sensitive operation and great care needs
> @@ -545,7 +574,7 @@ extern int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr);
> #ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
> /**
> * ftrace_modify_call - convert from one addr to another (no nop)
> - * @rec: the mcount call site record
> + * @rec: the call site record (e.g. mcount/fentry)
> * @old_addr: the address expected to be currently called to
> * @addr: the address to change to
> *
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index f296d89be757..5259d4dea675 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -2494,14 +2494,14 @@ struct dyn_ftrace *ftrace_rec_iter_record(struct ftrace_rec_iter *iter)
> }
>
> static int
> -ftrace_code_disable(struct module *mod, struct dyn_ftrace *rec)
> +ftrace_nop_initialize(struct module *mod, struct dyn_ftrace *rec)
> {
> int ret;
>
> if (unlikely(ftrace_disabled))
> return 0;
>
> - ret = ftrace_make_nop(mod, rec, MCOUNT_ADDR);
> + ret = ftrace_init_nop(mod, rec);
> if (ret) {
> ftrace_bug_type = FTRACE_BUG_INIT;
> ftrace_bug(ret, rec);
> @@ -2943,7 +2943,7 @@ static int ftrace_update_code(struct module *mod, struct ftrace_page *new_pgs)
> * to the NOP instructions.
> */
> if (!__is_defined(CC_USING_NOP_MCOUNT) &&
> - !ftrace_code_disable(mod, p))
> + !ftrace_nop_initialize(mod, p))
> break;
>
> update_cnt++;
On Tue, 29 Oct 2019 16:58:26 +0000
Mark Rutland <[email protected]> wrote:
> When using patchable-function-entry, the compiler will record the
> callsites into a section named "__patchable_function_entries" rather
> than "__mcount_loc". Let's abstract this difference behind a new
> FTRACE_CALLSITE_SECTION, so that architectures don't have to handle this
> explicitly (e.g. with custom module linker scripts).
>
> As parisc currently handles this explicitly, it is fixed up accordingly,
> with its custom linker script removed. Since FTRACE_CALLSITE_SECTION is
> only defined when DYNAMIC_FTRACE is selected, the parisc module loading
> code is updated to only use the definition in that case. When
> DYNAMIC_FTRACE is not selected, modules shouldn't have this section, so
> this removes some redundant work in that case.
>
> I built parisc generic-{32,64}bit_defconfig with DYNAMIC_FTRACE enabled,
> and verified that the section made it into the .ko files for modules.
>
> Signed-off-by: Mark Rutland <[email protected]>
> Reviewed-by: Ard Biesheuvel <[email protected]>
> Cc: Helge Deller <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: James E.J. Bottomley <[email protected]>
> Cc: Jessica Yu <[email protected]>
> Cc: Steven Rostedt <[email protected]>
> Cc: Sven Schnelle <[email protected]>
> Cc: [email protected]
>
Acked-by: Steven Rostedt (VMware) <[email protected]>
-- Steve
On Thu, 31 Oct 2019 13:00:22 +0000
Mark Rutland <[email protected]> wrote:
> Sure. I've folded the above into this patch, and pushed out an updated branch:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git/log/?h=arm64/ftrace-with-regs
Just to keep this change in lore, can you at a minimum reply to this
patch's thread with the new update?
Thanks!
-- Steve
On Sat, Nov 02, 2019 at 05:49:00PM +0530, Amit Daniel Kachhap wrote:
> Hi Mark,
Hi Amit,
Thanks for the review!
> On 10/29/19 10:28 PM, Mark Rutland wrote:
> > Architectures may need to perform special initialization of ftrace
> > callsites, and today they do so by special-casing ftrace_make_nop() when
> > the expected branch address is MCOUNT_ADDR. In some cases (e.g. for
> > patchable-function-entry), we don't have an mcount-like symbol and don't
> s/an mcount/a mcount.
> > want a synthetic MCOUNT_ADDR, but we may need to perform some
> > initialization of callsites.
> >
> > To make it possible to separate initialization from runtime
> > modification, and to handle cases without an mcount-like symbol, this
> Same as above.
Using 'an' in both of these cases is correct, even though 'mcount'
starts with a consonant, since it's pronounced as-if it were 'emcount'.
I will leave this as-is.
> > patch adds an optional ftrace_init_nop() function that architectures can
> > implement, which does not pass a branch address.
> >
> > Where an architecture does not provide ftrace_init_nop(), we will fall
> > back to the existing behaviour of calling ftrace_make_nop() with
> > MCOUNT_ADDR.
> >
> > At the same time, ftrace_code_disable() is renamed to
> > ftrace_nop_initialize() to make it clearer that it is intended to
> > intialize a callsite into a disabled state, and is not for disabling a
> > callsite that has been runtime enabled. The kerneldoc description of rec
> > arguments is updated to cover non-mcount callsites.
> >
> > Signed-off-by: Mark Rutland <[email protected]>
> > Reviewed-by: Ard Biesheuvel <[email protected]>
> > Cc: Ingo Molnar <[email protected]>
> > Cc: Steven Rostedt <[email protected]>
> > Cc: Torsten Duwe <[email protected]>
> > ---
> > include/linux/ftrace.h | 35 ++++++++++++++++++++++++++++++++---
> > kernel/trace/ftrace.c | 6 +++---
> > 2 files changed, 35 insertions(+), 6 deletions(-)
> >
> > diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> > index 8a8cb3c401b2..9867d90d635e 100644
> > --- a/include/linux/ftrace.h
> > +++ b/include/linux/ftrace.h
> > @@ -499,7 +499,7 @@ static inline int ftrace_disable_ftrace_graph_caller(void) { return 0; }
> > /**
> > * ftrace_make_nop - convert code into nop
> > * @mod: module structure if called by module load initialization
> > - * @rec: the mcount call site record
> > + * @rec: the call site record (e.g. mcount/fentry)
> > * @addr: the address that the call site should be calling
> > *
> > * This is a very sensitive operation and great care needs
> > @@ -520,9 +520,38 @@ static inline int ftrace_disable_ftrace_graph_caller(void) { return 0; }
> > extern int ftrace_make_nop(struct module *mod,
> > struct dyn_ftrace *rec, unsigned long addr);
> > +
> > +/**
> > + * ftrace_init_nop - initialize a nop call site
> > + * @mod: module structure if called by module load initialization
> > + * @rec: the call site record (e.g. mcount/fentry)
> > + *
> > + * This is a very sensitive operation and great care needs
> > + * to be taken by the arch. The operation should carefully
> > + * read the location, check to see if what is read is indeed
> > + * what we expect it to be, and then on success of the compare,
> > + * it should write to the location.
> > + *
> > + * The code segment at @rec->ip should contain the contents created by
> > + * the compiler
> Nit: Will it be better to write it as "@rec->ip should store the adjusted
> ftrace entry address of the call site" or something like that.
This was the specific wording requested by Steve, and it's trying to
describe the instructions at rec->ip, rather than the value of rec->ip,
so I think it's better to leave this as-is.
> > + *
> > + * Return must be:
> > + * 0 on success
> > + * -EFAULT on error reading the location
> > + * -EINVAL on a failed compare of the contents
> > + * -EPERM on error writing to the location
> > + * Any other value will be considered a failure.
> > + */
> > +#ifndef ftrace_init_nop
> > +static inline int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec)
> > +{
> > + return ftrace_make_nop(mod, rec, MCOUNT_ADDR);
> > +}
> > +#endif
> > +
> Now that ftrace_init_nop is also an arch implemented function so it may be
> added in Documentation/trace/ftrace-design.rst along with ftrace_make_nop.
> In general also, adding some description about patchable-function-entry
> in kernel Documentation will be useful.
I agree that would be a good thing, but as Steve suggests I will leave
this for subsequent rework, as I think that also implies more
rework/renaming in the core code (e.g. to more cleanly separate the
notion of a callsite from mcount specifically).
Steve, if you'd like help with that (or review), I'd be happy to help.
Thanks,
Mark.
On Mon, Nov 04, 2019 at 08:16:20AM -0500, Steven Rostedt wrote:
> On Tue, 29 Oct 2019 16:58:25 +0000
> Mark Rutland <[email protected]> wrote:
>
> > Architectures may need to perform special initialization of ftrace
> > callsites, and today they do so by special-casing ftrace_make_nop() when
> > the expected branch address is MCOUNT_ADDR. In some cases (e.g. for
> > patchable-function-entry), we don't have an mcount-like symbol and don't
> > want a synthetic MCOUNT_ADDR, but we may need to perform some
> > initialization of callsites.
> >
> > To make it possible to separate initialization from runtime
> > modification, and to handle cases without an mcount-like symbol, this
> > patch adds an optional ftrace_init_nop() function that architectures can
> > implement, which does not pass a branch address.
> >
> > Where an architecture does not provide ftrace_init_nop(), we will fall
> > back to the existing behaviour of calling ftrace_make_nop() with
> > MCOUNT_ADDR.
> >
> > At the same time, ftrace_code_disable() is renamed to
> > ftrace_nop_initialize() to make it clearer that it is intended to
> > intialize a callsite into a disabled state, and is not for disabling a
> > callsite that has been runtime enabled. The kerneldoc description of rec
> > arguments is updated to cover non-mcount callsites.
> >
> > Signed-off-by: Mark Rutland <[email protected]>
> > Reviewed-by: Ard Biesheuvel <[email protected]>
> > Cc: Ingo Molnar <[email protected]>
> > Cc: Steven Rostedt <[email protected]>
>
> Reviewed-by: Steven Rostedt (VMware) <[email protected]>
Thanks!
Just to check, are you happy if this were to go via the arm64 tree with
the rest of the patches?
Mark.
On Sat, Nov 02, 2019 at 05:51:46PM +0530, Amit Daniel Kachhap wrote:
> On 10/29/19 10:28 PM, Mark Rutland wrote:
> > +/*
> > + * Due to -fpatchable-function-entry=2, the compiler has placed two NOPs before
> > + * the regular function prologue. For an enabled callsite, ftrace_init_nop() and
> > + * ftrace_make_call() have patched those NOPs to:
> > + *
> > + * MOV X9, LR
> > + * BL <entry>
> > + *
> > + * ... where <entry> is either ftrace_caller or ftrace_regs_caller.
> > + *
> > + * Each instrumented function follows the AAPCS, so here x0-x8 and x19-x30 are
> > + * live, and x9-x18 are safe to clobber.
> > + *
> > + * We save the callsite's context into a pt_regs before invoking and ftrace
> s/invoking and ftrace callbacks/invoking the ftrace callbacks
Whoops, that was meant to be 'any'. I've fixed that up locally.
Thanks,
Mark.
On Sat, Nov 02, 2019 at 05:50:02PM +0530, Amit Daniel Kachhap wrote:
> On 10/29/19 10:28 PM, Mark Rutland wrote:
> > @@ -485,24 +486,33 @@ static const Elf_Shdr *find_section(const Elf_Ehdr *hdr,
> > return NULL;
> > }
> > +int module_init_ftrace_plt(const Elf_Ehdr *hdr,
> > + const Elf_Shdr *sechdrs,
> > + struct module *mod)
> I think this function can be made static as it is not used anywhere.
It's only called by module_finalize() below, so making it static makese
sense; done.
Thanks
Mark.
> > +{
> > +#if defined(CONFIG_ARM64_MODULE_PLTS) && defined(CONFIG_DYNAMIC_FTRACE)
> > + const Elf_Shdr *s;
> > + struct plt_entry *plt;
> > +
> > + s = find_section(hdr, sechdrs, ".text.ftrace_trampoline");
> > + if (!s)
> > + return -ENOEXEC;
> > +
> > + plt = (void *)s->sh_addr;
> > + *plt = get_plt_entry(FTRACE_ADDR, plt);
> > + mod->arch.ftrace_trampoline = plt;
> > +#endif
> > + return 0;
> > +}
> > +
> > int module_finalize(const Elf_Ehdr *hdr,
> > const Elf_Shdr *sechdrs,
> > struct module *me)
> > {
> > const Elf_Shdr *s;
> > -
> > s = find_section(hdr, sechdrs, ".altinstructions");
> > if (s)
> > apply_alternatives_module((void *)s->sh_addr, s->sh_size);
> > -#ifdef CONFIG_ARM64_MODULE_PLTS
> > - if (IS_ENABLED(CONFIG_DYNAMIC_FTRACE)) {
> > - s = find_section(hdr, sechdrs, ".text.ftrace_trampoline");
> > - if (!s)
> > - return -ENOEXEC;
> > - me->arch.ftrace_trampoline = (void *)s->sh_addr;
> > - }
> > -#endif
> > -
> > - return 0;
> > + return module_init_ftrace_plt(hdr, sechdrs, me);
> > }
> >
On Mon, 4 Nov 2019 13:38:36 +0000
Mark Rutland <[email protected]> wrote:
> > Reviewed-by: Steven Rostedt (VMware) <[email protected]>
>
> Thanks!
>
> Just to check, are you happy if this were to go via the arm64 tree with
> the rest of the patches?
Yes, if I wasn't I would have said something. But I guess I should have
been explicit and stated that I'm fine with it going in your tree. My
current updates should not conflict with this.
-- Steve
On Mon, Nov 04, 2019 at 08:28:10AM -0500, Steven Rostedt wrote:
> On Thu, 31 Oct 2019 13:00:22 +0000
> Mark Rutland <[email protected]> wrote:
>
> > Sure. I've folded the above into this patch, and pushed out an updated branch:
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git/log/?h=arm64/ftrace-with-regs
>
> Just to keep this change in lore, can you at a minimum reply to this
> patch's thread with the new update?
The new change is below (with all else unchanged). I can send a v3 of
the series if you want the whole patch?
Thanks,
Mark.
diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index dae64600ccbf..a9c4e4721434 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -110,17 +110,17 @@
#endif
#ifdef CONFIG_FTRACE_MCOUNT_RECORD
-#ifdef CC_USING_PATCHABLE_FUNCTION_ENTRY
-#define MCOUNT_REC() . = ALIGN(8); \
- __start_mcount_loc = .; \
- KEEP(*(__patchable_function_entries)) \
- __stop_mcount_loc = .;
-#else
+/*
+ * The ftrace call sites are logged to a section whose name depends on the
+ * compiler option used. A given kernel image will only use one, AKA
+ * FTRACE_CALLSITE_SECTION. We capture all of them here to avoid header
+ * dependencies for FTRACE_CALLSITE_SECTION's definition.
+ */
#define MCOUNT_REC() . = ALIGN(8); \
__start_mcount_loc = .; \
KEEP(*(__mcount_loc)) \
+ KEEP(*(__patchable_function_entries)) \
__stop_mcount_loc = .;
-#endif
#else
#define MCOUNT_REC()
#endif
On Mon, Nov 04, 2019 at 01:03:51PM +0000, Amit Kachhap wrote:
>
>
> On 11/4/19 6:26 PM, Will Deacon wrote:
> > On Sat, Nov 02, 2019 at 05:42:25PM +0530, Amit Daniel Kachhap wrote:
> >> On 10/29/19 10:28 PM, Mark Rutland wrote:
> >>> This series is a reworked version of Torsten's FTRACE_WITH_REGS series
> >>> [1]. I've tried to rework the existing code in preparatory patches so
> >>> that the patchable-function-entry bits slot in with fewer surprises.
> >>> This version is based on v5.4-rc3, and can be found in my
> >>> arm64/ftrace-with-regs branch [2].
> >>>
> >>> Patch 1 adds an (optional) ftrace_init_nop(), which the core code uses
> >>> to initialize callsites. This allows us to avoid a synthetic MCOUNT_ADDR
> >>> symbol, and more cleanly separates the one-time initialization of the
> >>> callsite from dynamic NOP<->CALL modification. Architectures which don't
> >>> implement this get the existing ftrace_make_nop() with MCOUNT_ADDR.
> >>>
> >>> Recently parisc gained ftrace support using patchable-function-entry.
> >>> Patch 2 makes the handling of module callsite locations common in
> >>> kernel/module.c with a new FTRACE_CALLSITE_SECTION definition, and
> >>> removed the newly redundant bits from arch/parisc.
> >>>
> >>> Patches 3 and 4 move the module PLT initialization to module load time,
> >>> which simplifies runtime callsite modification. This also means that we
> >>> don't transitently mark the module text RW, and will allow for the
> >>> removal of module_disable_ro().
> >>>
> >>> Patches 5 and 6 add some trivial infrastructure, with patch 7 finally
> >>> adding FTRACE_WITH_REGS support. Additional work will be required for
> >>> livepatching (e.g. implementing reliable stack trace), which is
> >>> commented as part of patch 7.
> >>>
> >>> Patch 8 is a trivial cleanup atop of the rest of the series, making the
> >>> code easier to read and less susceptible to config-specific breakage.
> >> I tested the whole series with my latest in-kernel ptrauth patches [1]
> >> and graph_tracer/function_graph_tracer works fine, So for the whole series,
> >> Tested-by: Amit Daniel Kachhap <[email protected]>
> >>
> >> Also I gave few minor comments in the individual patches. With those
> >> comments,
> >> Signed-off-by: Amit Daniel Kachhap <[email protected]>
> Oops sorry I meant,
> Reviewed-off-by: Amit Daniel Kachhap <[email protected]>
Thanks!
I've added the Tested-by for the whole series, and the Reviewed-by for
patches 4 and 7. I haven't added it for patch 1 just yet; please reply
to my comment there if you'd still like to add a Reviewed-by.
Mark.
Hi Jessica, Helge,
Are you ok with the module and parisc changes, repectively?
The kbuild test robot is happy building this for multiple architectures,
Sven has tested that this works correctly on parisc, and others have
tested other architectures.
I'd like to queue this in the arm64 tree soon if possible.
Thanks,
Mark.
On Tue, Oct 29, 2019 at 04:58:26PM +0000, Mark Rutland wrote:
> When using patchable-function-entry, the compiler will record the
> callsites into a section named "__patchable_function_entries" rather
> than "__mcount_loc". Let's abstract this difference behind a new
> FTRACE_CALLSITE_SECTION, so that architectures don't have to handle this
> explicitly (e.g. with custom module linker scripts).
>
> As parisc currently handles this explicitly, it is fixed up accordingly,
> with its custom linker script removed. Since FTRACE_CALLSITE_SECTION is
> only defined when DYNAMIC_FTRACE is selected, the parisc module loading
> code is updated to only use the definition in that case. When
> DYNAMIC_FTRACE is not selected, modules shouldn't have this section, so
> this removes some redundant work in that case.
>
> I built parisc generic-{32,64}bit_defconfig with DYNAMIC_FTRACE enabled,
> and verified that the section made it into the .ko files for modules.
>
> Signed-off-by: Mark Rutland <[email protected]>
> Reviewed-by: Ard Biesheuvel <[email protected]>
> Cc: Helge Deller <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: James E.J. Bottomley <[email protected]>
> Cc: Jessica Yu <[email protected]>
> Cc: Steven Rostedt <[email protected]>
> Cc: Sven Schnelle <[email protected]>
> Cc: [email protected]
> ---
> arch/parisc/Makefile | 1 -
> arch/parisc/kernel/module.c | 10 +++++++---
> arch/parisc/kernel/module.lds | 7 -------
> include/linux/ftrace.h | 5 +++++
> kernel/module.c | 2 +-
> 5 files changed, 13 insertions(+), 12 deletions(-)
> delete mode 100644 arch/parisc/kernel/module.lds
>
> diff --git a/arch/parisc/Makefile b/arch/parisc/Makefile
> index 36b834f1c933..dca8f2de8cf5 100644
> --- a/arch/parisc/Makefile
> +++ b/arch/parisc/Makefile
> @@ -60,7 +60,6 @@ KBUILD_CFLAGS += -DCC_USING_PATCHABLE_FUNCTION_ENTRY=1 \
> -DFTRACE_PATCHABLE_FUNCTION_SIZE=$(NOP_COUNT)
>
> CC_FLAGS_FTRACE := -fpatchable-function-entry=$(NOP_COUNT),$(shell echo $$(($(NOP_COUNT)-1)))
> -KBUILD_LDS_MODULE += $(srctree)/arch/parisc/kernel/module.lds
> endif
>
> OBJCOPY_FLAGS =-O binary -R .note -R .comment -S
> diff --git a/arch/parisc/kernel/module.c b/arch/parisc/kernel/module.c
> index ac5f34993b53..1c50093e2ebe 100644
> --- a/arch/parisc/kernel/module.c
> +++ b/arch/parisc/kernel/module.c
> @@ -43,6 +43,7 @@
> #include <linux/elf.h>
> #include <linux/vmalloc.h>
> #include <linux/fs.h>
> +#include <linux/ftrace.h>
> #include <linux/string.h>
> #include <linux/kernel.h>
> #include <linux/bug.h>
> @@ -862,7 +863,7 @@ int module_finalize(const Elf_Ehdr *hdr,
> const char *strtab = NULL;
> const Elf_Shdr *s;
> char *secstrings;
> - int err, symindex = -1;
> + int symindex = -1;
> Elf_Sym *newptr, *oldptr;
> Elf_Shdr *symhdr = NULL;
> #ifdef DEBUG
> @@ -946,11 +947,13 @@ int module_finalize(const Elf_Ehdr *hdr,
> /* patch .altinstructions */
> apply_alternatives(aseg, aseg + s->sh_size, me->name);
>
> +#ifdef CONFIG_DYNAMIC_FTRACE
> /* For 32 bit kernels we're compiling modules with
> * -ffunction-sections so we must relocate the addresses in the
> - *__mcount_loc section.
> + * ftrace callsite section.
> */
> - if (symindex != -1 && !strcmp(secname, "__mcount_loc")) {
> + if (symindex != -1 && !strcmp(secname, FTRACE_CALLSITE_SECTION)) {
> + int err;
> if (s->sh_type == SHT_REL)
> err = apply_relocate((Elf_Shdr *)sechdrs,
> strtab, symindex,
> @@ -962,6 +965,7 @@ int module_finalize(const Elf_Ehdr *hdr,
> if (err)
> return err;
> }
> +#endif
> }
> return 0;
> }
> diff --git a/arch/parisc/kernel/module.lds b/arch/parisc/kernel/module.lds
> deleted file mode 100644
> index 1a9a92aca5c8..000000000000
> --- a/arch/parisc/kernel/module.lds
> +++ /dev/null
> @@ -1,7 +0,0 @@
> -/* SPDX-License-Identifier: GPL-2.0 */
> -
> -SECTIONS {
> - __mcount_loc : {
> - *(__patchable_function_entries)
> - }
> -}
> diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> index 9867d90d635e..9141f2263286 100644
> --- a/include/linux/ftrace.h
> +++ b/include/linux/ftrace.h
> @@ -738,6 +738,11 @@ static inline unsigned long get_lock_parent_ip(void)
>
> #ifdef CONFIG_FTRACE_MCOUNT_RECORD
> extern void ftrace_init(void);
> +#ifdef CC_USING_PATCHABLE_FUNCTION_ENTRY
> +#define FTRACE_CALLSITE_SECTION "__patchable_function_entries"
> +#else
> +#define FTRACE_CALLSITE_SECTION "__mcount_loc"
> +#endif
> #else
> static inline void ftrace_init(void) { }
> #endif
> diff --git a/kernel/module.c b/kernel/module.c
> index ff2d7359a418..acf7962936c4 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -3222,7 +3222,7 @@ static int find_module_sections(struct module *mod, struct load_info *info)
> #endif
> #ifdef CONFIG_FTRACE_MCOUNT_RECORD
> /* sechdrs[0].sh_size is always zero */
> - mod->ftrace_callsites = section_objs(info, "__mcount_loc",
> + mod->ftrace_callsites = section_objs(info, FTRACE_CALLSITE_SECTION,
> sizeof(*mod->ftrace_callsites),
> &mod->num_ftrace_callsites);
> #endif
> --
> 2.11.0
>
Hi Mark,
On 04.11.19 16:51, Mark Rutland wrote:
> Hi Jessica, Helge,
>
> Are you ok with the module and parisc changes, repectively?
Sure, please add my:
Acked-by: Helge Deller <[email protected]>
Helge
> The kbuild test robot is happy building this for multiple architectures,
> Sven has tested that this works correctly on parisc, and others have
> tested other architectures.
>
> I'd like to queue this in the arm64 tree soon if possible.
>
> Thanks,
> Mark.
>
> On Tue, Oct 29, 2019 at 04:58:26PM +0000, Mark Rutland wrote:
>> When using patchable-function-entry, the compiler will record the
>> callsites into a section named "__patchable_function_entries" rather
>> than "__mcount_loc". Let's abstract this difference behind a new
>> FTRACE_CALLSITE_SECTION, so that architectures don't have to handle this
>> explicitly (e.g. with custom module linker scripts).
>>
>> As parisc currently handles this explicitly, it is fixed up accordingly,
>> with its custom linker script removed. Since FTRACE_CALLSITE_SECTION is
>> only defined when DYNAMIC_FTRACE is selected, the parisc module loading
>> code is updated to only use the definition in that case. When
>> DYNAMIC_FTRACE is not selected, modules shouldn't have this section, so
>> this removes some redundant work in that case.
>>
>> I built parisc generic-{32,64}bit_defconfig with DYNAMIC_FTRACE enabled,
>> and verified that the section made it into the .ko files for modules.
>>
>> Signed-off-by: Mark Rutland <[email protected]>
>> Reviewed-by: Ard Biesheuvel <[email protected]>
>> Cc: Helge Deller <[email protected]>
>> Cc: Ingo Molnar <[email protected]>
>> Cc: James E.J. Bottomley <[email protected]>
>> Cc: Jessica Yu <[email protected]>
>> Cc: Steven Rostedt <[email protected]>
>> Cc: Sven Schnelle <[email protected]>
>> Cc: [email protected]
>> ---
>> arch/parisc/Makefile | 1 -
>> arch/parisc/kernel/module.c | 10 +++++++---
>> arch/parisc/kernel/module.lds | 7 -------
>> include/linux/ftrace.h | 5 +++++
>> kernel/module.c | 2 +-
>> 5 files changed, 13 insertions(+), 12 deletions(-)
>> delete mode 100644 arch/parisc/kernel/module.lds
>>
>> diff --git a/arch/parisc/Makefile b/arch/parisc/Makefile
>> index 36b834f1c933..dca8f2de8cf5 100644
>> --- a/arch/parisc/Makefile
>> +++ b/arch/parisc/Makefile
>> @@ -60,7 +60,6 @@ KBUILD_CFLAGS += -DCC_USING_PATCHABLE_FUNCTION_ENTRY=1 \
>> -DFTRACE_PATCHABLE_FUNCTION_SIZE=$(NOP_COUNT)
>>
>> CC_FLAGS_FTRACE := -fpatchable-function-entry=$(NOP_COUNT),$(shell echo $$(($(NOP_COUNT)-1)))
>> -KBUILD_LDS_MODULE += $(srctree)/arch/parisc/kernel/module.lds
>> endif
>>
>> OBJCOPY_FLAGS =-O binary -R .note -R .comment -S
>> diff --git a/arch/parisc/kernel/module.c b/arch/parisc/kernel/module.c
>> index ac5f34993b53..1c50093e2ebe 100644
>> --- a/arch/parisc/kernel/module.c
>> +++ b/arch/parisc/kernel/module.c
>> @@ -43,6 +43,7 @@
>> #include <linux/elf.h>
>> #include <linux/vmalloc.h>
>> #include <linux/fs.h>
>> +#include <linux/ftrace.h>
>> #include <linux/string.h>
>> #include <linux/kernel.h>
>> #include <linux/bug.h>
>> @@ -862,7 +863,7 @@ int module_finalize(const Elf_Ehdr *hdr,
>> const char *strtab = NULL;
>> const Elf_Shdr *s;
>> char *secstrings;
>> - int err, symindex = -1;
>> + int symindex = -1;
>> Elf_Sym *newptr, *oldptr;
>> Elf_Shdr *symhdr = NULL;
>> #ifdef DEBUG
>> @@ -946,11 +947,13 @@ int module_finalize(const Elf_Ehdr *hdr,
>> /* patch .altinstructions */
>> apply_alternatives(aseg, aseg + s->sh_size, me->name);
>>
>> +#ifdef CONFIG_DYNAMIC_FTRACE
>> /* For 32 bit kernels we're compiling modules with
>> * -ffunction-sections so we must relocate the addresses in the
>> - *__mcount_loc section.
>> + * ftrace callsite section.
>> */
>> - if (symindex != -1 && !strcmp(secname, "__mcount_loc")) {
>> + if (symindex != -1 && !strcmp(secname, FTRACE_CALLSITE_SECTION)) {
>> + int err;
>> if (s->sh_type == SHT_REL)
>> err = apply_relocate((Elf_Shdr *)sechdrs,
>> strtab, symindex,
>> @@ -962,6 +965,7 @@ int module_finalize(const Elf_Ehdr *hdr,
>> if (err)
>> return err;
>> }
>> +#endif
>> }
>> return 0;
>> }
>> diff --git a/arch/parisc/kernel/module.lds b/arch/parisc/kernel/module.lds
>> deleted file mode 100644
>> index 1a9a92aca5c8..000000000000
>> --- a/arch/parisc/kernel/module.lds
>> +++ /dev/null
>> @@ -1,7 +0,0 @@
>> -/* SPDX-License-Identifier: GPL-2.0 */
>> -
>> -SECTIONS {
>> - __mcount_loc : {
>> - *(__patchable_function_entries)
>> - }
>> -}
>> diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
>> index 9867d90d635e..9141f2263286 100644
>> --- a/include/linux/ftrace.h
>> +++ b/include/linux/ftrace.h
>> @@ -738,6 +738,11 @@ static inline unsigned long get_lock_parent_ip(void)
>>
>> #ifdef CONFIG_FTRACE_MCOUNT_RECORD
>> extern void ftrace_init(void);
>> +#ifdef CC_USING_PATCHABLE_FUNCTION_ENTRY
>> +#define FTRACE_CALLSITE_SECTION "__patchable_function_entries"
>> +#else
>> +#define FTRACE_CALLSITE_SECTION "__mcount_loc"
>> +#endif
>> #else
>> static inline void ftrace_init(void) { }
>> #endif
>> diff --git a/kernel/module.c b/kernel/module.c
>> index ff2d7359a418..acf7962936c4 100644
>> --- a/kernel/module.c
>> +++ b/kernel/module.c
>> @@ -3222,7 +3222,7 @@ static int find_module_sections(struct module *mod, struct load_info *info)
>> #endif
>> #ifdef CONFIG_FTRACE_MCOUNT_RECORD
>> /* sechdrs[0].sh_size is always zero */
>> - mod->ftrace_callsites = section_objs(info, "__mcount_loc",
>> + mod->ftrace_callsites = section_objs(info, FTRACE_CALLSITE_SECTION,
>> sizeof(*mod->ftrace_callsites),
>> &mod->num_ftrace_callsites);
>> #endif
>> --
>> 2.11.0
>>
On Tue, 29 Oct 2019, Mark Rutland wrote:
> When using patchable-function-entry, the compiler will record the
> callsites into a section named "__patchable_function_entries" rather
> than "__mcount_loc". Let's abstract this difference behind a new
> FTRACE_CALLSITE_SECTION, so that architectures don't have to handle this
> explicitly (e.g. with custom module linker scripts).
>
> As parisc currently handles this explicitly, it is fixed up accordingly,
> with its custom linker script removed. Since FTRACE_CALLSITE_SECTION is
> only defined when DYNAMIC_FTRACE is selected, the parisc module loading
> code is updated to only use the definition in that case. When
> DYNAMIC_FTRACE is not selected, modules shouldn't have this section, so
> this removes some redundant work in that case.
>
> I built parisc generic-{32,64}bit_defconfig with DYNAMIC_FTRACE enabled,
> and verified that the section made it into the .ko files for modules.
>
> Signed-off-by: Mark Rutland <[email protected]>
> Reviewed-by: Ard Biesheuvel <[email protected]>
> Cc: Helge Deller <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: James E.J. Bottomley <[email protected]>
> Cc: Jessica Yu <[email protected]>
> Cc: Steven Rostedt <[email protected]>
> Cc: Sven Schnelle <[email protected]>
> Cc: [email protected]
For the updated patch
Reviewed-by: Miroslav Benes <[email protected]>
M
On 11/4/19 6:41 PM, Steven Rostedt wrote:
> On Sat, 2 Nov 2019 17:49:00 +0530
> Amit Daniel Kachhap <[email protected]> wrote:
>
>> Now that ftrace_init_nop is also an arch implemented function so it may
>> be added in Documentation/trace/ftrace-design.rst along with
>> ftrace_make_nop.
>> In general also, adding some description about patchable-function-entry
>> in kernel Documentation will be useful.
>
> I think this part is outside the scope of this patch set. Honestly, I
> need to chisel out some time to rewrite the ftrace-design document, as
> that's been long needed. But that can come at a later time. I'm
> currently rewriting some of it now, so it will be best to not waste
> effort to update a document that will soon become stale. ;-)
Yes it makes sense.
Thanks,
Amit
>
> -- Steve
>
Hi Mark,
On 11/4/19 7:06 PM, Mark Rutland wrote:
> On Sat, Nov 02, 2019 at 05:49:00PM +0530, Amit Daniel Kachhap wrote:
>> Hi Mark,
>
> Hi Amit,
>
> Thanks for the review!
>
>> On 10/29/19 10:28 PM, Mark Rutland wrote:
>>> Architectures may need to perform special initialization of ftrace
>>> callsites, and today they do so by special-casing ftrace_make_nop() when
>>> the expected branch address is MCOUNT_ADDR. In some cases (e.g. for
>>> patchable-function-entry), we don't have an mcount-like symbol and don't
>> s/an mcount/a mcount.
>>> want a synthetic MCOUNT_ADDR, but we may need to perform some
>>> initialization of callsites.
>>>
>>> To make it possible to separate initialization from runtime
>>> modification, and to handle cases without an mcount-like symbol, this
>> Same as above.
>
> Using 'an' in both of these cases is correct, even though 'mcount'
> starts with a consonant, since it's pronounced as-if it were 'emcount'.
> I will leave this as-is.
Ok sure. It makes sense.
>
>>> patch adds an optional ftrace_init_nop() function that architectures can
>>> implement, which does not pass a branch address.
>>>
>>> Where an architecture does not provide ftrace_init_nop(), we will fall
>>> back to the existing behaviour of calling ftrace_make_nop() with
>>> MCOUNT_ADDR.
>>>
>>> At the same time, ftrace_code_disable() is renamed to
>>> ftrace_nop_initialize() to make it clearer that it is intended to
>>> intialize a callsite into a disabled state, and is not for disabling a
>>> callsite that has been runtime enabled. The kerneldoc description of rec
>>> arguments is updated to cover non-mcount callsites.
>>>
>>> Signed-off-by: Mark Rutland <[email protected]>
>>> Reviewed-by: Ard Biesheuvel <[email protected]>
>>> Cc: Ingo Molnar <[email protected]>
>>> Cc: Steven Rostedt <[email protected]>
>>> Cc: Torsten Duwe <[email protected]>
>>> ---
>>> include/linux/ftrace.h | 35 ++++++++++++++++++++++++++++++++---
>>> kernel/trace/ftrace.c | 6 +++---
>>> 2 files changed, 35 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
>>> index 8a8cb3c401b2..9867d90d635e 100644
>>> --- a/include/linux/ftrace.h
>>> +++ b/include/linux/ftrace.h
>>> @@ -499,7 +499,7 @@ static inline int ftrace_disable_ftrace_graph_caller(void) { return 0; }
>>> /**
>>> * ftrace_make_nop - convert code into nop
>>> * @mod: module structure if called by module load initialization
>>> - * @rec: the mcount call site record
>>> + * @rec: the call site record (e.g. mcount/fentry)
>>> * @addr: the address that the call site should be calling
>>> *
>>> * This is a very sensitive operation and great care needs
>>> @@ -520,9 +520,38 @@ static inline int ftrace_disable_ftrace_graph_caller(void) { return 0; }
>>> extern int ftrace_make_nop(struct module *mod,
>>> struct dyn_ftrace *rec, unsigned long addr);
>>> +
>>> +/**
>>> + * ftrace_init_nop - initialize a nop call site
>>> + * @mod: module structure if called by module load initialization
>>> + * @rec: the call site record (e.g. mcount/fentry)
>>> + *
>>> + * This is a very sensitive operation and great care needs
>>> + * to be taken by the arch. The operation should carefully
>>> + * read the location, check to see if what is read is indeed
>>> + * what we expect it to be, and then on success of the compare,
>>> + * it should write to the location.
>>> + *
>>> + * The code segment at @rec->ip should contain the contents created by
>>> + * the compiler
>> Nit: Will it be better to write it as "@rec->ip should store the adjusted
>> ftrace entry address of the call site" or something like that.
>
> This was the specific wording requested by Steve, and it's trying to
> describe the instructions at rec->ip, rather than the value of rec->ip,
> so I think it's better to leave this as-is.
ok Its fine this way too. Actually from the comment, I could not
understand which one of the compiler contents this points to as in this
case there are 2 nops.
>
>>> + *
>>> + * Return must be:
>>> + * 0 on success
>>> + * -EFAULT on error reading the location
>>> + * -EINVAL on a failed compare of the contents
>>> + * -EPERM on error writing to the location
>>> + * Any other value will be considered a failure.
>>> + */
>>> +#ifndef ftrace_init_nop
>>> +static inline int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec)
>>> +{
>>> + return ftrace_make_nop(mod, rec, MCOUNT_ADDR);
>>> +}
>>> +#endif
>>> +
>> Now that ftrace_init_nop is also an arch implemented function so it may be
>> added in Documentation/trace/ftrace-design.rst along with ftrace_make_nop.
>> In general also, adding some description about patchable-function-entry
>> in kernel Documentation will be useful.
>
> I agree that would be a good thing, but as Steve suggests I will leave
> this for subsequent rework, as I think that also implies more
> rework/renaming in the core code (e.g. to more cleanly separate the
> notion of a callsite from mcount specifically).
ok.
Thanks,
Amit
>
> Steve, if you'd like help with that (or review), I'd be happy to help.
>
> Thanks,
> Mark.
>
Hi,
On 11/4/19 7:34 PM, Mark Rutland wrote:
> On Mon, Nov 04, 2019 at 01:03:51PM +0000, Amit Kachhap wrote:
>>
>>
>> On 11/4/19 6:26 PM, Will Deacon wrote:
>>> On Sat, Nov 02, 2019 at 05:42:25PM +0530, Amit Daniel Kachhap wrote:
>>>> On 10/29/19 10:28 PM, Mark Rutland wrote:
>>>>> This series is a reworked version of Torsten's FTRACE_WITH_REGS series
>>>>> [1]. I've tried to rework the existing code in preparatory patches so
>>>>> that the patchable-function-entry bits slot in with fewer surprises.
>>>>> This version is based on v5.4-rc3, and can be found in my
>>>>> arm64/ftrace-with-regs branch [2].
>>>>>
>>>>> Patch 1 adds an (optional) ftrace_init_nop(), which the core code uses
>>>>> to initialize callsites. This allows us to avoid a synthetic MCOUNT_ADDR
>>>>> symbol, and more cleanly separates the one-time initialization of the
>>>>> callsite from dynamic NOP<->CALL modification. Architectures which don't
>>>>> implement this get the existing ftrace_make_nop() with MCOUNT_ADDR.
>>>>>
>>>>> Recently parisc gained ftrace support using patchable-function-entry.
>>>>> Patch 2 makes the handling of module callsite locations common in
>>>>> kernel/module.c with a new FTRACE_CALLSITE_SECTION definition, and
>>>>> removed the newly redundant bits from arch/parisc.
>>>>>
>>>>> Patches 3 and 4 move the module PLT initialization to module load time,
>>>>> which simplifies runtime callsite modification. This also means that we
>>>>> don't transitently mark the module text RW, and will allow for the
>>>>> removal of module_disable_ro().
>>>>>
>>>>> Patches 5 and 6 add some trivial infrastructure, with patch 7 finally
>>>>> adding FTRACE_WITH_REGS support. Additional work will be required for
>>>>> livepatching (e.g. implementing reliable stack trace), which is
>>>>> commented as part of patch 7.
>>>>>
>>>>> Patch 8 is a trivial cleanup atop of the rest of the series, making the
>>>>> code easier to read and less susceptible to config-specific breakage.
>>>> I tested the whole series with my latest in-kernel ptrauth patches [1]
>>>> and graph_tracer/function_graph_tracer works fine, So for the whole series,
>>>> Tested-by: Amit Daniel Kachhap <[email protected]>
>>>>
>>>> Also I gave few minor comments in the individual patches. With those
>>>> comments,
>>>> Signed-off-by: Amit Daniel Kachhap <[email protected]>
>> Oops sorry I meant,
>> Reviewed-off-by: Amit Daniel Kachhap <[email protected]>
>
> Thanks!
>
> I've added the Tested-by for the whole series, and the Reviewed-by for
> patches 4 and 7. I haven't added it for patch 1 just yet; please reply
> to my comment there if you'd still like to add a Reviewed-by.
Those were minor comments. Please go ahead and add the Reviewed-by's.
Thanks,
Amit
>
> Mark.
>
On Tue, Nov 05, 2019 at 12:17:26PM +0530, Amit Kachhap wrote:
> On 11/4/19 7:06 PM, Mark Rutland wrote:
> > On Sat, Nov 02, 2019 at 05:49:00PM +0530, Amit Daniel Kachhap wrote:
> > > On 10/29/19 10:28 PM, Mark Rutland wrote:
> > > > +/**
> > > > + * ftrace_init_nop - initialize a nop call site
> > > > + * @mod: module structure if called by module load initialization
> > > > + * @rec: the call site record (e.g. mcount/fentry)
> > > > + *
> > > > + * This is a very sensitive operation and great care needs
> > > > + * to be taken by the arch. The operation should carefully
> > > > + * read the location, check to see if what is read is indeed
> > > > + * what we expect it to be, and then on success of the compare,
> > > > + * it should write to the location.
> > > > + *
> > > > + * The code segment at @rec->ip should contain the contents created by
> > > > + * the compiler
> > > Nit: Will it be better to write it as "@rec->ip should store the adjusted
> > > ftrace entry address of the call site" or something like that.
> >
> > This was the specific wording requested by Steve, and it's trying to
> > describe the instructions at rec->ip, rather than the value of rec->ip,
> > so I think it's better to leave this as-is.
> ok Its fine this way too. Actually from the comment, I could not understand
> which one of the compiler contents this points to as in this case there are
> 2 nops.
We can't say what the compiler contents will be. An architecture may use
this callback if it's using mcount, mfentry, patchable-function-entry,
or some other mechanism we're not aware of today. Depending on the
architecture and mechanism, the callsite could contain a number of
distinct things.
All the comment is trying to say is that when ftrace_init_nop() is
called, the callsite has not been modified in any way since being
compiled, so we can expect the contents to be whatever the compiler
generated.
Thanks,
Mark.
On 11/6/19 7:45 PM, Mark Rutland wrote:
> On Tue, Nov 05, 2019 at 12:17:26PM +0530, Amit Kachhap wrote:
>> On 11/4/19 7:06 PM, Mark Rutland wrote:
>>> On Sat, Nov 02, 2019 at 05:49:00PM +0530, Amit Daniel Kachhap wrote:
>>>> On 10/29/19 10:28 PM, Mark Rutland wrote:
>>>>> +/**
>>>>> + * ftrace_init_nop - initialize a nop call site
>>>>> + * @mod: module structure if called by module load initialization
>>>>> + * @rec: the call site record (e.g. mcount/fentry)
>>>>> + *
>>>>> + * This is a very sensitive operation and great care needs
>>>>> + * to be taken by the arch. The operation should carefully
>>>>> + * read the location, check to see if what is read is indeed
>>>>> + * what we expect it to be, and then on success of the compare,
>>>>> + * it should write to the location.
>>>>> + *
>>>>> + * The code segment at @rec->ip should contain the contents created by
>>>>> + * the compiler
>>>> Nit: Will it be better to write it as "@rec->ip should store the adjusted
>>>> ftrace entry address of the call site" or something like that.
>>>
>>> This was the specific wording requested by Steve, and it's trying to
>>> describe the instructions at rec->ip, rather than the value of rec->ip,
>>> so I think it's better to leave this as-is.
>> ok Its fine this way too. Actually from the comment, I could not understand
>> which one of the compiler contents this points to as in this case there are
>> 2 nops.
>
> We can't say what the compiler contents will be. An architecture may use
> this callback if it's using mcount, mfentry, patchable-function-entry,
> or some other mechanism we're not aware of today. Depending on the
> architecture and mechanism, the callsite could contain a number of
> distinct things.
>
> All the comment is trying to say is that when ftrace_init_nop() is
> called, the callsite has not been modified in any way since being
> compiled, so we can expect the contents to be whatever the compiler
> generated.
ok. Your details seems reasonable.
Thanks,
Amit Daniel
>
> Thanks,
> Mark.
>
On Tue, Oct 29, 2019 at 04:58:24PM +0000, Mark Rutland wrote:
> [2] git://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git arm64/ftrace-with-regs
I pulled the latest on this branch into the arm64 for-next/core.
Thanks.
--
Catalin
On Fri, 15 Nov 2019 07:05:39 +0900
Itaru Kitayama <[email protected]> wrote:
> Is this feature avail even when building kernel with Clang?
If your compiler can ...
[...]
> > compiler insert a configurable number of NOPs between the function
> > entry point and the usual prologue. This also ensures functions are
> > AAPCS compliant (e.g. disabling inter-procedural register
> > allocation).
> >
> > For example, with -fpatchable-function-entry=2, GCC 8.1.0 compiles
> > the following:
> >
> > | unsigned long bar(void);
> > |
> > | unsigned long foo(void)
> > | {
> > | return bar() + 1;
> > | }
> >
> > ... to:
> >
> > | <foo>:
> > | nop
> > | nop
> > | stp x29, x30, [sp, #-16]!
* insert e.g. 2 NOPs
* record all those locations in a section called
"patchable_function_entries"
* stick to the AAPCS
then: yes. So far I only implemented this for gcc.
Torsten
On Fri, Nov 15, 2019 at 07:05:39AM +0900, Itaru Kitayama wrote:
> Is this feature avail even when building kernel with Clang?
Clang doesn't currently support -fpatchable-function-entry.
Nick (Cc'd) created an LLVM bugzilla ticket for that:
https://bugs.llvm.org/show_bug.cgi?id=43912
... though it looks like that's having database issues at the moment.
Thanks,
Mark.
> On Wed, Oct 30, 2019 at 2:01 Mark Rutland <[email protected]> wrote:
>
> From: Torsten Duwe <[email protected]>
>
> This patch implements FTRACE_WITH_REGS for arm64, which allows a traced
> function's arguments (and some other registers) to be captured into a
> struct pt_regs, allowing these to be inspected and/or modified. This is
> a building block for live-patching, where a function's arguments may be
> forwarded to another function. This is also necessary to enable ftrace
> and in-kernel pointer authentication at the same time, as it allows the
> LR value to be captured and adjusted prior to signing.
>
> Using GCC's -fpatchable-function-entry=N option, we can have the
> compiler insert a configurable number of NOPs between the function entry
> point and the usual prologue. This also ensures functions are AAPCS
> compliant (e.g. disabling inter-procedural register allocation).
>
> For example, with -fpatchable-function-entry=2, GCC 8.1.0 compiles the
> following:
>
> | unsigned long bar(void);
> |
> | unsigned long foo(void)
> | {
> | return bar() + 1;
> | }
>
> ... to:
>
> | <foo>:
> | nop
> | nop
> | stp x29, x30, [sp, #-16]!
> | mov x29, sp
> | bl 0 <bar>
> | add x0, x0, #0x1
> | ldp x29, x30, [sp], #16
> | ret
>
> This patch builds the kernel with -fpatchable-function-entry=2,
> prefixing each function with two NOPs. To trace a function, we replace
> these NOPs with a sequence that saves the LR into a GPR, then calls an
> ftrace entry assembly function which saves this and other relevant
> registers:
>
> | mov x9, x30
> | bl <ftrace-entry>
>
> Since patchable functions are AAPCS compliant (and the kernel does not
> use x18 as a platform register), x9-x18 can be safely clobbered in the
> patched sequence and the ftrace entry code.
>
> There are now two ftrace entry functions, ftrace_regs_entry (which saves
> all GPRs), and ftrace_entry (which saves the bare minimum). A PLT is
> allocated for each within modules.
>
> Signed-off-by: Torsten Duwe <[email protected]>
> [Mark: rework asm, comments, PLTs, initialization, commit message]
> Signed-off-by: Mark Rutland <[email protected]>
> Reviewed-by: Ard Biesheuvel <[email protected]>
> Cc: AKASHI Takahiro <[email protected]>
> Cc: Amit Daniel Kachhap <[email protected]>
> Cc: Catalin Marinas <[email protected]>
> Cc: Josh Poimboeuf <[email protected]>
> Cc: Julien Thierry <[email protected]>
> Cc: Will Deacon <[email protected]>
> ---
> arch/arm64/Kconfig | 2 +
> arch/arm64/Makefile | 5 ++
> arch/arm64/include/asm/ftrace.h | 23 +++++++
> arch/arm64/include/asm/module.h | 2 +-
> arch/arm64/kernel/entry-ftrace.S | 140
> +++++++++++++++++++++++++++++++++++++--
> arch/arm64/kernel/ftrace.c | 84 +++++++++++++++++++----
> arch/arm64/kernel/module-plts.c | 3 +-
> arch/arm64/kernel/module.c | 18 +++--
> 8 files changed, 252 insertions(+), 25 deletions(-)
>
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 950a56b71ff0..0ffb8596b8a1 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -143,6 +143,8 @@ config ARM64
> select HAVE_DEBUG_KMEMLEAK
> select HAVE_DMA_CONTIGUOUS
> select HAVE_DYNAMIC_FTRACE
> + select HAVE_DYNAMIC_FTRACE_WITH_REGS \
> + if $(cc-option,-fpatchable-function-entry=2)
> select HAVE_EFFICIENT_UNALIGNED_ACCESS
> select HAVE_FAST_GUP
> select HAVE_FTRACE_MCOUNT_RECORD
> diff --git a/arch/arm64/Makefile b/arch/arm64/Makefile
> index 2c0238ce0551..1fbe24d4fdb6 100644
> --- a/arch/arm64/Makefile
> +++ b/arch/arm64/Makefile
> @@ -95,6 +95,11 @@ ifeq ($(CONFIG_ARM64_MODULE_PLTS),y)
> KBUILD_LDS_MODULE += $(srctree)/arch/arm64/kernel/module.lds
> endif
>
> +ifeq ($(CONFIG_DYNAMIC_FTRACE_WITH_REGS),y)
> + KBUILD_CPPFLAGS += -DCC_USING_PATCHABLE_FUNCTION_ENTRY
> + CC_FLAGS_FTRACE := -fpatchable-function-entry=2
> +endif
> +
> # Default value
> head-y := arch/arm64/kernel/head.o
>
> diff --git a/arch/arm64/include/asm/ftrace.h b/arch/arm64/include/asm/
> ftrace.h
> index d48667b04c41..91fa4baa1a93 100644
> --- a/arch/arm64/include/asm/ftrace.h
> +++ b/arch/arm64/include/asm/ftrace.h
> @@ -11,9 +11,20 @@
> #include <asm/insn.h>
>
> #define HAVE_FUNCTION_GRAPH_FP_TEST
> +
> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
> +#define ARCH_SUPPORTS_FTRACE_OPS 1
> +#else
> #define MCOUNT_ADDR ((unsigned long)_mcount)
> +#endif
> +
> +/* The BL at the callsite's adjusted rec->ip */
> #define MCOUNT_INSN_SIZE AARCH64_INSN_SIZE
>
> +#define FTRACE_PLT_IDX 0
> +#define FTRACE_REGS_PLT_IDX 1
> +#define NR_FTRACE_PLTS 2
> +
> /*
> * Currently, gcc tends to save the link register after the local
> variables
> * on the stack. This causes the max stack tracer to report the function
> @@ -44,12 +55,24 @@ extern void return_to_handler(void);
> static inline unsigned long ftrace_call_adjust(unsigned long addr)
> {
> /*
> + * Adjust addr to point at the BL in the callsite.
> + * See ftrace_init_nop() for the callsite sequence.
> + */
> + if (IS_ENABLED(CONFIG_DYNAMIC_FTRACE_WITH_REGS))
> + return addr + AARCH64_INSN_SIZE;
> + /*
> * addr is the address of the mcount call instruction.
> * recordmcount does the necessary offset calculation.
> */
> return addr;
> }
>
> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
> +struct dyn_ftrace;
> +int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec);
> +#define ftrace_init_nop ftrace_init_nop
> +#endif
> +
> #define ftrace_return_address(n) return_address(n)
>
> /*
> diff --git a/arch/arm64/include/asm/module.h b/arch/arm64/include/asm/
> module.h
> index f80e13cbf8ec..1e93de68c044 100644
> --- a/arch/arm64/include/asm/module.h
> +++ b/arch/arm64/include/asm/module.h
> @@ -21,7 +21,7 @@ struct mod_arch_specific {
> struct mod_plt_sec init;
>
> /* for CONFIG_DYNAMIC_FTRACE */
> - struct plt_entry *ftrace_trampoline;
> + struct plt_entry *ftrace_trampolines;
> };
> #endif
>
> diff --git a/arch/arm64/kernel/entry-ftrace.S b/arch/arm64/kernel/
> entry-ftrace.S
> index 33d003d80121..94720388957f 100644
> --- a/arch/arm64/kernel/entry-ftrace.S
> +++ b/arch/arm64/kernel/entry-ftrace.S
> @@ -7,10 +7,137 @@
> */
>
> #include <linux/linkage.h>
> +#include <asm/asm-offsets.h>
> #include <asm/assembler.h>
> #include <asm/ftrace.h>
> #include <asm/insn.h>
>
> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
> +/*
> + * Due to -fpatchable-function-entry=2, the compiler has placed two NOPs
> before
> + * the regular function prologue. For an enabled callsite, ftrace_init_nop
> () and
> + * ftrace_make_call() have patched those NOPs to:
> + *
> + * MOV X9, LR
> + * BL <entry>
> + *
> + * ... where <entry> is either ftrace_caller or ftrace_regs_caller.
> + *
> + * Each instrumented function follows the AAPCS, so here x0-x8 and x19-x30
> are
> + * live, and x9-x18 are safe to clobber.
> + *
> + * We save the callsite's context into a pt_regs before invoking and
> ftrace
> + * callbacks. So that we can get a sensible backtrace, we create a stack
> record
> + * for the callsite and the ftrace entry assembly. This is not sufficient
> for
> + * reliable stacktrace: until we create the callsite stack record, its
> caller
> + * is missing from the LR and existing chain of frame records.
> + */
> + .macro ftrace_regs_entry, allregs=0
> + /* Make room for pt_regs, plus a callee frame */
> + sub sp, sp, #(S_FRAME_SIZE + 16)
> +
> + /* Save function arguments (and x9 for simplicity) */
> + stp x0, x1, [sp, #S_X0]
> + stp x2, x3, [sp, #S_X2]
> + stp x4, x5, [sp, #S_X4]
> + stp x6, x7, [sp, #S_X6]
> + stp x8, x9, [sp, #S_X8]
> +
> + /* Optionally save the callee-saved registers, always save the FP *
> /
> + .if \allregs == 1
> + stp x10, x11, [sp, #S_X10]
> + stp x12, x13, [sp, #S_X12]
> + stp x14, x15, [sp, #S_X14]
> + stp x16, x17, [sp, #S_X16]
> + stp x18, x19, [sp, #S_X18]
> + stp x20, x21, [sp, #S_X20]
> + stp x22, x23, [sp, #S_X22]
> + stp x24, x25, [sp, #S_X24]
> + stp x26, x27, [sp, #S_X26]
> + stp x28, x29, [sp, #S_X28]
> + .else
> + str x29, [sp, #S_FP]
> + .endif
> +
> + /* Save the callsite's SP and LR */
> + add x10, sp, #(S_FRAME_SIZE + 16)
> + stp x9, x10, [sp, #S_LR]
> +
> + /* Save the PC after the ftrace callsite */
> + str x30, [sp, #S_PC]
> +
> + /* Create a frame record for the callsite above pt_regs */
> + stp x29, x9, [sp, #S_FRAME_SIZE]
> + add x29, sp, #S_FRAME_SIZE
> +
> + /* Create our frame record within pt_regs. */
> + stp x29, x30, [sp, #S_STACKFRAME]
> + add x29, sp, #S_STACKFRAME
> + .endm
> +
> +ENTRY(ftrace_regs_caller)
> + ftrace_regs_entry 1
> + b ftrace_common
> +ENDPROC(ftrace_regs_caller)
> +
> +ENTRY(ftrace_caller)
> + ftrace_regs_entry 0
> + b ftrace_common
> +ENDPROC(ftrace_caller)
> +
> +ENTRY(ftrace_common)
> + sub x0, x30, #AARCH64_INSN_SIZE // ip (callsite's BL insn)
> + mov x1, x9 // parent_ip (callsite's
> LR)
> + ldr_l x2, function_trace_op // op
> + mov x3, sp // regs
> +
> +GLOBAL(ftrace_call)
> + bl ftrace_stub
> +
> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> +GLOBAL(ftrace_graph_call) // ftrace_graph_caller();
> + nop // If enabled, this will be
> replaced
> + // "b ftrace_graph_caller"
> +#endif
> +
> +/*
> + * At the callsite x0-x8 and x19-x30 were live. Any C code will have
> preserved
> + * x19-x29 per the AAPCS, and we created frame records upon entry, so we
> need
> + * to restore x0-x8, x29, and x30.
> + */
> +ftrace_common_return:
> + /* Restore function arguments */
> + ldp x0, x1, [sp]
> + ldp x2, x3, [sp, #S_X2]
> + ldp x4, x5, [sp, #S_X4]
> + ldp x6, x7, [sp, #S_X6]
> + ldr x8, [sp, #S_X8]
> +
> + /* Restore the callsite's FP, LR, PC */
> + ldr x29, [sp, #S_FP]
> + ldr x30, [sp, #S_LR]
> + ldr x9, [sp, #S_PC]
> +
> + /* Restore the callsite's SP */
> + add sp, sp, #S_FRAME_SIZE + 16
> +
> + ret x9
> +ENDPROC(ftrace_common)
> +
> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> +ENTRY(ftrace_graph_caller)
> + ldr x0, [sp, #S_PC]
> + sub x0, x0, #AARCH64_INSN_SIZE // ip (callsite's BL insn)
> + add x1, sp, #S_LR // parent_ip (callsite's
> LR)
> + ldr x2, [sp, #S_FRAME_SIZE] // parent fp (callsite's
> FP)
> + bl prepare_ftrace_return
> + b ftrace_common_return
> +ENDPROC(ftrace_graph_caller)
> +#else
> +#endif
> +
> +#else /* CONFIG_DYNAMIC_FTRACE_WITH_REGS */
> +
> /*
> * Gcc with -pg will put the following code in the beginning of each
> function:
> * mov x0, x30
> @@ -160,11 +287,6 @@ GLOBAL(ftrace_graph_call) //
> ftrace_graph_caller();
>
> mcount_exit
> ENDPROC(ftrace_caller)
> -#endif /* CONFIG_DYNAMIC_FTRACE */
> -
> -ENTRY(ftrace_stub)
> - ret
> -ENDPROC(ftrace_stub)
>
> #ifdef CONFIG_FUNCTION_GRAPH_TRACER
> /*
> @@ -184,7 +306,15 @@ ENTRY(ftrace_graph_caller)
>
> mcount_exit
> ENDPROC(ftrace_graph_caller)
> +#endif /* CONFIG_FUNCTION_GRAPH_TRACER */
> +#endif /* CONFIG_DYNAMIC_FTRACE */
> +#endif /* CONFIG_DYNAMIC_FTRACE_WITH_REGS */
> +
> +ENTRY(ftrace_stub)
> + ret
> +ENDPROC(ftrace_stub)
>
> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> /*
> * void return_to_handler(void)
> *
> diff --git a/arch/arm64/kernel/ftrace.c b/arch/arm64/kernel/ftrace.c
> index 822718eafdb4..aea652c33a38 100644
> --- a/arch/arm64/kernel/ftrace.c
> +++ b/arch/arm64/kernel/ftrace.c
> @@ -62,6 +62,19 @@ int ftrace_update_ftrace_func(ftrace_func_t func)
> return ftrace_modify_code(pc, 0, new, false);
> }
>
> +#ifdef CONFIG_ARM64_MODULE_PLTS
> +static struct plt_entry *get_ftrace_plt(struct module *mod, unsigned long
> addr)
> +{
> + struct plt_entry *plt = mod->arch.ftrace_trampolines;
> +
> + if (addr == FTRACE_ADDR)
> + return &plt[FTRACE_PLT_IDX];
> + if (addr == FTRACE_REGS_ADDR && IS_ENABLED
> (CONFIG_FTRACE_WITH_REGS))
> + return &plt[FTRACE_REGS_PLT_IDX];
> + return NULL;
> +}
> +#endif
> +
> /*
> * Turn on the call to ftrace_caller() in instrumented function
> */
> @@ -74,19 +87,7 @@ int ftrace_make_call(struct dyn_ftrace *rec, unsigned
> long addr)
> if (offset < -SZ_128M || offset >= SZ_128M) {
> #ifdef CONFIG_ARM64_MODULE_PLTS
> struct module *mod;
> -
> - /*
> - * There is only one ftrace trampoline per module. For now,
> - * this is not a problem since on arm64, all dynamic ftrace
> - * invocations are routed via ftrace_caller(). This will
> need
> - * to be revisited if support for multiple ftrace entry
> points
> - * is added in the future, but for now, the pr_err() below
> - * deals with a theoretical issue only.
> - */
> - if (addr != FTRACE_ADDR) {
> - pr_err("ftrace: far branches to multiple entry
> points unsupported inside a single module\n");
> - return -EINVAL;
> - }
> + struct plt_entry *plt;
>
> /*
> * On kernels that support module PLTs, the offset between
> the
> @@ -105,7 +106,13 @@ int ftrace_make_call(struct dyn_ftrace *rec, unsigned
> long addr)
> if (WARN_ON(!mod))
> return -EINVAL;
>
> - addr = (unsigned long)mod->arch.ftrace_trampoline;
> + plt = get_ftrace_plt(mod, addr);
> + if (!plt) {
> + pr_err("ftrace: no module PLT for %ps\n", (void *)
> addr);
> + return -EINVAL;
> + }
> +
> + addr = (unsigned long)plt;
> #else /* CONFIG_ARM64_MODULE_PLTS */
> return -EINVAL;
> #endif /* CONFIG_ARM64_MODULE_PLTS */
> @@ -117,6 +124,55 @@ int ftrace_make_call(struct dyn_ftrace *rec, unsigned
> long addr)
> return ftrace_modify_code(pc, old, new, true);
> }
>
> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
> +int ftrace_modify_call(struct dyn_ftrace *rec, unsigned long old_addr,
> + unsigned long addr)
> +{
> + unsigned long pc = rec->ip;
> + u32 old, new;
> +
> + old = aarch64_insn_gen_branch_imm(pc, old_addr,
> + AARCH64_INSN_BRANCH_LINK);
> + new = aarch64_insn_gen_branch_imm(pc, addr,
> AARCH64_INSN_BRANCH_LINK);
> +
> + return ftrace_modify_code(pc, old, new, true);
> +}
> +
> +/*
> + * The compiler has inserted two NOPs before the regular function
> prologue.
> + * All instrumented functions follow the AAPCS, so x0-x8 and x19-x30 are
> live,
> + * and x9-x18 are free for our use.
> + *
> + * At runtime we want to be able to swing a single NOP <-> BL to enable or
> + * disable the ftrace call. The BL requires us to save the original LR
> value,
> + * so here we insert a <MOV X9, LR> over the first NOP so the instructions
> + * before the regular prologue are:
> + *
> + * | Compiled | Disabled | Enabled |
> + * +----------+------------+------------+
> + * | NOP | MOV X9, LR | MOV X9, LR |
> + * | NOP | NOP | BL <entry> |
> + *
> + * The LR value will be recovered by ftrace_regs_entry, and restored into
> LR
> + * before returning to the regular function prologue. When a function is
> not
> + * being traced, the MOV is not harmful given x9 is not live per the
> AAPCS.
> + *
> + * Note: ftrace_process_locs() has pre-adjusted rec->ip to be the address
> of
> + * the BL.
> + */
> +int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec)
> +{
> + unsigned long pc = rec->ip - AARCH64_INSN_SIZE;
> + u32 old, new;
> +
> + old = aarch64_insn_gen_nop();
> + new = aarch64_insn_gen_move_reg(AARCH64_INSN_REG_9,
> + AARCH64_INSN_REG_LR,
> + AARCH64_INSN_VARIANT_64BIT);
> + return ftrace_modify_code(pc, old, new, true);
> +}
> +#endif
> +
> /*
> * Turn off the call to ftrace_caller() in instrumented function
> */
> diff --git a/arch/arm64/kernel/module-plts.c b/arch/arm64/kernel/
> module-plts.c
> index b182442b87a3..65b08a74aec6 100644
> --- a/arch/arm64/kernel/module-plts.c
> +++ b/arch/arm64/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>
> @@ -330,7 +331,7 @@ int module_frob_arch_sections(Elf_Ehdr *ehdr, Elf_Shdr
> *sechdrs,
> tramp->sh_type = SHT_NOBITS;
> tramp->sh_flags = SHF_EXECINSTR | SHF_ALLOC;
> tramp->sh_addralign = __alignof__(struct plt_entry);
> - tramp->sh_size = sizeof(struct plt_entry);
> + tramp->sh_size = NR_FTRACE_PLTS * sizeof(struct plt_entry);
> }
>
> return 0;
> diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c
> index 5f5bc3b94da7..00d21a420c60 100644
> --- a/arch/arm64/kernel/module.c
> +++ b/arch/arm64/kernel/module.c
> @@ -486,21 +486,31 @@ static const Elf_Shdr *find_section(const Elf_Ehdr
> *hdr,
> return NULL;
> }
>
> +static inline void __init_plt(struct plt_entry *plt, unsigned long addr)
> +{
> + *plt = get_plt_entry(addr, plt);
> +}
> +
> int module_init_ftrace_plt(const Elf_Ehdr *hdr,
> const Elf_Shdr *sechdrs,
> struct module *mod)
> {
> #if defined(CONFIG_ARM64_MODULE_PLTS) && defined(CONFIG_DYNAMIC_FTRACE)
> const Elf_Shdr *s;
> - struct plt_entry *plt;
> + struct plt_entry *plts;
>
> s = find_section(hdr, sechdrs, ".text.ftrace_trampoline");
> if (!s)
> return -ENOEXEC;
>
> - plt = (void *)s->sh_addr;
> - *plt = get_plt_entry(FTRACE_ADDR, plt);
> - mod->arch.ftrace_trampoline = plt;
> + plts = (void *)s->sh_addr;
> +
> + __init_plt(&plts[FTRACE_PLT_IDX], FTRACE_ADDR);
> +
> + if (IS_ENABLED(CONFIG_DYNAMIC_FTRACE_WITH_REGS))
> + __init_plt(&plts[FTRACE_REGS_PLT_IDX], FTRACE_REGS_ADDR);
> +
> + mod->arch.ftrace_trampolines = plts;
> #endif
> return 0;
> }
> --
> 2.11.0
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>