[PATCH] drm/i915: Fix execlist cleanup bug

[PATCH] drm/i915: Fix execlist cleanup bug

[Dave Gordon]

From: John Harrison <John.C.Harrison@Intel.com>

Check whether each engine exists before trying to clean up the
corresponding logical ring.

Change-Id: I31b1ed941824db2d6bd7233360dbce05671979a8
Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c |   12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index cd74e5c..7a7d30a 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1444,13 +1444,17 @@ int intel_logical_rings_init(struct drm_device *dev)
 	return 0;
 
 cleanup_bsd2_ring:
-	intel_logical_ring_cleanup(&dev_priv->ring[VCS2]);
+	if (HAS_BSD2(dev))
+		intel_logical_ring_cleanup(&dev_priv->ring[VCS2]);
 cleanup_vebox_ring:
-	intel_logical_ring_cleanup(&dev_priv->ring[VECS]);
+	if (HAS_VEBOX(dev))
+		intel_logical_ring_cleanup(&dev_priv->ring[VECS]);
 cleanup_blt_ring:
-	intel_logical_ring_cleanup(&dev_priv->ring[BCS]);
+	if (HAS_BLT(dev))
+		intel_logical_ring_cleanup(&dev_priv->ring[BCS]);
 cleanup_bsd_ring:
-	intel_logical_ring_cleanup(&dev_priv->ring[VCS]);
+	if (HAS_BSD(dev))
+		intel_logical_ring_cleanup(&dev_priv->ring[VCS]);
 cleanup_render_ring:
 	intel_logical_ring_cleanup(&dev_priv->ring[RCS]);
 
-- 
1.7.9.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Fix execlist cleanup bug

[John Harrison]

This is an old version. I have a new and improved patch that also fixes 
the legacy ring buffer case (which was equally broken). Will post shortly...

On 30/10/2014 15:30, Dave Gordon wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
>
> Check whether each engine exists before trying to clean up the
> corresponding logical ring.
>
> Change-Id: I31b1ed941824db2d6bd7233360dbce05671979a8
> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_lrc.c |   12 ++++++++----
>   1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index cd74e5c..7a7d30a 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -1444,13 +1444,17 @@ int intel_logical_rings_init(struct drm_device *dev)
>   	return 0;
>   
>   cleanup_bsd2_ring:
> -	intel_logical_ring_cleanup(&dev_priv->ring[VCS2]);
> +	if (HAS_BSD2(dev))
> +		intel_logical_ring_cleanup(&dev_priv->ring[VCS2]);
>   cleanup_vebox_ring:
> -	intel_logical_ring_cleanup(&dev_priv->ring[VECS]);
> +	if (HAS_VEBOX(dev))
> +		intel_logical_ring_cleanup(&dev_priv->ring[VECS]);
>   cleanup_blt_ring:
> -	intel_logical_ring_cleanup(&dev_priv->ring[BCS]);
> +	if (HAS_BLT(dev))
> +		intel_logical_ring_cleanup(&dev_priv->ring[BCS]);
>   cleanup_bsd_ring:
> -	intel_logical_ring_cleanup(&dev_priv->ring[VCS]);
> +	if (HAS_BSD(dev))
> +		intel_logical_ring_cleanup(&dev_priv->ring[VCS]);
>   cleanup_render_ring:
>   	intel_logical_ring_cleanup(&dev_priv->ring[RCS]);
>   

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/i915: Fix null pointer dereference in ring cleanup code

[John.C.Harrison]

From: John Harrison <John.C.Harrison@Intel.com>

