* [PATCH] drm/i915: Mark the final obj->pages sg entry as last
@ 2015-06-09 9:19 Chris Wilson
2015-06-09 9:46 ` Imre Deak
0 siblings, 1 reply; 4+ messages in thread
From: Chris Wilson @ 2015-06-09 9:19 UTC (permalink / raw)
To: intel-gfx; +Cc: Chris Wilson, Imre Deak, stable
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);
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))
--
2.1.4
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] drm/i915: Mark the final obj->pages sg entry as last
2015-06-09 9:19 [PATCH] drm/i915: Mark the final obj->pages sg entry as last Chris Wilson
@ 2015-06-09 9:46 ` Imre Deak
2015-06-09 9:53 ` Chris Wilson
0 siblings, 1 reply; 4+ messages in thread
From: Imre Deak @ 2015-06-09 9:46 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx, stable
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))
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] drm/i915: Mark the final obj->pages sg entry as last
2015-06-09 9:46 ` Imre Deak
@ 2015-06-09 9:53 ` Chris Wilson
2015-06-09 10:06 ` [Intel-gfx] " Chris Wilson
0 siblings, 1 reply; 4+ messages in thread
From: Chris Wilson @ 2015-06-09 9:53 UTC (permalink / raw)
To: Imre Deak; +Cc: intel-gfx, stable
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, Intel Open Source Technology Centre
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [Intel-gfx] [PATCH] drm/i915: Mark the final obj->pages sg entry as last
2015-06-09 9:53 ` Chris Wilson
@ 2015-06-09 10:06 ` Chris Wilson
0 siblings, 0 replies; 4+ messages in thread
From: Chris Wilson @ 2015-06-09 10:06 UTC (permalink / raw)
To: Imre Deak, intel-gfx, stable
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
--
Chris Wilson, Intel Open Source Technology Centre
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2015-06-09 10:06 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-06-09 9:19 [PATCH] drm/i915: Mark the final obj->pages sg entry as last Chris Wilson
2015-06-09 9:46 ` Imre Deak
2015-06-09 9:53 ` Chris Wilson
2015-06-09 10:06 ` [Intel-gfx] " Chris Wilson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.