Message-ID: <20190925200020.GL218215@art_vandelay>
Date:   Wed, 25 Sep 2019 16:00:20 -0400
From:   Sean Paul <sean@...rly.run>
To:     Lyude Paul <lyude@...hat.com>
Cc:     dri-devel@...ts.freedesktop.org, nouveau@...ts.freedesktop.org,
        amd-gfx@...ts.freedesktop.org, Juston Li <juston.li@...el.com>,
        Imre Deak <imre.deak@...el.com>,
        Ville Syrjälä 
        <ville.syrjala@...ux.intel.com>, Harry Wentland <hwentlan@....com>,
        Daniel Vetter <daniel.vetter@...ll.ch>,
        Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Maxime Ripard <mripard@...nel.org>,
        Sean Paul <sean@...rly.run>, David Airlie <airlied@...ux.ie>,
        Daniel Vetter <daniel@...ll.ch>, linux-kernel@...r.kernel.org
Subject: Re: [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 <juston.li@...el.com>
> Cc: Imre Deak <imre.deak@...el.com>
> Cc: Ville Syrjälä <ville.syrjala@...ux.intel.com>
> Cc: Harry Wentland <hwentlan@....com>
> Cc: Daniel Vetter <daniel.vetter@...ll.ch>
> Signed-off-by: Lyude Paul <lyude@...hat.com>
> ---
>  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);
> -			goto fail;
> +	ret = drm_dp_port_set_pdt(port, new_pdt);
> +	if (ret == 1) {
> +		send_link_addr = true;
> +	} else if (ret < 0) {
> +		DRM_ERROR("Failed to change PDT on port %p: %d\n",
> +			  port, ret);
> +		goto fail_unlock;
> +	}
> +
> +	if (send_link_addr) {
> +		mutex_lock(&mgr->lock);
> +		if (port->mstb) {
> +			child_mstb = port->mstb;
> +			drm_dp_mst_get_mstb_malloc(child_mstb);
>  		}
> +		mutex_unlock(&mgr->lock);
>  	}
>  
> -	if (created && !port->input) {
> -		char proppath[255];
> +	/*
> +	 * We unset port->connector before dropping connection_mutex so that
> +	 * there's no chance any of the atomic MST helpers can accidentally
> +	 * associate a to-be-destroyed connector with a port.
> +	 */
> +	if (port->connector && port->input) {
> +		connector_to_destroy = port->connector;
> +		port->connector = NULL;
> +	} else if (!port->connector && !port->input) {
> +		create_connector = true;
> +	}
>  
> -		build_mst_prop_path(mstb, port->port_num, proppath,
> -				    sizeof(proppath));
> -		port->connector = (*mgr->cbs->add_connector)(mgr, port,
> -							     proppath);
> -		if (!port->connector)
> -			goto fail;
> +	drm_modeset_unlock(&dev->mode_config.connection_mutex);

Do you drop this early b/c it deadlocks with something upstack? If so, I
wonder if you could plumb an acquire context through the appropriate
functions to avoid needing the port->lock at all?

Sean

>  
> -		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);
> -		}
> +	if (connector_to_destroy)
> +		mgr->cbs->destroy_connector(mgr, connector_to_destroy);
> +	else if (create_connector)
> +		drm_dp_mst_port_add_connector(mstb, port);
> +
> +	mutex_unlock(&port->lock);
>  
> -		(*mgr->cbs->register_connector)(port->connector);
> +	if (send_link_addr && child_mstb) {
> +		drm_dp_send_link_address(mgr, child_mstb);
> +		drm_dp_mst_put_mstb_malloc(child_mstb);
>  	}
>  
>  	/* put reference to this port */
>  	drm_dp_mst_topology_put_port(port);
>  	return;
>  
> -fail:
> +fail_unlock:
> +	drm_modeset_unlock(&dev->mode_config.connection_mutex);
> +	mutex_unlock(&port->lock);
> +
>  	/* Remove it from the port list */
>  	mutex_lock(&mgr->lock);
>  	list_del(&port->next);
> @@ -2022,6 +2078,7 @@ static void
>  drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch *mstb,
>  			    struct drm_dp_connection_status_notify *conn_stat)
>  {
> +	struct drm_device *dev = mstb->mgr->dev;
>  	struct drm_dp_mst_port *port;
>  	int old_ddps;
>  	bool dowork = false;
> @@ -2030,6 +2087,8 @@ drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch *mstb,
>  	if (!port)
>  		return;
>  
> +	drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> +
>  	old_ddps = port->ddps;
>  	port->mcs = conn_stat->message_capability_status;
>  	port->ldps = conn_stat->legacy_device_plug_status;
> @@ -2055,6 +2114,7 @@ drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch *mstb,
>  		}
>  	}
>  
> +	drm_modeset_unlock(&dev->mode_config.connection_mutex);
>  	drm_dp_mst_topology_put_port(port);
>  	if (dowork)
>  		queue_work(system_long_wq, &mstb->mgr->work);
> @@ -2147,28 +2207,34 @@ drm_dp_get_mst_branch_device_by_guid(struct drm_dp_mst_topology_mgr *mgr,
>  static void drm_dp_check_and_send_link_address(struct drm_dp_mst_topology_mgr *mgr,
>  					       struct drm_dp_mst_branch *mstb)
>  {
> +	struct drm_device *dev = mgr->dev;
>  	struct drm_dp_mst_port *port;
> -	struct drm_dp_mst_branch *mstb_child;
> +
>  	if (!mstb->link_address_sent)
>  		drm_dp_send_link_address(mgr, mstb);
>  
>  	list_for_each_entry(port, &mstb->ports, next) {
> -		if (port->input)
> -			continue;
> +		struct drm_dp_mst_branch *mstb_child = NULL;
> +
> +		drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>  
> -		if (!port->ddps)
> +		if (port->input || !port->ddps) {
> +			drm_modeset_unlock(&dev->mode_config.connection_mutex);
>  			continue;
> +		}
>  
>  		if (!port->available_pbn)
>  			drm_dp_send_enum_path_resources(mgr, mstb, port);
>  
> -		if (port->mstb) {
> +		if (port->mstb)
>  			mstb_child = drm_dp_mst_topology_get_mstb_validated(
>  			    mgr, port->mstb);
> -			if (mstb_child) {
> -				drm_dp_check_and_send_link_address(mgr, mstb_child);
> -				drm_dp_mst_topology_put_mstb(mstb_child);
> -			}
> +
> +		drm_modeset_unlock(&dev->mode_config.connection_mutex);
> +
> +		if (mstb_child) {
> +			drm_dp_check_and_send_link_address(mgr, mstb_child);
> +			drm_dp_mst_topology_put_mstb(mstb_child);
>  		}
>  	}
>  }
> diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
> index 7d80c38ee00e..1efbb086f7ac 100644
> --- a/include/drm/drm_dp_mst_helper.h
> +++ b/include/drm/drm_dp_mst_helper.h
> @@ -45,23 +45,34 @@ struct drm_dp_vcpi {
>  /**
>   * struct drm_dp_mst_port - MST port
>   * @port_num: port number
> - * @input: if this port is an input port.
> - * @mcs: message capability status - DP 1.2 spec.
> - * @ddps: DisplayPort Device Plug Status - DP 1.2
> - * @pdt: Peer Device Type
> - * @ldps: Legacy Device Plug Status
> - * @dpcd_rev: DPCD revision of device on this port
> - * @num_sdp_streams: Number of simultaneous streams
> - * @num_sdp_stream_sinks: Number of stream sinks
> - * @available_pbn: Available bandwidth for this port.
> + * @input: if this port is an input port. Protected by
> + * &drm_device.mode_config.connection_mutex.
> + * @mcs: message capability status - DP 1.2 spec. Protected by
> + * &drm_device.mode_config.connection_mutex.
> + * @ddps: DisplayPort Device Plug Status - DP 1.2. Protected by
> + * &drm_device.mode_config.connection_mutex.
> + * @pdt: Peer Device Type. Protected by
> + * &drm_device.mode_config.connection_mutex.
> + * @ldps: Legacy Device Plug Status. Protected by
> + * &drm_device.mode_config.connection_mutex.
> + * @dpcd_rev: DPCD revision of device on this port. Protected by
> + * &drm_device.mode_config.connection_mutex.
> + * @num_sdp_streams: Number of simultaneous streams. Protected by
> + * &drm_device.mode_config.connection_mutex.
> + * @num_sdp_stream_sinks: Number of stream sinks. Protected by
> + * &drm_device.mode_config.connection_mutex.
> + * @available_pbn: Available bandwidth for this port. Protected by
> + * &drm_device.mode_config.connection_mutex.
>   * @next: link to next port on this branch device
>   * @mstb: branch device on this port, protected by
>   * &drm_dp_mst_topology_mgr.lock
>   * @aux: i2c aux transport to talk to device connected to this port, protected
> - * by &drm_dp_mst_topology_mgr.lock
> + * by &drm_device.mode_config.connection_mutex.
>   * @parent: branch device parent of this port
>   * @vcpi: Virtual Channel Payload info for this port.
> - * @connector: DRM connector this port is connected to.
> + * @connector: DRM connector this port is connected to. Protected by @lock.
> + * When there is already a connector registered for this port, this is also
> + * protected by &drm_device.mode_config.connection_mutex.
>   * @mgr: topology manager this port lives under.
>   *
>   * This structure represents an MST port endpoint on a device somewhere
> @@ -100,6 +111,12 @@ struct drm_dp_mst_port {
>  	struct drm_connector *connector;
>  	struct drm_dp_mst_topology_mgr *mgr;
>  
> +	/**
> +	 * @lock: Protects @connector. If needed, this lock should be grabbed
> +	 * before &drm_device.mode_config.connection_mutex.
> +	 */
> +	struct mutex lock;
> +
>  	/**
>  	 * @cached_edid: for DP logical ports - make tiling work by ensuring
>  	 * that the EDID for all connectors is read immediately.
> -- 
> 2.21.0
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Powered by blists - more mailing lists