Index: 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;

Michael S. Tsirkin

2009-10-27 17:52 UTC #2 lore raw

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

Quoted 20 lines · Marcelo Tosatti

Otherwise kvm will leak memory on multiple KVM_CREATE_IRQCHIP.
CC [email protected]
Signed-off-by Marcelo Tosatti <[email protected]>
Index: kvm/arch/x86/kvm/x86.c
===================================================================
arch/x86/kvm/x86.c +3 -0
@@ -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) {

arch/x86/kvm/x86.c long kvm_arch_vm_ioctl(struct file *filp · line 2310

Quoted 8 lines

arch/x86/kvm/x86.c +2 -0

@@ -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?

Quoted 5 lines

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

Marcelo Tosatti

author

2009-10-27 15:14 UTC #3 lore raw

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

In reply to Marcelo Tosatti

Otherwise kvm might attempt to dereference a NULL pointer.

CC [email protected]

Signed-off-by Marcelo Tosatti <[email protected]>

arch/x86/kvm/x86.c +6 -0

Index: 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)

Michael S. Tsirkin

2009-10-27 17:53 UTC #4 lore raw

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

Quoted 21 lines · Marcelo Tosatti

Otherwise kvm might attempt to dereference a NULL pointer.
CC [email protected]
Signed-off-by Marcelo Tosatti <[email protected]>
Index: kvm/arch/x86/kvm/x86.c
===================================================================
arch/x86/kvm/x86.c +3 -0
@@ -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;

arch/x86/kvm/x86.c long kvm_arch_vcpu_ioctl(struct file *fi · line 1841

Quoted 9 lines

arch/x86/kvm/x86.c +3 -0

@@ -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 ...

Quoted 4 lines

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

Avi Kivity

2009-10-28 09:32 UTC #5 lore raw

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

Quoted 3 lines · Michael S. Tsirkin

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.

Signature · 1 line

error compiling committee.c: too many arguments to function

Michael S. Tsirkin

2009-10-28 10:23 UTC #6 lore raw

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

Quoted 6 lines · Avi Kivity

Quoted 3 lines · Michael S. Tsirkin
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 ...

Quoted 2 lines

Signature · 1 line

error compiling committee.c: too many arguments to function

Gleb Natapov

2009-10-28 10:31 UTC #7 lore raw

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

Quoted 10 lines · Michael S. Tsirkin

Quoted 6 lines · Avi Kivity
Quoted 3 lines · Michael S. Tsirkin
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.

Quoted 11 lines

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 ...
Quoted 2 lines
Signature · 1 line
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
Gleb.

Michael S. Tsirkin

2009-10-28 10:35 UTC #8 lore raw

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

Quoted 7 lines · Gleb Natapov

Quoted 6 lines · Michael S. Tsirkin
Quoted 5 lines · Avi Kivity
Quoted 3 lines · Michael S. Tsirkin
Can the value of irqchip_in_kernel be changed by userspace after we have checked it? If yes, this check won't help ...

Quoted 6 lines

Quoted 4 lines
Quoted 1 line
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.

Quoted 14 lines

Quoted 11 lines
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 ...
Quoted 2 lines
Signature · 1 line
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Gleb.

Gleb Natapov

2009-10-28 10:40 UTC #9 lore raw

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

Quoted 10 lines · Michael S. Tsirkin

Quoted 9 lines · Gleb Natapov
Quoted 8 lines · Michael S. Tsirkin
Quoted 6 lines · Avi Kivity
Quoted 3 lines · Michael S. Tsirkin
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.

Quoted 8 lines

Quoted 4 lines
Quoted 2 lines
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.

Quoted 7 lines

Quoted 7 lines
Quoted 7 lines
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 ...
Quoted 2 lines
Signature · 1 line
error compiling committee.c: too many arguments to function

Quoted 7 lines

Quoted 7 lines
Quoted 4 lines
--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Gleb.

--
Gleb.

Avi Kivity

2009-10-28 10:34 UTC #10 lore raw

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

In reply to Michael S. Tsirkin

Quoted 8 lines · Michael S. Tsirkin

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).

Signature · 1 line

error compiling committee.c: too many arguments to function

Marcelo Tosatti

author

2009-10-27 15:14 UTC #11 lore raw

PATCH 3/3 KVM: only clear irq_source_id if irqchip is present

In reply to Marcelo Tosatti

Otherwise kvm might attempt to dereference a NULL pointer.

CC [email protected]

Signed-off-by Marcelo Tosatti <[email protected]>

virt/kvm/irq_comm.c +4 -1

Index: 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);
 }

