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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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





[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux