From mboxrd@z Thu Jan  1 00:00:00 1970
From: Tim Deegan <tim@xen.org>
Subject: Re: [V6 PATCH 6.1/7] pvh dom0: Add and remove foreign
 pages
Date: Mon, 9 Dec 2013 13:11:49 +0100
Message-ID: <20131209121149.GC95928@deinos.phlegethon.org>
References: <1386297524-15483-1-git-send-email-mukesh.rathor@oracle.com>
	<1386297524-15483-7-git-send-email-mukesh.rathor@oracle.com>
	<20131206183416.6c78f955@mantra.us.oracle.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
Content-Disposition: inline
In-Reply-To: <20131206183416.6c78f955@mantra.us.oracle.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Mukesh Rathor <mukesh.rathor@oracle.com>
Cc: george.dunlap@eu.citrix.com, Xen-devel@lists.xensource.com, keir.xen@gmail.com, JBeulich@suse.com, Ian Campbell <Ian.Campbell@citrix.com>
List-Id: xen-devel@lists.xenproject.org

Hi,

At 18:34 -0800 on 06 Dec (1386351256), Mukesh Rathor wrote:
> In this patch, a new function, xenmem_add_foreign_to_p2m(), is added
> to map pages from foreign guest into current dom0 for domU creation.
> Such pages are typed p2m_map_foreign. Another function
> xenmem_rem_foreign_from_p2m() is added to remove such pages. Note, in
> the remove path, we must release the refcount that was taken during
> the map phase.
> 
> Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>
[...]
> +/*
> + * Add frames from foreign domain to target domain's physmap. Similar to
> + * XENMAPSPACE_gmfn but the frame is foreign being mapped into current,
> + * and is not removed from foreign domain.
> + * Usage: libxl on pvh dom0 creating a guest and doing privcmd_ioctl_mmap.
> + * Side Effect: the mfn for fgfn will be refcounted so it is not lost
> + *              while mapped here. The refcnt is released in do_memory_op()
> + *              via XENMEM_remove_from_physmap.

Is that comment out of date?  AFAICS the put_page() happens...

> +/* Note, the refcnt released here is taken in xenmem_add_foreign_to_p2m */
> +int xenmem_rem_foreign_from_p2m(struct domain *d, unsigned long gpfn)
> +{
> +    unsigned long mfn;
> +    p2m_type_t p2mt;
> +    struct domain *foreign_dom;
> +
> +    mfn = mfn_x(get_gfn_query(d, gpfn, &p2mt));
> +    if ( !mfn_valid(mfn) )
> +    {
> +        gdprintk(XENLOG_WARNING, "Invalid mfn for gpfn:%lx domid:%d\n",
> +                 gpfn, d->domain_id);
> +        return -EINVAL;
> +    }
> +
> +    foreign_dom = page_get_owner(mfn_to_page(mfn));
> +    ASSERT(d != foreign_dom);
> +    ASSERT(is_pvh_domain(d));
> +
> +    guest_physmap_remove_page(d, gpfn, mfn, 0);
> +    put_page(mfn_to_page(mfn));

...here, and doesn't look safe.  This put_page() is to balance the
get_page() in xenmem_add_foreign_to_p2m() but (a) you haven't checked
here that the entry you're removing is actually a foreign one and (b)
you haven't updated any of the other paths that might clear a p2m
entry that contained a foreign mapping.

I think the refcounting will have to be done at the bottom of the
arch-specific implementation, where the actual p2m entry gets set or
cleared.

Tim.