diff mbox series

[1/1] Revert "UBUNTU: SAUCE: drm/i915: Synchronize active and retire callbacks"

Message ID 20201113044731.570886-2-kai.heng.feng@canonical.com
State New
Headers show
Series Fix noise lines on i915 + Xorg | expand

Commit Message

Kai-Heng Feng Nov. 13, 2020, 4:47 a.m. UTC
BugLink: https://bugs.launchpad.net/bugs/1896091

This reverts commit c1484c993957fe6b366ab453fbb7b50c0d2916de.

Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
 drivers/gpu/drm/i915/i915_active.c       | 52 ++++--------------------
 drivers/gpu/drm/i915/i915_active.h       | 10 +++--
 drivers/gpu/drm/i915/i915_active_types.h |  2 -
 3 files changed, 14 insertions(+), 50 deletions(-)

Comments

Stefan Bader Nov. 13, 2020, 9:15 a.m. UTC | #1
On 13.11.20 05:47, Kai-Heng Feng wrote:
> BugLink: https://bugs.launchpad.net/bugs/1896091
> 
> This reverts commit c1484c993957fe6b366ab453fbb7b50c0d2916de.
> 
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
Acked-by: Stefan Bader <stefan.bader@canonical.com>
> ---

Ok, with the additional info that there was a revert of this and an update for
it in Focal (just the revert was not done forward, and we probably should think
about how we could avoid this to happen again) it sounds less dangerous. You
want to make sure that this info goes into the bug reports regression potential,
too.

-Stefan

