[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



On Fri, Feb 15, 2019 at 07:15:53PM +0100, George Dunlap wrote:
> 
> 
> > 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?

Ack. I added the BUG_ON because trying to add a foreign entry with an
invalid mfn means something else went wrong in the caller, since it
should not be possible to perform such action. As you pointed out
however there's no reason to panic, since an error can be returned to
the caller.

Would you be fine with also adding an ASSERT_UNREACHABLE before
returning the error?

> > +
> > +        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.

Ack, I think what I suggested above should also be used here:

if ( !mfn_valid(ofn) )
{
    ASSERT_UNREACHABLE();
    return -EINVAL;
}

Thanks, Roger.

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.