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 <[email protected]> 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/[email protected]?part=1
