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
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 <[email protected]>
Reviewed-by: Christian König <[email protected]>
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 <[email protected]>
---
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
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 <[email protected]>
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 <[email protected]>
---
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
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 <[email protected]>
> 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 <[email protected]>
Reviewed-by: Christian König <[email protected]>
> ---
> 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);
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 <[email protected]>
>> 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 <[email protected]>
>
> Reviewed-by: Christian König <[email protected]>
Applied to drm-misc-next, thanks!
--
Best regards,
Dmitry