Re: [PATCH] tests/gem_reset_stats: add close-pending-fork

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Dec 04, 2013 at 04:39:09PM +0200, Mika Kuoppala wrote:
> From: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> 
> This triggers use after free oops on request->batch_obj when
> going through the rings and setting reset status on requests,
> after a gpu hang.
> 
> v2: Streamlined the test and added comments (Daniel)
> 
> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> ---
>  tests/gem_reset_stats.c |   62 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 62 insertions(+)
> 
> diff --git a/tests/gem_reset_stats.c b/tests/gem_reset_stats.c
> index 6c22bce..095b14b 100644
> --- a/tests/gem_reset_stats.c
> +++ b/tests/gem_reset_stats.c
> @@ -25,6 +25,7 @@
>   *
>   */
>  
> +#define _GNU_SOURCE
>  #include <unistd.h>
>  #include <stdlib.h>
>  #include <stdio.h>
> @@ -36,6 +37,7 @@
>  #include <sys/ioctl.h>
>  #include <sys/mman.h>
>  #include <time.h>
> +#include <signal.h>
>  
>  #include "i915_drm.h"
>  #include "intel_bufmgr.h"
> @@ -637,6 +639,63 @@ static void test_close_pending(void)
>  	close(fd);
>  }
>  
> +static void test_close_pending_fork(void)
> +{
> +	int pid;
> +	int fd, h;
> +
> +	fd = drm_open_any();
> +	igt_assert(fd >= 0);
> +
> +	assert_reset_status(fd, 0, RS_NO_ERROR);
> +
> +	h = inject_hang(fd, 0);
> +	igt_assert(h >= 0);
> +
> +	sleep(1);
> +
> +	/* Avoid helpers as we need to kill the child
> +	 * without any extra signal handling on behalf of
> +	 * lib/drmtest.c
> +	 */
> +	pid = fork();
> +	if (pid == 0) {
> +		/* Not first drm_open_any() so we need to do
> +		 * gem_quiescent_gpu() explicitly, as it is the
> +		 * key component to trigger the oops
> +		 */

I've added a small comment here explaining what exactly is the magic
ingredient in quiescent_gpu and applied and pushed the patch.

But on second thoughs it's a bit fragile to depend upon the test library
behaviour in such a way. I think it's better to copy-paste the code in
quiescent_gpu to this file to make sure we never accidentally change it
and end up breaking this test.

When doing that we could also try to run the empty batch on all rings in
reverse order, just in case someone reorders a list somewhere. That should
make the testcase more robust at catching issues.
-Daniel

> +		const int fd2 = drm_open_any();
> +		igt_assert(fd2 >= 0);
> +
> +		/* This adds same batch on each ring */
> +		gem_quiescent_gpu(fd2);
> +
> +		close(fd2);
> +		return;
> +	} else {
> +		igt_assert(pid > 0);
> +		sleep(1);
> +
> +		/* Kill the child to reduce refcounts on
> +		   batch_objs */
> +		kill(pid, SIGKILL);
> +	}
> +
> +	gem_close(fd, h);
> +	close(fd);
> +
> +	/* Then we just wait on hang to happen */
> +	fd = drm_open_any();
> +	igt_assert(fd >= 0);
> +
> +	h = exec_valid(fd, 0);
> +	igt_assert(h >= 0);
> +
> +	gem_sync(fd, h);
> +	gem_close(fd, h);
> +	close(fd);
> +}
> +
>  static void drop_root(void)
>  {
>  	igt_assert(getuid() == 0);
> @@ -837,6 +896,9 @@ igt_main
>  	igt_subtest("close-pending")
>  		test_close_pending();
>  
> +	igt_subtest("close-pending-fork")
> +		test_close_pending_fork();
> +
>  	igt_subtest("params")
>  		test_params();
>  }
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch