drm/i915: Mark the final obj->pages sg entry as last
diff mbox

Message ID 1433841566-1957-1-git-send-email-chris@chris-wilson.co.uk
State New
Headers show

Commit Message

Chris Wilson June 9, 2015, 9:19 a.m. UTC
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@chris-wilson.co.uk>
Cc: Imre Deak <imre.deak@intel.com>
Cc: stable@vger.kernel.org
---
 drivers/gpu/drm/i915/i915_gem.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

Comments

Imre Deak June 9, 2015, 9:46 a.m. UTC | #1
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@chris-wilson.co.uk>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: stable@vger.kernel.org
> ---
>  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().

>  			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..

>  			st->nents++;
>  			sg_set_page(sg, page, PAGE_SIZE, 0);
>  		} else {
> @@ -2273,7 +2273,7 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
>  #ifdef CONFIG_SWIOTLB
>  	if (!swiotlb_nr_tbl())
>  #endif
> -		sg_mark_end(sg);
> +		sg_mark_end(end);
>  	obj->pages = st;
>  
>  	if (i915_gem_object_needs_bit17_swizzle(obj))
Chris Wilson June 9, 2015, 9:53 a.m. UTC | #2
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@chris-wilson.co.uk>
> > Cc: Imre Deak <imre.deak@intel.com>
> > Cc: stable@vger.kernel.org
> > ---
> >  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 June 9, 2015, 10:06 a.m. UTC | #3
On Tue, Jun 09, 2015 at 10:53:23AM +0100, Chris Wilson wrote:
> > >  		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.

And without the complication of swiotlb_nr_tlb(), then sg after the loop
is pointing to the last set page anyway. The current code is correct,
sorry for the noise.
-Chris

Patch
diff mbox

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);
 			continue;
 		}
 #endif
 		if (!i || page_to_pfn(page) != last_pfn + 1) {
 			if (i)
-				sg = sg_next(sg);
+				sg = sg_next(end = sg);
 			st->nents++;
 			sg_set_page(sg, page, PAGE_SIZE, 0);
 		} else {
@@ -2273,7 +2273,7 @@  i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
 #ifdef CONFIG_SWIOTLB
 	if (!swiotlb_nr_tbl())
 #endif
-		sg_mark_end(sg);
+		sg_mark_end(end);
 	obj->pages = st;
 
 	if (i915_gem_object_needs_bit17_swizzle(obj))