[PATCH 1/2] drm/ttm: fix locking in vmap/vunmap TTM GEM helpers

[PATCH 1/2] drm/ttm: fix locking in vmap/vunmap TTM GEM helpers

[Christian König]

I've stumbled over this while reviewing patches for DMA-buf and it looks
like we completely messed the locking up here.

In general most TTM function should only be called while holding the
appropriate BO resv lock. Without this we could break the internal
buffer object state here.

Only compile tested!

Signed-off-by: Christian König <christian.koenig@amd.com>
Fixes: 43676605f890 drm/ttm: Add vmap/vunmap to TTM and TTM GEM helpers
---
 drivers/gpu/drm/drm_gem_ttm_helper.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_gem_ttm_helper.c b/drivers/gpu/drm/drm_gem_ttm_helper.c
index d5962a34c01d..e5fc875990c4 100644
--- a/drivers/gpu/drm/drm_gem_ttm_helper.c
+++ b/drivers/gpu/drm/drm_gem_ttm_helper.c
@@ -64,8 +64,13 @@ int drm_gem_ttm_vmap(struct drm_gem_object *gem,
 		     struct iosys_map *map)
 {
 	struct ttm_buffer_object *bo = drm_gem_ttm_of_gem(gem);
+	int ret;
+
+	dma_resv_lock(gem->resv, NULL);
+	ret = ttm_bo_vmap(bo, map);
+	dma_resv_unlock(gem->resv);
 
-	return ttm_bo_vmap(bo, map);
+	return ret;
 }
 EXPORT_SYMBOL(drm_gem_ttm_vmap);
 
