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; };
/**
On Tue, Aug 11, 2015 at 09:54:29AM +0200, Maarten Lankhorst wrote:
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
On Tue, 11 Aug 2015, Daniel Vetter daniel@ffwll.ch wrote:
Done.
BR, Jani.
On 11 August 2015 at 17:54, Maarten Lankhorst maarten.lankhorst@linux.intel.com wrote:
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.
On Sat, Aug 15, 2015 at 02:56:57PM +1000, Dave Airlie wrote:
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
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Dave Airlie -
Jani Nikula -
Maarten Lankhorst