[PATCH v7 0/2] DRM GEM fixes

[PATCH v7 0/2] DRM GEM fixes

[Dmitry Osipenko]

This patchset fixes two problems in the common GEM code. First fixed problem
is the bogus lockdep splat that complicates debugging of DRM drivers. Second
problem is the inconsistency in behaviour and improper handling of mapping
the imported GEMs by some drivers, to fix it we will prohibit to map the
imported GEMs like majority of drivers already do.

Changelog:

v7: - Factored out GEM patches from [1] since I'll be working on the
      dma-buf locking in a separate patchsets now.

[1] https://lore.kernel.org/all/20220526235040.678984-1-dmitry.osipenko@collabora.com/

    - Improved commit message and added fixes tag to the "Properly annotate
      WW context" patch.

    - Replaced "Move mapping of imported dma-bufs to drm_gem_mmap_obj()"
      patch with "Don't map imported GEMs", like was suggested by
      Thomas Hellström.

    - Added r-b and suggested-by from Thomas Hellström.

Dmitry Osipenko (2):
  drm/gem: Properly annotate WW context on drm_gem_lock_reservations()
    error
  drm/gem: Don't map imported GEMs

 drivers/gpu/drm/drm_gem.c              | 8 ++++++--
 drivers/gpu/drm/drm_gem_shmem_helper.c | 9 ---------
 2 files changed, 6 insertions(+), 11 deletions(-)

-- 
2.36.1

[PATCH v7 0/2] DRM GEM fixes

[Dmitry Osipenko]

This patchset fixes two problems in the common GEM code. First fixed problem
is the bogus lockdep splat that complicates debugging of DRM drivers. Second
problem is the inconsistency in behaviour and improper handling of mapping
the imported GEMs by some drivers, to fix it we will prohibit to map the
imported GEMs like majority of drivers already do.

Changelog:

v7: - Factored out GEM patches from [1] since I'll be working on the
      dma-buf locking in a separate patchsets now.

[1] https://lore.kernel.org/all/20220526235040.678984-1-dmitry.osipenko@collabora.com/

    - Improved commit message and added fixes tag to the "Properly annotate
      WW context" patch.

    - Replaced "Move mapping of imported dma-bufs to drm_gem_mmap_obj()"
      patch with "Don't map imported GEMs", like was suggested by
      Thomas Hellström.

    - Added r-b and suggested-by from Thomas Hellström.

Dmitry Osipenko (2):
  drm/gem: Properly annotate WW context on drm_gem_lock_reservations()
    error
  drm/gem: Don't map imported GEMs

 drivers/gpu/drm/drm_gem.c              | 8 ++++++--
 drivers/gpu/drm/drm_gem_shmem_helper.c | 9 ---------
 2 files changed, 6 insertions(+), 11 deletions(-)

-- 
2.36.1

[PATCH v7 1/2] drm/gem: Properly annotate WW context on drm_gem_lock_reservations() error

[Dmitry Osipenko]

Use ww_acquire_fini() in the error code paths. Otherwise lockdep
thinks that lock is held when lock's memory is freed after the
drm_gem_lock_reservations() error. The ww_acquire_context needs to be
annotated as "released", which fixes the noisy "WARNING: held lock freed!"
splat of VirtIO-GPU driver with CONFIG_DEBUG_MUTEXES=y and enabled lockdep.

Cc: stable@vger.kernel.org
Fixes: 7edc3e3b975b5 ("drm: Add helpers for locking an array of BO reservations.")
Reviewed-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
---
 drivers/gpu/drm/drm_gem.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index eb0c2d041f13..86d670c71286 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -1226,7 +1226,7 @@ drm_gem_lock_reservations(struct drm_gem_object **objs, int count,
 		ret = dma_resv_lock_slow_interruptible(obj->resv,
 								 acquire_ctx);
 		if (ret) {
-			ww_acquire_done(acquire_ctx);
+			ww_acquire_fini(acquire_ctx);
 			return ret;
 		}
 	}
@@ -1251,7 +1251,7 @@ drm_gem_lock_reservations(struct drm_gem_object **objs, int count,
 				goto retry;
 			}
 
