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
