drm/i915: Allow compaction upto SWIOTLB max segment size
Subsystems (3)
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
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoted 37 lines · 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. */
2340WARN_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);
Quoted 1 line
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
Signature · 5 lines
Chris Wilson, Intel Open Source Technology Centre _______________________________________________ Intel-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
PATCH 1/1 drm/i915: Don't touch NULL sg on i915_gem_object_get_pages_gtt() error
In reply to Dan CarpenterOn 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.
--- drivers/gpu/drm/i915/i915_gem.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/gpu/drm/i915/i915_gem.c +3 -2
@@ -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);
Signature · 6 lines
2.10.2 _______________________________________________ Intel-gfx mailing list [email protected] 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
Quoted 9 lines · 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 <[email protected]>Fixes e227330223a7 ("drm/i915: avoid leaking DMA mappings")Signed-off-by Chris Wilson <[email protected]>Cc Imre Deak <[email protected]>
_______________________________________________
Intel-gfx mailing list
[email protected]
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
Quoted 11 lines · Matthew Auld
Quoted 9 lines · 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 <[email protected]>Fixes e227330223a7 ("drm/i915: avoid leaking DMA mappings")Signed-off-by Chris Wilson <[email protected]>Cc Imre Deak <[email protected]>Reviewed-by Matthew Auld <[email protected]>
Thanks for the review, pushed,
-Chris
Signature · 1 line
Chris Wilson, Intel Open Source Technology Centre