-			ww_acquire_done(acquire_ctx);
+			ww_acquire_fini(acquire_ctx);
 			return ret;
 		}
 	}
-- 
2.36.1

[PATCH v7 1/2] drm/gem: Properly annotate WW context on drm_gem_lock_reservations() error

[Dmitry Osipenko]

Use ww_acquire_fini() in the error code paths. Otherwise lockdep
thinks that lock is held when lock's memory is freed after the
drm_gem_lock_reservations() error. The ww_acquire_context needs to be
annotated as "released", which fixes the noisy "WARNING: held lock freed!"
splat of VirtIO-GPU driver with CONFIG_DEBUG_MUTEXES=y and enabled lockdep.

Cc: stable@vger.kernel.org
Fixes: 7edc3e3b975b5 ("drm: Add helpers for locking an array of BO reservations.")
Reviewed-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
---
 drivers/gpu/drm/drm_gem.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index eb0c2d041f13..86d670c71286 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -1226,7 +1226,7 @@ drm_gem_lock_reservations(struct drm_gem_object **objs, int count,
 		ret = dma_resv_lock_slow_interruptible(obj->resv,
 								 acquire_ctx);
 		if (ret) {
-			ww_acquire_done(acquire_ctx);
+			ww_acquire_fini(acquire_ctx);
 			return ret;
 		}
 	}
@@ -1251,7 +1251,7 @@ drm_gem_lock_reservations(struct drm_gem_object **objs, int count,
 				goto retry;
 			}
 
-			ww_acquire_done(acquire_ctx);
+			ww_acquire_fini(acquire_ctx);
 			return ret;
 		}
 	}
-- 
2.36.1

[PATCH v7 2/2] drm/gem: Don't map imported GEMs

[Dmitry Osipenko]

Drivers that use drm_gem_mmap() and drm_gem_mmap_obj() helpers don't
handle imported dma-bufs properly, which results in mapping of something
else than the imported dma-buf. On NVIDIA Tegra we get a hard lockup when
userspace writes to the memory mapping of a dma-buf that was imported into
Tegra's DRM GEM.

Majority of DRM drivers prohibit mapping of the imported GEM objects.
Mapping of imported GEMs require special care from userspace since it
should sync dma-buf because mapping coherency of the exporter device may
not match the DRM device. Let's prohibit the mapping for all DRM drivers
for consistency.

