diff mbox series

[V1,10/16] xen/mm: Handle properly reference in set_foreign_p2m_entry() on Arm

Message ID 1599769330-17656-11-git-send-email-olekstysh@gmail.com (mailing list archive)
State Superseded
Headers show
Series IOREQ feature (+ virtio-mmio) on Arm | expand

Commit Message

Oleksandr Tyshchenko Sept. 10, 2020, 8:22 p.m. UTC 
From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

This patch implements reference counting of foreign entries in
in set_foreign_p2m_entry() on Arm. This is a mandatory action if
we want to run emulator (IOREQ server) in other than dom0 domain,
as we can't trust it to do the right thing if it is not running
in dom0. So we need to grab a reference on the page to avoid it
disappearing.

It was tested with IOREQ feature to confirm that all the pages given
to this function belong to a domain, so we can use the same approach
as for XENMAPSPACE_gmfn_foreign handling in xenmem_add_to_physmap_one().

This involves adding an extra parameter for the foreign domain to
set_foreign_p2m_entry().

Also remove restriction for the hardware domain in the common code
if we run on Arm.

Signed-off-by: Julien Grall <julien.grall@arm.com>
Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

---
Please note, this is a split/cleanup/hardening of Julien's PoC:
"Add support for Guest IO forwarding to a device emulator"

Changes RFC -> V1:
   - new patch, was split from:
     "[RFC PATCH V1 04/12] xen/arm: Introduce arch specific bits for IOREQ/DM features"
   - rewrite a logic to handle properly reference in set_foreign_p2m_entry()
     instead of treating foreign entries as p2m_ram_rw
---
---
 xen/arch/arm/p2m.c        | 16 ++++++++++++++++
 xen/arch/x86/mm/p2m.c     |  5 +++--
 xen/common/memory.c       |  4 +++-
 xen/include/asm-arm/p2m.h | 11 ++---------
 xen/include/asm-x86/p2m.h |  3 ++-
 5 files changed, 26 insertions(+), 13 deletions(-)

Comments

Jan Beulich Sept. 16, 2020, 7:17 a.m. UTC | #1 
On 10.09.2020 22:22, Oleksandr Tyshchenko wrote:
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -1155,6 +1155,7 @@ static int acquire_resource(
>          xen_pfn_t gfn_list[ARRAY_SIZE(mfn_list)];
>          unsigned int i;
>  
> +#ifndef CONFIG_ARM
>          /*
>           * FIXME: Until foreign pages inserted into the P2M are properly
>           *        reference counted, it is unsafe to allow mapping of
> @@ -1162,13 +1163,14 @@ static int acquire_resource(
>           */
>          if ( !is_hardware_domain(currd) )
>              return -EACCES;
> +#endif

Instead of #ifdef, may I ask that a predicate like arch_refcounts_p2m()
be used?

>          if ( copy_from_guest(gfn_list, xmar.frame_list, xmar.nr_frames) )
>              rc = -EFAULT;
>  
>          for ( i = 0; !rc && i < xmar.nr_frames; i++ )
>          {
> -            rc = set_foreign_p2m_entry(currd, gfn_list[i],
> +            rc = set_foreign_p2m_entry(currd, d, gfn_list[i],
>                                         _mfn(mfn_list[i]));

Is it going to lead to proper behavior when d == currd, specifically
for Arm but also in the abstract model? If you expose this to other
than Dom0, this case needs at least considering (and hence mentioning
in the description of why it's safe to permit if you don't reject
such attempts). Personally I'd view it as wrong to use
p2m_map_foreign_rw in this case, even in the event that it can be
shown that nothing breaks in such a case. But I'm open to be
convinced of the opposite.

> --- a/xen/include/asm-arm/p2m.h
> +++ b/xen/include/asm-arm/p2m.h
> @@ -381,15 +381,8 @@ static inline gfn_t gfn_next_boundary(gfn_t gfn, unsigned int order)
>      return gfn_add(gfn, 1UL << order);
>  }
>  
> -static inline int set_foreign_p2m_entry(struct domain *d, unsigned long gfn,
> -                                        mfn_t mfn)
> -{
> -    /*
> -     * NOTE: If this is implemented then proper reference counting of
> -     *       foreign entries will need to be implemented.
> -     */
> -    return -EOPNOTSUPP;
> -}
> +int set_foreign_p2m_entry(struct domain *d, struct domain *fd,
> +                          unsigned long gfn,  mfn_t mfn);

With this and ...

> --- a/xen/include/asm-x86/p2m.h
> +++ b/xen/include/asm-x86/p2m.h
> @@ -635,7 +635,8 @@ int p2m_is_logdirty_range(struct p2m_domain *, unsigned long start,
>                            unsigned long end);
>  
>  /* Set foreign entry in the p2m table (for priv-mapping) */
> -int set_foreign_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn);
> +int set_foreign_p2m_entry(struct domain *d, struct domain *fd,
> +                          unsigned long gfn, mfn_t mfn);

... this now being identical (and as a result it being expected
that future ports would also want to implement a proper function)
except for the stray blank in the Arm variant, I'd like it to be
at least considered to move the declaration to xen/p2m-common.h.

Jan
Jan Beulich Sept. 16, 2020, 8:08 a.m. UTC | #2 
On 10.09.2020 22:22, Oleksandr Tyshchenko wrote:
> --- a/xen/include/asm-x86/p2m.h
> +++ b/xen/include/asm-x86/p2m.h
> @@ -635,7 +635,8 @@ int p2m_is_logdirty_range(struct p2m_domain *, unsigned long start,
>                            unsigned long end);
>  
>  /* Set foreign entry in the p2m table (for priv-mapping) */
> -int set_foreign_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn);
> +int set_foreign_p2m_entry(struct domain *d, struct domain *fd,
> +                          unsigned long gfn, mfn_t mfn);

