Re: [Nouveau] [PATCH v2 20/27] drm/dp_mst: Protect drm_dp_mst_port members with connection_mutex
On Wed, 2019-09-25 at 16:00 -0400, Sean Paul wrote: > On Tue, Sep 03, 2019 at 04:45:58PM -0400, Lyude Paul wrote: > > Yes-you read that right. Currently there is literally no locking in > > place for any of the drm_dp_mst_port struct members that can be modified > > in response to a link address response, or a connection status response. > > Which literally means if we're unlucky enough to have any sort of > > hotplugging event happen before we're finished with reprobing link > > addresses, we'll race and the contents of said struct members becomes > > undefined. Fun! > > > > So, finally add some simple locking protections to our MST helpers by > > protecting any drm_dp_mst_port members which can be changed by link > > address responses or connection status notifications under > > drm_device->mode_config.connection_mutex. > > > > Cc: Juston Li > > Cc: Imre Deak > > Cc: Ville Syrjälä > > Cc: Harry Wentland > > Cc: Daniel Vetter > > Signed-off-by: Lyude Paul > > --- > > drivers/gpu/drm/drm_dp_mst_topology.c | 144 +++--- > > include/drm/drm_dp_mst_helper.h | 39 +-- > > 2 files changed, 133 insertions(+), 50 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c > > b/drivers/gpu/drm/drm_dp_mst_topology.c > > index 5101eeab4485..259634c5d6dc 100644 > > --- a/drivers/gpu/drm/drm_dp_mst_topology.c > > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c > > @@ -1354,6 +1354,7 @@ static void drm_dp_free_mst_port(struct kref *kref) > > container_of(kref, struct drm_dp_mst_port, malloc_kref); > > > > drm_dp_mst_put_mstb_malloc(port->parent); > > + mutex_destroy(&port->lock); > > kfree(port); > > } > > > > @@ -1906,6 +1907,36 @@ void drm_dp_mst_connector_early_unregister(struct > > drm_connector *connector, > > } > > EXPORT_SYMBOL(drm_dp_mst_connector_early_unregister); > > > > +static void > > +drm_dp_mst_port_add_connector(struct drm_dp_mst_branch *mstb, > > + struct drm_dp_mst_port *port) > > +{ > > + struct drm_dp_mst_topology_mgr *mgr = port->mgr; > > + char proppath[255]; > > + int ret; > > + > > + build_mst_prop_path(mstb, port->port_num, proppath, sizeof(proppath)); > > + port->connector = mgr->cbs->add_connector(mgr, port, proppath); > > + if (!port->connector) { > > + ret = -ENOMEM; > > + goto error; > > + } > > + > > + if ((port->pdt == DP_PEER_DEVICE_DP_LEGACY_CONV || > > +port->pdt == DP_PEER_DEVICE_SST_SINK) && > > + port->port_num >= DP_MST_LOGICAL_PORT_0) { > > + port->cached_edid = drm_get_edid(port->connector, > > +&port->aux.ddc); > > + drm_connector_set_tile_property(port->connector); > > + } > > + > > + mgr->cbs->register_connector(port->connector); > > + return; > > + > > +error: > > + DRM_ERROR("Failed to create connector for port %p: %d\n", port, ret); > > +} > > + > > static void > > drm_dp_mst_handle_link_address_port(struct drm_dp_mst_branch *mstb, > > struct drm_device *dev, > > @@ -1913,8 +1944,12 @@ drm_dp_mst_handle_link_address_port(struct > > drm_dp_mst_branch *mstb, > > { > > struct drm_dp_mst_topology_mgr *mgr = mstb->mgr; > > struct drm_dp_mst_port *port; > > - bool created = false; > > - int old_ddps = 0; > > + struct drm_dp_mst_branch *child_mstb = NULL; > > + struct drm_connector *connector_to_destroy = NULL; > > + int old_ddps = 0, ret; > > + u8 new_pdt = DP_PEER_DEVICE_NONE; > > + bool created = false, send_link_addr = false, > > +create_connector = false; > > > > port = drm_dp_get_port(mstb, port_msg->port_number); > > if (!port) { > > @@ -1923,6 +1958,7 @@ drm_dp_mst_handle_link_address_port(struct > > drm_dp_mst_branch *mstb, > > return; > > kref_init(&port->topology_kref); > > kref_init(&port->malloc_kref); > > + mutex_init(&port->lock); > > port->parent = mstb; > > port->port_num = port_msg->port_number; > > port->mgr = mgr; > > @@ -1937,11 +1973,17 @@ drm_dp_mst_handle_link_address_port(struct > > drm_dp_mst_branch *mstb, > > drm_dp_mst_get_mstb_malloc(mstb); > > > > created = true; > > - } else { > > - old_ddps = port->ddps; > > } > > > > + mutex_lock(&port->lock); > > + drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); > > + > > + if (!created) > > + old_ddps = port->ddps; > > + > > port->input = port_msg->input_port; > > + if (!port->input) > > + new_pdt = port_msg->peer_device_type; > > port->mcs = port_msg->mcs; > > port->ddps = port_msg->ddps; > > port->ldps = port_msg->legacy_device_plug_status; > > @@ -1969,44 +2011,58 @@ drm_dp_mst_handle_link_address_port(struct > > drm_dp_mst_branch *mstb, > > } > > } > > > > - if (!port->input) { > > - int ret = d
Re: [Nouveau] [PATCH v2 20/27] drm/dp_mst: Protect drm_dp_mst_port members with connection_mutex
On Tue, Sep 03, 2019 at 04:45:58PM -0400, Lyude Paul wrote: > Yes-you read that right. Currently there is literally no locking in > place for any of the drm_dp_mst_port struct members that can be modified > in response to a link address response, or a connection status response. > Which literally means if we're unlucky enough to have any sort of > hotplugging event happen before we're finished with reprobing link > addresses, we'll race and the contents of said struct members becomes > undefined. Fun! > > So, finally add some simple locking protections to our MST helpers by > protecting any drm_dp_mst_port members which can be changed by link > address responses or connection status notifications under > drm_device->mode_config.connection_mutex. > > Cc: Juston Li > Cc: Imre Deak > Cc: Ville Syrjälä > Cc: Harry Wentland > Cc: Daniel Vetter > Signed-off-by: Lyude Paul > --- > drivers/gpu/drm/drm_dp_mst_topology.c | 144 +++--- > include/drm/drm_dp_mst_helper.h | 39 +-- > 2 files changed, 133 insertions(+), 50 deletions(-) > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c > b/drivers/gpu/drm/drm_dp_mst_topology.c > index 5101eeab4485..259634c5d6dc 100644 > --- a/drivers/gpu/drm/drm_dp_mst_topology.c > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c > @@ -1354,6 +1354,7 @@ static void drm_dp_free_mst_port(struct kref *kref) > container_of(kref, struct drm_dp_mst_port, malloc_kref); > > drm_dp_mst_put_mstb_malloc(port->parent); > + mutex_destroy(&port->lock); > kfree(port); > } > > @@ -1906,6 +1907,36 @@ void drm_dp_mst_connector_early_unregister(struct > drm_connector *connector, > } > EXPORT_SYMBOL(drm_dp_mst_connector_early_unregister); > > +static void > +drm_dp_mst_port_add_connector(struct drm_dp_mst_branch *mstb, > + struct drm_dp_mst_port *port) > +{ > + struct drm_dp_mst_topology_mgr *mgr = port->mgr; > + char proppath[255]; > + int ret; > + > + build_mst_prop_path(mstb, port->port_num, proppath, sizeof(proppath)); > + port->connector = mgr->cbs->add_connector(mgr, port, proppath); > + if (!port->connector) { > + ret = -ENOMEM; > + goto error; > + } > + > + if ((port->pdt == DP_PEER_DEVICE_DP_LEGACY_CONV || > + port->pdt == DP_PEER_DEVICE_SST_SINK) && > + port->port_num >= DP_MST_LOGICAL_PORT_0) { > + port->cached_edid = drm_get_edid(port->connector, > + &port->aux.ddc); > + drm_connector_set_tile_property(port->connector); > + } > + > + mgr->cbs->register_connector(port->connector); > + return; > + > +error: > + DRM_ERROR("Failed to create connector for port %p: %d\n", port, ret); > +} > + > static void > drm_dp_mst_handle_link_address_port(struct drm_dp_mst_branch *mstb, > struct drm_device *dev, > @@ -1913,8 +1944,12 @@ drm_dp_mst_handle_link_address_port(struct > drm_dp_mst_branch *mstb, > { > struct drm_dp_mst_topology_mgr *mgr = mstb->mgr; > struct drm_dp_mst_port *port; > - bool created = false; > - int old_ddps = 0; > + struct drm_dp_mst_branch *child_mstb = NULL; > + struct drm_connector *connector_to_destroy = NULL; > + int old_ddps = 0, ret; > + u8 new_pdt = DP_PEER_DEVICE_NONE; > + bool created = false, send_link_addr = false, > + create_connector = false; > > port = drm_dp_get_port(mstb, port_msg->port_number); > if (!port) { > @@ -1923,6 +1958,7 @@ drm_dp_mst_handle_link_address_port(struct > drm_dp_mst_branch *mstb, > return; > kref_init(&port->topology_kref); > kref_init(&port->malloc_kref); > + mutex_init(&port->lock); > port->parent = mstb; > port->port_num = port_msg->port_number; > port->mgr = mgr; > @@ -1937,11 +1973,17 @@ drm_dp_mst_handle_link_address_port(struct > drm_dp_mst_branch *mstb, > drm_dp_mst_get_mstb_malloc(mstb); > > created = true; > - } else { > - old_ddps = port->ddps; > } > > + mutex_lock(&port->lock); > + drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); > + > + if (!created) > + old_ddps = port->ddps; > + > port->input = port_msg->input_port; > + if (!port->input) > + new_pdt = port_msg->peer_device_type; > port->mcs = port_msg->mcs; > port->ddps = port_msg->ddps; > port->ldps = port_msg->legacy_device_plug_status; > @@ -1969,44 +2011,58 @@ drm_dp_mst_handle_link_address_port(struct > drm_dp_mst_branch *mstb, > } > } > > - if (!port->input) { > - int ret = drm_dp_port_set_pdt(port, > - port_msg->peer_device_type); > - if (ret == 1) { > - drm_dp_send_link_addre
[Nouveau] [PATCH v2 20/27] drm/dp_mst: Protect drm_dp_mst_port members with connection_mutex
Yes-you read that right. Currently there is literally no locking in place for any of the drm_dp_mst_port struct members that can be modified in response to a link address response, or a connection status response. Which literally means if we're unlucky enough to have any sort of hotplugging event happen before we're finished with reprobing link addresses, we'll race and the contents of said struct members becomes undefined. Fun! So, finally add some simple locking protections to our MST helpers by protecting any drm_dp_mst_port members which can be changed by link address responses or connection status notifications under drm_device->mode_config.connection_mutex. Cc: Juston Li Cc: Imre Deak Cc: Ville Syrjälä Cc: Harry Wentland Cc: Daniel Vetter Signed-off-by: Lyude Paul --- drivers/gpu/drm/drm_dp_mst_topology.c | 144 +++--- include/drm/drm_dp_mst_helper.h | 39 +-- 2 files changed, 133 insertions(+), 50 deletions(-) diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 5101eeab4485..259634c5d6dc 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -1354,6 +1354,7 @@ static void drm_dp_free_mst_port(struct kref *kref) container_of(kref, struct drm_dp_mst_port, malloc_kref); drm_dp_mst_put_mstb_malloc(port->parent); + mutex_destroy(&port->lock); kfree(port); } @@ -1906,6 +1907,36 @@ void drm_dp_mst_connector_early_unregister(struct drm_connector *connector, } EXPORT_SYMBOL(drm_dp_mst_connector_early_unregister); +static void +drm_dp_mst_port_add_connector(struct drm_dp_mst_branch *mstb, + struct drm_dp_mst_port *port) +{ + struct drm_dp_mst_topology_mgr *mgr = port->mgr; + char proppath[255]; + int ret; + + build_mst_prop_path(mstb, port->port_num, proppath, sizeof(proppath)); + port->connector = mgr->cbs->add_connector(mgr, port, proppath); + if (!port->connector) { + ret = -ENOMEM; + goto error; + } + + if ((port->pdt == DP_PEER_DEVICE_DP_LEGACY_CONV || +port->pdt == DP_PEER_DEVICE_SST_SINK) && + port->port_num >= DP_MST_LOGICAL_PORT_0) { + port->cached_edid = drm_get_edid(port->connector, +&port->aux.ddc); + drm_connector_set_tile_property(port->connector); + } + + mgr->cbs->register_connector(port->connector); + return; + +error: + DRM_ERROR("Failed to create connector for port %p: %d\n", port, ret); +} + static void drm_dp_mst_handle_link_address_port(struct drm_dp_mst_branch *mstb, struct drm_device *dev, @@ -1913,8 +1944,12 @@ drm_dp_mst_handle_link_address_port(struct drm_dp_mst_branch *mstb, { struct drm_dp_mst_topology_mgr *mgr = mstb->mgr; struct drm_dp_mst_port *port; - bool created = false; - int old_ddps = 0; + struct drm_dp_mst_branch *child_mstb = NULL; + struct drm_connector *connector_to_destroy = NULL; + int old_ddps = 0, ret; + u8 new_pdt = DP_PEER_DEVICE_NONE; + bool created = false, send_link_addr = false, +create_connector = false; port = drm_dp_get_port(mstb, port_msg->port_number); if (!port) { @@ -1923,6 +1958,7 @@ drm_dp_mst_handle_link_address_port(struct drm_dp_mst_branch *mstb, return; kref_init(&port->topology_kref); kref_init(&port->malloc_kref); + mutex_init(&port->lock); port->parent = mstb; port->port_num = port_msg->port_number; port->mgr = mgr; @@ -1937,11 +1973,17 @@ drm_dp_mst_handle_link_address_port(struct drm_dp_mst_branch *mstb, drm_dp_mst_get_mstb_malloc(mstb); created = true; - } else { - old_ddps = port->ddps; } + mutex_lock(&port->lock); + drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); + + if (!created) + old_ddps = port->ddps; + port->input = port_msg->input_port; + if (!port->input) + new_pdt = port_msg->peer_device_type; port->mcs = port_msg->mcs; port->ddps = port_msg->ddps; port->ldps = port_msg->legacy_device_plug_status; @@ -1969,44 +2011,58 @@ drm_dp_mst_handle_link_address_port(struct drm_dp_mst_branch *mstb, } } - if (!port->input) { - int ret = drm_dp_port_set_pdt(port, - port_msg->peer_device_type); - if (ret == 1) { - drm_dp_send_link_address(mgr, port->mstb); - } else if (ret < 0) { - DRM_ERROR("Failed to change PDT on port %p: %d\n", - port, ret); -