Re: [Xen-devel] [PATCH v3 7/8] x86/mm: handle foreign mappings in p2m_entry_modify

[George Dunlap <George.Dunlap@xxxxxxxxxx>]

> On Feb 15, 2019, at 2:18 PM, Roger Pau Monne <roger.pau@xxxxxxxxxx> wrote:
> 
> So that the specific handling can be removed from
> atomic_write_ept_entry and be shared with npt and shadow code.
> 
> This commit also removes the check that prevent non-ept PVH dom0 from
> mapping foreign pages.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Mostly looks good; just a few comments...
> 
> diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
> index f4ec2becbd..1687b31571 100644
> --- a/xen/include/asm-x86/p2m.h
> +++ b/xen/include/asm-x86/p2m.h
> @@ -932,11 +932,14 @@ int p2m_set_ioreq_server(struct domain *d, unsigned int 
> flags,
> struct hvm_ioreq_server *p2m_get_ioreq_server(struct domain *d,
>                                               unsigned int *flags);
> 
> -static inline void p2m_entry_modify(struct p2m_domain *p2m, p2m_type_t nt,
> -                                    p2m_type_t ot, unsigned int level)
> +static inline int p2m_entry_modify(struct p2m_domain *p2m, p2m_type_t nt,
> +                                   p2m_type_t ot, mfn_t nfn, mfn_t ofn,
> +                                   unsigned int level)
> {
> -    if ( level != 1 || nt == ot )
> -        return;
> +    BUG_ON(level > 1 && (nt == p2m_ioreq_server || nt == p2m_map_foreign));
> +
> +    if ( level != 1 || (nt == ot && mfn_eq(nfn, ofn)) )
> +        return 0;
> 
>     switch ( nt )
>     {
> @@ -948,6 +951,14 @@ static inline void p2m_entry_modify(struct p2m_domain 
> *p2m, p2m_type_t nt,
>         p2m->ioreq.entry_count++;
>         break;
> 
> +    case p2m_map_foreign:
> +        BUG_ON(!mfn_valid(nfn));

Since we’re going to be returning errors anyway, why not retain the original 
functionality of returning -EINVAL in this case, rather than BUG_ON?

> +
> +        if ( !page_get_owner_and_reference(mfn_to_page(nfn)) )
> +            return -EBUSY;
> +
> +        break;
> +
>     default:
>         break;
>     }
> @@ -959,9 +970,16 @@ static inline void p2m_entry_modify(struct p2m_domain 
> *p2m, p2m_type_t nt,
>         p2m->ioreq.entry_count--;
>         break;
> 
> +    case p2m_map_foreign:
> +        BUG_ON(!mfn_valid(ofn));

If somehow this happened, then the bug isn’t here but somewhere else; 
continuing on won’t make things any worse than they would be if this page 
weren’t removed.  I think this should probably be an ASSERT() (to help narrow 
down where a bug may have come from), followed by a simple return.  Likely the 
worst that would happen here is an un-killable domain; no need to crash 
production servers in this case.

Thanks,
 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel