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