Once

https://lists.xenproject.org/archives/html/xen-devel/2020-09/msg01092.html

has gone in, the new parameter wants to be const-qualified.

Jan
Julien Grall Sept. 16, 2020, 8:50 a.m. UTC | #3 
Hi,

On 16/09/2020 08:17, Jan Beulich wrote:
> On 10.09.2020 22:22, Oleksandr Tyshchenko wrote:
>> --- a/xen/common/memory.c
>> +++ b/xen/common/memory.c
>> @@ -1155,6 +1155,7 @@ static int acquire_resource(
>>           xen_pfn_t gfn_list[ARRAY_SIZE(mfn_list)];
>>           unsigned int i;
>>   
>> +#ifndef CONFIG_ARM
>>           /*
>>            * FIXME: Until foreign pages inserted into the P2M are properly
>>            *        reference counted, it is unsafe to allow mapping of
>> @@ -1162,13 +1163,14 @@ static int acquire_resource(
>>            */
>>           if ( !is_hardware_domain(currd) )
>>               return -EACCES;
>> +#endif
> 
> Instead of #ifdef, may I ask that a predicate like arch_refcounts_p2m()
> be used?

+1

> 
>>           if ( copy_from_guest(gfn_list, xmar.frame_list, xmar.nr_frames) )
>>               rc = -EFAULT;
>>   
>>           for ( i = 0; !rc && i < xmar.nr_frames; i++ )
>>           {
>> -            rc = set_foreign_p2m_entry(currd, gfn_list[i],
>> +            rc = set_foreign_p2m_entry(currd, d, gfn_list[i],
>>                                          _mfn(mfn_list[i]));
> 
> Is it going to lead to proper behavior when d == currd, specifically
> for Arm but also in the abstract model? If you expose this to other
> than Dom0, this case needs at least considering (and hence mentioning
> in the description of why it's safe to permit if you don't reject
> such attempts).

This is already rejected by rcu_lock_remote_domain_by_id().

> Personally I'd view it as wrong to use
> p2m_map_foreign_rw in this case, even in the event that it can be
> shown that nothing breaks in such a case. But I'm open to be
> convinced of the opposite.

I would agree that p2m_map_foreign_rw would be wrong to use when currd 
== d. But this cannot happen, so I think p2m_map_foreign_rw is the 
proper type.

Cheers,
Jan Beulich Sept. 16, 2020, 8:52 a.m. UTC | #4 
On 16.09.2020 10:50, Julien Grall wrote:
> On 16/09/2020 08:17, Jan Beulich wrote:
>> On 10.09.2020 22:22, Oleksandr Tyshchenko wrote:
>>>           for ( i = 0; !rc && i < xmar.nr_frames; i++ )
>>>           {
>>> -            rc = set_foreign_p2m_entry(currd, gfn_list[i],
>>> +            rc = set_foreign_p2m_entry(currd, d, gfn_list[i],
>>>                                          _mfn(mfn_list[i]));
>>
>> Is it going to lead to proper behavior when d == currd, specifically
>> for Arm but also in the abstract model? If you expose this to other
>> than Dom0, this case needs at least considering (and hence mentioning
>> in the description of why it's safe to permit if you don't reject
>> such attempts).
> 
> This is already rejected by rcu_lock_remote_domain_by_id().

Oh, yes, I'm sorry for overlooking this.

Jan
Julien Grall Sept. 16, 2020, 8:55 a.m. UTC | #5 
On 16/09/2020 09:52, Jan Beulich wrote:
> On 16.09.2020 10:50, Julien Grall wrote:
>> On 16/09/2020 08:17, Jan Beulich wrote:
>>> On 10.09.2020 22:22, Oleksandr Tyshchenko wrote:
>>>>            for ( i = 0; !rc && i < xmar.nr_frames; i++ )
>>>>            {
>>>> -            rc = set_foreign_p2m_entry(currd, gfn_list[i],
>>>> +            rc = set_foreign_p2m_entry(currd, d, gfn_list[i],
>>>>                                           _mfn(mfn_list[i]));
>>>
>>> Is it going to lead to proper behavior when d == currd, specifically
>>> for Arm but also in the abstract model? If you expose this to other
>>> than Dom0, this case needs at least considering (and hence mentioning
>>> in the description of why it's safe to permit if you don't reject
>>> such attempts).
>>
>> This is already rejected by rcu_lock_remote_domain_by_id().
> 
> Oh, yes, I'm sorry for overlooking this.

That's fine, I also overlooked it when I originally wrote the code.

@oleksandr, it might be worth mentioning in the commit message and maybe 
in the code this subtlety.

Cheers,
Oleksandr Tyshchenko Sept. 22, 2020, 5:30 p.m. UTC | #6 
On 16.09.20 11:55, Julien Grall wrote:

Hi Julien, Jan

>
>
> On 16/09/2020 09:52, Jan Beulich wrote:
>> On 16.09.2020 10:50, Julien Grall wrote:
>>> On 16/09/2020 08:17, Jan Beulich wrote:
>>>> On 10.09.2020 22:22, Oleksandr Tyshchenko wrote:
>>>>>            for ( i = 0; !rc && i < xmar.nr_frames; i++ )
>>>>>            {
>>>>> -            rc = set_foreign_p2m_entry(currd, gfn_list[i],
>>>>> +            rc = set_foreign_p2m_entry(currd, d, gfn_list[i],
>>>>> _mfn(mfn_list[i]));
>>>>
>>>> Is it going to lead to proper behavior when d == currd, specifically
>>>> for Arm but also in the abstract model? If you expose this to other
>>>> than Dom0, this case needs at least considering (and hence mentioning
>>>> in the description of why it's safe to permit if you don't reject
>>>> such attempts).
>>>
>>> This is already rejected by rcu_lock_remote_domain_by_id().
>>
>> Oh, yes, I'm sorry for overlooking this.
>
> That's fine, I also overlooked it when I originally wrote the code.
>
> @oleksandr, it might be worth mentioning in the commit message and 
> maybe in the code this subtlety.

Yes, will do.

Also the following will be taken into the account:

1. Implement arch_refcounts_p2m()
2. Move function declaration to xen/p2m-common.h
3. Add const to new parameter
diff mbox series

Patch

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index ce59f2b..cb64fc5 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1385,6 +1385,22 @@  int guest_physmap_remove_page(struct domain *d, gfn_t gfn, mfn_t mfn,
     return p2m_remove_mapping(d, gfn, (1 << page_order), mfn);
 }
 
+int set_foreign_p2m_entry(struct domain *d, struct domain *fd,
+                          unsigned long gfn, mfn_t mfn)
+{
+    struct page_info *page = mfn_to_page(mfn);
+    int rc;
+
+    if ( !get_page(page, fd) )
+        return -EINVAL;
+
+    rc = guest_physmap_add_entry(d, _gfn(gfn), mfn, 0, p2m_map_foreign_rw);
+    if ( rc )
+        put_page(page);
+
+    return 0;
+}
+
 static struct page_info *p2m_allocate_root(void)
 {
     struct page_info *page;
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index db7bde0..f27f8a4 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1320,7 +1320,8 @@  static int set_typed_p2m_entry(struct domain *d, unsigned long gfn_l,
 }
 
 /* Set foreign mfn in the given guest's p2m table. */
-int set_foreign_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn)
+int set_foreign_p2m_entry(struct domain *d, struct domain *fd,
+                          unsigned long gfn, mfn_t mfn)
 {
     return set_typed_p2m_entry(d, gfn, mfn, PAGE_ORDER_4K, p2m_map_foreign,
                                p2m_get_hostp2m(d)->default_access);
@@ -2619,7 +2620,7 @@  int p2m_add_foreign(struct domain *tdom, unsigned long fgfn,
      * will update the m2p table which will result in  mfn -> gpfn of dom0
      * and not fgfn of domU.
      */
-    rc = set_foreign_p2m_entry(tdom, gpfn, mfn);
+    rc = set_foreign_p2m_entry(tdom, fdom, gpfn, mfn);
     if ( rc )
         gdprintk(XENLOG_WARNING, "set_foreign_p2m_entry failed. "
                  "gpfn:%lx mfn:%lx fgfn:%lx td:%d fd:%d\n",
diff --git a/xen/common/memory.c b/xen/common/memory.c
index e551fa6..78781f1 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -1155,6 +1155,7 @@  static int acquire_resource(
         xen_pfn_t gfn_list[ARRAY_SIZE(mfn_list)];
         unsigned int i;
 
+#ifndef CONFIG_ARM
         /*
          * FIXME: Until foreign pages inserted into the P2M are properly
          *        reference counted, it is unsafe to allow mapping of
@@ -1162,13 +1163,14 @@  static int acquire_resource(
          */
         if ( !is_hardware_domain(currd) )
             return -EACCES;
+#endif
 
         if ( copy_from_guest(gfn_list, xmar.frame_list, xmar.nr_frames) )
             rc = -EFAULT;
 
         for ( i = 0; !rc && i < xmar.nr_frames; i++ )
         {
-            rc = set_foreign_p2m_entry(currd, gfn_list[i],
+            rc = set_foreign_p2m_entry(currd, d, gfn_list[i],
                                        _mfn(mfn_list[i]));
             /* rc should be -EIO for any iteration other than the first */
             if ( rc && i )
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index 5fdb6e8..53ce373 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -381,15 +381,8 @@  static inline gfn_t gfn_next_boundary(gfn_t gfn, unsigned int order)
     return gfn_add(gfn, 1UL << order);
 }
 
-static inline int set_foreign_p2m_entry(struct domain *d, unsigned long gfn,
-                                        mfn_t mfn)
-{
-    /*
-     * NOTE: If this is implemented then proper reference counting of
-     *       foreign entries will need to be implemented.
-     */
-    return -EOPNOTSUPP;
-}
+int set_foreign_p2m_entry(struct domain *d, struct domain *fd,
+                          unsigned long gfn,  mfn_t mfn);
 
 /*
  * A vCPU has cache enabled only when the MMU is enabled and data cache
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index 8abae34..23bdca1 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -635,7 +635,8 @@  int p2m_is_logdirty_range(struct p2m_domain *, unsigned long start,
                           unsigned long end);
 
 /* Set foreign entry in the p2m table (for priv-mapping) */
-int set_foreign_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn);
+int set_foreign_p2m_entry(struct domain *d, struct domain *fd,
+                          unsigned long gfn, mfn_t mfn);
 
 /* Set mmio addresses in the p2m table (for pass-through) */
 int set_mmio_p2m_entry(struct domain *d, gfn_t gfn, mfn_t mfn,