Re: [PATCH v2 2/3] riscv: vector: allow non-preemptible kernel-mode vector with IRQs off

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Exported `get_cpu_vector_context` and `put_cpu_vector_context` dynamically evaluate `irqs_disabled()` to balance `local_bh_disable()` and `local_bh_enable()`. If a caller alters the IRQ state between the two calls, it will lead to preempt count corruption.
--

--- Patch [2]: [PATCH v2 2/3] riscv: vector: allow non-preemptible kernel-mode vector with IRQs off ---
commit cfe6cf4c78a9ea0fadcde3b4c39aafb48ebeaec6
Author: Andy Chiu <[email protected]>

riscv: vector: allow non-preemptible kernel-mode vector with IRQs off

This commit updates get_cpu_vector_context and put_cpu_vector_context to
conditionally disable and enable bottom halves based on whether interrupts
are disabled. It also exports these symbols so they can be used by the KVM
module, specifically for kvm_arch_vcpu_load and put.

> diff --git a/arch/riscv/kernel/kernel_mode_vector.c b/arch/riscv/kernel/kernel_mode_vector.c
> index 307ac369c3d4a..6627e1a82e19e 100644
> --- a/arch/riscv/kernel/kernel_mode_vector.c
> +++ b/arch/riscv/kernel/kernel_mode_vector.c
[ ... ]
> @@ -55,13 +55,16 @@ void get_cpu_vector_context(void)
>  	 * disable softirqs so it is impossible for softirqs to nest
>  	 * get_cpu_vector_context() when kernel is actively using Vector.
>  	 */
> -	if (!IS_ENABLED(CONFIG_PREEMPT_RT))
> -		local_bh_disable();
> -	else
> +	if (!IS_ENABLED(CONFIG_PREEMPT_RT)) {
> +		if (!irqs_disabled())
> +			local_bh_disable();
> +	} else {
>  		preempt_disable();
> +	}
>  
>  	riscv_v_start(RISCV_KERNEL_MODE_V);
>  }
> +EXPORT_SYMBOL_GPL(get_cpu_vector_context);
>  
>  /*
>   * Release the CPU vector context.
> @@ -74,11 +77,14 @@ void put_cpu_vector_context(void)
>  {
>  	riscv_v_stop(RISCV_KERNEL_MODE_V);
>  
> -	if (!IS_ENABLED(CONFIG_PREEMPT_RT))
> -		local_bh_enable();
> -	else
> +	if (!IS_ENABLED(CONFIG_PREEMPT_RT)) {
> +		if (!irqs_disabled())
> +			local_bh_enable();
> +	} else {

[Severity: High]
Does dynamically checking irqs_disabled() in both get_cpu_vector_context()
and put_cpu_vector_context() risk corrupting the preempt count if the IRQ
state changes between the two calls?

The commit message mentions these are intended for kvm_arch_vcpu_load() and
kvm_arch_vcpu_put(). If KVM calls vcpu_load() during sched_in with IRQs
enabled, get_cpu_vector_context() will call local_bh_disable().

If it later calls vcpu_put() during sched_out with IRQs disabled,
put_cpu_vector_context() will skip local_bh_enable().

Will this permanently leak the BH disable count on every context switch?

>  		preempt_enable();
> +	}
>  }
> +EXPORT_SYMBOL_GPL(put_cpu_vector_context);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2