KVM: Fix srcu struct leakage
PATCH 1/1 KVM: Fix srcu struct leakage
Clean up the srcu struct on vm destruction.
--- virt/kvm/kvm_main.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
virt/kvm/kvm_main.c +1 -0
@@ -513,6 +513,7 @@ static void kvm_destroy_vm(struct kvm *kvm)#else kvm_arch_flush_shadow (kvm);#endif +cleanup_srcu_struct (&kvm->srcu);kvm_arch_destroy_vm (kvm);hardware_disable_all ();mmdrop (mm);
Signature · 1 line
1.7.1
Quoted 1 line · Jan Kiszka
Clean up the srcu struct on vm destruction.
Applied and queued for 2.6.37; thanks.
Signature · 1 line
error compiling committee.c: too many arguments to function
Quoted 21 lines · Jan Kiszka
Clean up the srcu struct on vm destruction.
Signed-off-by Jan Kiszka <[email protected]>--- virt/kvm/kvm_main.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)virt/kvm/kvm_main.c +1
-0@@ -513,6 +513,7 @@ static void kvm_destroy_vm(struct kvm *kvm)#else kvm_arch_flush_shadow (kvm);#endif +cleanup_srcu_struct (&kvm->srcu);kvm_arch_destroy_vm (kvm);hardware_disable_all ();mmdrop (mm);Signature · 1 line
1.7.1
kvm_arch_destroy_vm does it.
Quoted 8 lines · Marcelo Tosatti
Quoted 7 lines · Jan Kiszka
Clean up the srcu struct on vm destruction.
Signed-off-by Jan Kiszka <[email protected]>--- virt/kvm/kvm_main.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
Quoted 16 lines
Quoted 14 lines
virt/kvm/kvm_main.c +1
-0@@ -513,6 +513,7 @@ static void kvm_destroy_vm(struct kvm *kvm)#else kvm_arch_flush_shadow (kvm);#endif +cleanup_srcu_struct (&kvm->srcu);kvm_arch_destroy_vm (kvm);hardware_disable_all ();mmdrop (mm);Signature · 1 line
1.7.1
kvm_arch_destroy_vmdoes it.
Oh, indeed.
That I guess it's time to clean up, move generic allocation and release into generic code. Having kvm_arch_create/destroy_vm manage the kvm object allocation does not look very nice (and caused this confusion of mine).
Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux
Quoted 23 lines · Marcelo Tosatti
Quoted 7 lines · Jan Kiszka
Clean up the srcu struct on vm destruction.
Signed-off-by Jan Kiszka<[email protected]>---
virt/kvm/kvm_main.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)Quoted 14 lines
diff --git
a/virt/kvm/kvm_main.cb/virt/kvm/kvm_main.cindex 4111a4b..6ec58d1 100644
---a/virt/kvm/kvm_main.c
+++b/virt/kvm/kvm_main.c
@@ -513,6 +513,7 @@ static voidkvm_destroy_vm(struct kvm *kvm)
#else
kvm_arch_flush_shadow(kvm);
#endif
+cleanup_srcu_struct(&kvm->srcu);
kvm_arch_destroy_vm(kvm);hardware_disable_all();
mmdrop(mm);
--
1.7.1
Quoted 1 line
kvm_arch_destroy_vmdoes it.
True. Dropped.
Signature · 1 line
error compiling committee.c: too many arguments to function
Quoted 26 lines · Jan Kiszka
Quoted 8 lines · Marcelo Tosatti
Quoted 7 lines · Jan Kiszka
Clean up the srcu struct on vm destruction.
Signed-off-by Jan Kiszka <[email protected]>--- virt/kvm/kvm_main.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)Quoted 16 lines
Quoted 14 lines
virt/kvm/kvm_main.c +1
-0@@ -513,6 +513,7 @@ static void kvm_destroy_vm(struct kvm *kvm)#else kvm_arch_flush_shadow (kvm);#endif +cleanup_srcu_struct (&kvm->srcu);kvm_arch_destroy_vm (kvm);hardware_disable_all ();mmdrop (mm);Signature · 1 line
1.7.1
kvm_arch_destroy_vmdoes it.
Quoted 8 lines
Oh, indeed.
That I guess it's time to clean up, move generic allocation and release into generic code. Having kvm_arch_create/destroy_vm manage the kvm object allocation does not look very nice (and caused this confusion of mine).
Jan
kvm_iommu_unmap_guest enters SRCU critical section, and "struct kvm" is freed at the end of kvm_arch_destroy_vm. Thats why its not in generic code.
But sure, it would be nicer if it could be moved to generic code.
void kvm_arch_destroy_vm(struct kvm *kvm)
{
kvm_iommu_unmap_guest(kvm);
kfree(kvm->arch.vpic);
kfree(kvm->arch.vioapic);
kvm_free_vcpus(kvm);
kvm_free_physmem(kvm);
if (kvm->arch.apic_access_page)
put_page(kvm->arch.apic_access_page);
if (kvm->arch.ept_identity_pagetable)
put_page(kvm->arch.ept_identity_pagetable);
cleanup_srcu_struct(&kvm->srcu);
kfree(kvm);
}Quoted 26 lines · Jan Kiszka
Quoted 8 lines · Marcelo Tosatti
Quoted 7 lines · Jan Kiszka
Clean up the srcu struct on vm destruction.
Signed-off-by Jan Kiszka <[email protected]>--- virt/kvm/kvm_main.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)Quoted 16 lines
Quoted 14 lines
virt/kvm/kvm_main.c +1
-0@@ -513,6 +513,7 @@ static void kvm_destroy_vm(struct kvm *kvm)#else kvm_arch_flush_shadow (kvm);#endif +cleanup_srcu_struct (&kvm->srcu);kvm_arch_destroy_vm (kvm);hardware_disable_all ();mmdrop (mm);Signature · 1 line
1.7.1
kvm_arch_destroy_vmdoes it.
Quoted 6 lines
Oh, indeed.
That I guess it's time to clean up, move generic allocation and release into generic code. Having kvm_arch_create/destroy_vm manage the kvm object allocation does not look very nice (and caused this confusion of mine).
Oh, the setup/cleanup maze is a tribute to the IA64 zombie. Mmh...
Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux
Quoted 34 lines · Marcelo Tosatti
Quoted 33 lines · Jan Kiszka
Quoted 8 lines · Marcelo Tosatti
Quoted 7 lines · Jan Kiszka
Clean up the srcu struct on vm destruction.
Signed-off-by Jan Kiszka <[email protected]>--- virt/kvm/kvm_main.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)Quoted 16 lines
Quoted 14 lines
virt/kvm/kvm_main.c +1
-0@@ -513,6 +513,7 @@ static void kvm_destroy_vm(struct kvm *kvm)#else kvm_arch_flush_shadow (kvm);#endif +cleanup_srcu_struct (&kvm->srcu);kvm_arch_destroy_vm (kvm);hardware_disable_all ();mmdrop (mm);Signature · 1 line
1.7.1
kvm_arch_destroy_vmdoes it.Oh, indeed.
That I guess it's time to clean up, move generic allocation and release into generic code. Having kvm_arch_create/destroy_vm manage the kvm object allocation does not look very nice (and caused this confusion of mine).
Quoted 5 lines
Quoted 1 line
Jan
kvm_iommu_unmap_guestenters SRCU critical section, and "struct kvm" is freed at the end ofkvm_arch_destroy_vm. Thats why its not in generic code.
The problem is the arch-specific allocation of the kvm object on IA64. All others simply call kmalloc/kzalloc for the struct itself, but IA64 embeds it into some larger context. Not sure why, though.
Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux
Quoted 8 lines · Jan Kiszka
Quoted 4 lines
kvm_iommu_unmap_guestenters SRCU critical section, and "struct kvm" is freed at the end ofkvm_arch_destroy_vm. Thats why its not in generic code.The problem is the arch-specific allocation of the kvm object on IA64. All others simply call kmalloc/kzalloc for the struct itself, but IA64 embeds it into some larger context. Not sure why, though.
ia64 virt is special - the guest has a virtual address space for host data. This data is shared with the host address space, but on different addresses.
I guess to reduce tlb costs ia64 kvm uses a large order allocation for both struct kvm and other random data, which is why the allocation is different.
It could be refactored to something like
#ifndef CONFIG_HAVE_SPECIAL_KVM_ALLOC
static inline struct kvm *kvm_arch_alloc_vm(void)
{
return kzalloc(...);
}static inline void kvm_arch_free_vm(struct kvm *kvm) {
kfree(kvm);
}
#endif
and have ia64 provide its special stuff. The practice of duplicating common code just because of one outlier is bad, there are better ways.
(and I will accept untested ia64 patches provided kvm-ia64@ is copied).
Signature · 1 line
error compiling committee.c: too many arguments to function
Quoted 27 lines · Avi Kivity
Quoted 8 lines · Jan Kiszka
Quoted 4 lines
kvm_iommu_unmap_guestenters SRCU critical section, and "struct kvm" is freed at the end ofkvm_arch_destroy_vm. Thats why its not in generic code.The problem is the arch-specific allocation of the kvm object on IA64. All others simply call kmalloc/kzalloc for the struct itself, but IA64 embeds it into some larger context. Not sure why, though.
ia64 virt is special - the guest has a virtual address space for host data. This data is shared with the host address space, but on different addresses.
I guess to reduce tlb costs ia64 kvm uses a large order allocation for both struct kvm and other random data, which is why the allocation is different.
It could be refactored to something like
#ifndef
CONFIG_HAVE_SPECIAL_KVM_ALLOCstatic inline struct kvm
*kvm_arch_alloc_vm(void){ return kzalloc(...); }
Quoted 6 lines
static inline void
kvm_arch_free_vm(struct kvm *kvm){ kfree(kvm); }#endif
Yep, that's what I'm already working on.
Quoted 5 lines
and have ia64 provide its special stuff. The practice of duplicating common code just because of one outlier is bad, there are better ways.
(and I will accept untested ia64 patches provided kvm-ia64@ is copied).
OK, will come. Refactoring will affect all archs, so all need to test and ack.
Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux
Quoted 7 lines · Jan Kiszka
Quoted 5 lines
and have ia64 provide its special stuff. The practice of duplicating common code just because of one outlier is bad, there are better ways.
(and I will accept untested ia64 patches provided kvm-ia64@ is copied).
OK, will come. Refactoring will affect all archs, so all need to test and ack.
I can at least build on all of them. Should set up a buildbot...