Patchwork [2/3] x86: fix do_update_va_mapping_otherdomain() wrt translated domains

login
register
mail settings
Submitter Jan Beulich
Date Oct. 12, 2017, 10 a.m.
Message ID <59DF596E020000780018555D@prv-mh.provo.novell.com>
Download mbox | patch
Permalink /patch/359919/
State New
Headers show

Comments

Jan Beulich - Oct. 12, 2017, 10 a.m.
While I can't seem to find any users of this hypercall (being a likely
explanation of why the problem wasn't noticed so far), just like for
do_mmu_update() paged-out and shared page handling is needed here. Move
all this logic into mod_l1_entry(), which then also results in no
longer
- doing any of this handling for non-present PTEs,
- acquiring two temporary page references when one is already more than
  enough.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Now that L1 entry handling in do_mmu_update() is sufficiently similar
again to that of L2/L3/L4 entries, I wonder whether it wouldn't it be
better for the function to refuse pg_owner != pt_owner for L2/L3/L4
updates. Right now the passed in foreign domain ID is simply ignored
in that case (except for the XSM check).
Andrew Cooper - Oct. 12, 2017, 11:18 a.m.
On 12/10/17 11:00, Jan Beulich wrote:
> While I can't seem to find any users of this hypercall (being a likely
> explanation of why the problem wasn't noticed so far), just like for

Judging by c/s a51ed685b which shifted
__HYPERVISOR_update_va_mapping_otherdomain's hypercall number to make
space for __HYPERVISOR_grant_table_op, I'd have said the chance of it
being used was slim.  However,

