diff mbox

[3/6] drm/atomic: Don't overrun the connector array when hotplugging

Message ID 1416418691-24747-3-git-send-email-daniel.vetter@ffwll.ch
State Accepted
Headers show

Commit Message

Daniel Vetter Nov. 19, 2014, 5:38 p.m. UTC
Yet another fallout from not considering DP MST hotplug. With the
previous patches we have stable indices, but it might still happen
that a connector gets added between when we allocate the array and
when we actually add a connector. Especially when we back off due to
ww mutex contention or similar issues.

So store the sizes of the arrays in struct drm_atomic_state and double
check them. We don't really care about races except that we want to
use a consistent value, so ACCESS_ONCE is all we need. And if we
indeed notice that we'd overrun the array then just give up and
restart the entire ioctl.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_atomic.c        | 26 +++++++++++++++++++++-----
 drivers/gpu/drm/drm_atomic_helper.c | 23 ++++++++---------------
 include/drm/drm_crtc.h              |  2 ++
 3 files changed, 31 insertions(+), 20 deletions(-)

Comments

Rob Clark Nov. 19, 2014, 8:24 p.m. UTC | #1
On Wed, Nov 19, 2014 at 12:38 PM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> Yet another fallout from not considering DP MST hotplug. With the
> previous patches we have stable indices, but it might still happen
> that a connector gets added between when we allocate the array and
> when we actually add a connector. Especially when we back off due to
> ww mutex contention or similar issues.
>
> So store the sizes of the arrays in struct drm_atomic_state and double
> check them. We don't really care about races except that we want to
> use a consistent value, so ACCESS_ONCE is all we need. And if we
> indeed notice that we'd overrun the array then just give up and
> restart the entire ioctl.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Reviewed-by: Rob Clark <robdclark@gmail.com>

