On Tue, Mar 12, 2019 at 03:03:25PM +0100, Cédric Le Goater wrote:
> On 2/25/19 1:35 AM, David Gibson wrote:
> > On Fri, Feb 22, 2019 at 12:28:27PM +0100, Cédric Le Goater wrote:
[snip]
> >> +int kvmppc_xive_native_connect_vcpu(struct kvm_device *dev,
> >> + struct kvm_vcpu *vcpu, u32 cpu)
> >> +{
> >> + struct kvmppc_xive *xive = dev->private;
> >> + struct kvmppc_xive_vcpu *xc;
> >> + int rc;
> >> +
> >> + pr_devel("native_connect_vcpu(cpu=%d)\n", cpu);
> >> +
> >> + if (dev->ops != &kvm_xive_native_ops) {
> >> + pr_devel("Wrong ops !\n");
> >> + return -EPERM;
> >> + }
> >> + if (xive->kvm != vcpu->kvm)
> >> + return -EPERM;
> >> + if (vcpu->arch.irq_type != KVMPPC_IRQ_DEFAULT)
> >> + return -EBUSY;
> >> + if (kvmppc_xive_find_server(vcpu->kvm, cpu)) {
> >
> > You haven't taken the kvm->lock yet, so couldn't a race mean a
> > duplicate server gets inserted after you make this check?
> >
> >> + pr_devel("Duplicate !\n");
> >> + return -EEXIST;
> >> + }
> >> + if (cpu >= KVM_MAX_VCPUS) {
> >> + pr_devel("Out of bounds !\n");
> >> + return -EINVAL;
> >> + }
> >> + xc = kzalloc(sizeof(*xc), GFP_KERNEL);
> >> + if (!xc)
> >> + return -ENOMEM;
> >> +
> >> + mutex_lock(&vcpu->kvm->lock);
> >> + vcpu->arch.xive_vcpu = xc;
> >
> > Similarly you don't verify this is NULL after taking the lock, so
> > couldn't another thread race and make a connect which gets clobbered
> > here?
>
> Yes. this is not very safe ... We need to clean up all the KVM device
> methods doing the connection of the presenter to the vCPU AFAICT.
> I will fix the XIVE native one for now.
>
> And also, this CPU parameter is useless. There is no reason to connect
> a vCPU from another vCPU.
Hmm.. I thought the point of the 'cpu' parameter (not a great name) is
that it lets userspace chose the guest visible irq server ID. I think
that's preferable to tying it to an existing cpu id, if possible.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
Attachment:
signature.asc
Description: PGP signature
