[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Mon, 29 Oct 2018 09:51:09 -0700
From: Sean Christopherson <sean.j.christopherson@...el.com>
To: Ahmed Abd El Mawgood <ahmedsoliman0x666@...il.com>
Cc: Paolo Bonzini <pbonzini@...hat.com>, rkrcmar@...hat.com,
Jonathan Corbet <corbet@....net>,
Thomas Gleixner <tglx@...utronix.de>,
Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
hpa@...or.com, x86@...nel.org, kvm@...r.kernel.org,
linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
ovich00@...il.com, kernel-hardening@...ts.openwall.com,
nigel.edwards@....com, Boris Lukashev <blukashev@...pervictus.com>,
Hossam Hassan <7ossam9063@...il.com>,
Ahmed Lotfy <A7med.lotfey@...il.com>
Subject: Re: [PATCH V5 5/5] KVM: Small Refactoring to kvm_free_memslot
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@...il.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
>
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
