[PATCH 0/2] KVM: s390: fixups for cmma migration (kvm/master)

[PATCH 0/2] KVM: s390: fixups for cmma migration (kvm/master)

[Christian Borntraeger]

This is targetted as minimal fixups. Claudio is reworking this
to handle this better in general (e.g. memory wholes).

I will wait for a review for patch2 (which is new) and then spin
a pull request as we now have 2 patches for master

Christian Borntraeger (2):
  KVM: s390: fix cmma migration for multiple memory slots
  KVM: s390: prevent buffer overrun on memory hotplug during migration

 arch/s390/kvm/kvm-s390.c | 9 +++++----
 arch/s390/kvm/priv.c     | 2 +-
 2 files changed, 6 insertions(+), 5 deletions(-)

-- 
2.13.4

[PATCH 1/2] KVM: s390: fix cmma migration for multiple memory slots

[Christian Borntraeger]

When multiple memory slots are present the cmma migration code
does not allocate enough memory for the bitmap. The memory slots
are sorted in reverse order, so we must use gfn and size of
slot[0] instead of the last one.

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
Reviewed-by: Claudio Imbrenda <imbrenda@linux.vnet.ibm.com>
Cc: stable@vger.kernel.org # 4.13+
Fixes: 190df4a212a7 (KVM: s390: CMMA tracking, ESSA emulation, migration mode)
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
---
 arch/s390/kvm/kvm-s390.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 40d0a1a97889..b87a930c2201 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -794,11 +794,12 @@ static int kvm_s390_vm_start_migration(struct kvm *kvm)
 
 	if (kvm->arch.use_cmma) {
 		/*
-		 * Get the last slot. They should be sorted by base_gfn, so the
-		 * last slot is also the one at the end of the address space.
-		 * We have verified above that at least one slot is present.
+		 * Get the first slot. They are reverse sorted by base_gfn, so
+		 * the first slot is also the one at the end of the address
+		 * space. We have verified above that at least one slot is
+		 * present.
 		 */
-		ms = slots->memslots + slots->used_slots - 1;
+		ms = slots->memslots;
 		/* round up so we only use full longs */
 		ram_pages = roundup(ms->base_gfn + ms->npages, BITS_PER_LONG);
 		/* allocate enough bytes to store all the bits */
-- 
2.13.4

[PATCH 2/2] KVM: s390: prevent buffer overrun on memory hotplug during migration

[Christian Borntraeger]

We must not go beyond the pre-allocated buffer. This can happen when
a new memory slot is added during migration.

Reported-by: David Hildenbrand <david@redhat.com>
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
Cc: stable@vger.kernel.org # 4.13+
Fixes: 190df4a212a7 (KVM: s390: CMMA tracking, ESSA emulation, migration mode)
---
 arch/s390/kvm/priv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c
index c954ac49eee4..3211bf58d838 100644
--- a/arch/s390/kvm/priv.c
+++ b/arch/s390/kvm/priv.c
@@ -1002,7 +1002,7 @@ static inline int do_essa(struct kvm_vcpu *vcpu, const int orc)
 		cbrlo[entries] = gfn << PAGE_SHIFT;
 	}
 
