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

Message ID 32605f3e-b533-d059-88a7-f49380215cf8@de.ibm.com
State New
Headers show

Commit Message

Christian Borntraeger Dec. 21, 2017, 12:12 p.m. UTC
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



Need to have a look.

Comments

Christian Borntraeger Dec. 21, 2017, 12:26 p.m. UTC | #1
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.
Cornelia Huck Dec. 21, 2017, 4:43 p.m. UTC | #2
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?

Patch
diff mbox

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