stable.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] drm/dp/mst: Remove port after removing connector.
@ 2015-08-11  7:54 Maarten Lankhorst
  2015-08-11  8:55 ` [Intel-gfx] " Daniel Vetter
  2015-08-15  4:56 ` Dave Airlie
  0 siblings, 2 replies; 5+ messages in thread
From: Maarten Lankhorst @ 2015-08-11  7:54 UTC (permalink / raw)
  To: dri-devel; +Cc: intel-gfx, Maarten Lankhorst, stable, Dave Airlie

The port is removed synchronously, but the connector delayed.
This causes a use after free which can cause a kernel BUG with
slug_debug=FPZU. This is fixed by freeing the port after the
connector.

This fixes a regression introduced with
6b8eeca65b18ae77e175cc2b6571731f0ee413bf
"drm/dp/mst: close deadlock in connector destruction."

Cc: stable@vger.kernel.org
Cc: Dave Airlie <airlied@redhat.com>
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/drm_dp_mst_topology.c | 19 +++++++++++++------
 include/drm/drm_crtc.h                |  2 --
 2 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index b0487c9f018c..eb603f1defc2 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -873,9 +873,10 @@ static void drm_dp_destroy_port(struct kref *kref)
 		   from an EDID retrieval */
 		if (port->connector) {
 			mutex_lock(&mgr->destroy_connector_lock);
-			list_add(&port->connector->destroy_list, &mgr->destroy_connector_list);
+			list_add(&port->next, &mgr->destroy_connector_list);
 			mutex_unlock(&mgr->destroy_connector_lock);
 			schedule_work(&mgr->destroy_connector_work);
+			return;
 		}
 		drm_dp_port_teardown_pdt(port, port->pdt);
 
@@ -2659,7 +2660,7 @@ static void drm_dp_tx_work(struct work_struct *work)
 static void drm_dp_destroy_connector_work(struct work_struct *work)
 {
 	struct drm_dp_mst_topology_mgr *mgr = container_of(work, struct drm_dp_mst_topology_mgr, destroy_connector_work);
-	struct drm_connector *connector;
+	struct drm_dp_mst_port *port;
 
 	/*
 	 * Not a regular list traverse as we have to drop the destroy
@@ -2668,15 +2669,21 @@ static void drm_dp_destroy_connector_work(struct work_struct *work)
 	 */
 	for (;;) {
 		mutex_lock(&mgr->destroy_connector_lock);
-		connector = list_first_entry_or_null(&mgr->destroy_connector_list, struct drm_connector, destroy_list);
-		if (!connector) {
+		port = list_first_entry_or_null(&mgr->destroy_connector_list, struct drm_dp_mst_port, next);
+		if (!port) {
 			mutex_unlock(&mgr->destroy_connector_lock);
 			break;
 		}
-		list_del(&connector->destroy_list);
+		list_del(&port->next);
 		mutex_unlock(&mgr->destroy_connector_lock);
 
-		mgr->cbs->destroy_connector(mgr, connector);
+		mgr->cbs->destroy_connector(mgr, port->connector);
+
+		drm_dp_port_teardown_pdt(port, port->pdt);
+
+		if (!port->input && port->vcpi.vcpi > 0)
+			drm_dp_mst_put_payload_id(mgr, port->vcpi.vcpi);
+		kfree(port);
 	}
 }
 
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 574656965126..373b1bc6de96 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -745,8 +745,6 @@ struct drm_connector {
 	uint8_t num_h_tile, num_v_tile;
 	uint8_t tile_h_loc, tile_v_loc;
 	uint16_t tile_h_size, tile_v_size;
-
-	struct list_head destroy_list;
 };
 
 /**
-- 
2.1.0


^ permalink raw reply related	[flat|nested] 5+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/dp/mst: Remove port after removing connector.
  2015-08-11  7:54 [PATCH] drm/dp/mst: Remove port after removing connector Maarten Lankhorst
@ 2015-08-11  8:55 ` Daniel Vetter
  2015-08-11  9:31   ` Jani Nikula
  2015-08-15  4:56 ` Dave Airlie
  1 sibling, 1 reply; 5+ messages in thread
From: Daniel Vetter @ 2015-08-11  8:55 UTC (permalink / raw)
  To: Maarten Lankhorst; +Cc: dri-devel, Dave Airlie, intel-gfx, stable, Jani Nikula

On Tue, Aug 11, 2015 at 09:54:29AM +0200, Maarten Lankhorst wrote:
> The port is removed synchronously, but the connector delayed.
> This causes a use after free which can cause a kernel BUG with
> slug_debug=FPZU. This is fixed by freeing the port after the
> connector.
> 
> This fixes a regression introduced with
> 6b8eeca65b18ae77e175cc2b6571731f0ee413bf
> "drm/dp/mst: close deadlock in connector destruction."
> 
> Cc: stable@vger.kernel.org
> Cc: Dave Airlie <airlied@redhat.com>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Jani, can you please pick this up for topic/drm-fixes since Dave's still
on vacation this week?
-Daniel

> ---
>  drivers/gpu/drm/drm_dp_mst_topology.c | 19 +++++++++++++------
>  include/drm/drm_crtc.h                |  2 --
>  2 files changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
> index b0487c9f018c..eb603f1defc2 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -873,9 +873,10 @@ static void drm_dp_destroy_port(struct kref *kref)
>  		   from an EDID retrieval */
>  		if (port->connector) {
>  			mutex_lock(&mgr->destroy_connector_lock);
> -			list_add(&port->connector->destroy_list, &mgr->destroy_connector_list);
> +			list_add(&port->next, &mgr->destroy_connector_list);
>  			mutex_unlock(&mgr->destroy_connector_lock);
>  			schedule_work(&mgr->destroy_connector_work);
> +			return;
>  		}
>  		drm_dp_port_teardown_pdt(port, port->pdt);
>  
> @@ -2659,7 +2660,7 @@ static void drm_dp_tx_work(struct work_struct *work)
>  static void drm_dp_destroy_connector_work(struct work_struct *work)
>  {
>  	struct drm_dp_mst_topology_mgr *mgr = container_of(work, struct drm_dp_mst_topology_mgr, destroy_connector_work);
> -	struct drm_connector *connector;
> +	struct drm_dp_mst_port *port;
>  
>  	/*
>  	 * Not a regular list traverse as we have to drop the destroy
> @@ -2668,15 +2669,21 @@ static void drm_dp_destroy_connector_work(struct work_struct *work)
>  	 */
>  	for (;;) {
>  		mutex_lock(&mgr->destroy_connector_lock);
> -		connector = list_first_entry_or_null(&mgr->destroy_connector_list, struct drm_connector, destroy_list);
> -		if (!connector) {
> +		port = list_first_entry_or_null(&mgr->destroy_connector_list, struct drm_dp_mst_port, next);
> +		if (!port) {
>  			mutex_unlock(&mgr->destroy_connector_lock);
>  			break;
>  		}
> -		list_del(&connector->destroy_list);
> +		list_del(&port->next);
>  		mutex_unlock(&mgr->destroy_connector_lock);
>  
> -		mgr->cbs->destroy_connector(mgr, connector);
> +		mgr->cbs->destroy_connector(mgr, port->connector);
> +
> +		drm_dp_port_teardown_pdt(port, port->pdt);
> +
> +		if (!port->input && port->vcpi.vcpi > 0)
> +			drm_dp_mst_put_payload_id(mgr, port->vcpi.vcpi);
> +		kfree(port);
>  	}
>  }
>  
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 574656965126..373b1bc6de96 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -745,8 +745,6 @@ struct drm_connector {
>  	uint8_t num_h_tile, num_v_tile;
>  	uint8_t tile_h_loc, tile_v_loc;
>  	uint16_t tile_h_size, tile_v_size;
> -
> -	struct list_head destroy_list;
>  };
>  
>  /**
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/dp/mst: Remove port after removing connector.
  2015-08-11  8:55 ` [Intel-gfx] " Daniel Vetter
@ 2015-08-11  9:31   ` Jani Nikula
  0 siblings, 0 replies; 5+ messages in thread
From: Jani Nikula @ 2015-08-11  9:31 UTC (permalink / raw)
  To: Daniel Vetter, Maarten Lankhorst
  Cc: dri-devel, Dave Airlie, intel-gfx, stable

On Tue, 11 Aug 2015, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Tue, Aug 11, 2015 at 09:54:29AM +0200, Maarten Lankhorst wrote:
>> The port is removed synchronously, but the connector delayed.
>> This causes a use after free which can cause a kernel BUG with
>> slug_debug=FPZU. This is fixed by freeing the port after the
>> connector.
>> 
>> This fixes a regression introduced with
>> 6b8eeca65b18ae77e175cc2b6571731f0ee413bf
>> "drm/dp/mst: close deadlock in connector destruction."
>> 
>> Cc: stable@vger.kernel.org
>> Cc: Dave Airlie <airlied@redhat.com>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> Jani, can you please pick this up for topic/drm-fixes since Dave's still
> on vacation this week?

Done.

BR,
Jani.

> -Daniel
>
>> ---
>>  drivers/gpu/drm/drm_dp_mst_topology.c | 19 +++++++++++++------
>>  include/drm/drm_crtc.h                |  2 --
>>  2 files changed, 13 insertions(+), 8 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
>> index b0487c9f018c..eb603f1defc2 100644
>> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
>> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
>> @@ -873,9 +873,10 @@ static void drm_dp_destroy_port(struct kref *kref)
>>  		   from an EDID retrieval */
>>  		if (port->connector) {
>>  			mutex_lock(&mgr->destroy_connector_lock);
>> -			list_add(&port->connector->destroy_list, &mgr->destroy_connector_list);
>> +			list_add(&port->next, &mgr->destroy_connector_list);
>>  			mutex_unlock(&mgr->destroy_connector_lock);
>>  			schedule_work(&mgr->destroy_connector_work);
>> +			return;
>>  		}
>>  		drm_dp_port_teardown_pdt(port, port->pdt);
>>  
>> @@ -2659,7 +2660,7 @@ static void drm_dp_tx_work(struct work_struct *work)
>>  static void drm_dp_destroy_connector_work(struct work_struct *work)
>>  {
>>  	struct drm_dp_mst_topology_mgr *mgr = container_of(work, struct drm_dp_mst_topology_mgr, destroy_connector_work);
>> -	struct drm_connector *connector;
>> +	struct drm_dp_mst_port *port;
>>  
>>  	/*
>>  	 * Not a regular list traverse as we have to drop the destroy
>> @@ -2668,15 +2669,21 @@ static void drm_dp_destroy_connector_work(struct work_struct *work)
>>  	 */
>>  	for (;;) {
>>  		mutex_lock(&mgr->destroy_connector_lock);
>> -		connector = list_first_entry_or_null(&mgr->destroy_connector_list, struct drm_connector, destroy_list);
>> -		if (!connector) {
>> +		port = list_first_entry_or_null(&mgr->destroy_connector_list, struct drm_dp_mst_port, next);
>> +		if (!port) {
>>  			mutex_unlock(&mgr->destroy_connector_lock);
>>  			break;
>>  		}
>> -		list_del(&connector->destroy_list);
>> +		list_del(&port->next);
>>  		mutex_unlock(&mgr->destroy_connector_lock);
>>  
>> -		mgr->cbs->destroy_connector(mgr, connector);
>> +		mgr->cbs->destroy_connector(mgr, port->connector);
>> +
>> +		drm_dp_port_teardown_pdt(port, port->pdt);
>> +
>> +		if (!port->input && port->vcpi.vcpi > 0)
>> +			drm_dp_mst_put_payload_id(mgr, port->vcpi.vcpi);
>> +		kfree(port);
>>  	}
>>  }
>>  
>> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
>> index 574656965126..373b1bc6de96 100644
>> --- a/include/drm/drm_crtc.h
>> +++ b/include/drm/drm_crtc.h
>> @@ -745,8 +745,6 @@ struct drm_connector {
>>  	uint8_t num_h_tile, num_v_tile;
>>  	uint8_t tile_h_loc, tile_v_loc;
>>  	uint16_t tile_h_size, tile_v_size;
>> -
>> -	struct list_head destroy_list;
>>  };
>>  
>>  /**
>> -- 
>> 2.1.0
>> 
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Jani Nikula, Intel Open Source Technology Center

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/dp/mst: Remove port after removing connector.
  2015-08-11  7:54 [PATCH] drm/dp/mst: Remove port after removing connector Maarten Lankhorst
  2015-08-11  8:55 ` [Intel-gfx] " Daniel Vetter
@ 2015-08-15  4:56 ` Dave Airlie
  2015-08-15 19:12   ` Daniel Vetter
  1 sibling, 1 reply; 5+ messages in thread
From: Dave Airlie @ 2015-08-15  4:56 UTC (permalink / raw)
  To: Maarten Lankhorst
  Cc: dri-devel, Dave Airlie, intel-gfx@lists.freedesktop.org, stable

On 11 August 2015 at 17:54, Maarten Lankhorst
<maarten.lankhorst@linux.intel.com> wrote:
> The port is removed synchronously, but the connector delayed.
> This causes a use after free which can cause a kernel BUG with
> slug_debug=FPZU. This is fixed by freeing the port after the
> connector.

Where is the use after free btw? I'm not sure I like delaying the port
destruction, there should be no need to.

The connector->port pointer shouldn't be used without validation
anywhere, and if it is that is a bug.

I'd like to reproduce this before pulling this in.

Dave.

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/dp/mst: Remove port after removing connector.
  2015-08-15  4:56 ` Dave Airlie
@ 2015-08-15 19:12   ` Daniel Vetter
  0 siblings, 0 replies; 5+ messages in thread
From: Daniel Vetter @ 2015-08-15 19:12 UTC (permalink / raw)
  To: Dave Airlie
  Cc: Maarten Lankhorst, Dave Airlie, intel-gfx@lists.freedesktop.org,
	stable, dri-devel

On Sat, Aug 15, 2015 at 02:56:57PM +1000, Dave Airlie wrote:
> On 11 August 2015 at 17:54, Maarten Lankhorst
> <maarten.lankhorst@linux.intel.com> wrote:
> > The port is removed synchronously, but the connector delayed.
> > This causes a use after free which can cause a kernel BUG with
> > slug_debug=FPZU. This is fixed by freeing the port after the
> > connector.
> 
> Where is the use after free btw? I'm not sure I like delaying the port
> destruction, there should be no need to.
> 
> The connector->port pointer shouldn't be used without validation
> anywhere, and if it is that is a bug.
> 
> I'd like to reproduce this before pulling this in.

The remove function needs to lock at the connector->port to shut down the
dp mst link. Before your patch that was done _before_ the final kfree on
the port, but with your patch that's now the other way round: First we
synchronously kfree the port, then we call the driver's connector cleanup
function asynchronously. And that is very unhappy that the port is now
gone.

So perfectly ok regression fix imo to restore the ordering we had before
your patch in the cleanup code.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 5+ messages in thread
end of thread, other threads:[~2015-08-15 19:12 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-08-11  7:54 [PATCH] drm/dp/mst: Remove port after removing connector Maarten Lankhorst
2015-08-11  8:55 ` [Intel-gfx] " Daniel Vetter
2015-08-11  9:31   ` Jani Nikula
2015-08-15  4:56 ` Dave Airlie
2015-08-15 19:12   ` Daniel Vetter

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).