[PATCH]: Fix memory corruption in-kernel IOAPIC emulation

[PATCH]: Fix memory corruption in-kernel IOAPIC emulation

[Chris Lalancette]

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

All,
     Attached is a patch that fixes the first (of at least a couple) migration
problem that I am running into.  Basically, using the setup I described in my
last post, I was always getting "Disabling IRQ #11" once the guest reached the
destination side, and then no further activity.  Dumping the APIC on both the
source and destination side revealed something interesting:

Source:
APIC 0x2
 (pad is 0x0
 IOAPIC state:
    base_address: 0xfec00000
    ioregsel:     0x2e
    id:           0x0
    irr:          0x0
    pad:          0x0

Destination:
APIC 0x2
 (pad is 0x38)
 IOAPIC state:
    base_address: 0xf2001000
    ioregsel:     0x2e
    id:           0x0
    irr:          0x78872f3d
    pad:          0x38

You'll notice that the base_address and irr are completely bogus on the
destination side.  Although KVM_CREATE_IRQCHIP does the right thing on the
destination side when first creating the "incoming" guest, the base_address and
other fields get blown away with bogus data during the restore.  The attached
patch fixes this by only restoring the bits that we know were saved on the
source side (i.e. what's in qemu/hw/apic.c:ioapic_save()).

Signed-off-by: Chris Lalancette <clalance-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

[-- Attachment #2: kvm-60-fix-ioapic-migration.patch --]
[-- Type: text/x-patch, Size: 995 bytes --]

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 8f94a0b..b07ea3a 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1314,6 +1314,9 @@ static int kvm_vm_ioctl_get_irqchip(struct kvm *kvm, struct kvm_irqchip *chip)
 static int kvm_vm_ioctl_set_irqchip(struct kvm *kvm, struct kvm_irqchip *chip)
 {
 	int r;
+	int i;
+	struct kvm_ioapic *kioapic;
+	struct kvm_ioapic_state *uioapic;
 
 	r = 0;
 	switch (chip->chip_id) {
@@ -1328,9 +1331,16 @@ static int kvm_vm_ioctl_set_irqchip(struct kvm *kvm, struct kvm_irqchip *chip)
 			sizeof(struct kvm_pic_state));
 		break;
 	case KVM_IRQCHIP_IOAPIC:
-		memcpy(ioapic_irqchip(kvm),
-			&chip->chip.ioapic,
-			sizeof(struct kvm_ioapic_state));
+		kioapic = ioapic_irqchip(kvm);
+		uioapic = &chip->chip.ioapic;
+
+		kioapic->id = uioapic->id;
+		kioapic->ioregsel = uioapic->ioregsel;
+
+		for (i = 0; i < IOAPIC_NUM_PINS; i++) {
+			kioapic->redirtbl[i].bits = uioapic->redirtbl[i].bits;
+		}
+
 		break;
 	default:
 		r = -EINVAL;

[-- Attachment #3: Type: text/plain, Size: 228 bytes --]

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

[-- Attachment #4: Type: text/plain, Size: 186 bytes --]

_______________________________________________
kvm-devel mailing list
kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/kvm-devel

Re: [PATCH]: Fix memory corruption in-kernel IOAPIC emulation

[Avi Kivity]

Chris Lalancette wrote:
> All,
>      Attached is a patch that fixes the first (of at least a couple) migration
> problem that I am running into.  Basically, using the setup I described in my
> last post, I was always getting "Disabling IRQ #11" once the guest reached the
> destination side, and then no further activity.  Dumping the APIC on both the
> source and destination side revealed something interesting:
>
>
>   

[...]

> You'll notice that the base_address and irr are completely bogus on the
> destination side.  Although KVM_CREATE_IRQCHIP does the right thing on the
> destination side when first creating the "incoming" guest, the base_address and
> other fields get blown away with bogus data during the restore.  The attached
> patch fixes this by only restoring the bits that we know were saved on the
> source side (i.e. what's in qemu/hw/apic.c:ioapic_save()).
>   

Excellent catch, but the fix is wrong.  Instead of partially restoring 
the ioapic state in the kernel, you should fully save it in qemu.

This is a trap that many fall into: considering kvm and qemu as one 
entity and making sure they work well together.  We need to make sure 
that kvm and libkvm are useful for other userspace programs as well.

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


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH]: Fix memory corruption in-kernel IOAPIC emulation

[Chris Lalancette]

Avi Kivity wrote:
> 
> Excellent catch, but the fix is wrong.  Instead of partially restoring 
> the ioapic state in the kernel, you should fully save it in qemu.
> 
> This is a trap that many fall into: considering kvm and qemu as one 
> entity and making sure they work well together.  We need to make sure 
> that kvm and libkvm are useful for other userspace programs as well.
> 

Ah.  Yeah, that makes sense.  OK, I'll work up another patch that does
that.  Thanks for the feedback.

Chris Lalancette

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH]: Fix memory corruption in-kernel IOAPIC emulation

[Chris Lalancette]

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

Avi Kivity wrote:
> 
> Excellent catch, but the fix is wrong.  Instead of partially restoring 
> the ioapic state in the kernel, you should fully save it in qemu.
> 
> This is a trap that many fall into: considering kvm and qemu as one 
> entity and making sure they work well together.  We need to make sure 
> that kvm and libkvm are useful for other userspace programs as well.
> 

Actually, let me ask a question here.  It seems to me that there are two
ways I could go about this:

1)  Change the save protocol so that it saves the relevant information
(i.e. base_address and irr), and then the restore protocol so it
actually pulls this stuff off the wire.  This solution seems more
"right" to me, but it has the downside that we are changing the
over-the-wire information, which will break migration between old and
new versions of qemu.

2)  Just change the restore protocol so that we properly fill in the
missing fields with real, hard-coded values instead of just random
memory.  This has the benefit that it doesn't change the protocol, but
has the downside that we won't reflect changes to base_address or irr
100% properly.  In practice, this doesn't seem like a big deal since
there doesn't currently seem to be a way to change base_address anyway,
and losing the irr doesn't seem to be catastrophic (although I'm not
100% certain about that).

The attached patch implements 2); does anyone have an opinion on which
way to go here?

Chris Lalancette

[-- Attachment #2: kvm-60-fix-ioapic-in-userland.patch --]
[-- Type: text/x-patch, Size: 790 bytes --]

diff --git a/qemu/hw/apic.c b/qemu/hw/apic.c
index c26a18d..6b41d7a 100644
--- a/qemu/hw/apic.c
+++ b/qemu/hw/apic.c
@@ -60,6 +60,7 @@
 
 /* FIXME: it's now hard coded to be equal with KVM_IOAPIC_NUM_PINS */
 #define IOAPIC_NUM_PINS			0x18
+#define IOAPIC_DEFAULT_BASE_ADDRESS  0xfec00000
 
 #define ESR_ILLEGAL_ADDRESS (1 << 7)
 
@@ -1143,8 +1144,11 @@ static void kvm_kernel_ioapic_load_from_user(IOAPICState *s)
     chip.chip_id = KVM_IRQCHIP_IOAPIC;
     kioapic = &chip.chip.ioapic;
 
-    kioapic->id = s->id;
+    kioapic->base_address = IOAPIC_DEFAULT_BASE_ADDRESS;
     kioapic->ioregsel = s->ioregsel;
+    kioapic->id = s->id;
+    kioapic->irr = 0;
+    kioapic->pad = 0;
     for (i = 0; i < IOAPIC_NUM_PINS; i++) {
         kioapic->redirtbl[i].bits = s->ioredtbl[i];
     }

[-- Attachment #3: Type: text/plain, Size: 228 bytes --]

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

[-- Attachment #4: Type: text/plain, Size: 186 bytes --]

_______________________________________________
kvm-devel mailing list
kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/kvm-devel

Re: [PATCH]: Fix memory corruption in-kernel IOAPIC emulation

[Avi Kivity]

Chris Lalancette wrote:
> Avi Kivity wrote:
>   
>> Excellent catch, but the fix is wrong.  Instead of partially restoring 
>> the ioapic state in the kernel, you should fully save it in qemu.
>>
>> This is a trap that many fall into: considering kvm and qemu as one 
>> entity and making sure they work well together.  We need to make sure 
>> that kvm and libkvm are useful for other userspace programs as well.
>>
>>     
>
> Actually, let me ask a question here.  It seems to me that there are two
> ways I could go about this:
>
> 1)  Change the save protocol so that it saves the relevant information
> (i.e. base_address and irr), and then the restore protocol so it
> actually pulls this stuff off the wire.  This solution seems more
> "right" to me, but it has the downside that we are changing the
> over-the-wire information, which will break migration between old and
> new versions of qemu.
>
>   

qemu supports backwards compatible changes to the protocol via the 
version field.

> 2)  Just change the restore protocol so that we properly fill in the
> missing fields with real, hard-coded values instead of just random
> memory.  This has the benefit that it doesn't change the protocol, but
> has the downside that we won't reflect changes to base_address or irr
> 100% properly.  In practice, this doesn't seem like a big deal since
> there doesn't currently seem to be a way to change base_address anyway,
> and losing the irr doesn't seem to be catastrophic (although I'm not
> 100% certain about that).
>
> The attached patch implements 2); does anyone have an opinion on which
> way to go here?
>   