Marcelo Tosatti

author

2009-10-28 20:46 UTC #12 lore raw

cover ioctl fixes v2

In reply to Marcelo Tosatti

Addressing comments.

Marcelo Tosatti

author

2009-10-28 20:46 UTC #13 lore raw

PATCH 1/3 KVM: x86: disallow multiple KVM_CREATE_IRQCHIP

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

Signed-off-by Marcelo Tosatti <[email protected]>

arch/x86/kvm/x86.c +21 -8

Index: 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;

Michael S. Tsirkin

2009-10-29 09:35 UTC #14 lore raw

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

Quoted 29 lines · Marcelo Tosatti

Otherwise kvm will leak memory on multiple KVM_CREATE_IRQCHIP. Also serialize multiple accesses with kvm->lock.
Signed-off-by Marcelo Tosatti <[email protected]>
Index: kvm/arch/x86/kvm/x86.c
===================================================================
arch/x86/kvm/x86.c +9 -4
@@ -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);

arch/x86/kvm/x86.c long kvm_arch_vm_ioctl(struct file *filp · line 2383

Quoted 10 lines

arch/x86/kvm/x86.c +5 -3

-				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()?

Quoted 18 lines

arch/x86/kvm/x86.c +7 -1

 		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;

Marcelo Tosatti

author

2009-10-29 14:11 UTC #15 lore raw

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

Quoted 42 lines · Michael S. Tsirkin

Quoted 29 lines · Marcelo Tosatti
Otherwise kvm will leak memory on multiple KVM_CREATE_IRQCHIP. Also serialize multiple accesses with kvm->lock.
Signed-off-by Marcelo Tosatti <[email protected]>
Index: kvm/arch/x86/kvm/x86.c
===================================================================
arch/x86/kvm/x86.c +9 -4
@@ -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);
arch/x86/kvm/x86.c long kvm_arch_vm_ioctl(struct file *filp · line 2383
Quoted 10 lines
arch/x86/kvm/x86.c +5 -3
-				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:

Quoted 6 lines

		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.

Quoted 25 lines

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()?
Quoted 18 lines
arch/x86/kvm/x86.c +7 -1
 		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;

Marcelo Tosatti

author

2009-10-28 20:46 UTC #16 lore raw

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

In reply to Marcelo Tosatti

Otherwise kvm might attempt to dereference a NULL pointer.

Signed-off-by Marcelo Tosatti <[email protected]>

arch/x86/kvm/x86.c +6 -0

Index: 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)

Marcelo Tosatti

author

2009-10-28 20:46 UTC #17 lore raw

PATCH 3/3 KVM: only clear irq_source_id if irqchip is present

In reply to Marcelo Tosatti

Otherwise kvm might attempt to dereference a NULL pointer.

Signed-off-by Marcelo Tosatti <[email protected]>

virt/kvm/irq_comm.c +4 -1

Index: 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);
 }

Marcelo Tosatti

author

2009-10-29 15:48 UTC #18 lore raw

cover ioctl fixes v3

In reply to Marcelo Tosatti

Addressing comments.

Marcelo Tosatti

author

2009-10-29 15:48 UTC #19 lore raw

PATCH 1/3 KVM: x86: disallow multiple KVM_CREATE_IRQCHIP

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

Signed-off-by Marcelo Tosatti <[email protected]>

arch/x86/kvm/x86.c +22 -8

Index: 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;

arch/x86/kvm/irq.h +5 -1

Index: 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);

Marcelo Tosatti

author

2009-10-29 15:48 UTC #20 lore raw

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

In reply to Marcelo Tosatti

Otherwise kvm might attempt to dereference a NULL pointer.

Signed-off-by Marcelo Tosatti <[email protected]>

arch/x86/kvm/x86.c +6 -0

Index: 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)

Marcelo Tosatti

author

2009-10-29 15:48 UTC #21 lore raw

PATCH 3/3 KVM: only clear irq_source_id if irqchip is present

In reply to Marcelo Tosatti

Otherwise kvm might attempt to dereference a NULL pointer.

Signed-off-by Marcelo Tosatti <[email protected]>

virt/kvm/irq_comm.c +4 -1

Index: 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);
 }

Avi Kivity

Applied

2009-11-02 09:53 UTC #22 lore raw

Re: [patch 0/3] ioctl fixes v3

In reply to Marcelo Tosatti

Quoted 4 lines · Marcelo Tosatti

Addressing comments.

Applied, thanks.

Signature · 1 line

error compiling committee.c: too many arguments to function