* Re: [PATCH] KVM: s390: add proper locking for CMMA migration bitmap
2017-12-22 11:21 [PATCH] KVM: s390: add proper locking for CMMA migration bitmap Christian Borntraeger
@ 2017-12-22 13:35 ` Cornelia Huck
2017-12-22 13:38 ` Christian Borntraeger
2018-01-08 13:56 ` Cornelia Huck
2018-01-23 16:43 ` Claudio Imbrenda
2 siblings, 1 reply; 5+ messages in thread
From: Cornelia Huck @ 2017-12-22 13:35 UTC (permalink / raw)
To: Christian Borntraeger
Cc: KVM, linux-s390, David Hildenbrand, Claudio Imbrenda
On Fri, 22 Dec 2017 12:21:45 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> Some parts of the cmma migration bitmap is already protected
> with the kvm->lock (e.g. the migration start). On the other
> hand the read of the cmma bits is not protected against a
> concurrent free, neither is the emulation of the ESSA instruction.
> Let's extend the locking to all related ioctls by using
> the slots lock and wait for the freeing until all unlocked
> users have finished (those hold kvm->srcu for read).
>
> 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/kvm-s390.c | 14 +++++++++-----
> 1 file changed, 9 insertions(+), 5 deletions(-)
I'm way too confused by the locking rules to give this a good review
before Christmas... do we have a general writeup somewhere?
>
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 3373d8dff131..4458d7fe45df 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -837,6 +837,8 @@ 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);
> + /* We have to wait for the essa emulation to finish */
> + synchronize_srcu(&kvm->srcu);
> vfree(mgs->pgste_bitmap);
> }
> kfree(mgs);
> @@ -846,14 +848,12 @@ static int kvm_s390_vm_stop_migration(struct kvm *kvm)
> static int kvm_s390_vm_set_migration(struct kvm *kvm,
> struct kvm_device_attr *attr)
> {
> - int idx, res = -ENXIO;
> + int res = -ENXIO;
>
> - mutex_lock(&kvm->lock);
> + mutex_lock(&kvm->slots_lock);
> switch (attr->attr) {
> case KVM_S390_VM_MIGRATION_START:
> - idx = srcu_read_lock(&kvm->srcu);
> res = kvm_s390_vm_start_migration(kvm);
> - srcu_read_unlock(&kvm->srcu, idx);
> break;
> case KVM_S390_VM_MIGRATION_STOP:
> res = kvm_s390_vm_stop_migration(kvm);
> @@ -861,7 +861,7 @@ static int kvm_s390_vm_set_migration(struct kvm *kvm,
> default:
> break;
> }
> - mutex_unlock(&kvm->lock);
> + mutex_unlock(&kvm->slots_lock);
>
> return res;
> }
> @@ -1763,7 +1763,9 @@ long kvm_arch_vm_ioctl(struct file *filp,
> r = -EFAULT;
> if (copy_from_user(&args, argp, sizeof(args)))
> break;
> + mutex_lock(&kvm->slots_lock);
> r = kvm_s390_get_cmma_bits(kvm, &args);
> + mutex_unlock(&kvm->slots_lock);
> if (!r) {
> r = copy_to_user(argp, &args, sizeof(args));
> if (r)
> @@ -1777,7 +1779,9 @@ long kvm_arch_vm_ioctl(struct file *filp,
> r = -EFAULT;
> if (copy_from_user(&args, argp, sizeof(args)))
> break;
> + mutex_lock(&kvm->slots_lock);
> r = kvm_s390_set_cmma_bits(kvm, &args);
> + mutex_unlock(&kvm->slots_lock);
> break;
> }
> default:
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] KVM: s390: add proper locking for CMMA migration bitmap
2017-12-22 13:35 ` Cornelia Huck
@ 2017-12-22 13:38 ` Christian Borntraeger
0 siblings, 0 replies; 5+ messages in thread
From: Christian Borntraeger @ 2017-12-22 13:38 UTC (permalink / raw)
To: Cornelia Huck; +Cc: KVM, linux-s390, David Hildenbrand, Claudio Imbrenda
On 12/22/2017 02:35 PM, Cornelia Huck wrote:
> On Fri, 22 Dec 2017 12:21:45 +0100
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
>
>> Some parts of the cmma migration bitmap is already protected
>> with the kvm->lock (e.g. the migration start). On the other
>> hand the read of the cmma bits is not protected against a
>> concurrent free, neither is the emulation of the ESSA instruction.
>> Let's extend the locking to all related ioctls by using
>> the slots lock and wait for the freeing until all unlocked
>> users have finished (those hold kvm->srcu for read).
>>
>> 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/kvm-s390.c | 14 +++++++++-----
>> 1 file changed, 9 insertions(+), 5 deletions(-)
>
> I'm way too confused by the locking rules to give this a good review
> before Christmas... do we have a general writeup somewhere?
The hiararchy is documented (Documentation/virtual/locking) but not the purpose
and what is locked by what. the slots_lock basically protects the memory slots
as it is used for memslot changes (normally you would use rcu but the mutex is
the big hammer to prevent concurrent memslot changes).
Lets defer this to after christmas. I will do a pull request for the other
2 patches.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] KVM: s390: add proper locking for CMMA migration bitmap
2017-12-22 11:21 [PATCH] KVM: s390: add proper locking for CMMA migration bitmap Christian Borntraeger
2017-12-22 13:35 ` Cornelia Huck
@ 2018-01-08 13:56 ` Cornelia Huck
2018-01-23 16:43 ` Claudio Imbrenda
2 siblings, 0 replies; 5+ messages in thread
From: Cornelia Huck @ 2018-01-08 13:56 UTC (permalink / raw)
To: Christian Borntraeger
Cc: KVM, linux-s390, David Hildenbrand, Claudio Imbrenda
On Fri, 22 Dec 2017 12:21:45 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> Some parts of the cmma migration bitmap is already protected
> with the kvm->lock (e.g. the migration start). On the other
> hand the read of the cmma bits is not protected against a
> concurrent free, neither is the emulation of the ESSA instruction.
> Let's extend the locking to all related ioctls by using
> the slots lock and wait for the freeing until all unlocked
> users have finished (those hold kvm->srcu for read).
>
> 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/kvm-s390.c | 14 +++++++++-----
> 1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 3373d8dff131..4458d7fe45df 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -837,6 +837,8 @@ 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);
> + /* We have to wait for the essa emulation to finish */
> + synchronize_srcu(&kvm->srcu);
> vfree(mgs->pgste_bitmap);
> }
> kfree(mgs);
> @@ -846,14 +848,12 @@ static int kvm_s390_vm_stop_migration(struct kvm *kvm)
> static int kvm_s390_vm_set_migration(struct kvm *kvm,
> struct kvm_device_attr *attr)
> {
> - int idx, res = -ENXIO;
> + int res = -ENXIO;
>
> - mutex_lock(&kvm->lock);
> + mutex_lock(&kvm->slots_lock);
> switch (attr->attr) {
> case KVM_S390_VM_MIGRATION_START:
> - idx = srcu_read_lock(&kvm->srcu);
> res = kvm_s390_vm_start_migration(kvm);
> - srcu_read_unlock(&kvm->srcu, idx);
> break;
> case KVM_S390_VM_MIGRATION_STOP:
> res = kvm_s390_vm_stop_migration(kvm);
> @@ -861,7 +861,7 @@ static int kvm_s390_vm_set_migration(struct kvm *kvm,
> default:
> break;
> }
> - mutex_unlock(&kvm->lock);
> + mutex_unlock(&kvm->slots_lock);
This changes the locking from the one described for
kvm_s390_vm_{start,stop}_migration() - the comments there should be
updated.
>
> return res;
> }
> @@ -1763,7 +1763,9 @@ long kvm_arch_vm_ioctl(struct file *filp,
> r = -EFAULT;
> if (copy_from_user(&args, argp, sizeof(args)))
> break;
> + mutex_lock(&kvm->slots_lock);
> r = kvm_s390_get_cmma_bits(kvm, &args);
> + mutex_unlock(&kvm->slots_lock);
> if (!r) {
> r = copy_to_user(argp, &args, sizeof(args));
> if (r)
> @@ -1777,7 +1779,9 @@ long kvm_arch_vm_ioctl(struct file *filp,
> r = -EFAULT;
> if (copy_from_user(&args, argp, sizeof(args)))
> break;
> + mutex_lock(&kvm->slots_lock);
> r = kvm_s390_set_cmma_bits(kvm, &args);
> + mutex_unlock(&kvm->slots_lock);
> break;
> }
> default:
Add a comment to kvm_s390_{get,set}_cmma_bits() that they are supposed
to be called with the slot_lock held and why?
The change in locking seems fine to me.
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] KVM: s390: add proper locking for CMMA migration bitmap
2017-12-22 11:21 [PATCH] KVM: s390: add proper locking for CMMA migration bitmap Christian Borntraeger
2017-12-22 13:35 ` Cornelia Huck
2018-01-08 13:56 ` Cornelia Huck
@ 2018-01-23 16:43 ` Claudio Imbrenda
2 siblings, 0 replies; 5+ messages in thread
From: Claudio Imbrenda @ 2018-01-23 16:43 UTC (permalink / raw)
To: Christian Borntraeger; +Cc: KVM, Cornelia Huck, linux-s390, David Hildenbrand
On Fri, 22 Dec 2017 12:21:45 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> Some parts of the cmma migration bitmap is already protected
> with the kvm->lock (e.g. the migration start). On the other
> hand the read of the cmma bits is not protected against a
> concurrent free, neither is the emulation of the ESSA instruction.
> Let's extend the locking to all related ioctls by using
> the slots lock and wait for the freeing until all unlocked
> users have finished (those hold kvm->srcu for read).
Reviewed-by: Claudio Imbrenda <imbrenda@linux.vnet.ibm.com>
I'm working on a better fix to apply on top of this patch, but it will
take some time. This patch will at least plug the holes until then.
> 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/kvm-s390.c | 14 +++++++++-----
> 1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 3373d8dff131..4458d7fe45df 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -837,6 +837,8 @@ 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);
> + /* We have to wait for the essa emulation to finish
> */
> + synchronize_srcu(&kvm->srcu);
> vfree(mgs->pgste_bitmap);
> }
> kfree(mgs);
> @@ -846,14 +848,12 @@ static int kvm_s390_vm_stop_migration(struct
> kvm *kvm) static int kvm_s390_vm_set_migration(struct kvm *kvm,
> struct kvm_device_attr *attr)
> {
> - int idx, res = -ENXIO;
> + int res = -ENXIO;
>
> - mutex_lock(&kvm->lock);
> + mutex_lock(&kvm->slots_lock);
> switch (attr->attr) {
> case KVM_S390_VM_MIGRATION_START:
> - idx = srcu_read_lock(&kvm->srcu);
> res = kvm_s390_vm_start_migration(kvm);
> - srcu_read_unlock(&kvm->srcu, idx);
> break;
> case KVM_S390_VM_MIGRATION_STOP:
> res = kvm_s390_vm_stop_migration(kvm);
> @@ -861,7 +861,7 @@ static int kvm_s390_vm_set_migration(struct kvm
> *kvm, default:
> break;
> }
> - mutex_unlock(&kvm->lock);
> + mutex_unlock(&kvm->slots_lock);
>
> return res;
> }
> @@ -1763,7 +1763,9 @@ long kvm_arch_vm_ioctl(struct file *filp,
> r = -EFAULT;
> if (copy_from_user(&args, argp, sizeof(args)))
> break;
> + mutex_lock(&kvm->slots_lock);
> r = kvm_s390_get_cmma_bits(kvm, &args);
> + mutex_unlock(&kvm->slots_lock);
> if (!r) {
> r = copy_to_user(argp, &args, sizeof(args));
> if (r)
> @@ -1777,7 +1779,9 @@ long kvm_arch_vm_ioctl(struct file *filp,
> r = -EFAULT;
> if (copy_from_user(&args, argp, sizeof(args)))
> break;
> + mutex_lock(&kvm->slots_lock);
> r = kvm_s390_set_cmma_bits(kvm, &args);
> + mutex_unlock(&kvm->slots_lock);
> break;
> }
> default:
^ permalink raw reply [flat|nested] 5+ messages in thread