| Message ID | 1574099545-20430-1-git-send-email-andrey.grodzovsky@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drm/scheduler: Avoid accessing freed bad job. | expand |
Am 18.11.19 um 18:52 schrieb Andrey Grodzovsky: > Problem: > Due to a race between drm_sched_cleanup_jobs in sched thread and > drm_sched_job_timedout in timeout work there is a possiblity that > bad job was already freed while still being accessed from the > timeout thread. > > Fix: > Instead of just peeking at the bad job in the mirror list > remove it from the list under lock and then put it back later when > we are garanteed no race with main sched thread is possible which > is after the thread is parked. > > v2: Lock around processing ring_mirror_list in drm_sched_cleanup_jobs. > > Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com> Please rebase on top of the changes in drm-misc-next. Apart from that one minor coding style nit pick below. > --- > drivers/gpu/drm/scheduler/sched_main.c | 44 +++++++++++++++++++++++++++++----- > 1 file changed, 38 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c > index 80ddbdf..b05b210 100644 > --- a/drivers/gpu/drm/scheduler/sched_main.c > +++ b/drivers/gpu/drm/scheduler/sched_main.c > @@ -287,10 +287,24 @@ static void drm_sched_job_timedout(struct work_struct *work) > unsigned long flags; > > sched = container_of(work, struct drm_gpu_scheduler, work_tdr.work); > + > + /* > + * Protects against concurrent deletion in drm_sched_cleanup_jobs that > + * is already in progress. > + */ > + spin_lock_irqsave(&sched->job_list_lock, flags); > job = list_first_entry_or_null(&sched->ring_mirror_list, > struct drm_sched_job, node); > > if (job) { > + /* > + * Remove the bad job so it cannot be freed by already in progress > + * drm_sched_cleanup_jobs. It will be reinsrted back after sched->thread > + * is parked at which point it's safe. > + */ > + list_del_init(&job->node); > + spin_unlock_irqrestore(&sched->job_list_lock, flags); > + > job->sched->ops->timedout_job(job); > > /* > @@ -302,6 +316,8 @@ static void drm_sched_job_timedout(struct work_struct *work) > sched->free_guilty = false; > } > } > + else Please use "} else {" instead, we have automated tests which complain about that now. Christian. > + spin_unlock_irqrestore(&sched->job_list_lock, flags); > > spin_lock_irqsave(&sched->job_list_lock, flags); > drm_sched_start_timeout(sched); > @@ -373,6 +389,19 @@ void drm_sched_stop(struct drm_gpu_scheduler *sched, struct drm_sched_job *bad) > kthread_park(sched->thread); > > /* > + * Reinsert back the bad job here - now it's safe as drm_sched_cleanup_jobs > + * cannot race against us and release the bad job at this point - we parked > + * (waited for) any in progress (earlier) cleanups and any later ones will > + * bail out due to sched->thread being parked. > + */ > + if (bad && bad->sched == sched) > + /* > + * Add at the head of the queue to reflect it was the earliest > + * job extracted. > + */ > + list_add(&bad->node, &sched->ring_mirror_list); > + > + /* > * Iterate the job list from later to earlier one and either deactive > * their HW callbacks or remove them from mirror list if they already > * signaled. > @@ -656,16 +685,19 @@ static void drm_sched_cleanup_jobs(struct drm_gpu_scheduler *sched) > __kthread_should_park(sched->thread)) > return; > > - > - while (!list_empty(&sched->ring_mirror_list)) { > + /* See drm_sched_job_timedout for why the locking is here */ > + while (true) { > struct drm_sched_job *job; > > - job = list_first_entry(&sched->ring_mirror_list, > - struct drm_sched_job, node); > - if (!dma_fence_is_signaled(&job->s_fence->finished)) > + spin_lock_irqsave(&sched->job_list_lock, flags); > + job = list_first_entry_or_null(&sched->ring_mirror_list, > + struct drm_sched_job, node); > + > + if (!job || !dma_fence_is_signaled(&job->s_fence->finished)) { > + spin_unlock_irqrestore(&sched->job_list_lock, flags); > break; > + } > > - spin_lock_irqsave(&sched->job_list_lock, flags); > /* remove job from ring_mirror_list */ > list_del_init(&job->node); > spin_unlock_irqrestore(&sched->job_list_lock, flags);
Tested-by: Emily Deng <Emily.Deng@amd.com> >-----Original Message----- >From: Andrey Grodzovsky <andrey.grodzovsky@amd.com> >Sent: Tuesday, November 19, 2019 1:52 AM >Cc: dri-devel@lists.freedesktop.org; amd-gfx@lists.freedesktop.org; Koenig, >Christian <Christian.Koenig@amd.com>; Deng, Emily ><Emily.Deng@amd.com>; Grodzovsky, Andrey ><Andrey.Grodzovsky@amd.com> >Subject: [PATCH v2] drm/scheduler: Avoid accessing freed bad job. > >Problem: >Due to a race between drm_sched_cleanup_jobs in sched thread and >drm_sched_job_timedout in timeout work there is a possiblity that bad job >was already freed while still being accessed from the timeout thread. > >Fix: >Instead of just peeking at the bad job in the mirror list remove it from the list >under lock and then put it back later when we are garanteed no race with >main sched thread is possible which is after the thread is parked. > >v2: Lock around processing ring_mirror_list in drm_sched_cleanup_jobs. > >Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com> >--- > drivers/gpu/drm/scheduler/sched_main.c | 44 >+++++++++++++++++++++++++++++----- > 1 file changed, 38 insertions(+), 6 deletions(-) > >diff --git a/drivers/gpu/drm/scheduler/sched_main.c >b/drivers/gpu/drm/scheduler/sched_main.c >index 80ddbdf..b05b210 100644 >--- a/drivers/gpu/drm/scheduler/sched_main.c >+++ b/drivers/gpu/drm/scheduler/sched_main.c >@@ -287,10 +287,24 @@ static void drm_sched_job_timedout(struct >work_struct *work) > unsigned long flags; > > sched = container_of(work, struct drm_gpu_scheduler, >work_tdr.work); >+ >+ /* >+ * Protects against concurrent deletion in drm_sched_cleanup_jobs >that >+ * is already in progress. >+ */ >+ spin_lock_irqsave(&sched->job_list_lock, flags); > job = list_first_entry_or_null(&sched->ring_mirror_list, > struct drm_sched_job, node); > > if (job) { >+ /* >+ * Remove the bad job so it cannot be freed by already in >progress >+ * drm_sched_cleanup_jobs. It will be reinsrted back after >sched->thread >+ * is parked at which point it's safe. >+ */ >+ list_del_init(&job->node); >+ spin_unlock_irqrestore(&sched->job_list_lock, flags); >+ > job->sched->ops->timedout_job(job); > > /* >@@ -302,6 +316,8 @@ static void drm_sched_job_timedout(struct >work_struct *work) > sched->free_guilty = false; > } > } >+ else >+ spin_unlock_irqrestore(&sched->job_list_lock, flags); > > spin_lock_irqsave(&sched->job_list_lock, flags); > drm_sched_start_timeout(sched); >@@ -373,6 +389,19 @@ void drm_sched_stop(struct drm_gpu_scheduler >*sched, struct drm_sched_job *bad) > kthread_park(sched->thread); > > /* >+ * Reinsert back the bad job here - now it's safe as >drm_sched_cleanup_jobs >+ * cannot race against us and release the bad job at this point - we >parked >+ * (waited for) any in progress (earlier) cleanups and any later ones will >+ * bail out due to sched->thread being parked. >+ */ >+ if (bad && bad->sched == sched) >+ /* >+ * Add at the head of the queue to reflect it was the earliest >+ * job extracted. >+ */ >+ list_add(&bad->node, &sched->ring_mirror_list); >+ >+ /* > * Iterate the job list from later to earlier one and either deactive > * their HW callbacks or remove them from mirror list if they already > * signaled. >@@ -656,16 +685,19 @@ static void drm_sched_cleanup_jobs(struct >drm_gpu_scheduler *sched) > __kthread_should_park(sched->thread)) > return; > >- >- while (!list_empty(&sched->ring_mirror_list)) { >+ /* See drm_sched_job_timedout for why the locking is here */ >+ while (true) { > struct drm_sched_job *job; > >- job = list_first_entry(&sched->ring_mirror_list, >- struct drm_sched_job, node); >- if (!dma_fence_is_signaled(&job->s_fence->finished)) >+ spin_lock_irqsave(&sched->job_list_lock, flags); >+ job = list_first_entry_or_null(&sched->ring_mirror_list, >+ struct drm_sched_job, node); >+ >+ if (!job || !dma_fence_is_signaled(&job->s_fence->finished)) { >+ spin_unlock_irqrestore(&sched->job_list_lock, flags); > break; >+ } > >- spin_lock_irqsave(&sched->job_list_lock, flags); > /* remove job from ring_mirror_list */ > list_del_init(&job->node); > spin_unlock_irqrestore(&sched->job_list_lock, flags); >-- >2.7.4
diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c index 80ddbdf..b05b210 100644 --- a/drivers/gpu/drm/scheduler/sched_main.c +++ b/drivers/gpu/drm/scheduler/sched_main.c @@ -287,10 +287,24 @@ static void drm_sched_job_timedout(struct work_struct *work) unsigned long flags; sched = container_of(work, struct drm_gpu_scheduler, work_tdr.work); + + /* + * Protects against concurrent deletion in drm_sched_cleanup_jobs that + * is already in progress. + */ + spin_lock_irqsave(&sched->job_list_lock, flags); job = list_first_entry_or_null(&sched->ring_mirror_list, struct drm_sched_job, node); if (job) { + /* + * Remove the bad job so it cannot be freed by already in progress + * drm_sched_cleanup_jobs. It will be reinsrted back after sched->thread + * is parked at which point it's safe. + */ + list_del_init(&job->node); + spin_unlock_irqrestore(&sched->job_list_lock, flags); + job->sched->ops->timedout_job(job); /* @@ -302,6 +316,8 @@ static void drm_sched_job_timedout(struct work_struct *work) sched->free_guilty = false; } } + else + spin_unlock_irqrestore(&sched->job_list_lock, flags); spin_lock_irqsave(&sched->job_list_lock, flags); drm_sched_start_timeout(sched); @@ -373,6 +389,19 @@ void drm_sched_stop(struct drm_gpu_scheduler *sched, struct drm_sched_job *bad) kthread_park(sched->thread); /* + * Reinsert back the bad job here - now it's safe as drm_sched_cleanup_jobs + * cannot race against us and release the bad job at this point - we parked + * (waited for) any in progress (earlier) cleanups and any later ones will + * bail out due to sched->thread being parked. + */ + if (bad && bad->sched == sched) + /* + * Add at the head of the queue to reflect it was the earliest + * job extracted. + */ + list_add(&bad->node, &sched->ring_mirror_list); + + /* * Iterate the job list from later to earlier one and either deactive * their HW callbacks or remove them from mirror list if they already * signaled. @@ -656,16 +685,19 @@ static void drm_sched_cleanup_jobs(struct drm_gpu_scheduler *sched) __kthread_should_park(sched->thread)) return; - - while (!list_empty(&sched->ring_mirror_list)) { + /* See drm_sched_job_timedout for why the locking is here */ + while (true) { struct drm_sched_job *job; - job = list_first_entry(&sched->ring_mirror_list, - struct drm_sched_job, node); - if (!dma_fence_is_signaled(&job->s_fence->finished)) + spin_lock_irqsave(&sched->job_list_lock, flags); + job = list_first_entry_or_null(&sched->ring_mirror_list, + struct drm_sched_job, node); + + if (!job || !dma_fence_is_signaled(&job->s_fence->finished)) { + spin_unlock_irqrestore(&sched->job_list_lock, flags); break; + } - spin_lock_irqsave(&sched->job_list_lock, flags); /* remove job from ring_mirror_list */ list_del_init(&job->node); spin_unlock_irqrestore(&sched->job_list_lock, flags);
Problem: Due to a race between drm_sched_cleanup_jobs in sched thread and drm_sched_job_timedout in timeout work there is a possiblity that bad job was already freed while still being accessed from the timeout thread. Fix: Instead of just peeking at the bad job in the mirror list remove it from the list under lock and then put it back later when we are garanteed no race with main sched thread is possible which is after the thread is parked. v2: Lock around processing ring_mirror_list in drm_sched_cleanup_jobs. Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com> --- drivers/gpu/drm/scheduler/sched_main.c | 44 +++++++++++++++++++++++++++++----- 1 file changed, 38 insertions(+), 6 deletions(-)