Re: [Intel-gfx] [PATCH] drm/i915/gem: Avoid iterating an empty list

[Matthew Auld]

On Fri, 22 May 2020 at 13:02, Chris Wilson  wrote:
>
> Quoting Chris Wilson (2020-05-22 11:42:07)
> > Our __sgt_iter assumes that the scattergather list has at least one
> > element. But during construction we may fail in allocating the first
> > page, and so mark the first element as the terminator. This is
> > unexpected!
> >
> > [22555.524752] RIP: 0010:shmem_get_pages+0x506/0x710 [i915]
> > [22555.524759] Code: 49 8b 2c 24 31 c0 66 89 44 24 40 48 85 ed 0f 84 62 01 
> > 00 00 4c 8b 75 00 8b 5d 08 44 8b 7d 0c 48 8b 0d 7e 34 07 e2 49 83 e6 fc 
> > <49> 8b 16 41 01 df 48 89 cf 48 89 d0 48 c1 e8 2d 48 85 c9 0f 84 c8
> > [22555.524765] RSP: 0018:c953f9d0 EFLAGS: 00010246
> > [22555.524770] RAX:  RBX:  RCX: 
> > 8881a000
> > [22555.524774] RDX: fff4 RSI:  RDI: 
> > 821efe00
> > [22555.524778] RBP: 8881b099ab00 R08:  R09: 
> > fff4
> > [22555.524782] R10: 0002 R11: ffec0a02 R12: 
> > 8881cd3c8d60
> > [22555.524786] R13: fff4 R14:  R15: 
> > 
> > [22555.524790] FS:  7f4fbeb9b9c0() GS:8881f858() 
> > knlGS:
> > [22555.524795] CS:  0010 DS:  ES:  CR0: 80050033
> > [22555.524799] CR2:  CR3: 0001ec7f0004 CR4: 
> > 001606e0
> > [22555.524803] Call Trace:
> > [22555.524919]  __i915_gem_object_get_pages+0x4f/0x60 [i915]
> >
>
> Fixes: 85d1225ec066 ("drm/i915: Introduce & use new lightweight SGL 
> iterators")
> > Signed-off-by: Chris Wilson 
> > Cc: Matthew Auld 
> > Cc: Tvrtko Ursulin 
> Cc:  # v4.8+
Reviewed-by: Matthew Auld 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915/gem: Avoid iterating an empty list

[Chris Wilson]

Quoting Chris Wilson (2020-05-22 11:42:07)
> Our __sgt_iter assumes that the scattergather list has at least one
> element. But during construction we may fail in allocating the first
> page, and so mark the first element as the terminator. This is
> unexpected!
> 
> [22555.524752] RIP: 0010:shmem_get_pages+0x506/0x710 [i915]
> [22555.524759] Code: 49 8b 2c 24 31 c0 66 89 44 24 40 48 85 ed 0f 84 62 01 00 
> 00 4c 8b 75 00 8b 5d 08 44 8b 7d 0c 48 8b 0d 7e 34 07 e2 49 83 e6 fc <49> 8b 
> 16 41 01 df 48 89 cf 48 89 d0 48 c1 e8 2d 48 85 c9 0f 84 c8
> [22555.524765] RSP: 0018:c953f9d0 EFLAGS: 00010246
> [22555.524770] RAX:  RBX:  RCX: 
> 8881a000
> [22555.524774] RDX: fff4 RSI:  RDI: 
> 821efe00
> [22555.524778] RBP: 8881b099ab00 R08:  R09: 
> fff4
> [22555.524782] R10: 0002 R11: ffec0a02 R12: 
> 8881cd3c8d60
> [22555.524786] R13: fff4 R14:  R15: 
> 
> [22555.524790] FS:  7f4fbeb9b9c0() GS:8881f858() 
> knlGS:
> [22555.524795] CS:  0010 DS:  ES:  CR0: 80050033
> [22555.524799] CR2:  CR3: 0001ec7f0004 CR4: 
> 001606e0
> [22555.524803] Call Trace:
> [22555.524919]  __i915_gem_object_get_pages+0x4f/0x60 [i915]
> 

Fixes: 85d1225ec066 ("drm/i915: Introduce & use new lightweight SGL iterators")
> Signed-off-by: Chris Wilson 
> Cc: Matthew Auld 
> Cc: Tvrtko Ursulin 
Cc:  # v4.8+

> ---
>  drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 15 +--
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c 
> b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> index 5d5d7eef3f43..7aff3514d97a 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> @@ -39,7 +39,6 @@ static int shmem_get_pages(struct drm_i915_gem_object *obj)
> unsigned long last_pfn = 0; /* suppress gcc warning */
> unsigned int max_segment = i915_sg_segment_size();
> unsigned int sg_page_sizes;
> -   struct pagevec pvec;
> gfp_t noreclaim;
> int ret;
>  
> @@ -192,13 +191,17 @@ static int shmem_get_pages(struct drm_i915_gem_object 
> *obj)
> sg_mark_end(sg);
>  err_pages:
> mapping_clear_unevictable(mapping);
> -   pagevec_init();
> -   for_each_sgt_page(page, sgt_iter, st) {
> -   if (!pagevec_add(, page))
> +   if (sg != st->sgl) {
> +   struct pagevec pvec;
> +
> +   pagevec_init();
> +   for_each_sgt_page(page, sgt_iter, st) {
> +   if (!pagevec_add(, page))
> +   check_release_pagevec();
> +   }
> +   if (pagevec_count())
> check_release_pagevec();
> }
> -   if (pagevec_count())
> -   check_release_pagevec();
> sg_free_table(st);
> kfree(st);
>  
> -- 
> 2.20.1
> 
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH] drm/i915/gem: Avoid iterating an empty list

[Chris Wilson]

Our __sgt_iter assumes that the scattergather list has at least one
element. But during construction we may fail in allocating the first
page, and so mark the first element as the terminator. This is
unexpected!

[22555.524752] RIP: 0010:shmem_get_pages+0x506/0x710 [i915]
[22555.524759] Code: 49 8b 2c 24 31 c0 66 89 44 24 40 48 85 ed 0f 84 62 01 00 
00 4c 8b 75 00 8b 5d 08 44 8b 7d 0c 48 8b 0d 7e 34 07 e2 49 83 e6 fc <49> 8b 16 
41 01 df 48 89 cf 48 89 d0 48 c1 e8 2d 48 85 c9 0f 84 c8
[22555.524765] RSP: 0018:c953f9d0 EFLAGS: 00010246
[22555.524770] RAX:  RBX:  RCX: 8881a000
[22555.524774] RDX: fff4 RSI:  RDI: 821efe00
[22555.524778] RBP: 8881b099ab00 R08:  R09: fff4
[22555.524782] R10: 0002 R11: ffec0a02 R12: 8881cd3c8d60
[22555.524786] R13: fff4 R14:  R15: 
[22555.524790] FS:  7f4fbeb9b9c0() GS:8881f858() 
knlGS:
[22555.524795] CS:  0010 DS:  ES:  CR0: 80050033
[22555.524799] CR2:  CR3: 0001ec7f0004 CR4: 001606e0
[22555.524803] Call Trace:
[22555.524919]  __i915_gem_object_get_pages+0x4f/0x60 [i915]

Signed-off-by: Chris Wilson 
Cc: Matthew Auld 
Cc: Tvrtko Ursulin 
---
 drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 15 +--
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c 
b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
index 5d5d7eef3f43..7aff3514d97a 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
@@ -39,7 +39,6 @@ static int shmem_get_pages(struct drm_i915_gem_object *obj)
unsigned long last_pfn = 0; /* suppress gcc warning */
unsigned int max_segment = i915_sg_segment_size();
unsigned int sg_page_sizes;
-   struct pagevec pvec;
gfp_t noreclaim;
int ret;
 
@@ -192,13 +191,17 @@ static int shmem_get_pages(struct drm_i915_gem_object 
*obj)
sg_mark_end(sg);
 err_pages:
mapping_clear_unevictable(mapping);
-   pagevec_init();
-   for_each_sgt_page(page, sgt_iter, st) {
-   if (!pagevec_add(, page))
+   if (sg != st->sgl) {
+   struct pagevec pvec;
+
+   pagevec_init();
+   for_each_sgt_page(page, sgt_iter, st) {
+   if (!pagevec_add(, page))
+   check_release_pagevec();
+   }
+   if (pagevec_count())
check_release_pagevec();
}
-   if (pagevec_count())
-   check_release_pagevec();
sg_free_table(st);
kfree(st);
 
-- 
2.20.1

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