Make page_find() return 0 for too-large addresses

PATCH Applied Eduardo Habkost 3 participants 10 messages Sep 12, 2008 – Sep 15, 2008 KVM

Share & tools

Eduardo Habkost

author

2008-09-12 19:00 UTC #0 lore raw

PATCH 1/1 Make page_find() return 0 for too-large addresses

On some cases, such as under KVM, tb_invalidate_phys_page_range() may be called for large addresses, when qemu is configured to more than 4GB of RAM.

On these cases, qemu was crashing because it was using an index too large for l1_map[], that supports only 32-bit addresses when compiling without CONFIG_USER_ONLY.

Below is a sample backtrace of this happening, under KVM. 'start' on tb_invalidate_phys_page_range() is 0x132b1c812, beyond 2^32, making page_find() return a bogus pointer.

 #0  tb_invalidate_phys_page_range (start=5145479186, end=5145479246, is_cpu_write_access=0) at /usr/src/debug/kvm-74/qemu/exec.c:877
 #1  0x000000000049cd28 in cpu_physical_memory_rw (addr=5682350098, buf=0x7389dd10 "RT", len=60, is_write=1) at /usr/src/debug/kvm-74/qemu/exec.c:2818
 #2  0x0000000000428f54 in rtl8139_do_receive (opaque=0x1f472240, buf=<value optimized out>, size=60, do_interrupt=1) at ../cpu-all.h:929
 #3  0x000000000040717b in qemu_send_packet (vc1=0x1e54f480, buf=0x7389ddc0 "RT", size=42) at /usr/src/debug/kvm-74/qemu/vl.c:4146
 #4  0x0000000000480990 in slirp_input (pkt=0x1f4a9010 "������RT", pkt_len=42) at slirp/slirp.c:597
 #5  0x000000000040717b in qemu_send_packet (vc1=0x1f475d70, buf=0x1f4a9010 "������RT", size=42) at /usr/src/debug/kvm-74/qemu/vl.c:4146
 #6  0x00000000004295bd in rtl8139_io_writeb (opaque=0x1f472240, addr=<value optimized out>, val=<value optimized out>) at /usr/src/debug/kvm-74/qemu/hw/rtl8139.c:2299
 #7  0x000000000049cb62 in cpu_physical_memory_rw (addr=4060090585, buf=0x2aaadcf11028 <Address 0x2aaadcf11028 out of bounds>, len=1, is_write=1)
     at /usr/src/debug/kvm-74/qemu/exec.c:2807
 #8  0x00000000004eebf8 in kvm_mmio_write (opaque=<value optimized out>, addr=5145479186, data=0x132b1c84e <Address 0x132b1c84e out of bounds>, len=707232)
     at /usr/src/debug/kvm-74/qemu/qemu-kvm.c:689
 #9  0x0000000000515646 in handle_mmio (kvm=0x1e54e040, kvm_run=0x2aaadcf11000) at libkvm.c:849
 #10 0x0000000000515b57 in kvm_run (kvm=0x1e54e040, vcpu=0) at libkvm.c:975

The following patch makes page_find() safe for >32-bit addresses, by just returning 0 if the address is too large.

The translation block handling code on exec.c still doesn't support addresses >32-bit if CONFIG_USER_ONLY is not defined (KVM qemu currently crashes immediately when using -no-kvm with more than 4 GB of memory), but this is another issue.

Signed-off-by Eduardo Habkost <[email protected]>

---

exec.c +4 -1

