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

[Antoine, Peter]

On Mon, 2015-04-27 at 16:33 +0100, Chris Wilson wrote:
> On Mon, Apr 27, 2015 at 04:24:37PM +0100, Thomas Wood wrote:
> > On 23 April 2015 at 15:07, Peter Antoine <peter.antoine at 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 at 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;
> 
> Would not a simpler test be to try and legally acquire a hwlock?
> If it fails, hwlocks are not supported. No need for a PARAM.
> -Chris
> 

The test relies on the hw_lock not being created (f the HW-LOCKs are
supported). If I grab, then delete the lock I might find more problems
with this code. :)

As you have to be root to create and delete the hwlock the security
issues are minimal as you have root already you don't really need to use
these ioctls to harm the system. So as the exposure is minimal, any more
fixes on code that is legacy and being turned off seems to be a bit of a
time waste.

Also, the PARAM should give legitimate code the opportunity to avoid the
problems but avoiding these features completely.

Peter.