-	if (orc) {
+	if (orc && gfn < ms->bitmap_size) {
 		/* increment only if we are really flipping the bit to 1 */
 		if (!test_and_set_bit(gfn, ms->pgste_bitmap))
 			atomic64_inc(&ms->dirty_pages);
-- 
2.13.4

Re: [PATCH 2/2] KVM: s390: prevent buffer overrun on memory hotplug during migration

[Cornelia Huck]

On Thu, 21 Dec 2017 10:04:16 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> We must not go beyond the pre-allocated buffer. This can happen when
> a new memory slot is added during migration.
> 
> Reported-by: David Hildenbrand <david@redhat.com>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Cc: stable@vger.kernel.org # 4.13+
> Fixes: 190df4a212a7 (KVM: s390: CMMA tracking, ESSA emulation, migration mode)
> ---
>  arch/s390/kvm/priv.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c
> index c954ac49eee4..3211bf58d838 100644
> --- a/arch/s390/kvm/priv.c
> +++ b/arch/s390/kvm/priv.c
> @@ -1002,7 +1002,7 @@ static inline int do_essa(struct kvm_vcpu *vcpu, const int orc)
>  		cbrlo[entries] = gfn << PAGE_SHIFT;
>  	}
>  
> -	if (orc) {
> +	if (orc && gfn < ms->bitmap_size) {
>  		/* increment only if we are really flipping the bit to 1 */
>  		if (!test_and_set_bit(gfn, ms->pgste_bitmap))
>  			atomic64_inc(&ms->dirty_pages);

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Re: [PATCH 2/2] KVM: s390: prevent buffer overrun on memory hotplug during migration

[David Hildenbrand]

On 21.12.2017 10:04, Christian Borntraeger wrote:
> We must not go beyond the pre-allocated buffer. This can happen when
> a new memory slot is added during migration.
> 
> Reported-by: David Hildenbrand <david@redhat.com>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Cc: stable@vger.kernel.org # 4.13+
> Fixes: 190df4a212a7 (KVM: s390: CMMA tracking, ESSA emulation, migration mode)
> ---
>  arch/s390/kvm/priv.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c
> index c954ac49eee4..3211bf58d838 100644
> --- a/arch/s390/kvm/priv.c
> +++ b/arch/s390/kvm/priv.c
> @@ -1002,7 +1002,7 @@ static inline int do_essa(struct kvm_vcpu *vcpu, const int orc)
>  		cbrlo[entries] = gfn << PAGE_SHIFT;
>  	}
>  
> -	if (orc) {
> +	if (orc && gfn < ms->bitmap_size) {
>  		/* increment only if we are really flipping the bit to 1 */
>  		if (!test_and_set_bit(gfn, ms->pgste_bitmap))
>  			atomic64_inc(&ms->dirty_pages);
> 

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 

Thanks,

David / dhildenb

Re: [PATCH 0/2] KVM: s390: fixups for cmma migration (kvm/master)

[David Hildenbrand]

On 21.12.2017 10:04, Christian Borntraeger wrote:
> This is targetted as minimal fixups. Claudio is reworking this
> to handle this better in general (e.g. memory wholes).
> 
> I will wait for a review for patch2 (which is new) and then spin
> a pull request as we now have 2 patches for master
> 
> Christian Borntraeger (2):
>   KVM: s390: fix cmma migration for multiple memory slots
>   KVM: s390: prevent buffer overrun on memory hotplug during migration
> 
>  arch/s390/kvm/kvm-s390.c | 9 +++++----
>  arch/s390/kvm/priv.c     | 2 +-
>  2 files changed, 6 insertions(+), 5 deletions(-)
> 

[waking up from vacation mode]

Sorry to say, but I guess there is more.

do_essa() can race with
kvm_s390_vm_set_migration(KVM_S390_VM_MIGRATION_STOP).

CPU0: handle_essa()
	-> vcpu->kvm->arch.migration_state == true
	-> do_essa()
CPU1: kvm_s390_vm_set_migration(KVM_S390_VM_MIGRATION_STOP)
	-> vcpu->kvm->arch.migration_state = false;
	-> vfree(mgs->pgste_bitmap)
CPU0: test_and_set_bit(gfn, ms->pgste_bitmap)
	-> access to freed data structure

The kvm_s390_sync_request_broadcast(kvm, KVM_REQ_STOP_MIGRATION) will
only make sure that all CPUs have left SIE mode, not that the request
has been processed.

Does that make sense?

[going back to vacation mode]

-- 

Thanks,

David / dhildenb

Re: [PATCH 0/2] KVM: s390: fixups for cmma migration (kvm/master)

[David Hildenbrand]

On 21.12.2017 12:57, David Hildenbrand wrote:
> On 21.12.2017 10:04, Christian Borntraeger wrote:
>> This is targetted as minimal fixups. Claudio is reworking this
>> to handle this better in general (e.g. memory wholes).
>>
>> I will wait for a review for patch2 (which is new) and then spin
>> a pull request as we now have 2 patches for master
>>
>> Christian Borntraeger (2):
>>   KVM: s390: fix cmma migration for multiple memory slots
>>   KVM: s390: prevent buffer overrun on memory hotplug during migration
>>
>>  arch/s390/kvm/kvm-s390.c | 9 +++++----
>>  arch/s390/kvm/priv.c     | 2 +-
>>  2 files changed, 6 insertions(+), 5 deletions(-)
>>
> 
> [waking up from vacation mode]
> 
> Sorry to say, but I guess there is more.
> 
> do_essa() can race with
> kvm_s390_vm_set_migration(KVM_S390_VM_MIGRATION_STOP).
> 
> CPU0: handle_essa()
> 	-> vcpu->kvm->arch.migration_state == true
> 	-> do_essa()
> CPU1: kvm_s390_vm_set_migration(KVM_S390_VM_MIGRATION_STOP)
> 	-> vcpu->kvm->arch.migration_state = false;
> 	-> vfree(mgs->pgste_bitmap)
> CPU0: test_and_set_bit(gfn, ms->pgste_bitmap)
> 	-> access to freed data structure
> 
> The kvm_s390_sync_request_broadcast(kvm, KVM_REQ_STOP_MIGRATION) will
> only make sure that all CPUs have left SIE mode, not that the request
> has been processed.
> 
> Does that make sense?
> 
> [going back to vacation mode]
> 

FWIW, doesn't apply the same thing to kvm_s390_get_cmma_bits(), too? I
can't find locking while quickly glimpsing over it. But I might be wrong.

-- 

Thanks,

David / dhildenb

Re: [PATCH 0/2] KVM: s390: fixups for cmma migration (kvm/master)

[Christian Borntraeger]

On 12/21/2017 01:03 PM, David Hildenbrand wrote:
> On 21.12.2017 12:57, David Hildenbrand wrote:
>> On 21.12.2017 10:04, Christian Borntraeger wrote:
>>> This is targetted as minimal fixups. Claudio is reworking this
>>> to handle this better in general (e.g. memory wholes).
>>>
>>> I will wait for a review for patch2 (which is new) and then spin
>>> a pull request as we now have 2 patches for master
>>>
>>> Christian Borntraeger (2):
>>>   KVM: s390: fix cmma migration for multiple memory slots
>>>   KVM: s390: prevent buffer overrun on memory hotplug during migration
>>>
>>>  arch/s390/kvm/kvm-s390.c | 9 +++++----
>>>  arch/s390/kvm/priv.c     | 2 +-
>>>  2 files changed, 6 insertions(+), 5 deletions(-)
>>>
>>
>> [waking up from vacation mode]
>>
>> Sorry to say, but I guess there is more.
>>
>> do_essa() can race with
>> kvm_s390_vm_set_migration(KVM_S390_VM_MIGRATION_STOP).
>>
>> CPU0: handle_essa()
>> 	-> vcpu->kvm->arch.migration_state == true
>> 	-> do_essa()
>> CPU1: kvm_s390_vm_set_migration(KVM_S390_VM_MIGRATION_STOP)
>> 	-> vcpu->kvm->arch.migration_state = false;
>> 	-> vfree(mgs->pgste_bitmap)
>> CPU0: test_and_set_bit(gfn, ms->pgste_bitmap)
>> 	-> access to freed data structure
>>
>> The kvm_s390_sync_request_broadcast(kvm, KVM_REQ_STOP_MIGRATION) will
>> only make sure that all CPUs have left SIE mode, not that the request
>> has been processed.
>>
>> Does that make sense?
>>
>> [going back to vacation mode]
>>
> 
> FWIW, doesn't apply the same thing to kvm_s390_get_cmma_bits(), too? I
> can't find locking while quickly glimpsing over it. But I might be wrong.

If there is a bug in a piece of code it is certainly a good idea to look deeper....
I think we can prevent both bugs by something like

diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 8951ad4d33be..6ee12afced5a 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -841,6 +841,7 @@ static int kvm_s390_vm_stop_migration(struct kvm *kvm)
 
        if (kvm->arch.use_cmma) {
                kvm_s390_sync_request_broadcast(kvm, KVM_REQ_STOP_MIGRATION);
+               synchronize_srcu(&kvm->srcu);
                vfree(mgs->pgste_bitmap);
        }
        kfree(mgs);


Need to have a look.

Re: [PATCH 0/2] KVM: s390: fixups for cmma migration (kvm/master)

[Christian Borntraeger]

On 12/21/2017 01:12 PM, Christian Borntraeger wrote:
> 
> 
> On 12/21/2017 01:03 PM, David Hildenbrand wrote:
>> On 21.12.2017 12:57, David Hildenbrand wrote:
>>> On 21.12.2017 10:04, Christian Borntraeger wrote:
>>>> This is targetted as minimal fixups. Claudio is reworking this
>>>> to handle this better in general (e.g. memory wholes).
>>>>
>>>> I will wait for a review for patch2 (which is new) and then spin
>>>> a pull request as we now have 2 patches for master
>>>>
>>>> Christian Borntraeger (2):
>>>>   KVM: s390: fix cmma migration for multiple memory slots
>>>>   KVM: s390: prevent buffer overrun on memory hotplug during migration
>>>>
>>>>  arch/s390/kvm/kvm-s390.c | 9 +++++----
>>>>  arch/s390/kvm/priv.c     | 2 +-
>>>>  2 files changed, 6 insertions(+), 5 deletions(-)
>>>>
>>>
>>> [waking up from vacation mode]
>>>
>>> Sorry to say, but I guess there is more.
>>>
>>> do_essa() can race with
>>> kvm_s390_vm_set_migration(KVM_S390_VM_MIGRATION_STOP).
>>>
>>> CPU0: handle_essa()
>>> 	-> vcpu->kvm->arch.migration_state == true
>>> 	-> do_essa()
>>> CPU1: kvm_s390_vm_set_migration(KVM_S390_VM_MIGRATION_STOP)
>>> 	-> vcpu->kvm->arch.migration_state = false;
>>> 	-> vfree(mgs->pgste_bitmap)
>>> CPU0: test_and_set_bit(gfn, ms->pgste_bitmap)
>>> 	-> access to freed data structure
>>>
>>> The kvm_s390_sync_request_broadcast(kvm, KVM_REQ_STOP_MIGRATION) will
>>> only make sure that all CPUs have left SIE mode, not that the request
>>> has been processed.
>>>
>>> Does that make sense?
>>>
>>> [going back to vacation mode]
>>>
>>
>> FWIW, doesn't apply the same thing to kvm_s390_get_cmma_bits(), too? I
>> can't find locking while quickly glimpsing over it. But I might be wrong.
> 
> If there is a bug in a piece of code it is certainly a good idea to look deeper....
> I think we can prevent both bugs by something like
> 
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 8951ad4d33be..6ee12afced5a 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -841,6 +841,7 @@ static int kvm_s390_vm_stop_migration(struct kvm *kvm)
>  
>         if (kvm->arch.use_cmma) {
>                 kvm_s390_sync_request_broadcast(kvm, KVM_REQ_STOP_MIGRATION);
> +               synchronize_srcu(&kvm->srcu);
>                 vfree(mgs->pgste_bitmap);
>         }
>         kfree(mgs);
> 
> 
> Need to have a look.

It needs a bit more changes for the get_cmma case.

Re: [PATCH 0/2] KVM: s390: fixups for cmma migration (kvm/master)

[Cornelia Huck]

On Thu, 21 Dec 2017 13:26:45 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 12/21/2017 01:12 PM, Christian Borntraeger wrote:

> > If there is a bug in a piece of code it is certainly a good idea to look deeper....
> > I think we can prevent both bugs by something like
> > 
> > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> > index 8951ad4d33be..6ee12afced5a 100644
> > --- a/arch/s390/kvm/kvm-s390.c
> > +++ b/arch/s390/kvm/kvm-s390.c
> > @@ -841,6 +841,7 @@ static int kvm_s390_vm_stop_migration(struct kvm *kvm)
> >  
> >         if (kvm->arch.use_cmma) {
> >                 kvm_s390_sync_request_broadcast(kvm, KVM_REQ_STOP_MIGRATION);
> > +               synchronize_srcu(&kvm->srcu);
> >                 vfree(mgs->pgste_bitmap);
> >         }
> >         kfree(mgs);
> > 
> > 
> > Need to have a look.  
> 
> It needs a bit more changes for the get_cmma case.

Given that Christmas is approaching quickly: Get out the two fixes we
have now, worry about the rest later?