Hi, thanks for the patch. Just a quick note: I'm not a huge fan of patches like this because they attempt to fix a potential issue, usually at the cost of introducing real regressions, which is not a great tradeoff. Especially nowadays when writing code has become much cheaper, patches that don't mention the type of testing/validation that has been done are going to need to wait a long time because they trade the part that's cheap to reproduce "llm please fix this sashiko bug" for the expensive "run the full test/validation tests". However, lacking proper fail/skip lists for igt in the kernel for vmwgfx, explicit testing instructions, or a public CI machine is our fault and we need to fix that.
On Wed, Sep 23, 2026 at 9:50 AM Aldo Ariel Panzardo <[email protected]> wrote: > > vmw_gb_surface_reference_internal() checks srf->res.guest_memory_bo for > NULL and reads its buffer_map_handle and buffer_size outside the > cmdbuf_mutex scope that protects the backup pointer. > > A concurrent surface eviction can NULL out guest_memory_bo between the > check and the use, causing a NULL pointer dereference in > drm_gem_handle_create(). The post-mutex reads of vma_node and base.size > race with any concurrent backup change. Thanks, the locking change itself looks right, but the description needs adjusting. A successful eviction keeps the backup. A concrete racing writer is a backup switch, e.g. from SVGA_3D_CMD_BIND_GB_SURFACE: vmw_resource_unreserve() drops its reference to the old buffer and installs the new one under cmdbuf_mutex. So the unlocked check can see a transient NULL, and the reads after the unlock can see NULL or return another buffer's offset and size. The new handle keeps the original buffer alive, but this ioctl holds no reference to that other buffer. A further switch can drop its last reference and free it while we read it. Please describe that instead. > Move the NULL check inside the mutex, and read buffer_map_handle and > buffer_size while still holding it. > > Fixes: 14b1c33e8429 ("drm/vmwgfx: Add new ioctl for GB surface create and > reference") afaict the unlocked check and the reads after the unlock go back to the original ioctl: Fixes: a97e21923b42 ("drm/vmwgfx: Hook up guest-backed surfaces") > Cc: [email protected] > Reported-by: Sashiko <[email protected]> > Signed-off-by: Aldo Ariel Panzardo <[email protected]> > --- > drivers/gpu/drm/vmwgfx/vmwgfx_surface.c | 19 +++++++++++-------- > 1 file changed, 11 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c > b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c > index bd05637..fff4402 100644 > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c > @@ -1718,15 +1718,21 @@ vmw_gb_surface_reference_internal(struct drm_device > *dev, > > user_srf = container_of(base, struct vmw_user_surface, prime.base); > srf = &user_srf->srf; > + metadata = &srf->metadata; > + > + mutex_lock(&dev_priv->cmdbuf_mutex); /* Protect res->guest_memory_bo > */ > if (!srf->res.guest_memory_bo) { > + mutex_unlock(&dev_priv->cmdbuf_mutex); > DRM_ERROR("Shared GB surface is missing a backup buffer.\n"); > goto out_bad_resource; > } This touches the same lines as your "return an error when a shared GB surface has no backup buffer" v2, and this branch still returns 0. I'd treat them as a series and just rebase this one on top of that patch, keeping both its ret = -EINVAL here and its surface-reference cleanup on failure. z
smime.p7s
Description: S/MIME Cryptographic Signature
