drm/i915: Fix global state use-after-frees with a refcount

While the current locking/serialization of the global state
suffices for protecting the obj->state access and the actual
hardware reprogramming, we do have a problem with accessing
the old/new states during nonblocking commits.

The state computation and swap will be protected by the crtc
locks, but the commit_tails can finish out of order, thus also
causing the atomic states to be cleaned up out of order. This
would mean the commit that started first but finished last has
had its new state freed as the no-longer-needed old state by the
other commit.

To fix this let's just refcount the states. obj->state amounts
to one reference, and the intel_atomic_state holds extra references
to both its new and old global obj states.

Fixes: 0ef1905ecf2e ("drm/i915: Introduce better global state handling")
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20200527200245.13184-1-ville.syrjala@linux.intel.com
Reviewed-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>

diff --git a/drivers/gpu/drm/i915/display/intel_global_state.c b/drivers/gpu/drm/i915/display/intel_global_state.c
index 212d4ee682059e7dec696cd8cecb7c8ddbad2698..7a19215ad8441c481e38e10addcd34039cd309cd 100644 (file)
--- a/drivers/gpu/drm/i915/display/intel_global_state.c
+++ b/drivers/gpu/drm/i915/display/intel_global_state.c
@@ -10,6 +10,28 @@
 #include "intel_display_types.h"
 #include "intel_global_state.h"
 
+static void __intel_atomic_global_state_free(struct kref *kref)
+{
+       struct intel_global_state *obj_state =
+               container_of(kref, struct intel_global_state, ref);
+       struct intel_global_obj *obj = obj_state->obj;
+
+       obj->funcs->atomic_destroy_state(obj, obj_state);
+}
+
+static void intel_atomic_global_state_put(struct intel_global_state *obj_state)
+{
+       kref_put(&obj_state->ref, __intel_atomic_global_state_free);
+}
+
+static struct intel_global_state *
+intel_atomic_global_state_get(struct intel_global_state *obj_state)
+{
+       kref_get(&obj_state->ref);
+
+       return obj_state;
+}
+
 void intel_atomic_global_obj_init(struct drm_i915_private *dev_priv,
                                  struct intel_global_obj *obj,
                                  struct intel_global_state *state,
@@ -17,6 +39,10 @@ void intel_atomic_global_obj_init(struct drm_i915_private *dev_priv,
 {
        memset(obj, 0, sizeof(*obj));
 
+       state->obj = obj;
+
+       kref_init(&state->ref);
+
        obj->state = state;
        obj->funcs = funcs;
        list_add_tail(&obj->head, &dev_priv->global_obj_list);
@@ -28,7 +54,9 @@ void intel_atomic_global_obj_cleanup(struct drm_i915_private *dev_priv)
 
        list_for_each_entry_safe(obj, next, &dev_priv->global_obj_list, head) {
                list_del(&obj->head);
-               obj->funcs->atomic_destroy_state(obj, obj->state);
+
+               drm_WARN_ON(&dev_priv->drm, kref_read(&obj->state->ref) != 1);
+               intel_atomic_global_state_put(obj->state);
        }
 }
 
@@ -97,10 +125,14 @@ intel_atomic_get_global_obj_state(struct intel_atomic_state *state,
        if (!obj_state)
                return ERR_PTR(-ENOMEM);
 
+       obj_state->obj = obj;
        obj_state->changed = false;
 
+       kref_init(&obj_state->ref);
+
        state->global_objs[index].state = obj_state;
-       state->global_objs[index].old_state = obj->state;
+       state->global_objs[index].old_state =
+               intel_atomic_global_state_get(obj->state);
        state->global_objs[index].new_state = obj_state;
        state->global_objs[index].ptr = obj;
        obj_state->state = state;
@@ -163,7 +195,9 @@ void intel_atomic_swap_global_state(struct intel_atomic_state *state)
                new_obj_state->state = NULL;
 
                state->global_objs[i].state = old_obj_state;
-               obj->state = new_obj_state;
+
+               intel_atomic_global_state_put(obj->state);
+               obj->state = intel_atomic_global_state_get(new_obj_state);
        }
 }
 
@@ -172,10 +206,9 @@ void intel_atomic_clear_global_state(struct intel_atomic_state *state)
        int i;
 
        for (i = 0; i < state->num_global_objs; i++) {
-               struct intel_global_obj *obj = state->global_objs[i].ptr;
+               intel_atomic_global_state_put(state->global_objs[i].old_state);
+               intel_atomic_global_state_put(state->global_objs[i].new_state);
 
-               obj->funcs->atomic_destroy_state(obj,
-                                                state->global_objs[i].state);
                state->global_objs[i].ptr = NULL;
                state->global_objs[i].state = NULL;
                state->global_objs[i].old_state = NULL;

diff --git a/drivers/gpu/drm/i915/display/intel_global_state.h b/drivers/gpu/drm/i915/display/intel_global_state.h
index e6163a4690296678f9bc425a9deb8bb46b60fb04..1f16fa3073c9fcead38d38122697f83209ae7bf1 100644 (file)
--- a/drivers/gpu/drm/i915/display/intel_global_state.h
+++ b/drivers/gpu/drm/i915/display/intel_global_state.h
@@ -6,6 +6,7 @@
 #ifndef __INTEL_GLOBAL_STATE_H__
 #define __INTEL_GLOBAL_STATE_H__
 
+#include <linux/kref.h>
 #include <linux/list.h>
 
 struct drm_i915_private;
@@ -54,7 +55,9 @@ struct intel_global_obj {
                for_each_if(obj)
 
 struct intel_global_state {
+       struct intel_global_obj *obj;
        struct intel_atomic_state *state;
+       struct kref ref;
        bool changed;
 };