* [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
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Maarten Lankhorst @ 2015-08-11 7:54 UTC (permalink / raw)
To: dri-devel; +Cc: Dave Airlie, intel-gfx, stable
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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 6+ 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-14 13:33 ` shuang.he
2015-08-15 4:56 ` [Intel-gfx] " Dave Airlie
2 siblings, 1 reply; 6+ 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] 6+ 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; 6+ 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] 6+ messages in thread
* Re: [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-14 13:33 ` shuang.he
2015-08-15 4:56 ` [Intel-gfx] " Dave Airlie
2 siblings, 0 replies; 6+ messages in thread
From: shuang.he @ 2015-08-14 13:33 UTC (permalink / raw)
To: shuang.he, julianx.dumez, christophe.sureau, lei.a.liu, intel-gfx,
maarten.lankhorst
Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 7141
-------------------------------------Summary-------------------------------------
Platform Delta drm-intel-nightly Series Applied
ILK 302/302 302/302
SNB 315/315 315/315
IVB 336/336 336/336
BYT 283/283 283/283
HSW 378/378 378/378
-------------------------------------Detailed-------------------------------------
Platform Test drm-intel-nightly Series Applied
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 6+ 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-14 13:33 ` shuang.he
@ 2015-08-15 4:56 ` Dave Airlie
2015-08-15 19:12 ` Daniel Vetter
2 siblings, 1 reply; 6+ 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] 6+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/dp/mst: Remove port after removing connector.
2015-08-15 4:56 ` [Intel-gfx] " Dave Airlie
@ 2015-08-15 19:12 ` Daniel Vetter
0 siblings, 0 replies; 6+ messages in thread
From: Daniel Vetter @ 2015-08-15 19:12 UTC (permalink / raw)
To: Dave Airlie
Cc: Dave Airlie, intel-gfx@lists.freedesktop.org, dri-devel, stable
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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-08-15 19:12 UTC | newest]
Thread overview: 6+ 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-14 13:33 ` shuang.he
2015-08-15 4:56 ` [Intel-gfx] " 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