Cc: stable@vger.kernel.org
Suggested-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
---
 drivers/gpu/drm/drm_gem.c              | 4 ++++
 drivers/gpu/drm/drm_gem_shmem_helper.c | 9 ---------
 2 files changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index 86d670c71286..fc9ec42fa0ab 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -1034,6 +1034,10 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size,
 {
 	int ret;
 
+	/* Don't allow imported objects to be mapped */
+	if (obj->import_attach)
+		return -EINVAL;
+
 	/* Check for valid size. */
 	if (obj_size < vma->vm_end - vma->vm_start)
 		return -EINVAL;
diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
index 8ad0e02991ca..6190f5018986 100644
--- a/drivers/gpu/drm/drm_gem_shmem_helper.c
+++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
@@ -609,17 +609,8 @@ EXPORT_SYMBOL_GPL(drm_gem_shmem_vm_ops);
  */
 int drm_gem_shmem_mmap(struct drm_gem_shmem_object *shmem, struct vm_area_struct *vma)
 {
-	struct drm_gem_object *obj = &shmem->base;
 	int ret;
 
-	if (obj->import_attach) {
-		/* Drop the reference drm_gem_mmap_obj() acquired.*/
-		drm_gem_object_put(obj);
-		vma->vm_private_data = NULL;
-
-		return dma_buf_mmap(obj->dma_buf, vma, 0);
-	}
-
 	ret = drm_gem_shmem_get_pages(shmem);
 	if (ret) {
 		drm_gem_vm_close(vma);
-- 
2.36.1

[PATCH v7 2/2] drm/gem: Don't map imported GEMs

[Dmitry Osipenko]

Drivers that use drm_gem_mmap() and drm_gem_mmap_obj() helpers don't
handle imported dma-bufs properly, which results in mapping of something
else than the imported dma-buf. On NVIDIA Tegra we get a hard lockup when
userspace writes to the memory mapping of a dma-buf that was imported into
Tegra's DRM GEM.

Majority of DRM drivers prohibit mapping of the imported GEM objects.
Mapping of imported GEMs require special care from userspace since it
should sync dma-buf because mapping coherency of the exporter device may
not match the DRM device. Let's prohibit the mapping for all DRM drivers
for consistency.

Cc: stable@vger.kernel.org
Suggested-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
---
 drivers/gpu/drm/drm_gem.c              | 4 ++++
 drivers/gpu/drm/drm_gem_shmem_helper.c | 9 ---------
 2 files changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index 86d670c71286..fc9ec42fa0ab 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -1034,6 +1034,10 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size,
 {
 	int ret;
 
+	/* Don't allow imported objects to be mapped */
+	if (obj->import_attach)
+		return -EINVAL;
+
 	/* Check for valid size. */
 	if (obj_size < vma->vm_end - vma->vm_start)
 		return -EINVAL;
diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
index 8ad0e02991ca..6190f5018986 100644
--- a/drivers/gpu/drm/drm_gem_shmem_helper.c
+++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
@@ -609,17 +609,8 @@ EXPORT_SYMBOL_GPL(drm_gem_shmem_vm_ops);
  */
 int drm_gem_shmem_mmap(struct drm_gem_shmem_object *shmem, struct vm_area_struct *vma)
 {
-	struct drm_gem_object *obj = &shmem->base;
 	int ret;
 
-	if (obj->import_attach) {
-		/* Drop the reference drm_gem_mmap_obj() acquired.*/
-		drm_gem_object_put(obj);
-		vma->vm_private_data = NULL;
-
-		return dma_buf_mmap(obj->dma_buf, vma, 0);
-	}
-
 	ret = drm_gem_shmem_get_pages(shmem);
 	if (ret) {
 		drm_gem_vm_close(vma);
-- 
2.36.1

Re: [PATCH v7 2/2] drm/gem: Don't map imported GEMs

[Thomas Hellström (Intel)]

Hi, Dmitry,

On 6/30/22 22:04, Dmitry Osipenko wrote:
> Drivers that use drm_gem_mmap() and drm_gem_mmap_obj() helpers don't
> handle imported dma-bufs properly, which results in mapping of something
> else than the imported dma-buf. On NVIDIA Tegra we get a hard lockup when
> userspace writes to the memory mapping of a dma-buf that was imported into
> Tegra's DRM GEM.
>
> Majority of DRM drivers prohibit mapping of the imported GEM objects.
> Mapping of imported GEMs require special care from userspace since it
> should sync dma-buf because mapping coherency of the exporter device may
> not match the DRM device. Let's prohibit the mapping for all DRM drivers
> for consistency.
>
> Cc: stable@vger.kernel.org
> Suggested-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>

This might break drivers whose obj->funcs->mmap() callback already 
handles this case, and has userspace that does the right thing.

I think the disabling must be checked on a per-driver basis to avoid 
that; in particular drivers that already call dma_buf_mmap() should be 
able to continue doing this.

Also the Fixes: review comment remains,

thanks,

Thomas



> ---
>   drivers/gpu/drm/drm_gem.c              | 4 ++++
>   drivers/gpu/drm/drm_gem_shmem_helper.c | 9 ---------
>   2 files changed, 4 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index 86d670c71286..fc9ec42fa0ab 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -1034,6 +1034,10 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size,
>   {
>   	int ret;
>   
> +	/* Don't allow imported objects to be mapped */
> +	if (obj->import_attach)
> +		return -EINVAL;
> +
>   	/* Check for valid size. */
>   	if (obj_size < vma->vm_end - vma->vm_start)
>   		return -EINVAL;
> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
> index 8ad0e02991ca..6190f5018986 100644
> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> @@ -609,17 +609,8 @@ EXPORT_SYMBOL_GPL(drm_gem_shmem_vm_ops);
>    */
>   int drm_gem_shmem_mmap(struct drm_gem_shmem_object *shmem, struct vm_area_struct *vma)
>   {
> -	struct drm_gem_object *obj = &shmem->base;
>   	int ret;
>   
> -	if (obj->import_attach) {
> -		/* Drop the reference drm_gem_mmap_obj() acquired.*/
> -		drm_gem_object_put(obj);
> -		vma->vm_private_data = NULL;
> -
> -		return dma_buf_mmap(obj->dma_buf, vma, 0);
> -	}
> -
>   	ret = drm_gem_shmem_get_pages(shmem);
>   	if (ret) {
>   		drm_gem_vm_close(vma);

Re: [PATCH v7 2/2] drm/gem: Don't map imported GEMs

[Thomas Hellström (Intel)]

Hi, Dmitry,

On 6/30/22 22:04, Dmitry Osipenko wrote:
> Drivers that use drm_gem_mmap() and drm_gem_mmap_obj() helpers don't
> handle imported dma-bufs properly, which results in mapping of something
> else than the imported dma-buf. On NVIDIA Tegra we get a hard lockup when
> userspace writes to the memory mapping of a dma-buf that was imported into
> Tegra's DRM GEM.
>
> Majority of DRM drivers prohibit mapping of the imported GEM objects.
> Mapping of imported GEMs require special care from userspace since it
> should sync dma-buf because mapping coherency of the exporter device may
> not match the DRM device. Let's prohibit the mapping for all DRM drivers
> for consistency.
>
> Cc: stable@vger.kernel.org
> Suggested-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>

This might break drivers whose obj->funcs->mmap() callback already 
handles this case, and has userspace that does the right thing.

I think the disabling must be checked on a per-driver basis to avoid 
that; in particular drivers that already call dma_buf_mmap() should be 
able to continue doing this.

Also the Fixes: review comment remains,

thanks,

Thomas



> ---
>   drivers/gpu/drm/drm_gem.c              | 4 ++++
>   drivers/gpu/drm/drm_gem_shmem_helper.c | 9 ---------
>   2 files changed, 4 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index 86d670c71286..fc9ec42fa0ab 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -1034,6 +1034,10 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size,
>   {
>   	int ret;
>   
> +	/* Don't allow imported objects to be mapped */
> +	if (obj->import_attach)
> +		return -EINVAL;
> +
>   	/* Check for valid size. */
>   	if (obj_size < vma->vm_end - vma->vm_start)
>   		return -EINVAL;
> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
> index 8ad0e02991ca..6190f5018986 100644
> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> @@ -609,17 +609,8 @@ EXPORT_SYMBOL_GPL(drm_gem_shmem_vm_ops);
>    */
>   int drm_gem_shmem_mmap(struct drm_gem_shmem_object *shmem, struct vm_area_struct *vma)
>   {
> -	struct drm_gem_object *obj = &shmem->base;
>   	int ret;
>   
> -	if (obj->import_attach) {
> -		/* Drop the reference drm_gem_mmap_obj() acquired.*/
> -		drm_gem_object_put(obj);
> -		vma->vm_private_data = NULL;
> -
> -		return dma_buf_mmap(obj->dma_buf, vma, 0);
> -	}
> -
>   	ret = drm_gem_shmem_get_pages(shmem);
>   	if (ret) {
>   		drm_gem_vm_close(vma);

Re: [PATCH v7 2/2] drm/gem: Don't map imported GEMs

[Dmitry Osipenko]

Hello Thomas,

On 6/30/22 23:15, Thomas Hellström (Intel) wrote:
> Hi, Dmitry,
> 
> On 6/30/22 22:04, Dmitry Osipenko wrote:
>> Drivers that use drm_gem_mmap() and drm_gem_mmap_obj() helpers don't
>> handle imported dma-bufs properly, which results in mapping of something
>> else than the imported dma-buf. On NVIDIA Tegra we get a hard lockup when
>> userspace writes to the memory mapping of a dma-buf that was imported
>> into
>> Tegra's DRM GEM.
>>
>> Majority of DRM drivers prohibit mapping of the imported GEM objects.
>> Mapping of imported GEMs require special care from userspace since it
>> should sync dma-buf because mapping coherency of the exporter device may
>> not match the DRM device. Let's prohibit the mapping for all DRM drivers
>> for consistency.
>>
>> Cc: stable@vger.kernel.org
>> Suggested-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> 
> This might break drivers whose obj->funcs->mmap() callback already
> handles this case, and has userspace that does the right thing.

The drm-shmem helper should be the only that maps imported GEMs
properly, but drivers that use drm-shmem already prohibit to map
imported GEMs. Okay, I'll try to re-check once again to be sure.

> I think the disabling must be checked on a per-driver basis to avoid
> that; in particular drivers that already call dma_buf_mmap() should be
> able to continue doing this.
> 
> Also the Fixes: review comment remains,

This should've been broken forever, don't think that we have a fixes tag
here. I looked thought all my previous patches and added the Fixes
wherever was possible. If I really missed something, then please let me
know.

-- 
Best regards,
Dmitry

Re: [PATCH v7 2/2] drm/gem: Don't map imported GEMs

[Dmitry Osipenko]

Hello Thomas,

On 6/30/22 23:15, Thomas Hellström (Intel) wrote:
> Hi, Dmitry,
> 
> On 6/30/22 22:04, Dmitry Osipenko wrote:
>> Drivers that use drm_gem_mmap() and drm_gem_mmap_obj() helpers don't
>> handle imported dma-bufs properly, which results in mapping of something
>> else than the imported dma-buf. On NVIDIA Tegra we get a hard lockup when
>> userspace writes to the memory mapping of a dma-buf that was imported
>> into
>> Tegra's DRM GEM.
>>
>> Majority of DRM drivers prohibit mapping of the imported GEM objects.
>> Mapping of imported GEMs require special care from userspace since it
>> should sync dma-buf because mapping coherency of the exporter device may
>> not match the DRM device. Let's prohibit the mapping for all DRM drivers
>> for consistency.
>>
>> Cc: stable@vger.kernel.org
>> Suggested-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> 
> This might break drivers whose obj->funcs->mmap() callback already
> handles this case, and has userspace that does the right thing.

The drm-shmem helper should be the only that maps imported GEMs
properly, but drivers that use drm-shmem already prohibit to map
imported GEMs. Okay, I'll try to re-check once again to be sure.

> I think the disabling must be checked on a per-driver basis to avoid
> that; in particular drivers that already call dma_buf_mmap() should be
> able to continue doing this.
> 
> Also the Fixes: review comment remains,

This should've been broken forever, don't think that we have a fixes tag
here. I looked thought all my previous patches and added the Fixes
wherever was possible. If I really missed something, then please let me
know.

-- 
Best regards,
Dmitry

Re: [PATCH v7 2/2] drm/gem: Don't map imported GEMs

[Thomas Hellström (Intel)]

On 6/30/22 22:22, Dmitry Osipenko wrote:
> Hello Thomas,
>
> On 6/30/22 23:15, Thomas Hellström (Intel) wrote:
>> Hi, Dmitry,
>>
>> On 6/30/22 22:04, Dmitry Osipenko wrote:
>>> Drivers that use drm_gem_mmap() and drm_gem_mmap_obj() helpers don't
>>> handle imported dma-bufs properly, which results in mapping of something
>>> else than the imported dma-buf. On NVIDIA Tegra we get a hard lockup when
>>> userspace writes to the memory mapping of a dma-buf that was imported
>>> into
>>> Tegra's DRM GEM.
>>>
>>> Majority of DRM drivers prohibit mapping of the imported GEM objects.
>>> Mapping of imported GEMs require special care from userspace since it
>>> should sync dma-buf because mapping coherency of the exporter device may
>>> not match the DRM device. Let's prohibit the mapping for all DRM drivers
>>> for consistency.
>>>
>>> Cc: stable@vger.kernel.org
>>> Suggested-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
>>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
>> This might break drivers whose obj->funcs->mmap() callback already
>> handles this case, and has userspace that does the right thing.
> The drm-shmem helper should be the only that maps imported GEMs
> properly, but drivers that use drm-shmem already prohibit to map
> imported GEMs. Okay, I'll try to re-check once again to be sure.

OK. If you aren't 100.1% sure, then please drop the Cc: stable tag and 
let the patch ride out at least an -rc series, because breaking a stable 
kernel is something we woudln't want to do.

Thanks,

Thomas



>
>> I think the disabling must be checked on a per-driver basis to avoid
>> that; in particular drivers that already call dma_buf_mmap() should be
>> able to continue doing this.
>>
>> Also the Fixes: review comment remains,
> This should've been broken forever, don't think that we have a fixes tag
> here. I looked thought all my previous patches and added the Fixes
> wherever was possible. If I really missed something, then please let me
> know.
>

Re: [PATCH v7 2/2] drm/gem: Don't map imported GEMs

[Thomas Hellström (Intel)]

On 6/30/22 22:22, Dmitry Osipenko wrote:
> Hello Thomas,
>
> On 6/30/22 23:15, Thomas Hellström (Intel) wrote:
>> Hi, Dmitry,
>>
>> On 6/30/22 22:04, Dmitry Osipenko wrote:
>>> Drivers that use drm_gem_mmap() and drm_gem_mmap_obj() helpers don't
>>> handle imported dma-bufs properly, which results in mapping of something
>>> else than the imported dma-buf. On NVIDIA Tegra we get a hard lockup when
>>> userspace writes to the memory mapping of a dma-buf that was imported
>>> into
>>> Tegra's DRM GEM.
>>>
>>> Majority of DRM drivers prohibit mapping of the imported GEM objects.
>>> Mapping of imported GEMs require special care from userspace since it
>>> should sync dma-buf because mapping coherency of the exporter device may
>>> not match the DRM device. Let's prohibit the mapping for all DRM drivers
>>> for consistency.
>>>
>>> Cc: stable@vger.kernel.org
>>> Suggested-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
>>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
>> This might break drivers whose obj->funcs->mmap() callback already
>> handles this case, and has userspace that does the right thing.
> The drm-shmem helper should be the only that maps imported GEMs
> properly, but drivers that use drm-shmem already prohibit to map
> imported GEMs. Okay, I'll try to re-check once again to be sure.

OK. If you aren't 100.1% sure, then please drop the Cc: stable tag and 
let the patch ride out at least an -rc series, because breaking a stable 
kernel is something we woudln't want to do.

Thanks,

Thomas



>
>> I think the disabling must be checked on a per-driver basis to avoid
>> that; in particular drivers that already call dma_buf_mmap() should be
>> able to continue doing this.
>>
>> Also the Fixes: review comment remains,
> This should've been broken forever, don't think that we have a fixes tag
> here. I looked thought all my previous patches and added the Fixes
> wherever was possible. If I really missed something, then please let me
> know.
>

Re: [PATCH v7 2/2] drm/gem: Don't map imported GEMs

[Dmitry Osipenko]

On 6/30/22 23:26, Thomas Hellström (Intel) wrote:
> 
> On 6/30/22 22:22, Dmitry Osipenko wrote:
>> Hello Thomas,
>>
>> On 6/30/22 23:15, Thomas Hellström (Intel) wrote:
>>> Hi, Dmitry,
>>>
>>> On 6/30/22 22:04, Dmitry Osipenko wrote:
>>>> Drivers that use drm_gem_mmap() and drm_gem_mmap_obj() helpers don't
>>>> handle imported dma-bufs properly, which results in mapping of
>>>> something
>>>> else than the imported dma-buf. On NVIDIA Tegra we get a hard lockup
>>>> when
>>>> userspace writes to the memory mapping of a dma-buf that was imported
>>>> into
>>>> Tegra's DRM GEM.
>>>>
>>>> Majority of DRM drivers prohibit mapping of the imported GEM objects.
>>>> Mapping of imported GEMs require special care from userspace since it
>>>> should sync dma-buf because mapping coherency of the exporter device
>>>> may
>>>> not match the DRM device. Let's prohibit the mapping for all DRM
>>>> drivers
>>>> for consistency.
>>>>
>>>> Cc: stable@vger.kernel.org
>>>> Suggested-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
>>>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
>>> This might break drivers whose obj->funcs->mmap() callback already
>>> handles this case, and has userspace that does the right thing.
>> The drm-shmem helper should be the only that maps imported GEMs
>> properly, but drivers that use drm-shmem already prohibit to map
>> imported GEMs. Okay, I'll try to re-check once again to be sure.
> 
> OK. If you aren't 100.1% sure, then please drop the Cc: stable tag and
> let the patch ride out at least an -rc series, because breaking a stable
> kernel is something we woudln't want to do.

Apparently the OMAP DRM driver should be broken similarly to the Tegra
DRM. Unlikely that anyone else maps the imported GEMs in practice, other
drivers are prohibiting the mapping AFAICS. I'll make the v8 without the
stable tag since it's not a critical problem after all because it never
worked for the broken drivers.

-- 
Best regards,
Dmitry

Re: [PATCH v7 2/2] drm/gem: Don't map imported GEMs

[Dmitry Osipenko]

On 6/30/22 23:26, Thomas Hellström (Intel) wrote:
> 
> On 6/30/22 22:22, Dmitry Osipenko wrote:
>> Hello Thomas,
>>
>> On 6/30/22 23:15, Thomas Hellström (Intel) wrote:
>>> Hi, Dmitry,
>>>
>>> On 6/30/22 22:04, Dmitry Osipenko wrote:
>>>> Drivers that use drm_gem_mmap() and drm_gem_mmap_obj() helpers don't
>>>> handle imported dma-bufs properly, which results in mapping of
>>>> something
>>>> else than the imported dma-buf. On NVIDIA Tegra we get a hard lockup
>>>> when
>>>> userspace writes to the memory mapping of a dma-buf that was imported
>>>> into
>>>> Tegra's DRM GEM.
>>>>
>>>> Majority of DRM drivers prohibit mapping of the imported GEM objects.
>>>> Mapping of imported GEMs require special care from userspace since it
>>>> should sync dma-buf because mapping coherency of the exporter device
>>>> may
>>>> not match the DRM device. Let's prohibit the mapping for all DRM
>>>> drivers
>>>> for consistency.
>>>>
>>>> Cc: stable@vger.kernel.org
>>>> Suggested-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
>>>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
>>> This might break drivers whose obj->funcs->mmap() callback already
>>> handles this case, and has userspace that does the right thing.
>> The drm-shmem helper should be the only that maps imported GEMs
>> properly, but drivers that use drm-shmem already prohibit to map
>> imported GEMs. Okay, I'll try to re-check once again to be sure.
> 
> OK. If you aren't 100.1% sure, then please drop the Cc: stable tag and
> let the patch ride out at least an -rc series, because breaking a stable
> kernel is something we woudln't want to do.

Apparently the OMAP DRM driver should be broken similarly to the Tegra
DRM. Unlikely that anyone else maps the imported GEMs in practice, other
drivers are prohibiting the mapping AFAICS. I'll make the v8 without the
stable tag since it's not a critical problem after all because it never
worked for the broken drivers.

-- 
Best regards,
Dmitry

Re: [PATCH v7 1/2] drm/gem: Properly annotate WW context on drm_gem_lock_reservations() error

[Daniel Vetter]

On Thu, Jun 30, 2022 at 11:04:04PM +0300, Dmitry Osipenko wrote:
> Use ww_acquire_fini() in the error code paths. Otherwise lockdep
> thinks that lock is held when lock's memory is freed after the
> drm_gem_lock_reservations() error. The ww_acquire_context needs to be
> annotated as "released", which fixes the noisy "WARNING: held lock freed!"
> splat of VirtIO-GPU driver with CONFIG_DEBUG_MUTEXES=y and enabled lockdep.
> 
> Cc: stable@vger.kernel.org
> Fixes: 7edc3e3b975b5 ("drm: Add helpers for locking an array of BO reservations.")
> Reviewed-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>

I merged this one to drm-misc-next-fixes. The other one looks like there's
still opens pending, pls resubmit appropriately (and maybe with some
analysis in the commit message of how exactly this impacts other drivers).
-Daniel

> ---
>  drivers/gpu/drm/drm_gem.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index eb0c2d041f13..86d670c71286 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -1226,7 +1226,7 @@ drm_gem_lock_reservations(struct drm_gem_object **objs, int count,
>  		ret = dma_resv_lock_slow_interruptible(obj->resv,
>  								 acquire_ctx);
>  		if (ret) {
> -			ww_acquire_done(acquire_ctx);
> +			ww_acquire_fini(acquire_ctx);
>  			return ret;
>  		}
>  	}
> @@ -1251,7 +1251,7 @@ drm_gem_lock_reservations(struct drm_gem_object **objs, int count,
>  				goto retry;
>  			}
>  
> -			ww_acquire_done(acquire_ctx);
> +			ww_acquire_fini(acquire_ctx);
>  			return ret;
>  		}
>  	}
> -- 
> 2.36.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v7 1/2] drm/gem: Properly annotate WW context on drm_gem_lock_reservations() error

[Daniel Vetter]

On Thu, Jun 30, 2022 at 11:04:04PM +0300, Dmitry Osipenko wrote:
> Use ww_acquire_fini() in the error code paths. Otherwise lockdep
> thinks that lock is held when lock's memory is freed after the
> drm_gem_lock_reservations() error. The ww_acquire_context needs to be
> annotated as "released", which fixes the noisy "WARNING: held lock freed!"
> splat of VirtIO-GPU driver with CONFIG_DEBUG_MUTEXES=y and enabled lockdep.
> 
> Cc: stable@vger.kernel.org
> Fixes: 7edc3e3b975b5 ("drm: Add helpers for locking an array of BO reservations.")
> Reviewed-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>

I merged this one to drm-misc-next-fixes. The other one looks like there's
still opens pending, pls resubmit appropriately (and maybe with some
analysis in the commit message of how exactly this impacts other drivers).
-Daniel

> ---
>  drivers/gpu/drm/drm_gem.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index eb0c2d041f13..86d670c71286 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -1226,7 +1226,7 @@ drm_gem_lock_reservations(struct drm_gem_object **objs, int count,
>  		ret = dma_resv_lock_slow_interruptible(obj->resv,
>  								 acquire_ctx);
>  		if (ret) {
> -			ww_acquire_done(acquire_ctx);
> +			ww_acquire_fini(acquire_ctx);
>  			return ret;
>  		}
>  	}
> @@ -1251,7 +1251,7 @@ drm_gem_lock_reservations(struct drm_gem_object **objs, int count,
>  				goto retry;
>  			}
>  
> -			ww_acquire_done(acquire_ctx);
> +			ww_acquire_fini(acquire_ctx);
>  			return ret;
>  		}
>  	}
> -- 
> 2.36.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v7 1/2] drm/gem: Properly annotate WW context on drm_gem_lock_reservations() error

[Daniel Vetter]

On Thu, Jun 30, 2022 at 11:04:04PM +0300, Dmitry Osipenko wrote:
> Use ww_acquire_fini() in the error code paths. Otherwise lockdep
> thinks that lock is held when lock's memory is freed after the
> drm_gem_lock_reservations() error. The ww_acquire_context needs to be
> annotated as "released", which fixes the noisy "WARNING: held lock freed!"
> splat of VirtIO-GPU driver with CONFIG_DEBUG_MUTEXES=y and enabled lockdep.
> 
> Cc: stable@vger.kernel.org
> Fixes: 7edc3e3b975b5 ("drm: Add helpers for locking an array of BO reservations.")
> Reviewed-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>

I merged this one to drm-misc-next-fixes. The other one looks like there's
still opens pending, pls resubmit appropriately (and maybe with some
analysis in the commit message of how exactly this impacts other drivers).
-Daniel

> ---
>  drivers/gpu/drm/drm_gem.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index eb0c2d041f13..86d670c71286 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -1226,7 +1226,7 @@ drm_gem_lock_reservations(struct drm_gem_object **objs, int count,
>  		ret = dma_resv_lock_slow_interruptible(obj->resv,
>  								 acquire_ctx);
>  		if (ret) {
> -			ww_acquire_done(acquire_ctx);
> +			ww_acquire_fini(acquire_ctx);
>  			return ret;
>  		}
>  	}
> @@ -1251,7 +1251,7 @@ drm_gem_lock_reservations(struct drm_gem_object **objs, int count,
>  				goto retry;
>  			}
>  
> -			ww_acquire_done(acquire_ctx);
> +			ww_acquire_fini(acquire_ctx);
>  			return ret;
>  		}
>  	}
> -- 
> 2.36.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch