[Intel-gfx] [PATCH i-g-t] tests/gem_userptr_blits: Test object invalidation more thoroughly

[Chris Wilson]

On Mon, Oct 31, 2016 at 03:36:15PM +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> 
> Verify that the userspace will get told if it changes to what the userptr
> object backing store points to, *after* having created the GEM object.
> 
> Two variants are tested:
>   1. One where the object is used after it has been invalidated but while
>      the address space for the backing store is still invalid.
>   2. Second one where the backing store becomes valid immediately after the
>      object has been invalidated.
> 
> In both cases we want the kernel to give us -EFAULT on the buffer object in
> question since the userspace is acting strangely so the driver does not want
> to take part in corrupting memory.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> ---
>  tests/gem_userptr_blits.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 64 insertions(+)
> 
> diff --git a/tests/gem_userptr_blits.c b/tests/gem_userptr_blits.c
> index f30e14360727..11a29ff5fdde 100644
> --- a/tests/gem_userptr_blits.c
> +++ b/tests/gem_userptr_blits.c
> @@ -1151,6 +1151,58 @@ static void test_unmap_cycles(int fd, int expected)
>  		test_unmap(fd, expected);
>  }
>  
> +/*
> + * Verify that the userspace will get told if it changes to what the userptr
> + * object backing store points to, *after* having created the GEM object.
> + *
> + * Two variants are tested:
> + *   1. One where the object is used after it has been invalidated but while
> + *      the address space for the backing store is still invalid.
> + *   2. Second one where the backing store becomes valid immediately after the
> + *      object has been invalidated.

We still end up with inconsistent behaviour because we do a deferred
gup, and only store ourselves in the mmu tree whilst we have pages. To
close that we would have to add ourselves to the mmutree during
creation (restructuring it to keep the lazy gup).

Current holes would be between creation and execbuf, or swap-out and
execbuf. What we currently check is that the location is valid at the
time of use, not that the contents match both clients expectations.

My concern with using the mmu invalidate notify was that it may kick a
legitimate object. I will have to go and check to see if that is a valid
concern.

> + * In both cases we want the kernel to give us -EFAULT on the buffer object in
> + * question since the userspace is acting strangely so the driver does not want
> + * to take part in corrupting memory.
> + */
> +static void test_remap(int fd, bool midaccess, int expected)
> +{
> +	char *ptr, *ptr2, *bo_ptr;
> +	uint32_t bo[2];
> +	size_t map_size = sizeof(linear) + (PAGE_SIZE - 1);
> +	int ret;
> +
> +	ptr = mmap(NULL, map_size, PROT_READ | PROT_WRITE,
> +		   MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
> +	igt_assert(ptr != MAP_FAILED);
> +
> +	bo_ptr = (char *)ALIGN((unsigned long)ptr, PAGE_SIZE);
> +	gem_userptr(fd, bo_ptr, sizeof(linear), 0, userptr_flags, &bo[0]);
> +
> +	bo[1] = create_bo(fd, 0);
> +

if (gup)
> +	copy(fd, bo[1], bo[0], 0);

if (force_swap)
	mlock(all_memory);

> +	ret = munmap(ptr, map_size);
> +	igt_assert_eq(ret, 0);
> +
> +	if (midaccess)
> +		copy(fd, bo[1], bo[0], expected);
> +
> +	ptr2 = mmap(ptr, map_size, PROT_READ | PROT_WRITE,
> +		    MAP_ANONYMOUS | MAP_PRIVATE | MAP_FIXED, -1, 0);
> +	igt_assert(ptr2 != MAP_FAILED);
> +	igt_assert(ptr2 == ptr);
> +
> +	copy(fd, bo[1], bo[0], expected);
> +
> +	gem_close(fd, bo[1]);
> +	gem_close(fd, bo[0]);
> +
> +	ret = munmap(ptr, map_size);
> +	igt_assert_eq(ret, 0);
> +}

Basically the current attitude is that we didn't care for the client
games, our guarantee was that we didn't subvert the kernel. I do think
your proposed semantic is safer, we might as well try and catch all the
corner cases whilst here and just some pondering over if there are paths
that call invalidate range but we don't want to lose the object.

There are some fun tricks userspace could do with mmap(), such as
sparse textures (mapping the object to the zero page, then populating on
the fly), that don't fit well at all with our object-orientated approach
and would fall afoul of the above scheme. Oh well, we need to treat this
with all the care of an abi change and see if we can find more exotic
users.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre