Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 8 Oct 2018 15:46:01 +0200
From:   Boris Brezillon <boris.brezillon@bootlin.com>
To:     Eric Anholt <eric@anholt.net>
Cc:     dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/4] drm/v3d: Fix a use-after-free race accessing the
 scheduler's fences.
Message-ID: <20181008154601.21077f67@bbrezillon>
In-Reply-To: <20180928232126.4332-1-eric@anholt.net>
References: <20180928232126.4332-1-eric@anholt.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, 28 Sep 2018 16:21:23 -0700
Eric Anholt <eric@anholt.net> wrote:

> Once we push the job, the scheduler could run it and free it.  So, if
> we want to reference their fences, we need to grab them before then.
> I haven't seen this happen in many days of conformance test runtime,
> but let's still close the race.
> 
> Signed-off-by: Eric Anholt <eric@anholt.net>
> Fixes: 57692c94dcbe ("drm/v3d: Introduce a new DRM driver for Broadcom V3D V3.x+")

Reviewed-by: Boris Brezillon <boris.brezillon@bootlin.com>

> ---
>  drivers/gpu/drm/v3d/v3d_drv.h | 5 +++++
>  drivers/gpu/drm/v3d/v3d_gem.c | 8 ++++++--
>  2 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/v3d/v3d_drv.h b/drivers/gpu/drm/v3d/v3d_drv.h
> index 5042573e97f4..83c55ab6e1c0 100644
> --- a/drivers/gpu/drm/v3d/v3d_drv.h
> +++ b/drivers/gpu/drm/v3d/v3d_drv.h
> @@ -204,6 +204,11 @@ struct v3d_exec_info {
>  	 */
>  	struct dma_fence *bin_done_fence;
>  
> +	/* Fence for when the scheduler considers the render to be
> +	 * done, for when the BOs reservations should be complete.
> +	 */
> +	struct dma_fence *render_done_fence;
> +
>  	struct kref refcount;
>  
>  	/* This is the array of BOs that were looked up at the start of exec. */
> diff --git a/drivers/gpu/drm/v3d/v3d_gem.c b/drivers/gpu/drm/v3d/v3d_gem.c
> index e1fcbb4cd0ae..c98fbfbdb68e 100644
> --- a/drivers/gpu/drm/v3d/v3d_gem.c
> +++ b/drivers/gpu/drm/v3d/v3d_gem.c
> @@ -209,7 +209,7 @@ v3d_flush_caches(struct v3d_dev *v3d)
>  static void
>  v3d_attach_object_fences(struct v3d_exec_info *exec)
>  {
> -	struct dma_fence *out_fence = &exec->render.base.s_fence->finished;
> +	struct dma_fence *out_fence = exec->render_done_fence;
>  	struct v3d_bo *bo;
>  	int i;
>  
> @@ -409,6 +409,7 @@ v3d_exec_cleanup(struct kref *ref)
>  	dma_fence_put(exec->render.done_fence);
>  
>  	dma_fence_put(exec->bin_done_fence);
> +	dma_fence_put(exec->render_done_fence);
>  
>  	for (i = 0; i < exec->bo_count; i++)
>  		drm_gem_object_put_unlocked(&exec->bo[i]->base);
> @@ -574,6 +575,9 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data,
>  	if (ret)
>  		goto fail_unreserve;
>  
> +	exec->render_done_fence =
> +		dma_fence_get(&exec->render.base.s_fence->finished);
> +
>  	kref_get(&exec->refcount); /* put by scheduler job completion */
>  	drm_sched_entity_push_job(&exec->render.base,
>  				  &v3d_priv->sched_entity[V3D_RENDER]);
> @@ -587,7 +591,7 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data,
>  	sync_out = drm_syncobj_find(file_priv, args->out_sync);
>  	if (sync_out) {
>  		drm_syncobj_replace_fence(sync_out,
> -					  &exec->render.base.s_fence->finished);
> +					  exec->render_done_fence);
>  		drm_syncobj_put(sync_out);
>  	}
>