[patch 0/3] ioctl fixes

[patch 0/3] ioctl fixes

[Marcelo Tosatti]

See individual patches for details.

[patch 1/3] KVM: x86: disallow multiple KVM_CREATE_IRQCHIP

[Marcelo Tosatti]

[-- Attachment #1: irqchip-create --]
[-- Type: text/plain, Size: 766 bytes --]

Otherwise kvm will leak memory on multiple KVM_CREATE_IRQCHIP.

CC: stable@kernel.org
Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

Index: kvm/arch/x86/kvm/x86.c
===================================================================
--- kvm.orig/arch/x86/kvm/x86.c
+++ kvm/arch/x86/kvm/x86.c
@@ -2285,6 +2285,9 @@ long kvm_arch_vm_ioctl(struct file *filp
 			goto out;
 		break;
 	case KVM_CREATE_IRQCHIP:
+		r = -EEXIST;
+		if (kvm->arch.vpic)
+			goto out;
 		r = -ENOMEM;
 		kvm->arch.vpic = kvm_create_pic(kvm);
 		if (kvm->arch.vpic) {
@@ -2300,6 +2303,8 @@ long kvm_arch_vm_ioctl(struct file *filp
 		if (r) {
 			kfree(kvm->arch.vpic);
 			kfree(kvm->arch.vioapic);
+			kvm->arch.vpic = NULL;
+			kvm->arch.vioapic = NULL;
 			goto out;
 		}
 		break;

[patch 2/3] KVM: x86: disallow KVM_{SET,GET}_LAPIC without in kernel irqchip

[Marcelo Tosatti]

[-- Attachment #1: get-set-lapic --]
[-- Type: text/plain, Size: 817 bytes --]

Otherwise kvm might attempt to dereference a NULL pointer.

CC: stable@kernel.org
Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

Index: kvm/arch/x86/kvm/x86.c
===================================================================
--- kvm.orig/arch/x86/kvm/x86.c
+++ kvm/arch/x86/kvm/x86.c
@@ -1815,6 +1815,9 @@ long kvm_arch_vcpu_ioctl(struct file *fi
 
 	switch (ioctl) {
 	case KVM_GET_LAPIC: {
+		r = -EINVAL;
+		if (!irqchip_in_kernel(vcpu->kvm))
+			goto out;
 		lapic = kzalloc(sizeof(struct kvm_lapic_state), GFP_KERNEL);
 
 		r = -ENOMEM;
@@ -1830,6 +1833,9 @@ long kvm_arch_vcpu_ioctl(struct file *fi
 		break;
 	}
 	case KVM_SET_LAPIC: {
+		r = -EINVAL;
+		if (!irqchip_in_kernel(vcpu->kvm))
+			goto out;
 		lapic = kmalloc(sizeof(struct kvm_lapic_state), GFP_KERNEL);
 		r = -ENOMEM;
 		if (!lapic)

[patch 3/3] KVM: only clear irq_source_id if irqchip is present

[Marcelo Tosatti]

[-- Attachment #1: free-irqsource-id --]
[-- Type: text/plain, Size: 911 bytes --]

Otherwise kvm might attempt to dereference a NULL pointer.

CC: stable@kernel.org
Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

Index: kvm/virt/kvm/irq_comm.c
===================================================================
--- kvm.orig/virt/kvm/irq_comm.c
+++ kvm/virt/kvm/irq_comm.c
@@ -243,6 +243,10 @@ void kvm_free_irq_source_id(struct kvm *
 		printk(KERN_ERR "kvm: IRQ source ID out of range!\n");
 		goto unlock;
 	}
+	clear_bit(irq_source_id, &kvm->arch.irq_sources_bitmap);
+	if (!irqchip_in_kernel(kvm))
+		goto unlock;
+
 	for (i = 0; i < KVM_IOAPIC_NUM_PINS; i++) {
 		clear_bit(irq_source_id, &kvm->arch.vioapic->irq_states[i]);
 		if (i >= 16)
@@ -251,7 +255,6 @@ void kvm_free_irq_source_id(struct kvm *
 		clear_bit(irq_source_id, &pic_irqchip(kvm)->irq_states[i]);
 #endif
 	}
-	clear_bit(irq_source_id, &kvm->arch.irq_sources_bitmap);
 unlock:
 	mutex_unlock(&kvm->irq_lock);
 }

Re: [patch 1/3] KVM: x86: disallow multiple KVM_CREATE_IRQCHIP

[Michael S. Tsirkin]

On Tue, Oct 27, 2009 at 01:10:43PM -0200, Marcelo Tosatti wrote:
> Otherwise kvm will leak memory on multiple KVM_CREATE_IRQCHIP.
> 
> CC: stable@kernel.org
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> 
> Index: kvm/arch/x86/kvm/x86.c
> ===================================================================
> --- kvm.orig/arch/x86/kvm/x86.c
> +++ kvm/arch/x86/kvm/x86.c
> @@ -2285,6 +2285,9 @@ long kvm_arch_vm_ioctl(struct file *filp
>  			goto out;
>  		break;
>  	case KVM_CREATE_IRQCHIP:
> +		r = -EEXIST;
> +		if (kvm->arch.vpic)
> +			goto out;
>  		r = -ENOMEM;
>  		kvm->arch.vpic = kvm_create_pic(kvm);
>  		if (kvm->arch.vpic) {
> @@ -2300,6 +2303,8 @@ long kvm_arch_vm_ioctl(struct file *filp
>  		if (r) {
>  			kfree(kvm->arch.vpic);
>  			kfree(kvm->arch.vioapic);
> +			kvm->arch.vpic = NULL;
> +			kvm->arch.vioapic = NULL;
>  			goto out;
>  		}
>  		break;

Is there a lock that protects this structure?
Can memory leak still occur if multiple threads call
KVM_CREATE_IRQCHIP in parallel?

> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch 2/3] KVM: x86: disallow KVM_{SET,GET}_LAPIC without in kernel irqchip

[Michael S. Tsirkin]

On Tue, Oct 27, 2009 at 01:10:44PM -0200, Marcelo Tosatti wrote:
> Otherwise kvm might attempt to dereference a NULL pointer.
> 
> CC: stable@kernel.org
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> 
> Index: kvm/arch/x86/kvm/x86.c
> ===================================================================
> --- kvm.orig/arch/x86/kvm/x86.c
> +++ kvm/arch/x86/kvm/x86.c
> @@ -1815,6 +1815,9 @@ long kvm_arch_vcpu_ioctl(struct file *fi
>  
>  	switch (ioctl) {
>  	case KVM_GET_LAPIC: {
> +		r = -EINVAL;
> +		if (!irqchip_in_kernel(vcpu->kvm))
> +			goto out;
>  		lapic = kzalloc(sizeof(struct kvm_lapic_state), GFP_KERNEL);
>  
>  		r = -ENOMEM;
> @@ -1830,6 +1833,9 @@ long kvm_arch_vcpu_ioctl(struct file *fi
>  		break;
>  	}
>  	case KVM_SET_LAPIC: {
> +		r = -EINVAL;
> +		if (!irqchip_in_kernel(vcpu->kvm))
> +			goto out;
>  		lapic = kmalloc(sizeof(struct kvm_lapic_state), GFP_KERNEL);
>  		r = -ENOMEM;
>  		if (!lapic)
> 

Can the value of irqchip_in_kernel be changed by userspace
after we have checked it? If yes, this check won't help ...


> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch 2/3] KVM: x86: disallow KVM_{SET,GET}_LAPIC without in kernel irqchip

[Avi Kivity]

On 10/27/2009 07:50 PM, Michael S. Tsirkin wrote:
> Can the value of irqchip_in_kernel be changed by userspace
> after we have checked it? If yes, this check won't help ...
>    

A change from false to true is possible, but not the reverse.

-- 
error compiling committee.c: too many arguments to function

Re: [patch 2/3] KVM: x86: disallow KVM_{SET,GET}_LAPIC without in kernel irqchip

[Michael S. Tsirkin]

On Wed, Oct 28, 2009 at 11:32:00AM +0200, Avi Kivity wrote:
> On 10/27/2009 07:50 PM, Michael S. Tsirkin wrote:
>> Can the value of irqchip_in_kernel be changed by userspace
>> after we have checked it? If yes, this check won't help ...
>>    
>
> A change from false to true is possible, but not the reverse.

Hmm. If we want to rely on this, we have to play with
memory barriers to write/read it. Doable, but hard to get right.
Can we always have the irqchip object exist?
It doesn't use a lot of memory, does it?
Maybe have it inline, save an extra indirection on
fastpath ...

> -- 
> error compiling committee.c: too many arguments to function

Re: [patch 2/3] KVM: x86: disallow KVM_{SET,GET}_LAPIC without in kernel irqchip

[Gleb Natapov]

On Wed, Oct 28, 2009 at 12:20:42PM +0200, Michael S. Tsirkin wrote:
> On Wed, Oct 28, 2009 at 11:32:00AM +0200, Avi Kivity wrote:
> > On 10/27/2009 07:50 PM, Michael S. Tsirkin wrote:
> >> Can the value of irqchip_in_kernel be changed by userspace
> >> after we have checked it? If yes, this check won't help ...
> >>    
> >
> > A change from false to true is possible, but not the reverse.
> 
> Hmm. If we want to rely on this, we have to play with
> memory barriers to write/read it. Doable, but hard to get right.
Why? If userspace is so racy that it tries to use vpic while other
thread creates it let it fail and burn in hell.

> Can we always have the irqchip object exist?
> It doesn't use a lot of memory, does it?
> Maybe have it inline, save an extra indirection on
> fastpath ...
> 
> > -- 
> > error compiling committee.c: too many arguments to function
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
			Gleb.

Re: [patch 2/3] KVM: x86: disallow KVM_{SET,GET}_LAPIC without in kernel irqchip

[Michael S. Tsirkin]

On Wed, Oct 28, 2009 at 12:30:39PM +0200, Gleb Natapov wrote:
> On Wed, Oct 28, 2009 at 12:20:42PM +0200, Michael S. Tsirkin wrote:
> > On Wed, Oct 28, 2009 at 11:32:00AM +0200, Avi Kivity wrote:
> > > On 10/27/2009 07:50 PM, Michael S. Tsirkin wrote:
> > >> Can the value of irqchip_in_kernel be changed by userspace
> > >> after we have checked it? If yes, this check won't help ...
> > >>    
> > >
> > > A change from false to true is possible, but not the reverse.
> > 
> > Hmm. If we want to rely on this, we have to play with
> > memory barriers to write/read it. Doable, but hard to get right.
> Why? If userspace is so racy that it tries to use vpic while other
> thread creates it let it fail and burn in hell.

Yes but reading uninitialized memory in kernel can
lead to host kernel crashes.

> > Can we always have the irqchip object exist?
> > It doesn't use a lot of memory, does it?
> > Maybe have it inline, save an extra indirection on
> > fastpath ...
> > 
> > > -- 
> > > error compiling committee.c: too many arguments to function
> > --
> > To unsubscribe from this list: send the line "unsubscribe kvm" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> --
> 			Gleb.

Re: [patch 2/3] KVM: x86: disallow KVM_{SET,GET}_LAPIC without in kernel irqchip

[Avi Kivity]

On 10/28/2009 12:20 PM, Michael S. Tsirkin wrote:
>
> Hmm. If we want to rely on this, we have to play with
> memory barriers to write/read it. Doable, but hard to get right.
> Can we always have the irqchip object exist?
> It doesn't use a lot of memory, does it?
> Maybe have it inline, save an extra indirection on
> fastpath ...
>    

Good idea.  Note we still need to check irqchip_in_kernel() for 
correctness (for example hlt behaviour changes).

-- 
error compiling committee.c: too many arguments to function

Re: [patch 2/3] KVM: x86: disallow KVM_{SET,GET}_LAPIC without in kernel irqchip

[Gleb Natapov]

On Wed, Oct 28, 2009 at 12:32:24PM +0200, Michael S. Tsirkin wrote:
> On Wed, Oct 28, 2009 at 12:30:39PM +0200, Gleb Natapov wrote:
> > On Wed, Oct 28, 2009 at 12:20:42PM +0200, Michael S. Tsirkin wrote:
> > > On Wed, Oct 28, 2009 at 11:32:00AM +0200, Avi Kivity wrote:
> > > > On 10/27/2009 07:50 PM, Michael S. Tsirkin wrote:
> > > >> Can the value of irqchip_in_kernel be changed by userspace
> > > >> after we have checked it? If yes, this check won't help ...
> > > >>    
> > > >
> > > > A change from false to true is possible, but not the reverse.
> > > 
> > > Hmm. If we want to rely on this, we have to play with
> > > memory barriers to write/read it. Doable, but hard to get right.
> > Why? If userspace is so racy that it tries to use vpic while other
> > thread creates it let it fail and burn in hell.
> 
> Yes but reading uninitialized memory in kernel can
> lead to host kernel crashes.
> 
A you concerned that arch.vpic pointer assignment will be seen before vpic
initialization? Yes this theoretically possible.

> > > Can we always have the irqchip object exist?
> > > It doesn't use a lot of memory, does it?
> > > Maybe have it inline, save an extra indirection on
> > > fastpath ...
> > > 
> > > > -- 
> > > > error compiling committee.c: too many arguments to function
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe kvm" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> > --
> > 			Gleb.

--
			Gleb.

[patch 0/3] ioctl fixes v2

[Marcelo Tosatti]

Addressing comments.

[patch 1/3] KVM: x86: disallow multiple KVM_CREATE_IRQCHIP

[Marcelo Tosatti]

[-- Attachment #1: irqchip-create --]
[-- Type: text/plain, Size: 1414 bytes --]

Otherwise kvm will leak memory on multiple KVM_CREATE_IRQCHIP. 
Also serialize multiple accesses with kvm->lock.

Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

Index: kvm/arch/x86/kvm/x86.c
===================================================================
--- kvm.orig/arch/x86/kvm/x86.c
+++ kvm/arch/x86/kvm/x86.c
@@ -2362,25 +2362,38 @@ long kvm_arch_vm_ioctl(struct file *filp
 		if (r)
 			goto out;
 		break;
-	case KVM_CREATE_IRQCHIP:
+	case KVM_CREATE_IRQCHIP: {
+		struct kvm_pic *vpic;
+
+		mutex_lock(&kvm->lock);
+		r = -EEXIST;
+		if (kvm->arch.vpic)
+			goto create_irqchip_unlock;
 		r = -ENOMEM;
-		kvm->arch.vpic = kvm_create_pic(kvm);
-		if (kvm->arch.vpic) {
+		vpic = kvm_create_pic(kvm);
+		if (vpic) {
 			r = kvm_ioapic_init(kvm);
 			if (r) {
-				kfree(kvm->arch.vpic);
-				kvm->arch.vpic = NULL;
-				goto out;
+				kfree(vpic);
+				goto create_irqchip_unlock;
 			}
 		} else
-			goto out;
+			goto create_irqchip_unlock;
+		kvm->arch.vpic = vpic;
+		smp_wmb();
 		r = kvm_setup_default_irq_routing(kvm);
 		if (r) {
+			mutex_lock(&kvm->irq_lock);
 			kfree(kvm->arch.vpic);
 			kfree(kvm->arch.vioapic);
-			goto out;
+			kvm->arch.vpic = NULL;
+			kvm->arch.vioapic = NULL;
+			mutex_unlock(&kvm->irq_lock);
 		}
+	create_irqchip_unlock:
+		mutex_unlock(&kvm->lock);
 		break;
+	}
 	case KVM_CREATE_PIT:
 		u.pit_config.flags = KVM_PIT_SPEAKER_DUMMY;
 		goto create_pit;

[patch 2/3] KVM: x86: disallow KVM_{SET,GET}_LAPIC without allocated in-kernel lapic

[Marcelo Tosatti]

[-- Attachment #1: get-set-lapic --]
[-- Type: text/plain, Size: 769 bytes --]

Otherwise kvm might attempt to dereference a NULL pointer.

Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

Index: kvm/arch/x86/kvm/x86.c
===================================================================
--- kvm.orig/arch/x86/kvm/x86.c
+++ kvm/arch/x86/kvm/x86.c
@@ -1893,6 +1893,9 @@ long kvm_arch_vcpu_ioctl(struct file *fi
 
 	switch (ioctl) {
 	case KVM_GET_LAPIC: {
+		r = -EINVAL;
+		if (!vcpu->arch.apic)
+			goto out;
 		lapic = kzalloc(sizeof(struct kvm_lapic_state), GFP_KERNEL);
 
 		r = -ENOMEM;
@@ -1908,6 +1911,9 @@ long kvm_arch_vcpu_ioctl(struct file *fi
 		break;
 	}
 	case KVM_SET_LAPIC: {
+		r = -EINVAL;
+		if (!vcpu->arch.apic)
+			goto out;
 		lapic = kmalloc(sizeof(struct kvm_lapic_state), GFP_KERNEL);
 		r = -ENOMEM;
 		if (!lapic)

[patch 3/3] KVM: only clear irq_source_id if irqchip is present

[Marcelo Tosatti]

[-- Attachment #1: free-irqsource-id --]
[-- Type: text/plain, Size: 889 bytes --]

Otherwise kvm might attempt to dereference a NULL pointer.

Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

Index: kvm/virt/kvm/irq_comm.c
===================================================================
--- kvm.orig/virt/kvm/irq_comm.c
+++ kvm/virt/kvm/irq_comm.c
@@ -243,6 +243,10 @@ void kvm_free_irq_source_id(struct kvm *
 		printk(KERN_ERR "kvm: IRQ source ID out of range!\n");
 		goto unlock;
 	}
+	clear_bit(irq_source_id, &kvm->arch.irq_sources_bitmap);
+	if (!irqchip_in_kernel(kvm))
+		goto unlock;
+
 	for (i = 0; i < KVM_IOAPIC_NUM_PINS; i++) {
 		clear_bit(irq_source_id, &kvm->arch.vioapic->irq_states[i]);
 		if (i >= 16)
@@ -251,7 +255,6 @@ void kvm_free_irq_source_id(struct kvm *
 		clear_bit(irq_source_id, &pic_irqchip(kvm)->irq_states[i]);
 #endif
 	}
-	clear_bit(irq_source_id, &kvm->arch.irq_sources_bitmap);
 unlock:
 	mutex_unlock(&kvm->irq_lock);
 }

Re: [patch 1/3] KVM: x86: disallow multiple KVM_CREATE_IRQCHIP

[Michael S. Tsirkin]

On Wed, Oct 28, 2009 at 06:42:38PM -0200, Marcelo Tosatti wrote:
> Otherwise kvm will leak memory on multiple KVM_CREATE_IRQCHIP. 
> Also serialize multiple accesses with kvm->lock.
> 
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> 
> Index: kvm/arch/x86/kvm/x86.c
> ===================================================================
> --- kvm.orig/arch/x86/kvm/x86.c
> +++ kvm/arch/x86/kvm/x86.c
> @@ -2362,25 +2362,38 @@ long kvm_arch_vm_ioctl(struct file *filp
>  		if (r)
>  			goto out;
>  		break;
> -	case KVM_CREATE_IRQCHIP:
> +	case KVM_CREATE_IRQCHIP: {
> +		struct kvm_pic *vpic;
> +
> +		mutex_lock(&kvm->lock);
> +		r = -EEXIST;
> +		if (kvm->arch.vpic)
> +			goto create_irqchip_unlock;
>  		r = -ENOMEM;
> -		kvm->arch.vpic = kvm_create_pic(kvm);
> -		if (kvm->arch.vpic) {
> +		vpic = kvm_create_pic(kvm);
> +		if (vpic) {
>  			r = kvm_ioapic_init(kvm);
>  			if (r) {
> -				kfree(kvm->arch.vpic);
> -				kvm->arch.vpic = NULL;
> -				goto out;
> +				kfree(vpic);
> +				goto create_irqchip_unlock;
>  			}
>  		} else
> -			goto out;
> +			goto create_irqchip_unlock;
> +		kvm->arch.vpic = vpic;
> +		smp_wmb();

Hmm, I think we want the reverse order:
		smp_wmb();
		kvm->arch.vpic = vpic;

The point is preventing vpic pointer
from being written to memory before vpic data itself.
Right?

BTW, the reason that we have wmb here but no read barrier anywhere is
that this code is x86 specific and reads are not reordered across a
dependency on x86. Writes are also not reordered on x86, so this really
acts as a compiler barrier, but I agree it's better to be portable.  So
maybe, for documentation purposes, we should add read_barrier_depends
within irqchip_in_kernel()?

>  		r = kvm_setup_default_irq_routing(kvm);
>  		if (r) {
> +			mutex_lock(&kvm->irq_lock);
>  			kfree(kvm->arch.vpic);
>  			kfree(kvm->arch.vioapic);
> -			goto out;
> +			kvm->arch.vpic = NULL;
> +			kvm->arch.vioapic = NULL;
> +			mutex_unlock(&kvm->irq_lock);
>  		}
> +	create_irqchip_unlock:
> +		mutex_unlock(&kvm->lock);
>  		break;
> +	}
>  	case KVM_CREATE_PIT:
>  		u.pit_config.flags = KVM_PIT_SPEAKER_DUMMY;
>  		goto create_pit;
> 

Re: [patch 1/3] KVM: x86: disallow multiple KVM_CREATE_IRQCHIP

[Marcelo Tosatti]

On Thu, Oct 29, 2009 at 11:32:48AM +0200, Michael S. Tsirkin wrote:
> On Wed, Oct 28, 2009 at 06:42:38PM -0200, Marcelo Tosatti wrote:
> > Otherwise kvm will leak memory on multiple KVM_CREATE_IRQCHIP. 
> > Also serialize multiple accesses with kvm->lock.
> > 
> > Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> > 
> > Index: kvm/arch/x86/kvm/x86.c
> > ===================================================================
> > --- kvm.orig/arch/x86/kvm/x86.c
> > +++ kvm/arch/x86/kvm/x86.c
> > @@ -2362,25 +2362,38 @@ long kvm_arch_vm_ioctl(struct file *filp
> >  		if (r)
> >  			goto out;
> >  		break;
> > -	case KVM_CREATE_IRQCHIP:
> > +	case KVM_CREATE_IRQCHIP: {
> > +		struct kvm_pic *vpic;
> > +
> > +		mutex_lock(&kvm->lock);
> > +		r = -EEXIST;
> > +		if (kvm->arch.vpic)
> > +			goto create_irqchip_unlock;
> >  		r = -ENOMEM;
> > -		kvm->arch.vpic = kvm_create_pic(kvm);
> > -		if (kvm->arch.vpic) {
> > +		vpic = kvm_create_pic(kvm);
> > +		if (vpic) {
> >  			r = kvm_ioapic_init(kvm);
> >  			if (r) {
> > -				kfree(kvm->arch.vpic);
> > -				kvm->arch.vpic = NULL;
> > -				goto out;
> > +				kfree(vpic);
> > +				goto create_irqchip_unlock;
> >  			}
> >  		} else
> > -			goto out;
> > +			goto create_irqchip_unlock;
> > +		kvm->arch.vpic = vpic;
> > +		smp_wmb();
> 
> Hmm, I think we want the reverse order:
> 		smp_wmb();
> 		kvm->arch.vpic = vpic;
> 
> The point is preventing vpic pointer
> from being written to memory before vpic data itself.
> Right?

Point is preventing arch.vpic assignment (and everything before) from
reordering with kvm_setup_default_irq_routing (you cannot have a present
irq route without arch.vioapic or arch.vpic set, since kvm_set_irq
assumes they are present).

But, now that you say, we also want a smp_wmb before vpic assignment so
the irqchip_in_kernel() test is safe (that is, everything including
vioapic is in place before irqchip_in_kernel() can succeed).

I'll resend, thanks.

> BTW, the reason that we have wmb here but no read barrier anywhere is
> that this code is x86 specific and reads are not reordered across a
> dependency on x86. Writes are also not reordered on x86, so this really
> acts as a compiler barrier, but I agree it's better to be portable.  So
> maybe, for documentation purposes, we should add read_barrier_depends
> within irqchip_in_kernel()?
> 
> >  		r = kvm_setup_default_irq_routing(kvm);
> >  		if (r) {
> > +			mutex_lock(&kvm->irq_lock);
> >  			kfree(kvm->arch.vpic);
> >  			kfree(kvm->arch.vioapic);
> > -			goto out;
> > +			kvm->arch.vpic = NULL;
> > +			kvm->arch.vioapic = NULL;
> > +			mutex_unlock(&kvm->irq_lock);
> >  		}
> > +	create_irqchip_unlock:
> > +		mutex_unlock(&kvm->lock);
> >  		break;
> > +	}
> >  	case KVM_CREATE_PIT:
> >  		u.pit_config.flags = KVM_PIT_SPEAKER_DUMMY;
> >  		goto create_pit;
> > 

[patch 0/3] ioctl fixes v3

[Marcelo Tosatti]

Addressing comments.

[patch 1/3] KVM: x86: disallow multiple KVM_CREATE_IRQCHIP

[Marcelo Tosatti]

[-- Attachment #1: irqchip-create --]
[-- Type: text/plain, Size: 1867 bytes --]

Otherwise kvm will leak memory on multiple KVM_CREATE_IRQCHIP. 
Also serialize multiple accesses with kvm->lock.

Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

Index: kvm/arch/x86/kvm/x86.c
===================================================================
--- kvm.orig/arch/x86/kvm/x86.c
+++ kvm/arch/x86/kvm/x86.c
@@ -2362,25 +2362,39 @@ long kvm_arch_vm_ioctl(struct file *filp
 		if (r)
 			goto out;
 		break;
-	case KVM_CREATE_IRQCHIP:
+	case KVM_CREATE_IRQCHIP: {
+		struct kvm_pic *vpic;
+
+		mutex_lock(&kvm->lock);
+		r = -EEXIST;
+		if (kvm->arch.vpic)
+			goto create_irqchip_unlock;
 		r = -ENOMEM;
-		kvm->arch.vpic = kvm_create_pic(kvm);
-		if (kvm->arch.vpic) {
+		vpic = kvm_create_pic(kvm);
+		if (vpic) {
 			r = kvm_ioapic_init(kvm);
 			if (r) {
-				kfree(kvm->arch.vpic);
-				kvm->arch.vpic = NULL;
-				goto out;
+				kfree(vpic);
+				goto create_irqchip_unlock;
 			}
 		} else
-			goto out;
+			goto create_irqchip_unlock;
+		smp_wmb();
+		kvm->arch.vpic = vpic;
+		smp_wmb();
 		r = kvm_setup_default_irq_routing(kvm);
 		if (r) {
+			mutex_lock(&kvm->irq_lock);
 			kfree(kvm->arch.vpic);
 			kfree(kvm->arch.vioapic);
-			goto out;
+			kvm->arch.vpic = NULL;
+			kvm->arch.vioapic = NULL;
+			mutex_unlock(&kvm->irq_lock);
 		}
+	create_irqchip_unlock:
+		mutex_unlock(&kvm->lock);
 		break;
+	}
 	case KVM_CREATE_PIT:
 		u.pit_config.flags = KVM_PIT_SPEAKER_DUMMY;
 		goto create_pit;
Index: kvm/arch/x86/kvm/irq.h
===================================================================
--- kvm.orig/arch/x86/kvm/irq.h
+++ kvm/arch/x86/kvm/irq.h
@@ -86,7 +86,11 @@ static inline struct kvm_pic *pic_irqchi
 
 static inline int irqchip_in_kernel(struct kvm *kvm)
 {
-	return pic_irqchip(kvm) != NULL;
+	int ret;
+
+	ret = (pic_irqchip(kvm) != NULL);
+	smp_rmb();
+	return ret;
 }
 
 void kvm_pic_reset(struct kvm_kpic_state *s);

[patch 2/3] KVM: x86: disallow KVM_{SET,GET}_LAPIC without allocated in-kernel lapic

[Marcelo Tosatti]

[-- Attachment #1: get-set-lapic --]
[-- Type: text/plain, Size: 769 bytes --]

Otherwise kvm might attempt to dereference a NULL pointer.

Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

Index: kvm/arch/x86/kvm/x86.c
===================================================================
--- kvm.orig/arch/x86/kvm/x86.c
+++ kvm/arch/x86/kvm/x86.c
@@ -1893,6 +1893,9 @@ long kvm_arch_vcpu_ioctl(struct file *fi
 
 	switch (ioctl) {
 	case KVM_GET_LAPIC: {
+		r = -EINVAL;
+		if (!vcpu->arch.apic)
+			goto out;
 		lapic = kzalloc(sizeof(struct kvm_lapic_state), GFP_KERNEL);
 
 		r = -ENOMEM;
@@ -1908,6 +1911,9 @@ long kvm_arch_vcpu_ioctl(struct file *fi
 		break;
 	}
 	case KVM_SET_LAPIC: {
+		r = -EINVAL;
+		if (!vcpu->arch.apic)
+			goto out;
 		lapic = kmalloc(sizeof(struct kvm_lapic_state), GFP_KERNEL);
 		r = -ENOMEM;
 		if (!lapic)

[patch 3/3] KVM: only clear irq_source_id if irqchip is present

[Marcelo Tosatti]

[-- Attachment #1: free-irqsource-id --]
[-- Type: text/plain, Size: 889 bytes --]

Otherwise kvm might attempt to dereference a NULL pointer.

Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

Index: kvm/virt/kvm/irq_comm.c
===================================================================
--- kvm.orig/virt/kvm/irq_comm.c
+++ kvm/virt/kvm/irq_comm.c
@@ -243,6 +243,10 @@ void kvm_free_irq_source_id(struct kvm *
 		printk(KERN_ERR "kvm: IRQ source ID out of range!\n");
 		goto unlock;
 	}
+	clear_bit(irq_source_id, &kvm->arch.irq_sources_bitmap);
+	if (!irqchip_in_kernel(kvm))
+		goto unlock;
+
 	for (i = 0; i < KVM_IOAPIC_NUM_PINS; i++) {
 		clear_bit(irq_source_id, &kvm->arch.vioapic->irq_states[i]);
 		if (i >= 16)
@@ -251,7 +255,6 @@ void kvm_free_irq_source_id(struct kvm *
 		clear_bit(irq_source_id, &pic_irqchip(kvm)->irq_states[i]);
 #endif
 	}
-	clear_bit(irq_source_id, &kvm->arch.irq_sources_bitmap);
 unlock:
 	mutex_unlock(&kvm->irq_lock);
 }

Re: [patch 0/3] ioctl fixes v3

[Avi Kivity]

On 10/29/2009 05:44 PM, Marcelo Tosatti wrote:
> Addressing comments.
>
>
>    

Applied, thanks.

-- 
error compiling committee.c: too many arguments to function