From mboxrd@z Thu Jan 1 00:00:00 1970 From: Konrad Rzeszutek Wilk Subject: Re: [PATCH v6] Userspace grant communication Date: Mon, 14 Feb 2011 11:14:03 -0500 Message-ID: <20110214161403.GB11034@dumpdata.com> References: <1296753544-13323-1-git-send-email-dgdegra@tycho.nsa.gov> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: <1296753544-13323-1-git-send-email-dgdegra@tycho.nsa.gov> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Daniel De Graaf Cc: jeremy@goop.org, xen-devel@lists.xensource.com, Ian.Campbell@citrix.com List-Id: xen-devel@lists.xenproject.org On Thu, Feb 03, 2011 at 12:18:58PM -0500, Daniel De Graaf wrote: > Changes since v5: > - Added a tested xen version to workaround in #4 > - Cleaned up variable names & structures > - Clarified some of the cleanup in gntalloc > - Removed copyright statement from public-domain files > > [PATCH 1/6] xen-gntdev: Change page limit to be global instead of per-open > [PATCH 2/6] xen-gntdev: Use find_vma rather than iterating our vma list manually > [PATCH 3/6] xen-gntdev: Add reference counting to maps > [PATCH 4/6] xen-gntdev: Support mapping in HVM domains > [PATCH 5/6] xen-gntalloc: Userspace grant allocation driver > [PATCH 6/6] xen/gntalloc,gntdev: Add unmap notify ioctl Hey Daniel, I took a look at the patchset and then the bug-fixes: Daniel De Graaf (12): xen-gntdev: Change page limit to be global instead of per-open xen-gntdev: Use find_vma rather than iterating our vma list manually xen-gntdev: Add reference counting to maps xen-gntdev: Support mapping in HVM domains xen-gntalloc: Userspace grant allocation driver xen/gntalloc,gntdev: Add unmap notify ioctl xen-gntdev: Fix memory leak when mmap fails xen-gntdev: Fix unmap notify on PV domains xen-gntdev: Use map->vma for checking map validity xen-gntdev: Avoid unmapping ranges twice xen-gntdev: prevent using UNMAP_NOTIFY_CLEAR_BYTE on read-only mappings xen-gntdev: Avoid double-mapping memory And besides the two questions I posted today they look OK to me. However I have on question that I think points to a bug. Say that I call GNTDEV_MAP_GRANT_REF three times. The first time I provide a count of 4, then 1, and then once more 1. The first call would end up with priv having: priv-map[0] => map.count=4, map.user=1, map.index=0. We return op.index as 0. The next call: priv-map[0] => map.count=4, map.user=1, map.index=0. priv-map[1] => map.count=1, map.user=1, map.index=5 (gntdev_add_map ends up adding the index and the count from the previous map to it). We return op.index as 20480. The last call ends up with priv-map[0] => map.count=4, map.user=1, map.index=0. priv-map[1] => map.count=1, map.user=1, map.index=5 priv-map[2] => map.count=1, map.user=1, map.index=0. And we return op.index as = 0. It looks as gntdev_add_map ends does not do anything to the map.index if the "if (add->index + add->count < map->index)" comes out true, and we end up with op.index=0. Which naturally is incorrect as that is associated with grant_map that has four entries! I hadn't yet tried to modify the nice test-case program you provided to see if this is can happen in practice, but it sure looks like it could?