RE: [Intel-gfx] [PATCH i-g-t] tests/drm_hw_lock: Tests for hw_lock fixes.

Thanks for the review, new patch inbound.

-----Original Message----- From: Thomas Wood [mailto:thomas.wood@intel.com] Sent: Monday, April 27, 2015 4:25 PM To: Antoine, Peter Cc: Intel Graphics Development; airlied@redhat.com; dri-devel@lists.freedesktop.org; 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@intel.com 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@intel.com

---

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@intel.com
• */

+/*

• • 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@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx