[Intel-gfx] [PATCH 4/5] drm: Make HW_LOCK access functions optional.
Antoine, Peter
peter.antoine at intel.com
Tue Apr 28 04:29:06 PDT 2015
reply at end.
On Tue, 2015-04-28 at 13:40 +0300, Ville Syrjälä wrote:
> On Tue, Apr 28, 2015 at 05:52:20AM +0000, Antoine, Peter wrote:
> > Hi,
> >
> > (replies inline)
> >
> > -----Original Message-----
> > From: Ville Syrjälä [mailto:ville.syrjala at linux.intel.com]
> > Sent: Monday, April 27, 2015 6:04 PM
> > To: Antoine, Peter
> > Cc: intel-gfx at lists.freedesktop.org; airlied at redhat.com; dri-devel at lists.freedesktop.org; daniel.vetter at ffwll.ch
> > Subject: Re: [Intel-gfx] [PATCH 4/5] drm: Make HW_LOCK access functions optional.
> >
> > On Thu, Apr 23, 2015 at 03:07:57PM +0100, Peter Antoine wrote:
> > > As these functions are only used by one driver and there are security
> > > holes in these functions. Make the functions optional.
> > >
> > > Issue: VIZ-5485
> > > Signed-off-by: Peter Antoine <peter.antoine at intel.com>
> > > ---
> > > drivers/gpu/drm/drm_lock.c | 6 ++++++
> > > drivers/gpu/drm/i915/i915_dma.c | 3 +++
> > > drivers/gpu/drm/nouveau/nouveau_drm.c | 3 ++-
> > > include/drm/drmP.h | 23 ++++++++++++-----------
> > > include/uapi/drm/i915_drm.h | 1 +
> > > 5 files changed, 24 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_lock.c b/drivers/gpu/drm/drm_lock.c
> > > index 94500930..b61d4c7 100644
> > > --- a/drivers/gpu/drm/drm_lock.c
> > > +++ b/drivers/gpu/drm/drm_lock.c
> > > @@ -61,6 +61,9 @@ int drm_legacy_lock(struct drm_device *dev, void *data,
> > > struct drm_master *master = file_priv->master;
> > > int ret = 0;
> > >
> > > + if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT))
> > > + return -EINVAL;
> > > +
> > > ++file_priv->lock_count;
> > >
> > > if (_DRM_LOCKING_CONTEXT(lock->context) == DRM_KERNEL_CONTEXT) { @@
> > > -153,6 +156,9 @@ int drm_legacy_unlock(struct drm_device *dev, void *data, struct drm_file *file_
> > > struct drm_lock *lock = data;
> > > struct drm_master *master = file_priv->master;
> > >
> > > + if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT))
> > > + return -EINVAL;
> > > +
> > > if (_DRM_LOCKING_CONTEXT(lock->context) == DRM_KERNEL_CONTEXT) {
> > > DRM_ERROR("Process %d using kernel context %d\n",
> > > task_pid_nr(current), lock->context); diff --git
> > > a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> > > index e44116f..c771ef0 100644
> > > --- a/drivers/gpu/drm/i915/i915_dma.c
> > > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > > @@ -163,6 +163,9 @@ static int i915_getparam(struct drm_device *dev, void *data,
> > > if (!value)
> > > return -ENODEV;
> > > break;
> > > + case I915_PARAM_HAS_LEGACY_CONTEXT:
> > > + value = drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT);
> > > + break;
> >
> > Seems pointless to add a parameter that'll always be false.
> >
> > There is some history to these changes, the HW_LOCK functions were removed previously but causes an issue with the Nouveau drivers. So that the functions where put back in. So the parameter has been added to allow for that driver to turn the legacy context on as it is needed.
> >
> > > default:
> > > DRM_DEBUG("Unknown parameter %d\n", param->param);
> > > return -EINVAL;
> > > diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c
> > > b/drivers/gpu/drm/nouveau/nouveau_drm.c
> > > index 8763deb..936b423 100644
> > > --- a/drivers/gpu/drm/nouveau/nouveau_drm.c
> > > +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
> > > @@ -940,7 +940,8 @@ static struct drm_driver driver_stub = {
> > > .driver_features =
> > > DRIVER_USE_AGP |
> > > - DRIVER_GEM | DRIVER_MODESET | DRIVER_PRIME | DRIVER_RENDER,
> > > + DRIVER_GEM | DRIVER_MODESET | DRIVER_PRIME | DRIVER_RENDER |
> > > + DRIVER_KMS_LEGACY_CONTEXT,
> >
> > Why is this here? AFAICS only the via driver cares about legacy contexts, and only dri1 drivers care about the hw lock.
> >
> > See above.
> > >
> > > .load = nouveau_drm_load,
> > > .unload = nouveau_drm_unload,
> > > diff --git a/include/drm/drmP.h b/include/drm/drmP.h index
> > > 62c40777..367e42f 100644
> > > --- a/include/drm/drmP.h
> > > +++ b/include/drm/drmP.h
> > > @@ -137,17 +137,18 @@ void drm_err(const char *format, ...); /*@{*/
> > >
> > > /* driver capabilities and requirements mask */
> > > -#define DRIVER_USE_AGP 0x1
> > > -#define DRIVER_PCI_DMA 0x8
> > > -#define DRIVER_SG 0x10
> > > -#define DRIVER_HAVE_DMA 0x20
> > > -#define DRIVER_HAVE_IRQ 0x40
> > > -#define DRIVER_IRQ_SHARED 0x80
> > > -#define DRIVER_GEM 0x1000
> > > -#define DRIVER_MODESET 0x2000
> > > -#define DRIVER_PRIME 0x4000
> > > -#define DRIVER_RENDER 0x8000
> > > -#define DRIVER_ATOMIC 0x10000
> > > +#define DRIVER_USE_AGP 0x1
> > > +#define DRIVER_PCI_DMA 0x8
> > > +#define DRIVER_SG 0x10
> > > +#define DRIVER_HAVE_DMA 0x20
> > > +#define DRIVER_HAVE_IRQ 0x40
> > > +#define DRIVER_IRQ_SHARED 0x80
> > > +#define DRIVER_GEM 0x1000
> > > +#define DRIVER_MODESET 0x2000
> > > +#define DRIVER_PRIME 0x4000
> > > +#define DRIVER_RENDER 0x8000
> > > +#define DRIVER_ATOMIC 0x10000
> > > +#define DRIVER_KMS_LEGACY_CONTEXT 0x20000
> >
> > Why is there KMS in the name?
> >
> > By suggestion of Daniel.
> >
> > I was thinking just checking for GEM, but I think there was some
> > gem+dri1 userland for i915 at some point in time. ums and dri1 are
> > now dead as far as i915 is concerned, so in theory it should be fine.
> > But I'm not sure if some other driver might have the same baggage.
> >
> > Other drivers have the same baggage.
> >
> > I suppose one option would be to check for MODESET instead. kms+dri1 doesn't sound like an entirely sane combination to me.
> >
> > Can't use the MODESET as this was how it was turned off in the previous incarnation and was reverted by Dave Airle.
>
> Reference?
From the next commit [5/5] as it is the one that actually turns off the
functions that were turned off before.
These changes are based on the two patches:
commit c21eb21cb50d58e7cbdcb8b9e7ff68b85cfa5095
Author: Dave Airlie <airlied at redhat.com>
And the commit that the above patch reverts:
commit 7c510133d93dd6f15ca040733ba7b2891ed61fd1
Author: Daniel Vetter <daniel.vetter at ffwll.ch>
>
> >
> > Peter.
> >
> > >
> > >
> > > /*********************************************************************
> > > **/
> > > /** \name Macros to make printk easier */ diff --git
> > > a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h index
> > > 4851d66..0ad611a2 100644
> > > --- a/include/uapi/drm/i915_drm.h
> > > +++ b/include/uapi/drm/i915_drm.h
> > > @@ -350,6 +350,7 @@ typedef struct drm_i915_irq_wait {
> > > #define I915_PARAM_REVISION 32
> > > #define I915_PARAM_SUBSLICE_TOTAL 33
> > > #define I915_PARAM_EU_TOTAL 34
> > > +#define I915_PARAM_HAS_LEGACY_CONTEXT 35
> > >
> > > typedef struct drm_i915_getparam {
> > > int param;
> > > --
> > > 1.9.1
> > >
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx at lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> > --
> > Ville Syrjälä
> > Intel OTC
>
More information about the dri-devel
mailing list