[Intel-gfx] [PATCH v7] drm/i915: Avoid writing relocs with addresses in non-canonical form

[Chris Wilson]

On Tue, Dec 29, 2015 at 04:49:01PM +0100, Michał Winiarski wrote:
> @@ -994,6 +1013,20 @@ validate_exec_list(struct drm_device *dev,
>  		if (exec[i].flags & invalid_flags)
>  			return -EINVAL;
>  
> +		/* Offset can be used as input (EXEC_OBJECT_PINNED), reject
> +		 * any non-page-aligned or non-canonial addresses.
> +		 */
> +		if (exec[i].flags & EXEC_OBJECT_PINNED &&
> +		    (exec[i].offset != gen8_canonical_addr(exec[i].offset) ||
> +		     offset_in_page(exec[i].offset)))
> +			return -EINVAL;
> +

if (exec[i].flags & EXEC_OBJECT_PINNED) {
	if (exec[i].offset != gen8_canonical_addr(exec[i].offset & PAGE_MASK))
		return -EINVAL;

	/* From drm_mm perspective address space is continuous,
	 * so from this point we're always using non-canonical form
	 * internally.
	 */
	exec[i].offset &= (1ULL << 48) - 1;
}

Splitting up the two tests just makes it a bit easier to read (imo, and
I've been told on numerous occasions to do the same :) Whilst not as
obvious atm, it also helps when we have multiple extension checks in the
validate(). As a secondary point, we can then also demonstate that we
can fully restrict manipulating exec[i].offset to the pinned path.

#define GEN8_HIGH_ADDRESS_BIT 47
#define GEN8_ADDRESS_MASK (1ULL << (GEN8_HIGH_ADDRESS_BIT+1)) - 1

GEN8_CANONICAL_HIGH_BIT ?

since we have two places now that know about the address format, or
perhaps

static u64 gen8_undo_canonical_addr(u64);
exec[i].offset = gen8_undo_canonical_addr(exec[i].offset);

so that we can put them next to each other. That seems a better idea.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre