Re: [Xen-devel] [V8 PATCH 5/8] pvh dom0: Add and remove foreign pages

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 11.04.14 at 03:33, <mukesh.rathor@xxxxxxxxxx> wrote:
> On Mon, 24 Mar 2014 09:26:58 +0000
> "Jan Beulich" <JBeulich@xxxxxxxx> wrote:
> 
>> >>> On 22.03.14 at 02:39, <mukesh.rathor@xxxxxxxxxx> wrote:
>> > +static inline void atomic_write_ept_entry(ept_entry_t *entryptr,
>> > +                                          const ept_entry_t *new)
>> > +{
>> > +    unsigned long oldmfn = 0;
>> > +
>> > +    if ( p2m_is_foreign(new->sa_p2mt) )
>> > +    {
>> > +        struct page_info *page;
>> > +        struct domain *fdom;
>> > +
>> > +        ASSERT(mfn_valid(new->mfn));
>> > +        page = mfn_to_page(new->mfn);
>> > +        fdom = page_get_owner(page);
>> > +        get_page(page, fdom);
>> 
>> I'm afraid the checking here is too weak, or at least inconsistent
>> (considering the ASSERT()): mfn_valid() isn't a sufficient condition
>> for page_get_owner() to return non-NULL or get_page() to
>> succeed. If all callers are supposed to guarantee this, then further
>> ASSERT()s should be added here. If not, error checking is going to
> 
> Correct, callers pretty much guarantee that. There are stringent checks
> done in p2m_add_foreign. How about:
> 
>         ASSERT(mfn_valid(new->mfn));
>         page = mfn_to_page(new->mfn);
>         fdom = page_get_owner(page);
>         ASSERT(fdom);
>         rc = get_page(page, fdom);
>         ASSERT(rc == 0);

Yes, minimally that (so we'll at least know if some of the assumptions
are wrong).

>> > +int p2m_add_foreign(struct domain *tdom, unsigned long fgfn,
>> > +                    unsigned long gpfn, domid_t fdid)
>> > +{
>> > +    int rc = -ESRCH;
>> > +    p2m_type_t p2mt, p2mt_prev;
>> > +    unsigned long prev_mfn, mfn;
>> > +    struct page_info *page = NULL;
>> > +    struct domain *fdom = NULL;
>> > +
>> > +    /* xentrace mapping pages from xen heap are owned by DOMID_XEN
>> > */
>> > +    if ( (fdid == DOMID_XEN && (fdom = rcu_lock_domain(dom_xen))
>> > == NULL) ||
>> > +         (fdid != DOMID_XEN && (fdom =
>> > rcu_lock_domain_by_id(fdid)) == NULL) )
>> > +        goto out;
>> 
>> Didn't you mean to call get_pg_owner() here, at once taking care of
>> DOMID_IO too? Also I don't think the reference to xentrace is really
>> useful here - DOMID_XEN owned pages certainly exist elsewhere too.
> 
> IIRC, I think I didn't because it will let you get away with DOMID_SELF.
> I could just check for it before calling get_pg_owner:
> 
> if ( fdid == DOMID_SELF )
>     goto out with -EINVAL; 
> else if ( (fdom = get_pg_owner(fdid)) == NULL )
>     goto out with -ESRCH; 
> ..
> 
> what do you think?

Much more readable than the original imo.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel