linux-kernel - Re: [PATCH] drm/msm: Fix hw

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <a7aa6e21-6f79-0029-5034-db368239980a@linaro.org>
Date:   Thu, 13 Jul 2023 23:03:17 +0300
From:   Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To:     Rob Clark <robdclark@...il.com>, dri-devel@...ts.freedesktop.org
Cc:     linux-arm-msm@...r.kernel.org, freedreno@...ts.freedesktop.org,
        Rob Clark <robdclark@...omium.org>,
        Abhinav Kumar <quic_abhinavk@...cinc.com>,
        Sean Paul <sean@...rly.run>,
        Marijn Suijten <marijn.suijten@...ainline.org>,
        David Airlie <airlied@...il.com>,
        Daniel Vetter <daniel@...ll.ch>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] drm/msm: Fix hw_fence error path cleanup

On 13/07/2023 01:25, Rob Clark wrote:
> From: Rob Clark <robdclark@...omium.org>
> 
> In an error path where the submit is free'd without the job being run,
> the hw_fence pointer is simply a kzalloc'd block of memory.  In this
> case we should just kfree() it, rather than trying to decrement it's
> reference count.  Fortunately we can tell that this is the case by
> checking for a zero refcount, since if the job was run, the submit would
> be holding a reference to the hw_fence.
> 
> Fixes: f94e6a51e17c ("drm/msm: Pre-allocate hw_fence")
> Signed-off-by: Rob Clark <robdclark@...omium.org>
> ---
>   drivers/gpu/drm/msm/msm_fence.c      |  6 ++++++
>   drivers/gpu/drm/msm/msm_gem_submit.c | 14 +++++++++++++-
>   2 files changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/msm/msm_fence.c b/drivers/gpu/drm/msm/msm_fence.c
> index 96599ec3eb78..1a5d4f1c8b42 100644
> --- a/drivers/gpu/drm/msm/msm_fence.c
> +++ b/drivers/gpu/drm/msm/msm_fence.c
> @@ -191,6 +191,12 @@ msm_fence_init(struct dma_fence *fence, struct msm_fence_context *fctx)
>   
>   	f->fctx = fctx;
>   
> +	/*
> +	 * Until this point, the fence was just some pre-allocated memory,
> +	 * no-one should have taken a reference to it yet.
> +	 */
> +	WARN_ON(kref_read(&fence->refcount));

It this really correct to return a refcounted object with 0 refcount 
(I'm looking at submit_create() / msm_fence_alloc() )? Maybe it would be 
better to move dma_fence_get() to msm_fence_alloc() ? But don't 
immediately see, which one should be moved.

> +
>   	dma_fence_init(&f->base, &msm_fence_ops, &fctx->spinlock,
>   		       fctx->context, ++fctx->last_fence);
>   }
> diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c
> index 3f1aa4de3b87..9d66498cdc04 100644
> --- a/drivers/gpu/drm/msm/msm_gem_submit.c
> +++ b/drivers/gpu/drm/msm/msm_gem_submit.c
> @@ -86,7 +86,19 @@ void __msm_gem_submit_destroy(struct kref *kref)
>   	}
>   
>   	dma_fence_put(submit->user_fence);
> -	dma_fence_put(submit->hw_fence);
> +
> +	/*
> +	 * If the submit is freed before msm_job_run(), then hw_fence is
> +	 * just some pre-allocated memory, not a reference counted fence.
> +	 * Once the job runs and the hw_fence is initialized, it will
> +	 * have a refcount of at least one, since the submit holds a ref
> +	 * to the hw_fence.
> +	 */
> +	if (kref_read(&submit->hw_fence->refcount) == 0) {
> +		kfree(submit->hw_fence);
> +	} else {
> +		dma_fence_put(submit->hw_fence);
> +	}
>   
>   	put_pid(submit->pid);
>   	msm_submitqueue_put(submit->queue);

-- 
With best wishes
Dmitry