From: "Christian König" <christian.koenig@amd.com>
To: Wentao_Liang <Wentao_Liang_g@163.com>, maarten.lankhorst@linux.intel.com
Cc: mripard@kernel.org, tzimmermann@suse.de, airlied@linux.ie,
daniel@ffwll.ch, sumit.semwal@linaro.org,
dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org,
linux-media@vger.kernel.org, linaro-mm-sig@lists.linaro.org
Subject: Re: [PATCH] drm/prime: fix a potential double put (release) bug
Date: Wed, 18 Aug 2021 15:25:59 +0200 [thread overview]
Message-ID: <14aa6dfe-faba-8632-01a4-8119f199005c@amd.com> (raw)
In-Reply-To: <20210818130231.3484-1-Wentao_Liang_g@163.com>
Am 18.08.21 um 15:02 schrieb Wentao_Liang:
> In line 317 (#1), drm_gem_prime_import() is called, it will call
> drm_gem_prime_import_dev(). At the end of the function
> drm_gem_prime_import_dev() (line 956, #2), "dma_buf_put(dma_buf);" puts
> dma_buf->file and may cause it to be released. However, after
> drm_gem_prime_import() returning, the dma_buf may be put again by the
> same put function in lines 342, 351 and 358 (#3, #4, #5). Putting the
> dma_buf improperly more than once can lead to an incorrect dma_buf-
>> file put.
> We believe that the put of the dma_buf in the function
> drm_gem_prime_import() is unnecessary (#2). We can fix the above bug by
> removing the redundant "dma_buf_put(dma_buf);" in line 956.
Guys I'm getting tired of NAKing those incorrect reference count analysis.
The dma_buf_put() in the error handling of drm_gem_prime_import_dev()
function is balanced with the get_dma_buf() in the same function
directly above.
This is for the creating a GEM object for a DMA-buf imported from other
device use case and certainly correct.
The various dma_buf_put() in drm_gem_prime_fd_to_handle() is balanced
with the dma_buf_get(prime_fd) at the beginning of the function.
This is for extracting the DMA-buf from the file descriptor and keeping
a reference to it while we are busy importing it (e.g. to prevent a race
when somebody changes the fd at the same time).
As far as I can see this is correct as well.
Regards,
Christian.
>
> 314 if (dev->driver->gem_prime_import)
> 315 obj = dev->driver->gem_prime_import(dev, dma_buf);
> 316 else
> 317 obj = drm_gem_prime_import(dev, dma_buf);
> //#1 call to drm_gem_prime_import
> // ->drm_gem_prime_import_dev
> // ->dma_buf_put
> ...
>
> 336 ret = drm_prime_add_buf_handle(&file_priv->prime,
> 337 dma_buf, *handle);
>
> ...
>
> 342 dma_buf_put(dma_buf); //#3 put again
> 343
> 344 return 0;
> 345
> 346 fail:
>
> 351 dma_buf_put(dma_buf); //#4 put again
> 352 return ret;
>
> 356 out_put:
> 357 mutex_unlock(&file_priv->prime.lock);
> 358 dma_buf_put(dma_buf); //#5 put again
> 359 return ret;
> 360 }
>
> 905 struct drm_gem_object *drm_gem_prime_import_dev
> (struct drm_device *dev,
> 906 struct dma_buf *dma_buf,
> 907 struct device *attach_dev)
> 908 {
>
> ...
>
> 952 fail_unmap:
> 953 dma_buf_unmap_attachment(attach, sgt, DMA_BIDIRECTIONAL);
> 954 fail_detach:
> 955 dma_buf_detach(dma_buf, attach);
> 956 dma_buf_put(dma_buf); //#2 the first put of dma_buf
> // (unnecessary)
> 957
> 958 return ERR_PTR(ret);
> 959 }
>
> Signed-off-by: Wentao_Liang <Wentao_Liang_g@163.com>
> ---
> drivers/gpu/drm/drm_prime.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> index 2a54f86856af..cef03ad0d5cd 100644
> --- a/drivers/gpu/drm/drm_prime.c
> +++ b/drivers/gpu/drm/drm_prime.c
> @@ -953,7 +953,6 @@ struct drm_gem_object *drm_gem_prime_import_dev(struct drm_device *dev,
> dma_buf_unmap_attachment(attach, sgt, DMA_BIDIRECTIONAL);
> fail_detach:
> dma_buf_detach(dma_buf, attach);
> - dma_buf_put(dma_buf);
>
> return ERR_PTR(ret);
> }
next prev parent reply other threads:[~2021-08-18 13:26 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-08-18 13:02 [PATCH] drm/prime: fix a potential double put (release) bug Wentao_Liang
2021-08-18 13:25 ` Christian König [this message]
2021-08-18 14:07 ` Daniel Vetter
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=14aa6dfe-faba-8632-01a4-8119f199005c@amd.com \
--to=christian.koenig@amd.com \
--cc=Wentao_Liang_g@163.com \
--cc=airlied@linux.ie \
--cc=daniel@ffwll.ch \
--cc=dri-devel@lists.freedesktop.org \
--cc=linaro-mm-sig@lists.linaro.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=maarten.lankhorst@linux.intel.com \
--cc=mripard@kernel.org \
--cc=sumit.semwal@linaro.org \
--cc=tzimmermann@suse.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.