[PATCH v2] drm/scheduler: Avoid accessing freed bad job.

[Christian König]

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