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

From: Chris Wilson <chris@chris-wilson.co.uk>
To: intel-gfx@lists.freedesktop.org
Cc: Matthew Auld <matthew.auld@intel.com>
Subject: Re: [Intel-gfx] [PATCH] drm/i915/gem: Avoid iterating an empty list
Date: Fri, 22 May 2020 13:02:05 +0100	[thread overview]
Message-ID: <159014892558.32320.14917683830875571843@build.alporthouse.com> (raw)
In-Reply-To: <20200522104207.19439-1-chris@chris-wilson.co.uk>

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:ffffc9000053f9d0 EFLAGS: 00010246
> [22555.524770] RAX: 0000000000000000 RBX: 0000000000000000 RCX: ffff8881ffffa000
> [22555.524774] RDX: fffffffffffffff4 RSI: ffffffffffffffff RDI: ffffffff821efe00
> [22555.524778] RBP: ffff8881b099ab00 R08: 0000000000000000 R09: 00000000fffffff4
> [22555.524782] R10: 0000000000000002 R11: 00000000ffec0a02 R12: ffff8881cd3c8d60
> [22555.524786] R13: 00000000fffffff4 R14: 0000000000000000 R15: 0000000000000000
> [22555.524790] FS:  00007f4fbeb9b9c0(0000) GS:ffff8881f8580000(0000) knlGS:0000000000000000
> [22555.524795] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [22555.524799] CR2: 0000000000000000 CR3: 00000001ec7f0004 CR4: 00000000001606e0
> [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 <chris@chris-wilson.co.uk>
> Cc: Matthew Auld <matthew.auld@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: <stable@vger.kernel.org> # 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(&pvec);
> -       for_each_sgt_page(page, sgt_iter, st) {
> -               if (!pagevec_add(&pvec, page))
> +       if (sg != st->sgl) {
> +               struct pagevec pvec;
> +
> +               pagevec_init(&pvec);
> +               for_each_sgt_page(page, sgt_iter, st) {
> +                       if (!pagevec_add(&pvec, page))
> +                               check_release_pagevec(&pvec);
> +               }
> +               if (pagevec_count(&pvec))
>                         check_release_pagevec(&pvec);
>         }
> -       if (pagevec_count(&pvec))
> -               check_release_pagevec(&pvec);
>         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