Re: [PATCH 07/10] drm: Revamp connector_list protection

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Jun 21, 2016 at 11:10:32AM +0200, Daniel Vetter wrote:
> This is a pretty good horror show, but I think it's the best tradeoff:
> - Thanks to srcu and delayed freeing the locking doesn't leak out to
>   callers, hence no added headaches with locking inversions.
> - For core and drivers which hot-remove connectors all the connector
>   list walking details are hidden in a macro.
> - Other drivers which don't care about hot-removing can simply keep on
>   using the normal list walking macros.
> 
> The new design is:
> - New dev->mode_config.connector_list_lock to protect the
>   connector_list, and the connector_ida (since that's also
>   unprotected), plus num_connectors. This is a pure leaf lock, nothing
>   is allowed to nest within, nothing outside of connector init/cleanup
>   will ever need it.
> - Protect connector_list itself with srcu. This allows sleeping and
>   anything else. The only thing which is not allowed is calling
>   synchronize_srcu, or grabbing any locks or waiting on
>   waitqueues/completions/whatever which might call that. To make this
>   100% safe we opt to not have any calls to synchronize_srcu.
> - Protect against use-after-free of connectors using call_srcu, to
>   delay the kfree enough.
> - To protect against zombie connectors which are in progress of final
>   destruction use kref_get_unless_zero. This is safe since srcu
>   prevents against use-after-free, and that's the only guarantee we
>   need.
> 
> For this demo I only bothered converting i915, and there also not
> everything - I left the connector loops in the modeset code unchanged
> since those will all be converted over to
> drm_for_each_connector_in_state to make them work correctly for
> nonblocking atomic commits. Only loops outside of atomic commits
> should be converted to drm_for_each_connector.
> 
> Note that the i915 DP MST code still uses drm_modeset_lock_all(). But
> that's not because of drm core connector lifetime issues, but because
> the fbdev helper reuses core locks to mange its own lists and data
> structures. Thierry Reding has a patch series to gift fbdev its own
> lock, which will remedy this.
> 
> v2: Totally rewrite everything to pack it up in a neat
> iterator macro, with init/check/next extracted into helpers.
> 
> v3: Tiny rebase conflict.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx>

Ok, CI pointed out a flaw in this: break (or worse goto) means that the
unreference and the read_unlock_srcu isn't done at the end, which means
leaks and lockdep complaints. break can be fixed, but goto is impossible
to fix with plain C ...

Not sure what do to now, since I'd really like to avoid leaking the
connector_list locking all over the place. It means lots of churn, and
also issues (if we go without srcu) with locking inversions.
-Daniel

