Regression X Hangs at bootup -- PATCH

Regression X Hangs at bootup -- PATCH

[Florian Mickler]

[-- Attachment #1.1: Type: text/plain, Size: 670 bytes --]

[resent, because i got the lkml-adress wrong the first time]
Hi Eric!

To put this mail into context:
http://bugs.freedesktop.org/show_bug.cgi?id=20985

I finally poked a little bit at the code, since i figured you would be
glad if i came up with something.

I think I understood the problem and made a correct patch, but kernel
is new territory for me. So please doublecheck if i made the correct
choices for the error-returns.

Can you make shure that this patch (if acceptable) goes into mainline?

Sincerely,
Florian


p.s.: does somebody know where the actual implementation of
copy_[from/to]_user for my core2duo @64bit is? it is a mistery!


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: 0001-Fix-use-of-uninitialized-var.patch --]
[-- Type: text/x-patch, Size: 7432 bytes --]

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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: Regression X Hangs at bootup -- PATCH

[Eric Anholt]

[-- Attachment #1: Type: text/plain, Size: 4205 bytes --]

From bce4dab0061fa195360f3b88ce22c44895d3d197 Mon Sep 17 00:00:00 2001
From: Florian Mickler <florian@mickler.org>
Date: Mon, 6 Apr 2009 22:55:41 +0200
Subject: [PATCH] drm/i915: Fix use of uninitialized var in 40a5f0de

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

Signed-off-by: Florian Mickler <florian@mickler.org>
Signed-off-by: Eric Anholt <eric@anholt.net>
---

On Mon, 2009-04-06 at 22:55 +0200, Florian Mickler wrote:
> [resent, because i got the lkml-adress wrong the first time]
> Hi Eric!
> 
> To put this mail into context:
> http://bugs.freedesktop.org/show_bug.cgi?id=20985
> 
> I finally poked a little bit at the code, since i figured you would be
> glad if i came up with something.
> 
> I think I understood the problem and made a correct patch, but kernel
> is new territory for me. So please doublecheck if i made the correct
> choices for the error-returns.
> 
> Can you make shure that this patch (if acceptable) goes into mainline?

Nice catch!  Thanks.  I did some cleanup that brings it more in line
with style elsewhere in the code and cuts some of the gratuitous looking
changes.  Would you be OK with these changes rolled into your original
diff?

 drivers/gpu/drm/i915/i915_gem.c |   34 ++++++++++++++++++++++------------
 1 files changed, 22 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 33ab07b..6f7d0e2 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -141,15 +141,18 @@ fast_shmem_read(struct page **pages,
 		int length)
 {
 	char __iomem *vaddr;
-	int ret;
+	int unwritten;
 
 	vaddr = kmap_atomic(pages[page_base >> PAGE_SHIFT], KM_USER0);
 	if (vaddr == NULL)
 		return -ENOMEM;
-	ret = __copy_to_user_inatomic(data, vaddr + page_offset, length);
+	unwritten = __copy_to_user_inatomic(data, vaddr + page_offset, length);
 	kunmap_atomic(vaddr, KM_USER0);
 
-	return ret;
+	if (unwritten)
+		return -EFAULT;
+
+	return 0;
 }
 
 static inline int
@@ -3000,13 +3003,13 @@ 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;
+	return 0;
 }
 
 static int
@@ -3015,23 +3018,28 @@ 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;
+		int unwritten;
 
 		user_relocs = (void __user *)(uintptr_t)exec_list[i].relocs_ptr;
 
-		if (ret == 0) {
-			ret = copy_to_user(user_relocs,
-					   &relocs[reloc_count],
-					   exec_list[i].relocation_count *
-					   sizeof(*relocs));
+		unwritten = copy_to_user(user_relocs,
+					 &relocs[reloc_count],
+					 exec_list[i].relocation_count *
+					 sizeof(*relocs));
+
+		if (unwritten) {
+			ret = -EFAULT;
+			goto err;
 		}
 
 		reloc_count += exec_list[i].relocation_count;
 	}
 
+err:
 	drm_free(relocs, reloc_count * sizeof(*relocs), DRM_MEM_DRIVER);
 
 	return ret;
