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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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)) )
>     {
>         ASSERT_UNREACHABLE();
>         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.

Jan



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