Re: [PATCH v2 04/11] KVM: x86/xen: Wait out memslot updates in the Xen attribute ioctls

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] kvm: unprotected kvm_memslots() dereference in kvm_xen_start_timer() due to missing srcu_read_lock()
--

commit 4fa2fbe5b2a708cc92e83f1efa33364d90021a44
Author: David Woodhouse <dwmw@amazon.co.uk>

KVM: x86/xen: Wait out memslot updates in the Xen attribute ioctls

Activating a shared_info, vcpu_info, pvclock or runstate cache can fail if it
races a memslot update: previously with -EFAULT, now with -EAGAIN. These
ioctls may sleep, so pass wait and let the activation ride the update out.
The overarching SRCU read sections around the activation calls are removed
to prevent deadlocks against memslot updates.

> diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
> index 74dad6789e428..cd1063d659cc5 100644
> --- a/arch/x86/kvm/xen.c
> +++ b/arch/x86/kvm/xen.c

[ ... ]

> @@ -937,7 +938,6 @@ int kvm_xen_vcpu_set_attr(struct kvm_vcpu *vcpu, struct kvm_xen_vcpu_attr *data)
>  	int idx, r = -ENOENT;
>  
>  	mutex_lock(&vcpu->kvm->arch.xen.xen_lock);
> -	idx = srcu_read_lock(&vcpu->kvm->srcu);
>  
>  	switch (data->type) {
>  	case KVM_XEN_VCPU_ATTR_TYPE_VCPU_INFO:

[Severity: High]
With the overarching SRCU read lock removed here, are there paths in the
switch statement that still require the lock but were missed?

For example, in the KVM_XEN_VCPU_ATTR_TYPE_TIMER case:

arch/x86/kvm/xen.c:kvm_xen_vcpu_set_attr() {
    ...
    case KVM_XEN_VCPU_ATTR_TYPE_TIMER:
        ...
        if (data->u.timer.port && data->u.timer.expires_ns)
            kvm_xen_start_timer(vcpu, data->u.timer.expires_ns, false);
    ...
}

This calls down into xen_get_guest_pvclock() and kvm_gpc_check(), which
then accesses kvm_memslots(gpc->kvm):

virt/kvm/pfncache.c:kvm_gpc_check() {
    struct kvm_memslots *slots = kvm_memslots(gpc->kvm);
    ...
}

Accessing kvm_memslots() triggers a srcu_dereference_check() that fails
without the SRCU lock held, and could result in a use-after-free if
kvm->memslots is freed concurrently.

Will this path now result in a lockdep splat ('suspicious RCU usage') and
potential use-after-free when a userspace process calls the
KVM_XEN_VCPU_SET_ATTR ioctl with KVM_XEN_VCPU_ATTR_TYPE_TIMER?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261014220508.518772-1-dwmw2@infradead.org?part=4