Commit f8c86ffa authored by Ville Syrjälä's avatar Ville Syrjälä

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: 0ef1905e ("drm/i915: Introduce better global state handling")
Signed-off-by: default avatarVille Syrjälä <ville.syrjala@linux.intel.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20200527200245.13184-1-ville.syrjala@linux.intel.comReviewed-by: default avatarStanislav Lisovskiy <stanislav.lisovskiy@intel.com>
parent 03c10f47
...@@ -10,6 +10,28 @@ ...@@ -10,6 +10,28 @@
#include "intel_display_types.h" #include "intel_display_types.h"
#include "intel_global_state.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, void intel_atomic_global_obj_init(struct drm_i915_private *dev_priv,
struct intel_global_obj *obj, struct intel_global_obj *obj,
struct intel_global_state *state, struct intel_global_state *state,
...@@ -17,6 +39,10 @@ void intel_atomic_global_obj_init(struct drm_i915_private *dev_priv, ...@@ -17,6 +39,10 @@ void intel_atomic_global_obj_init(struct drm_i915_private *dev_priv,
{ {
memset(obj, 0, sizeof(*obj)); memset(obj, 0, sizeof(*obj));
state->obj = obj;
kref_init(&state->ref);
obj->state = state; obj->state = state;
obj->funcs = funcs; obj->funcs = funcs;
list_add_tail(&obj->head, &dev_priv->global_obj_list); 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) ...@@ -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_for_each_entry_safe(obj, next, &dev_priv->global_obj_list, head) {
list_del(&obj->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, ...@@ -97,10 +125,14 @@ intel_atomic_get_global_obj_state(struct intel_atomic_state *state,
if (!obj_state) if (!obj_state)
return ERR_PTR(-ENOMEM); return ERR_PTR(-ENOMEM);
obj_state->obj = obj;
obj_state->changed = false; obj_state->changed = false;
kref_init(&obj_state->ref);
state->global_objs[index].state = obj_state; 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].new_state = obj_state;
state->global_objs[index].ptr = obj; state->global_objs[index].ptr = obj;
obj_state->state = state; obj_state->state = state;
...@@ -163,7 +195,9 @@ void intel_atomic_swap_global_state(struct intel_atomic_state *state) ...@@ -163,7 +195,9 @@ void intel_atomic_swap_global_state(struct intel_atomic_state *state)
new_obj_state->state = NULL; new_obj_state->state = NULL;
state->global_objs[i].state = old_obj_state; 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) ...@@ -172,10 +206,9 @@ void intel_atomic_clear_global_state(struct intel_atomic_state *state)
int i; int i;
for (i = 0; i < state->num_global_objs; 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].ptr = NULL;
state->global_objs[i].state = NULL; state->global_objs[i].state = NULL;
state->global_objs[i].old_state = NULL; state->global_objs[i].old_state = NULL;
......
...@@ -6,6 +6,7 @@ ...@@ -6,6 +6,7 @@
#ifndef __INTEL_GLOBAL_STATE_H__ #ifndef __INTEL_GLOBAL_STATE_H__
#define __INTEL_GLOBAL_STATE_H__ #define __INTEL_GLOBAL_STATE_H__
#include <linux/kref.h>
#include <linux/list.h> #include <linux/list.h>
struct drm_i915_private; struct drm_i915_private;
...@@ -54,7 +55,9 @@ struct intel_global_obj { ...@@ -54,7 +55,9 @@ struct intel_global_obj {
for_each_if(obj) for_each_if(obj)
struct intel_global_state { struct intel_global_state {
struct intel_global_obj *obj;
struct intel_atomic_state *state; struct intel_atomic_state *state;
struct kref ref;
bool changed; bool changed;
}; };
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment