[BUG] : kvm_set_phys_mem: error unregistering overlapping slot: Invalid argument

Submitted by Xiao Guangrong on May 31, 2013, 6:51 a.m.

Details

Message ID 51A8485A.4000605@linux.vnet.ibm.com
State New
Headers show

Commit Message

Xiao Guangrong May 31, 2013, 6:51 a.m.
On 05/31/2013 12:50 AM, Jordan Justen wrote:
> On Thu, May 30, 2013 at 9:08 AM, Luiz Capitulino <lcapitulino@redhat.com> wrote:
>> On Thu, 30 May 2013 18:03:04 +0200
>> Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>>> Il 30/05/2013 17:46, Luiz Capitulino ha scritto:
>>>> The culprit is commit:
>>>>
>>>> commit 235e8982ad393e5611cb892df54881c872eea9e1
>>>> Author: Jordan Justen <jordan.l.justen@intel.com>
>>>> Date:   Wed May 29 01:27:26 2013 -0700
>>>>
>>>>     kvm: support using KVM_MEM_READONLY flag for regions
>>>>
>>>> I'm running 3.9.2-200.fc18, btw. And, error checking is missing on the
>>>> first call to kvm_vm_ioctl().
> 
> As noted in the code, the first call is for KVM commit 75d61fbc.
> 
> I'm not sure we want to fail if an error occurs when making that call.
> (I'm pretty sure we don't want to in fact.)
> 
> Xiao, any thoughts?

I have reproduced this bug, it seems that the bug is caused by double free
the memslot. After these change, it can boot the guest now.

Comments

Jordan Justen May 31, 2013, 7:14 a.m.
On Thu, May 30, 2013 at 11:51 PM, Xiao Guangrong
<xiaoguangrong@linux.vnet.ibm.com> wrote:
> On 05/31/2013 12:50 AM, Jordan Justen wrote:
>> On Thu, May 30, 2013 at 9:08 AM, Luiz Capitulino <lcapitulino@redhat.com> wrote:
>>> On Thu, 30 May 2013 18:03:04 +0200
>>> Paolo Bonzini <pbonzini@redhat.com> wrote:
>>>
>>>> Il 30/05/2013 17:46, Luiz Capitulino ha scritto:
>>>>> The culprit is commit:
>>>>>
>>>>> commit 235e8982ad393e5611cb892df54881c872eea9e1
>>>>> Author: Jordan Justen <jordan.l.justen@intel.com>
>>>>> Date:   Wed May 29 01:27:26 2013 -0700
>>>>>
>>>>>     kvm: support using KVM_MEM_READONLY flag for regions
>>>>>
>>>>> I'm running 3.9.2-200.fc18, btw. And, error checking is missing on the
>>>>> first call to kvm_vm_ioctl().
>>
>> As noted in the code, the first call is for KVM commit 75d61fbc.
>>
>> I'm not sure we want to fail if an error occurs when making that call.
>> (I'm pretty sure we don't want to in fact.)
>>
>> Xiao, any thoughts?
>
> I have reproduced this bug, it seems that the bug is caused by double free
> the memslot. After these change, it can boot the guest now.

I think that I might have found that the flash memory had an issue
with transitioning from flash to rom mode with code like this on the
3.8 kernel. I'll test it on 3.8 & 3.9.

-Jordan

