Hi Zack, Maaz, Following up on the v2 series sent October 7.
Is there any additional testing or any change that would help move the review forward? I'm happy to rerun the VMware/IGT tests or address any remaining concerns. Thanks, Aldo On Wed, 07 Oct 2026 18:20:47 -0300, Aldo Ariel Panzardo <[email protected]> wrote: > Hi Zack, > > Thank you for the detailed review. Here is v2 as a two-patch series > addressing all your feedback: > > 1/2: return an error and drop the surface reference when backup is > missing (unchanged from v2 RESEND, included for series base) > 2/2: close the TOCTOU on guest_memory_bo (v2, description and > Fixes tag corrected per your review) > > Changes in 2/2 since v1: > - Description rewritten: the racing writer is a backup switch via > SVGA_3D_CMD_BIND_GB_SURFACE (vmw_resource_unreserve), not eviction > - Fixes tag corrected to a97e21923b42 > - Rebased on top of 1/2 (no longer duplicates ret = -EINVAL) > - mutex scope kept to the minimum: NULL check, handle creation, > buffer_map_handle and buffer_size reads > > > 1. Validation test for patch 1/2 > --------------------------------- > > A test program creates a shareable surface WITHOUT a backup buffer > (drm_surface_flags=shareable, buffer_handle=SVGA3D_INVALID_ID, no > drm_vmw_surface_flag_create_buffer), then calls the surface reference > ioctl via DRM_VMW_GB_SURFACE_REF_EXT. > > UNFIXED module: ioctl() returns 0 (success) without filling the > reply structure — the kernel handler never sets ret. > FIXED module: ioctl() returns -1 with errno set to EINVAL. Kernel > dmesg shows: > [drm:vmw_gb_surface_reference_internal [vmwgfx]] > *ERROR* Shared GB surface is missing a backup buffer. > > The test exercises the missing-backup error path, but does not > independently verify reference cleanup. The cleanup was verified by > source inspection — see section 2 below. > > Test program (test_no_backup.c, requires <drm.h> and the kernel UAPI > vmwgfx_drm.h): > > #include <stdio.h> > #include <string.h> > #include <fcntl.h> > #include <unistd.h> > #include <sys/ioctl.h> > #include <errno.h> > #include <stdint.h> > #include <drm.h> > #include "vmwgfx_drm.h" > > #define VMW_IOCTL_GB_SURFACE_CREATE_EXT \ > DRM_IOWR(DRM_COMMAND_BASE + DRM_VMW_GB_SURFACE_CREATE_EXT, \ > union drm_vmw_gb_surface_create_ext_arg) > #define VMW_IOCTL_GB_SURFACE_REF_EXT \ > DRM_IOWR(DRM_COMMAND_BASE + DRM_VMW_GB_SURFACE_REF_EXT, \ > union drm_vmw_gb_surface_reference_ext_arg) > > int main(void) { > int fd = open("/dev/dri/renderD128", O_RDWR); > if (fd < 0) { perror("open"); return 77; } > > union drm_vmw_gb_surface_create_ext_arg create; > memset(&create, 0, sizeof(create)); > create.req.base.format = 2; > create.req.base.mip_levels = 1; > create.req.base.array_size = 1; > create.req.base.drm_surface_flags = drm_vmw_surface_flag_shareable; > create.req.base.buffer_handle = 0xFFFFFFFF; > create.req.base.base_size.width = 64; > create.req.base.base_size.height = 64; > create.req.base.base_size.depth = 1; > > if (ioctl(fd, VMW_IOCTL_GB_SURFACE_CREATE_EXT, &create) < 0) > { printf("create failed: %s\n", strerror(errno)); return 77; } > if (create.rep.buffer_handle != 0xFFFFFFFF) > { printf("surface has backup, skip\n"); return 77; } > > union drm_vmw_gb_surface_reference_ext_arg ref; > memset(&ref, 0, sizeof(ref)); > ref.req.sid = create.rep.handle; > > int ret = ioctl(fd, VMW_IOCTL_GB_SURFACE_REF_EXT, &ref); > if (ret < 0 && errno == EINVAL) > { printf("PASS: -EINVAL\n"); return 0; } > if (ret == 0) > { printf("FAIL: success (ret=0)\n"); return 1; } > printf("OTHER: errno=%d\n", errno); > return 1; > } > > Build: gcc -I/usr/include/libdrm -I$HOME -o test_no_backup test_no_backup.c > Run as a user with read/write access to the render node: > ./test_no_backup > > > 2. Reference cleanup verification (source inspection) > ------------------------------------------------------ > > vmw_surface_handle_reference() acquires two refs: > > (a) ttm_base_object_lookup_for_ref() — kref_get on base->refcount > (the "lookup ref") > (b) ttm_ref_object_add() — acquires a per-file reference in tfile > (the "per-file ref") > > On the SUCCESS path (ret == 0, falls through to out_bad_resource): > - ttm_base_object_unref(&base) releases the lookup ref (a). > - The per-file ref (b) stays alive; userspace releases it later > via DRM_VMW_UNREF_SURFACE → ttm_ref_object_base_unref(). > > On ERROR paths (ret != 0, goto out_bad_resource): > - if (ret) ttm_ref_object_base_unref() undoes the per-file > acquisition (b). > - ttm_base_object_unref(&base) releases the lookup ref (a). > - No GEM handle needs to be released on these error paths > (drm_gem_handle_create either was not reached or failed). > > Both paths are balanced. > > > 3. Race analysis (source inspection) > -------------------------------------- > > Writers of res->guest_memory_bo in vmwgfx (at commit 6edd14dd67d7): > > vmwgfx_resource.c:473 vmw_resource_unreserve(): = vmw_user_bo_ref(new) > vmwgfx_resource.c:483 vmw_resource_unreserve(): = NULL > Both under cmdbuf_mutex, called from vmw_execbuf_process > (vmwgfx_execbuf.c:4242) and vmw_resource_pin > (vmwgfx_resource.c:1012). > > vmwgfx_resource.c:229 vmw_resource_init(): = NULL (construction) > vmwgfx_resource.c:354 vmw_resource_buf_alloc(): = gbo (init path) > vmwgfx_surface.c:~1618 vmw_gb_surface_define_internal(): via > vmw_user_bo_lookup (construction, surface not yet registered) > vmwgfx_bo.c:63 vmw_bo_free(): = NULL (destroy) > vmwgfx_cotable.c:485/496 vmw_cotable_resize(): cotable-only > vmwgfx_shader.c:185 vmw_dx_shader_unscrub(): shader init > > The construction-time writers run before the surface is registered > in the per-file object table, so no concurrent reference ioctl can > reach them. vmw_bo_free runs during destroy. vmw_cotable_resize and > vmw_dx_shader_unscrub operate on resource types not reachable via > DRM_VMW_GB_SURFACE_REF_EXT. vmw_resource_unreserve is the remaining > concurrent writer; both its writes are under cmdbuf_mutex. The patch > moves the reads of guest_memory_bo inside cmdbuf_mutex, serializing > them against these writes. > > The race was not reproduced with a dedicated test. The fix is based on > inspection of the locking protocol. > > > 4. IGT regression test > ----------------------- > > Environment: > Host: VMware Workstation 26.0.0 build 25388281 > Guest: Debian 12, kernel 7.3.0-rc4+ (commit 6edd14dd67d7) > IGT: version 2.6 (commit 5e43e9df3a15, x86_64) > Kernel config: CONFIG_KASAN=y (generic, inline), > CONFIG_INIT_STACK_ALL_ZERO=y, CONFIG_DRM_VMWGFX=m > Module swap: sudo rmmod vmwgfx && sudo insmod <module>.ko > Module files on disk: > UNFIXED (vmwgfx-UNFIXED.ko): md5 43a0f8caaf651a274d44a3faa51d3929 > FIXED (vmwgfx-TOCTOU-FIXED.ko): md5 01c6785d713de957fc92478cb213f0b4 > > Results (18 SUCCESS, 9 FAIL, identical in both runs): > > vmw_execution_buffer: > mob-create-map: SUCCESS SUCCESS > buffer-create: SUCCESS SUCCESS > execution-buffer-submit-sync: SUCCESS SUCCESS > vmw_mob_stress: > max_mob_mem_stress: FAIL FAIL > vmw_ref_count: > surface_prime_transfer_explicit_mob: SUCCESS SUCCESS > surface_prime_transfer_implicit_mob: SUCCESS SUCCESS > surface_prime_transfer_fd_dup: SUCCESS SUCCESS > surface_prime_transfer_two_surfaces: SUCCESS SUCCESS > surface_prime_transfer_single_surface_multiple_handle: > SUCCESS SUCCESS > mob_repeated_unref: SUCCESS SUCCESS > surface_repeated_unref: SUCCESS SUCCESS > surface_alloc_ref_unref: SUCCESS SUCCESS > surface_buffer_ref: SUCCESS SUCCESS > surface_prime_refs: SUCCESS SUCCESS > surface_buffer_prime_refs: SUCCESS SUCCESS > vmw_surface_copy: > test_invalid_copies: SUCCESS SUCCESS > test_invalid_copies_3d: SUCCESS SUCCESS > vmw_tri: > tri: FAIL FAIL > tri-no-sync-coherent: FAIL FAIL > tri-2d: SUCCESS SUCCESS > vmw_prime: > basic-vgem: SUCCESS SUCCESS > tri-map-gem: FAIL FAIL > tri-map-dmabuf: FAIL FAIL > draw-dumb-buffer: FAIL FAIL > buffer-surface-fb-sharing-sync-readback: FAIL FAIL > buffer-surface-fb-sharing-sync: FAIL FAIL > buffer-surface-fb-sharing: FAIL FAIL > > The same nine subtests failed in both runs. No additional failures > were observed with the two-patch series applied. > > Aldo Ariel Panzardo (2): > drm/vmwgfx: return an error when a shared GB surface has no backup > buffer > drm/vmwgfx: close TOCTOU on guest_memory_bo in surface reference > ioctl > > drivers/gpu/drm/vmwgfx/vmwgfx_surface.c | 22 ++++++++++++++++------ > 1 file changed, 16 insertions(+), 6 deletions(-) > -- > 2.43.0
