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

[Julien Grall]

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,

--
Julien Grall