If a ring failed to initialise for any reason then the error path would try to
clean up all rings including those that had not yet been allocated. The ring
clean up code did a check that the ring was valid before starting its work.
Unfortunately, that was after it had already dereferenced the ring to obtain a
dev_private pointer.

Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c        |    4 +++-
 drivers/gpu/drm/i915/intel_ringbuffer.c |    7 +++++--
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index cd74e5c..76776fa 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1214,11 +1214,13 @@ static int gen8_emit_request(struct intel_ringbuffer *ringbuf)
  */
 void intel_logical_ring_cleanup(struct intel_engine_cs *ring)
 {
-	struct drm_i915_private *dev_priv = ring->dev->dev_private;
+	struct drm_i915_private *dev_priv;
 
 	if (!intel_ring_initialized(ring))
 		return;
 
+	dev_priv = ring->dev->dev_private;
+
 	intel_logical_ring_stop(ring);
 	WARN_ON((I915_READ_MODE(ring) & MODE_IDLE) == 0);
 	ring->preallocated_lazy_request = NULL;
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index a8f72e8..f457146 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -1845,12 +1845,15 @@ error:
 
 void intel_cleanup_ring_buffer(struct intel_engine_cs *ring)
 {
-	struct drm_i915_private *dev_priv = to_i915(ring->dev);
-	struct intel_ringbuffer *ringbuf = ring->buffer;
+	struct drm_i915_private *dev_priv;
+	struct intel_ringbuffer *ringbuf;
 
 	if (!intel_ring_initialized(ring))
 		return;
 
+	dev_priv = to_i915(ring->dev);
+	ringbuf = ring->buffer;
+
 	intel_stop_ring_buffer(ring);
 	WARN_ON(!IS_GEN2(ring->dev) && (I915_READ_MODE(ring) & MODE_IDLE) == 0);
 
-- 
1.7.9.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Fix null pointer dereference in ring cleanup code

[Damien Lespiau]

On Fri, Oct 31, 2014 at 12:00:26PM +0000, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
> 
> If a ring failed to initialise for any reason then the error path would try to
> clean up all rings including those that had not yet been allocated. The ring
> clean up code did a check that the ring was valid before starting its work.
> Unfortunately, that was after it had already dereferenced the ring to obtain a
> dev_private pointer.
> 
> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>

This looks good to me.

Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>


-- 
Damien

> ---
>  drivers/gpu/drm/i915/intel_lrc.c        |    4 +++-
>  drivers/gpu/drm/i915/intel_ringbuffer.c |    7 +++++--
>  2 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index cd74e5c..76776fa 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -1214,11 +1214,13 @@ static int gen8_emit_request(struct intel_ringbuffer *ringbuf)
>   */
>  void intel_logical_ring_cleanup(struct intel_engine_cs *ring)
>  {
> -	struct drm_i915_private *dev_priv = ring->dev->dev_private;
> +	struct drm_i915_private *dev_priv;
>  
>  	if (!intel_ring_initialized(ring))
>  		return;
>  
> +	dev_priv = ring->dev->dev_private;
> +
>  	intel_logical_ring_stop(ring);
>  	WARN_ON((I915_READ_MODE(ring) & MODE_IDLE) == 0);
>  	ring->preallocated_lazy_request = NULL;
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index a8f72e8..f457146 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -1845,12 +1845,15 @@ error:
>  
>  void intel_cleanup_ring_buffer(struct intel_engine_cs *ring)
>  {
> -	struct drm_i915_private *dev_priv = to_i915(ring->dev);
> -	struct intel_ringbuffer *ringbuf = ring->buffer;
> +	struct drm_i915_private *dev_priv;
> +	struct intel_ringbuffer *ringbuf;
>  
>  	if (!intel_ring_initialized(ring))
>  		return;
>  
> +	dev_priv = to_i915(ring->dev);
> +	ringbuf = ring->buffer;
> +
>  	intel_stop_ring_buffer(ring);
>  	WARN_ON(!IS_GEN2(ring->dev) && (I915_READ_MODE(ring) & MODE_IDLE) == 0);
>  
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Fix null pointer dereference in ring cleanup code

[Chris Wilson]

On Fri, Oct 31, 2014 at 02:52:40PM +0000, Damien Lespiau wrote:
> On Fri, Oct 31, 2014 at 12:00:26PM +0000, John.C.Harrison@Intel.com wrote:
> > From: John Harrison <John.C.Harrison@Intel.com>
> > 
> > If a ring failed to initialise for any reason then the error path would try to
> > clean up all rings including those that had not yet been allocated. The ring
> > clean up code did a check that the ring was valid before starting its work.
> > Unfortunately, that was after it had already dereferenced the ring to obtain a
> > dev_private pointer.
> > 
> > Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
> 
> This looks good to me.

Really? These functions(!!!) are only called under controlled conditions...
I would have been happy to see this follow my suggestion I made to fix
this bug months ago.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Fix null pointer dereference in ring cleanup code

[Daniel Vetter]

On Fri, Oct 31, 2014 at 04:07:35PM +0000, Chris Wilson wrote:
> On Fri, Oct 31, 2014 at 02:52:40PM +0000, Damien Lespiau wrote:
> > On Fri, Oct 31, 2014 at 12:00:26PM +0000, John.C.Harrison@Intel.com wrote:
> > > From: John Harrison <John.C.Harrison@Intel.com>
> > > 
> > > If a ring failed to initialise for any reason then the error path would try to
> > > clean up all rings including those that had not yet been allocated. The ring
> > > clean up code did a check that the ring was valid before starting its work.
> > > Unfortunately, that was after it had already dereferenced the ring to obtain a
> > > dev_private pointer.
> > > 
> > > Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
> > 
> > This looks good to me.
> 
> Really? These functions(!!!) are only called under controlled conditions...
> I would have been happy to see this follow my suggestion I made to fix
> this bug months ago.

Hm, do you mean to shuffle the ring_initialized checks into callers? Or
something else?

John's patch didn't look offensive really, so merged it for now.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Fix null pointer dereference in ring cleanup code

[Dave Gordon]

On 31/10/14 14:52, Damien Lespiau wrote:
> On Fri, Oct 31, 2014 at 12:00:26PM +0000, John.C.Harrison@Intel.com wrote:
>> From: John Harrison <John.C.Harrison@Intel.com>
>>
>> If a ring failed to initialise for any reason then the error path would try to
>> clean up all rings including those that had not yet been allocated. The ring
>> clean up code did a check that the ring was valid before starting its work.
>> Unfortunately, that was after it had already dereferenced the ring to obtain a
>> dev_private pointer.
>>
>> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
> 
> This looks good to me.
> 
> Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

And simpler than the version I previously posted, as that would have
had to have another test added for each new ring in future hardware.
However I think the description above is slightly misleading, as the
problem wasn't dereferencing "ring" but "ring->dev". "ring" is always
non-NULL (it's the address of a member of an array inside dev_priv),
but the backpointer "ring->dev" is only filled in during ring
initialisation.

.Dave.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Fix null pointer dereference in ring cleanup code

[Chris Wilson]

On Mon, Nov 03, 2014 at 01:54:00PM +0100, Daniel Vetter wrote:
> On Fri, Oct 31, 2014 at 04:07:35PM +0000, Chris Wilson wrote:
> > On Fri, Oct 31, 2014 at 02:52:40PM +0000, Damien Lespiau wrote:
> > > On Fri, Oct 31, 2014 at 12:00:26PM +0000, John.C.Harrison@Intel.com wrote:
> > > > From: John Harrison <John.C.Harrison@Intel.com>
> > > > 
> > > > If a ring failed to initialise for any reason then the error path would try to
> > > > clean up all rings including those that had not yet been allocated. The ring
> > > > clean up code did a check that the ring was valid before starting its work.
> > > > Unfortunately, that was after it had already dereferenced the ring to obtain a
> > > > dev_private pointer.
> > > > 
> > > > Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
> > > 
> > > This looks good to me.
> > 
> > Really? These functions(!!!) are only called under controlled conditions...
> > I would have been happy to see this follow my suggestion I made to fix
> > this bug months ago.
> 
> Hm, do you mean to shuffle the ring_initialized checks into callers? Or
> something else?


i915_gem_cleanup_engines()
{
	/* Not the regular for_each_engine so we can cleanup a failed setup */
	for (int i =0 ; i < I915_NUM_ENGINES; i++)
		intel_engine_cleanup(&to_i915(dev)->engine[i]);
}

And that is callable then from an incomplete setup as well as normal
teardown.

And intel_engine_cleanup() need just be:
void intel_engine_cleanup(struct intel_engine_cs *engine)
{
	WARN_ON(engine->last_request);

	if (engine->cleanup)
		engine->cleanup(engine);
}

Remove bugs by removing code, win.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx