[PATCH v2 0/2] Fixes for dma-buf locking issues found by Smatch

[PATCH v2 0/2] Fixes for dma-buf locking issues found by Smatch

[Dmitry Osipenko]

Hello,

Here are the two patches fixing minor problems introduced by my
"dma-buf locking convention" series. Thanks to Dan Carpenter who
checked linux-next with Smatch and reported the found issues.

Changelog:

v2: - Added ack from Christian König to the dma-buf patch and improved
      the dma-buf/attachment pointer check.

    - Dropped v1 patch that added GEM NULL-checking to
      drm_gem_vunmap_unlocked() and replaced it with the NULL-checking
      in drm_client_buffer_delete(), like was suggested by Christian König.

Dmitry Osipenko (2):
  dma-buf: Make locking consistent in dma_buf_detach()
  drm/client: Prevent NULL dereference in drm_client_buffer_delete()

 drivers/dma-buf/dma-buf.c    | 4 ++--
 drivers/gpu/drm/drm_client.c | 6 +++---
 2 files changed, 5 insertions(+), 5 deletions(-)

-- 
2.37.3

[PATCH v2 1/2] dma-buf: Make locking consistent in dma_buf_detach()

[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>
Reviewed-by: Christian König <christian.koenig@amd.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>
---
 drivers/dma-buf/dma-buf.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
index c40d72d318fd..13bfd2d09c56 100644
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -995,10 +995,10 @@ static void __unmap_dma_buf(struct dma_buf_attachment *attach,
  */
 void dma_buf_detach(struct dma_buf *dmabuf, struct dma_buf_attachment *attach)
 {
-	if (WARN_ON(!dmabuf || !attach))
+	if (WARN_ON(!dmabuf || !attach || dmabuf != attach->dmabuf))
 		return;
 
-	dma_resv_lock(attach->dmabuf->resv, NULL);
+	dma_resv_lock(dmabuf->resv, NULL);
 
 	if (attach->sgt) {
 
-- 
2.37.3

[PATCH v2 2/2] drm/client: Prevent NULL dereference in drm_client_buffer_delete()

[Dmitry Osipenko]

The drm_gem_vunmap() will crash with a NULL dereference if the passed
object pointer is NULL. It wasn't a problem before we added the locking
support to drm_gem_vunmap function because the mapping argument was always
NULL together with the object. Make drm_client_buffer_delete() to check
whether GEM is NULL before trying to unmap the GEM, it will happen on
framebuffer creation error.

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Link: https://lore.kernel.org/dri-devel/Y1kFEGxT8MVlf32V@kili/
Fixes: 79e2cf2e7a19 ("drm/gem: Take reservation lock for vmap/vunmap operations")
Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
---
 drivers/gpu/drm/drm_client.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c
index 38e1be991caa..fd67efe37c63 100644
--- a/drivers/gpu/drm/drm_client.c
+++ b/drivers/gpu/drm/drm_client.c
@@ -235,10 +235,10 @@ static void drm_client_buffer_delete(struct drm_client_buffer *buffer)
 {
 	struct drm_device *dev = buffer->client->dev;
 
-	drm_gem_vunmap_unlocked(buffer->gem, &buffer->map);
-
-	if (buffer->gem)
+	if (buffer->gem) {
+		drm_gem_vunmap_unlocked(buffer->gem, &buffer->map);
 		drm_gem_object_put(buffer->gem);
+	}
 
 	if (buffer->handle)
 		drm_mode_destroy_dumb(dev, buffer->handle, buffer->client->file);
-- 
2.37.3

Re: [PATCH v2 2/2] drm/client: Prevent NULL dereference in drm_client_buffer_delete()

[Christian König]

Am 30.10.22 um 16:44 schrieb Dmitry Osipenko:
> The drm_gem_vunmap() will crash with a NULL dereference if the passed
> object pointer is NULL. It wasn't a problem before we added the locking
> support to drm_gem_vunmap function because the mapping argument was always
> NULL together with the object. Make drm_client_buffer_delete() to check
> whether GEM is NULL before trying to unmap the GEM, it will happen on
> framebuffer creation error.
>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Link: https://lore.kernel.org/dri-devel/Y1kFEGxT8MVlf32V@kili/
> Fixes: 79e2cf2e7a19 ("drm/gem: Take reservation lock for vmap/vunmap operations")
> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>

Reviewed-by: Christian König <christian.koenig@amd.com>

> ---
>   drivers/gpu/drm/drm_client.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c
> index 38e1be991caa..fd67efe37c63 100644
> --- a/drivers/gpu/drm/drm_client.c
> +++ b/drivers/gpu/drm/drm_client.c
> @@ -235,10 +235,10 @@ static void drm_client_buffer_delete(struct drm_client_buffer *buffer)
>   {
>   	struct drm_device *dev = buffer->client->dev;
>   
> -	drm_gem_vunmap_unlocked(buffer->gem, &buffer->map);
> -
> -	if (buffer->gem)
> +	if (buffer->gem) {
> +		drm_gem_vunmap_unlocked(buffer->gem, &buffer->map);
>   		drm_gem_object_put(buffer->gem);
> +	}
>   
>   	if (buffer->handle)
>   		drm_mode_destroy_dumb(dev, buffer->handle, buffer->client->file);

Re: [PATCH v2 2/2] drm/client: Prevent NULL dereference in drm_client_buffer_delete()

[Dmitry Osipenko]

On 11/2/22 12:15, Christian König wrote:
> Am 30.10.22 um 16:44 schrieb Dmitry Osipenko:
>> The drm_gem_vunmap() will crash with a NULL dereference if the passed
>> object pointer is NULL. It wasn't a problem before we added the locking
>> support to drm_gem_vunmap function because the mapping argument was
>> always
>> NULL together with the object. Make drm_client_buffer_delete() to check
>> whether GEM is NULL before trying to unmap the GEM, it will happen on
>> framebuffer creation error.
>>
>> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
>> Link: https://lore.kernel.org/dri-devel/Y1kFEGxT8MVlf32V@kili/
>> Fixes: 79e2cf2e7a19 ("drm/gem: Take reservation lock for vmap/vunmap
>> operations")
>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> 
> Reviewed-by: Christian König <christian.koenig@amd.com>

Applied to drm-misc-next, thanks!

-- 
Best regards,
Dmitry