>  drivers/gpu/drm/i915/i915_active.c       | 52 ++++--------------------
>  drivers/gpu/drm/i915/i915_active.h       | 10 +++--
>  drivers/gpu/drm/i915/i915_active_types.h |  2 -
>  3 files changed, 14 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_active.c b/drivers/gpu/drm/i915/i915_active.c
> index 768713657e85..d960d0be5bd2 100644
> --- a/drivers/gpu/drm/i915/i915_active.c
> +++ b/drivers/gpu/drm/i915/i915_active.c
> @@ -148,22 +148,8 @@ __active_retire(struct i915_active *ref)
>  	spin_unlock_irqrestore(&ref->tree_lock, flags);
>  
>  	/* After the final retire, the entire struct may be freed */
> -	if (ref->retire) {
> -		if (ref->active) {
> -			bool freed = false;
> -
> -			/* Don't race with the active callback, and avoid UaF */
> -			down_write(&ref->rwsem);
> -			ref->freed = &freed;
> -			ref->retire(ref);
> -			if (!freed) {
> -				ref->freed = NULL;
> -				up_write(&ref->rwsem);
> -			}
> -		} else {
> -			ref->retire(ref);
> -		}
> -	}
> +	if (ref->retire)
> +		ref->retire(ref);
>  
>  	/* ... except if you wait on it, you must manage your own references! */
>  	wake_up_var(ref);
> @@ -293,8 +279,7 @@ void __i915_active_init(struct i915_active *ref,
>  			int (*active)(struct i915_active *ref),
>  			void (*retire)(struct i915_active *ref),
>  			struct lock_class_key *mkey,
> -			struct lock_class_key *wkey,
> -			struct lock_class_key *rkey)
> +			struct lock_class_key *wkey)
>  {
>  	unsigned long bits;
>  
> @@ -303,13 +288,8 @@ void __i915_active_init(struct i915_active *ref,
>  	ref->flags = 0;
>  	ref->active = active;
>  	ref->retire = ptr_unpack_bits(retire, &bits, 2);
> -	ref->freed = NULL;
> -	if (ref->active && ref->retire) {
> -		__init_rwsem(&ref->rwsem, "i915_active.rwsem", rkey);
> +	if (bits & I915_ACTIVE_MAY_SLEEP)
>  		ref->flags |= I915_ACTIVE_RETIRE_SLEEPS;
> -	} else if (bits & I915_ACTIVE_MAY_SLEEP) {
> -		ref->flags |= I915_ACTIVE_RETIRE_SLEEPS;
> -	}
>  
>  	spin_lock_init(&ref->tree_lock);
>  	ref->tree = RB_ROOT;
> @@ -448,20 +428,8 @@ int i915_active_acquire(struct i915_active *ref)
>  		return err;
>  
>  	if (likely(!i915_active_acquire_if_busy(ref))) {
> -		if (ref->active) {
> -			if (ref->retire) {
> -				/*
> -				 * This can be a recursive call, and the mutex
> -				 * above already protects from concurrent active
> -				 * callbacks, so a read lock fits best.
> -				 */
> -				down_read(&ref->rwsem);
> -				err = ref->active(ref);
> -				up_read(&ref->rwsem);
> -			} else {
> -				err = ref->active(ref);
> -			}
> -		}
> +		if (ref->active)
> +			err = ref->active(ref);
>  		if (!err) {
>  			spin_lock_irq(&ref->tree_lock); /* __active_retire() */
>  			debug_active_activate(ref);
> @@ -683,20 +651,16 @@ int i915_sw_fence_await_active(struct i915_sw_fence *fence,
>  	return await_active(ref, flags, sw_await_fence, fence, fence);
>  }
>  
> +#if IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM)
>  void i915_active_fini(struct i915_active *ref)
>  {
> -	if (ref->freed) {
> -		*ref->freed = true;
> -		up_write(&ref->rwsem);
> -	}
> -#if IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM)
>  	debug_active_fini(ref);
>  	GEM_BUG_ON(atomic_read(&ref->count));
>  	GEM_BUG_ON(work_pending(&ref->work));
>  	GEM_BUG_ON(!RB_EMPTY_ROOT(&ref->tree));
>  	mutex_destroy(&ref->mutex);
> -#endif
>  }
> +#endif
>  
>  static inline bool is_idle_barrier(struct active_node *node, u64 idx)
>  {
> diff --git a/drivers/gpu/drm/i915/i915_active.h b/drivers/gpu/drm/i915/i915_active.h
> index a2e3742b1090..cf4058150966 100644
> --- a/drivers/gpu/drm/i915/i915_active.h
> +++ b/drivers/gpu/drm/i915/i915_active.h
> @@ -153,16 +153,14 @@ void __i915_active_init(struct i915_active *ref,
>  			int (*active)(struct i915_active *ref),
>  			void (*retire)(struct i915_active *ref),
>  			struct lock_class_key *mkey,
> -			struct lock_class_key *wkey,
> -			struct lock_class_key *rkey);
> +			struct lock_class_key *wkey);
>  
>  /* Specialise each class of i915_active to avoid impossible lockdep cycles. */
>  #define i915_active_init(ref, active, retire) do {		\
>  	static struct lock_class_key __mkey;				\
>  	static struct lock_class_key __wkey;				\
> -	static struct lock_class_key __rkey;				\
>  									\
> -	__i915_active_init(ref, active, retire, &__mkey, &__wkey, &__rkey);	\
> +	__i915_active_init(ref, active, retire, &__mkey, &__wkey);	\
>  } while (0)
>  
>  int i915_active_ref(struct i915_active *ref,
> @@ -215,7 +213,11 @@ i915_active_is_idle(const struct i915_active *ref)
>  	return !atomic_read(&ref->count);
>  }
>  
> +#if IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM)
>  void i915_active_fini(struct i915_active *ref);
> +#else
> +static inline void i915_active_fini(struct i915_active *ref) { }
> +#endif
>  
>  int i915_active_acquire_preallocate_barrier(struct i915_active *ref,
>  					    struct intel_engine_cs *engine);
> diff --git a/drivers/gpu/drm/i915/i915_active_types.h b/drivers/gpu/drm/i915/i915_active_types.h
> index aaee2548cb19..6360c3e4b765 100644
> --- a/drivers/gpu/drm/i915/i915_active_types.h
> +++ b/drivers/gpu/drm/i915/i915_active_types.h
> @@ -32,8 +32,6 @@ struct active_node;
>  struct i915_active {
>  	atomic_t count;
>  	struct mutex mutex;
> -	struct rw_semaphore rwsem;
> -	bool *freed;
>  
>  	spinlock_t tree_lock;
>  	struct active_node *cache;
>
Kleber Souza Nov. 20, 2020, 10:09 a.m. UTC | #2
On 13.11.20 05:47, Kai-Heng Feng wrote:
> BugLink: https://bugs.launchpad.net/bugs/1896091
> 
> This reverts commit c1484c993957fe6b366ab453fbb7b50c0d2916de.
> 
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>

I'm also affected by this and it's pretty annoying. Thanks for finding
a fix!

Acked-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com>


> ---
>   drivers/gpu/drm/i915/i915_active.c       | 52 ++++--------------------
>   drivers/gpu/drm/i915/i915_active.h       | 10 +++--
>   drivers/gpu/drm/i915/i915_active_types.h |  2 -
>   3 files changed, 14 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_active.c b/drivers/gpu/drm/i915/i915_active.c
> index 768713657e85..d960d0be5bd2 100644
> --- a/drivers/gpu/drm/i915/i915_active.c
> +++ b/drivers/gpu/drm/i915/i915_active.c
> @@ -148,22 +148,8 @@ __active_retire(struct i915_active *ref)
>   	spin_unlock_irqrestore(&ref->tree_lock, flags);
>   
>   	/* After the final retire, the entire struct may be freed */
> -	if (ref->retire) {
> -		if (ref->active) {
> -			bool freed = false;
> -
> -			/* Don't race with the active callback, and avoid UaF */
> -			down_write(&ref->rwsem);
> -			ref->freed = &freed;
> -			ref->retire(ref);
> -			if (!freed) {
> -				ref->freed = NULL;
> -				up_write(&ref->rwsem);
> -			}
> -		} else {
> -			ref->retire(ref);
> -		}
> -	}
> +	if (ref->retire)
> +		ref->retire(ref);
>   
>   	/* ... except if you wait on it, you must manage your own references! */
>   	wake_up_var(ref);
> @@ -293,8 +279,7 @@ void __i915_active_init(struct i915_active *ref,
>   			int (*active)(struct i915_active *ref),
>   			void (*retire)(struct i915_active *ref),
>   			struct lock_class_key *mkey,
> -			struct lock_class_key *wkey,
> -			struct lock_class_key *rkey)
> +			struct lock_class_key *wkey)
>   {
>   	unsigned long bits;
>   
> @@ -303,13 +288,8 @@ void __i915_active_init(struct i915_active *ref,
>   	ref->flags = 0;
>   	ref->active = active;
>   	ref->retire = ptr_unpack_bits(retire, &bits, 2);
> -	ref->freed = NULL;
> -	if (ref->active && ref->retire) {
> -		__init_rwsem(&ref->rwsem, "i915_active.rwsem", rkey);
> +	if (bits & I915_ACTIVE_MAY_SLEEP)
>   		ref->flags |= I915_ACTIVE_RETIRE_SLEEPS;
> -	} else if (bits & I915_ACTIVE_MAY_SLEEP) {
> -		ref->flags |= I915_ACTIVE_RETIRE_SLEEPS;
> -	}
>   
>   	spin_lock_init(&ref->tree_lock);
>   	ref->tree = RB_ROOT;
> @@ -448,20 +428,8 @@ int i915_active_acquire(struct i915_active *ref)
>   		return err;
>   
>   	if (likely(!i915_active_acquire_if_busy(ref))) {
> -		if (ref->active) {
> -			if (ref->retire) {
> -				/*
> -				 * This can be a recursive call, and the mutex
> -				 * above already protects from concurrent active
> -				 * callbacks, so a read lock fits best.
> -				 */
> -				down_read(&ref->rwsem);
> -				err = ref->active(ref);
> -				up_read(&ref->rwsem);
> -			} else {
> -				err = ref->active(ref);
> -			}
> -		}
> +		if (ref->active)
> +			err = ref->active(ref);
>   		if (!err) {
>   			spin_lock_irq(&ref->tree_lock); /* __active_retire() */
>   			debug_active_activate(ref);
> @@ -683,20 +651,16 @@ int i915_sw_fence_await_active(struct i915_sw_fence *fence,
>   	return await_active(ref, flags, sw_await_fence, fence, fence);
>   }
>   
> +#if IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM)
>   void i915_active_fini(struct i915_active *ref)
>   {
> -	if (ref->freed) {
> -		*ref->freed = true;
> -		up_write(&ref->rwsem);
> -	}
> -#if IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM)
>   	debug_active_fini(ref);
>   	GEM_BUG_ON(atomic_read(&ref->count));
>   	GEM_BUG_ON(work_pending(&ref->work));
>   	GEM_BUG_ON(!RB_EMPTY_ROOT(&ref->tree));
>   	mutex_destroy(&ref->mutex);
> -#endif
>   }
> +#endif
>   
>   static inline bool is_idle_barrier(struct active_node *node, u64 idx)
>   {
> diff --git a/drivers/gpu/drm/i915/i915_active.h b/drivers/gpu/drm/i915/i915_active.h
> index a2e3742b1090..cf4058150966 100644
> --- a/drivers/gpu/drm/i915/i915_active.h
> +++ b/drivers/gpu/drm/i915/i915_active.h
> @@ -153,16 +153,14 @@ void __i915_active_init(struct i915_active *ref,
>   			int (*active)(struct i915_active *ref),
>   			void (*retire)(struct i915_active *ref),
>   			struct lock_class_key *mkey,
> -			struct lock_class_key *wkey,
> -			struct lock_class_key *rkey);
> +			struct lock_class_key *wkey);
>   
>   /* Specialise each class of i915_active to avoid impossible lockdep cycles. */
>   #define i915_active_init(ref, active, retire) do {		\
>   	static struct lock_class_key __mkey;				\
>   	static struct lock_class_key __wkey;				\
> -	static struct lock_class_key __rkey;				\
>   									\
> -	__i915_active_init(ref, active, retire, &__mkey, &__wkey, &__rkey);	\
> +	__i915_active_init(ref, active, retire, &__mkey, &__wkey);	\
>   } while (0)
>   
>   int i915_active_ref(struct i915_active *ref,
> @@ -215,7 +213,11 @@ i915_active_is_idle(const struct i915_active *ref)
>   	return !atomic_read(&ref->count);
>   }
>   
> +#if IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM)
>   void i915_active_fini(struct i915_active *ref);
> +#else
> +static inline void i915_active_fini(struct i915_active *ref) { }
> +#endif
>   
>   int i915_active_acquire_preallocate_barrier(struct i915_active *ref,
>   					    struct intel_engine_cs *engine);
> diff --git a/drivers/gpu/drm/i915/i915_active_types.h b/drivers/gpu/drm/i915/i915_active_types.h
> index aaee2548cb19..6360c3e4b765 100644
> --- a/drivers/gpu/drm/i915/i915_active_types.h
> +++ b/drivers/gpu/drm/i915/i915_active_types.h
> @@ -32,8 +32,6 @@ struct active_node;
>   struct i915_active {
>   	atomic_t count;
>   	struct mutex mutex;
> -	struct rw_semaphore rwsem;
> -	bool *freed;
>   
>   	spinlock_t tree_lock;
>   	struct active_node *cache;
>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/i915_active.c b/drivers/gpu/drm/i915/i915_active.c
index 768713657e85..d960d0be5bd2 100644
--- a/drivers/gpu/drm/i915/i915_active.c
+++ b/drivers/gpu/drm/i915/i915_active.c
@@ -148,22 +148,8 @@  __active_retire(struct i915_active *ref)
 	spin_unlock_irqrestore(&ref->tree_lock, flags);
 
 	/* After the final retire, the entire struct may be freed */
-	if (ref->retire) {
-		if (ref->active) {
-			bool freed = false;
-
-			/* Don't race with the active callback, and avoid UaF */
-			down_write(&ref->rwsem);
-			ref->freed = &freed;
-			ref->retire(ref);
-			if (!freed) {
-				ref->freed = NULL;
-				up_write(&ref->rwsem);
-			}
-		} else {
-			ref->retire(ref);
-		}
-	}
+	if (ref->retire)
+		ref->retire(ref);
 
 	/* ... except if you wait on it, you must manage your own references! */
 	wake_up_var(ref);
@@ -293,8 +279,7 @@  void __i915_active_init(struct i915_active *ref,
 			int (*active)(struct i915_active *ref),
 			void (*retire)(struct i915_active *ref),
 			struct lock_class_key *mkey,
-			struct lock_class_key *wkey,
-			struct lock_class_key *rkey)
+			struct lock_class_key *wkey)
 {
 	unsigned long bits;
 
@@ -303,13 +288,8 @@  void __i915_active_init(struct i915_active *ref,
 	ref->flags = 0;
 	ref->active = active;
 	ref->retire = ptr_unpack_bits(retire, &bits, 2);
-	ref->freed = NULL;
-	if (ref->active && ref->retire) {
-		__init_rwsem(&ref->rwsem, "i915_active.rwsem", rkey);
+	if (bits & I915_ACTIVE_MAY_SLEEP)
 		ref->flags |= I915_ACTIVE_RETIRE_SLEEPS;
-	} else if (bits & I915_ACTIVE_MAY_SLEEP) {
-		ref->flags |= I915_ACTIVE_RETIRE_SLEEPS;
-	}
 
 	spin_lock_init(&ref->tree_lock);
 	ref->tree = RB_ROOT;
@@ -448,20 +428,8 @@  int i915_active_acquire(struct i915_active *ref)
 		return err;
 
 	if (likely(!i915_active_acquire_if_busy(ref))) {
-		if (ref->active) {
-			if (ref->retire) {
-				/*
-				 * This can be a recursive call, and the mutex
-				 * above already protects from concurrent active
-				 * callbacks, so a read lock fits best.
-				 */
-				down_read(&ref->rwsem);
-				err = ref->active(ref);
-				up_read(&ref->rwsem);
-			} else {
-				err = ref->active(ref);
-			}
-		}
+		if (ref->active)
+			err = ref->active(ref);
 		if (!err) {
 			spin_lock_irq(&ref->tree_lock); /* __active_retire() */
 			debug_active_activate(ref);
@@ -683,20 +651,16 @@  int i915_sw_fence_await_active(struct i915_sw_fence *fence,
 	return await_active(ref, flags, sw_await_fence, fence, fence);
 }
 
+#if IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM)
 void i915_active_fini(struct i915_active *ref)
 {
-	if (ref->freed) {
-		*ref->freed = true;
-		up_write(&ref->rwsem);
-	}
-#if IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM)
 	debug_active_fini(ref);
 	GEM_BUG_ON(atomic_read(&ref->count));
 	GEM_BUG_ON(work_pending(&ref->work));
 	GEM_BUG_ON(!RB_EMPTY_ROOT(&ref->tree));
 	mutex_destroy(&ref->mutex);
-#endif
 }
+#endif
 
 static inline bool is_idle_barrier(struct active_node *node, u64 idx)
 {
diff --git a/drivers/gpu/drm/i915/i915_active.h b/drivers/gpu/drm/i915/i915_active.h
index a2e3742b1090..cf4058150966 100644
--- a/drivers/gpu/drm/i915/i915_active.h
+++ b/drivers/gpu/drm/i915/i915_active.h
@@ -153,16 +153,14 @@  void __i915_active_init(struct i915_active *ref,
 			int (*active)(struct i915_active *ref),
 			void (*retire)(struct i915_active *ref),
 			struct lock_class_key *mkey,
-			struct lock_class_key *wkey,
-			struct lock_class_key *rkey);
+			struct lock_class_key *wkey);
 
 /* Specialise each class of i915_active to avoid impossible lockdep cycles. */
 #define i915_active_init(ref, active, retire) do {		\
 	static struct lock_class_key __mkey;				\
 	static struct lock_class_key __wkey;				\
-	static struct lock_class_key __rkey;				\
 									\
-	__i915_active_init(ref, active, retire, &__mkey, &__wkey, &__rkey);	\
+	__i915_active_init(ref, active, retire, &__mkey, &__wkey);	\
 } while (0)
 
 int i915_active_ref(struct i915_active *ref,
@@ -215,7 +213,11 @@  i915_active_is_idle(const struct i915_active *ref)
 	return !atomic_read(&ref->count);
 }
 
+#if IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM)
 void i915_active_fini(struct i915_active *ref);
+#else
+static inline void i915_active_fini(struct i915_active *ref) { }
+#endif
 
 int i915_active_acquire_preallocate_barrier(struct i915_active *ref,
 					    struct intel_engine_cs *engine);
diff --git a/drivers/gpu/drm/i915/i915_active_types.h b/drivers/gpu/drm/i915/i915_active_types.h
index aaee2548cb19..6360c3e4b765 100644
--- a/drivers/gpu/drm/i915/i915_active_types.h
+++ b/drivers/gpu/drm/i915/i915_active_types.h
@@ -32,8 +32,6 @@  struct active_node;
 struct i915_active {
 	atomic_t count;
 	struct mutex mutex;
-	struct rw_semaphore rwsem;
-	bool *freed;
 
 	spinlock_t tree_lock;
 	struct active_node *cache;