> ---
>  drivers/gpu/drm/drm_crtc.c          | 57 +++++++++++++++++++++-----
>  drivers/gpu/drm/i915/intel_dp_mst.c |  2 +-
>  include/drm/drm_crtc.h              | 82 +++++++++++++++++++++++++++----------
>  3 files changed, 109 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 6a8f91e8821b..dc22c0bfbe99 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -44,6 +44,9 @@
>  #include "drm_crtc_internal.h"
>  #include "drm_internal.h"
>  
> +DEFINE_SRCU(drm_connector_list_srcu);
> +EXPORT_SYMBOL(drm_connector_list_srcu);
> +
>  static struct drm_framebuffer *
>  internal_framebuffer_create(struct drm_device *dev,
>  			    const struct drm_mode_fb_cmd2 *r,
> @@ -825,7 +828,7 @@ static void drm_connector_get_cmdline_mode(struct drm_connector *connector)
>  		      mode->interlace ?  " interlaced" : "");
>  }
>  
> -static void drm_connector_free(struct kref *kref)
> +static void drm_connector_kref_release(struct kref *kref)
>  {
>  	struct drm_connector *connector =
>  		container_of(kref, struct drm_connector, base.refcount);
> @@ -858,11 +861,10 @@ int drm_connector_init(struct drm_device *dev,
>  	struct ida *connector_ida =
>  		&drm_connector_enum_list[connector_type].ida;
>  
> -	drm_modeset_lock_all(dev);
> -
> +	mutex_lock(&dev->mode_config.connector_list_lock);
>  	ret = drm_mode_object_get_reg(dev, &connector->base,
>  				      DRM_MODE_OBJECT_CONNECTOR,
> -				      false, drm_connector_free);
> +				      false, drm_connector_kref_release);
>  	if (ret)
>  		goto out_unlock;
>  
> @@ -899,11 +901,6 @@ int drm_connector_init(struct drm_device *dev,
>  
>  	drm_connector_get_cmdline_mode(connector);
>  
> -	/* We should add connectors at the end to avoid upsetting the connector
> -	 * index too much. */
> -	list_add_tail(&connector->head, &config->connector_list);
> -	config->num_connector++;
> -
>  	if (connector_type != DRM_MODE_CONNECTOR_VIRTUAL)
>  		drm_object_attach_property(&connector->base,
>  					      config->edid_property,
> @@ -917,6 +914,11 @@ int drm_connector_init(struct drm_device *dev,
>  	}
>  
>  	connector->debugfs_entry = NULL;
> +
> +	/* We must add the connector to the list last. */
> +	list_add_tail_rcu(&connector->head, &config->connector_list);
> +	config->num_connector++;
> +
>  out_put_type_id:
>  	if (ret)
>  		ida_remove(connector_ida, connector->connector_type_id);
> @@ -928,7 +930,7 @@ out_put:
>  		drm_mode_object_unregister(dev, &connector->base);
>  
>  out_unlock:
> -	drm_modeset_unlock_all(dev);
> +	mutex_unlock(&dev->mode_config.connector_list_lock);
>  
>  	return ret;
>  }
> @@ -951,6 +953,7 @@ void drm_connector_cleanup(struct drm_connector *connector)
>  	if (WARN_ON(connector->registered))
>  		drm_connector_unregister(connector);
>  
> +	mutex_lock(&dev->mode_config.connector_list_lock);
>  	if (connector->tile_group) {
>  		drm_mode_put_tile_group(dev, connector->tile_group);
>  		connector->tile_group = NULL;
> @@ -981,9 +984,42 @@ void drm_connector_cleanup(struct drm_connector *connector)
>  						       connector->state);
>  
>  	memset(connector, 0, sizeof(*connector));
> +	mutex_unlock(&dev->mode_config.connector_list_lock);
>  }
>  EXPORT_SYMBOL(drm_connector_cleanup);
>  
> +static void drm_connector_free_cb(struct rcu_head *head)
> +{
> +	struct drm_connector *connector = container_of(head,
> +						       struct drm_connector,
> +						       free_head);
> +
> +	kfree(connector);
> +}
> +
> +/**
> + * drm_connector_free - frees a connector structure
> + * @connector: connector to free
> + *
> + * This frees @connector using call_srcu() and kfree(). If the driver subclasses
> + * the connector, then the embedded struct &drm_connector must be the first
> + * element, and @connector must have been allocated using kmalloc() or one of
> + * the functions wrapping that.
> + *
> + * Delayed freeing is required to avoid use-after-free when walking the
> + * connector list, which is srcu protected. Hence drivers must use this function
> + * for connectors which can be removed while the driver is loaded. Currently
> + * that's only true for DP MST connectors. For any other connectors it is valid
> + * to call kfree directly.
> + */
> +void drm_connector_free(struct drm_connector *connector)
> +{
> +	call_srcu(&drm_connector_list_srcu,
> +		  &connector->free_head,
> +		  drm_connector_free_cb);
> +}
> +EXPORT_SYMBOL(drm_connector_free);
> +
>  /**
>   * drm_connector_register - register a connector
>   * @connector: the connector to register
> @@ -5554,6 +5590,7 @@ void drm_mode_config_init(struct drm_device *dev)
>  	mutex_init(&dev->mode_config.idr_mutex);
>  	mutex_init(&dev->mode_config.fb_lock);
>  	mutex_init(&dev->mode_config.blob_lock);
> +	mutex_init(&dev->mode_config.connector_list_lock);
>  	INIT_LIST_HEAD(&dev->mode_config.fb_list);
>  	INIT_LIST_HEAD(&dev->mode_config.crtc_list);
>  	INIT_LIST_HEAD(&dev->mode_config.connector_list);
> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> index 9646816604be..11bbbb194f2d 100644
> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> @@ -327,7 +327,7 @@ intel_dp_mst_connector_destroy(struct drm_connector *connector)
>  		kfree(intel_connector->edid);
>  
>  	drm_connector_cleanup(connector);
> -	kfree(connector);
> +	drm_connector_free(connector);
>  }
>  
>  static const struct drm_connector_funcs intel_dp_mst_connector_funcs = {
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index ba574b9c1ec4..8f4380ac113e 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -32,6 +32,7 @@
>  #include <linux/fb.h>
>  #include <linux/hdmi.h>
>  #include <linux/media-bus-format.h>
> +#include <linux/srcu.h>
>  #include <uapi/drm/drm_mode.h>
>  #include <uapi/drm/drm_fourcc.h>
>  #include <drm/drm_modeset_lock.h>
> @@ -1186,6 +1187,7 @@ struct drm_encoder {
>   * @kdev: kernel device for sysfs attributes
>   * @attr: sysfs attributes
>   * @head: list management
> + * @free_head: rcu delayed freeing management
>   * @base: base KMS object
>   * @name: human readable name, can be overwritten by the driver
>   * @connector_id: compacted connector id useful indexing arrays
> @@ -1241,6 +1243,7 @@ struct drm_connector {
>  	struct device *kdev;
>  	struct device_attribute *attr;
>  	struct list_head head;
> +	struct rcu_head free_head;
>  
>  	struct drm_mode_object base;
>  
> @@ -2309,7 +2312,7 @@ struct drm_mode_config {
>  	 * @tile_idr:
>  	 *
>  	 * Use this idr for allocating new IDs for tiled sinks like use in some
> -	 * high-res DP MST screens.
> +	 * high-res DP MST screens. Protected by @idr_mutex.
>  	 */
>  	struct idr tile_idr;
>  
> @@ -2320,6 +2323,14 @@ struct drm_mode_config {
>  	int num_connector;
>  	struct ida connector_ida;
>  	struct list_head connector_list;
> +
> +	/**
> +	 * @connector_list_lock:
> +	 *
> +	 * Protects @connector_list, @connector_ida and * @num_conncetors.
> +	 */
> +	struct mutex connector_list_lock;
> +
>  	int num_encoder;
>  	struct list_head encoder_list;
>  
> @@ -2426,6 +2437,8 @@ struct drm_mode_config {
>  	struct drm_mode_config_helper_funcs *helper_private;
>  };
>  
> +extern struct srcu_struct drm_connector_list_srcu;
> +
>  /**
>   * drm_for_each_plane_mask - iterate over planes specified by bitmask
>   * @plane: the loop cursor
> @@ -2505,6 +2518,7 @@ int drm_connector_register(struct drm_connector *connector);
>  void drm_connector_unregister(struct drm_connector *connector);
>  
>  extern void drm_connector_cleanup(struct drm_connector *connector);
> +extern void drm_connector_free(struct drm_connector *connector);
>  static inline unsigned drm_connector_index(struct drm_connector *connector)
>  {
>  	return connector->connector_id;
> @@ -2835,31 +2849,57 @@ static inline void drm_connector_unreference(struct drm_connector *connector)
>  #define drm_for_each_crtc(crtc, dev) \
>  	list_for_each_entry(crtc, &(dev)->mode_config.crtc_list, head)
>  
> +struct drm_connector_iter {
> +	struct drm_mode_config *mode_config;
> +	struct drm_connector *connector;
> +	int srcu_ret;
> +};
> +
>  static inline void
> -assert_drm_connector_list_read_locked(struct drm_mode_config *mode_config)
> +__drm_connector_iter_init(struct drm_mode_config *mode_config,
> +			  struct drm_connector_iter *iter)
>  {
> -	/*
> -	 * The connector hotadd/remove code currently grabs both locks when
> -	 * updating lists. Hence readers need only hold either of them to be
> -	 * safe and the check amounts to
> -	 *
> -	 * WARN_ON(not_holding(A) && not_holding(B)).
> -	 */
> -	WARN_ON(!mutex_is_locked(&mode_config->mutex) &&
> -		!drm_modeset_is_locked(&mode_config->connection_mutex));
> +	iter->mode_config = mode_config;
> +	iter->srcu_ret = srcu_read_lock(&drm_connector_list_srcu);
> +	iter->connector = list_entry_rcu(mode_config->connector_list.next,
> +					 struct drm_connector, head);
>  }
>  
> -struct drm_connector_iter {
> -	struct drm_mode_config *mode_config;
> -};
> +static inline struct drm_connector *
> +__drm_connector_iter_check(struct drm_connector_iter *iter)
> +{
> +retry:
> +	if (&iter->connector->head == &iter->mode_config->connector_list) {
> +		/* last iteration, clean up */
> +		srcu_read_unlock(&drm_connector_list_srcu, iter->srcu_ret);
> +		return NULL;
> +	}
> +
> +	/* srcu protects against iter->connector disappearing */
> +	if (!kref_get_unless_zero(&iter->connector->base.refcount)) {
> +		iter->connector = list_entry_rcu(iter->connector->head.next,
> +						 struct drm_connector, head);
> +		goto retry;
> +	}
> +
> +	return iter->connector;
> +}
> +
> +static inline void
> +__drm_connector_iter_next(struct drm_connector_iter *iter)
> +{
> +	/* _next() is only called when _check() succeeded on iter->connector,
> +	 * and _check() only succeeds if kref_get_unless_zero() succeeded.
> +	 * Therefore dropping the reference here is the correct place. */
> +	drm_connector_unreference(iter->connector);
> +	iter->connector = list_entry_rcu(iter->connector->head.next,
> +					 struct drm_connector, head);
> +}
>  
> -#define drm_for_each_connector(connector, dev, iter) \
> -	for ((iter).mode_config = NULL,						\
> -	     assert_drm_connector_list_read_locked(&(dev)->mode_config),	\
> -	     connector = list_first_entry(&(dev)->mode_config.connector_list,	\
> -					  struct drm_connector, head);		\
> -	     &connector->head != (&(dev)->mode_config.connector_list);		\
> -	     connector = list_next_entry(connector, head))
> +#define drm_for_each_connector(connector, dev, iter) 			\
> +	for (__drm_connector_iter_init(&(dev)->mode_config, &(iter));	\
> +	     (connector = __drm_connector_iter_check(&(iter)));		\
> +	     __drm_connector_iter_next(&(iter)))
>  
>  #define drm_for_each_encoder(encoder, dev) \
>  	list_for_each_entry(encoder, &(dev)->mode_config.encoder_list, head)
> -- 
> 2.8.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux