[PATCH] drm/vmwgfx: Return gem fd handle for scanout buffers

[PATCH] drm/vmwgfx: Return gem fd handle for scanout buffers

[Maaz Mombasawala]

When prime_handle_to_fd is called on a gem buffer handle, we return an
fd for the surface handle if the gem buffer backs a surface.
Return the gem buffer's handle instead for cases when the surface is a
scanout buffer.
Also bump the minor version since this is a slight change in behaviour.

This fixes gem_close() errors in kwin where the compositor expects a gem
handle but gets a vmw surface handle instead.

Signed-off-by: Maaz Mombasawala <maaz.mombasawala@broadcom.com>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_drv.h     | 10 ++++---
 drivers/gpu/drm/vmwgfx/vmwgfx_prime.c   | 14 +++++----
 drivers/gpu/drm/vmwgfx/vmwgfx_surface.c | 38 ++++++++++++-------------
 3 files changed, 34 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
index 38bea8abab84..ff33ba1075a4 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
@@ -39,7 +39,7 @@
 
 #define VMWGFX_DRIVER_NAME "vmwgfx"
 #define VMWGFX_DRIVER_MAJOR 2
-#define VMWGFX_DRIVER_MINOR 21
+#define VMWGFX_DRIVER_MINOR 22
 #define VMWGFX_DRIVER_PATCHLEVEL 0
 #define VMWGFX_FIFO_STATIC_SIZE (1024*1024)
 #define VMWGFX_NUM_DISPLAY_UNITS 8
