Re: [PATCH v10 2/9] riscv: add infrastructure for preemptible this_cpu operations

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] riscv: ptrace: wrong x0 offset assumption in
  riscv_pcpu_regs_fixup()
- [Low] riscv: entry: non-__always_inline helpers called from noinstr
  riscv_pcpu_regs_fixup()
--

commit dbc8e3fbcb707ac729e711e7321ce4a62f50767e
Author: Yunhui Cui <cuiyunhui@bytedance.com>

riscv: add infrastructure for preemptible this_cpu operations

This commit adds per-task and exception-frame tracking for registers used in
preemptible per-CPU operations so exception return can fix up the per-CPU
offset and address registers if the task migrated.

Link: https://lore.kernel.org/r/20261108213254.67805ef0@pumpkin
Link: https://lore.kernel.org/r/avG07pmpcLGkq5i-@xhacker

> diff --git a/arch/riscv/include/asm/ptrace.h b/arch/riscv/include/asm/ptrace.h
> index addc8188152f7..ceff8f63bc866 100644
> --- a/arch/riscv/include/asm/ptrace.h
> +++ b/arch/riscv/include/asm/ptrace.h
> @@ -53,6 +53,8 @@ struct pt_regs {
>  	unsigned long cause;
>  	/* a0 value before the syscall */
>  	unsigned long orig_a0;
> +	/* Active preemptible per-CPU operation, if any. */
> +	unsigned long pcpu_gprs;
>  };
>  
>  #define PTRACE_SYSEMU			0x1f
> @@ -145,6 +147,19 @@ static inline unsigned long regs_get_register(struct pt_regs *regs,
>  	return *(unsigned long *)((unsigned long)regs + offset);
>  }
>  
> +static inline void regs_set_register(struct pt_regs *regs,
> +				     unsigned int offset, unsigned long val)
> +{
> +	if (unlikely(offset > MAX_REG_OFFSET))
> +		return;
> +
> +	/* Register x0 is represented by the first, read-only slot. */
> +	if (offset)
> +		*(unsigned long *)((unsigned long)regs + offset) = val;
> +}
> +
> +void noinstr riscv_pcpu_regs_fixup(struct pt_regs *regs);

[Severity: Low]
Should regs_get_register() and regs_set_register() (or the GPR helpers called
by riscv_pcpu_regs_fixup()) be marked __always_inline?

riscv_pcpu_regs_fixup() is a noinstr function called from ret_from_exception
after irqentry_exit(), and under sanitizer configurations such as KASAN or
KCSAN the compiler may not inline plain static inline functions into a
noinstr caller.

[ ... ]

> diff --git a/arch/riscv/kernel/entry-common.c b/arch/riscv/kernel/entry-common.c
> new file mode 100644
> index 0000000000000..023d856b1679b
> --- /dev/null
> +++ b/arch/riscv/kernel/entry-common.c
> @@ -0,0 +1,32 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include <linux/bitfield.h>
> +#include <linux/compiler.h>
> +#include <linux/percpu.h>
> +
> +#include <asm/current.h>
> +#include <asm/ptrace.h>
> +
> +/*
> + * If the interrupted context had an active per-CPU operation, rewrite its
> + * saved offset and address registers for the CPU on which it will resume.
> + */
> +void noinstr riscv_pcpu_regs_fixup(struct pt_regs *regs)
> +{
> +	unsigned long pcpu_offset = __my_cpu_offset;
> +	unsigned long gprs = regs->pcpu_gprs;
> +	unsigned long pcp;
> +	unsigned int reg_pcp;
> +	unsigned int reg_off;
> +	unsigned int reg_addr;
> +
> +	reg_pcp = FIELD_GET(PCPU_GPR_PCP, gprs);
> +	reg_off = FIELD_GET(PCPU_GPR_OFF, gprs);
> +	reg_addr = FIELD_GET(PCPU_GPR_ADDR, gprs);
> +
> +	/* pt_regs offset zero represents x0, followed by x1 through x31. */
> +	pcp = regs_get_register(regs, reg_pcp * sizeof(unsigned long));
> +	regs_set_register(regs, reg_off * sizeof(unsigned long), pcpu_offset);
> +	regs_set_register(regs, reg_addr * sizeof(unsigned long),
> +			  pcp + pcpu_offset);
> +}

[Severity: Medium]
Does riscv_pcpu_regs_fixup() read regs->epc instead of 0 if reg_pcp is x0?

Offset 0 of struct pt_regs is epc rather than x0, and regs_get_register(regs,
0) returns regs->epc without special-casing offset 0.  By contrast,
regs_set_register() treats offset 0 as read-only x0, making it inconsistent
with regs_get_register() and regs_query_register_offset() for epc while
duplicating regs_get_gpr() and regs_set_gpr() in arch/riscv/mm/extable.c:

arch/riscv/mm/extable.c:regs_get_gpr() {
	if (unlikely(!offset || offset > MAX_REG_OFFSET))
		return 0;

	return *(unsigned long *)((unsigned long)regs + offset);
}

Sharing regs_get_gpr() and regs_set_gpr() (or providing dedicated GPR-index
helpers) would return 0 for x0 and keep the byte-offset regs_get_register()
API consistent.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112104149.38589-1-cuiyunhui@bytedance.com?part=2