[PATCH] hypercall/mem: Introduce XENMEM_machphys_compat_mfn_list

[PATCH] hypercall/mem: Introduce XENMEM_machphys_compat_mfn_list

[Andrew Cooper]

To correctly migrate a PV guest, the toolstack must remove Xen mappings from
the guest pagetables.  For 32bit PV guests, the pagetables cannot be walked
from the top so upon encountering an L2 table, the toolstack must decide
whether it contains Xen mappings or not, to avoid corrupting L2s without Xen
mappings.

The migration code performs this search efficiently by knowing that the Xen
mappings will start at a known L2e and point to a known mfn, which will be the
fist mfn in the m2p table.

Unfortunately there are two m2p tables in use; a regular and a compatibility
one.  The toolstack looks for the first mfn of its own m2p table in the guest
pagetables.  This only works if the toolstack is the same bitness as the 32bit
domain being migrated, and leaves a problem for 64bit toolstacks which will
never be able to find its regular m2p in a compat guest.

It appears that this bug for 64bit toolstacks was discovered, but hacked
around in an unsafe manor.  The code currently shoots any invalid L2es and
doesn't report a failure for L2 tables in a 32 bit guest, even after the guest
is paused.  This means that non Xen entries which should fail the migration
don't, and the guest will resume on the far side with unexpectedly fewer
present pagetable entries.

This patch introduces XENMEM_machphys_compat_mfn_list which permits a 64bit
toolstack to access the compat m2p mfn list, for the purpose of correctly
identifying Xen entries in a 32bit guest.

It is worth noting for completeness that 64bit PV guests don't have any of
these games to play.  The Xen mappings are present at a known location in all
L4 tables, so can be safely shot by 32 and 64bit toolstacks without looking at
where the mapping points to.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 xen/arch/x86/x86_64/compat/mm.c |    1 +
 xen/arch/x86/x86_64/mm.c        |   32 ++++++++++++++++++++++++++++++++
 xen/include/public/memory.h     |   10 ++++++++++
 3 files changed, 43 insertions(+)

diff --git a/xen/arch/x86/x86_64/compat/mm.c b/xen/arch/x86/x86_64/compat/mm.c
index 0a8408b..6d3bc31 100644
--- a/xen/arch/x86/x86_64/compat/mm.c
+++ b/xen/arch/x86/x86_64/compat/mm.c
@@ -146,6 +146,7 @@ int compat_arch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg)
     }
 
     case XENMEM_machphys_mfn_list:
+    case XENMEM_machphys_compat_mfn_list:
     {
         unsigned long limit;
         compat_pfn_t last_mfn;
diff --git a/xen/arch/x86/x86_64/mm.c b/xen/arch/x86/x86_64/mm.c
index 71ae519..3d272ac 100644
--- a/xen/arch/x86/x86_64/mm.c
+++ b/xen/arch/x86/x86_64/mm.c
@@ -1000,6 +1000,38 @@ long subarch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg)
 
         break;
 
+    case XENMEM_machphys_compat_mfn_list:
+    {
+        unsigned long limit, last_mfn;
+
+        if ( copy_from_guest(&xmml, arg, 1) )
+            return -EFAULT;
+
+        limit = (unsigned long)(compat_machine_to_phys_mapping + max_page);
+        if ( limit > RDWR_COMPAT_MPT_VIRT_END )
+            limit = RDWR_COMPAT_MPT_VIRT_END;
+        for ( i = 0, v = RDWR_COMPAT_MPT_VIRT_START, last_mfn = 0;
+              (i != xmml.max_extents) && (v < limit);
+              i++, v += 1 << L2_PAGETABLE_SHIFT )
+        {
+            l2e = compat_idle_pg_table_l2[l2_table_offset(v)];
+            if ( l2e_get_flags(l2e) & _PAGE_PRESENT )
+                mfn = l2e_get_pfn(l2e);
+            else
+                mfn = last_mfn;
+            ASSERT(mfn);
+            if ( copy_to_guest_offset(xmml.extent_start, i, &mfn, 1) )
+                return -EFAULT;
+            last_mfn = mfn;
+        }
+
+        xmml.nr_extents = i;
+        if ( __copy_to_guest(arg, &xmml, 1) )
+            rc = -EFAULT;
+
+        break;
+    }
+
     case XENMEM_get_sharing_freed_pages:
         return mem_sharing_get_nr_saved_mfns();
 
diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
index f19ac14..be49beb 100644
--- a/xen/include/public/memory.h
+++ b/xen/include/public/memory.h
@@ -465,6 +465,16 @@ DEFINE_XEN_GUEST_HANDLE(xen_mem_sharing_op_t);
  * The zero value is appropiate.
  */
 
+/*
+ * For a compat toolstack domain, this is indentical to
+ * XENMEM_machphys_mfn_list.
+ *
+ * For a non compat toolstack domain, this functions similarly to
+ * XENMEM_machphys_mfn_list, but returns the mfns making up the compatibility
+ * m2p table.
+ */
+#define XENMEM_machphys_compat_mfn_list     25
+
 #endif /* defined(__XEN__) || defined(__XEN_TOOLS__) */
 
 #endif /* __XEN_PUBLIC_MEMORY_H__ */
-- 
1.7.10.4

Re: [PATCH] hypercall/mem: Introduce XENMEM_machphys_compat_mfn_list

[Jan Beulich]

>>> On 17.04.14 at 13:10, <andrew.cooper3@citrix.com> wrote:
> To correctly migrate a PV guest, the toolstack must remove Xen mappings from
> the guest pagetables.  For 32bit PV guests, the pagetables cannot be walked
> from the top so upon encountering an L2 table, the toolstack must decide
> whether it contains Xen mappings or not, to avoid corrupting L2s without Xen
> mappings.
> 
> The migration code performs this search efficiently by knowing that the Xen
> mappings will start at a known L2e and point to a known mfn, which will be 
> the
> fist mfn in the m2p table.
> 
> Unfortunately there are two m2p tables in use; a regular and a compatibility
> one.  The toolstack looks for the first mfn of its own m2p table in the 
> guest
> pagetables.  This only works if the toolstack is the same bitness as the 
> 32bit
> domain being migrated, and leaves a problem for 64bit toolstacks which will
> never be able to find its regular m2p in a compat guest.
> 
> It appears that this bug for 64bit toolstacks was discovered, but hacked
> around in an unsafe manor.  The code currently shoots any invalid L2es and
> doesn't report a failure for L2 tables in a 32 bit guest, even after the 
> guest
> is paused.  This means that non Xen entries which should fail the migration
> don't, and the guest will resume on the far side with unexpectedly fewer
> present pagetable entries.

So since that hack isn't being removed here, do you have plans to do
so? Knowing where that is would help judging on the need for this
patch (and I don't want to waste time hunting for it knowing that you
already spotted it)...

