| 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 |
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 --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);
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(-)