@@ -3306,10 +3314,12 @@ err:
 				   (uintptr_t) args->buffers_ptr,
 				   exec_list,
 				   sizeof(*exec_list) * args->buffer_count);
-		if (ret)
+		if (ret) {
+			ret = -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
-- 
1.6.2.1


-- 
Eric Anholt
eric@anholt.net                         eric.anholt@intel.com



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: Regression X Hangs at bootup -- PATCH

[Florian Mickler]

[-- Attachment #1: Type: text/plain, Size: 3399 bytes --]

On Mon, 06 Apr 2009 19:03:55 -0700
Eric Anholt <eric@anholt.net> wrote:

> Nice catch!  Thanks.  I did some cleanup that brings it more in line
> with style elsewhere in the code and cuts some of the gratuitous
> looking changes.  Would you be OK with these changes rolled into your
> original diff?

 i take it, you appended the endresult?

i'm ok with it, it's less invasive. but i think your
i915_gem_put_relocs part is wrong. (see below)


> 
>  drivers/gpu/drm/i915/i915_gem.c |   34
> ++++++++++++++++++++++------------ 1 files changed, 22 insertions(+),
> 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c
> b/drivers/gpu/drm/i915/i915_gem.c index 33ab07b..6f7d0e2 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -141,15 +141,18 @@ fast_shmem_read(struct page **pages,
>  		int length)
>  {
>  	char __iomem *vaddr;
> -	int ret;
> +	int unwritten;
>  
>  	vaddr = kmap_atomic(pages[page_base >> PAGE_SHIFT],
> KM_USER0); if (vaddr == NULL)
>  		return -ENOMEM;
> -	ret = __copy_to_user_inatomic(data, vaddr + page_offset,
> length);
> +	unwritten = __copy_to_user_inatomic(data, vaddr +
> page_offset, length); kunmap_atomic(vaddr, KM_USER0);
>  
> -	return ret;
> +	if (unwritten)
> +		return -EFAULT;
> +
> +	return 0;
>  }

yep thats ok.

>  
>  static inline int
> @@ -3000,13 +3003,13 @@ 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;
> +	return 0;
>  }
>  

right.

>  static int
> @@ -3015,23 +3018,28 @@ 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;
> +		int unwritten;
>  
>  		user_relocs = (void __user
> *)(uintptr_t)exec_list[i].relocs_ptr; 
> -		if (ret == 0) {
> -			ret = copy_to_user(user_relocs,
> -					   &relocs[reloc_count],
> -
> exec_list[i].relocation_count *
> -					   sizeof(*relocs));
> +		unwritten = copy_to_user(user_relocs,
> +					 &relocs[reloc_count],
> +
> exec_list[i].relocation_count *
> +					 sizeof(*relocs));
> +
> +		if (unwritten) {
> +			ret = -EFAULT;
> +			goto err;
>  		}
>  
>  		reloc_count += exec_list[i].relocation_count;
>  	}
>  

i wondered too at first about the if (ret == 0) part, but you need the
whole reloc_count to free everything in the next part:

> +err:
>  	drm_free(relocs, reloc_count * sizeof(*relocs),
> DRM_MEM_DRIVER); 
>  	return ret;


so i think, this would be a memleak in the error-case (if it ever
happens)


> @@ -3306,10 +3314,12 @@ err:
>  				   (uintptr_t) args->buffers_ptr,
>  				   exec_list,
>  				   sizeof(*exec_list) *
> args->buffer_count);
> -		if (ret)
> +		if (ret) {
> +			ret = -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





[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: Regression X Hangs at bootup -- PATCH

[Eric Anholt]

[-- Attachment #1: Type: text/plain, Size: 3496 bytes --]

On Tue, 2009-04-07 at 09:23 +0200, Florian Mickler wrote:
> On Mon, 06 Apr 2009 19:03:55 -0700
> Eric Anholt <eric@anholt.net> wrote:
> 
> > Nice catch!  Thanks.  I did some cleanup that brings it more in line
> > with style elsewhere in the code and cuts some of the gratuitous
> > looking changes.  Would you be OK with these changes rolled into your
> > original diff?
> 
>  i take it, you appended the endresult?
> 
> i'm ok with it, it's less invasive. but i think your
> i915_gem_put_relocs part is wrong. (see below)
> 
> 
> > 
> >  drivers/gpu/drm/i915/i915_gem.c |   34
> > ++++++++++++++++++++++------------ 1 files changed, 22 insertions(+),
> > 12 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c
> > b/drivers/gpu/drm/i915/i915_gem.c index 33ab07b..6f7d0e2 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -141,15 +141,18 @@ fast_shmem_read(struct page **pages,
> >  		int length)
> >  {
> >  	char __iomem *vaddr;
> > -	int ret;
> > +	int unwritten;
> >  
> >  	vaddr = kmap_atomic(pages[page_base >> PAGE_SHIFT],
> > KM_USER0); if (vaddr == NULL)
> >  		return -ENOMEM;
> > -	ret = __copy_to_user_inatomic(data, vaddr + page_offset,
> > length);
> > +	unwritten = __copy_to_user_inatomic(data, vaddr +
> > page_offset, length); kunmap_atomic(vaddr, KM_USER0);
> >  
> > -	return ret;
> > +	if (unwritten)
> > +		return -EFAULT;
> > +
> > +	return 0;
> >  }
> 
> yep thats ok.
> 
> >  
> >  static inline int
> > @@ -3000,13 +3003,13 @@ 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;
> > +	return 0;
> >  }
> >  
> 
> right.
> 
> >  static int
> > @@ -3015,23 +3018,28 @@ 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;
> > +		int unwritten;
> >  
> >  		user_relocs = (void __user
> > *)(uintptr_t)exec_list[i].relocs_ptr; 
> > -		if (ret == 0) {
> > -			ret = copy_to_user(user_relocs,
> > -					   &relocs[reloc_count],
> > -
> > exec_list[i].relocation_count *
> > -					   sizeof(*relocs));
> > +		unwritten = copy_to_user(user_relocs,
> > +					 &relocs[reloc_count],
> > +
> > exec_list[i].relocation_count *
> > +					 sizeof(*relocs));
> > +
> > +		if (unwritten) {
> > +			ret = -EFAULT;
> > +			goto err;
> >  		}
> >  
> >  		reloc_count += exec_list[i].relocation_count;
> >  	}
> >  
> 
> i wondered too at first about the if (ret == 0) part, but you need the
> whole reloc_count to free everything in the next part:
> 
> > +err:
> >  	drm_free(relocs, reloc_count * sizeof(*relocs),
> > DRM_MEM_DRIVER); 
> >  	return ret;
> 
> 
> so i think, this would be a memleak in the error-case (if it ever
> happens)

drm_free's other arguments are unused memory debug leftovers.  I've got
a patch I need to push at airlied to remove
drm_malloc/drm_calloc/drm_free.

-- 
Eric Anholt
eric@anholt.net                         eric.anholt@intel.com



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: Regression X Hangs at bootup -- PATCH

[Florian Mickler]

[-- Attachment #1: Type: text/plain, Size: 3690 bytes --]

On Tue, 07 Apr 2009 09:21:40 -0700
Eric Anholt <eric@anholt.net> wrote:


> drm_free's other arguments are unused memory debug leftovers.  I've
> got a patch I need to push at airlied to remove
> drm_malloc/drm_calloc/drm_free.
> 
in that case it is of course a non issue. but would you mind 
to add a note like 'this adds a memleak to i915_gem_put_relocs_to_user
which will be fixed in a followup patch', or just rebase it onto that
patch? 

anyhow, below patch is what i'm currently running.. if you rebase
the other version onto that patch of your's removing the
allocation thats fine too. if not, probably ok too. it's just that i
have too much time to care about this :)

sincerely,

Florian


p.s.: thx for doing this great work! 

From 868c938874d66aa3e507d8312cefc7730487f442 Mon Sep 17 00:00:00 2001
From: Florian Mickler <florian@mickler.org>
Date: Tue, 7 Apr 2009 18:41:32 +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 |   22 +++++++++++++++-------
 1 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c
b/drivers/gpu/drm/i915/i915_gem.c index 1449b45..e73844a 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -143,15 +143,18 @@ fast_shmem_read(struct page **pages,
 		int length)
 {
 	char __iomem *vaddr;
-	int ret;
+	int unwritten;
 
 	vaddr = kmap_atomic(pages[page_base >> PAGE_SHIFT], KM_USER0);
 	if (vaddr == NULL)
 		return -ENOMEM;
-	ret = __copy_to_user_inatomic(data, vaddr + page_offset,
length);
+	unwritten = __copy_to_user_inatomic(data, vaddr + page_offset,
length); kunmap_atomic(vaddr, KM_USER0);
 
-	return ret;
+	if (unwritten)
+		return -EFAULT;
+
+	return 0;
 }
 
 static inline int
@@ -3002,13 +3005,13 @@ 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;
+	return 0;
 }
 
 static int
@@ -3017,7 +3020,7 @@ 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; @@ -3036,6 +3039,9 @@ i915_gem_put_relocs_to_user(struct
drm_i915_gem_exec_object *exec_list, 
 	drm_free(relocs, reloc_count * sizeof(*relocs),
DRM_MEM_DRIVER); 
+	if (ret != 0)
+		ret = -EFAULT;
+
 	return ret;
 }
 
@@ -3308,10 +3314,12 @@ err:
 				   (uintptr_t) args->buffers_ptr,
 				   exec_list,
 				   sizeof(*exec_list) *
args->buffer_count);
-		if (ret)
+		if (ret) {
+			ret = -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
-- 
1.6.2


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: Regression X Hangs at bootup -- PATCH

[Eric Anholt]

[-- Attachment #1: Type: text/plain, Size: 1117 bytes --]

On Tue, 2009-04-07 at 22:14 +0200, Florian Mickler wrote:
> On Tue, 07 Apr 2009 09:21:40 -0700
> Eric Anholt <eric@anholt.net> wrote:
> 
> 
> > drm_free's other arguments are unused memory debug leftovers.  I've
> > got a patch I need to push at airlied to remove
> > drm_malloc/drm_calloc/drm_free.
> > 
> in that case it is of course a non issue. but would you mind 
> to add a note like 'this adds a memleak to i915_gem_put_relocs_to_user
> which will be fixed in a followup patch', or just rebase it onto that
> patch? 

Just to be clear, there is no memleak:

/** Wrapper around kfree() */
static __inline__ void drm_free(void *pt, size_t size, int area)
{
	kfree(pt);
}

The arg would only get used if DRM_DEBUG_MEMORY was set, but there's no
way in the kernel to do so.  I don't think anybody's used it in years,
and I'm sure there would be broken drm_free arguments since it's
untested.  It was never very useful even back in the day, since most
everything ended up lumped under DRM_MEM_DRIVER.

-- 
Eric Anholt
eric@anholt.net                         eric.anholt@intel.com



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: Regression X Hangs at bootup -- PATCH

[Florian Mickler]

[-- Attachment #1: Type: text/plain, Size: 1248 bytes --]

On Tue, 07 Apr 2009 19:33:59 -0700
Eric Anholt <eric@anholt.net> wrote:

> On Tue, 2009-04-07 at 22:14 +0200, Florian Mickler wrote:
> > On Tue, 07 Apr 2009 09:21:40 -0700
> > Eric Anholt <eric@anholt.net> wrote:
> > 
> > 
> > > drm_free's other arguments are unused memory debug leftovers.
> > > I've got a patch I need to push at airlied to remove
> > > drm_malloc/drm_calloc/drm_free.
> > > 
> > in that case it is of course a non issue. but would you mind 
> > to add a note like 'this adds a memleak to
> > i915_gem_put_relocs_to_user which will be fixed in a followup
> > patch', or just rebase it onto that patch? 
> 
> Just to be clear, there is no memleak:
> 
> /** Wrapper around kfree() */
> static __inline__ void drm_free(void *pt, size_t size, int area)
> {
> 	kfree(pt);
> }
> 
> The arg would only get used if DRM_DEBUG_MEMORY was set, but there's
> no way in the kernel to do so.  I don't think anybody's used it in
> years, and I'm sure there would be broken drm_free arguments since
> it's untested.  It was never very useful even back in the day, since
> most everything ended up lumped under DRM_MEM_DRIVER.
> 

wtf? alright :)
thx for clarifying. i should have just looked. 

bye,
Florian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 197 bytes --]