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

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

>>> On 14.02.19 at 13:12, <roger.pau@xxxxxxxxxx> wrote:
> On Thu, Feb 14, 2019 at 04:25:49AM -0700, Jan Beulich wrote:
>> >>> On 11.02.19 at 18:46, <roger.pau@xxxxxxxxxx> wrote:
>> > @@ -948,6 +951,11 @@ 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) ||
>> > +               !page_get_owner_and_reference(mfn_to_page(nfn)));
>> > +        break;
>> Asserting that the passed in MFN is valid is fine. Asserting that a
>> reference can be got is not, as this sets us up for a DoS in case
>> of a refcount overflow, or the page having got ballooned out by
>> its owner. That is, the issue of you folding the two original calls
>> into one is wider than just the two distinct error codes getting lost
>> that were previously produced - you can't (currently) report up
>> any error from this low layer. (And I'm sorry, I should have noticed
>> this on v1 already.)
> What about using something like:
> case p2m_map_foreign:
>     BUG_ON(!mfn_valid(nfn));
>     if ( !page_get_owner_and_reference(mfn_to_page(nfn)) )
>     {
>         return;
>     }
>     break;

How would this be any better? In a release build the caller
will now assume all is fine, and the subsequent put_page()
will screw up reference counts.

> It's not strictly worse than what's currently done in EPT code, but I
> agree it should be improved.

The EPT code return -EBUSY in this case, clearly flagging to
the caller that an error has occurred. Most callers of
atomic_write_ept_entry() indeed ASSERT() right now, but
there's the one crucial one which doesn't, in ept_set_entry().

> Improving this will mean modifying the write_p2m_entry hook, and all
> the callers, together with fixing the handling of the return code from
> atomic_write_ept_entry, which is currently only asserted.

... in most cases.

The alternative is to leave EPT code as it is, and introduce similar
handling into NPT/shadow code.


Xen-devel mailing list



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