| 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: <20240403130201.3340e5fe1d01a4f86dd2d772@kernel.org>
Date: Wed, 3 Apr 2024 13:02:01 +0900
From: Masami Hiramatsu (Google) <mhiramat@...nel.org>
To: Björn Töpel <bjorn@...nel.org>
Cc: Puranjay Mohan <puranjay12@...il.com>, Alexandre Ghiti
<alexghiti@...osinc.com>, Andy Chiu <andy.chiu@...ive.com>, Paul Walmsley
<paul.walmsley@...ive.com>, Palmer Dabbelt <palmer@...belt.com>, Albert Ou
<aou@...s.berkeley.edu>, Steven Rostedt <rostedt@...dmis.org>, Mark Rutland
<mark.rutland@....com>, Sami Tolvanen <samitolvanen@...gle.com>, Guo Ren
<guoren@...nel.org>, Ley Foon Tan <leyfoon.tan@...rfivetech.com>, Deepak
Gupta <debug@...osinc.com>, Sia Jee Heng <jeeheng.sia@...rfivetech.com>,
Song Shuai <suagrfillet@...il.com>, Björn Töpel
<bjorn@...osinc.com>, Clément Léger
<cleger@...osinc.com>, Al Viro <viro@...iv.linux.org.uk>,
linux-riscv@...ts.infradead.org, linux-kernel@...r.kernel.org,
linux-trace-kernel@...r.kernel.org
Subject: Re: [RFC PATCH] ftrace: riscv: move from REGS to ARGS
On Tue, 02 Apr 2024 15:02:41 +0200
Björn Töpel <bjorn@...nel.org> wrote:
> Puranjay Mohan <puranjay12@...il.com> writes:
>
> > This commit replaces riscv's support for FTRACE_WITH_REGS with support
> > for FTRACE_WITH_ARGS. This is required for the ongoing effort to stop
> > relying on stop_machine() for RISCV's implementation of ftrace.
> >
> > The main relevant benefit that this change will bring for the above
> > use-case is that now we don't have separate ftrace_caller and
> > ftrace_regs_caller trampolines. This will allow the callsite to call
> > ftrace_caller by modifying a single instruction. Now the callsite can
> > do something similar to:
> >
> > When not tracing: | When tracing:
> >
> > func: func:
> > auipc t0, ftrace_caller_top auipc t0, ftrace_caller_top
> > nop <=========<Enable/Disable>=========> jalr t0, ftrace_caller_bottom
> > [...] [...]
> >
> > The above assumes that we are dropping the support of calling a direct
> > trampoline from the callsite. We need to drop this as the callsite can't
> > change the target address to call, it can only enable/disable a call to
> > a preset target (ftrace_caller in the above diagram).
> >
> > Currently, ftrace_regs_caller saves all CPU registers in the format of
> > struct pt_regs and allows the tracer to modify them. We don't need to
> > save all of the CPU registers because at function entry only a subset of
> > pt_regs is live:
> >
> > |----------+----------+---------------------------------------------|
> > | Register | ABI Name | Description |
> > |----------+----------+---------------------------------------------|
> > | x1 | ra | Return address for traced function |
> > | x2 | sp | Stack pointer |
> > | x5 | t0 | Return address for ftrace_caller trampoline |
> > | x8 | s0/fp | Frame pointer |
> > | x10-11 | a0-1 | Function arguments/return values |
> > | x12-17 | a2-7 | Function arguments |
> > |----------+----------+---------------------------------------------|
> >
> > See RISCV calling convention[1] for the above table.
> >
> > Saving just the live registers decreases the amount of stack space
> > required from 288 Bytes to 112 Bytes.
> >
> > Basic testing was done with this on the VisionFive 2 development board.
> >
> > Note:
> > - Moving from REGS to ARGS will mean that RISCV will stop supporting
> > KPROBES_ON_FTRACE as it requires full pt_regs to be saved.
>
> ...and FPROBES, but Masami is (still?) working on a series to address
> that [1].
Yes, even with this patch, FPROBE can be worked with my series.
So I'm OK for this change.
Thank you,
>
> My perspective; This is following the work Mark et al has done for
> arm64, and it does make sense for RISC-V as well. I'm in favor having
> this change as part of the bigger call_ops/text patch change for RISC-V.
>
> [...]
>
> > diff --git a/arch/riscv/include/asm/ftrace.h b/arch/riscv/include/asm/ftrace.h
> > index 1276d7d9ca8b..1e95bf4ded6c 100644
> > --- a/arch/riscv/include/asm/ftrace.h
> > +++ b/arch/riscv/include/asm/ftrace.h
> > @@ -124,20 +124,87 @@ struct dyn_ftrace;
> > int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec);
> > #define ftrace_init_nop ftrace_init_nop
> >
> > -#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
> > +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_ARGS
> > +#define arch_ftrace_get_regs(regs) NULL
> > struct ftrace_ops;
> > -struct ftrace_regs;
> > +struct ftrace_regs {
> > + unsigned long epc;
> > + unsigned long ra;
> > + unsigned long sp;
> > + unsigned long s0;
> > + unsigned long t1;
> > + union {
> > + unsigned long args[8];
> > + struct {
> > + unsigned long a0;
> > + unsigned long a1;
> > + unsigned long a2;
> > + unsigned long a3;
> > + unsigned long a4;
> > + unsigned long a5;
> > + unsigned long a6;
> > + unsigned long a7;
> > + };
> > + };
> > +};
> > +
> > +static __always_inline unsigned long
> > +ftrace_regs_get_instruction_pointer(const struct ftrace_regs *fregs)
> > +{
> > + return fregs->epc;
> > +}
> > +
> > +static __always_inline void
> > +ftrace_regs_set_instruction_pointer(struct ftrace_regs *fregs,
> > + unsigned long pc)
> > +{
> > + fregs->epc = pc;
> > +}
> > +
> > +static __always_inline unsigned long
> > +ftrace_regs_get_stack_pointer(const struct ftrace_regs *fregs)
> > +{
> > + return fregs->sp;
> > +}
> > +
> > +static __always_inline unsigned long
> > +ftrace_regs_get_argument(struct ftrace_regs *fregs, unsigned int n)
> > +{
> > + if (n < 8)
> > + return fregs->args[n];
> > + return 0;
> > +}
> > +
> > +static __always_inline unsigned long
> > +ftrace_regs_get_return_value(const struct ftrace_regs *fregs)
> > +{
> > + return fregs->a0;
> > +}
> > +
> > +static __always_inline void
> > +ftrace_regs_set_return_value(struct ftrace_regs *fregs,
> > + unsigned long ret)
> > +{
> > + fregs->a0 = ret;
> > +}
> > +
> > +static __always_inline void
> > +ftrace_override_function_with_return(struct ftrace_regs *fregs)
> > +{
> > + fregs->epc = fregs->ra;
> > +}
>
> Style/nit: All above; Try to use the full 100 chars, and keep the
> function name return value on the same line for grepability.
>
>
> Björn
>
> [1] https://lore.kernel.org/all/170887410337.564249.6360118840946697039.stgit@devnote2/
--
Masami Hiramatsu (Google) <mhiramat@...nel.org>
Powered by blists - more mailing lists