* [PATCH][next] KVM: x86: Fix allocation sizeof argument
@ 2021-10-01 11:01 Colin King
2021-10-05 15:41 ` Sean Christopherson
0 siblings, 1 reply; 7+ messages in thread
From: Colin King @ 2021-10-01 11:01 UTC (permalink / raw)
To: Paolo Bonzini, Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li,
Jim Mattson, Joerg Roedel, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, x86, H . Peter Anvin, David Stevens, kvm
Cc: kernel-janitors, linux-kernel
From: Colin Ian King <colin.king@canonical.com>
The allocation for *gfn_track should be for a slot->npages lot of
short integers, however the current allocation is using sizeof(*gfn_track)
and that is the size of a pointer, which is too large. Fix this by
using sizeof(**gfn_track) instead.
Addresses-Coverity: ("Wrong sizeof argument")
Fixes: 35b330bba6a7 ("KVM: x86: only allocate gfn_track when necessary")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
arch/x86/kvm/mmu/page_track.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/kvm/mmu/page_track.c b/arch/x86/kvm/mmu/page_track.c
index bb5d60bd4dbf..5b785a5f7dc9 100644
--- a/arch/x86/kvm/mmu/page_track.c
+++ b/arch/x86/kvm/mmu/page_track.c
@@ -92,7 +92,7 @@ int kvm_page_track_enable_mmu_write_tracking(struct kvm *kvm)
slots = __kvm_memslots(kvm, i);
kvm_for_each_memslot(slot, slots) {
gfn_track = slot->arch.gfn_track + KVM_PAGE_TRACK_WRITE;
- *gfn_track = kvcalloc(slot->npages, sizeof(*gfn_track),
+ *gfn_track = kvcalloc(slot->npages, sizeof(**gfn_track),
GFP_KERNEL_ACCOUNT);
if (*gfn_track == NULL) {
mutex_unlock(&kvm->slots_arch_lock);
--
2.32.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH][next] KVM: x86: Fix allocation sizeof argument
2021-10-01 11:01 [PATCH][next] KVM: x86: Fix allocation sizeof argument Colin King
@ 2021-10-05 15:41 ` Sean Christopherson
2021-10-05 17:27 ` Paolo Bonzini
2021-10-06 0:22 ` David Stevens
0 siblings, 2 replies; 7+ messages in thread
From: Sean Christopherson @ 2021-10-05 15:41 UTC (permalink / raw)
To: Colin King
Cc: Paolo Bonzini, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
Joerg Roedel, Thomas Gleixner, Ingo Molnar, Borislav Petkov, x86,
H . Peter Anvin, David Stevens, kvm, kernel-janitors,
linux-kernel
On Fri, Oct 01, 2021, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> The allocation for *gfn_track should be for a slot->npages lot of
> short integers, however the current allocation is using sizeof(*gfn_track)
> and that is the size of a pointer, which is too large. Fix this by
> using sizeof(**gfn_track) instead.
>
> Addresses-Coverity: ("Wrong sizeof argument")
> Fixes: 35b330bba6a7 ("KVM: x86: only allocate gfn_track when necessary")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
> arch/x86/kvm/mmu/page_track.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/kvm/mmu/page_track.c b/arch/x86/kvm/mmu/page_track.c
> index bb5d60bd4dbf..5b785a5f7dc9 100644
> --- a/arch/x86/kvm/mmu/page_track.c
> +++ b/arch/x86/kvm/mmu/page_track.c
> @@ -92,7 +92,7 @@ int kvm_page_track_enable_mmu_write_tracking(struct kvm *kvm)
> slots = __kvm_memslots(kvm, i);
> kvm_for_each_memslot(slot, slots) {
> gfn_track = slot->arch.gfn_track + KVM_PAGE_TRACK_WRITE;
> - *gfn_track = kvcalloc(slot->npages, sizeof(*gfn_track),
> + *gfn_track = kvcalloc(slot->npages, sizeof(**gfn_track),
> GFP_KERNEL_ACCOUNT);
Eww (not your patch, the original code). IMO the double indirection is completely
unnecessary, e.g. I find this far easier to follow
diff --git a/arch/x86/kvm/mmu/page_track.c b/arch/x86/kvm/mmu/page_track.c
index bb5d60bd4dbf..8cae41b831dd 100644
--- a/arch/x86/kvm/mmu/page_track.c
+++ b/arch/x86/kvm/mmu/page_track.c
@@ -75,7 +75,7 @@ int kvm_page_track_enable_mmu_write_tracking(struct kvm *kvm)
{
struct kvm_memslots *slots;
struct kvm_memory_slot *slot;
- unsigned short **gfn_track;
+ unsigned short *gfn_track;
int i;
if (write_tracking_enabled(kvm))
@@ -91,13 +91,13 @@ int kvm_page_track_enable_mmu_write_tracking(struct kvm *kvm)
for (i = 0; i < KVM_ADDRESS_SPACE_NUM; i++) {
slots = __kvm_memslots(kvm, i);
kvm_for_each_memslot(slot, slots) {
- gfn_track = slot->arch.gfn_track + KVM_PAGE_TRACK_WRITE;
- *gfn_track = kvcalloc(slot->npages, sizeof(*gfn_track),
- GFP_KERNEL_ACCOUNT);
- if (*gfn_track == NULL) {
+ gfn_track = kvcalloc(slot->npages, sizeof(*gfn_track),
+ GFP_KERNEL_ACCOUNT);
+ if (gfn_track == NULL) {
mutex_unlock(&kvm->slots_arch_lock);
return -ENOMEM;
}
+ slot->arch.gfn_track[KVM_PAGE_TRACK_WRITE] = gfn_track;
}
}
> if (*gfn_track == NULL) {
> mutex_unlock(&kvm->slots_arch_lock);
Hrm, this fails to free the gfn_track allocations for previous memslots. The
on-demand rmaps code has the exact same bug (it frees rmaps for previous lpages
in the _current_ slot, but does not free previous slots).
And having two separate flows (and flags) for rmaps vs. gfn_track is pointless,
and means we have to maintain two near-identical copies of non-obvious code.
Paolo, is it too late to just drop the original deae4a10f166 ("KVM: x86: only
allocate gfn_track when necessary")?
> --
> 2.32.0
>
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH][next] KVM: x86: Fix allocation sizeof argument
2021-10-05 15:41 ` Sean Christopherson
@ 2021-10-05 17:27 ` Paolo Bonzini
2021-10-05 17:55 ` Sean Christopherson
2021-10-06 0:22 ` David Stevens
1 sibling, 1 reply; 7+ messages in thread
From: Paolo Bonzini @ 2021-10-05 17:27 UTC (permalink / raw)
To: Sean Christopherson, Colin King
Cc: Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel,
Thomas Gleixner, Ingo Molnar, Borislav Petkov, x86,
H . Peter Anvin, David Stevens, kvm, kernel-janitors,
linux-kernel
On 05/10/21 17:41, Sean Christopherson wrote:
>> if (*gfn_track == NULL) {
>> mutex_unlock(&kvm->slots_arch_lock);
> Hrm, this fails to free the gfn_track allocations for previous memslots. The
> on-demand rmaps code has the exact same bug (it frees rmaps for previous lpages
> in the_current_ slot, but does not free previous slots).
That's not a huge deal because the syscall is failing. So as long as
it's not leaked forever, it's okay. The problem is the
WARN_ON(slot->arch.rmap[i]), or the missing check in
kvm_page_track_enable_mmu_write_tracking, but that's easily fixed. I'd
even remove the call to memslot_rmaps_free.
> And having two separate flows (and flags) for rmaps vs. gfn_track is pointless,
> and means we have to maintain two near-identical copies of non-obvious code.
I was thinking the separate flow (not so much the flag) is needed
because, if KVMGT is enabled, gfn_track is allocated unconditionally.
rmaps are added on top of that if shadow paging is enabled; but
kvm_page_track_create_memslot will have already created the counter,
including the one for KVM_PAGE_TRACK_WRITE.
But looking at the code again, I guess you could call
kvm_page_track_enable_mmu_write_tracking inside alloc_all_memslots_rmaps
(with a little bit of renaming), and with that the flag would go away.
I'll take a look tomorrow, but I'd rather avoid reverting the patch.
Thanks,
Paolo
> Paolo, is it too late to just drop the original deae4a10f166 ("KVM: x86: only
> allocate gfn_track when necessary")?
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH][next] KVM: x86: Fix allocation sizeof argument
2021-10-05 17:27 ` Paolo Bonzini
@ 2021-10-05 17:55 ` Sean Christopherson
2021-10-05 20:52 ` Paolo Bonzini
0 siblings, 1 reply; 7+ messages in thread
From: Sean Christopherson @ 2021-10-05 17:55 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Colin King, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
Joerg Roedel, Thomas Gleixner, Ingo Molnar, Borislav Petkov, x86,
H . Peter Anvin, David Stevens, kvm, kernel-janitors,
linux-kernel
On Tue, Oct 05, 2021, Paolo Bonzini wrote:
> On 05/10/21 17:41, Sean Christopherson wrote:
> > > if (*gfn_track == NULL) {
> > > mutex_unlock(&kvm->slots_arch_lock);
> > Hrm, this fails to free the gfn_track allocations for previous memslots. The
> > on-demand rmaps code has the exact same bug (it frees rmaps for previous lpages
> > in the_current_ slot, but does not free previous slots).
>
> That's not a huge deal because the syscall is failing. So as long as it's
> not leaked forever, it's okay. The problem is the
> WARN_ON(slot->arch.rmap[i]), or the missing check in
> kvm_page_track_enable_mmu_write_tracking, but that's easily fixed. I'd even
> remove the call to memslot_rmaps_free.
It can be leaked forever though, e.g. if userspace invokes KVM_RUN over and over
on -ENOMEM. That would trigger the WARN_ON(slot->arch.rmap[i]) and leak the
previous allocation. I think it would be safe to change that WARN_ON to a
check-and-continue, i.e. to preserve the previous allocation
> > And having two separate flows (and flags) for rmaps vs. gfn_track is pointless,
> > and means we have to maintain two near-identical copies of non-obvious code.
>
> I was thinking the separate flow (not so much the flag) is needed because,
> if KVMGT is enabled, gfn_track is allocated unconditionally. rmaps are added
> on top of that if shadow paging is enabled; but
> kvm_page_track_create_memslot will have already created the counter,
> including the one for KVM_PAGE_TRACK_WRITE.
>
> But looking at the code again, I guess you could call
> kvm_page_track_enable_mmu_write_tracking inside alloc_all_memslots_rmaps
> (with a little bit of renaming), and with that the flag would go away.
Yes, and reuse the control flow, which is what I really care about since that's
the part that both features get wrong.
> I'll take a look tomorrow, but I'd rather avoid reverting the patch.
I can poke at it too if you don't have time. I wasn't suggesting a full revert,
rather a "drop and pretend it never got applied", with a plan to apply a new
version instead of fixing up the current code.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH][next] KVM: x86: Fix allocation sizeof argument
2021-10-05 17:55 ` Sean Christopherson
@ 2021-10-05 20:52 ` Paolo Bonzini
0 siblings, 0 replies; 7+ messages in thread
From: Paolo Bonzini @ 2021-10-05 20:52 UTC (permalink / raw)
To: Sean Christopherson
Cc: Colin King, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
Joerg Roedel, Thomas Gleixner, Ingo Molnar, Borislav Petkov, x86,
H . Peter Anvin, David Stevens, kvm, kernel-janitors,
linux-kernel
On 05/10/21 19:55, Sean Christopherson wrote:
> I wasn't suggesting a full revert,
> rather a "drop and pretend it never got applied", with a plan to apply a new
> version instead of fixing up the current code.
Considering that there are issues in the rmaps as well, I'd rather fix
both the right way.
Paolo
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH][next] KVM: x86: Fix allocation sizeof argument
2021-10-05 15:41 ` Sean Christopherson
2021-10-05 17:27 ` Paolo Bonzini
@ 2021-10-06 0:22 ` David Stevens
2021-10-06 0:41 ` Sean Christopherson
1 sibling, 1 reply; 7+ messages in thread
From: David Stevens @ 2021-10-06 0:22 UTC (permalink / raw)
To: Sean Christopherson
Cc: Colin King, Paolo Bonzini, Vitaly Kuznetsov, Wanpeng Li,
Jim Mattson, Joerg Roedel, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, x86, H . Peter Anvin, kvm, kernel-janitors,
open list
On Wed, Oct 6, 2021 at 12:41 AM Sean Christopherson <seanjc@google.com> wrote:
>
> On Fri, Oct 01, 2021, Colin King wrote:
> > From: Colin Ian King <colin.king@canonical.com>
> >
> > The allocation for *gfn_track should be for a slot->npages lot of
> > short integers, however the current allocation is using sizeof(*gfn_track)
> > and that is the size of a pointer, which is too large. Fix this by
> > using sizeof(**gfn_track) instead.
> >
> > Addresses-Coverity: ("Wrong sizeof argument")
> > Fixes: 35b330bba6a7 ("KVM: x86: only allocate gfn_track when necessary")
> > Signed-off-by: Colin Ian King <colin.king@canonical.com>
> > ---
> > arch/x86/kvm/mmu/page_track.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/x86/kvm/mmu/page_track.c b/arch/x86/kvm/mmu/page_track.c
> > index bb5d60bd4dbf..5b785a5f7dc9 100644
> > --- a/arch/x86/kvm/mmu/page_track.c
> > +++ b/arch/x86/kvm/mmu/page_track.c
> > @@ -92,7 +92,7 @@ int kvm_page_track_enable_mmu_write_tracking(struct kvm *kvm)
> > slots = __kvm_memslots(kvm, i);
> > kvm_for_each_memslot(slot, slots) {
> > gfn_track = slot->arch.gfn_track + KVM_PAGE_TRACK_WRITE;
> > - *gfn_track = kvcalloc(slot->npages, sizeof(*gfn_track),
> > + *gfn_track = kvcalloc(slot->npages, sizeof(**gfn_track),
> > GFP_KERNEL_ACCOUNT);
>
> Eww (not your patch, the original code). IMO the double indirection is completely
> unnecessary, e.g. I find this far easier to follow
>
> diff --git a/arch/x86/kvm/mmu/page_track.c b/arch/x86/kvm/mmu/page_track.c
> index bb5d60bd4dbf..8cae41b831dd 100644
> --- a/arch/x86/kvm/mmu/page_track.c
> +++ b/arch/x86/kvm/mmu/page_track.c
> @@ -75,7 +75,7 @@ int kvm_page_track_enable_mmu_write_tracking(struct kvm *kvm)
> {
> struct kvm_memslots *slots;
> struct kvm_memory_slot *slot;
> - unsigned short **gfn_track;
> + unsigned short *gfn_track;
> int i;
>
> if (write_tracking_enabled(kvm))
> @@ -91,13 +91,13 @@ int kvm_page_track_enable_mmu_write_tracking(struct kvm *kvm)
> for (i = 0; i < KVM_ADDRESS_SPACE_NUM; i++) {
> slots = __kvm_memslots(kvm, i);
> kvm_for_each_memslot(slot, slots) {
> - gfn_track = slot->arch.gfn_track + KVM_PAGE_TRACK_WRITE;
> - *gfn_track = kvcalloc(slot->npages, sizeof(*gfn_track),
> - GFP_KERNEL_ACCOUNT);
> - if (*gfn_track == NULL) {
> + gfn_track = kvcalloc(slot->npages, sizeof(*gfn_track),
> + GFP_KERNEL_ACCOUNT);
> + if (gfn_track == NULL) {
> mutex_unlock(&kvm->slots_arch_lock);
> return -ENOMEM;
> }
> + slot->arch.gfn_track[KVM_PAGE_TRACK_WRITE] = gfn_track;
> }
> }
>
>
>
> > if (*gfn_track == NULL) {
> > mutex_unlock(&kvm->slots_arch_lock);
>
> Hrm, this fails to free the gfn_track allocations for previous memslots. The
> on-demand rmaps code has the exact same bug (it frees rmaps for previous lpages
> in the _current_ slot, but does not free previous slots).
>
> And having two separate flows (and flags) for rmaps vs. gfn_track is pointless,
> and means we have to maintain two near-identical copies of non-obvious code.
I agree that's better than my patch. I can put together a new patch
once it's decided whether or not my patch should be dropped.
-David
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH][next] KVM: x86: Fix allocation sizeof argument
2021-10-06 0:22 ` David Stevens
@ 2021-10-06 0:41 ` Sean Christopherson
0 siblings, 0 replies; 7+ messages in thread
From: Sean Christopherson @ 2021-10-06 0:41 UTC (permalink / raw)
To: David Stevens
Cc: Colin King, Paolo Bonzini, Vitaly Kuznetsov, Wanpeng Li,
Jim Mattson, Joerg Roedel, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, x86, H . Peter Anvin, kvm, kernel-janitors,
open list
On Wed, Oct 06, 2021, David Stevens wrote:
> On Wed, Oct 6, 2021 at 12:41 AM Sean Christopherson <seanjc@google.com> wrote:
> > Hrm, this fails to free the gfn_track allocations for previous memslots. The
> > on-demand rmaps code has the exact same bug (it frees rmaps for previous lpages
> > in the _current_ slot, but does not free previous slots).
> >
> > And having two separate flows (and flags) for rmaps vs. gfn_track is pointless,
> > and means we have to maintain two near-identical copies of non-obvious code.
>
> I agree that's better than my patch. I can put together a new patch
> once it's decided whether or not my patch should be dropped.
All yours, unless Paolo wants to fight you for it :-) I'm totally ok doing
cleanup/fixes on top.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-10-06 0:41 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-01 11:01 [PATCH][next] KVM: x86: Fix allocation sizeof argument Colin King
2021-10-05 15:41 ` Sean Christopherson
2021-10-05 17:27 ` Paolo Bonzini
2021-10-05 17:55 ` Sean Christopherson
2021-10-05 20:52 ` Paolo Bonzini
2021-10-06 0:22 ` David Stevens
2021-10-06 0:41 ` Sean Christopherson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).