diff mbox series

drm/sched: Fix drm_sched_fence_free() so it can be passed an uninitialized fence

Message ID 20210903120554.444101-1-boris.brezillon@collabora.com (mailing list archive)
State New, archived
Headers show
Series drm/sched: Fix drm_sched_fence_free() so it can be passed an uninitialized fence | expand

Commit Message

Boris Brezillon Sept. 3, 2021, 12:05 p.m. UTC
drm_sched_job_cleanup() will pass an uninitialized fence to
drm_sched_fence_free(), which will cause to_drm_sched_fence() to return
a NULL fence object, causing a NULL pointer deref when this NULL object
is passed to kmem_cache_free().

Let's create a new drm_sched_fence_free() function that takes a
drm_sched_fence pointer and suffix the old function with _rcu. While at
it, complain if drm_sched_fence_free() is passed an initialized fence
or if drm_sched_fence_free_rcu() is passed an uninitialized fence.

Fixes: dbe48d030b28 ("drm/sched: Split drm_sched_job_init")
Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
Found while debugging another issue in panfrost causing a failure in
the submit ioctl and exercising the error path (path that calls
drm_sched_job_cleanup() on an unarmed job).
---
 drivers/gpu/drm/scheduler/sched_fence.c | 29 ++++++++++++++++---------
 drivers/gpu/drm/scheduler/sched_main.c  |  2 +-
 include/drm/gpu_scheduler.h             |  2 +-
 3 files changed, 21 insertions(+), 12 deletions(-)

Comments

Daniel Vetter Sept. 8, 2021, 5:15 p.m. UTC | #1
On Fri, Sep 03, 2021 at 02:05:54PM +0200, Boris Brezillon wrote:
> drm_sched_job_cleanup() will pass an uninitialized fence to
> drm_sched_fence_free(), which will cause to_drm_sched_fence() to return
> a NULL fence object, causing a NULL pointer deref when this NULL object
> is passed to kmem_cache_free().
> 
> Let's create a new drm_sched_fence_free() function that takes a
> drm_sched_fence pointer and suffix the old function with _rcu. While at
> it, complain if drm_sched_fence_free() is passed an initialized fence
> or if drm_sched_fence_free_rcu() is passed an uninitialized fence.
> 
> Fixes: dbe48d030b28 ("drm/sched: Split drm_sched_job_init")
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> ---
> Found while debugging another issue in panfrost causing a failure in
> the submit ioctl and exercising the error path (path that calls
> drm_sched_job_cleanup() on an unarmed job).

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

I already provided an irc r-b, just here for the record too.
-Daniel

> ---
>  drivers/gpu/drm/scheduler/sched_fence.c | 29 ++++++++++++++++---------
>  drivers/gpu/drm/scheduler/sched_main.c  |  2 +-
>  include/drm/gpu_scheduler.h             |  2 +-
>  3 files changed, 21 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/scheduler/sched_fence.c b/drivers/gpu/drm/scheduler/sched_fence.c
> index db3fd1303fc4..7fd869520ef2 100644
> --- a/drivers/gpu/drm/scheduler/sched_fence.c
> +++ b/drivers/gpu/drm/scheduler/sched_fence.c
> @@ -69,19 +69,28 @@ static const char *drm_sched_fence_get_timeline_name(struct dma_fence *f)
>  	return (const char *)fence->sched->name;
>  }
>  
> -/**
> - * drm_sched_fence_free - free up the fence memory
> - *
> - * @rcu: RCU callback head
> - *
> - * Free up the fence memory after the RCU grace period.
> - */
> -void drm_sched_fence_free(struct rcu_head *rcu)
> +static void drm_sched_fence_free_rcu(struct rcu_head *rcu)
>  {
>  	struct dma_fence *f = container_of(rcu, struct dma_fence, rcu);
>  	struct drm_sched_fence *fence = to_drm_sched_fence(f);
>  
> -	kmem_cache_free(sched_fence_slab, fence);
> +	if (!WARN_ON_ONCE(!fence))
> +		kmem_cache_free(sched_fence_slab, fence);
> +}
> +
> +/**
> + * drm_sched_fence_free - free up an uninitialized fence
> + *
> + * @fence: fence to free
> + *
> + * Free up the fence memory. Should only be used if drm_sched_fence_init()
> + * has not been called yet.
> + */
> +void drm_sched_fence_free(struct drm_sched_fence *fence)
> +{
> +	/* This function should not be called if the fence has been initialized. */
> +	if (!WARN_ON_ONCE(fence->sched))
> +		kmem_cache_free(sched_fence_slab, fence);
>  }
>  
>  /**
> @@ -97,7 +106,7 @@ static void drm_sched_fence_release_scheduled(struct dma_fence *f)
>  	struct drm_sched_fence *fence = to_drm_sched_fence(f);
>  
>  	dma_fence_put(fence->parent);
> -	call_rcu(&fence->finished.rcu, drm_sched_fence_free);
> +	call_rcu(&fence->finished.rcu, drm_sched_fence_free_rcu);
>  }
>  
>  /**
> diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
> index fbbd3b03902f..6987d412a946 100644
> --- a/drivers/gpu/drm/scheduler/sched_main.c
> +++ b/drivers/gpu/drm/scheduler/sched_main.c
> @@ -750,7 +750,7 @@ void drm_sched_job_cleanup(struct drm_sched_job *job)
>  		dma_fence_put(&job->s_fence->finished);
>  	} else {
>  		/* aborted job before committing to run it */
> -		drm_sched_fence_free(&job->s_fence->finished.rcu);
> +		drm_sched_fence_free(job->s_fence);
>  	}
>  
>  	job->s_fence = NULL;
> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
> index 7f77a455722c..f011e4c407f2 100644
> --- a/include/drm/gpu_scheduler.h
> +++ b/include/drm/gpu_scheduler.h
> @@ -509,7 +509,7 @@ struct drm_sched_fence *drm_sched_fence_alloc(
>  	struct drm_sched_entity *s_entity, void *owner);
>  void drm_sched_fence_init(struct drm_sched_fence *fence,
>  			  struct drm_sched_entity *entity);
> -void drm_sched_fence_free(struct rcu_head *rcu);
> +void drm_sched_fence_free(struct drm_sched_fence *fence);
>  
>  void drm_sched_fence_scheduled(struct drm_sched_fence *fence);
>  void drm_sched_fence_finished(struct drm_sched_fence *fence);
> -- 
> 2.31.1
>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/scheduler/sched_fence.c b/drivers/gpu/drm/scheduler/sched_fence.c
index db3fd1303fc4..7fd869520ef2 100644
--- a/drivers/gpu/drm/scheduler/sched_fence.c
+++ b/drivers/gpu/drm/scheduler/sched_fence.c
@@ -69,19 +69,28 @@  static const char *drm_sched_fence_get_timeline_name(struct dma_fence *f)
 	return (const char *)fence->sched->name;
 }
 
