bitopts functions overflowing page boundarys

bitopts functions overflowing page boundarys

[Scott Parish]

[-- Attachment #1: Type: text/plain, Size: 961 bytes --]

u.inuse.type_info is at the end of the pfn_info structure, and is
u32 for both x86_32 and x86_64--in this location it can also be the
last 32 bits of a page.

several functions use bitopts.h functions to manipulate this member, and
on x86_64 these functions use u64 instructions, which will overflow the
page boundary, and possibly the end of memory as we see here:

   (XEN) &page->u.inuse.type_info = 0xffff828402fffffc
   (XEN) CPU:    0
   (XEN) EIP:    e010:[<ffff830000129089>]      

   <registers and stack ommitted>

   (XEN) Pagetable walk from ffff828403000000:
   (XEN)  L4 = 00000000016a5063
   (XEN)   L3 = 00000000016a6063
   (XEN)    L2 = 0000000000000000 

   ****************************************
   Panic on CPU0:
   CPU0 FATAL PAGE FAULT
   [error_code=0002]
   Faulting linear address: ffff828403000000
   ****************************************

the attached patch fixes this.

sRp

-- 
Scott Parish
Signed-off-by: srparish@us.ibm.com

[-- Attachment #2: 32bit.diff --]
[-- Type: text/plain, Size: 6607 bytes --]

diff -rN -u -p old-xen-64-4/xen/arch/x86/domain.c new-xen-64-4/xen/arch/x86/domain.c
--- old-xen-64-4/xen/arch/x86/domain.c	2005-05-25 18:41:08.000000000 +0000
+++ new-xen-64-4/xen/arch/x86/domain.c	2005-05-28 02:52:19.000000000 +0000
@@ -927,10 +927,10 @@ static void relinquish_memory(struct dom
             continue;
         }
 
-        if ( test_and_clear_bit(_PGT_pinned, &page->u.inuse.type_info) )
+        if ( test_and_clear_bit32(_PGT_pinned, &page->u.inuse.type_info) )
             put_page_and_type(page);
 
-        if ( test_and_clear_bit(_PGC_allocated, &page->count_info) )
+        if ( test_and_clear_bit32(_PGC_allocated, &page->count_info) )
             put_page(page);
 
         /*
diff -rN -u -p old-xen-64-4/xen/arch/x86/domain_build.c new-xen-64-4/xen/arch/x86/domain_build.c
--- old-xen-64-4/xen/arch/x86/domain_build.c	2005-05-25 18:41:08.000000000 +0000
+++ new-xen-64-4/xen/arch/x86/domain_build.c	2005-05-28 02:41:41.000000000 +0000
@@ -309,7 +309,7 @@ int construct_dom0(struct domain *d,
             /* Get another ref to L2 page so that it can be pinned. */
             if ( !get_page_and_type(page, d, PGT_l2_page_table) )
                 BUG();
-            set_bit(_PGT_pinned, &page->u.inuse.type_info);
+            set_bit32(_PGT_pinned, &page->u.inuse.type_info);
         }
         else
         {
diff -rN -u -p old-xen-64-4/xen/arch/x86/mm.c new-xen-64-4/xen/arch/x86/mm.c
--- old-xen-64-4/xen/arch/x86/mm.c	2005-05-24 17:35:00.000000000 +0000
+++ new-xen-64-4/xen/arch/x86/mm.c	2005-05-28 02:45:37.000000000 +0000
@@ -1299,7 +1299,7 @@ int get_page_type(struct pfn_info *page,
         }
 
         /* Noone else is updating simultaneously. */
-        __set_bit(_PGT_validated, &page->u.inuse.type_info);
+        __set_bit32(_PGT_validated, &page->u.inuse.type_info);
     }
 
     return 1;
@@ -1522,8 +1522,8 @@ int do_mmuext_op(
                 break;
             }
             
-            if ( unlikely(test_and_set_bit(_PGT_pinned,
-                                           &page->u.inuse.type_info)) )
+            if ( unlikely(test_and_set_bit32(_PGT_pinned,
+                                             &page->u.inuse.type_info)) )
             {
                 MEM_LOG("Mfn %lx already pinned", op.mfn);
                 put_page_and_type(page);
@@ -1553,8 +1553,8 @@ int do_mmuext_op(
                 MEM_LOG("Mfn %lx bad domain (dom=%p)",
                         op.mfn, page_get_owner(page));
             }
-            else if ( likely(test_and_clear_bit(_PGT_pinned, 
-                                                &page->u.inuse.type_info)) )
+            else if ( likely(test_and_clear_bit32(_PGT_pinned, 
+                                                  &page->u.inuse.type_info)) )
             {
                 put_page_and_type(page);
                 put_page(page);
diff -rN -u -p old-xen-64-4/xen/arch/x86/shadow.c new-xen-64-4/xen/arch/x86/shadow.c
--- old-xen-64-4/xen/arch/x86/shadow.c	2005-05-25 18:41:08.000000000 +0000
+++ new-xen-64-4/xen/arch/x86/shadow.c	2005-05-28 02:52:53.000000000 +0000
@@ -81,7 +81,7 @@ shadow_promote(struct domain *d, unsigne
         FSH_LOG("%s: couldn't find/remove all write accesses, gpfn=%lx gmfn=%lx",
                 __func__, gpfn, gmfn);
 #if 1 || defined(LIVE_DANGEROUSLY)
-        set_bit(_PGC_page_table, &page->count_info);
+        set_bit32(_PGC_page_table, &page->count_info);
         return 1;
 #endif
         return 0;
@@ -103,14 +103,15 @@ shadow_promote(struct domain *d, unsigne
     //
     if ( unlikely(!get_page(page, d)) )
         BUG(); // XXX -- needs more thought for a graceful failure
-    if ( unlikely(test_and_clear_bit(_PGT_pinned, &page->u.inuse.type_info)) )
+    if ( unlikely(test_and_clear_bit32(_PGT_pinned,
+                                       &page->u.inuse.type_info)) )
     {
         pinned = 1;
         put_page_and_type(page);
     }
     if ( get_page_type(page, PGT_base_page_table) )
     {
-        set_bit(_PGC_page_table, &page->count_info);
+        set_bit32(_PGC_page_table, &page->count_info);
         put_page_type(page);
     }
     else
diff -rN -u -p old-xen-64-4/xen/common/dom_mem_ops.c new-xen-64-4/xen/common/dom_mem_ops.c
--- old-xen-64-4/xen/common/dom_mem_ops.c	2005-05-24 17:35:00.000000000 +0000
+++ new-xen-64-4/xen/common/dom_mem_ops.c	2005-05-28 02:53:10.000000000 +0000
@@ -106,10 +106,10 @@ free_dom_mem(struct domain *d,
                 return i;
             }
 
-            if ( test_and_clear_bit(_PGT_pinned, &page->u.inuse.type_info) )
+            if ( test_and_clear_bit32(_PGT_pinned, &page->u.inuse.type_info) )
                 put_page_and_type(page);
             
-            if ( test_and_clear_bit(_PGC_allocated, &page->count_info) )
+            if ( test_and_clear_bit32(_PGC_allocated, &page->count_info) )
                 put_page(page);
 
             shadow_sync_and_drop_references(d, page);
diff -rN -u -p old-xen-64-4/xen/include/asm-x86/bitops.h new-xen-64-4/xen/include/asm-x86/bitops.h
--- old-xen-64-4/xen/include/asm-x86/bitops.h	2005-05-25 18:41:08.000000000 +0000
+++ new-xen-64-4/xen/include/asm-x86/bitops.h	2005-05-28 04:06:14.000000000 +0000
@@ -46,6 +46,14 @@ static __inline__ void set_bit(long nr, 
 		:"dIr" (nr));
 }
 
+static __inline__ void set_bit32(int nr, volatile void * addr)
+{
+	__asm__ __volatile__( LOCK_PREFIX
+		"btsl %1,%0"
+		:"=m" (ADDR)
+		:"dIr" (nr));
+}
+
 /**
  * __set_bit - Set a bit in memory
  * @nr: the bit to set
@@ -63,6 +71,14 @@ static __inline__ void __set_bit(long nr
 		:"dIr" (nr));
 }
 
+static __inline__ void __set_bit32(int nr, volatile void * addr)
+{
+	__asm__(
+		"btsl %1,%0"
+		:"=m" (ADDR)
+		:"dIr" (nr));
+}
+
 /**
  * clear_bit - Clears a bit in memory
  * @nr: Bit to clear
@@ -136,6 +152,17 @@ static __inline__ int test_and_set_bit(l
 	return oldbit;
 }
 
+static __inline__ int test_and_set_bit32(int nr, volatile void * addr)
+{
+	int oldbit;
+
+	__asm__ __volatile__( LOCK_PREFIX
+		"btsl %2,%1\n\tsbbl %0,%0"
+		:"=r" (oldbit),"=m" (ADDR)
+		:"dIr" (nr) : "memory");
+	return oldbit;
+}
+
 /**
  * __test_and_set_bit - Set a bit and return its old value
  * @nr: Bit to set
@@ -175,6 +202,17 @@ static __inline__ int test_and_clear_bit
 	return oldbit;
 }
 
+static __inline__ int test_and_clear_bit32(int nr, volatile void * addr)
+{
+	int oldbit;
+
+	__asm__ __volatile__( LOCK_PREFIX
+		"btrl %2,%1\n\tsbbl %0,%0"
+		:"=r" (oldbit),"=m" (ADDR)
+		:"dIr" (nr) : "memory");
+	return oldbit;
+}
+
 /**
  * __test_and_clear_bit - Clear a bit and return its old value
  * @nr: Bit to set


[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: bitopts functions overflowing page boundarys

[Keir Fraser]

On 28 May 2005, at 05:43, Scott Parish wrote:

> u.inuse.type_info is at the end of the pfn_info structure, and is
> u32 for both x86_32 and x86_64--in this location it can also be the
> last 32 bits of a page.
>
> several functions use bitopts.h functions to manipulate this member, 
> and
> on x86_64 these functions use u64 instructions, which will overflow the
> page boundary, and possibly the end of memory as we see here:

You really see this in practise? I'm very surprised. The memory map 
would have to be just big enough that the last pfn_info structure falls 
at the end of an aligned 2MB boundary. If you reduce max_page by 1, 
does the problem disappear?

  -- Keir

Re: bitopts functions overflowing page boundarys

[Scott Parish]

On Sat, May 28, 2005 at 10:01:27AM +0100, Keir Fraser wrote:

> 
> On 28 May 2005, at 05:43, Scott Parish wrote:
> 
> >u.inuse.type_info is at the end of the pfn_info structure, and is
> >u32 for both x86_32 and x86_64--in this location it can also be the
> >last 32 bits of a page.
> >
> >several functions use bitopts.h functions to manipulate this member, 
> >and
> >on x86_64 these functions use u64 instructions, which will overflow the
> >page boundary, and possibly the end of memory as we see here:
> 
> You really see this in practise? I'm very surprised. The memory map 
> would have to be just big enough that the last pfn_info structure falls 
> at the end of an aligned 2MB boundary. If you reduce max_page by 1, 
> does the problem disappear?

Here's the memory map for one of the boxes we're seeing this on:

(XEN) Physical RAM map:
(XEN)  0000000000000000 - 000000000009dc00 (usable)
(XEN)  000000000009dc00 - 00000000000a0000 (reserved)
(XEN)  00000000000d0000 - 0000000000100000 (reserved)
(XEN)  0000000000100000 - 00000000dff60000 (usable)
(XEN)  00000000dff60000 - 00000000dff72000 (ACPI data)
(XEN)  00000000dff72000 - 00000000dff80000 (ACPI NVS)
(XEN)  00000000dff80000 - 00000000e0000000 (reserved)
(XEN)  00000000fec00000 - 00000000fec00400 (reserved)
(XEN)  00000000fee00000 - 00000000fee01000 (reserved)
(XEN)  00000000fff80000 - 0000000100000000 (reserved)
(XEN)  0000000100000000 - 0000000180000000 (usable)

No problem when dom0_mem is less then 2048K; at exactly 2048 we hit
the next sized "order" which can't be fulfilled from the 0x100-0xdff60
range. All initial allocation for dom0 that i've seen that fall in the
"usable" above the hole have the problem i described.

Setting,

  max_page = init_e820(e820_raw, &e820_raw_nr) - 1;

seems to unravel a number of things. shall i preceed to figure out
what all, or is such still needed?

sRp

-- 
Scott Parish
Signed-off-by: srparish@us.ibm.com

Re: bitopts functions overflowing page boundarys

[Scott Parish]

On Sat, May 28, 2005 at 10:51:06AM +0100, Keir Fraser wrote:

> 
> On 28 May 2005, at 10:04, Scott Parish wrote:
> 
> >o problem when dom0_mem is less then 2048K; at exactly 2048 we hit
> >the next sized "order" which can't be fulfilled from the 0x100-0xdff60
> >range. All initial allocation for dom0 that i've seen that fall in the
> >"usable" above the hole have the problem i described.
> >
> >Setting,
> >
> >  max_page = init_e820(e820_raw, &e820_raw_nr) - 1;
> >
> >seems to unravel a number of things. shall i preceed to figure out
> >what all, or is such still needed?
> 
> No, I guess this situation just isn't as unlikely as I though it would 
> be. :-)
> 
> I checked in a fixed up bitops.h that should fix the problem for you 
> without needing new bitop functions.

splendid!

sRp

-- 
Scott Parish
Signed-off-by: srparish@us.ibm.com

Re: bitopts functions overflowing page boundarys

[Keir Fraser]

On 28 May 2005, at 10:04, Scott Parish wrote:

> o problem when dom0_mem is less then 2048K; at exactly 2048 we hit
> the next sized "order" which can't be fulfilled from the 0x100-0xdff60
> range. All initial allocation for dom0 that i've seen that fall in the
> "usable" above the hole have the problem i described.
>
> Setting,
>
>   max_page = init_e820(e820_raw, &e820_raw_nr) - 1;
>
> seems to unravel a number of things. shall i preceed to figure out
> what all, or is such still needed?

No, I guess this situation just isn't as unlikely as I though it would 
be. :-)

I checked in a fixed up bitops.h that should fix the problem for you 
without needing new bitop functions.

  -- Keir