[Intel-gfx] [PATCH] drm/i915: Avoid undefined behaviour of "u32 >> 32"

[Tvrtko Ursulin]

On 29/06/2017 14:49, Chris Wilson wrote:
> When computing a hash for looking up relcoation target handles in an
> execbuf, we start with a large size for the hashtable and proceed to
> reduce it until the allocation suceeds. The final attempt is with an
> order of 0 (i.e. a single element). This means that we then pass bits=0
> to hash_32() which then computes "hash >> (32 - 0)" to lookup the single
> element. Right shifting by a value the width of the operand is
> undefined, so limit the smallest hash table we use to order 1.
> 
> Fixes: 4ff4b44cbb70 ("drm/i915: Store a direct lookup from object handle to vma")
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> ---
>   drivers/gpu/drm/i915/i915_gem_execbuffer.c | 22 +++++++++++-----------
>   1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 718bb75ad387..54791bcb8ccb 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -296,12 +296,8 @@ static int eb_create(struct i915_execbuffer *eb)
>   				break;
>   		} while (--size);
>   
> -		if (unlikely(!eb->buckets)) {
> -			eb->buckets = kzalloc(sizeof(struct hlist_head),
> -					      GFP_TEMPORARY);

Want to maybe drop the NORETRY | NOWARN from the remaining allocation 
now? Wasn't it recently discussed that it is to feeble in it's attempts 
to allocate?

> -			if (unlikely(!eb->buckets))
> -				return -ENOMEM;
> -		}
> +		if (unlikely(!eb->buckets))
> +			return -ENOMEM;
>   
>   		eb->lut_size = size;
>   	} else {
> @@ -453,7 +449,7 @@ eb_add_vma(struct i915_execbuffer *eb,
>   			return err;
>   	}
>   
> -	if (eb->lut_size >= 0) {
> +	if (eb->lut_size > 0) {
>   		vma->exec_handle = entry->handle;
>   		hlist_add_head(&vma->exec_node,
>   			       &eb->buckets[hash_32(entry->handle,
> @@ -897,7 +893,7 @@ static void eb_release_vmas(const struct i915_execbuffer *eb)
>   static void eb_reset_vmas(const struct i915_execbuffer *eb)
>   {
>   	eb_release_vmas(eb);
> -	if (eb->lut_size >= 0)
> +	if (eb->lut_size > 0)
>   		memset(eb->buckets, 0,
>   		       sizeof(struct hlist_head) << eb->lut_size);
>   }
> @@ -906,7 +902,7 @@ static void eb_destroy(const struct i915_execbuffer *eb)
>   {
>   	GEM_BUG_ON(eb->reloc_cache.rq);
>   
> -	if (eb->lut_size >= 0)
> +	if (eb->lut_size > 0)
>   		kfree(eb->buckets);
>   }
>   
> @@ -2185,8 +2181,11 @@ i915_gem_do_execbuffer(struct drm_device *dev,
>   		}
>   	}
>   
> -	if (eb_create(&eb))
> -		return -ENOMEM;
> +	err = eb_create(&eb);
> +	if (err)
> +		goto err_out_fence;
> +
> +	GEM_BUG_ON(!eb.lut_size);
>   
>   	err = eb_select_context(&eb);
>   	if (unlikely(err))
> @@ -2346,6 +2345,7 @@ i915_gem_do_execbuffer(struct drm_device *dev,
>   	i915_gem_context_put(eb.ctx);
>   err_destroy:
>   	eb_destroy(&eb);
> +err_out_fence:
>   	if (out_fence_fd != -1)
>   		put_unused_fd(out_fence_fd);
>   err_in_fence:
> 

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>

Regards,

Tvrtko