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?
>
> 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
--
Ville Syrjälä
Intel OTC
