[Intel-gfx] [PATCH 4/5] drm: Make HW_LOCK access functions optional.
> > 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.
[Intel-gfx] [PATCH 4/5] drm: Make HW_LOCK access functions optional.
On Tue, Apr 28, 2015 at 01:29:21PM +, 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 +, 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_DMA0x20
> > > > > > -#define DRIVER_HAVE_IRQ0x40
> > > > > > -#define DRIVER_IRQ_SHARED 0x80
> > > > > > -#define DRIVER_GEM 0x1000
> > > > > > -#define DRIVER_MODESET 0x2000
> > > > > > -#define DRIVER_PRIME 0x4000
> > > > > > -#define DRIVER_RENDER 0x8000
> > > > > > -#define DRIVER_ATOMIC 0x1
> > > > > > +#define DRIVER_USE_AGP 0x1
> > > > > > +#define DRIVER_PCI_DMA 0x8
> > > > > > +#define DRIVER_SG 0x10
> > > > > > +#define DRIVER_HAVE_DMA0x20
> > > > > > +#define DRIVER_HAVE_IRQ0x40
> > > > > > +#define DRIVER_IRQ_SHARED 0x80
> > > > > > +#define DRIVER_GEM 0x1000
> > > > > > +#define DRIVER_MODESET 0x2000
> > > > > > +#define DRIVER_PRIME 0x4000
> > > > > > +#define DRIVER_RENDER 0x8000
> > > > > > +#define DRIVER_ATOMIC 0x1
> > > > > > +#define DRIVER_KMS_LEGACY_CONTEXT 0x2
> > > > >
> > > > > 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
> > >
> > > And the commit that the above patch reverts:
> > > commit 7c510133d93dd6f15ca040733ba7b2891ed61fd1
> > > Author: Daniel Vetter
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
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
[Intel-gfx] [PATCH 4/5] drm: Make HW_LOCK access functions optional.
On Tue, Apr 28, 2015 at 11:29:06AM +, 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_DMA0x20
> > > > -#define DRIVER_HAVE_IRQ0x40
> > > > -#define DRIVER_IRQ_SHARED 0x80
> > > > -#define DRIVER_GEM 0x1000
> > > > -#define DRIVER_MODESET 0x2000
> > > > -#define DRIVER_PRIME 0x4000
> > > > -#define DRIVER_RENDER 0x8000
> > > > -#define DRIVER_ATOMIC 0x1
> > > > +#define DRIVER_USE_AGP 0x1
> > > > +#define DRIVER_PCI_DMA 0x8
> > > > +#define DRIVER_SG 0x10
> > > > +#define DRIVER_HAVE_DMA0x20
> > > > +#define DRIVER_HAVE_IRQ0x40
> > > > +#define DRIVER_IRQ_SHARED 0x80
> > > > +#define DRIVER_GEM 0x1000
> > > > +#define DRIVER_MODESET 0x2000
> > > > +#define DRIVER_PRIME 0x4000
> > > > +#define DRIVER_RENDER 0x8000
> > > > +#define DRIVER_ATOMIC 0x1
> > > > +#define DRIVER_KMS_LEGACY_CONTEXT 0x2
> > >
> > > 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
>
> And the commit that the above patch reverts:
> commit 7c510133d93dd6f15ca040733ba7b2891ed61fd1
> Author: Daniel Vetter
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?
--
Ville Syrjälä
Intel OTC
[Intel-gfx] [PATCH 4/5] drm: Make HW_LOCK access functions optional.
On Tue, Apr 28, 2015 at 05:52:20AM +, 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
> > ---
> > 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_DMA0x20
> > -#define DRIVER_
[Intel-gfx] [PATCH 4/5] drm: Make HW_LOCK access functions optional.
On Tue, 2015-04-28 at 16:08 +0300, Ville Syrjälä wrote:
> On Tue, Apr 28, 2015 at 11:29:06AM +, 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_DMA0x20
> > > > > -#define DRIVER_HAVE_IRQ0x40
> > > > > -#define DRIVER_IRQ_SHARED 0x80
> > > > > -#define DRIVER_GEM 0x1000
> > > > > -#define DRIVER_MODESET 0x2000
> > > > > -#define DRIVER_PRIME 0x4000
> > > > > -#define DRIVER_RENDER 0x8000
> > > > > -#define DRIVER_ATOMIC 0x1
> > > > > +#define DRIVER_USE_AGP 0x1
> > > > > +#define DRIVER_PCI_DMA 0x8
> > > > > +#define DRIVER_SG0x10
> > > > > +#define DRIVER_HAVE_DMA 0x20
> > > > > +#define DRIVER_HAVE_IRQ 0x40
> > > > > +#define DRIVER_IRQ_SHARED0x80
> > > > > +#define DRIVER_GEM 0x1000
> > > > > +#define DRIVER_MODESET 0x2000
> > > > > +#define DRIVER_PRIME 0x4000
> > > > > +#define DRIVER_RENDER0x8000
> > > > > +#define DRIVER_ATOMIC0x1
> > > > > +#define DRIVER_KMS_LEGACY_CONTEXT0x2
> > > >
> > > > 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
> >
> > And the commit that the above patch reverts:
> > commit 7c510133d93dd6f15ca040733ba7b2891ed61fd1
> > Author: Daniel Vetter
>
> 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.
[Intel-gfx] [PATCH 4/5] drm: Make HW_LOCK access functions optional.
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 +, 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
> > > ---
> > > 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/i
[Intel-gfx] [PATCH 4/5] drm: Make HW_LOCK access functions optional.
Hi,
(replies inline)
-Original Message-
From: Ville Syrjälä [mailto:ville.syrj...@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
> ---
> 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_DMA0x20
> -#define DRIVER_HAVE_IRQ0x40
> -#define DRIVER_IRQ_SHARED 0x80
> -#define DRIVER_GEM 0x1000
> -#define DRIVER_MODESET 0x2000
> -#define DRIVER_PRIME 0x4000
> -#define DRIVER_RENDER 0x8000
> -#define DRIVER_ATOMIC 0x1
> +#define DRIVER_USE_AGP 0x1
> +#define DRIVER_PCI_DMA 0x8
> +#define DRIVER_SG0x10
> +#define DRIVER_HAVE_DMA 0x20
> +#define DRIVER_HAVE_IRQ 0x40
>
[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
> ---
> 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_DMA0x20
> -#define DRIVER_HAVE_IRQ0x40
> -#define DRIVER_IRQ_SHARED 0x80
> -#define DRIVER_GEM 0x1000
> -#define DRIVER_MODESET 0x2000
> -#define DRIVER_PRIME 0x4000
> -#define DRIVER_RENDER 0x8000
> -#define DRIVER_ATOMIC 0x1
> +#define DRIVER_USE_AGP 0x1
> +#define DRIVER_PCI_DMA 0x8
> +#define DRIVER_SG0x10
> +#define DRIVER_HAVE_DMA 0x20
> +#define DRIVER_HAVE_IRQ 0x40
> +#define DRIVER_IRQ_SHARED0x80
> +#define DRIVER_GEM 0x1000
> +#define DRIVER_MODESET 0x2000
> +#define DRIVER_PRIME 0x4000
> +#define DRIVER_RENDER0x8000
> +#define DRIVER_ATOMIC0x1
> +#define DRIVER_KMS_LEGACY_CONTEXT0x2
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
[PATCH 4/5] drm: Make HW_LOCK access functions optional.
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
---
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;
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_DMA0x20
-#define DRIVER_HAVE_IRQ0x40
-#define DRIVER_IRQ_SHARED 0x80
-#define DRIVER_GEM 0x1000
-#define DRIVER_MODESET 0x2000
-#define DRIVER_PRIME 0x4000
-#define DRIVER_RENDER 0x8000
-#define DRIVER_ATOMIC 0x1
+#define DRIVER_USE_AGP 0x1
+#define DRIVER_PCI_DMA 0x8
+#define DRIVER_SG 0x10
+#define DRIVER_HAVE_DMA0x20
+#define DRIVER_HAVE_IRQ0x40
+#define DRIVER_IRQ_SHARED 0x80
+#define DRIVER_GEM 0x1000
+#define DRIVER_MODESET 0x2000
+#define DRIVER_PRIME 0x4000
+#define DRIVER_RENDER 0x8000
+#define DRIVER_ATOMIC 0x1
+#define DRIVER_KMS_LEGACY_CONTEXT 0x2
/***/
/** \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
