All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/3] drm/i915: Catch abuse of I915_EXEC_GEN7_SOL_RESET
@ 2014-04-23 18:32 Daniel Vetter
  2014-04-23 18:32 ` [PATCH 2/3] drm/i915: Catch abuse of I915_EXEC_CONSTANTS_* Daniel Vetter
  2014-04-23 18:32 ` [PATCH 3/3] drm/i915: Catch dirt in unused execbuffer fields Daniel Vetter
  0 siblings, 2 replies; 9+ messages in thread
From: Daniel Vetter @ 2014-04-23 18:32 UTC (permalink / raw)
  To: Intel Graphics Development; +Cc: Daniel Vetter

Currently we catch it, but silently succeed. Our userspace is
better than this.

Testcase: igt/gem_exec_params/sol-reset-*
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 0ec8621eb4f8..40ae5ff0a031 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -982,7 +982,7 @@ i915_reset_gen7_sol_offsets(struct drm_device *dev,
 	int ret, i;
 
 	if (!IS_GEN7(dev) || ring != &dev_priv->ring[RCS])
-		return 0;
+		return -EINVAL;
 
 	ret = intel_ring_begin(ring, 4 * 3);
 	if (ret)
-- 
1.8.1.4

^ permalink raw reply related	[flat|nested] 9+ messages in thread
* [PATCH 2/3] drm/i915: Catch abuse of I915_EXEC_CONSTANTS_*
  2014-04-23 18:32 [PATCH 1/3] drm/i915: Catch abuse of I915_EXEC_GEN7_SOL_RESET Daniel Vetter
@ 2014-04-23 18:32 ` Daniel Vetter
  2014-04-23 18:32 ` [PATCH 3/3] drm/i915: Catch dirt in unused execbuffer fields Daniel Vetter
  1 sibling, 0 replies; 9+ messages in thread
From: Daniel Vetter @ 2014-04-23 18:32 UTC (permalink / raw)
  To: Intel Graphics Development; +Cc: Daniel Vetter

A bit tricky since 0 is also a valid constant ...

Testcase: igt/gem_exec_params/rel-constants-*
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 40ae5ff0a031..c2e5d39a1df8 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1058,8 +1058,10 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 	case I915_EXEC_CONSTANTS_REL_GENERAL:
 	case I915_EXEC_CONSTANTS_ABSOLUTE:
 	case I915_EXEC_CONSTANTS_REL_SURFACE:
-		if (ring == &dev_priv->ring[RCS] &&
-		    mode != dev_priv->relative_constants_mode) {
+		if (mode != 0 && ring != &dev_priv->ring[RCS])
+			return -EINVAL;
+
+		if (mode != dev_priv->relative_constants_mode) {
 			if (INTEL_INFO(dev)->gen < 4)
 				return -EINVAL;
 
-- 
1.8.1.4

^ permalink raw reply related	[flat|nested] 9+ messages in thread
* [PATCH 3/3] drm/i915: Catch dirt in unused execbuffer fields
  2014-04-23 18:32 [PATCH 1/3] drm/i915: Catch abuse of I915_EXEC_GEN7_SOL_RESET Daniel Vetter
  2014-04-23 18:32 ` [PATCH 2/3] drm/i915: Catch abuse of I915_EXEC_CONSTANTS_* Daniel Vetter
@ 2014-04-23 18:32 ` Daniel Vetter
  2014-04-24  6:19   ` Chris Wilson
  1 sibling, 1 reply; 9+ messages in thread
From: Daniel Vetter @ 2014-04-23 18:32 UTC (permalink / raw)
  To: Intel Graphics Development; +Cc: Daniel Vetter

We need to make sure that userspace keeps on following the contract,
otherwise we won't be able to use the reserved fields at all.

Testcase: igt/gem_exec_params/*-dirt
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index c2e5d39a1df8..0f0aebdd8dbd 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1115,6 +1115,9 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 			ret = -EFAULT;
 			goto pre_mutex_err;
 		}
+	} else {
+		if (args->DR1 || args->DR4 || args->cliprects_ptr)
+			return -EINVAL;
 	}
 
 	intel_runtime_pm_get(dev_priv);
@@ -1392,6 +1395,9 @@ i915_gem_execbuffer2(struct drm_device *dev, void *data,
 		return -EINVAL;
 	}
 
+	if (args->rsvd2 != 0)
+		return -EINVAL;
+
 	exec2_list = kmalloc(sizeof(*exec2_list)*args->buffer_count,
 			     GFP_TEMPORARY | __GFP_NOWARN | __GFP_NORETRY);
 	if (exec2_list == NULL)
-- 
1.8.1.4

^ permalink raw reply related	[flat|nested] 9+ messages in thread
* [PATCH 1/3] drm/i915: Catch abuse of I915_EXEC_GEN7_SOL_RESET
  2014-04-24  6:24     ` Chris Wilson
@ 2014-04-24  6:09       ` Daniel Vetter
  2014-04-24  6:09         ` [PATCH 2/3] drm/i915: Catch abuse of I915_EXEC_CONSTANTS_* Daniel Vetter
  2014-04-24  6:09         ` [PATCH 3/3] drm/i915: Catch dirt in unused execbuffer fields Daniel Vetter
  2014-04-24  7:12       ` Daniel Vetter
  1 sibling, 2 replies; 9+ messages in thread
From: Daniel Vetter @ 2014-04-24  6:09 UTC (permalink / raw)
  To: Intel Graphics Development; +Cc: Daniel Vetter

Currently we catch it, but silently succeed. Our userspace is
better than this.

v2: Add DRM_DEBUG (Chris)

Testcase: igt/gem_exec_params/sol-reset-*
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 0ec8621eb4f8..04102cc80ef4 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -981,8 +981,10 @@ i915_reset_gen7_sol_offsets(struct drm_device *dev,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int ret, i;
 
-	if (!IS_GEN7(dev) || ring != &dev_priv->ring[RCS])
-		return 0;
+	if (!IS_GEN7(dev) || ring != &dev_priv->ring[RCS]) {
+		DRM_DEBUG("sol reset is gen7/rcs only\n");
+		return -EINVAL;
+	}
 
 	ret = intel_ring_begin(ring, 4 * 3);
 	if (ret)
-- 
1.8.1.4

^ permalink raw reply related	[flat|nested] 9+ messages in thread
* [PATCH 2/3] drm/i915: Catch abuse of I915_EXEC_CONSTANTS_*
  2014-04-24  6:09       ` [PATCH 1/3] drm/i915: Catch abuse of I915_EXEC_GEN7_SOL_RESET Daniel Vetter
@ 2014-04-24  6:09         ` Daniel Vetter
  2014-04-24  6:09         ` [PATCH 3/3] drm/i915: Catch dirt in unused execbuffer fields Daniel Vetter
  1 sibling, 0 replies; 9+ messages in thread
From: Daniel Vetter @ 2014-04-24  6:09 UTC (permalink / raw)
  To: Intel Graphics Development; +Cc: Daniel Vetter

A bit tricky since 0 is also a valid constant ...

v2: Add DRM_DEBUG (Chris)

Testcase: igt/gem_exec_params/rel-constants-*
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 04102cc80ef4..c7ef65c3ae91 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1060,14 +1060,22 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 	case I915_EXEC_CONSTANTS_REL_GENERAL:
 	case I915_EXEC_CONSTANTS_ABSOLUTE:
 	case I915_EXEC_CONSTANTS_REL_SURFACE:
-		if (ring == &dev_priv->ring[RCS] &&
-		    mode != dev_priv->relative_constants_mode) {
-			if (INTEL_INFO(dev)->gen < 4)
+		if (mode != 0 && ring != &dev_priv->ring[RCS]) {
+			DRM_DEBUG("non-0 rel constants mode on non-RCS\n");
+			return -EINVAL;
+		}
+
+		if (mode != dev_priv->relative_constants_mode) {
+			if (INTEL_INFO(dev)->gen < 4) {
+				DRM_DEBUG("no rel constants on pre-gen4\n");
 				return -EINVAL;
+			}
 
 			if (INTEL_INFO(dev)->gen > 5 &&
-			    mode == I915_EXEC_CONSTANTS_REL_SURFACE)
+			    mode == I915_EXEC_CONSTANTS_REL_SURFACE) {
+				DRM_DEBUG("rel surface constants mode invalid on gen5+\n");
 				return -EINVAL;
+			}
 
 			/* The HW changed the meaning on this bit on gen6 */
 			if (INTEL_INFO(dev)->gen >= 6)
-- 
1.8.1.4

^ permalink raw reply related	[flat|nested] 9+ messages in thread
* [PATCH 3/3] drm/i915: Catch dirt in unused execbuffer fields
  2014-04-24  6:09       ` [PATCH 1/3] drm/i915: Catch abuse of I915_EXEC_GEN7_SOL_RESET Daniel Vetter
  2014-04-24  6:09         ` [PATCH 2/3] drm/i915: Catch abuse of I915_EXEC_CONSTANTS_* Daniel Vetter
@ 2014-04-24  6:09         ` Daniel Vetter
  1 sibling, 0 replies; 9+ messages in thread
From: Daniel Vetter @ 2014-04-24  6:09 UTC (permalink / raw)
  To: Intel Graphics Development; +Cc: Daniel Vetter

We need to make sure that userspace keeps on following the contract,
otherwise we won't be able to use the reserved fields at all.

v2: Add DRM_DEBUG (Chris)

Testcase: igt/gem_exec_params/*-dirt
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index c7ef65c3ae91..b9dcc2869edc 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1123,6 +1123,11 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 			ret = -EFAULT;
 			goto pre_mutex_err;
 		}
+	} else {
+		if (args->DR1 || args->DR4 || args->cliprects_ptr) {
+			DRM_DEBUG("0 cliprects but dirt in cliprects fields\n");
+			return -EINVAL;
+		}
 	}
 
 	intel_runtime_pm_get(dev_priv);
@@ -1400,6 +1405,11 @@ i915_gem_execbuffer2(struct drm_device *dev, void *data,
 		return -EINVAL;
 	}
 
+	if (args->rsvd2 != 0) {
+		DRM_DEBUG("dirty rvsd2 field\n");
+		return -EINVAL;
+	}
+
 	exec2_list = kmalloc(sizeof(*exec2_list)*args->buffer_count,
 			     GFP_TEMPORARY | __GFP_NOWARN | __GFP_NORETRY);
 	if (exec2_list == NULL)
-- 
1.8.1.4

^ permalink raw reply related	[flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] drm/i915: Catch dirt in unused execbuffer fields
  2014-04-23 18:32 ` [PATCH 3/3] drm/i915: Catch dirt in unused execbuffer fields Daniel Vetter
@ 2014-04-24  6:19   ` Chris Wilson
  2014-04-24  6:24     ` Chris Wilson
  0 siblings, 1 reply; 9+ messages in thread
From: Chris Wilson @ 2014-04-24  6:19 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: Intel Graphics Development

On Wed, Apr 23, 2014 at 08:32:20PM +0200, Daniel Vetter wrote:
> We need to make sure that userspace keeps on following the contract,
> otherwise we won't be able to use the reserved fields at all.
> 
> Testcase: igt/gem_exec_params/*-dirt
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index c2e5d39a1df8..0f0aebdd8dbd 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -1115,6 +1115,9 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>  			ret = -EFAULT;
>  			goto pre_mutex_err;
>  		}
> +	} else {
> +		if (args->DR1 || args->DR4 || args->cliprects_ptr)
> +			return -EINVAL;

We're inside a mutex here.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] drm/i915: Catch dirt in unused execbuffer fields
  2014-04-24  6:19   ` Chris Wilson
@ 2014-04-24  6:24     ` Chris Wilson
  2014-04-24  6:09       ` [PATCH 1/3] drm/i915: Catch abuse of I915_EXEC_GEN7_SOL_RESET Daniel Vetter
  2014-04-24  7:12       ` Daniel Vetter
  0 siblings, 2 replies; 9+ messages in thread
From: Chris Wilson @ 2014-04-24  6:24 UTC (permalink / raw)
  To: Daniel Vetter, Intel Graphics Development

On Thu, Apr 24, 2014 at 07:19:56AM +0100, Chris Wilson wrote:
> On Wed, Apr 23, 2014 at 08:32:20PM +0200, Daniel Vetter wrote:
> > We need to make sure that userspace keeps on following the contract,
> > otherwise we won't be able to use the reserved fields at all.
> > 
> > Testcase: igt/gem_exec_params/*-dirt
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > index c2e5d39a1df8..0f0aebdd8dbd 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > @@ -1115,6 +1115,9 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
> >  			ret = -EFAULT;
> >  			goto pre_mutex_err;
> >  		}
> > +	} else {
> > +		if (args->DR1 || args->DR4 || args->cliprects_ptr)
> > +			return -EINVAL;
> 
> We're inside a mutex here.

No, no we're not.

Fine, your series passes.
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

The only thing we perhaps should consider is a sprinkling of DRM_DEBUG()
to distinguish the EINVALs.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] drm/i915: Catch dirt in unused execbuffer fields
  2014-04-24  6:24     ` Chris Wilson
  2014-04-24  6:09       ` [PATCH 1/3] drm/i915: Catch abuse of I915_EXEC_GEN7_SOL_RESET Daniel Vetter
@ 2014-04-24  7:12       ` Daniel Vetter
  1 sibling, 0 replies; 9+ messages in thread
From: Daniel Vetter @ 2014-04-24  7:12 UTC (permalink / raw)
  To: Chris Wilson, Daniel Vetter, Intel Graphics Development

On Thu, Apr 24, 2014 at 8:24 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> The only thing we perhaps should consider is a sprinkling of DRM_DEBUG()
> to distinguish the EINVALs.

I was on the fence with that, but since you're suggesting it too I'll add it.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 9+ messages in thread
end of thread, other threads:[~2014-04-24  7:32 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-04-23 18:32 [PATCH 1/3] drm/i915: Catch abuse of I915_EXEC_GEN7_SOL_RESET Daniel Vetter
2014-04-23 18:32 ` [PATCH 2/3] drm/i915: Catch abuse of I915_EXEC_CONSTANTS_* Daniel Vetter
2014-04-23 18:32 ` [PATCH 3/3] drm/i915: Catch dirt in unused execbuffer fields Daniel Vetter
2014-04-24  6:19   ` Chris Wilson
2014-04-24  6:24     ` Chris Wilson
2014-04-24  6:09       ` [PATCH 1/3] drm/i915: Catch abuse of I915_EXEC_GEN7_SOL_RESET Daniel Vetter
2014-04-24  6:09         ` [PATCH 2/3] drm/i915: Catch abuse of I915_EXEC_CONSTANTS_* Daniel Vetter
2014-04-24  6:09         ` [PATCH 3/3] drm/i915: Catch dirt in unused execbuffer fields Daniel Vetter
2014-04-24  7:12       ` Daniel Vetter

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.