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

[Julien Grall <julien@xxxxxxx>]

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