Re: [PATCH v2 03/16] KVM: PPC: Book3S HV: XIVE: introduce a new capability KVM_CAP_PPC_IRQ_XIVE

From: David Gibson <david@gibson.dropbear.id.au>
To: "Cédric Le Goater" <clg@kaod.org>
Cc: kvm@vger.kernel.org, kvm-ppc@vger.kernel.org,
	Paul Mackerras <paulus@samba.org>,
	linuxppc-dev@lists.ozlabs.org
Subject: Re: [PATCH v2 03/16] KVM: PPC: Book3S HV: XIVE: introduce a new capability KVM_CAP_PPC_IRQ_XIVE
Date: Wed, 13 Mar 2019 15:05:42 +1100	[thread overview]
Message-ID: <20190313040542.GL9881@umbus.fritz.box> (raw)
In-Reply-To: <2f029721-0b20-2706-6627-0bac36afd03e@kaod.org>

[-- Attachment #1: Type: text/plain, Size: 2164 bytes --]

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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]