@@ -82,7 +87,9 @@ void drm_gem_ttm_vunmap(struct drm_gem_object *gem,
 {
 	struct ttm_buffer_object *bo = drm_gem_ttm_of_gem(gem);
 
+	dma_resv_lock(gem->resv, NULL);
 	ttm_bo_vunmap(bo, map);
+	dma_resv_unlock(gem->resv);
 }
 EXPORT_SYMBOL(drm_gem_ttm_vunmap);
 
-- 
2.25.1

[PATCH 2/2] drm/ttm: add dma_resv_assert_held() calls to vmap/vunmap

[Christian König]

Let's make sure nobody is calling those functions without holding the
appropriate locks.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/ttm/ttm_bo_util.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c b/drivers/gpu/drm/ttm/ttm_bo_util.c
index 1530982338e9..497ee1fdbad7 100644
--- a/drivers/gpu/drm/ttm/ttm_bo_util.c
+++ b/drivers/gpu/drm/ttm/ttm_bo_util.c
@@ -402,6 +402,8 @@ int ttm_bo_vmap(struct ttm_buffer_object *bo, struct iosys_map *map)
 	struct ttm_resource *mem = bo->resource;
 	int ret;
 
+	dma_resv_assert_held(bo->base.resv);
+
 	ret = ttm_mem_io_reserve(bo->bdev, mem);
 	if (ret)
 		return ret;
@@ -460,6 +462,8 @@ void ttm_bo_vunmap(struct ttm_buffer_object *bo, struct iosys_map *map)
 {
 	struct ttm_resource *mem = bo->resource;
 
+	dma_resv_assert_held(bo->base.resv);
+
 	if (iosys_map_is_null(map))
 		return;
 
-- 
2.25.1

Re: [PATCH 1/2] drm/ttm: fix locking in vmap/vunmap TTM GEM helpers

[Dmitry Osipenko]

On 7/15/22 14:15, Christian König wrote:
> I've stumbled over this while reviewing patches for DMA-buf and it looks
> like we completely messed the locking up here.
> 
> In general most TTM function should only be called while holding the
> appropriate BO resv lock. Without this we could break the internal
> buffer object state here.

Could you please clarify which part of the TTM state is affected?

-- 
Best regards,
Dmitry

Re: [PATCH 1/2] drm/ttm: fix locking in vmap/vunmap TTM GEM helpers

[Christian König]

Am 15.07.22 um 13:31 schrieb Dmitry Osipenko:
> On 7/15/22 14:15, Christian König wrote:
>> I've stumbled over this while reviewing patches for DMA-buf and it looks
>> like we completely messed the locking up here.
>>
>> In general most TTM function should only be called while holding the
>> appropriate BO resv lock. Without this we could break the internal
>> buffer object state here.
> Could you please clarify which part of the TTM state is affected?

The ttm_bo_vmap() function calls ttm_mem_io_reserve() with bo->resource 
as parameter.

Since bo->resource is protected by the lock we must make sure that we 
are holding the lock while doing this.

Regards,
Christian.

Re: [PATCH 1/2] drm/ttm: fix locking in vmap/vunmap TTM GEM helpers

[Dmitry Osipenko]

On 7/15/22 14:33, Christian König wrote:
> Am 15.07.22 um 13:31 schrieb Dmitry Osipenko:
>> On 7/15/22 14:15, Christian König wrote:
>>> I've stumbled over this while reviewing patches for DMA-buf and it looks
>>> like we completely messed the locking up here.
>>>
>>> In general most TTM function should only be called while holding the
>>> appropriate BO resv lock. Without this we could break the internal
>>> buffer object state here.
>> Could you please clarify which part of the TTM state is affected?
> 
> The ttm_bo_vmap() function calls ttm_mem_io_reserve() with bo->resource
> as parameter.
> 
> Since bo->resource is protected by the lock we must make sure that we
> are holding the lock while doing this.

Thanks, I see now that the resource members need locking.

-- 
Best regards,
Dmitry

Re: [PATCH 1/2] drm/ttm: fix locking in vmap/vunmap TTM GEM helpers

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 2020 bytes --]

Hi

Am 15.07.22 um 13:15 schrieb Christian König:
> I've stumbled over this while reviewing patches for DMA-buf and it looks
> like we completely messed the locking up here.
> 
> In general most TTM function should only be called while holding the
> appropriate BO resv lock. Without this we could break the internal
> buffer object state here.

I remember that I tried to fix this before and you mentioned that it's 
not allowed to hold this lock here. It would possibly deadlock with 
exported buffers. Did this change with Dmitry's rework of the locking 
semantics?

Best regards
Thomas

> 
> Only compile tested!
> 
> Signed-off-by: Christian König <christian.koenig@amd.com>
> Fixes: 43676605f890 drm/ttm: Add vmap/vunmap to TTM and TTM GEM helpers
> ---
>   drivers/gpu/drm/drm_gem_ttm_helper.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_gem_ttm_helper.c b/drivers/gpu/drm/drm_gem_ttm_helper.c
> index d5962a34c01d..e5fc875990c4 100644
> --- a/drivers/gpu/drm/drm_gem_ttm_helper.c
> +++ b/drivers/gpu/drm/drm_gem_ttm_helper.c
> @@ -64,8 +64,13 @@ int drm_gem_ttm_vmap(struct drm_gem_object *gem,
>   		     struct iosys_map *map)
>   {
>   	struct ttm_buffer_object *bo = drm_gem_ttm_of_gem(gem);
> +	int ret;
> +
> +	dma_resv_lock(gem->resv, NULL);
> +	ret = ttm_bo_vmap(bo, map);
> +	dma_resv_unlock(gem->resv);
>   
> -	return ttm_bo_vmap(bo, map);
> +	return ret;
>   }
>   EXPORT_SYMBOL(drm_gem_ttm_vmap);
>   
> @@ -82,7 +87,9 @@ void drm_gem_ttm_vunmap(struct drm_gem_object *gem,
>   {
>   	struct ttm_buffer_object *bo = drm_gem_ttm_of_gem(gem);
>   
> +	dma_resv_lock(gem->resv, NULL);
>   	ttm_bo_vunmap(bo, map);
> +	dma_resv_unlock(gem->resv);
>   }
>   EXPORT_SYMBOL(drm_gem_ttm_vunmap);
>   

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 1/2] drm/ttm: fix locking in vmap/vunmap TTM GEM helpers

[Christian König]

Am 15.07.22 um 17:36 schrieb Thomas Zimmermann:
> Hi
>
> Am 15.07.22 um 13:15 schrieb Christian König:
>> I've stumbled over this while reviewing patches for DMA-buf and it looks
>> like we completely messed the locking up here.
>>
>> In general most TTM function should only be called while holding the
>> appropriate BO resv lock. Without this we could break the internal
>> buffer object state here.
>
> I remember that I tried to fix this before and you mentioned that it's 
> not allowed to hold this lock here. It would possibly deadlock with 
> exported buffers. Did this change with Dmitry's rework of the locking 
> semantics?

No, can you point me to that?

My assumption was that drm_gem_vmap()/vunmap() is always called with the 
lock held, but Dmitry's work is now suggesting otherwise.

We certainly need to hold the lock when we call ttm_bo_vmap()/vunmap() 
because it needs to access the bo->resource.

Thanks,
Christian.

>
> Best regards
> Thomas
>
>>
>> Only compile tested!
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> Fixes: 43676605f890 drm/ttm: Add vmap/vunmap to TTM and TTM GEM helpers
>> ---
>>   drivers/gpu/drm/drm_gem_ttm_helper.c | 9 ++++++++-
>>   1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/drm_gem_ttm_helper.c 
>> b/drivers/gpu/drm/drm_gem_ttm_helper.c
>> index d5962a34c01d..e5fc875990c4 100644
>> --- a/drivers/gpu/drm/drm_gem_ttm_helper.c
>> +++ b/drivers/gpu/drm/drm_gem_ttm_helper.c
>> @@ -64,8 +64,13 @@ int drm_gem_ttm_vmap(struct drm_gem_object *gem,
>>                struct iosys_map *map)
>>   {
>>       struct ttm_buffer_object *bo = drm_gem_ttm_of_gem(gem);
>> +    int ret;
>> +
>> +    dma_resv_lock(gem->resv, NULL);
>> +    ret = ttm_bo_vmap(bo, map);
>> +    dma_resv_unlock(gem->resv);
>>   -    return ttm_bo_vmap(bo, map);
>> +    return ret;
>>   }
>>   EXPORT_SYMBOL(drm_gem_ttm_vmap);
>>   @@ -82,7 +87,9 @@ void drm_gem_ttm_vunmap(struct drm_gem_object *gem,
>>   {
>>       struct ttm_buffer_object *bo = drm_gem_ttm_of_gem(gem);
>>   +    dma_resv_lock(gem->resv, NULL);
>>       ttm_bo_vunmap(bo, map);
>> +    dma_resv_unlock(gem->resv);
>>   }
>>   EXPORT_SYMBOL(drm_gem_ttm_vunmap);
>

Re: [PATCH 1/2] drm/ttm: fix locking in vmap/vunmap TTM GEM helpers

[Dmitry Osipenko]

On 7/15/22 18:58, Christian König wrote:
> Am 15.07.22 um 17:36 schrieb Thomas Zimmermann:
>> Hi
>>
>> Am 15.07.22 um 13:15 schrieb Christian König:
>>> I've stumbled over this while reviewing patches for DMA-buf and it looks
>>> like we completely messed the locking up here.
>>>
>>> In general most TTM function should only be called while holding the
>>> appropriate BO resv lock. Without this we could break the internal
>>> buffer object state here.
>>
>> I remember that I tried to fix this before and you mentioned that it's
>> not allowed to hold this lock here. It would possibly deadlock with
>> exported buffers. Did this change with Dmitry's rework of the locking
>> semantics?
> 
> No, can you point me to that?
> 
> My assumption was that drm_gem_vmap()/vunmap() is always called with the
> lock held, but Dmitry's work is now suggesting otherwise.
> 
> We certainly need to hold the lock when we call ttm_bo_vmap()/vunmap()
> because it needs to access the bo->resource.

The today's vmapping code paths all should be unlocked for the exported
TTM dma-bufs. My locking patches will change that. To me the Christian's
fix looks good. If there are out-of-tree drivers that do the locking on
the importer side, then we don't really care about them in the upstream
and they will have to adapt.

Reviewed-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>

-- 
Best regards,
Dmitry

Re: [PATCH 1/2] drm/ttm: fix locking in vmap/vunmap TTM GEM helpers

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 3018 bytes --]

Hi

Am 15.07.22 um 17:58 schrieb Christian König:
> Am 15.07.22 um 17:36 schrieb Thomas Zimmermann:
>> Hi
>>
>> Am 15.07.22 um 13:15 schrieb Christian König:
>>> I've stumbled over this while reviewing patches for DMA-buf and it looks
>>> like we completely messed the locking up here.
>>>
>>> In general most TTM function should only be called while holding the
>>> appropriate BO resv lock. Without this we could break the internal
>>> buffer object state here.
>>
>> I remember that I tried to fix this before and you mentioned that it's 
>> not allowed to hold this lock here. It would possibly deadlock with 
>> exported buffers. Did this change with Dmitry's rework of the locking 
>> semantics?
> 
> No, can you point me to that?

I thought it was somewhere in the lengthy discussion at

 
https://lore.kernel.org/dri-devel/20201112132117.27228-1-tzimmermann@suse.de/

but now I cannot find it any longer. :/

> 
> My assumption was that drm_gem_vmap()/vunmap() is always called with the 
> lock held, but Dmitry's work is now suggesting otherwise.

IIRC the lock was supposed to be held, but every drivers does it 
differently. And dma-buf locking essentially worked by chance.

Best regards
Thomas

> 
> We certainly need to hold the lock when we call ttm_bo_vmap()/vunmap() 
> because it needs to access the bo->resource.
> 
> Thanks,
> Christian.
> 
>>
>> Best regards
>> Thomas
>>
>>>
>>> Only compile tested!
>>>
>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>> Fixes: 43676605f890 drm/ttm: Add vmap/vunmap to TTM and TTM GEM helpers
>>> ---
>>>   drivers/gpu/drm/drm_gem_ttm_helper.c | 9 ++++++++-
>>>   1 file changed, 8 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_gem_ttm_helper.c 
>>> b/drivers/gpu/drm/drm_gem_ttm_helper.c
>>> index d5962a34c01d..e5fc875990c4 100644
>>> --- a/drivers/gpu/drm/drm_gem_ttm_helper.c
>>> +++ b/drivers/gpu/drm/drm_gem_ttm_helper.c
>>> @@ -64,8 +64,13 @@ int drm_gem_ttm_vmap(struct drm_gem_object *gem,
>>>                struct iosys_map *map)
>>>   {
>>>       struct ttm_buffer_object *bo = drm_gem_ttm_of_gem(gem);
>>> +    int ret;
>>> +
>>> +    dma_resv_lock(gem->resv, NULL);
>>> +    ret = ttm_bo_vmap(bo, map);
>>> +    dma_resv_unlock(gem->resv);
>>>   -    return ttm_bo_vmap(bo, map);
>>> +    return ret;
>>>   }
>>>   EXPORT_SYMBOL(drm_gem_ttm_vmap);
>>>   @@ -82,7 +87,9 @@ void drm_gem_ttm_vunmap(struct drm_gem_object *gem,
>>>   {
>>>       struct ttm_buffer_object *bo = drm_gem_ttm_of_gem(gem);
>>>   +    dma_resv_lock(gem->resv, NULL);
>>>       ttm_bo_vunmap(bo, map);
>>> +    dma_resv_unlock(gem->resv);
>>>   }
>>>   EXPORT_SYMBOL(drm_gem_ttm_vunmap);
>>
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 1/2] drm/ttm: fix locking in vmap/vunmap TTM GEM helpers

[Christian König]

Am 19.07.22 um 09:19 schrieb Thomas Zimmermann:
> Hi
>
> Am 15.07.22 um 17:58 schrieb Christian König:
>> Am 15.07.22 um 17:36 schrieb Thomas Zimmermann:
>>> Hi
>>>
>>> Am 15.07.22 um 13:15 schrieb Christian König:
>>>> I've stumbled over this while reviewing patches for DMA-buf and it 
>>>> looks
>>>> like we completely messed the locking up here.
>>>>
>>>> In general most TTM function should only be called while holding the
>>>> appropriate BO resv lock. Without this we could break the internal
>>>> buffer object state here.
>>>
>>> I remember that I tried to fix this before and you mentioned that 
>>> it's not allowed to hold this lock here. It would possibly deadlock 
>>> with exported buffers. Did this change with Dmitry's rework of the 
>>> locking semantics?
>>
>> No, can you point me to that?
>
> I thought it was somewhere in the lengthy discussion at
>
>
> https://lore.kernel.org/dri-devel/20201112132117.27228-1-tzimmermann@suse.de/ 
>
>
> but now I cannot find it any longer. :/
>
>>
>> My assumption was that drm_gem_vmap()/vunmap() is always called with 
>> the lock held, but Dmitry's work is now suggesting otherwise.
>
> IIRC the lock was supposed to be held, but every drivers does it 
> differently. And dma-buf locking essentially worked by chance.

I guess we where just lucky that nobody moved the BO while it was being 
mapped.

I've now pushed the patch to take the look to drm-misc-fixes. If some 
call path really called this while already holding the locks we will 
obviously run into trouble, but I think that should be pretty easy to 
sort out.

Regards,
Christian.

>
> Best regards
> Thomas
>
>>
>> We certainly need to hold the lock when we call 
>> ttm_bo_vmap()/vunmap() because it needs to access the bo->resource.
>>
>> Thanks,
>> Christian.
>>
>>>
>>> Best regards
>>> Thomas
>>>
>>>>
>>>> Only compile tested!
>>>>
>>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>>> Fixes: 43676605f890 drm/ttm: Add vmap/vunmap to TTM and TTM GEM 
>>>> helpers
>>>> ---
>>>>   drivers/gpu/drm/drm_gem_ttm_helper.c | 9 ++++++++-
>>>>   1 file changed, 8 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_gem_ttm_helper.c 
>>>> b/drivers/gpu/drm/drm_gem_ttm_helper.c
>>>> index d5962a34c01d..e5fc875990c4 100644
>>>> --- a/drivers/gpu/drm/drm_gem_ttm_helper.c
>>>> +++ b/drivers/gpu/drm/drm_gem_ttm_helper.c
>>>> @@ -64,8 +64,13 @@ int drm_gem_ttm_vmap(struct drm_gem_object *gem,
>>>>                struct iosys_map *map)
>>>>   {
>>>>       struct ttm_buffer_object *bo = drm_gem_ttm_of_gem(gem);
>>>> +    int ret;
>>>> +
>>>> +    dma_resv_lock(gem->resv, NULL);
>>>> +    ret = ttm_bo_vmap(bo, map);
>>>> +    dma_resv_unlock(gem->resv);
>>>>   -    return ttm_bo_vmap(bo, map);
>>>> +    return ret;
>>>>   }
>>>>   EXPORT_SYMBOL(drm_gem_ttm_vmap);
>>>>   @@ -82,7 +87,9 @@ void drm_gem_ttm_vunmap(struct drm_gem_object 
>>>> *gem,
>>>>   {
>>>>       struct ttm_buffer_object *bo = drm_gem_ttm_of_gem(gem);
>>>>   +    dma_resv_lock(gem->resv, NULL);
>>>>       ttm_bo_vunmap(bo, map);
>>>> +    dma_resv_unlock(gem->resv);
>>>>   }
>>>>   EXPORT_SYMBOL(drm_gem_ttm_vunmap);
>>>
>>
>