> +    case XENMEM_machphys_compat_mfn_list:
> +    {
> +        unsigned long limit, last_mfn;

You re-use other function scope variables - why not also last_mfn?
Which then raises the question whether limit shouldn't be simply
added there too.

Jan

Re: [PATCH] hypercall/mem: Introduce XENMEM_machphys_compat_mfn_list

[Andrew Cooper]

On 17/04/14 12:42, Jan Beulich wrote:
>>>> On 17.04.14 at 13:10, <andrew.cooper3@citrix.com> wrote:
>> To correctly migrate a PV guest, the toolstack must remove Xen mappings from
>> the guest pagetables.  For 32bit PV guests, the pagetables cannot be walked
>> from the top so upon encountering an L2 table, the toolstack must decide
>> whether it contains Xen mappings or not, to avoid corrupting L2s without Xen
>> mappings.
>>
>> The migration code performs this search efficiently by knowing that the Xen
>> mappings will start at a known L2e and point to a known mfn, which will be 
>> the
>> fist mfn in the m2p table.
>>
>> Unfortunately there are two m2p tables in use; a regular and a compatibility
>> one.  The toolstack looks for the first mfn of its own m2p table in the 
>> guest
>> pagetables.  This only works if the toolstack is the same bitness as the 
>> 32bit
>> domain being migrated, and leaves a problem for 64bit toolstacks which will
>> never be able to find its regular m2p in a compat guest.
>>
>> It appears that this bug for 64bit toolstacks was discovered, but hacked
>> around in an unsafe manor.  The code currently shoots any invalid L2es and
>> doesn't report a failure for L2 tables in a 32 bit guest, even after the 
>> guest
>> is paused.  This means that non Xen entries which should fail the migration
>> don't, and the guest will resume on the far side with unexpectedly fewer
>> present pagetable entries.
> So since that hack isn't being removed here, do you have plans to do
> so?

I am rewriting migration basically from scratch, which will involve
removing/replacing almost all of xc_domain_{save,restore}.c

Specifically fixing this bug here, just to delete the fix in the near
future seems pointless.


I suppose the one piece of information I missed was that I am happy to
keep this as part of my migration series, but would like it to be
reviewed slightly independently.

> Knowing where that is would help judging on the need for this
> patch (and I don't want to waste time hunting for it knowing that you
> already spotted it)...

The bug is in canonicalize_pagetable() in xc_domain_save.c

The search for affected L2 tables at line 445 will never succeed for a
64 bit guest, and the decision at line 500 will fail to identify any
errors which should be fatal.

>
>> +    case XENMEM_machphys_compat_mfn_list:
>> +    {
>> +        unsigned long limit, last_mfn;
> You re-use other function scope variables - why not also last_mfn?
> Which then raises the question whether limit shouldn't be simply
> added there too.
>
> Jan
>

I didn't spot last_mfn in the other scope variables.  I shall use the
outer scope ones

~Andrew

Re: [PATCH] hypercall/mem: Introduce XENMEM_machphys_compat_mfn_list

[Andrew Cooper]

On 17/04/14 12:58, Andrew Cooper wrote:
> On 17/04/14 12:42, Jan Beulich wrote:
>
>> Knowing where that is would help judging on the need for this
>> patch (and I don't want to waste time hunting for it knowing that you
>> already spotted it)...
> The bug is in canonicalize_pagetable() in xc_domain_save.c
>
> The search for affected L2 tables at line 445 will never succeed for a
> 64 bit guest, and the decision at line 500 will fail to identify any
> errors which should be fatal.

I do in fact mean 32 bit guest and 64 bit toolstack.

~Andrew

[PATCH] hypercall/mem: Introduce XENMEM_machphys_compat_mfn_list

[Andrew Cooper]

To correctly migrate a PV guest, the toolstack must remove Xen mappings from
the guest pagetables.  For 32bit PV guests, the pagetables cannot be walked
from the top so upon encountering an L2 table, the toolstack must decide
whether it contains Xen mappings or not, to avoid corrupting L2s without Xen
mappings.

The migration code performs this search efficiently by knowing that the Xen
mappings will start at a known L2e and point to a known mfn, which will be the
fist mfn in the m2p table.

Unfortunately there are two m2p tables in use; a regular and a compatibility
one.  The toolstack looks for the first mfn of its own m2p table in the guest
pagetables.  This only works if the toolstack is the same bitness as the 32bit
domain being migrated, and leaves a problem for 64bit toolstacks which will
never be able to find its regular m2p in a compat guest.

It appears that this bug for 64bit toolstacks was discovered, but hacked
around in an unsafe manor.  The code currently shoots any invalid L2es and
doesn't report a failure for L2 tables in a 32 bit guest, even after the guest
is paused.  This means that non Xen entries which should fail the migration
don't, and the guest will resume on the far side with unexpectedly fewer
present pagetable entries.

This patch introduces XENMEM_machphys_compat_mfn_list which permits a 64bit
toolstack to access the compat m2p mfn list, for the purpose of correctly
identifying Xen entries in a 32bit guest.

It is worth noting for completeness that 64bit PV guests don't have any of
these games to play.  The Xen mappings are present at a known location in all
L4 tables, so can be safely shot by 32 and 64bit toolstacks without looking at
where the mapping points to.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>

---

I am happy for this to live as part of my "migration v2" series, but is
presented here for individual review.

Changes in v2:
 * Don't alias other local scope variables in subarch_memory_op
---
 xen/arch/x86/x86_64/compat/mm.c |    1 +
 xen/arch/x86/x86_64/mm.c        |   30 +++++++++++++++++++++++++++++-
 xen/include/public/memory.h     |   10 ++++++++++
 3 files changed, 40 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/x86_64/compat/mm.c b/xen/arch/x86/x86_64/compat/mm.c
index 0a8408b..6d3bc31 100644
--- a/xen/arch/x86/x86_64/compat/mm.c
+++ b/xen/arch/x86/x86_64/compat/mm.c
@@ -146,6 +146,7 @@ int compat_arch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg)
     }
 
     case XENMEM_machphys_mfn_list:
+    case XENMEM_machphys_compat_mfn_list:
     {
         unsigned long limit;
         compat_pfn_t last_mfn;
diff --git a/xen/arch/x86/x86_64/mm.c b/xen/arch/x86/x86_64/mm.c
index 71ae519..ff96997 100644
--- a/xen/arch/x86/x86_64/mm.c
+++ b/xen/arch/x86/x86_64/mm.c
@@ -953,7 +953,7 @@ long subarch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg)
     struct xen_machphys_mfn_list xmml;
     l3_pgentry_t l3e;
     l2_pgentry_t l2e;
