On 21.02.20 09:22, David Hildenbrand wrote:
>>>> +
>>>> + break;
>>>> + }
>>>> + case KVM_PV_DISABLE: {
>>>> + r = -EINVAL;
>>>> + if (!kvm_s390_pv_is_protected(kvm))
>>>> + break;
>>>> +
>>>> + kvm_s390_cpus_from_pv(kvm, &cmd->rc, &cmd->rrc);
>>>> + r = kvm_s390_pv_destroy_vm(kvm, &cmd->rc, &cmd->rrc);
>>>> + if (!r)
>>>> + kvm_s390_pv_dealloc_vm(kvm);
>>>
>>> Hm, if destroy fails, the CPUs would already have been removed.
>>>
>>> Is there a way to make kvm_s390_pv_destroy_vm() never fail? The return
>>> value is always ignored except here ... which looks wrong.
>>
>> This should not fail. But if it does we should not free the memory that
>> we donated to the ultravisor. We then do have a memory leak, but this is
>> necessary to keep the integrity of the host.
>> I will fix the other places to only call dealloc when destroy succeeded.
>>
>> Same for VCPU destroy. If that fails I should not free arch.pv.stor_base
>> will fix.
>
> A comment would be nice, documenting exactly that.
something like this on top:
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 738f7fefcaec..cad04e26dccf 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -2173,6 +2173,11 @@ static void kvm_s390_cpus_from_pv(struct kvm *kvm, u16 *rc, u16 *rrc)
struct kvm_vcpu *vcpu;
int i;
+ /*
+ * we ignore failures and try to destroy as many CPUs as possible.
+ * At the same time we must not free the assigned ressources when
+ * this fails, as the ultravisor has still access to that memory.
+ */
kvm_for_each_vcpu(i, vcpu, kvm) {
mutex_lock(&vcpu->mutex);
kvm_s390_pv_destroy_cpu(vcpu, rc, rrc);
@@ -2221,6 +2226,7 @@ static int kvm_s390_handle_pv(struct kvm *kvm, struct kvm_pv_cmd *cmd)
kvm_s390_pv_dealloc_vm(kvm);
break;
}
+ /* we never switch back to bsca from esca */
r = kvm_s390_pv_create_vm(kvm, &cmd->rc, &cmd->rrc);
if (r) {
kvm_s390_pv_dealloc_vm(kvm);
diff --git a/arch/s390/kvm/pv.c b/arch/s390/kvm/pv.c
index 6b581b5f8521..f93811a0a441 100644
--- a/arch/s390/kvm/pv.c
+++ b/arch/s390/kvm/pv.c
@@ -16,6 +16,7 @@
#include <asm/mman.h>
#include "kvm-s390.h"
+/* only free ressources when the destroy was successful */
void kvm_s390_pv_dealloc_vm(struct kvm *kvm)
{
vfree(kvm->arch.pv.stor_var);
@@ -62,6 +63,7 @@ int kvm_s390_pv_alloc_vm(struct kvm *kvm)
return -ENOMEM;
}
+/* this should not fail, but if it does we must not free the donated memory */
int kvm_s390_pv_destroy_vm(struct kvm *kvm, u16 *rc, u16 *rrc)
{
int cc;
