Re: [PATCH] drm/i915: Mark the final obj->pages sg entry as last

[Imre Deak <imre.deak@intel.com>]

On ti, 2015-06-09 at 10:19 +0100, Chris Wilson wrote:
> Currently we may mark the subsequent sg entry as the last, instead of
> the actual last element we used. If a later iterator only used
> sg_is_last() (such as sg_next()) then we may access the NULL page stored
> in the elements beyond the contracted table. This may explain the
> occasional NULL dereference we see in insert pages, such as
> https://bugzilla.redhat.com/show_bug.cgi?id=1227892
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: stable@vger.kernel.org
> ---
>  drivers/gpu/drm/i915/i915_gem.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index be35f0486202..f3b66461dc68 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2195,7 +2195,7 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
>  	int page_count, i;
>  	struct address_space *mapping;
>  	struct sg_table *st;
> -	struct scatterlist *sg;
> +	struct scatterlist *sg, *end;
>  	struct sg_page_iter sg_iter;
>  	struct page *page;
>  	unsigned long last_pfn = 0;	/* suppress gcc warning */
> @@ -2227,7 +2227,7 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
>  	gfp = mapping_gfp_mask(mapping);
>  	gfp |= __GFP_NORETRY | __GFP_NOWARN | __GFP_NO_KSWAPD;
>  	gfp &= ~(__GFP_IO | __GFP_WAIT);
> -	sg = st->sgl;
> +	end = sg = st->sgl;
>  	st->nents = 0;
>  	for (i = 0; i < page_count; i++) {
>  		page = shmem_read_mapping_page_gfp(mapping, i, gfp);
> @@ -2253,13 +2253,13 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
>  		if (swiotlb_nr_tbl()) {
>  			st->nents++;
>  			sg_set_page(sg, page, PAGE_SIZE, 0);
> -			sg = sg_next(sg);
> +			sg = sg_next(end = sg);

This would make sense, but for swiotlb_nr_tbl() we don't mark the last
element, it's done by sg_alloc_table().

>  			continue;
>  		}
>  #endif
>  		if (!i || page_to_pfn(page) != last_pfn + 1) {
>  			if (i)
> -				sg = sg_next(sg);
> +				sg = sg_next(end = sg);

But this looks incorrect to me, sg points now to the last element for
which we set the page below and end points to one before the last. Or
I'm (still) missing something..

>  			st->nents++;
>  			sg_set_page(sg, page, PAGE_SIZE, 0);
>  		} else {
> @@ -2273,7 +2273,7 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
>  #ifdef CONFIG_SWIOTLB
>  	if (!swiotlb_nr_tbl())
>  #endif
> -		sg_mark_end(sg);
> +		sg_mark_end(end);
>  	obj->pages = st;
>  
>  	if (i915_gem_object_needs_bit17_swizzle(obj))