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

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Critical] Removing `!irqs_disabled()` from `may_use_simd()` allows hardirqs to clobber KVM guest vector state.
- [Low] Missing include for EXPORT_SYMBOL_FOR_KVM causes build failure.

Pre-existing issues:
- [High] `may_use_simd()` bypasses `RISCV_KERNEL_MODE_V` reentrancy protection when preemptive vector is enabled.
--

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

riscv: vector: allow non-preemptible kernel-mode vector with IRQs off
    
This commit updates get/put_cpu_vector_context to allow kvm_arch_vcpu_load
and put to be called safely under both irq off and regular process context.
It also exports both symbols for the KVM module.

> diff --git a/arch/riscv/include/asm/simd.h b/arch/riscv/include/asm/simd.h
> index adb50f3ec2057..678c8b97cd497 100644
> --- a/arch/riscv/include/asm/simd.h
> +++ b/arch/riscv/include/asm/simd.h
> @@ -44,12 +44,8 @@ static __must_check inline bool may_use_simd(void)
>  		if (!riscv_preempt_v_started(current))
>  			return true;

[Severity: High]
This is a pre-existing issue, but does this logic bypass the
RISCV_KERNEL_MODE_V reentrancy protection?

If a task such as a KVM thread calls get_cpu_vector_context() directly, it
sets RISCV_KERNEL_MODE_V but not RISCV_PREEMPT_V. If an interrupt occurs
during this non-preemptible vector block, riscv_preempt_v_started(current)
evaluates to false.

Would may_use_simd() then immediately return true and completely bypass the
RISCV_KERNEL_MODE_V check below, falsely permitting the interrupt handler to
clobber the active non-preemptible vector context?

>  	}
> -	/*
> -	 * Non-preemptible kernel-mode Vector temporarily disables bh. So we
> -	 * must not return true on irq_disabled(). Otherwise we would fail the
> -	 * lockdep check calling local_bh_enable()
> -	 */
> -	return !irqs_disabled() && !(riscv_v_flags() & RISCV_KERNEL_MODE_V);
> +
> +	return !(riscv_v_flags() & RISCV_KERNEL_MODE_V);
>  }

[Severity: Critical]
Does removing the !irqs_disabled() check allow hardirqs to clobber KVM
guest vector state?

Without a check preventing hardirqs (such as !in_hardirq() && !in_nmi()),
may_use_simd() can now return true when a host hardirq interrupts a running
KVM guest. The hardirq might use kernel_vector_begin(), which blindly
assumes the hardware vector registers belong to the host thread:

arch/riscv/kernel/kernel_mode_vector.c:kernel_vector_begin() {
    ...
    riscv_v_vstate_save(&current->thread.vstate, task_pt_regs(current));
    ...
}

Could this cause the guest's state to be saved into the host thread's user
space vstate, corrupting the guest's vector registers upon resume and
leaking guest state to the host?

> diff --git a/arch/riscv/kernel/kernel_mode_vector.c b/arch/riscv/kernel/kernel_mode_vector.c
> index 307ac369c3d4a..f76e52de11178 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_FOR_KVM(get_cpu_vector_context);

[Severity: Low]
Will this cause a build failure due to a missing include?

EXPORT_SYMBOL_FOR_KVM is used here, but it doesn't appear that
<linux/kvm_types.h> is included in this file or its dependencies.

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