andrewcoop@andrewcoop:/local/xen.git/xen$ git checkout a51ed685
andrewcoop@andrewcoop:/local/xen.git/xen$ git grep update_va_mapping_otherdomain -- :/
../linux-2.6.7-xen-sparse/drivers/xen/blkback/blkback.c:320:    if ( HYPERVISOR_update_va_mapping_otherdomain(
../linux-2.6.7-xen-sparse/drivers/xen/blkback/blkback.c:404:        mcl[i].op = __HYPERVISOR_update_va_mapping_otherdomain;
../linux-2.6.7-xen-sparse/drivers/xen/netback/netback.c:516:        mcl[0].op = __HYPERVISOR_update_va_mapping_otherdomain;
../linux-2.6.7-xen-sparse/include/asm-xen/hypervisor.h:458:static inline int HYPERVISOR_update_va_mapping_otherdomain(
../linux-2.6.7-xen-sparse/include/asm-xen/hypervisor.h:464:        : "=a" (ret) : "0" (__HYPERVISOR_update_va_mapping_otherdomain), 
arch/x86/memory.c:1264:int do_update_va_mapping_otherdomain(unsigned long page_nr, 
arch/x86/x86_32/entry.S:723:        .long SYMBOL_NAME(do_update_va_mapping_otherdomain)
include/hypervisor-ifs/hypervisor-if.h:50:#define __HYPERVISOR_update_va_mapping_otherdomain 22


It certainly was used at that point in history.  If none of that code
has survived into more recent version {blk,net}back, it is probably that
the hypercall isn't used any more.

> do_mmu_update() paged-out and shared page handling is needed here. Move
> all this logic into mod_l1_entry(), which then also results in no
> longer
> - doing any of this handling for non-present PTEs,
> - acquiring two temporary page references when one is already more than
>   enough.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> Now that L1 entry handling in do_mmu_update() is sufficiently similar
> again to that of L2/L3/L4 entries, I wonder whether it wouldn't it be
> better for the function to refuse pg_owner != pt_owner for L2/L3/L4
> updates. Right now the passed in foreign domain ID is simply ignored
> in that case (except for the XSM check).

I can't see anything good coming from having pg_owner != pt_owner in non
L1 pagetables.  Explicit rejection is certainly better than doing the
wrong thing silently under the hood.

Do you want to do a separate patch for that, or fold it into this one?

> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -1632,7 +1632,6 @@ static int mod_l1_entry(l1_pgentry_t *pl
>  
>      if ( l1e_get_flags(nl1e) & _PAGE_PRESENT )
>      {
> -        /* Translate foreign guest addresses. */
>          struct page_info *page = NULL;
>  
>          if ( unlikely(l1e_get_flags(nl1e) & l1_disallow_mask(pt_dom)) )
> @@ -1642,9 +1641,35 @@ static int mod_l1_entry(l1_pgentry_t *pl
>              return -EINVAL;
>          }
>  
> +        /* Translate foreign guest address. */
>          if ( paging_mode_translate(pg_dom) )
>          {
> -            page = get_page_from_gfn(pg_dom, l1e_get_pfn(nl1e), NULL, P2M_ALLOC);
> +            p2m_type_t p2mt;
> +            p2m_query_t q = l1e_get_flags(nl1e) & _PAGE_RW ?
> +                            P2M_ALLOC | P2M_UNSHARE : P2M_ALLOC;
> +
> +            page = get_page_from_gfn(pg_dom, l1e_get_pfn(nl1e), &p2mt, q);
> +
> +            if ( p2m_is_paged(p2mt) )
> +            {
> +                if ( page )
> +                    put_page(page);
> +                p2m_mem_paging_populate(pg_dom, l1e_get_pfn(nl1e));
> +                return -ENOENT;
> +            }
> +
> +            if ( p2mt == p2m_ram_paging_in && !page )
> +                return -ENOENT;
> +
> +            /* Did our attempt to unshare fail? */
> +            if ( (q & P2M_UNSHARE) && p2m_is_shared(p2mt) )
> +            {
> +                /* We could not have obtained a page ref. */
> +                ASSERT(!page);
> +                /* And mem_sharing_notify has already been called. */
> +                return -ENOMEM;
> +            }
> +
>              if ( !page )
>                  return -EINVAL;
>              nl1e = l1e_from_page(page, l1e_get_flags(nl1e));
> @@ -3315,47 +3340,10 @@ long do_mmu_update(
>                  switch ( page->u.inuse.type_info & PGT_type_mask )
>                  {
>                  case PGT_l1_page_table:
> -                {
> -                    l1_pgentry_t l1e = l1e_from_intpte(req.val);
> -                    p2m_type_t l1e_p2mt = p2m_ram_rw;
> -                    struct page_info *target = NULL;
> -                    p2m_query_t q = (l1e_get_flags(l1e) & _PAGE_RW) ?
> -                                        P2M_UNSHARE : P2M_ALLOC;
> -
> -                    if ( paging_mode_translate(pg_owner) )
> -                        target = get_page_from_gfn(pg_owner, l1e_get_pfn(l1e),
> -                                                   &l1e_p2mt, q);
> -
> -                    if ( p2m_is_paged(l1e_p2mt) )
> -                    {
> -                        if ( target )
> -                            put_page(target);
> -                        p2m_mem_paging_populate(pg_owner, l1e_get_pfn(l1e));
> -                        rc = -ENOENT;
> -                        break;
> -                    }
> -                    else if ( p2m_ram_paging_in == l1e_p2mt && !target )
> -                    {
> -                        rc = -ENOENT;
> -                        break;
> -                    }
> -                    /* If we tried to unshare and failed */
> -                    else if ( (q & P2M_UNSHARE) && p2m_is_shared(l1e_p2mt) )
> -                    {
> -                        /* We could not have obtained a page ref. */
> -                        ASSERT(target == NULL);
> -                        /* And mem_sharing_notify has already been called. */
> -                        rc = -ENOMEM;
> -                        break;
> -                    }
> -
> -                    rc = mod_l1_entry(va, l1e, mfn,
> +                    rc = mod_l1_entry(va, l1e_from_intpte(req.val), mfn,
>                                        cmd == MMU_PT_UPDATE_PRESERVE_AD, v,
>                                        pg_owner);
> -                    if ( target )
> -                        put_page(target);
> -                }
> -                break;
> +                    break;
>                  case PGT_l2_page_table:
>                      rc = mod_l2_entry(va, l2e_from_intpte(req.val), mfn,
>                                        cmd == MMU_PT_UPDATE_PRESERVE_AD, v);
> @@ -3367,7 +3355,7 @@ long do_mmu_update(
>                  case PGT_l4_page_table:
>                      rc = mod_l4_entry(va, l4e_from_intpte(req.val), mfn,
>                                        cmd == MMU_PT_UPDATE_PRESERVE_AD, v);
> -                break;
> +                    break;

If we are tidying up the style, could we also get some newlines between
break and case?

Either way, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

>                  case PGT_writable_page:
>                      perfc_incr(writable_mmu_updates);
>                      if ( paging_write_guest_entry(v, va, req.val, _mfn(mfn)) )
>
>
Jan Beulich - Oct. 12, 2017, 11:27 a.m.
>>> On 12.10.17 at 13:18, <andrew.cooper3@citrix.com> wrote:
> On 12/10/17 11:00, Jan Beulich wrote:
>> While I can't seem to find any users of this hypercall (being a likely
>> explanation of why the problem wasn't noticed so far), just like for
> 
> Judging by c/s a51ed685b which shifted
> __HYPERVISOR_update_va_mapping_otherdomain's hypercall number to make
> space for __HYPERVISOR_grant_table_op, I'd have said the chance of it
> being used was slim.  However,
> 
> andrewcoop@andrewcoop:/local/xen.git/xen$ git checkout a51ed685
> andrewcoop@andrewcoop:/local/xen.git/xen$ git grep 
> update_va_mapping_otherdomain -- :/
> ../linux-2.6.7-xen-sparse/drivers/xen/blkback/blkback.c:320:    if ( 
> HYPERVISOR_update_va_mapping_otherdomain(
> ../linux-2.6.7-xen-sparse/drivers/xen/blkback/blkback.c:404:        
> mcl[i].op = __HYPERVISOR_update_va_mapping_otherdomain;
> ../linux-2.6.7-xen-sparse/drivers/xen/netback/netback.c:516:        
> mcl[0].op = __HYPERVISOR_update_va_mapping_otherdomain;
> ../linux-2.6.7-xen-sparse/include/asm-xen/hypervisor.h:458:static inline int 
> HYPERVISOR_update_va_mapping_otherdomain(
> ../linux-2.6.7-xen-sparse/include/asm-xen/hypervisor.h:464:        : "=a" 
> (ret) : "0" (__HYPERVISOR_update_va_mapping_otherdomain), 
> arch/x86/memory.c:1264:int do_update_va_mapping_otherdomain(unsigned long 
> page_nr, 
> arch/x86/x86_32/entry.S:723:        .long 
> SYMBOL_NAME(do_update_va_mapping_otherdomain)
> include/hypervisor-ifs/hypervisor-if.h:50:#define 
> __HYPERVISOR_update_va_mapping_otherdomain 22
> 
> 
> It certainly was used at that point in history.  If none of that code
> has survived into more recent version {blk,net}back, it is probably that
> the hypercall isn't used any more.

I did my check on Linux 4.4.88 (plus tool stack and qemu),
without finding anything.

>> do_mmu_update() paged-out and shared page handling is needed here. Move
>> all this logic into mod_l1_entry(), which then also results in no
>> longer
>> - doing any of this handling for non-present PTEs,
>> - acquiring two temporary page references when one is already more than
>>   enough.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> Now that L1 entry handling in do_mmu_update() is sufficiently similar
>> again to that of L2/L3/L4 entries, I wonder whether it wouldn't it be
>> better for the function to refuse pg_owner != pt_owner for L2/L3/L4
>> updates. Right now the passed in foreign domain ID is simply ignored
>> in that case (except for the XSM check).
> 
> I can't see anything good coming from having pg_owner != pt_owner in non
> L1 pagetables.  Explicit rejection is certainly better than doing the
> wrong thing silently under the hood.
> 
> Do you want to do a separate patch for that, or fold it into this one?

I'll do it separately - this again wouldn't really qualify for 4.10.

>> @@ -3315,47 +3340,10 @@ long do_mmu_update(
>>                  switch ( page->u.inuse.type_info & PGT_type_mask )
>>                  {
>>                  case PGT_l1_page_table:
>> -                {
>> -                    l1_pgentry_t l1e = l1e_from_intpte(req.val);
>> -                    p2m_type_t l1e_p2mt = p2m_ram_rw;
>> -                    struct page_info *target = NULL;
>> -                    p2m_query_t q = (l1e_get_flags(l1e) & _PAGE_RW) ?
>> -                                        P2M_UNSHARE : P2M_ALLOC;
>> -
>> -                    if ( paging_mode_translate(pg_owner) )
>> -                        target = get_page_from_gfn(pg_owner, l1e_get_pfn(l1e),
>> -                                                   &l1e_p2mt, q);
>> -
>> -                    if ( p2m_is_paged(l1e_p2mt) )
>> -                    {
>> -                        if ( target )
>> -                            put_page(target);
>> -                        p2m_mem_paging_populate(pg_owner, l1e_get_pfn(l1e));
>> -                        rc = -ENOENT;
>> -                        break;
>> -                    }
>> -                    else if ( p2m_ram_paging_in == l1e_p2mt && !target )
>> -                    {
>> -                        rc = -ENOENT;
>> -                        break;
>> -                    }
>> -                    /* If we tried to unshare and failed */
>> -                    else if ( (q & P2M_UNSHARE) && p2m_is_shared(l1e_p2mt) )
>> -                    {
>> -                        /* We could not have obtained a page ref. */
>> -                        ASSERT(target == NULL);
>> -                        /* And mem_sharing_notify has already been called. */
>> -                        rc = -ENOMEM;
>> -                        break;
>> -                    }
>> -
>> -                    rc = mod_l1_entry(va, l1e, mfn,
>> +                    rc = mod_l1_entry(va, l1e_from_intpte(req.val), mfn,
>>                                        cmd == MMU_PT_UPDATE_PRESERVE_AD, v,
>>                                        pg_owner);
>> -                    if ( target )
>> -                        put_page(target);
>> -                }
>> -                break;
>> +                    break;
>>                  case PGT_l2_page_table:
>>                      rc = mod_l2_entry(va, l2e_from_intpte(req.val), mfn,
>>                                        cmd == MMU_PT_UPDATE_PRESERVE_AD, v);
>> @@ -3367,7 +3355,7 @@ long do_mmu_update(
>>                  case PGT_l4_page_table:
>>                      rc = mod_l4_entry(va, l4e_from_intpte(req.val), mfn,
>>                                        cmd == MMU_PT_UPDATE_PRESERVE_AD, v);
>> -                break;
>> +                    break;
> 
> If we are tidying up the style, could we also get some newlines between
> break and case?

I had considered that, but then discarded the idea for the switch
as whole not being all that large, yet the diff becoming quite a bit
larger if I did.

> Either way, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Thanks, Jan

Patch

--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -1632,7 +1632,6 @@  static int mod_l1_entry(l1_pgentry_t *pl
 
     if ( l1e_get_flags(nl1e) & _PAGE_PRESENT )
     {
-        /* Translate foreign guest addresses. */
         struct page_info *page = NULL;
 
         if ( unlikely(l1e_get_flags(nl1e) & l1_disallow_mask(pt_dom)) )
@@ -1642,9 +1641,35 @@  static int mod_l1_entry(l1_pgentry_t *pl
             return -EINVAL;
         }
 
+        /* Translate foreign guest address. */
         if ( paging_mode_translate(pg_dom) )
         {
-            page = get_page_from_gfn(pg_dom, l1e_get_pfn(nl1e), NULL, P2M_ALLOC);
+            p2m_type_t p2mt;
+            p2m_query_t q = l1e_get_flags(nl1e) & _PAGE_RW ?
+                            P2M_ALLOC | P2M_UNSHARE : P2M_ALLOC;
+
+            page = get_page_from_gfn(pg_dom, l1e_get_pfn(nl1e), &p2mt, q);
+
+            if ( p2m_is_paged(p2mt) )
+            {
+                if ( page )
+                    put_page(page);
+                p2m_mem_paging_populate(pg_dom, l1e_get_pfn(nl1e));
+                return -ENOENT;
+            }
+
+            if ( p2mt == p2m_ram_paging_in && !page )
+                return -ENOENT;
+
+            /* Did our attempt to unshare fail? */
+            if ( (q & P2M_UNSHARE) && p2m_is_shared(p2mt) )
+            {
+                /* We could not have obtained a page ref. */
+                ASSERT(!page);
+                /* And mem_sharing_notify has already been called. */
+                return -ENOMEM;
+            }
+
             if ( !page )
                 return -EINVAL;
             nl1e = l1e_from_page(page, l1e_get_flags(nl1e));
@@ -3315,47 +3340,10 @@  long do_mmu_update(
                 switch ( page->u.inuse.type_info & PGT_type_mask )
                 {
                 case PGT_l1_page_table:
-                {
-                    l1_pgentry_t l1e = l1e_from_intpte(req.val);
-                    p2m_type_t l1e_p2mt = p2m_ram_rw;
-                    struct page_info *target = NULL;
-                    p2m_query_t q = (l1e_get_flags(l1e) & _PAGE_RW) ?
-                                        P2M_UNSHARE : P2M_ALLOC;
-
-                    if ( paging_mode_translate(pg_owner) )
-                        target = get_page_from_gfn(pg_owner, l1e_get_pfn(l1e),
-                                                   &l1e_p2mt, q);
-
-                    if ( p2m_is_paged(l1e_p2mt) )
-                    {
-                        if ( target )
-                            put_page(target);
-                        p2m_mem_paging_populate(pg_owner, l1e_get_pfn(l1e));
-                        rc = -ENOENT;
-                        break;
-                    }
-                    else if ( p2m_ram_paging_in == l1e_p2mt && !target )
-                    {
-                        rc = -ENOENT;
-                        break;
-                    }
-                    /* If we tried to unshare and failed */
-                    else if ( (q & P2M_UNSHARE) && p2m_is_shared(l1e_p2mt) )
-                    {
-                        /* We could not have obtained a page ref. */
-                        ASSERT(target == NULL);
-                        /* And mem_sharing_notify has already been called. */
-                        rc = -ENOMEM;
-                        break;
-                    }
-
-                    rc = mod_l1_entry(va, l1e, mfn,
+                    rc = mod_l1_entry(va, l1e_from_intpte(req.val), mfn,
                                       cmd == MMU_PT_UPDATE_PRESERVE_AD, v,
                                       pg_owner);
-                    if ( target )
-                        put_page(target);
-                }
-                break;
+                    break;
                 case PGT_l2_page_table:
                     rc = mod_l2_entry(va, l2e_from_intpte(req.val), mfn,
                                       cmd == MMU_PT_UPDATE_PRESERVE_AD, v);
@@ -3367,7 +3355,7 @@  long do_mmu_update(
                 case PGT_l4_page_table:
                     rc = mod_l4_entry(va, l4e_from_intpte(req.val), mfn,
                                       cmd == MMU_PT_UPDATE_PRESERVE_AD, v);
-                break;
+                    break;
                 case PGT_writable_page:
                     perfc_incr(writable_mmu_updates);
                     if ( paging_write_guest_entry(v, va, req.val, _mfn(mfn)) )