Re: [PATCH v2] LoongArch: KVM: Advertise already-supported capabilities

Hi Bibo,

The Sashiko automated review of this patch flagged two pre-existing items
— neither is from this patch. Wanted to get your thoughts,

在 2026/8/5 14:06, [email protected] 写道:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] The unsafe user-access macros (`unsafe_put_user()` and `unsafe_get_user()`) are used without the required `user_access_begin()` and `user_access_end()` guards, violating the kernel API contract.
The steal-time code uses unsafe_put_user()/unsafe_get_user() without
   the usual user_access_begin()/end() around them. On LoongArch it
   doesn't actually cause any trouble — there's no hardware user-access
   protection (user_access_begin() just resolves to access_ok()),
   __put_user is a plain store, and the ghc->hva address is already
   validated by kvm_gfn_to_hva_cache_init(), so the access_ok() is
   redundant anyway. So I see it more as a convention inconsistency /
   style nit than a real bug. I'd be happy to put together a small
   cleanup wrapping those in user_access_begin()/end() (or switching to
   kvm_write_guest_cached()). Does that sound like just cleanup to you,
   and would you want it?
> - [High] A lockless read-modify-write on the shared VM state `kvm->arch.pv_features` allows userspace to bypass the validation check that requires all vCPUs to share identical PV features.
The check-then-set on kvm->arch.pv_features in cpucfg_set_attr() is a
   genuine TOCTOU — again pre-existing, unrelated to this patch. It's not
   really reachable in practice: 0/3000 with two vCPUs hammering it
   concurrently, though a debug widening of the window does reproduce it
   every time (3000/3000). It'd basically need a userspace firing
   KVM_SET_DEVICE_ATTR with different values on two vCPUs at once. The
   odds are low, but I do think edge cases like this are still worth
   fixing — happy to send a separate patch using a mutex or cmpxchg.

Neither of these affects v2 itself, so I'd leave it as-is.

Thanks,
Tao
> --
> 
> --- Patch [1]: [PATCH v2] LoongArch: KVM: Advertise already-supported capabilities ---
> commit 91e3758510b96f271c2589373d1340c246fab45a
> Author: Tao Cui <[email protected]>
> 
> LoongArch: KVM: Advertise already-supported capabilities
>     
> This commit advertises KVM_CAP_STEAL_TIME and KVM_CAP_VCPU_ATTRIBUTES on
> LoongArch, making existing features discoverable to userspace.
> 
>> diff --git a/arch/loongarch/kvm/vm.c b/arch/loongarch/kvm/vm.c
>> index 1317c718f896a..0a51931d6f6e0 100644
>> --- a/arch/loongarch/kvm/vm.c
>> +++ b/arch/loongarch/kvm/vm.c
>> @@ -122,6 +122,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
>>  	case KVM_CAP_IOEVENTFD:
>>  	case KVM_CAP_MP_STATE:
>>  	case KVM_CAP_SET_GUEST_DEBUG:
>> +	case KVM_CAP_VCPU_ATTRIBUTES:
> 
> [Severity: High]
> This is a pre-existing issue, but does advertising KVM_CAP_VCPU_ATTRIBUTES
> expose a data race in kvm_loongarch_cpucfg_set_attr()?
> 
> Looking at arch/loongarch/kvm/vcpu.c, there is a lockless read-modify-write
> sequence on the shared VM state:
> 
> arch/loongarch/kvm/vcpu.c:kvm_loongarch_cpucfg_set_attr() {
> ...
> 	/* All vCPUs need set the same PV features */
> 	if ((kvm->arch.pv_features & LOONGARCH_PV_FEAT_UPDATED)
> 			&& ((kvm->arch.pv_features & valid) != val))
> 		return -EINVAL;
> 	kvm->arch.pv_features = val | LOONGARCH_PV_FEAT_UPDATED;
> ...
> }
> 
> If multiple threads concurrently call KVM_SET_DEVICE_ATTR on different vCPUs,
> can they bypass this validation check by both reading before either writes,
> resulting in divergent configurations being silently applied globally instead
> of returning -EINVAL?
> 
>>  		r = 1;
>>  		break;
>>  	case KVM_CAP_NR_VCPUS:
> 
> [ ... ]
> 
>> @@ -136,6 +137,9 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
>>  	case KVM_CAP_NR_MEMSLOTS:
>>  		r = KVM_USER_MEM_SLOTS;
>>  		break;
>> +	case KVM_CAP_STEAL_TIME:
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but does advertising
> KVM_CAP_STEAL_TIME expose an issue with unsafe memory accesses?
> 
> In arch/loongarch/kvm/vcpu.c, unsafe_put_user() is used without the required
> user_access_begin() and user_access_end() guards:
> 
> arch/loongarch/kvm/vcpu.c:kvm_update_stolen_time() {
> ...
> 	unsafe_put_user(0, &st->preempted, out);
> ...
> }
> 
> And similarly in kvm_vcpu_set_pv_preempted():
> 
> arch/loongarch/kvm/vcpu.c:kvm_vcpu_set_pv_preempted() {
> ...
> 	unsafe_put_user(KVM_VCPU_PREEMPTED, &st->preempted, out);
> ...
> }
> 
> Since omitting user_access_begin() skips the access_ok() check at the access
> site, could this cause a kernel fault if hardware-enforced user access
> protections are introduced?
> 
>> +		r = kvm_pvtime_supported();
>> +		break;
>>  	default:
>>  		r = 0;
>>  		break;
>