Thanks for the review, new patch inbound. -----Original Message----- From: Thomas Wood [mailto:thomas.wood@xxxxxxxxx] Sent: Monday, April 27, 2015 4:25 PM To: Antoine, Peter Cc: Intel Graphics Development; airlied@xxxxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx; Daniel Vetter Subject: Re: [Intel-gfx] [PATCH i-g-t] tests/drm_hw_lock: Tests for hw_lock fixes. On 23 April 2015 at 15:07, Peter Antoine <peter.antoine@xxxxxxxxx> wrote: > There are several issues with the hardware locks functions that > stretch from kernel crashes to priority escalations. This new test > will test the the fixes for these features. > > This test will cause a driver/kernel crash on un-patched kernels, the > following patches should be applied to stop the crashes: > > drm: Kernel Crash in drm_unlock > drm: Fixes unsafe deference in locks. > > Issue: VIZ-5485 > Signed-off-by: Peter Antoine <peter.antoine@xxxxxxxxx> > --- > lib/ioctl_wrappers.c | 19 +++++ > lib/ioctl_wrappers.h | 1 + > tests/Makefile.sources | 1 + > tests/drm_hw_lock.c | 207 +++++++++++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 228 insertions(+) > create mode 100644 tests/drm_hw_lock.c > > diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c index > 000d394..ad8b3d3 100644 > --- a/lib/ioctl_wrappers.c > +++ b/lib/ioctl_wrappers.c > @@ -964,6 +964,25 @@ bool gem_has_bsd2(int fd) { > return gem_has_enable_ring(fd,LOCAL_I915_PARAM_HAS_BSD2); > } > +#define I915_PARAM_HAS_LEGACY_CONTEXT 35 Please add some API documentation for this new function here. > +bool drm_has_legacy_context(int fd) > +{ > + int tmp = 0; > + drm_i915_getparam_t gp; > + > + memset(&gp, 0, sizeof(gp)); > + gp.value = &tmp; > + gp.param = I915_PARAM_HAS_LEGACY_CONTEXT; > + > + /* > + * if legacy context param is not supported, then it's old and we > + * can assume that the HW_LOCKS are supported. > + */ > + if (drmIoctl(fd, DRM_IOCTL_I915_GETPARAM, &gp) != 0) > + return true; > + > + return tmp == 1; > +} > /** > * gem_available_aperture_size: > * @fd: open i915 drm file descriptor diff --git > a/lib/ioctl_wrappers.h b/lib/ioctl_wrappers.h index ced7ef3..3adc700 > 100644 > --- a/lib/ioctl_wrappers.h > +++ b/lib/ioctl_wrappers.h > @@ -120,6 +120,7 @@ bool gem_has_bsd(int fd); bool gem_has_blt(int > fd); bool gem_has_vebox(int fd); bool gem_has_bsd2(int fd); > +bool drm_has_legacy_context(int fd); > bool gem_uses_aliasing_ppgtt(int fd); int gem_available_fences(int > fd); uint64_t gem_available_aperture_size(int fd); diff --git > a/tests/Makefile.sources b/tests/Makefile.sources index > 71de6de..2f69afc 100644 > --- a/tests/Makefile.sources > +++ b/tests/Makefile.sources > @@ -84,6 +84,7 @@ TESTS_progs_M = \ > pm_sseu \ > prime_self_import \ > template \ > + drm_hw_lock \ Please also add the binary name to .gitignore. > $(NULL) > > TESTS_progs = \ > diff --git a/tests/drm_hw_lock.c b/tests/drm_hw_lock.c new file mode > 100644 index 0000000..aad50ba > --- /dev/null > +++ b/tests/drm_hw_lock.c > @@ -0,0 +1,207 @@ > +/* > + * Copyright © 2015 Intel Corporation > + * > + * Permission is hereby granted, free of charge, to any person > +obtaining a > + * copy of this software and associated documentation files (the > +"Software"), > + * to deal in the Software without restriction, including without > +limitation > + * the rights to use, copy, modify, merge, publish, distribute, > +sublicense, > + * and/or sell copies of the Software, and to permit persons to whom > +the > + * Software is furnished to do so, subject to the following conditions: > + * > + * The above copyright notice and this permission notice (including > +the next > + * paragraph) shall be included in all copies or substantial portions > +of the > + * Software. > + * > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, > +EXPRESS OR > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF > +MERCHANTABILITY, > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT > +SHALL > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES > +OR OTHER > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, > +ARISING > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR > +OTHER DEALINGS > + * IN THE SOFTWARE. > + * > + * Authors: > + * Peter Antoine <peter.antoine@xxxxxxxxx> > + */ > + > +/* > + * Testcase: Test the HW_LOCKs for correct support and non-crashing. This would be a good sentence to use in the IGT_TEST_DESCRIPTION macro, to add a description for the test. > + * > + * This test will check that he hw_locks are only g_supported for > +drivers that > + * require that support. If it is not g_supported then the functions > +all return > + * the correct error code. > + * > + * If g_supported it will check that the functions do not crash when > +the crash > + * tests are used, also that one of the tests is a security level > +escalation > + * that should be rejected. > + */ > +#include <stdlib.h> > +#include <signal.h> > +#include <sys/ioctl.h> > +#include <drm.h> > +#include "drmtest.h" > +#include "igt_core.h" > +#include "ioctl_wrappers.h" > + > +#ifndef DRM_KERNEL_CONTEXT > +# define DRM_KERNEL_CONTEXT (0) > +#endif > + > +#ifndef _DRM_LOCK_HELD > +# define _DRM_LOCK_HELD 0x80000000U /**< Hardware lock is held */ > +#endif > + > +#ifndef _DRM_LOCK_CONT > +# define _DRM_LOCK_CONT 0x40000000U /**< Hardware lock is contended */ > +#endif > + > +static bool g_sig_fired; > +static bool g_supported; > +static struct sigaction old_action; > + > +static void sig_term_handler(int value) { > + g_sig_fired = true; > +} > + > +static bool set_term_handler(void) > +{ > + static struct sigaction new_action; > + > + new_action.sa_handler = sig_term_handler; > + sigemptyset(&new_action.sa_mask); > + new_action.sa_flags = 0; > + > + igt_assert(sigaction(SIGTERM, &new_action, &old_action) == 0); > + > + if (old_action.sa_handler != SIG_IGN) > + return true; > + else > + return false; > +} > + > +static void restore_term_handler(void) { > + sigaction(SIGTERM, &old_action, NULL); } > + > +static void does_lock_crash(int fd) > +{ > + struct drm_lock rubbish; > + int ret; > + > + memset(&rubbish, 'A', sizeof(struct drm_lock)); > + > + g_sig_fired = false; > + igt_assert(set_term_handler()); Since set_term_handler and restore_term_handler are called at the start and end of every subtest, they could be placed in the respective igt_fixture blocks before and after the subtests in igt_main. > + > + ret = ioctl(fd, DRM_IOCTL_LOCK, &rubbish); > + > + igt_assert(ret == -1 && (!g_supported || g_sig_fired)); > + igt_assert(ret == -1 && (g_supported || errno == EINVAL)); > + > + restore_term_handler(); > +} > + > +static void does_unlock_crash(int fd) { > + struct drm_lock rubbish; > + int ret; > + > + g_sig_fired = false; > + igt_assert(set_term_handler()); > + > + memset(&rubbish, 'A', sizeof(struct drm_lock)); > + > + ret = ioctl(fd, DRM_IOCTL_UNLOCK, &rubbish); > + > + igt_assert(ret == -1 && (!g_supported || g_sig_fired)); > + igt_assert(ret == -1 && (g_supported || errno == EINVAL)); > + > + restore_term_handler(); > +} > + > +static void priority_escalation(int fd) { > + struct drm_lock rubbish; > + int ret; > + > + g_sig_fired = false; > + igt_assert(set_term_handler()); g_sig_fired is not checked in these tests, so presumably it doesn't need to be set before each test? > + > + /* this should be rejected */ > + rubbish.context = DRM_KERNEL_CONTEXT; > + ret = ioctl(fd, DRM_IOCTL_LOCK, &rubbish); > + igt_assert(ret == -1 && (!g_supported || errno == EPERM)); > + igt_assert(ret == -1 && (g_supported || errno == EINVAL)); > + > + /* this should also be rejected */ > + rubbish.context = _DRM_LOCK_HELD | DRM_KERNEL_CONTEXT; > + g_sig_fired = false; > + ret = ioctl(fd, DRM_IOCTL_LOCK, &rubbish); > + igt_assert(ret == -1 && (!g_supported || errno == EPERM)); > + igt_assert(ret == -1 && (g_supported || errno == EINVAL)); > + > + /* this should also be rejected */ > + rubbish.context = _DRM_LOCK_CONT | DRM_KERNEL_CONTEXT; > + g_sig_fired = false; > + ret = ioctl(fd, DRM_IOCTL_LOCK, &rubbish); > + igt_assert(ret == -1 && (!g_supported || errno == EPERM)); > + igt_assert(ret == -1 && (g_supported || errno == EINVAL)); > + > + /* this should be rejected */ > + rubbish.context = DRM_KERNEL_CONTEXT; > + g_sig_fired = false; > + ret = ioctl(fd, DRM_IOCTL_UNLOCK, &rubbish); > + igt_assert(ret == -1 && (!g_supported || errno == EPERM)); > + igt_assert(ret == -1 && (g_supported || errno == EINVAL)); > + > + /* this should also be rejected */ > + rubbish.context = _DRM_LOCK_HELD | DRM_KERNEL_CONTEXT; > + g_sig_fired = false; > + ret = ioctl(fd, DRM_IOCTL_UNLOCK, &rubbish); > + igt_assert(ret == -1 && (!g_supported || errno == EPERM)); > + igt_assert(ret == -1 && (g_supported || errno == EINVAL)); > + > + /* this should also be rejected */ > + rubbish.context = _DRM_LOCK_CONT | DRM_KERNEL_CONTEXT; > + g_sig_fired = false; > + ret = ioctl(fd, DRM_IOCTL_UNLOCK, &rubbish); > + igt_assert(ret == -1 && (!g_supported || errno == EPERM)); > + igt_assert(ret == -1 && (g_supported || errno == EINVAL)); > + > + restore_term_handler(); > +} > + > +igt_main > +{ > + int fd = -1; > + > + g_sig_fired = false; > + g_supported = false; > + > + igt_fixture { > + fd = drm_open_any(); > + igt_assert(fd >= 0); drm_open_any will always return a valid file descriptor. If no device is found, it will call igt_skip. > + } > + > + g_supported = drm_has_legacy_context(fd); This needs to be inside the igt_fixture block above, otherwise it may interfere with subtest enumeration. > + > + igt_info("HW LOCK Supported: %s\n", g_supported?"Yes":"No"); > + > + igt_subtest("lock-crash") { > + does_lock_crash(fd); > + } > + > + igt_subtest("unlock-crash") { > + does_unlock_crash(fd); > + } > + > + igt_subtest("priority-escalation") { > + priority_escalation(fd); > + } > + > + igt_fixture > + close(fd); > +} > + > -- > 1.9.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > http://lists.freedesktop.org/mailman/listinfo/intel-gfx _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
