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

On 2026/8/5 下午4:28, Tao Cui wrote:
> 
> 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.
Happy to see the patch.

It is design problem to set pv_features per-VM, there is concurrent 
access issue and it can only be set for one time.

Regards
Bibo Mao
> 
> 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;
>>