@@ -1180,9 +1180,11 @@ int vmw_gb_surface_define(struct vmw_private *dev_priv,
 struct vmw_surface *vmw_lookup_surface_for_buffer(struct vmw_private *vmw,
 						  struct vmw_bo *bo,
 						  u32 handle);
-u32 vmw_lookup_surface_handle_for_buffer(struct vmw_private *vmw,
-					 struct vmw_bo *bo,
-					 u32 handle);
+void vmw_lookup_surface_and_handle_for_buffer(struct vmw_private *vmw,
+					      struct vmw_bo *bo,
+					      u32 handle,
+					      u32 *srf_handle,
+					      struct vmw_surface **p_srf);
 int vmw_dumb_create(struct drm_file *file_priv,
 		    struct drm_device *dev,
 		    struct drm_mode_create_dumb *args);
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_prime.c b/drivers/gpu/drm/vmwgfx/vmwgfx_prime.c
index 598b90ac7590..7e6581ee09cc 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_prime.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_prime.c
@@ -93,8 +93,9 @@ int vmw_prime_handle_to_fd(struct drm_device *dev,
 	struct vmw_private *vmw = vmw_priv(dev);
 	struct ttm_object_file *tfile = vmw_fpriv(file_priv)->tfile;
 	struct vmw_bo *vbo;
+	struct vmw_surface *surface = NULL;
+	int surf_handle = 0;
 	int ret;
-	int surf_handle;
 
 	if (handle > VMWGFX_NUM_MOB) {
 		ret = ttm_prime_handle_to_fd(tfile, handle, flags, prime_fd);
@@ -106,16 +107,19 @@ int vmw_prime_handle_to_fd(struct drm_device *dev,
 			ret = drm_gem_prime_handle_to_fd(dev, file_priv, handle,
 							 flags, prime_fd);
 		} else {
-			surf_handle = vmw_lookup_surface_handle_for_buffer(vmw,
-									   vbo,
-									   handle);
-			if (surf_handle > 0)
+			vmw_lookup_surface_and_handle_for_buffer(vmw, vbo,
+								 handle,
+								 &surf_handle,
+								 &surface);
+			if (surface && !surface->metadata.scanout)
 				ret = ttm_prime_handle_to_fd(tfile, surf_handle,
 							     flags, prime_fd);
 			else
 				ret = drm_gem_prime_handle_to_fd(dev, file_priv,
 								 handle, flags,
 								 prime_fd);
+			if (surface)
+				vmw_surface_unreference(&surface);
 		}
 		vmw_user_bo_unref(&vbo);
 	}
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
index bd0563741e89..15455bdba402 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
@@ -895,38 +895,38 @@ vmw_lookup_user_surface_for_buffer(struct vmw_private *vmw, struct vmw_bo *bo,
 	return user_srf;
 }
 
-struct vmw_surface *vmw_lookup_surface_for_buffer(struct vmw_private *vmw,
-						  struct vmw_bo *bo,
-						  u32 handle)
+void vmw_lookup_surface_and_handle_for_buffer(struct vmw_private *vmw,
+					      struct vmw_bo *bo,
+					      u32 handle,
+					      u32 *srf_handle,
+					      struct vmw_surface **p_srf)
 {
 	struct vmw_user_surface *user_srf =
 		vmw_lookup_user_surface_for_buffer(vmw, bo, handle);
-	struct vmw_surface *surf = NULL;
 	struct ttm_base_object *base;
 
+	if (p_srf)
+		*p_srf = NULL;
+	if (srf_handle)
+		*srf_handle = 0;
 	if (user_srf) {
-		surf = vmw_surface_reference(&user_srf->srf);
+		if (p_srf)
+			*p_srf = vmw_surface_reference(&user_srf->srf);
 		base = &user_srf->prime.base;
+		if (srf_handle)
+			*srf_handle = (u32)base->handle;
 		ttm_base_object_unref(&base);
 	}
-	return surf;
 }
 
-u32 vmw_lookup_surface_handle_for_buffer(struct vmw_private *vmw,
-					 struct vmw_bo *bo,
-					 u32 handle)
+struct vmw_surface *vmw_lookup_surface_for_buffer(struct vmw_private *vmw,
+						  struct vmw_bo *bo,
+						  u32 handle)
 {
-	struct vmw_user_surface *user_srf =
-		vmw_lookup_user_surface_for_buffer(vmw, bo, handle);
-	int surf_handle = 0;
-	struct ttm_base_object *base;
+	struct vmw_surface *surface = NULL;
 
-	if (user_srf) {
-		base = &user_srf->prime.base;
-		surf_handle = (u32)base->handle;
-		ttm_base_object_unref(&base);
-	}
-	return surf_handle;
+	vmw_lookup_surface_and_handle_for_buffer(vmw, bo, handle, NULL, &surface);
+	return surface;
 }
 
 static int vmw_buffer_prime_to_surface_base(struct vmw_private *dev_priv,
-- 
2.55.0

Re: [PATCH] drm/vmwgfx: Return gem fd handle for scanout buffers

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Low] Passing an `int *` to a function expecting a `u32 *` causes an incompatible pointer type build failure.

Pre-existing issues:
- [High] Concurrent lockless access to the RB-tree `vbo->res_tree` leads to tree corruption and an associated Use-After-Free window.
--

--- Patch [1]: [PATCH] drm/vmwgfx: Return gem fd handle for scanout buffers ---
commit b5937886d1e50afefe640e402b3c52ae509b9578
Author: Maaz Mombasawala <maaz.mombasawala@broadcom.com>

drm/vmwgfx: Return gem fd handle for scanout buffers

When prime_handle_to_fd is called on a gem buffer handle, we return an
fd for the surface handle if the gem buffer backs a surface.
Return the gem buffer's handle instead for cases when the surface is a
scanout buffer.

> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> index 38bea8abab84c..ff33ba1075a40 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h

[ ... ]

> @@ -1180,9 +1180,11 @@ int vmw_gb_surface_define(struct vmw_private *dev_priv,
>  struct vmw_surface *vmw_lookup_surface_for_buffer(struct vmw_private *vmw,
>  						  struct vmw_bo *bo,
>  						  u32 handle);
> -u32 vmw_lookup_surface_handle_for_buffer(struct vmw_private *vmw,
> -					 struct vmw_bo *bo,
> -					 u32 handle);
> +void vmw_lookup_surface_and_handle_for_buffer(struct vmw_private *vmw,
> +					      struct vmw_bo *bo,
> +					      u32 handle,
> +					      u32 *srf_handle,
> +					      struct vmw_surface **p_srf);
>  int vmw_dumb_create(struct drm_file *file_priv,

[ ... ]

> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_prime.c b/drivers/gpu/drm/vmwgfx/vmwgfx_prime.c
> index 598b90ac7590b..7e6581ee09cc1 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_prime.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_prime.c
> @@ -93,8 +93,9 @@ int vmw_prime_handle_to_fd(struct drm_device *dev,
>  	struct vmw_private *vmw = vmw_priv(dev);
>  	struct ttm_object_file *tfile = vmw_fpriv(file_priv)->tfile;
>  	struct vmw_bo *vbo;
> +	struct vmw_surface *surface = NULL;
> +	int surf_handle = 0;
>  	int ret;
> -	int surf_handle;
>  
>  	if (handle > VMWGFX_NUM_MOB) {
>  		ret = ttm_prime_handle_to_fd(tfile, handle, flags, prime_fd);
> @@ -106,16 +107,19 @@ int vmw_prime_handle_to_fd(struct drm_device *dev,
>  			ret = drm_gem_prime_handle_to_fd(dev, file_priv, handle,
>  							 flags, prime_fd);
>  		} else {
> -			surf_handle = vmw_lookup_surface_handle_for_buffer(vmw,
> -									   vbo,
> -									   handle);
> -			if (surf_handle > 0)
> +			vmw_lookup_surface_and_handle_for_buffer(vmw, vbo,
> +								 handle,
> +								 &surf_handle,
                                                                 ^^^^^^^^^^^^

[Severity: Low]
Does passing an int pointer to a parameter that expects a u32 pointer cause a
compiler warning or build failure?

The function signature for vmw_lookup_surface_and_handle_for_buffer() explicitly
requires a u32 pointer for the fourth parameter.

[Severity: High]
This isn't a bug introduced by this patch, but does calling this locklessly
lead to tree corruption or a use-after-free?

When vmw_prime_handle_to_fd() calls vmw_lookup_surface_and_handle_for_buffer(),
it eventually calls vmw_bo_surface() which iterates over the vbo->res_tree
RB-tree locklessly:

drivers/gpu/drm/vmwgfx/vmwgfx_bo.c:vmw_bo_surface() {
    ...
    for (rb_itr = rb_first(&vbo->res_tree); rb_itr;
         rb_itr = rb_next(rb_itr)) {
    ...
}

Concurrently, other ioctls can acquire the dma_resv lock and modify the
res_tree. Can this concurrent modification result in an infinite loop or a
kernel crash?

Also, in vmw_lookup_user_surface_for_buffer(), the RCU read lock is acquired
after the pointer surf has already been retrieved:

drivers/gpu/drm/vmwgfx/vmwgfx_surface.c:vmw_lookup_user_surface_for_buffer() {
    ...
    surf = vmw_bo_surface(bo);
    if (surf) {
        rcu_read_lock();
    ...
}

Could this leave a window where the object can be freed before the lock is
acquired?

> +								 &surface);
> +			if (surface && !surface->metadata.scanout)
>  				ret = ttm_prime_handle_to_fd(tfile, surf_handle,
>  							     flags, prime_fd);
>  			else
>  				ret = drm_gem_prime_handle_to_fd(dev, file_priv,
>  								 handle, flags,
>  								 prime_fd);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260820165838.442723-1-maaz.mombasawala@broadcom.com?part=1