Re: [PATCH] perf riscv: Add SDT argument parsing for RISC-V

[Chen Pei <cp0613@linux.alibaba.com>]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=y, Size: 3224 bytes --]

On Tue, 14 Apr 2026 13:17:52 +0000, sashiko-bot@kernel.org wrote:

> > diff --git a/tools/perf/util/perf-regs-arch/perf_regs_riscv.c b/tools/perf/util/perf-regs-arch/perf_regs_riscv.c
> > index 5b5f21fcba8c6..51b43e68e0e3d 100644
> > --- a/tools/perf/util/perf-regs-arch/perf_regs_riscv.c
> > +++ b/tools/perf/util/perf-regs-arch/perf_regs_riscv.c
> 
> [ ... ]
> 
> > +/* RISC-V register ABI names: zero, ra, sp, gp, tp, t0-t6, s0-s11, a0-a7 */
> > +#define SDT_OP_REGEX1  "^(zero|ra|sp|gp|tp|t[0-6]|s[0-9]|s1[01]|a[0-7])$"
> > +
> > +/* RISC-V memory operand: [-]NUM(REG) */
> > +#define SDT_OP_REGEX2  "^(\\-)?([0-9]+)\\((zero|ra|sp|gp|tp|t[0-6]|s[0-9]|s1[01]|a[0-7])\\)$"
> 
> Since the 'zero' register is hardwired and not saved in pt_regs, will the
> kernel reject probes with a BAD_REG_NAME error when perf translates it
> to '%zero' for tracefs?
> 
> Should 'zero' be excluded from these regexes and instead treated as an
> unsupported constant-like argument to be skipped with SDT_ARG_SKIP?

Good point. The 'zero' register (x0) is not saved in pt_regs and is absent
from regoffset_table in arch/riscv/kernel/ptrace.c, so passing '%zero' to
tracefs would trigger BAD_REG_NAME. Fixed by removing 'zero' from both regexes
and letting it fall through to SDT_ARG_SKIP.

> [ ... ]
> 
> > +static int sdt_init_op_regex(void)
> > +{
> > + static int initialized;
> > + int ret = 0;
> > +
> > + if (initialized)
> > +  return 0;
> > +
> > + ret = regcomp(&sdt_op_regex1, SDT_OP_REGEX1, REG_EXTENDED);
> > + if (ret)
> > +  goto error;
> > +
> > + ret = regcomp(&sdt_op_regex2, SDT_OP_REGEX2, REG_EXTENDED);
> > + if (ret)
> > +  goto free_regex1;
> > +
> > + initialized = 1;
> > + return 0;
> > +
> > +free_regex1:
> > + regfree(&sdt_op_regex1);
> > +error:
> > + pr_debug4("Regex compilation error.\n");
> > + return ret;
> > +}
> 
> [ ... ]
> 
> > +int __perf_sdt_arg_parse_op_riscv(char *old_op, char **new_op)
> > +{
> > + int ret, new_len;
> > + regmatch_t rm[4];
> > + char prefix;
> 
> [ ... ]
> 
> > + ret = sdt_init_op_regex();
> > + if (ret < 0)
> > +  return ret;
> 
> Does this properly catch regex compilation failures? 
> 
> POSIX regcomp() returns a strictly positive integer (like REG_ESPACE)
> on error, so this condition will evaluate to false if regcomp() fails.
> 
> If the first regcomp() in sdt_init_op_regex() fails, could this lead to a
> crash when calling regexec() on the zero-initialized sdt_op_regex1 below?
> 
> Alternatively, if the second regcomp() fails, sdt_init_op_regex()
> calls regfree(&sdt_op_regex1) before returning the positive error code.
> 
> Could this cause a use-after-free when regexec() operates on the freed
> regex object?

Good catch. regcomp() returns a strictly positive integer on error, so
ret < 0 was indeed always false, causing regexec() to be called on an
uninitialized or freed regex object.

Fixed by returning -ret at the error path, so the caller's if (ret < 0)
check properly catches the failure and bails out before reaching regexec().

> > +
> > + if (!regexec(&sdt_op_regex1, old_op, 2, rm, 0)) {
> > +  /* REG --> %REG */
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260414124655.129463-1-cp0613@linux.alibaba.com?part=1

Thanks,
Pei