From: Dmitry Osipenko <dmitry.osipenko@collabora.com> To: "Christian König" <christian.koenig@amd.com>, "Maarten Lankhorst" <maarten.lankhorst@linux.intel.com>, "Maxime Ripard" <mripard@kernel.org>, "Sumit Semwal" <sumit.semwal@linaro.org>, "Thomas Zimmermann" <tzimmermann@suse.de>, "David Airlie" <airlied@gmail.com>, "Daniel Vetter" <daniel@ffwll.ch>, noralf@tronnes.org, "Dan Carpenter" <dan.carpenter@oracle.com> Cc: dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v1 1/2] dma-buf: Make locking consistent in dma_buf_detach() Date: Thu, 27 Oct 2022 13:34:56 +0300 [thread overview] Message-ID: <46e22491-5891-cd24-850e-699fadb284ee@collabora.com> (raw) In-Reply-To: <01f62e6c-a40b-42e4-6cb0-338bd268b0a5@amd.com> On 10/27/22 09:13, Christian König wrote: > Am 27.10.22 um 00:46 schrieb Dmitry Osipenko: >> The dma_buf_detach() locks attach->dmabuf->resv and then unlocks >> dmabuf->resv, which could be a two different locks from a static >> code checker perspective. In particular this triggers Smatch to >> report the "double unlock" error. Make the locking pointers consistent. >> >> Reported-by: Dan Carpenter <dan.carpenter@oracle.com> >> Link: https://lore.kernel.org/dri-devel/Y1fLfsccW3AS%2Fo+%2F@kili/ >> Fixes: 809d9c72c2f8 ("dma-buf: Move dma_buf_attach() to dynamic >> locking specification") >> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> > > It would be even cleaner if we completely drop the dmabuf parameter for > the function and just use the inside the attachment. > > Anyway patch is Reviewed-by: Christian König <christian.koenig@amd.com> > for now, wider cleanups can come later on. I had the same thought about dropping the dmabuf parameter. Looking at this patch again, perhaps a better dmabuf sanity-check will be: - if (WARN_ON(!dmabuf || !attach)) + if (WARN_ON(!dmabuf || !attach || dmabuf != attach->dmabuf)) I'll switch to this version in v2, if there are no objections. > >> --- >> drivers/dma-buf/dma-buf.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c >> index c40d72d318fd..6e33ef4fde34 100644 >> --- a/drivers/dma-buf/dma-buf.c >> +++ b/drivers/dma-buf/dma-buf.c >> @@ -998,9 +998,10 @@ void dma_buf_detach(struct dma_buf *dmabuf, >> struct dma_buf_attachment *attach) >> if (WARN_ON(!dmabuf || !attach)) >> return; >> - dma_resv_lock(attach->dmabuf->resv, NULL); >> + dma_resv_lock(dmabuf->resv, NULL); >> if (attach->sgt) { >> + WARN_ON(dmabuf != attach->dmabuf); >> __unmap_dma_buf(attach, attach->sgt, attach->dir); >> > -- Best regards, Dmitry
WARNING: multiple messages have this Message-ID (diff)
From: Dmitry Osipenko <dmitry.osipenko@collabora.com> To: "Christian König" <christian.koenig@amd.com>, "Maarten Lankhorst" <maarten.lankhorst@linux.intel.com>, "Maxime Ripard" <mripard@kernel.org>, "Sumit Semwal" <sumit.semwal@linaro.org>, "Thomas Zimmermann" <tzimmermann@suse.de>, "David Airlie" <airlied@gmail.com>, "Daniel Vetter" <daniel@ffwll.ch>, noralf@tronnes.org, "Dan Carpenter" <dan.carpenter@oracle.com> Cc: linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org Subject: Re: [PATCH v1 1/2] dma-buf: Make locking consistent in dma_buf_detach() Date: Thu, 27 Oct 2022 13:34:56 +0300 [thread overview] Message-ID: <46e22491-5891-cd24-850e-699fadb284ee@collabora.com> (raw) In-Reply-To: <01f62e6c-a40b-42e4-6cb0-338bd268b0a5@amd.com> On 10/27/22 09:13, Christian König wrote: > Am 27.10.22 um 00:46 schrieb Dmitry Osipenko: >> The dma_buf_detach() locks attach->dmabuf->resv and then unlocks >> dmabuf->resv, which could be a two different locks from a static >> code checker perspective. In particular this triggers Smatch to >> report the "double unlock" error. Make the locking pointers consistent. >> >> Reported-by: Dan Carpenter <dan.carpenter@oracle.com> >> Link: https://lore.kernel.org/dri-devel/Y1fLfsccW3AS%2Fo+%2F@kili/ >> Fixes: 809d9c72c2f8 ("dma-buf: Move dma_buf_attach() to dynamic >> locking specification") >> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> > > It would be even cleaner if we completely drop the dmabuf parameter for > the function and just use the inside the attachment. > > Anyway patch is Reviewed-by: Christian König <christian.koenig@amd.com> > for now, wider cleanups can come later on. I had the same thought about dropping the dmabuf parameter. Looking at this patch again, perhaps a better dmabuf sanity-check will be: - if (WARN_ON(!dmabuf || !attach)) + if (WARN_ON(!dmabuf || !attach || dmabuf != attach->dmabuf)) I'll switch to this version in v2, if there are no objections. > >> --- >> drivers/dma-buf/dma-buf.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c >> index c40d72d318fd..6e33ef4fde34 100644 >> --- a/drivers/dma-buf/dma-buf.c >> +++ b/drivers/dma-buf/dma-buf.c >> @@ -998,9 +998,10 @@ void dma_buf_detach(struct dma_buf *dmabuf, >> struct dma_buf_attachment *attach) >> if (WARN_ON(!dmabuf || !attach)) >> return; >> - dma_resv_lock(attach->dmabuf->resv, NULL); >> + dma_resv_lock(dmabuf->resv, NULL); >> if (attach->sgt) { >> + WARN_ON(dmabuf != attach->dmabuf); >> __unmap_dma_buf(attach, attach->sgt, attach->dir); >> > -- Best regards, Dmitry
next prev parent reply other threads:[~2022-10-27 10:35 UTC|newest] Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-10-26 22:46 [PATCH v1 0/2] Fixes for dma-buf locking issues found by Smatch Dmitry Osipenko 2022-10-26 22:46 ` Dmitry Osipenko 2022-10-26 22:46 ` [PATCH v1 1/2] dma-buf: Make locking consistent in dma_buf_detach() Dmitry Osipenko 2022-10-26 22:46 ` Dmitry Osipenko 2022-10-27 6:13 ` Christian König 2022-10-27 6:13 ` Christian König 2022-10-27 10:34 ` Dmitry Osipenko [this message] 2022-10-27 10:34 ` Dmitry Osipenko 2022-10-26 22:46 ` [PATCH v1 2/2] drm/gem: Check whether object is NULL in drm_gem_vunmap() Dmitry Osipenko 2022-10-26 22:46 ` Dmitry Osipenko 2022-10-27 6:17 ` Christian König 2022-10-27 6:17 ` Christian König 2022-10-27 7:28 ` Dan Carpenter 2022-10-27 7:28 ` Dan Carpenter 2022-10-27 7:40 ` Christian König 2022-10-27 7:40 ` Christian König
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=46e22491-5891-cd24-850e-699fadb284ee@collabora.com \ --to=dmitry.osipenko@collabora.com \ --cc=airlied@gmail.com \ --cc=christian.koenig@amd.com \ --cc=dan.carpenter@oracle.com \ --cc=daniel@ffwll.ch \ --cc=dri-devel@lists.freedesktop.org \ --cc=linux-kernel@vger.kernel.org \ --cc=maarten.lankhorst@linux.intel.com \ --cc=mripard@kernel.org \ --cc=noralf@tronnes.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: linkBe 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.