| Message ID | 1439279669-27058-1-git-send-email-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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 '*'
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.
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
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; }; /**
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(-)