On Tue, Jun 09, 2015 at 12:46:30PM +0300, Imre Deak wrote: > 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@xxxxxxxxxxxxxxxxxx> > > Cc: Imre Deak <imre.deak@xxxxxxxxx> > > Cc: stable@xxxxxxxxxxxxxxx > > --- > > 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(). sg_alloc_table() uses sg_mark_end(&sgl[st->orig_nents-1]); Hmm, we don't do page compression for swiotlb? Ok, then this part is redundant. > > > 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.. Nah, this was a change I made to try and reduce te patch size. Originally I did sg_set_page(); end = sg; but then thought I could avoid resetting end to the same sg in a few cases. Thanks, -Chris -- Chris Wilson, Intel Open Source Technology Centre -- To unsubscribe from this list: send the line "unsubscribe stable" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html