[PATCH 07/10] drm: Revamp connector_list protection
Daniel Vetter
daniel at ffwll.ch
Wed Jun 22 09:09:38 UTC 2016
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 at intel.com>
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
More information about the dri-devel
mailing list