I prefer doing a full save/restore, since we don't know what guests 
depend on.

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


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH]: Fix memory corruption in-kernel IOAPIC emulation

[Chris Lalancette]

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

Another version of the patch, done by changing the on-the-wire protocol
as Avi suggested.  I've tested this with:

old -> old - Migration works, but runs into the bug I'm trying to fix
old -> new - Migration works, but runs into the bug I'm trying to fix
new -> old - Migration fails gracefully with ioapic version mismatch
new -> new - Migration works, and doesn't run into this particular bug

Signed-off-by: Chris Lalancette <clalance-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

[-- Attachment #2: kvm-60-qemu-fix-ioapic-migration3.patch --]
[-- Type: text/x-patch, Size: 2612 bytes --]

diff --git a/qemu/hw/apic.c b/qemu/hw/apic.c
index c26a18d..7f18e38 100644
--- a/qemu/hw/apic.c
+++ b/qemu/hw/apic.c
@@ -94,6 +94,7 @@ typedef struct APICState {
 struct IOAPICState {
     uint8_t id;
     uint8_t ioregsel;
+    uint64_t base_address;
 
     uint32_t irr;
     uint64_t ioredtbl[IOAPIC_NUM_PINS];
@@ -1125,24 +1126,36 @@ static void kvm_kernel_ioapic_save_to_user(IOAPICState *s)
     kvm_get_irqchip(kvm_context, &chip);
     kioapic = &chip.chip.ioapic;
 
-    s->id = kioapic->id;
+    s->base_address = kioapic->base_address;
     s->ioregsel = kioapic->ioregsel;
+    s->id = kioapic->id;
+    s->irr = kioapic->irr;
     for (i = 0; i < IOAPIC_NUM_PINS; i++) {
         s->ioredtbl[i] = kioapic->redirtbl[i].bits;
     }
 #endif
 }
 
-static void kvm_kernel_ioapic_load_from_user(IOAPICState *s)
+static int kvm_kernel_ioapic_load_from_user(IOAPICState *s, int version_id)
 {
 #if defined(KVM_CAP_IRQCHIP) && defined(TARGET_I386)
     struct kvm_irqchip chip;
     struct kvm_ioapic_state *kioapic;
     int i;
 
+    if (version_id > 2) {
+        return -EINVAL;
+    }
+
     chip.chip_id = KVM_IRQCHIP_IOAPIC;
     kioapic = &chip.chip.ioapic;
 
+    if (version_id == 2) {
+        kioapic->base_address = s->base_address;
+        kioapic->irr = s->irr;
+    }
+
+    /* fields saved since version 1 */
     kioapic->id = s->id;
     kioapic->ioregsel = s->ioregsel;
     for (i = 0; i < IOAPIC_NUM_PINS; i++) {
@@ -1151,6 +1164,8 @@ static void kvm_kernel_ioapic_load_from_user(IOAPICState *s)
 
     kvm_set_irqchip(kvm_context, &chip);
 #endif
+
+    return 0;
 }
 
 static void ioapic_save(QEMUFile *f, void *opaque)
@@ -1173,8 +1188,9 @@ static int ioapic_load(QEMUFile *f, void *opaque, int version_id)
 {
     IOAPICState *s = opaque;
     int i;
+    int ret;
 
-    if (version_id != 1)
+    if (version_id > 2)
         return -EINVAL;
 
     qemu_get_8s(f, &s->id);
@@ -1184,7 +1200,9 @@ static int ioapic_load(QEMUFile *f, void *opaque, int version_id)
     }
 
     if (kvm_enabled() && qemu_kvm_irqchip_in_kernel()) {
-        kvm_kernel_ioapic_load_from_user(s);
+        ret = kvm_kernel_ioapic_load_from_user(s, version_id);
+        if (ret < 0)
+	    return ret;
     }
 
     return 0;
@@ -1227,7 +1245,7 @@ IOAPICState *ioapic_init(void)
                                        ioapic_mem_write, s);
     cpu_register_physical_memory(0xfec00000, 0x1000, io_memory);
 
-    register_savevm("ioapic", 0, 1, ioapic_save, ioapic_load, s);
+    register_savevm("ioapic", 0, 2, ioapic_save, ioapic_load, s);
     qemu_register_reset(ioapic_reset, s);
 
     return s;

[-- Attachment #3: Type: text/plain, Size: 228 bytes --]

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

[-- Attachment #4: Type: text/plain, Size: 186 bytes --]

_______________________________________________
kvm-devel mailing list
kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/kvm-devel

Re: [PATCH]: Fix memory corruption in-kernel IOAPIC emulation

[Avi Kivity]

Chris Lalancette wrote:
> Another version of the patch, done by changing the on-the-wire protocol
> as Avi suggested.  I've tested this with:
>
> old -> old - Migration works, but runs into the bug I'm trying to fix
> old -> new - Migration works, but runs into the bug I'm trying to fix
> new -> old - Migration fails gracefully with ioapic version mismatch
> new -> new - Migration works, and doesn't run into this particular bug
>   

>  
> -static void kvm_kernel_ioapic_load_from_user(IOAPICState *s)
> +static int kvm_kernel_ioapic_load_from_user(IOAPICState *s, int version_id)
>  {
>  #if defined(KVM_CAP_IRQCHIP) && defined(TARGET_I386)
>      struct kvm_irqchip chip;
>      struct kvm_ioapic_state *kioapic;
>      int i;
>  
> +    if (version_id > 2) {
> +        return -EINVAL;
> +    }
> +
>      chip.chip_id = KVM_IRQCHIP_IOAPIC;
>      kioapic = &chip.chip.ioapic;
>  
> +    if (version_id == 2) {
> +        kioapic->base_address = s->base_address;
> +        kioapic->irr = s->irr;
> +    }
> +
>   

Preferably this function would always set all variables; the caller 
should make sure all the values are sane (by initializing them to sane 
values on ioapic creation, and not modifying them when a version 1 
migration happens).  It's best to limit protocol change handling to the 
actual qemu save/restore routines.

>  static void ioapic_save(QEMUFile *f, void *opaque)
> @@ -1173,8 +1188,9 @@ static int ioapic_load(QEMUFile *f, void *opaque, int version_id)
>  {
>      IOAPICState *s = opaque;
>      int i;
> +    int ret;
>  
> -    if (version_id != 1)
> +    if (version_id > 2)
>          return -EINVAL;
>   

This allows version_id == 0 to sneak in, where it couldn't before.  It's 
not a problem in practice, but let's get it right.



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


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH]: Fix memory corruption in-kernel IOAPIC emulation

[Chris Lalancette]

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

Another version of the patch, taking into account more of Avi's comments.  This
one was tested the same way as the previous one, by doing all the combinations
of new and old QEMU versions; the results were the same as last time:

old -> old: Bug
old -> new: Sane values, but not transferred over the wire
new -> old: Graceful fail, version mismatch
new -> new: Fixed values, taken from the wire

Signed-off-by: Chris Lalancette <clalance-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

[-- Attachment #2: kvm-60-qemu-fix-ioapic-migration4.patch --]
[-- Type: text/x-patch, Size: 2640 bytes --]

diff --git a/qemu/hw/apic.c b/qemu/hw/apic.c
index a47c366..21e5790 100644
--- a/qemu/hw/apic.c
+++ b/qemu/hw/apic.c
@@ -64,6 +64,7 @@ extern kvm_context_t kvm_context;
 
 /* FIXME: it's now hard coded to be equal with KVM_IOAPIC_NUM_PINS */
 #define IOAPIC_NUM_PINS			0x18
+#define IOAPIC_DEFAULT_BASE_ADDRESS  0xfec00000
 
 #define ESR_ILLEGAL_ADDRESS (1 << 7)
 
@@ -98,6 +99,7 @@ typedef struct APICState {
 struct IOAPICState {
     uint8_t id;
     uint8_t ioregsel;
+    uint64_t base_address;
 
     uint32_t irr;
     uint64_t ioredtbl[IOAPIC_NUM_PINS];
@@ -1145,6 +1147,8 @@ static void kvm_kernel_ioapic_save_to_user(IOAPICState *s)
 
     s->id = kioapic->id;
     s->ioregsel = kioapic->ioregsel;
+    s->base_address = kioapic->base_address;
+    s->irr = kioapic->irr;
     for (i = 0; i < IOAPIC_NUM_PINS; i++) {
         s->ioredtbl[i] = kioapic->redirtbl[i].bits;
     }
@@ -1163,6 +1167,8 @@ static void kvm_kernel_ioapic_load_from_user(IOAPICState *s)
 
     kioapic->id = s->id;
     kioapic->ioregsel = s->ioregsel;
+    kioapic->base_address = s->base_address;
+    kioapic->irr = s->irr;
     for (i = 0; i < IOAPIC_NUM_PINS; i++) {
         kioapic->redirtbl[i].bits = s->ioredtbl[i];
     }
@@ -1185,6 +1191,8 @@ static void ioapic_save(QEMUFile *f, void *opaque)
 
     qemu_put_8s(f, &s->id);
     qemu_put_8s(f, &s->ioregsel);
+    qemu_put_be64s(f, &s->base_address);
+    qemu_put_be32s(f, &s->irr);
     for (i = 0; i < IOAPIC_NUM_PINS; i++) {
         qemu_put_be64s(f, &s->ioredtbl[i]);
     }
@@ -1195,11 +1203,21 @@ static int ioapic_load(QEMUFile *f, void *opaque, int version_id)
     IOAPICState *s = opaque;
     int i;
 
-    if (version_id != 1)
+    if (version_id < 1 || version_id > 2)
         return -EINVAL;
 
     qemu_get_8s(f, &s->id);
     qemu_get_8s(f, &s->ioregsel);
+    if (version_id == 2) {
+      /* for version 2, we get this data off of the wire */
+      qemu_get_be64s(f, &s->base_address);
+      qemu_get_be32s(f, &s->irr);
+    }
+    else {
+      /* in case we are doing version 1, we just set these to sane values */
+      s->base_address = IOAPIC_DEFAULT_BASE_ADDRESS;
+      s->irr = 0;
+    }
     for (i = 0; i < IOAPIC_NUM_PINS; i++) {
         qemu_get_be64s(f, &s->ioredtbl[i]);
     }
@@ -1250,7 +1268,7 @@ IOAPICState *ioapic_init(void)
                                        ioapic_mem_write, s);
     cpu_register_physical_memory(0xfec00000, 0x1000, io_memory);
 
-    register_savevm("ioapic", 0, 1, ioapic_save, ioapic_load, s);
+    register_savevm("ioapic", 0, 2, ioapic_save, ioapic_load, s);
     qemu_register_reset(ioapic_reset, s);
 
     return s;

[-- Attachment #3: Type: text/plain, Size: 228 bytes --]

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

[-- Attachment #4: Type: text/plain, Size: 186 bytes --]

_______________________________________________
kvm-devel mailing list
kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/kvm-devel