| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAFTtA3NqC+r9HBS2cJvwzZMyTECJe3VbWrsZgWo6e-LTgpPOaA@mail.gmail.com>
Date: Wed, 7 May 2025 22:18:41 +0800
From: Andy Chiu <andybnac@...il.com>
To: Alexandre Ghiti <alex@...ti.fr>
Cc: linux-riscv@...ts.infradead.org, alexghiti@...osinc.com,
palmer@...belt.com, Andy Chiu <andy.chiu@...ive.com>,
Björn Töpel <bjorn@...osinc.com>,
linux-kernel@...r.kernel.org, linux-trace-kernel@...r.kernel.org,
Mark Rutland <mark.rutland@....com>, puranjay12@...il.com, paul.walmsley@...ive.com,
greentime.hu@...ive.com, nick.hu@...ive.com, nylon.chen@...ive.com,
eric.lin@...ive.com, vicent.chen@...ive.com, zong.li@...ive.com,
yongxuan.wang@...ive.com, samuel.holland@...ive.com, olivia.chu@...ive.com,
c2232430@...il.com
Subject: Re: [PATCH v4 05/12] riscv: ftrace: prepare ftrace for atomic code patching
On Mon, May 5, 2025 at 10:06 PM Alexandre Ghiti <alex@...ti.fr> wrote:
>
> On 23/04/2025 10:22, Alexandre Ghiti wrote:
> > Hi Andy,
> >
> > On 07/04/2025 20:08, Andy Chiu wrote:
> >> From: Andy Chiu <andy.chiu@...ive.com>
> >>
> >> We use an AUIPC+JALR pair to jump into a ftrace trampoline. Since
> >> instruction fetch can break down to 4 byte at a time, it is impossible
> >> to update two instructions without a race. In order to mitigate it, we
> >> initialize the patchable entry to AUIPC + NOP4. Then, the run-time code
> >> patching can change NOP4 to JALR to eable/disable ftrcae from a
> >> function. This limits the reach of each ftrace entry to +-2KB displacing
> >> from ftrace_caller.
> >>
> >> Starting from the trampoline, we add a level of indirection for it to
> >> reach ftrace caller target. Now, it loads the target address from a
> >> memory location, then perform the jump. This enable the kernel to update
> >> the target atomically.
> >>
> >> The new don't-stop-the-world text patching on change only one RISC-V
> >> instruction:
> >>
> >> | -8: &ftrace_ops of the associated tracer function.
> >> | <ftrace enable>:
> >> | 0: auipc t0, hi(ftrace_caller)
> >> | 4: jalr t0, lo(ftrace_caller)
> >> |
> >> | -8: &ftrace_nop_ops
> >> | <ftrace disable>:
> >> | 0: auipc t0, hi(ftrace_caller)
> >> | 4: nop
> >>
> >> This means that f+0x0 is fixed, and should not be claimed by ftrace,
> >> e.g. kprobe should be able to put a probe in f+0x0. Thus, we adjust the
> >> offset and MCOUNT_INSN_SIZE accordingly.
> >>
> >> Co-developed-by: Björn Töpel <bjorn@...osinc.com>
> >> Signed-off-by: Björn Töpel <bjorn@...osinc.com>
> >> Signed-off-by: Andy Chiu <andy.chiu@...ive.com>
> >> ---
> >> Changelog v4:
> >> - Include Björn's fix for kprobe
> >> - Refactor code for better reading (Robbin, Björn)
> >> - Remove make_call_ra and friedns (Björn)
> >> - Update comments to match reality (Björn)
> >> - Drop code defined by !WITH_ARG
> >> - Add a synchronization point when updating ftrace_call_dest (Björn)
> >> ---
> >> arch/riscv/include/asm/ftrace.h | 49 ++++++------
> >> arch/riscv/kernel/ftrace.c | 130 ++++++++++++++++----------------
> >> arch/riscv/kernel/mcount-dyn.S | 9 +--
> >> 3 files changed, 92 insertions(+), 96 deletions(-)
> >>
> >> diff --git a/arch/riscv/include/asm/ftrace.h
> >> b/arch/riscv/include/asm/ftrace.h
> >> index d8b2138bd9c6..6a5c0a7fb826 100644
> >> --- a/arch/riscv/include/asm/ftrace.h
> >> +++ b/arch/riscv/include/asm/ftrace.h
> >> @@ -20,10 +20,9 @@ extern void *return_address(unsigned int level);
> >> #define ftrace_return_address(n) return_address(n)
> >> void _mcount(void);
> >> -static inline unsigned long ftrace_call_adjust(unsigned long addr)
> >> -{
> >> - return addr;
> >> -}
> >> +unsigned long ftrace_call_adjust(unsigned long addr);
> >> +unsigned long arch_ftrace_get_symaddr(unsigned long fentry_ip);
> >> +#define ftrace_get_symaddr(fentry_ip)
> >> arch_ftrace_get_symaddr(fentry_ip)
> >> /*
> >> * Let's do like x86/arm64 and ignore the compat syscalls.
> >> @@ -57,12 +56,21 @@ struct dyn_arch_ftrace {
> >> * 2) jalr: setting low-12 offset to ra, jump to ra, and set ra to
> >> * return address (original pc + 4)
> >> *
> >> + * The first 2 instructions for each tracable function is compiled
> >> to 2 nop
> >> + * instructions. Then, the kernel initializes the first instruction
> >> to auipc at
> >> + * boot time (<ftrace disable>). The second instruction is patched
> >> to jalr to
> >> + * start the trace.
> >> + *
> >> + *<Image>:
> >> + * 0: nop
> >> + * 4: nop
> >> + *
> >> *<ftrace enable>:
> >> - * 0: auipc t0/ra, 0x?
> >> - * 4: jalr t0/ra, ?(t0/ra)
> >> + * 0: auipc t0, 0x?
> >> + * 4: jalr t0, ?(t0)
> >> *
> >> *<ftrace disable>:
> >> - * 0: nop
> >> + * 0: auipc t0, 0x?
> >> * 4: nop
> >> *
> >> * Dynamic ftrace generates probes to call sites, so we must deal with
> >> @@ -75,10 +83,9 @@ struct dyn_arch_ftrace {
> >> #define AUIPC_OFFSET_MASK (0xfffff000)
> >> #define AUIPC_PAD (0x00001000)
> >> #define JALR_SHIFT 20
> >> -#define JALR_RA (0x000080e7)
> >> -#define AUIPC_RA (0x00000097)
> >> #define JALR_T0 (0x000282e7)
> >> #define AUIPC_T0 (0x00000297)
> >> +#define JALR_RANGE (JALR_SIGN_MASK - 1)
> >> #define to_jalr_t0(offset) \
> >> (((offset & JALR_OFFSET_MASK) << JALR_SHIFT) | JALR_T0)
> >> @@ -96,26 +103,14 @@ do { \
> >> call[1] = to_jalr_t0(offset); \
> >> } while (0)
> >> -#define to_jalr_ra(offset) \
> >> - (((offset & JALR_OFFSET_MASK) << JALR_SHIFT) | JALR_RA)
> >> -
> >> -#define to_auipc_ra(offset) \
> >> - ((offset & JALR_SIGN_MASK) ? \
> >> - (((offset & AUIPC_OFFSET_MASK) + AUIPC_PAD) | AUIPC_RA) : \
> >> - ((offset & AUIPC_OFFSET_MASK) | AUIPC_RA))
> >> -
> >> -#define make_call_ra(caller, callee, call) \
> >> -do { \
> >> - unsigned int offset = \
> >> - (unsigned long) (callee) - (unsigned long) (caller); \
> >> - call[0] = to_auipc_ra(offset); \
> >> - call[1] = to_jalr_ra(offset); \
> >> -} while (0)
> >> -
> >> /*
> >> - * Let auipc+jalr be the basic *mcount unit*, so we make it 8 bytes
> >> here.
> >> + * Only the jalr insn in the auipc+jalr is patched, so we make it 4
> >> + * bytes here.
> >> */
> >> -#define MCOUNT_INSN_SIZE 8
> >> +#define MCOUNT_INSN_SIZE 4
> >> +#define MCOUNT_AUIPC_SIZE 4
> >> +#define MCOUNT_JALR_SIZE 4
> >> +#define MCOUNT_NOP4_SIZE 4
> >> #ifndef __ASSEMBLY__
> >> struct dyn_ftrace;
> >> diff --git a/arch/riscv/kernel/ftrace.c b/arch/riscv/kernel/ftrace.c
> >> index 1fd10555c580..cf78eef073a0 100644
> >> --- a/arch/riscv/kernel/ftrace.c
> >> +++ b/arch/riscv/kernel/ftrace.c
> >> @@ -8,10 +8,21 @@
> >> #include <linux/ftrace.h>
> >> #include <linux/uaccess.h>
> >> #include <linux/memory.h>
> >> +#include <linux/irqflags.h>
> >> #include <linux/stop_machine.h>
> >> #include <asm/cacheflush.h>
> >> #include <asm/text-patching.h>
> >> +unsigned long ftrace_call_adjust(unsigned long addr)
> >> +{
> >> + return addr + MCOUNT_AUIPC_SIZE;
> >> +}
> >> +
> >> +unsigned long arch_ftrace_get_symaddr(unsigned long fentry_ip)
> >> +{
> >> + return fentry_ip - MCOUNT_AUIPC_SIZE;
> >> +}
> >> +
> >
> >
> > Those functions cause the following errors when building with
> > !CONFIG_DYNAMIC_FTRACE, but I'm not sure how to fix this:
> >
> > ../arch/riscv/kernel/ftrace.c: In function 'ftrace_call_adjust':
> > ../arch/riscv/kernel/ftrace.c:19:35: error: 'MCOUNT_AUIPC_SIZE'
> > undeclared (first use in this function)
> > 19 | return addr + 8 + MCOUNT_AUIPC_SIZE;
> > | ^~~~~~~~~~~~~~~~~
> > ../arch/riscv/kernel/ftrace.c:19:35: note: each undeclared identifier
> > is reported only once for each function it appears in
> > CC fs/9p/vfs_dir.o
> > ../arch/riscv/kernel/ftrace.c: In function 'arch_ftrace_get_symaddr':
> > ../arch/riscv/kernel/ftrace.c:26:28: error: 'MCOUNT_AUIPC_SIZE'
> > undeclared (first use in this function)
> > 26 | return fentry_ip - MCOUNT_AUIPC_SIZE;
> > | ^~~~~~~~~~~~~~~~~
> > CC drivers/pci/pcie/pme.o
> > ../arch/riscv/kernel/ftrace.c: In function 'ftrace_call_adjust':
> > ../arch/riscv/kernel/ftrace.c:22:1: error: control reaches end of
> > non-void function [-Werror=return-type]
> > 22 | }
> > | ^
> > ../arch/riscv/kernel/ftrace.c: In function 'arch_ftrace_get_symaddr':
> > ../arch/riscv/kernel/ftrace.c:27:1: error: control reaches end of
> > non-void function [-Werror=return-type]
> > 27 | }
> > | ^
> > cc1: some warnings being treated as errors
> > make[5]: *** [../scripts/Makefile.build:203:
> > arch/riscv/kernel/ftrace.o] Error 1
> >
>
> So I fixed those errors with the following, let me know if that's not
> correct:
>
> diff --git a/arch/riscv/kernel/ftrace.c b/arch/riscv/kernel/ftrace.c
> index d65f06bfb4573..4c6c24380cfd9 100644
> --- a/arch/riscv/kernel/ftrace.c
> +++ b/arch/riscv/kernel/ftrace.c
> @@ -13,6 +13,7 @@
> #include <asm/cacheflush.h>
> #include <asm/text-patching.h>
>
> +#ifdef CONFIG_DYNAMIC_FTRACE
> unsigned long ftrace_call_adjust(unsigned long addr)
> {
> if (IS_ENABLED(CONFIG_DYNAMIC_FTRACE_WITH_CALL_OPS))
> @@ -26,7 +27,6 @@ unsigned long arch_ftrace_get_symaddr(unsigned long
> fentry_ip)
> return fentry_ip - MCOUNT_AUIPC_SIZE;
> }
>
> -#ifdef CONFIG_DYNAMIC_FTRACE
> void arch_ftrace_update_code(int command)
> {
> mutex_lock(&text_mutex);
> @@ -191,7 +191,12 @@ int ftrace_update_ftrace_func(ftrace_func_t func)
> return 0;
> }
>
> -#endif
> +#else /* CONFIG_DYNAMIC_FTRACE */
> +unsigned long ftrace_call_adjust(unsigned long addr)
> +{
> + return addr;
> +}
> +#endif /* CONFIG_DYNAMIC_FTRACE */
>
> #ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
> int ftrace_modify_call(struct dyn_ftrace *rec, unsigned long old_addr,
>
Hi Alex,
Yes, this is valid, thanks for noticing and fixing the error. I would
personally prefer leaving the #else /* CONFIG_DYNAMIC_FTRACE */ part
in ftrace.h, but it can also come later as a refactor. Or, I can
respin the series, with fixes on this and the previous patch, along
with some typos and variable declarations that Robin mentioned.
Thanks,
Andy
Powered by blists - more mailing lists