| Message ID | 20181026151223.16810-6-ahmedsoliman0x666@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: X86: Introducing ROE Protection Kernel Hardening | expand |
On Fri, Oct 26, 2018 at 05:12:23PM +0200, Ahmed Abd El Mawgood wrote: > This should be a little bit more readable and prone to memory leaks Describe what is being, both in the subject line and continuing on in the full changelog, e.g. "Small Refactoring to kvm_free_memslot" doesn't provide any clue as to what is being done. And this is not what I would describe as refactoring, e.g. verifying the new behavior means tracing through its impact on __kvm_set_memory_region(). Lastly, this should be sent as a separate patch. There is no dependency on the ROE code and if it actually addresses a potential memory leak (I haven't actually reviewed the code itself) it should go in sooner rather than later. > > Signed-off-by: Ahmed Abd El Mawgood <ahmedsoliman0x666@gmail.com> > --- > virt/kvm/kvm_main.c | 15 +++++++-------- > 1 file changed, 7 insertions(+), 8 deletions(-) > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index 2d3011e8490e..79c98db03c84 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -550,11 +550,11 @@ static void kvm_destroy_dirty_bitmap(struct kvm_memory_slot *memslot) > * Free any memory in @free but not in @dont. > */ > static void kvm_free_memslot(struct kvm *kvm, struct kvm_memory_slot *free, > - struct kvm_memory_slot *dont) > + struct kvm_memory_slot *dont, > + enum kvm_mr_change change) > { > + if (change == KVM_MR_DELETE) { > #ifdef CONFIG_KVM_ROE > - if (!dont) { > - //TODO still this might leak > struct protected_chunk *pos, *n; > struct list_head *head = free->prot_list; > kvfree(free->roe_bitmap); > @@ -564,10 +564,9 @@ static void kvm_free_memslot(struct kvm *kvm, struct kvm_memory_slot *free, > kvfree(pos); > } > kvfree(free->prot_list); > - } > #endif > - if (!dont || free->dirty_bitmap != dont->dirty_bitmap) > kvm_destroy_dirty_bitmap(free); > + } > > kvm_arch_free_memslot(kvm, free, dont); > > @@ -582,7 +581,7 @@ static void kvm_free_memslots(struct kvm *kvm, struct kvm_memslots *slots) > return; > > kvm_for_each_memslot(memslot, slots) > - kvm_free_memslot(kvm, memslot, NULL); > + kvm_free_memslot(kvm, memslot, NULL, KVM_MR_DELETE); > > kvfree(slots); > } > @@ -1100,14 +1099,14 @@ int __kvm_set_memory_region(struct kvm *kvm, > > kvm_arch_commit_memory_region(kvm, mem, &old, &new, change); > > - kvm_free_memslot(kvm, &old, &new); > + kvm_free_memslot(kvm, &old, &new, change); > kvfree(old_memslots); > return 0; > > out_slots: > kvfree(slots); > out_free: > - kvm_free_memslot(kvm, &new, &old); > + kvm_free_memslot(kvm, &new, &old, change); > out: > return r; > } > -- > 2.18.1 >
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 2d3011e8490e..79c98db03c84 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -550,11 +550,11 @@ static void kvm_destroy_dirty_bitmap(struct kvm_memory_slot *memslot) * Free any memory in @free but not in @dont. */ static void kvm_free_memslot(struct kvm *kvm, struct kvm_memory_slot *free, - struct kvm_memory_slot *dont) + struct kvm_memory_slot *dont, + enum kvm_mr_change change) { + if (change == KVM_MR_DELETE) { #ifdef CONFIG_KVM_ROE - if (!dont) { - //TODO still this might leak struct protected_chunk *pos, *n; struct list_head *head = free->prot_list; kvfree(free->roe_bitmap); @@ -564,10 +564,9 @@ static void kvm_free_memslot(struct kvm *kvm, struct kvm_memory_slot *free, kvfree(pos); } kvfree(free->prot_list); - } #endif - if (!dont || free->dirty_bitmap != dont->dirty_bitmap) kvm_destroy_dirty_bitmap(free); + } kvm_arch_free_memslot(kvm, free, dont); @@ -582,7 +581,7 @@ static void kvm_free_memslots(struct kvm *kvm, struct kvm_memslots *slots) return; kvm_for_each_memslot(memslot, slots) - kvm_free_memslot(kvm, memslot, NULL); + kvm_free_memslot(kvm, memslot, NULL, KVM_MR_DELETE); kvfree(slots); } @@ -1100,14 +1099,14 @@ int __kvm_set_memory_region(struct kvm *kvm, kvm_arch_commit_memory_region(kvm, mem, &old, &new, change); - kvm_free_memslot(kvm, &old, &new); + kvm_free_memslot(kvm, &old, &new, change); kvfree(old_memslots); return 0; out_slots: kvfree(slots); out_free: - kvm_free_memslot(kvm, &new, &old); + kvm_free_memslot(kvm, &new, &old, change); out: return r; }
This should be a little bit more readable and prone to memory leaks Signed-off-by: Ahmed Abd El Mawgood <ahmedsoliman0x666@gmail.com> --- virt/kvm/kvm_main.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-)