-    unsigned long v;
+    unsigned long v, limit;
     xen_pfn_t mfn, last_mfn;
     unsigned int i;
     long rc = 0;
@@ -1000,6 +1000,34 @@ long subarch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg)
 
         break;
 
+    case XENMEM_machphys_compat_mfn_list:
+        if ( copy_from_guest(&xmml, arg, 1) )
+            return -EFAULT;
+
+        limit = (unsigned long)(compat_machine_to_phys_mapping + max_page);
+        if ( limit > RDWR_COMPAT_MPT_VIRT_END )
+            limit = RDWR_COMPAT_MPT_VIRT_END;
+        for ( i = 0, v = RDWR_COMPAT_MPT_VIRT_START, last_mfn = 0;
+              (i != xmml.max_extents) && (v < limit);
+              i++, v += 1 << L2_PAGETABLE_SHIFT )
+        {
+            l2e = compat_idle_pg_table_l2[l2_table_offset(v)];
+            if ( l2e_get_flags(l2e) & _PAGE_PRESENT )
+                mfn = l2e_get_pfn(l2e);
+            else
+                mfn = last_mfn;
+            ASSERT(mfn);
+            if ( copy_to_guest_offset(xmml.extent_start, i, &mfn, 1) )
+                return -EFAULT;
+            last_mfn = mfn;
+        }
+
+        xmml.nr_extents = i;
+        if ( __copy_to_guest(arg, &xmml, 1) )
+            rc = -EFAULT;
+
+        break;
+
     case XENMEM_get_sharing_freed_pages:
         return mem_sharing_get_nr_saved_mfns();
 
diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
index f19ac14..be49beb 100644
--- a/xen/include/public/memory.h
+++ b/xen/include/public/memory.h
@@ -465,6 +465,16 @@ DEFINE_XEN_GUEST_HANDLE(xen_mem_sharing_op_t);
  * The zero value is appropiate.
  */
 
+/*
+ * For a compat toolstack domain, this is indentical to
+ * XENMEM_machphys_mfn_list.
+ *
+ * For a non compat toolstack domain, this functions similarly to
+ * XENMEM_machphys_mfn_list, but returns the mfns making up the compatibility
+ * m2p table.
+ */
+#define XENMEM_machphys_compat_mfn_list     25
+
 #endif /* defined(__XEN__) || defined(__XEN_TOOLS__) */
 
 #endif /* __XEN_PUBLIC_MEMORY_H__ */
-- 
1.7.10.4

Re: [PATCH] hypercall/mem: Introduce XENMEM_machphys_compat_mfn_list

[Konrad Rzeszutek Wilk]

On Thu, Apr 17, 2014 at 04:53:25PM +0100, Andrew Cooper wrote:
> To correctly migrate a PV guest, the toolstack must remove Xen mappings from
> the guest pagetables.  For 32bit PV guests, the pagetables cannot be walked
> from the top so upon encountering an L2 table, the toolstack must decide
> whether it contains Xen mappings or not, to avoid corrupting L2s without Xen
> mappings.
> 
> The migration code performs this search efficiently by knowing that the Xen
> mappings will start at a known L2e and point to a known mfn, which will be the
> fist mfn in the m2p table.

first
> 
> Unfortunately there are two m2p tables in use; a regular and a compatibility
                                               ^- ":"
> one.  The toolstack looks for the first mfn of its own m2p table in the guest
> pagetables.  This only works if the toolstack is the same bitness as the 32bit
> domain being migrated, and leaves a problem for 64bit toolstacks which will
> never be able to find its regular m2p in a compat guest.
> 
> It appears that this bug for 64bit toolstacks was discovered, but hacked
> around in an unsafe manor.  The code currently shoots any invalid L2es and
                      ^^^^ - manner
> doesn't report a failure for L2 tables in a 32 bit guest, even after the guest
> is paused.  This means that non Xen entries which should fail the migration
> don't, and the guest will resume on the far side with unexpectedly fewer
> present pagetable entries.

