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

Re: [Xen-devel] [PATCH] libxc: fix xc_gntshr_munmap semantic

On 04/30/2013 10:00 AM, Ian Campbell wrote:
On Tue, 2013-04-30 at 14:21 +0100, Daniel De Graaf wrote:
On 04/30/2013 06:39 AM, Ian Campbell wrote:
On Fri, 2013-04-26 at 17:17 +0100, Daniel De Graaf wrote:
On 04/26/2013 11:26 AM, Ian Campbell wrote:
On Fri, 2013-04-26 at 16:15 +0100, Daniel De Graaf wrote:
On 04/26/2013 10:44 AM, Ian Campbell wrote:
On Fri, 2013-04-26 at 13:40 +0100, Marek Marczykowski wrote:
"count" parameter should be pages count (as stated in comment in
xenctrl.h), not bytes count.
This patch fixes also the only user of this function (in xen sources) -

Looks ok to me but Daniel De Graaf wrote all this stuff, Ccing him.

This also looks good to me.

May I take that as an Ack (or a Reviewed-by if you prefer)?

Yes, either one is fine.

Acked-by: Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>

Is the change from munmap to xc_gntshr_munmap, which wasn't mentioned in
the changelog description (tut tut), correct? It seems like these
mappings can either be establish with xc_gntshr_share_pages or with "=
((void*)ctrl->ring) + LARGE_RING_OFFSET", with the second one being the
case I'm concerned about... Should it not duplicate the switch used at
mapping time?


The unmap is only done if (ctrl->read.order >= PAGE_SHIFT), which is a
functional duplicate of the switch statement.

It's a pretty opaque transformation ;-)

But for e.g. order 9, it isn't the same is it? The switch on alloc will
hit the default case while the free won't hit this if statement.

Order 9 isn't valid: the only permitted values are 10, 11, and 12+, all of
which are handled correctly here.

  However, this does bring
up a different issue: the munmap() should be replaced with the correct
one of xc_gntshr_munmap and xc_gnttab_munmap depending on ctrl->is_server,
in the same way as ctrl->ring is unmapped.

OK, I take it the Ack is rescinded for the time being?

The change is still a strict improvement over the old code, but since more
changes are needed to complete the fix, it is rescinded for now.

Daniel De Graaf
National Security Agency

Xen-devel mailing list



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