Re: [PATCH v6 04/21] RISC-V: Define indirect CSR access helpers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Unsafe statement-expression macro definitions causing delayed evaluation and variable shadowing hazards.
--

commit 572b7555547c1b5bc07701482ba67d379f6a2177
Author: Atish Patra <atishp@rivosinc.com>

RISC-V: Define indirect CSR access helpers

This commit introduces helper macros to read, write, and perform WARL
operations on indirect CSRs. The macros use statement-expressions to handle
the multiple instructions required while disabling interrupts.

> diff --git a/arch/riscv/include/asm/csr_ind.h b/arch/riscv/include/asm/csr_ind.h
> new file mode 100644
> index 0000000000000..6fd7d44dc640a
> --- /dev/null
> +++ b/arch/riscv/include/asm/csr_ind.h
> @@ -0,0 +1,44 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (C) 2024 Rivos Inc.
> + */
> +
> +#ifndef _ASM_RISCV_CSR_IND_H
> +#define _ASM_RISCV_CSR_IND_H
> +
> +#include <linux/irqflags.h>
> +
> +#include <asm/csr.h>
> +
> +#define csr_ind_read(iregcsr, iselbase, iseloff) ({		\
> +	unsigned long __value = 0;				\
> +	unsigned long __flags;					\

[Severity: Medium]
Will these generic variable names (__value, __flags) cause shadowing issues if
the caller passes in arguments with the same names?

Evaluating macro arguments into unique local variables before entering the
IRQ-disabled region would avoid this regression.

> +	local_irq_save(__flags);				\
> +	csr_write(CSR_ISELECT, (iselbase) + (iseloff));		\
> +	__value = csr_read(iregcsr);				\
> +	local_irq_restore(__flags);				\
> +	__value;						\
> +})
> +
> +#define csr_ind_write(iregcsr, iselbase, iseloff, value) ({	\
> +	unsigned long __flags;					\
> +	local_irq_save(__flags);				\
> +	csr_write(CSR_ISELECT, (iselbase) + (iseloff));		\
> +	csr_write(iregcsr, (value));				\

[Severity: Medium]
Since value is evaluated here after CSR_ISELECT has been programmed, what
happens if a caller passes an expression that accesses another indirect CSR,
such as a nested csr_ind_read(), as the value argument?

It appears the nested call would overwrite CSR_ISELECT, causing the write to
silently corrupt the wrong hardware register. Evaluating arguments into local
variables before disabling interrupts would prevent this, and also avoid the
risk of evaluating expressions with side effects while interrupts are disabled.

> +	local_irq_restore(__flags);				\
> +})
> +
> +#define csr_ind_warl(iregcsr, iselbase, iseloff, warl_val) ({	\
> +	unsigned long __old_val = 0, __value = 0;		\
> +	unsigned long __flags;					\

[Severity: Medium]
Do these generic local variables risk shadowing caller arguments?

> +	local_irq_save(__flags);				\
> +	csr_write(CSR_ISELECT, (iselbase) + (iseloff));		\
> +	__old_val = csr_read(iregcsr);				\
> +	csr_write(iregcsr, (warl_val));				\

[Severity: Medium]
Is there a risk that evaluating warl_val here could overwrite CSR_ISELECT if
the caller provides an expression that performs another indirect CSR access?

> +	__value = csr_read(iregcsr);				\
> +	csr_write(iregcsr, __old_val);				\
> +	local_irq_restore(__flags);				\
> +	__value;						\
> +})
> +
> +#endif

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260608-counter_delegation-v6-0-285b72ed65a9@meta.com?part=4