Re: [PATCH v3 11/11] s390/percpu: Rework to simplify percpu_entry() and percpu_exit()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] s390/percpu: GCC can allocate %r0 for offset register causing memory corruption
--

commit 7d2dae81caa8a6021304f844d79c9291c54c5a8b
Author: Heiko Carstens <hca@linux.ibm.com>

s390/percpu: Rework to simplify percpu_entry() and percpu_exit()

This commit reworks s390 percpu code sections to use a new register encoding
strategy similar to arm64. It removes the need to check specific instructions
during interrupts by using a register pair to encode the percpu variable
address and offset, adjusting them in percpu_exit() upon CPU migration.

Link: https://lore.kernel.org/all/20260908151741.394589-1-mark.rutland@arm.com/

> diff --git a/arch/s390/include/asm/percpu.h b/arch/s390/include/asm/percpu.h
> index 46368131b932b..db30f5ac40427 100644
> --- a/arch/s390/include/asm/percpu.h
> +++ b/arch/s390/include/asm/percpu.h
[ ... ]
> +#define __PCPU_LA(regptr, regoff, regpcp)					\
> +	"	la	" regptr ",0(" regoff "," regpcp ")\n"

[Severity: Critical]
Does this code safely handle the case where regoff is assigned to %r0?
The la (load address) instruction treats %r0 as 0 when used as an index or
base register. If regoff evaluates to %r0, the instruction would effectively
ignore the percpu offset entirely. This could cause subsequent atomic
operations to modify the base .data..percpu variable instead of the current
CPU's local instance.

> +
> +#define __PCPU_CALC_REGVAL(regpcp, regoff)					\
> +	"(" regpcp " << " __stringify(PCPU_REG_PCP_SHIFT) ") |"			\
> +	"(" regoff " << " __stringify(PCPU_REG_OFF_SHIFT) ")"
> +
> +#define __PCPU_CHECK_REGS(regpcp, regoff, regptr)				\
> +	".if " regoff " & 1\n"							\
> +	"	.error \"Percpu offset register must be even\"\n"		\
> +	".endif\n"								\
> +	".if " regptr " != " regoff " + 1\n"					\
> +	"	.error \"Percpu pointer and offset register must be a pair\"\n" \
> +	".endif\n"								\
> +	".if " regptr " == 0\n"							\
> +	"	.error \"Percpu pointer register must be non-zero\"\n"		\

[Severity: Critical]
Does this assembler check protect against the %r0 base/index semantic
described above? It looks like it checks regptr instead of regoff. If the
compiler allocates the %r0 and %r1 pair, regptr will be %r1, which silently
bypasses this check. Should this be checking if regoff is 0 instead?

[ ... ]
> @@ -158,25 +194,27 @@ do {										\
>  	if (__builtin_constant_p(val__) &&					\
>  	    ((szcast)val__ > -129) && ((szcast)val__ < 128)) {			\
>  		asm volatile(							\
> -			__PCPU_BEGIN("%[lcreg]","%[lcoff]","%[ptr__]")		\
> -			op2 "   0(%[ptr__]),%[val__]\n"				\
> +			__PCPU_BEGIN("%[lcreg]","%[lcoff]","%[ptr__]",		\
> +				      "%[pair__]","%N[pair__]")			\
> +			op2 "   0(%N[pair__]),%[val__]\n"			\
>  			__PCPU_END("%[lcreg]")					\
> -			: [ptr__] "+&a" (ptr__), "+m" (*ptr__),			\
> +			: [pair__] "=&d" (rp__.pair), "+m" (*ptr__),		\

[Severity: Critical]
Can GCC allocate %r0 for the pair here? The "=&d" constraint allows the
compiler to select any general-purpose register, meaning it could allocate
the %r0 and %r1 pair for rp__.pair. Would using "=&a" (address registers)
instead prevent %r0 allocation and avoid the code generation issue with the
la instruction above? This seems to apply to all modified percpu macros in
this file using the "=&d" constraint for the register pair.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260921084005.4022574-1-hca@linux.ibm.com?part=11