Ouch1
> 
> This patch introduces XENMEM_machphys_compat_mfn_list which permits a 64bit
> toolstack to access the compat m2p mfn list, for the purpose of correctly
> identifying Xen entries in a 32bit guest.
> 
> It is worth noting for completeness that 64bit PV guests don't have any of
> these games to play.  The Xen mappings are present at a known location in all
> L4 tables, so can be safely shot by 32 and 64bit toolstacks without looking at
> where the mapping points to.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Keir Fraser <keir@xen.org>
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Tim Deegan <tim@xen.org>
> CC: Ian Campbell <Ian.Campbell@citrix.com>
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> 
> ---
> 
> I am happy for this to live as part of my "migration v2" series, but is
> presented here for individual review.
> 
> Changes in v2:
>  * Don't alias other local scope variables in subarch_memory_op
> ---
>  xen/arch/x86/x86_64/compat/mm.c |    1 +
>  xen/arch/x86/x86_64/mm.c        |   30 +++++++++++++++++++++++++++++-
>  xen/include/public/memory.h     |   10 ++++++++++
>  3 files changed, 40 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/x86/x86_64/compat/mm.c b/xen/arch/x86/x86_64/compat/mm.c
> index 0a8408b..6d3bc31 100644
> --- a/xen/arch/x86/x86_64/compat/mm.c
> +++ b/xen/arch/x86/x86_64/compat/mm.c
> @@ -146,6 +146,7 @@ int compat_arch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg)
>      }
>  
>      case XENMEM_machphys_mfn_list:
> +    case XENMEM_machphys_compat_mfn_list:
>      {
>          unsigned long limit;
>          compat_pfn_t last_mfn;
> diff --git a/xen/arch/x86/x86_64/mm.c b/xen/arch/x86/x86_64/mm.c
> index 71ae519..ff96997 100644
> --- a/xen/arch/x86/x86_64/mm.c
> +++ b/xen/arch/x86/x86_64/mm.c
> @@ -953,7 +953,7 @@ long subarch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg)
>      struct xen_machphys_mfn_list xmml;
>      l3_pgentry_t l3e;
>      l2_pgentry_t l2e;
> -    unsigned long v;
> +    unsigned long v, limit;
>      xen_pfn_t mfn, last_mfn;
>      unsigned int i;
>      long rc = 0;
> @@ -1000,6 +1000,34 @@ long subarch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg)
>  
>          break;
>  
> +    case XENMEM_machphys_compat_mfn_list:
> +        if ( copy_from_guest(&xmml, arg, 1) )
> +            return -EFAULT;
> +
> +        limit = (unsigned long)(compat_machine_to_phys_mapping + max_page);
> +        if ( limit > RDWR_COMPAT_MPT_VIRT_END )
> +            limit = RDWR_COMPAT_MPT_VIRT_END;
> +        for ( i = 0, v = RDWR_COMPAT_MPT_VIRT_START, last_mfn = 0;
> +              (i != xmml.max_extents) && (v < limit);
> +              i++, v += 1 << L2_PAGETABLE_SHIFT )
> +        {
> +            l2e = compat_idle_pg_table_l2[l2_table_offset(v)];
> +            if ( l2e_get_flags(l2e) & _PAGE_PRESENT )
> +                mfn = l2e_get_pfn(l2e);
> +            else
> +                mfn = last_mfn;
> +            ASSERT(mfn);
> +            if ( copy_to_guest_offset(xmml.extent_start, i, &mfn, 1) )
> +                return -EFAULT;
> +            last_mfn = mfn;
> +        }
> +
> +        xmml.nr_extents = i;
> +        if ( __copy_to_guest(arg, &xmml, 1) )
> +            rc = -EFAULT;
> +
> +        break;
> +
>      case XENMEM_get_sharing_freed_pages:
>          return mem_sharing_get_nr_saved_mfns();
>  
> diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
> index f19ac14..be49beb 100644
> --- a/xen/include/public/memory.h
> +++ b/xen/include/public/memory.h
> @@ -465,6 +465,16 @@ DEFINE_XEN_GUEST_HANDLE(xen_mem_sharing_op_t);
>   * The zero value is appropiate.
>   */
>  
> +/*
> + * For a compat toolstack domain, this is indentical to
                                             ^^^^^^^^^ - identical

> + * XENMEM_machphys_mfn_list.
> + *
> + * For a non compat toolstack domain, this functions similarly to
> + * XENMEM_machphys_mfn_list, but returns the mfns making up the compatibility
> + * m2p table.
> + */
> +#define XENMEM_machphys_compat_mfn_list     25
> +
>  #endif /* defined(__XEN__) || defined(__XEN_TOOLS__) */
>  
>  #endif /* __XEN_PUBLIC_MEMORY_H__ */
> -- 
> 1.7.10.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel