| Message ID | 1385995666-3541-1-git-send-email-mika.kuoppala@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Dec 02, 2013 at 04:47:46PM +0200, Mika Kuoppala wrote: > Fork and create another filedesc to wait access to already > hung GPU and then kill it. This triggers use after free of the > request->batch_obj. > > Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> > --- > +static int __test_open_any(void) > +{ > + char *name; > + int ret, fd; > + > + ret = asprintf(&name, "/dev/dri/card%d", drm_get_card()); > + if (ret == -1) > + return -1; > + > + fd = open(name, O_RDWR); > + free(name); > + > + return fd; > +} > + > +static void test_close_pending_fork(void) > +{ > + int pid; > + int fd, fd2, h; > + > + fd = drm_open_any(); > + igt_assert(fd >= 0); [snip] > + close(fd); > + > + fd = drm_open_any(); This breaks the testsuite since drm_open_any() will now return the closed fd. Hence your __test_open_any() above. However this does not justify kernel doing anything other than terminating your process with extreme prejudice. -Chris
Chris Wilson <chris@chris-wilson.co.uk> writes: > On Mon, Dec 02, 2013 at 04:47:46PM +0200, Mika Kuoppala wrote: >> Fork and create another filedesc to wait access to already >> hung GPU and then kill it. This triggers use after free of the >> request->batch_obj. >> >> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> >> --- >> +static int __test_open_any(void) >> +{ >> + char *name; >> + int ret, fd; >> + >> + ret = asprintf(&name, "/dev/dri/card%d", drm_get_card()); >> + if (ret == -1) >> + return -1; >> + >> + fd = open(name, O_RDWR); >> + free(name); >> + >> + return fd; >> +} >> + >> +static void test_close_pending_fork(void) >> +{ >> + int pid; >> + int fd, fd2, h; >> + >> + fd = drm_open_any(); >> + igt_assert(fd >= 0); > > [snip] > >> + close(fd); >> + >> + fd = drm_open_any(); > > This breaks the testsuite since drm_open_any() will now return the > closed fd. Hence your __test_open_any() above. > > However this does not justify kernel doing anything other than terminating > your process with extreme prejudice. I discussed this with Chris in IRC. There seems to be no problem as drm_open_any doesn't cache file descriptors. The reason i use __test_open_any is that I needed a way to bypass testsuites exit handling. --Mika
On Mon, Dec 02, 2013 at 06:32:51PM +0200, Mika Kuoppala wrote: > Chris Wilson <chris@chris-wilson.co.uk> writes: > > > On Mon, Dec 02, 2013 at 04:47:46PM +0200, Mika Kuoppala wrote: > >> Fork and create another filedesc to wait access to already > >> hung GPU and then kill it. This triggers use after free of the > >> request->batch_obj. > >> > >> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> > >> --- > >> +static int __test_open_any(void) > >> +{ > >> + char *name; > >> + int ret, fd; > >> + > >> + ret = asprintf(&name, "/dev/dri/card%d", drm_get_card()); > >> + if (ret == -1) > >> + return -1; > >> + > >> + fd = open(name, O_RDWR); > >> + free(name); > >> + > >> + return fd; > >> +} > >> + > >> +static void test_close_pending_fork(void) > >> +{ > >> + int pid; > >> + int fd, fd2, h; > >> + > >> + fd = drm_open_any(); > >> + igt_assert(fd >= 0); > > > > [snip] > > > >> + close(fd); > >> + > >> + fd = drm_open_any(); > > > > This breaks the testsuite since drm_open_any() will now return the > > closed fd. Hence your __test_open_any() above. > > > > However this does not justify kernel doing anything other than terminating > > your process with extreme prejudice. > > I discussed this with Chris in IRC. There seems to be no problem > as drm_open_any doesn't cache file descriptors. > > The reason i use __test_open_any is that I needed a way to bypass > testsuites exit handling. I guess a quick comment to explain that in the code would help. Can you please update the patch? -Daniel
diff --git a/tests/gem_reset_stats.c b/tests/gem_reset_stats.c index 6c22bce..242300c 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,64 @@ static void test_close_pending(void) close(fd); } +static int __test_open_any(void) +{ + char *name; + int ret, fd; + + ret = asprintf(&name, "/dev/dri/card%d", drm_get_card()); + if (ret == -1) + return -1; + + fd = open(name, O_RDWR); + free(name); + + return fd; +} + +static void test_close_pending_fork(void) +{ + int pid; + int fd, fd2, h; + + fd = drm_open_any(); + igt_assert(fd >= 0); + + assert_reset_status(fd, 0, RS_NO_ERROR); + + igt_disable_exit_handler(); + + h = inject_hang(fd, 0); + igt_assert(h >= 0); + + sleep(1); + + pid = fork(); + if (pid == 0) { + fd2 = __test_open_any(); + igt_assert(fd2 >= 0); + gem_quiescent_gpu(fd2); + close(fd2); + } else { + igt_assert(pid > 0); + sleep(1); + kill(pid, SIGKILL); + } + + gem_close(fd, h); + close(fd); + + 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 +897,9 @@ igt_main igt_subtest("close-pending") test_close_pending(); + igt_subtest("close-pending-fork") + test_close_pending_fork(); + igt_subtest("params") test_params(); }
Fork and create another filedesc to wait access to already hung GPU and then kill it. This triggers use after free of the request->batch_obj. Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> --- tests/gem_reset_stats.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 63 insertions(+)