Re: [PATCH] KVM: fix the handling of dirty bitmaps to avoid overflows
BTW, just from my curiosity, are there any cases in which we use such huge number of pages currently? ALIGN(memslot-npages, BITS_PER_LONG) / 8; More than G pages need really big memory! -- We are assuming some special cases like short int size? No, int is 32 bits, but memslot-npages is not our under control. Note that you don't actually need all those pages to create a large memory slot. If so, we may have to care about a lot of things from now on, because common functions like __set_bit() don't support such long buffers. It's better to limit memory slots to something that can be handled by everything, then. 2^31 pages is plenty. Return -EINVAL if the slot is too large. I agree with that, so we make this patch pending to fix like that? -- or should make a new patch based on this patch? -- To unsubscribe from this list: send the line unsubscribe kvm-ppc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] KVM: fix the handling of dirty bitmaps to avoid overflows
On 04/13/2010 10:03 AM, Takuya Yoshikawa wrote: It's better to limit memory slots to something that can be handled by everything, then. 2^31 pages is plenty. Return -EINVAL if the slot is too large. I agree with that, so we make this patch pending to fix like that? -- or should make a new patch based on this patch? We need a new patch to block oversize memory slots. The current patch can come on top (but now page numbers fit inside an int, so it is just a cleanup, not a bugfix). -- Do not meddle in the internals of kernels, for they are subtle and quick to panic. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] KVM: fix the handling of dirty bitmaps to avoid overflows
On 12.04.2010, at 12:35, Takuya Yoshikawa wrote:
This patch fixes a bug found by Avi during the review process
of my dirty bitmap related work.
To ppc and ia64 people:
The fix is really simple but touches all architectures using
dirty bitmaps. So please check this will not suffer your part.
===
Int is not long enough to store the size of a dirty bitmap.
This patch fixes this problem with the introduction of a wrapper
function to calculate the sizes of dirty bitmaps.
Note: in mark_page_dirty(), we have to consider the fact that
__set_bit() takes the offset as int, not long.
Signed-off-by: Takuya Yoshikawa yoshikawa.tak...@oss.ntt.co.jp
---
arch/ia64/kvm/kvm-ia64.c |9 +
arch/powerpc/kvm/book3s.c |5 +++--
arch/x86/kvm/x86.c|5 +++--
include/linux/kvm_host.h |5 +
virt/kvm/kvm_main.c | 13 -
5 files changed, 24 insertions(+), 13 deletions(-)
diff --git a/arch/ia64/kvm/kvm-ia64.c b/arch/ia64/kvm/kvm-ia64.c
index b0ed80c..566aff6 100644
--- a/arch/ia64/kvm/kvm-ia64.c
+++ b/arch/ia64/kvm/kvm-ia64.c
@@ -1806,7 +1806,8 @@ static int kvm_ia64_sync_dirty_log(struct kvm *kvm,
{
struct kvm_memory_slot *memslot;
int r, i;
- long n, base;
+ long base;
+ unsigned long n;
unsigned long *dirty_bitmap = (unsigned long *)(kvm-arch.vm_base +
offsetof(struct kvm_vm_data, kvm_mem_dirty_log));
@@ -1819,7 +1820,7 @@ static int kvm_ia64_sync_dirty_log(struct kvm *kvm,
if (!memslot-dirty_bitmap)
goto out;
- n = ALIGN(memslot-npages, BITS_PER_LONG) / 8;
+ n = kvm_dirty_bitmap_bytes(memslot);
base = memslot-base_gfn / BITS_PER_LONG;
for (i = 0; i n/sizeof(long); ++i) {
@@ -1835,7 +1836,7 @@ int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
struct kvm_dirty_log *log)
{
int r;
- int n;
+ unsigned long n;
struct kvm_memory_slot *memslot;
int is_dirty = 0;
@@ -1854,7 +1855,7 @@ int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
if (is_dirty) {
kvm_flush_remote_tlbs(kvm);
memslot = kvm-memslots-memslots[log-slot];
- n = ALIGN(memslot-npages, BITS_PER_LONG) / 8;
+ n = kvm_dirty_bitmap_bytes(memslot);
memset(memslot-dirty_bitmap, 0, n);
}
r = 0;
diff --git a/arch/powerpc/kvm/book3s.c b/arch/powerpc/kvm/book3s.c
index a7ab2ea..4ac7b15 100644
--- a/arch/powerpc/kvm/book3s.c
+++ b/arch/powerpc/kvm/book3s.c
@@ -1118,7 +1118,8 @@ int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
struct kvm_vcpu *vcpu;
ulong ga, ga_end;
int is_dirty = 0;
- int r, n;
+ int r;
+ unsigned long n;
mutex_lock(kvm-slots_lock);
@@ -1136,7 +1137,7 @@ int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
kvm_for_each_vcpu(n, vcpu, kvm)
kvmppc_mmu_pte_pflush(vcpu, ga, ga_end);
- n = ALIGN(memslot-npages, BITS_PER_LONG) / 8;
+ n = kvm_dirty_bitmap_bytes(memslot);
Looks like a pretty mechanical change, so no objections from me.
Alex
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] KVM: fix the handling of dirty bitmaps to avoid overflows
On Mon, Apr 12, 2010 at 07:35:35PM +0900, Takuya Yoshikawa wrote: This patch fixes a bug found by Avi during the review process of my dirty bitmap related work. To ppc and ia64 people: The fix is really simple but touches all architectures using dirty bitmaps. So please check this will not suffer your part. === Int is not long enough to store the size of a dirty bitmap. This patch fixes this problem with the introduction of a wrapper function to calculate the sizes of dirty bitmaps. Note: in mark_page_dirty(), we have to consider the fact that __set_bit() takes the offset as int, not long. Signed-off-by: Takuya Yoshikawa yoshikawa.tak...@oss.ntt.co.jp Applied, thanks. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] KVM: fix the handling of dirty bitmaps to avoid overflows
(2010/04/13 2:39), Marcelo Tosatti wrote:
On Mon, Apr 12, 2010 at 07:35:35PM +0900, Takuya Yoshikawa wrote:
This patch fixes a bug found by Avi during the review process
of my dirty bitmap related work.
To ppc and ia64 people:
The fix is really simple but touches all architectures using
dirty bitmaps. So please check this will not suffer your part.
===
Int is not long enough to store the size of a dirty bitmap.
This patch fixes this problem with the introduction of a wrapper
function to calculate the sizes of dirty bitmaps.
Note: in mark_page_dirty(), we have to consider the fact that
__set_bit() takes the offset as int, not long.
Signed-off-by: Takuya Yoshikawayoshikawa.tak...@oss.ntt.co.jp
Applied, thanks.
Thanks everyone!
BTW, just from my curiosity, are there any cases in which we use such huge
number of pages currently?
ALIGN(memslot-npages, BITS_PER_LONG) / 8;
More than G pages need really big memory!
-- We are assuming some special cases like short int size?
If so, we may have to care about a lot of things from now on, because common
functions like __set_bit() don't support such long buffers.
If not, my patch might be over hacking -- especially the following part:
@@ -1183,10 +1183,13 @@ void mark_page_dirty(struct kvm *kvm, gfn_t gfn)
memslot = gfn_to_memslot_unaliased(kvm, gfn);
if (memslot memslot-dirty_bitmap) {
unsigned long rel_gfn = gfn - memslot-base_gfn;
+ unsigned long *p = memslot-dirty_bitmap +
+ rel_gfn / BITS_PER_LONG;
+ int offset = rel_gfn % BITS_PER_LONG;
/* avoid RMW */
- if (!generic_test_le_bit(rel_gfn, memslot-dirty_bitmap))
- generic___set_le_bit(rel_gfn, memslot-dirty_bitmap);
+ if (!generic_test_le_bit(offset, p))
+ generic___set_le_bit(offset, p);
}
}
--
To unsubscribe from this list: send the line unsubscribe kvm-ppc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
