Re: [PATCH v6] Userspace grant communication

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

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?