diff -purN qemu.orig/exec.c qemu/exec.c
@@ -317,8 +317,11 @@ static inline PageDesc *page_find_alloc(
 static inline PageDesc *page_find(target_ulong index)
 {
     PageDesc *p;
+    target_ulong l1_index = (index >> L2_BITS);
+    if (l1_index > L1_SIZE)
+        return 0;
 
-    p = l1_map[index >> L2_BITS];
+    p = l1_map[l1_index];
     if (!p)
         return 0;
     return p + (index & (L2_SIZE - 1));

Signature · 1 line

Eduardo

Anthony Liguori

2008-09-12 19:51 UTC #1 lore raw

Quoted 8 lines · Eduardo Habkost

On some cases, such as under KVM, tb_invalidate_phys_page_range() may be called for large addresses, when qemu is configured to more than 4GB of RAM.
On these cases, qemu was crashing because it was using an index too large for l1_map[], that supports only 32-bit addresses when compiling without CONFIG_USER_ONLY.

Did you have kqemu enabled in the build? l1_map should be sufficiently large when you have kqemu disabled.

Regards,

Anthony Liguori

Eduardo Habkost

author

2008-09-12 20:15 UTC #2 lore raw

Quoted 5 lines · Anthony Liguori

Quoted 4 lines · Eduardo Habkost
On some cases, such as under KVM, tb_invalidate_phys_page_range() may be called for large addresses, when qemu is configured to more than 4GB of RAM.

Quoted 7 lines

Quoted 4 lines
On these cases, qemu was crashing because it was using an index too large for l1_map[], that supports only 32-bit addresses when compiling without CONFIG_USER_ONLY.
Did you have kqemu enabled in the build? l1_map should be sufficiently large when you have kqemu disabled.

KVM uses './configure --disable-kqemu', but exec.c has this:

#if defined(CONFIG_USER_ONLY) && defined(TARGET_VIRT_ADDR_SPACE_BITS) /* XXX: this is a temporary hack for alpha target.
* In the future, this is to be replaced by a multi-level table
* to actually be able to handle the complete 64 bits address space.
*/
#define L1_BITS (TARGET_VIRT_ADDR_SPACE_BITS - L2_BITS - TARGET_PAGE_BITS) #else
#define L1_BITS (32 - L2_BITS - TARGET_PAGE_BITS)
#endif

And CONFIG_USER_ONLY is not defined, making l1_map work only for 32-bit addresses.

BTW, I've just noticed page_find_alloc() has this:

#if TARGET_LONG_BITS > 32
    /* Host memory outside guest VM.  For 32-bit targets we have already
       excluded high addresses.  */
    if (index > ((target_ulong)L2_SIZE * L1_SIZE))
        return NULL;

#endif

So, we can just use a similar check on page_find().

Side note: the check on the kvm git tree looks broken: it is checking for (L2_SIZE * L1_SIZE * TARGET_PAGE_SIZE) instead.

Signature · 1 line

Eduardo

Eduardo Habkost

author

2008-09-12 20:45 UTC #3 lore raw

Quoted 10 lines · Eduardo Habkost

Quoted 9 lines · Anthony Liguori
Quoted 8 lines · Eduardo Habkost
On some cases, such as under KVM, tb_invalidate_phys_page_range() may be called for large addresses, when qemu is configured to more than 4GB of RAM.
On these cases, qemu was crashing because it was using an index too large for l1_map[], that supports only 32-bit addresses when compiling without CONFIG_USER_ONLY.

<snip>

Quoted 11 lines

BTW, I've just noticed page_find_alloc() has this:
#if TARGET_LONG_BITS > 32
    /* Host memory outside guest VM.  For 32-bit targets we have already
       excluded high addresses.  */
    if (index > ((target_ulong)L2_SIZE * L1_SIZE))
        return NULL;
#endif
So, we can just use a similar check on page_find().

New patch, reusing the range check from page_find_alloc() on page_find(). Untested.

Signed-off-by Eduardo Habkost <[email protected]>

---

exec.c +16 -6

Index: qemu/exec.c
===================================================================
@@ -279,17 +279,24 @@ static void page_init(void)
 #endif
 }
 
-static inline PageDesc *page_find_alloc(target_ulong index)
+static inline PageDesc **page_l1_map(target_ulong index)
 {
-    PageDesc **lp, *p;
-
 #if TARGET_LONG_BITS > 32
     /* Host memory outside guest VM.  For 32-bit targets we have already
        excluded high addresses.  */
     if (index > ((target_ulong)L2_SIZE * L1_SIZE))
         return NULL;
 #endif
-    lp = &l1_map[index >> L2_BITS];
+    return &l1_map[index >> L2_BITS];
+}
+
+static inline PageDesc *page_find_alloc(target_ulong index)
+{
+    PageDesc **lp, *p;
+    lp = page_l1_map(index);
+    if (!lp)
+        return NULL;
+
     p = *lp;
     if (!p) {
         /* allocate if not found */

@@ -316,9 +323,12 @@ static inline PageDesc *page_find_alloc(
 
 static inline PageDesc *page_find(target_ulong index)
 {
-    PageDesc *p;
+    PageDesc **lp, *p;
+    lp = page_l1_map(index);
+    if (!lp)
+        return NULL;
 
-    p = l1_map[index >> L2_BITS];
+    p = *lp;
     if (!p)
         return 0;
     return p + (index & (L2_SIZE - 1));

Signature · 1 line

Eduardo

Anthony Liguori

2008-09-12 21:28 UTC #4 lore raw

Quoted 10 lines · Eduardo Habkost

Quoted 8 lines · Eduardo Habkost
Quoted 6 lines · Anthony Liguori
Quoted 4 lines · Eduardo Habkost
On some cases, such as under KVM, tb_invalidate_phys_page_range() may be called for large addresses, when qemu is configured to more than 4GB of RAM.

Quoted 7 lines

Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
On these cases, qemu was crashing because it was using an index too large for l1_map[], that supports only 32-bit addresses when compiling without CONFIG_USER_ONLY.
<snip>

So... are you building with kqemu enabled? If so, TARGET_PHYS_ADDR_SPACE_BITS will be 32. So I don't think this sort of work-around is correct. A better solution would be to validate ram_size against TARGET_PHYS_ADDR_SPACE_BITS.

To put it another way, if you have kqemu enabled (at build time), you cannot use > ~4GB of memory for the guest.

Regards,

Anthony Liguori

Eduardo Habkost

author

2008-09-12 21:49 UTC #5 lore raw

Quoted 11 lines · Anthony Liguori

Quoted 10 lines · Eduardo Habkost
Quoted 8 lines · Eduardo Habkost
Quoted 6 lines · Anthony Liguori
Quoted 4 lines · Eduardo Habkost
On some cases, such as under KVM, tb_invalidate_phys_page_range() may be called for large addresses, when qemu is configured to more than 4GB of RAM.

Quoted 8 lines

Quoted 6 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
On these cases, qemu was crashing because it was using an index too large for l1_map[], that supports only 32-bit addresses when compiling without CONFIG_USER_ONLY.
<snip>
So... are you building with kqemu enabled?

No, as I've stated in the reply I've sent before the patch.

Quoted 4 lines

If so,
TARGET_PHYS_ADDR_SPACE_BITS will be 32. So I don't think this sort of work-around is correct. A better solution would be to validate ram_size against TARGET_PHYS_ADDR_SPACE_BITS.

This is a good idea (maybe it is already checked?), but it is not the case I am dealing with.

Quoted 3 lines

To put it another way, if you have kqemu enabled (at build time), you cannot use > ~4GB of memory for the guest.

Right.

Signature · 1 line

Eduardo

Glauber Costa

2008-09-15 13:09 UTC #6 lore raw

In reply to Anthony Liguori

Quoted 18 lines · Anthony Liguori

Quoted 10 lines · Eduardo Habkost
Quoted 8 lines · Eduardo Habkost
Quoted 6 lines · Anthony Liguori
Quoted 4 lines · Eduardo Habkost
On some cases, such as under KVM, tb_invalidate_phys_page_range() may be called for large addresses, when qemu is configured to more than 4GB of RAM.
Quoted 6 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
On these cases, qemu was crashing because it was using an index too large for l1_map[], that supports only 32-bit addresses when compiling without CONFIG_USER_ONLY.
<snip>

Quoted 7 lines

So... are you building with kqemu enabled? If so, TARGET_PHYS_ADDR_SPACE_BITS will be 32. So I don't think this sort of work-around is correct. A better solution would be to validate ram_size against TARGET_PHYS_ADDR_SPACE_BITS.
To put it another way, if you have kqemu enabled (at build time), you cannot use > ~4GB of memory for the guest.

I can be very wrong here, but page_find() does not seem to limit our ability to run guests if > 4GB, as it is only concerned about the code translation layer. Differently from phys_page_find(), that does manage physical memory, and even presented problems about it in the past.

Quoted 5 lines

Regards,
Anthony Liguori

Anthony Liguori

2008-09-15 15:30 UTC #7 lore raw

In reply to Eduardo Habkost

Quoted 26 lines · Eduardo Habkost

Quoted 13 lines · Eduardo Habkost
Quoted 11 lines · Anthony Liguori
Quoted 9 lines · Eduardo Habkost
On some cases, such as under KVM, tb_invalidate_phys_page_range() may be called for large addresses, when qemu is configured to more than 4GB of RAM.
On these cases, qemu was crashing because it was using an index too large for l1_map[], that supports only 32-bit addresses when compiling without CONFIG_USER_ONLY.
<snip>
Quoted 9 lines
BTW, I've just noticed page_find_alloc() has this:
#if TARGET_LONG_BITS > 32
    /* Host memory outside guest VM.  For 32-bit targets we have already
       excluded high addresses.  */
    if (index > ((target_ulong)L2_SIZE * L1_SIZE))
        return NULL;
#endif

Quoted 6 lines

Quoted 2 lines
So, we can just use a similar check on page_find().
New patch, reusing the range check from page_find_alloc() on page_find(). Untested.

Have you tested this patch yet? I like to avoid being the first one to test something when it's not my code :-)

Regards,

Anthony Liguori

Quoted 53 lines

Signed-off-by Eduardo Habkost <[email protected]>

---
Index: qemu/exec.c
===================================================================

exec.c +16 -6

@@ -279,17 +279,24 @@ static void page_init(void)
 #endif
 }
 
-static inline PageDesc *page_find_alloc(target_ulong index)
+static inline PageDesc **page_l1_map(target_ulong index)
 {
-    PageDesc **lp, *p;
-
 #if TARGET_LONG_BITS > 32
     /* Host memory outside guest VM.  For 32-bit targets we have already
        excluded high addresses.  */
     if (index > ((target_ulong)L2_SIZE * L1_SIZE))
         return NULL;
 #endif
-    lp = &l1_map[index >> L2_BITS];
+    return &l1_map[index >> L2_BITS];
+}
+
+static inline PageDesc *page_find_alloc(target_ulong index)
+{
+    PageDesc **lp, *p;
+    lp = page_l1_map(index);
+    if (!lp)
+        return NULL;
+
     p = *lp;
     if (!p) {
         /* allocate if not found */

@@ -316,9 +323,12 @@ static inline PageDesc *page_find_alloc(
 
 static inline PageDesc *page_find(target_ulong index)
 {
-    PageDesc *p;
+    PageDesc **lp, *p;
+    lp = page_l1_map(index);
+    if (!lp)
+        return NULL;
 
-    p = l1_map[index >> L2_BITS];
+    p = *lp;
     if (!p)
         return 0;
     return p + (index & (L2_SIZE - 1));

Eduardo Habkost

author

2008-09-15 15:50 UTC #8 lore raw

Quoted 1 line · Anthony Liguori

Eduardo Habkost wrote:

<snip>

Quoted 7 lines

Quoted 4 lines
New patch, reusing the range check from page_find_alloc() on page_find(). Untested.
Have you tested this patch yet? I like to avoid being the first one to test something when it's not my code :-)

I've tested it now, both with KVM enabled and with KVM disabled. My machine didn't explode yet, so it seems to be fine. :)

Quoted 57 lines

Regards,

Anthony Liguori

Quoted 50 lines

Signed-off-by Eduardo Habkost <[email protected]>

---
Index: qemu/exec.c
===================================================================

exec.c +16 -4

@@ -279,17 +279,24 @@ static void page_init(void)
 #endif
 }
 -static inline PageDesc *page_find_alloc(target_ulong index)
+static inline PageDesc **page_l1_map(target_ulong index)
 {
-    PageDesc **lp, *p;
-
 #if TARGET_LONG_BITS > 32
     /* Host memory outside guest VM.  For 32-bit targets we have already
        excluded high addresses.  */
     if (index > ((target_ulong)L2_SIZE * L1_SIZE))
         return NULL;
 #endif
-    lp = &l1_map[index >> L2_BITS];
+    return &l1_map[index >> L2_BITS];
+}
+
+static inline PageDesc *page_find_alloc(target_ulong index)
+{
+    PageDesc **lp, *p;
+    lp = page_l1_map(index);
+    if (!lp)
+        return NULL;
+
     p = *lp;
     if (!p) {
         /* allocate if not found */

@@ -316,9 +323,12 @@ static inline PageDesc *page_find_alloc(
  static inline PageDesc *page_find(target_ulong index)
 {
-    PageDesc *p;
+    PageDesc **lp, *p;
+    lp = page_l1_map(index);
+    if (!lp)
+        return NULL;
 -    p = l1_map[index >> L2_BITS];
+    p = *lp;
     if (!p)
         return 0;
     return p + (index & (L2_SIZE - 1));

Signature · 1 line

Eduardo

Anthony Liguori

Applied

2008-09-15 15:58 UTC #9 lore raw

Quoted 10 lines · Eduardo Habkost

Quoted 2 lines · Anthony Liguori
Eduardo Habkost wrote:
<snip>
Quoted 4 lines
Quoted 4 lines
New patch, reusing the range check from page_find_alloc() on page_find(). Untested.

Quoted 6 lines

Quoted 2 lines
Have you tested this patch yet? I like to avoid being the first one to test something when it's not my code :-)
I've tested it now, both with KVM enabled and with KVM disabled. My machine didn't explode yet, so it seems to be fine. :)

Applied. Thanks.

Regards,

Anthony Liguori