| Message ID | 1429798078-18987-5-git-send-email-peter.antoine@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@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. > 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. > > .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? 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. I suppose one option would be to check for MODESET instead. kms+dri1 doesn't sound like an entirely sane combination to me. > > /***********************************************************************/ > /** \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@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Hi, (replies inline) -----Original Message----- From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] Sent: Monday, April 27, 2015 6:04 PM To: Antoine, Peter Cc: intel-gfx@lists.freedesktop.org; airlied@redhat.com; dri-devel@lists.freedesktop.org; daniel.vetter@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@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. 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@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ville Syrjälä Intel OTC
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@linux.intel.com] > Sent: Monday, April 27, 2015 6:04 PM > To: Antoine, Peter > Cc: intel-gfx@lists.freedesktop.org; airlied@redhat.com; dri-devel@lists.freedesktop.org; daniel.vetter@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@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@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Ville Syrjälä > Intel OTC
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@linux.intel.com] > > Sent: Monday, April 27, 2015 6:04 PM > > To: Antoine, Peter > > Cc: intel-gfx@lists.freedesktop.org; airlied@redhat.com; dri-devel@lists.freedesktop.org; daniel.vetter@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@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@redhat.com> And the commit that the above patch reverts: commit 7c510133d93dd6f15ca040733ba7b2891ed61fd1 Author: Daniel Vetter <daniel.vetter@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@lists.freedesktop.org > > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > > Ville Syrjälä > > Intel OTC >
On Tue, Apr 28, 2015 at 11:29:06AM +0000, Antoine, Peter wrote: > > > > 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@redhat.com> > > And the commit that the above patch reverts: > commit 7c510133d93dd6f15ca040733ba7b2891ed61fd1 > Author: Daniel Vetter <daniel.vetter@ffwll.ch> Looking at ancient libdrm sources makes me think nouveau just used to create and destroy the context, but not actually use it for anything. So nopping out the ioctls should be good enough AFAICS. Or am I missing something?
On Tue, 2015-04-28 at 16:08 +0300, Ville Syrjälä wrote: > On Tue, Apr 28, 2015 at 11:29:06AM +0000, Antoine, Peter wrote: > > > > > 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@redhat.com> > > > > And the commit that the above patch reverts: > > commit 7c510133d93dd6f15ca040733ba7b2891ed61fd1 > > Author: Daniel Vetter <daniel.vetter@ffwll.ch> > > Looking at ancient libdrm sources makes me think nouveau just used to > create and destroy the context, but not actually use it for anything. > So nopping out the ioctls should be good enough AFAICS. Or am I missing > something? > An old version of libdrm that still requires support needs them, it's the reason that David Airlie reverted the patch that Daniel did to remove the functions. Do they still need support, I don't know? David Airlie is on the cc list. A discussion was had and this was the way that it was suggested it be done. This seems a good half-way house, the actual security holes that have been found have been fixed and the functions (that seem to have a lot more security issues in them) are turned off for the drivers that don't use them, and if a driver does require them, it will be a one line change to reintroduce them. Are we carrying code we don't need to support, probably. Peter.
On Tue, Apr 28, 2015 at 01:29:21PM +0000, Antoine, Peter wrote: > On Tue, 2015-04-28 at 16:08 +0300, Ville Syrjälä wrote: > > On Tue, Apr 28, 2015 at 11:29:06AM +0000, Antoine, Peter wrote: > > > > > > 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@redhat.com> > > > > > > And the commit that the above patch reverts: > > > commit 7c510133d93dd6f15ca040733ba7b2891ed61fd1 > > > Author: Daniel Vetter <daniel.vetter@ffwll.ch> These two commits definitely should be referenced from the commit message, othwerise no one will understand the history. > > Looking at ancient libdrm sources makes me think nouveau just used to > > create and destroy the context, but not actually use it for anything. > > So nopping out the ioctls should be good enough AFAICS. Or am I missing > > something? > > > > An old version of libdrm that still requires support needs them, it's > the reason that David Airlie reverted the patch that Daniel did to > remove the functions. Do they still need support, I don't know? David > Airlie is on the cc list. > > A discussion was had and this was the way that it was suggested it be > done. This seems a good half-way house, the actual security holes that > have been found have been fixed and the functions (that seem to have a > lot more security issues in them) are turned off for the drivers that > don't use them, and if a driver does require them, it will be a one line > change to reintroduce them. Are we carrying code we don't need to > support, probably. Iirc it was in the ddx, and it was actually using the mmap code. Leftovers from ums, but unfortunately X crashes if we take them away. If I recall correctly nouveau was in staging still, but per Linus staging or not doesn't matter when distros are shipping with the code already. I did dig out the details way back out of curiosity, but lost them since then again. -Daniel
> > Iirc it was in the ddx, and it was actually using the mmap code. Leftovers > from ums, but unfortunately X crashes if we take them away. If I recall > correctly nouveau was in staging still, but per Linus staging or not > doesn't matter when distros are shipping with the code already. I did dig > out the details way back out of curiosity, but lost them since then again. > -Daniel There were two different nouveau problems a) old libdrm used contexts, it called drmCreateContext and drmDestroyContext from what I could see. b) old DDX used DRI1 functions, DRIOpenDRMMaster in the X server when it wanted to use open, that function added maps with locks and mapped them. I get the impression this is case (b). Dave.
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; 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, .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 /***********************************************************************/ /** \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;
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@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(-)