-/**
- * drm_sched_fence_free - free up the fence memory
- *
- * @rcu: RCU callback head
- *
- * Free up the fence memory after the RCU grace period.
- */
-void drm_sched_fence_free(struct rcu_head *rcu)
+static void drm_sched_fence_free_rcu(struct rcu_head *rcu)
 {
 	struct dma_fence *f = container_of(rcu, struct dma_fence, rcu);
 	struct drm_sched_fence *fence = to_drm_sched_fence(f);
 
-	kmem_cache_free(sched_fence_slab, fence);
+	if (!WARN_ON_ONCE(!fence))
+		kmem_cache_free(sched_fence_slab, fence);
+}
+
+/**
+ * drm_sched_fence_free - free up an uninitialized fence
+ *
+ * @fence: fence to free
+ *
+ * Free up the fence memory. Should only be used if drm_sched_fence_init()
+ * has not been called yet.
+ */
+void drm_sched_fence_free(struct drm_sched_fence *fence)
+{
+	/* This function should not be called if the fence has been initialized. */
+	if (!WARN_ON_ONCE(fence->sched))
+		kmem_cache_free(sched_fence_slab, fence);
 }
 
 /**
@@ -97,7 +106,7 @@  static void drm_sched_fence_release_scheduled(struct dma_fence *f)
 	struct drm_sched_fence *fence = to_drm_sched_fence(f);
 
 	dma_fence_put(fence->parent);
-	call_rcu(&fence->finished.rcu, drm_sched_fence_free);
+	call_rcu(&fence->finished.rcu, drm_sched_fence_free_rcu);
 }
 
 /**
diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
index fbbd3b03902f..6987d412a946 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -750,7 +750,7 @@  void drm_sched_job_cleanup(struct drm_sched_job *job)
 		dma_fence_put(&job->s_fence->finished);
 	} else {
 		/* aborted job before committing to run it */
-		drm_sched_fence_free(&job->s_fence->finished.rcu);
+		drm_sched_fence_free(job->s_fence);
 	}
 
 	job->s_fence = NULL;
diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
index 7f77a455722c..f011e4c407f2 100644
--- a/include/drm/gpu_scheduler.h
+++ b/include/drm/gpu_scheduler.h
@@ -509,7 +509,7 @@  struct drm_sched_fence *drm_sched_fence_alloc(
 	struct drm_sched_entity *s_entity, void *owner);
 void drm_sched_fence_init(struct drm_sched_fence *fence,
 			  struct drm_sched_entity *entity);
-void drm_sched_fence_free(struct rcu_head *rcu);
+void drm_sched_fence_free(struct drm_sched_fence *fence);
 
 void drm_sched_fence_scheduled(struct drm_sched_fence *fence);
 void drm_sched_fence_finished(struct drm_sched_fence *fence);