> diff --git a/kvm-all.c b/kvm-all.c
> index 8e7bbf8..405480e 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -206,7 +206,8 @@ static int kvm_set_user_memory_region(KVMState *s, KVMSlot *slot)
>      if (s->migration_log) {
>          mem.flags |= KVM_MEM_LOG_DIRTY_PAGES;
>      }
> -    if (mem.flags & KVM_MEM_READONLY) {
> +
> +    if (slot->memory_size && mem.flags & KVM_MEM_READONLY) {
>          /* Set the slot size to 0 before setting the slot to the desired
>           * value. This is needed based on KVM commit 75d61fbc. */
>          mem.memory_size = 0;
>
>
Jordan Justen May 31, 2013, 8:01 a.m.
On Fri, May 31, 2013 at 12:14 AM, Jordan Justen <jljusten@gmail.com> wrote:
> On Thu, May 30, 2013 at 11:51 PM, Xiao Guangrong
> <xiaoguangrong@linux.vnet.ibm.com> wrote:
>> On 05/31/2013 12:50 AM, Jordan Justen wrote:
>>> On Thu, May 30, 2013 at 9:08 AM, Luiz Capitulino <lcapitulino@redhat.com> wrote:
>>>> On Thu, 30 May 2013 18:03:04 +0200
>>>> Paolo Bonzini <pbonzini@redhat.com> wrote:
>>>>
>>>>> Il 30/05/2013 17:46, Luiz Capitulino ha scritto:
>>>>>> The culprit is commit:
>>>>>>
>>>>>> commit 235e8982ad393e5611cb892df54881c872eea9e1
>>>>>> Author: Jordan Justen <jordan.l.justen@intel.com>
>>>>>> Date:   Wed May 29 01:27:26 2013 -0700
>>>>>>
>>>>>>     kvm: support using KVM_MEM_READONLY flag for regions
>>>>>>
>>>>>> I'm running 3.9.2-200.fc18, btw. And, error checking is missing on the
>>>>>> first call to kvm_vm_ioctl().
>>>
>>> As noted in the code, the first call is for KVM commit 75d61fbc.
>>>
>>> I'm not sure we want to fail if an error occurs when making that call.
>>> (I'm pretty sure we don't want to in fact.)
>>>
>>> Xiao, any thoughts?
>>
>> I have reproduced this bug, it seems that the bug is caused by double free
>> the memslot. After these change, it can boot the guest now.
>
> I think that I might have found that the flash memory had an issue
> with transitioning from flash to rom mode with code like this on the
> 3.8 kernel. I'll test it on 3.8 & 3.9.

Yes, the issue still occurs on 3.9, and even the "double free" doesn't
work on 3.9 like it did on 3.8.

This issue is a bit of a corner case for flash, but still important. I
hope maybe Xiao might have time to help me understand it. (I'll email
Xiao off-list about this.)

But, for now, I think Xiao's patch will fix things for the non-flash
case, and will not totally break flash support either.

-Jordan
Paolo Bonzini May 31, 2013, 8:23 a.m.
Il 31/05/2013 08:51, Xiao Guangrong ha scritto:
> On 05/31/2013 12:50 AM, Jordan Justen wrote:
>> On Thu, May 30, 2013 at 9:08 AM, Luiz Capitulino <lcapitulino@redhat.com> wrote:
>>> On Thu, 30 May 2013 18:03:04 +0200
>>> Paolo Bonzini <pbonzini@redhat.com> wrote:
>>>
>>>> Il 30/05/2013 17:46, Luiz Capitulino ha scritto:
>>>>> The culprit is commit:
>>>>>
>>>>> commit 235e8982ad393e5611cb892df54881c872eea9e1
>>>>> Author: Jordan Justen <jordan.l.justen@intel.com>
>>>>> Date:   Wed May 29 01:27:26 2013 -0700
>>>>>
>>>>>     kvm: support using KVM_MEM_READONLY flag for regions
>>>>>
>>>>> I'm running 3.9.2-200.fc18, btw. And, error checking is missing on the
>>>>> first call to kvm_vm_ioctl().
>>
>> As noted in the code, the first call is for KVM commit 75d61fbc.
>>
>> I'm not sure we want to fail if an error occurs when making that call.
>> (I'm pretty sure we don't want to in fact.)
>>
>> Xiao, any thoughts?
> 
> I have reproduced this bug, it seems that the bug is caused by double free
> the memslot. After these change, it can boot the guest now.
> 
> diff --git a/kvm-all.c b/kvm-all.c
> index 8e7bbf8..405480e 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -206,7 +206,8 @@ static int kvm_set_user_memory_region(KVMState *s, KVMSlot *slot)
>      if (s->migration_log) {
>          mem.flags |= KVM_MEM_LOG_DIRTY_PAGES;
>      }
> -    if (mem.flags & KVM_MEM_READONLY) {
> +
> +    if (slot->memory_size && mem.flags & KVM_MEM_READONLY) {
>          /* Set the slot size to 0 before setting the slot to the desired
>           * value. This is needed based on KVM commit 75d61fbc. */
>          mem.memory_size = 0;
> 
> 
> 
> 

Thanks, can you submit it for uq/master?  Please Cc kvm@vger.kernel.org
too, and use [PATCH uq/master] as the prefix.

Paolo

Patch hide | download patch | download mbox

diff --git a/kvm-all.c b/kvm-all.c
index 8e7bbf8..405480e 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -206,7 +206,8 @@  static int kvm_set_user_memory_region(KVMState *s, KVMSlot *slot)
     if (s->migration_log) {
         mem.flags |= KVM_MEM_LOG_DIRTY_PAGES;
     }
-    if (mem.flags & KVM_MEM_READONLY) {
+
+    if (slot->memory_size && mem.flags & KVM_MEM_READONLY) {
         /* Set the slot size to 0 before setting the slot to the desired
          * value. This is needed based on KVM commit 75d61fbc. */
         mem.memory_size = 0;