Attachment 24614 Details for Bug 20985 – this makes everything working for me again

[patch] this makes everything working for me again

0001-Fix-use-of-uninitialized-var.patch (text/plain), 7.01 KB, created by Florian Mickler on 2009-04-06 13:04:17 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Florian Mickler

Created: 2009-04-06 13:04:17 UTC

Size: 7.01 KB

patch

obsolete

>From 95d4c8702dbd0bbc06291105c3fbfe1927b13f2d Mon Sep 17 00:00:00 2001
>From: Florian Mickler <florian@mickler.org>
>Date: Mon, 6 Apr 2009 21:35:33 +0200
>Subject: [PATCH] Fix: use of uninitialized var
>
>i915_gem_put_relocs_to_user returned an uninitialized value which
>got returned to userspace. This caused libdrm in my setup to never
>get out of a do{}while() loop retrying i915_gem_execbuffer.
>
>result was hanging X, overheating of cpu and 2-3gb of logfile-spam.
>
>This patch adresses the issue by
> 1. initializing vars in this file where necessary
> 2. correcting wrongly interpreted return values of
> copy_[from/to]_user
>
> Nr. 2  helps libdrm from getting out of its loop and Nr. 1 helps
> i915_gem_execbuffer to not think there was an error.
>
>Signed-off-by: Florian Mickler <florian@mickler.org>
>---
> drivers/gpu/drm/i915/i915_gem.c |   63 +++++++++++++++++++++++++-------------
> 1 files changed, 41 insertions(+), 22 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>index 1449b45..99c01f5 100644
>--- a/drivers/gpu/drm/i915/i915_gem.c
>+++ b/drivers/gpu/drm/i915/i915_gem.c
>@@ -151,6 +151,7 @@ fast_shmem_read(struct page **pages,
> 	ret = __copy_to_user_inatomic(data, vaddr + page_offset, length);
> 	kunmap_atomic(vaddr, KM_USER0);
> 
>+	/* return a number of bytes */
> 	return ret;
> }
> 
>@@ -2976,7 +2977,7 @@ i915_gem_get_relocs_from_user(struct drm_i915_gem_exec_object *exec_list,
> 			      struct drm_i915_gem_relocation_entry **relocs)
> {
> 	uint32_t reloc_count = 0, reloc_index = 0, i;
>-	int ret;
>+	int ret = 0;
> 
> 	*relocs = NULL;
> 	for (i = 0; i < buffer_count; i++) {
>@@ -3002,13 +3003,14 @@ i915_gem_get_relocs_from_user(struct drm_i915_gem_exec_object *exec_list,
> 			drm_free(*relocs, reloc_count * sizeof(**relocs),
> 				 DRM_MEM_DRIVER);
> 			*relocs = NULL;
>-			return ret;
>+			return -EFAULT;
> 		}
> 
> 		reloc_index += exec_list[i].relocation_count;
> 	}
> 
>-	return ret;
>+	/* success */
>+	return 0;
> }
> 
> static int
>@@ -3017,14 +3019,14 @@ i915_gem_put_relocs_to_user(struct drm_i915_gem_exec_object *exec_list,
> 			    struct drm_i915_gem_relocation_entry *relocs)
> {
> 	uint32_t reloc_count = 0, i;
>-	int ret;
>+	int ret = 0;
> 
> 	for (i = 0; i < buffer_count; i++) {
> 		struct drm_i915_gem_relocation_entry __user *user_relocs;
> 
> 		user_relocs = (void __user *)(uintptr_t)exec_list[i].relocs_ptr;
> 
>-		if (ret == 0) {
>+		if ( ret == 0) {
> 			ret = copy_to_user(user_relocs,
> 					   &relocs[reloc_count],
> 					   exec_list[i].relocation_count *
>@@ -3036,6 +3038,12 @@ i915_gem_put_relocs_to_user(struct drm_i915_gem_exec_object *exec_list,
> 
> 	drm_free(relocs, reloc_count * sizeof(*relocs), DRM_MEM_DRIVER);
> 
>+	/* copy_to_user returns a number of bytes */
>+	if (ret != 0) {
>+		ret = -EFAULT;
>+	}
>+
>+	
> 	return ret;
> }
> 
>@@ -3052,11 +3060,14 @@ i915_gem_execbuffer(struct drm_device *dev, void *data,
> 	struct drm_i915_gem_object *obj_priv;
> 	struct drm_clip_rect *cliprects = NULL;
> 	struct drm_i915_gem_relocation_entry *relocs;
>-	int ret, ret2, i, pinned = 0;
>+	int ret, error, i, pinned;
> 	uint64_t exec_offset;
> 	uint32_t seqno, flush_domains, reloc_index;
> 	int pin_tries;
> 
>+	error = 0;
>+	pinned = 0;
>+
> #if WATCH_EXEC
> 	DRM_INFO("buffers_ptr %d buffer_count %d len %08x\n",
> 		  (int) args->buffers_ptr, args->buffer_count, args->batch_len);
>@@ -3075,7 +3086,7 @@ i915_gem_execbuffer(struct drm_device *dev, void *data,
> 		DRM_ERROR("Failed to allocate exec or object list "
> 			  "for %d buffers\n",
> 			  args->buffer_count);
>-		ret = -ENOMEM;
>+		error = -ENOMEM;
> 		goto pre_mutex_err;
> 	}
> 	ret = copy_from_user(exec_list,
>@@ -3085,6 +3096,7 @@ i915_gem_execbuffer(struct drm_device *dev, void *data,
> 	if (ret != 0) {
> 		DRM_ERROR("copy %d exec entries failed %d\n",
> 			  args->buffer_count, ret);
>+		error = ret;
> 		goto pre_mutex_err;
> 	}
> 
>@@ -3101,15 +3113,17 @@ i915_gem_execbuffer(struct drm_device *dev, void *data,
> 		if (ret != 0) {
> 			DRM_ERROR("copy %d cliprects failed: %d\n",
> 				  args->num_cliprects, ret);
>+			error = ret;
> 			goto pre_mutex_err;
> 		}
> 	}
> 
> 	ret = i915_gem_get_relocs_from_user(exec_list, args->buffer_count,
> 					    &relocs);
>-	if (ret != 0)
>+	if (ret != 0) {
>+		error = ret;
> 		goto pre_mutex_err;
>-
>+	}
> 	mutex_lock(&dev->struct_mutex);
> 
> 	i915_verify_inactive(dev, __FILE__, __LINE__);
>@@ -3117,14 +3131,14 @@ i915_gem_execbuffer(struct drm_device *dev, void *data,
> 	if (dev_priv->mm.wedged) {
> 		DRM_ERROR("Execbuf while wedged\n");
> 		mutex_unlock(&dev->struct_mutex);
>-		ret = -EIO;
>+		error = -EIO;
> 		goto pre_mutex_err;
> 	}
> 
> 	if (dev_priv->mm.suspended) {
> 		DRM_ERROR("Execbuf while VT-switched.\n");
> 		mutex_unlock(&dev->struct_mutex);
>-		ret = -EBUSY;
>+		error = -EBUSY;
> 		goto pre_mutex_err;
> 	}
> 
>@@ -3135,7 +3149,7 @@ i915_gem_execbuffer(struct drm_device *dev, void *data,
> 		if (object_list[i] == NULL) {
> 			DRM_ERROR("Invalid object handle %d at index %d\n",
> 				   exec_list[i].handle, i);
>-			ret = -EBADF;
>+			error = -EBADF;
> 			goto err;
> 		}
> 
>@@ -3143,7 +3157,7 @@ i915_gem_execbuffer(struct drm_device *dev, void *data,
> 		if (obj_priv->in_execbuffer) {
> 			DRM_ERROR("Object %p appears more than once in object list\n",
> 				   object_list[i]);
>-			ret = -EBADF;
>+			error = -EBADF;
> 			goto err;
> 		}
> 		obj_priv->in_execbuffer = true;
>@@ -3174,6 +3188,7 @@ i915_gem_execbuffer(struct drm_device *dev, void *data,
> 		if (ret != -ENOMEM || pin_tries >= 1) {
> 			if (ret != -ERESTARTSYS)
> 				DRM_ERROR("Failed to pin buffers %d\n", ret);
>+			error = ret;
> 			goto err;
> 		}
> 
>@@ -3184,8 +3199,10 @@ i915_gem_execbuffer(struct drm_device *dev, void *data,
> 
> 		/* evict everyone we can from the aperture */
> 		ret = i915_gem_evict_everything(dev);
>-		if (ret)
>+		if (ret){
>+			error = ret;
> 			goto err;
>+		}
> 	}
> 
> 	/* Set the pending read domains for the batch buffer to COMMAND */
>@@ -3253,6 +3270,7 @@ i915_gem_execbuffer(struct drm_device *dev, void *data,
> 	ret = i915_dispatch_gem_execbuffer(dev, args, cliprects, exec_offset);
> 	if (ret) {
> 		DRM_ERROR("dispatch failed %d\n", ret);
>+		error = ret;
> 		goto err;
> 	}
> 
>@@ -3308,10 +3326,12 @@ err:
> 				   (uintptr_t) args->buffers_ptr,
> 				   exec_list,
> 				   sizeof(*exec_list) * args->buffer_count);
>-		if (ret)
>+		if (ret) {
>+			error = -EFAULT;
> 			DRM_ERROR("failed to copy %d exec entries "
> 				  "back to user (%d)\n",
> 				  args->buffer_count, ret);
>+		}
> 	}
> 
> 	/* Copy the updated relocations out regardless of current error
>@@ -3319,13 +3339,12 @@ err:
> 	 * time userland calls execbuf, it would do so with presumed offset
> 	 * state that didn't match the actual object state.
> 	 */
>-	ret2 = i915_gem_put_relocs_to_user(exec_list, args->buffer_count,
>+	ret = i915_gem_put_relocs_to_user(exec_list, args->buffer_count,
> 					   relocs);
>-	if (ret2 != 0) {
>-		DRM_ERROR("Failed to copy relocations back out: %d\n", ret2);
>-
>-		if (ret == 0)
>-			ret = ret2;
>+	if (ret != 0) {
>+		DRM_ERROR("Failed to copy relocations back out\n");
>+		if (error == 0)
>+			error = ret;
> 	}
> 
> pre_mutex_err:
>@@ -3336,7 +3355,7 @@ pre_mutex_err:
> 	drm_free(cliprects, sizeof(*cliprects) * args->num_cliprects,
> 		 DRM_MEM_DRIVER);
> 
>-	return ret;
>+	return error;
> }
> 
> int
>-- 
>1.6.2
>

Actions: View | Splinter Review

Attachments on bug 20985: 24418 | 24419 | 24420 | 24440 | 24441 | 24442 | 24516 | 24597 | 24614

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.