> ---
>  drivers/gpu/drm/drm_atomic.c        | 26 +++++++++++++++++++++-----
>  drivers/gpu/drm/drm_atomic_helper.c | 23 ++++++++---------------
>  include/drm/drm_crtc.h              |  2 ++
>  3 files changed, 31 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 67c1dc894bd9..3624632084e2 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -56,6 +56,8 @@ drm_atomic_state_alloc(struct drm_device *dev)
>         if (!state)
>                 return NULL;
>
> +       state->num_connector = ACCESS_ONCE(dev->mode_config.num_connector);
> +
>         state->crtcs = kcalloc(dev->mode_config.num_crtc,
>                                sizeof(*state->crtcs), GFP_KERNEL);
>         if (!state->crtcs)
> @@ -72,12 +74,12 @@ drm_atomic_state_alloc(struct drm_device *dev)
>                                       sizeof(*state->plane_states), GFP_KERNEL);
>         if (!state->plane_states)
>                 goto fail;
> -       state->connectors = kcalloc(dev->mode_config.num_connector,
> +       state->connectors = kcalloc(state->num_connector,
>                                     sizeof(*state->connectors),
>                                     GFP_KERNEL);
>         if (!state->connectors)
>                 goto fail;
> -       state->connector_states = kcalloc(dev->mode_config.num_connector,
> +       state->connector_states = kcalloc(state->num_connector,
>                                           sizeof(*state->connector_states),
>                                           GFP_KERNEL);
>         if (!state->connector_states)
> @@ -117,7 +119,7 @@ void drm_atomic_state_clear(struct drm_atomic_state *state)
>
>         DRM_DEBUG_KMS("Clearing atomic state %p\n", state);
>
> -       for (i = 0; i < config->num_connector; i++) {
> +       for (i = 0; i < state->num_connector; i++) {
>                 struct drm_connector *connector = state->connectors[i];
>
>                 if (!connector)
> @@ -304,6 +306,21 @@ drm_atomic_get_connector_state(struct drm_atomic_state *state,
>
>         index = drm_connector_index(connector);
>
> +       /*
> +        * Construction of atomic state updates can race with a connector
> +        * hot-add which might overflow. In this case flip the table and just
> +        * restart the entire ioctl - no one is fast enough to livelock a cpu
> +        * with physical hotplug events anyway.
> +        *
> +        * Note that we only grab the indexes once we have the right lock to
> +        * prevent hotplug/unplugging of connectors. So removal is no problem,
> +        * at most the array is a bit too large.
> +        */
> +       if (index >= state->num_connector) {
> +               DRM_DEBUG_KMS("Hot-added connector would overflow state array, restarting\n");
> +               return -EAGAIN;
> +       }
> +
>         if (state->connector_states[index])
>                 return state->connector_states[index];
>
> @@ -499,10 +516,9 @@ int
>  drm_atomic_connectors_for_crtc(struct drm_atomic_state *state,
>                                struct drm_crtc *crtc)
>  {
> -       int nconnectors = state->dev->mode_config.num_connector;
>         int i, num_connected_connectors = 0;
>
> -       for (i = 0; i < nconnectors; i++) {
> +       for (i = 0; i < state->num_connector; i++) {
>                 struct drm_connector_state *conn_state;
>
>                 conn_state = state->connector_states[i];
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 0cd054615920..99095ef147ef 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -249,7 +249,6 @@ static int
>  mode_fixup(struct drm_atomic_state *state)
>  {
>         int ncrtcs = state->dev->mode_config.num_crtc;
> -       int nconnectors = state->dev->mode_config.num_connector;
>         struct drm_crtc_state *crtc_state;
>         struct drm_connector_state *conn_state;
>         int i;
> @@ -264,7 +263,7 @@ mode_fixup(struct drm_atomic_state *state)
>                 drm_mode_copy(&crtc_state->adjusted_mode, &crtc_state->mode);
>         }
>
> -       for (i = 0; i < nconnectors; i++) {
> +       for (i = 0; i < state->num_connector; i++) {
>                 struct drm_encoder_helper_funcs *funcs;
>                 struct drm_encoder *encoder;
>
> @@ -336,7 +335,6 @@ drm_atomic_helper_check_prepare(struct drm_device *dev,
>                                 struct drm_atomic_state *state)
>  {
>         int ncrtcs = dev->mode_config.num_crtc;
> -       int nconnectors = dev->mode_config.num_connector;
>         struct drm_crtc *crtc;
>         struct drm_crtc_state *crtc_state;
>         int i, ret;
> @@ -361,7 +359,7 @@ drm_atomic_helper_check_prepare(struct drm_device *dev,
>                 }
>         }
>
> -       for (i = 0; i < nconnectors; i++) {
> +       for (i = 0; i < state->num_connector; i++) {
>                 /*
>                  * This only sets crtc->mode_changed for routing changes,
>                  * drivers must set crtc->mode_changed themselves when connector
> @@ -485,10 +483,9 @@ static void
>  disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
>  {
>         int ncrtcs = old_state->dev->mode_config.num_crtc;
> -       int nconnectors = old_state->dev->mode_config.num_connector;
>         int i;
>
> -       for (i = 0; i < nconnectors; i++) {
> +       for (i = 0; i < old_state->num_connector; i++) {
>                 struct drm_connector_state *old_conn_state;
>                 struct drm_connector *connector;
>                 struct drm_encoder_helper_funcs *funcs;
> @@ -553,12 +550,11 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
>  static void
>  set_routing_links(struct drm_device *dev, struct drm_atomic_state *old_state)
>  {
> -       int nconnectors = dev->mode_config.num_connector;
>         int ncrtcs = old_state->dev->mode_config.num_crtc;
>         int i;
>
>         /* clear out existing links */
> -       for (i = 0; i < nconnectors; i++) {
> +       for (i = 0; i < old_state->num_connector; i++) {
>                 struct drm_connector *connector;
>
>                 connector = old_state->connectors[i];
> @@ -573,7 +569,7 @@ set_routing_links(struct drm_device *dev, struct drm_atomic_state *old_state)
>         }
>
>         /* set new links */
> -       for (i = 0; i < nconnectors; i++) {
> +       for (i = 0; i < old_state->num_connector; i++) {
>                 struct drm_connector *connector;
>
>                 connector = old_state->connectors[i];
> @@ -608,7 +604,6 @@ static void
>  crtc_set_mode(struct drm_device *dev, struct drm_atomic_state *old_state)
>  {
>         int ncrtcs = old_state->dev->mode_config.num_crtc;
> -       int nconnectors = old_state->dev->mode_config.num_connector;
>         int i;
>
>         for (i = 0; i < ncrtcs; i++) {
> @@ -626,7 +621,7 @@ crtc_set_mode(struct drm_device *dev, struct drm_atomic_state *old_state)
>                         funcs->mode_set_nofb(crtc);
>         }
>
> -       for (i = 0; i < nconnectors; i++) {
> +       for (i = 0; i < old_state->num_connector; i++) {
>                 struct drm_connector *connector;
>                 struct drm_crtc_state *new_crtc_state;
>                 struct drm_encoder_helper_funcs *funcs;
> @@ -687,7 +682,6 @@ void drm_atomic_helper_commit_post_planes(struct drm_device *dev,
>                                           struct drm_atomic_state *old_state)
>  {
>         int ncrtcs = old_state->dev->mode_config.num_crtc;
> -       int nconnectors = old_state->dev->mode_config.num_connector;
>         int i;
>
>         for (i = 0; i < ncrtcs; i++) {
> @@ -706,7 +700,7 @@ void drm_atomic_helper_commit_post_planes(struct drm_device *dev,
>                         funcs->commit(crtc);
>         }
>
> -       for (i = 0; i < nconnectors; i++) {
> +       for (i = 0; i < old_state->num_connector; i++) {
>                 struct drm_connector *connector;
>                 struct drm_encoder_helper_funcs *funcs;
>                 struct drm_encoder *encoder;
> @@ -1304,7 +1298,6 @@ static int update_output_state(struct drm_atomic_state *state,
>  {
>         struct drm_device *dev = set->crtc->dev;
>         struct drm_connector_state *conn_state;
> -       int nconnectors = state->dev->mode_config.num_connector;
>         int ncrtcs = state->dev->mode_config.num_crtc;
>         int ret, i, j;
>
> @@ -1333,7 +1326,7 @@ static int update_output_state(struct drm_atomic_state *state,
>         }
>
>         /* Then recompute connector->crtc links and crtc enabling state. */
> -       for (i = 0; i < nconnectors; i++) {
> +       for (i = 0; i < state->num_connector; i++) {
>                 struct drm_connector *connector;
>
>                 connector = state->connectors[i];
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 91c09520aad3..cdbae7bdac70 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -845,6 +845,7 @@ struct drm_bridge {
>   * @plane_states: pointer to array of plane states pointers
>   * @crtcs: pointer to array of CRTC pointers
>   * @crtc_states: pointer to array of CRTC states pointers
> + * @num_connector: size of the @connectors and @connector_states arrays
>   * @connectors: pointer to array of connector pointers
>   * @connector_states: pointer to array of connector states pointers
>   * @acquire_ctx: acquire context for this atomic modeset state update
> @@ -856,6 +857,7 @@ struct drm_atomic_state {
>         struct drm_plane_state **plane_states;
>         struct drm_crtc **crtcs;
>         struct drm_crtc_state **crtc_states;
> +       int num_connector;
>         struct drm_connector **connectors;
>         struct drm_connector_state **connector_states;
>
> --
> 2.1.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Thierry Reding Nov. 20, 2014, 8:53 a.m. UTC | #2
On Wed, Nov 19, 2014 at 06:38:08PM +0100, Daniel Vetter wrote:
[...]
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
[...]
> @@ -304,6 +306,21 @@ drm_atomic_get_connector_state(struct drm_atomic_state *state,
>  
>  	index = drm_connector_index(connector);
>  
> +	/*
> +	 * Construction of atomic state updates can race with a connector
> +	 * hot-add which might overflow. In this case flip the table and just
> +	 * restart the entire ioctl - no one is fast enough to livelock a cpu
> +	 * with physical hotplug events anyway.
> +	 *
> +	 * Note that we only grab the indexes once we have the right lock to
> +	 * prevent hotplug/unplugging of connectors. So removal is no problem,
> +	 * at most the array is a bit too large.
> +	 */
> +	if (index >= state->num_connector) {
> +		DRM_DEBUG_KMS("Hot-added connector would overflow state array, restarting\n");
> +		return -EAGAIN;

This function returns a pointer, so this needs to be ERR_PTR(-EAGAIN).

Thierry
diff mbox

Patch

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 67c1dc894bd9..3624632084e2 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -56,6 +56,8 @@  drm_atomic_state_alloc(struct drm_device *dev)
 	if (!state)
 		return NULL;
 
+	state->num_connector = ACCESS_ONCE(dev->mode_config.num_connector);
+
 	state->crtcs = kcalloc(dev->mode_config.num_crtc,
 			       sizeof(*state->crtcs), GFP_KERNEL);
 	if (!state->crtcs)
@@ -72,12 +74,12 @@  drm_atomic_state_alloc(struct drm_device *dev)
 				      sizeof(*state->plane_states), GFP_KERNEL);
 	if (!state->plane_states)
 		goto fail;
-	state->connectors = kcalloc(dev->mode_config.num_connector,
+	state->connectors = kcalloc(state->num_connector,
 				    sizeof(*state->connectors),
 				    GFP_KERNEL);
 	if (!state->connectors)
 		goto fail;
-	state->connector_states = kcalloc(dev->mode_config.num_connector,
+	state->connector_states = kcalloc(state->num_connector,
 					  sizeof(*state->connector_states),
 					  GFP_KERNEL);
 	if (!state->connector_states)
@@ -117,7 +119,7 @@  void drm_atomic_state_clear(struct drm_atomic_state *state)
 
 	DRM_DEBUG_KMS("Clearing atomic state %p\n", state);
 
-	for (i = 0; i < config->num_connector; i++) {
+	for (i = 0; i < state->num_connector; i++) {
 		struct drm_connector *connector = state->connectors[i];
 
 		if (!connector)
@@ -304,6 +306,21 @@  drm_atomic_get_connector_state(struct drm_atomic_state *state,
 
 	index = drm_connector_index(connector);
 
+	/*
+	 * Construction of atomic state updates can race with a connector
+	 * hot-add which might overflow. In this case flip the table and just
+	 * restart the entire ioctl - no one is fast enough to livelock a cpu
+	 * with physical hotplug events anyway.
+	 *
+	 * Note that we only grab the indexes once we have the right lock to
+	 * prevent hotplug/unplugging of connectors. So removal is no problem,
+	 * at most the array is a bit too large.
+	 */
+	if (index >= state->num_connector) {
+		DRM_DEBUG_KMS("Hot-added connector would overflow state array, restarting\n");
+		return -EAGAIN;
+	}
+
 	if (state->connector_states[index])
 		return state->connector_states[index];
 
@@ -499,10 +516,9 @@  int
 drm_atomic_connectors_for_crtc(struct drm_atomic_state *state,
 			       struct drm_crtc *crtc)
 {
-	int nconnectors = state->dev->mode_config.num_connector;
 	int i, num_connected_connectors = 0;
 
-	for (i = 0; i < nconnectors; i++) {
+	for (i = 0; i < state->num_connector; i++) {
 		struct drm_connector_state *conn_state;
 
 		conn_state = state->connector_states[i];
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 0cd054615920..99095ef147ef 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -249,7 +249,6 @@  static int
 mode_fixup(struct drm_atomic_state *state)
 {
 	int ncrtcs = state->dev->mode_config.num_crtc;
-	int nconnectors = state->dev->mode_config.num_connector;
 	struct drm_crtc_state *crtc_state;
 	struct drm_connector_state *conn_state;
 	int i;
@@ -264,7 +263,7 @@  mode_fixup(struct drm_atomic_state *state)
 		drm_mode_copy(&crtc_state->adjusted_mode, &crtc_state->mode);
 	}
 
-	for (i = 0; i < nconnectors; i++) {
+	for (i = 0; i < state->num_connector; i++) {
 		struct drm_encoder_helper_funcs *funcs;
 		struct drm_encoder *encoder;
 
@@ -336,7 +335,6 @@  drm_atomic_helper_check_prepare(struct drm_device *dev,
 				struct drm_atomic_state *state)
 {
 	int ncrtcs = dev->mode_config.num_crtc;
-	int nconnectors = dev->mode_config.num_connector;
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *crtc_state;
 	int i, ret;
@@ -361,7 +359,7 @@  drm_atomic_helper_check_prepare(struct drm_device *dev,
 		}
 	}
 
-	for (i = 0; i < nconnectors; i++) {
+	for (i = 0; i < state->num_connector; i++) {
 		/*
 		 * This only sets crtc->mode_changed for routing changes,
 		 * drivers must set crtc->mode_changed themselves when connector
@@ -485,10 +483,9 @@  static void
 disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
 {
 	int ncrtcs = old_state->dev->mode_config.num_crtc;
-	int nconnectors = old_state->dev->mode_config.num_connector;
 	int i;
 
-	for (i = 0; i < nconnectors; i++) {
+	for (i = 0; i < old_state->num_connector; i++) {
 		struct drm_connector_state *old_conn_state;
 		struct drm_connector *connector;
 		struct drm_encoder_helper_funcs *funcs;
@@ -553,12 +550,11 @@  disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
 static void
 set_routing_links(struct drm_device *dev, struct drm_atomic_state *old_state)
 {
-	int nconnectors = dev->mode_config.num_connector;
 	int ncrtcs = old_state->dev->mode_config.num_crtc;
 	int i;
 
 	/* clear out existing links */
-	for (i = 0; i < nconnectors; i++) {
+	for (i = 0; i < old_state->num_connector; i++) {
 		struct drm_connector *connector;
 
 		connector = old_state->connectors[i];
@@ -573,7 +569,7 @@  set_routing_links(struct drm_device *dev, struct drm_atomic_state *old_state)
 	}
 
 	/* set new links */
-	for (i = 0; i < nconnectors; i++) {
+	for (i = 0; i < old_state->num_connector; i++) {
 		struct drm_connector *connector;
 
 		connector = old_state->connectors[i];
@@ -608,7 +604,6 @@  static void
 crtc_set_mode(struct drm_device *dev, struct drm_atomic_state *old_state)
 {
 	int ncrtcs = old_state->dev->mode_config.num_crtc;
-	int nconnectors = old_state->dev->mode_config.num_connector;
 	int i;
 
 	for (i = 0; i < ncrtcs; i++) {
@@ -626,7 +621,7 @@  crtc_set_mode(struct drm_device *dev, struct drm_atomic_state *old_state)
 			funcs->mode_set_nofb(crtc);
 	}
 
-	for (i = 0; i < nconnectors; i++) {
+	for (i = 0; i < old_state->num_connector; i++) {
 		struct drm_connector *connector;
 		struct drm_crtc_state *new_crtc_state;
 		struct drm_encoder_helper_funcs *funcs;
@@ -687,7 +682,6 @@  void drm_atomic_helper_commit_post_planes(struct drm_device *dev,
 					  struct drm_atomic_state *old_state)
 {
 	int ncrtcs = old_state->dev->mode_config.num_crtc;
-	int nconnectors = old_state->dev->mode_config.num_connector;
 	int i;
 
 	for (i = 0; i < ncrtcs; i++) {
@@ -706,7 +700,7 @@  void drm_atomic_helper_commit_post_planes(struct drm_device *dev,
 			funcs->commit(crtc);
 	}
 
-	for (i = 0; i < nconnectors; i++) {
+	for (i = 0; i < old_state->num_connector; i++) {
 		struct drm_connector *connector;
 		struct drm_encoder_helper_funcs *funcs;
 		struct drm_encoder *encoder;
@@ -1304,7 +1298,6 @@  static int update_output_state(struct drm_atomic_state *state,
 {
 	struct drm_device *dev = set->crtc->dev;
 	struct drm_connector_state *conn_state;
-	int nconnectors = state->dev->mode_config.num_connector;
 	int ncrtcs = state->dev->mode_config.num_crtc;
 	int ret, i, j;
 
@@ -1333,7 +1326,7 @@  static int update_output_state(struct drm_atomic_state *state,
 	}
 
 	/* Then recompute connector->crtc links and crtc enabling state. */
-	for (i = 0; i < nconnectors; i++) {
+	for (i = 0; i < state->num_connector; i++) {
 		struct drm_connector *connector;
 
 		connector = state->connectors[i];
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 91c09520aad3..cdbae7bdac70 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -845,6 +845,7 @@  struct drm_bridge {
  * @plane_states: pointer to array of plane states pointers
  * @crtcs: pointer to array of CRTC pointers
  * @crtc_states: pointer to array of CRTC states pointers
+ * @num_connector: size of the @connectors and @connector_states arrays
  * @connectors: pointer to array of connector pointers
  * @connector_states: pointer to array of connector states pointers
  * @acquire_ctx: acquire context for this atomic modeset state update
@@ -856,6 +857,7 @@  struct drm_atomic_state {
 	struct drm_plane_state **plane_states;
 	struct drm_crtc **crtcs;
 	struct drm_crtc_state **crtc_states;
+	int num_connector;
 	struct drm_connector **connectors;
 	struct drm_connector_state **connector_states;