[bug report] drm/i915: Allow compaction upto SWIOTLB max segment size

[bug report] drm/i915: Allow compaction upto SWIOTLB max segment size

[Dan Carpenter]

Hello Chris Wilson,

The patch 871dfbd67d4e: "drm/i915: Allow compaction upto SWIOTLB max
segment size" from Oct 11, 2016, leads to the following static
checker warning:

	drivers/gpu/drm/i915/i915_gem.c:2357 i915_gem_object_get_pages_gtt()
	error: we previously assumed 'sg' could be null (see line 2341)

drivers/gpu/drm/i915/i915_gem.c
  2339                  /* Check that the i965g/gm workaround works. */
  2340                  WARN_ON((gfp & __GFP_DMA32) && (last_pfn >= 0x00100000UL));
  2341          }
  2342          if (sg) /* loop terminated early; short sg table */

We added a new check for NULL.

  2343                  sg_mark_end(sg);
  2344  
  2345          /* Trim unused sg entries to avoid wasting memory. */
  2346          i915_sg_trim(st);
  2347  
  2348          ret = i915_gem_gtt_prepare_pages(obj, st);
  2349          if (ret)
  2350                  goto err_pages;

but we hit this goto

  2351  
  2352          if (i915_gem_object_needs_bit17_swizzle(obj))
  2353                  i915_gem_object_do_bit_17_swizzle(obj, st);
  2354  
  2355          return st;
  2356  
  2357  err_pages:
  2358          sg_mark_end(sg);

Then don't check here.  Also do we really need to sg_mark_end() twice?

  2359          for_each_sgt_page(page, sgt_iter, st)
  2360                  put_page(page);
  2361          sg_free_table(st);
  2362          kfree(st);

regards,
dan carpenter
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [bug report] drm/i915: Allow compaction upto SWIOTLB max segment size

[Chris Wilson]

On Mon, Nov 14, 2016 at 02:14:56PM +0300, Dan Carpenter wrote:
> Hello Chris Wilson,
> 
> The patch 871dfbd67d4e: "drm/i915: Allow compaction upto SWIOTLB max
> segment size" from Oct 11, 2016, leads to the following static
> checker warning:
> 
> 	drivers/gpu/drm/i915/i915_gem.c:2357 i915_gem_object_get_pages_gtt()
> 	error: we previously assumed 'sg' could be null (see line 2341)
> 
> drivers/gpu/drm/i915/i915_gem.c
>   2339                  /* Check that the i965g/gm workaround works. */
>   2340                  WARN_ON((gfp & __GFP_DMA32) && (last_pfn >= 0x00100000UL));
>   2341          }
>   2342          if (sg) /* loop terminated early; short sg table */
> 
> We added a new check for NULL.
> 
>   2343                  sg_mark_end(sg);
>   2344  
>   2345          /* Trim unused sg entries to avoid wasting memory. */
>   2346          i915_sg_trim(st);
>   2347  
>   2348          ret = i915_gem_gtt_prepare_pages(obj, st);
>   2349          if (ret)
>   2350                  goto err_pages;
> 
> but we hit this goto
> 
>   2351  
>   2352          if (i915_gem_object_needs_bit17_swizzle(obj))
>   2353                  i915_gem_object_do_bit_17_swizzle(obj, st);
>   2354  
>   2355          return st;
>   2356  
>   2357  err_pages:
>   2358          sg_mark_end(sg);
> 
> Then don't check here.  Also do we really need to sg_mark_end() twice?

We need the mark_end when escaping from the loop, but after the loop, sg
may be NULL.

Archaelogy, suggests commit e227330223a7 ("drm/i915: avoid leaking DMA
mappings") as the original culprit (i.e. the introduction of goto
err_pages after the loop).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/i915: Don't touch NULL sg on i915_gem_object_get_pages_gtt() error

[Chris Wilson]

On the DMA mapping error path, sg may be NULL (it has already been
marked as the last scatterlist entry), and we should avoid dereferencing
it again.

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Fixes: e227330223a7 ("drm/i915: avoid leaking DMA mappings")
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 | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 888d7f79f36d..7c57ba9ed2ea 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2383,7 +2383,7 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
 			page = shmem_read_mapping_page(mapping, i);
 			if (IS_ERR(page)) {
 				ret = PTR_ERR(page);
-				goto err_pages;
+				goto err_sg;
 			}
 		}
 		if (!i ||
@@ -2417,8 +2417,9 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
 
 	return st;
 
-err_pages:
+err_sg:
 	sg_mark_end(sg);
+err_pages:
 	for_each_sgt_page(page, sgt_iter, st) {
 		set_page_private(page, 0);
 		put_page(page);
-- 
2.10.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Don't touch NULL sg on i915_gem_object_get_pages_gtt() error

[Matthew Auld]

On 14 November 2016 at 11:29, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On the DMA mapping error path, sg may be NULL (it has already been
> marked as the last scatterlist entry), and we should avoid dereferencing
> it again.
>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Fixes: e227330223a7 ("drm/i915: avoid leaking DMA mappings")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: stable@vger.kernel.org
Reviewed-by: Matthew Auld <matthew.auld@intel.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Don't touch NULL sg on i915_gem_object_get_pages_gtt() error

[Chris Wilson]

On Fri, Nov 18, 2016 at 05:19:58PM +0000, Matthew Auld wrote:
> On 14 November 2016 at 11:29, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > On the DMA mapping error path, sg may be NULL (it has already been
> > marked as the last scatterlist entry), and we should avoid dereferencing
> > it again.
> >
> > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> > Fixes: e227330223a7 ("drm/i915: avoid leaking DMA mappings")
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Imre Deak <imre.deak@intel.com>
> > Cc: stable@vger.kernel.org
> Reviewed-by: Matthew Auld <matthew.auld@intel.com>

Thanks for the review, pushed,
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre