[Intel-gfx] [PATCH] drm/i915: check execbuffer for validity earlier to save on work.

[Owain G. Ainsworth]

Before, we were waiting until we knew the batch object's gtt offset
before we checked for validity. However, since this is purely an
alignment check (it is impossible for the batch object to get to
execbuffer stage without being pinned and bound) we can check alignment
before we do any other expensive work.

Additionally, check that batch_start_offset +  batch_len is <= the size
of the batchbuffer object. And that batch_len and batch_start_offset do
not overflow a u_int32_t.

Signed-off-by: Owain G. Ainsworth <oga at openbsd.org>
---
 drivers/gpu/drm/i915/i915_gem.c |   43 +++++++++++++++-----------------------
 1 files changed, 17 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index e52a277..d66b402 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3653,23 +3653,6 @@ err:
 	return ret;
 }
 
-static int
-i915_gem_check_execbuffer (struct drm_i915_gem_execbuffer2 *exec,
-			   uint64_t exec_offset)
-{
-	uint32_t exec_start, exec_len;
-
-	exec_start = (uint32_t) exec_offset + exec->batch_start_offset;
-	exec_len = (uint32_t) exec->batch_len;
-
-	if ((exec_start | exec_len) & 0x7)
-		return -EINVAL;
-
-	if (!exec_start)
-		return -EINVAL;
-
-	return 0;
-}
 
 static int
 i915_gem_wait_for_pending_flip(struct drm_device *dev,
@@ -3719,7 +3702,6 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 	struct drm_clip_rect *cliprects = NULL;
 	struct drm_i915_gem_relocation_entry *relocs = NULL;
 	int ret = 0, ret2, i, pinned = 0;
-	uint64_t exec_offset;
 	uint32_t seqno, flush_domains, reloc_index;
 	int pin_tries, flips;
 
@@ -3727,6 +3709,16 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 	DRM_INFO("buffers_ptr %d buffer_count %d len %08x\n",
 		  (int) args->buffers_ptr, args->buffer_count, args->batch_len);
 #endif
+	/*
+	 * check for valid execbuffer offset. We can do this early because
+	 * bound objects are always page aligned, so only the start offset
+	 * matters. Also check for overflow.
+	 */
+	if ((args->batch_start_offset | args->batch_len) & 0x7 ||
+	    args->batch_start_offset + args->batch_len < args->batch_len ||
+	    args->batch_start_offset + args->batch_len <
+	    args->batch_start_offset)
+		return -EINVAL;
 
 	if (args->buffer_count < 1) {
 		DRM_ERROR("execbuf with %d buffers\n", args->buffer_count);
@@ -3875,16 +3867,13 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 		ret = -EINVAL;
 		goto err;
 	}
-	batch_obj->pending_read_domains |= I915_GEM_DOMAIN_COMMAND;
-
-	/* Sanity check the batch buffer, prior to moving objects */
-	exec_offset = exec_list[args->buffer_count - 1].offset;
-	ret = i915_gem_check_execbuffer (args, exec_offset);
-	if (ret != 0) {
-		DRM_ERROR("execbuf with invalid offset/length\n");
+	if (args->batch_start_offset + args->batch_len > batch_obj->size) {
+		DRM_ERROR("batchbuffer shorter than program length\n");
+		ret = EINVAL;
 		goto err;
 	}
+	batch_obj->pending_read_domains |= I915_GEM_DOMAIN_COMMAND;
 
 	i915_verify_inactive(dev, __FILE__, __LINE__);
 
 	/* Zero the global flush/invalidate flags. These
@@ -3952,7 +3942,8 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 #endif
 
 	/* Exec the batchbuffer */
-	ret = i915_dispatch_gem_execbuffer(dev, args, cliprects, exec_offset);
+	ret = i915_dispatch_gem_execbuffer(dev, args, cliprects,
+	    exec_list[args->buffer_count - 1].offset);
 	if (ret) {
 		DRM_ERROR("dispatch failed %d\n", ret);
 		goto err;
-- 
1.6.5.7