DRM_VMW_GB_SURFACE_REF_EXT with DRM_VMW_HANDLE_PRIME first tries
ttm_prime_fd_to_handle(). If the fd is not a vmwgfx surface export,
vmw_surface_handle_reference() falls back to
vmw_buffer_prime_to_surface_base(). That function calls
drm_gem_prime_fd_to_handle() to turn the fd into a GEM handle in the
caller's file, then looks for a surface attached to that buffer.
That handle is never returned to userspace, since the ioctl reports the
surface handle and a separate backup buffer handle, and it is never
deleted. Every call that gets this far therefore leaves a GEM handle
behind in the file, whether it fails or succeeds, and whatever the
handle references stays alive until the file is closed.
For a dma-buf from another exporter, such as udmabuf, this is an
actual import: drm_gem_prime_fd_to_handle() attaches to the dma-buf,
maps it and creates a new GEM object. The surface lookup then always
fails, because such a buffer never has a surface. The WARN_ON() fires
and the ioctl returns -EINVAL, but the import stays referenced by the
stray handle.
Mesa's svga winsys passes dma-buf fds to DRM_VMW_GB_SURFACE_REF_EXT as
DRM_VMW_HANDLE_PRIME, and KWin 6.7 imports every wl_shm client buffer
as a udmabuf through EGL. On a Plasma Wayland session this pins one
window-sized buffer inside the compositor's DRM file for every
attempted import, for as long as the compositor runs, and logs a full
WARN backtrace each time. On a VirtualBox VMSVGA guest with 3D
acceleration on and the previous patch applied, 103 udmabufs (750 MiB)
had accumulated this way 40 minutes after boot. 99 of them had a
dma-buf refcount of 3, were still attached to the vmwgfx device and
were held by no process fd or mapping.
Look the buffer up through the dma-buf instead of importing it. Only
GEM buffers exported by this device can have a surface, so reject
anything else with drm_gem_is_prime_exported_dma_buf() before touching
it. For our own buffers, use dma_buf->priv directly: the dma-buf holds
a reference to the GEM object for as long as we hold the dma-buf, so
neither a handle nor an extra object reference is needed. The surface
lookup and the ttm_ref_object_add() reference that the callers rely on
are unchanged.
While restructuring the function, also:
- replace the WARN_ON() on a buffer without a surface with a debug
message. Userspace can trigger that case at will.
- drop the base object reference taken by
vmw_lookup_user_surface_for_buffer() when ttm_ref_object_add() fails.
It was leaked before.
Build tested on drm-misc-fixes (4600b4d1a9ee) with W=1 (no warnings in
drivers/gpu/drm/vmwgfx/) and checkpatch.pl --strict. Runtime tested on
the same guest and 7.2.3 kernel as the previous patch, comparing a
vmwgfx.ko with only the previous patch against one with both. Calls
from a second render file:
- udmabuf fd: -EINVAL both times. Before, each call left the import
held by the file (refcount 3, attached) and logged a WARN. Now
nothing is imported and nothing is logged.
- fd of a dumb buffer exported from the primary node: -EINVAL both
times. Before, a stray GEM handle and a WARN per call; now neither.
- fd of a buffer exported before a GB surface was created on it, which
reaches this function and succeeds: the exporter's surface is
returned both times. Before, each call left a GEM handle in the
file; now it does not.
- fd of a buffer created together with its surface, which is exported
as a TTM prime surface and does not reach this function: succeeds
both times.
During a 5-minute KWin 6.7 test replaying a terminal workload, the
udmabuf count went from 4 to 20 (140 MiB) with only the previous patch,
with WARNs logged. With both patches it stayed at 6 during the run,
returned to 4 afterwards, and no WARN was logged.
Fixes: d6667f0ddf46 ("drm/vmwgfx: Fix handling of dumb buffers")
Cc: [email protected] # v6.11+
Assisted-by: LLM
Signed-off-by: Michal TOMA <[email protected]>
---
Reproducer (results and environment are in the cover letter). Run as
root, since it counts udmabufs in debugfs:
cc -o vmwgfx-surfref-fd-leak vmwgfx-surfref-fd-leak.c
./vmwgfx-surfref-fd-leak
// SPDX-License-Identifier: MIT
/*
* Reproducer for "drm/vmwgfx: Don't leak a GEM handle when referencing a
* surface by fd": DRM_VMW_GB_SURFACE_REF_EXT(DRM_VMW_HANDLE_PRIME) with a
* udmabuf fd. Run as root, since it counts udmabufs in debugfs.
*
* cc -o vmwgfx-surfref-fd-leak vmwgfx-surfref-fd-leak.c &&
./vmwgfx-surfref-fd-leak
*
* The render node stays open after the calls. Expected lines 2 and 3
* (udmabufs left while the node is open / after it is closed):
* with this fix: 0 / 0
* without it: 8 / 0 if "drm/vmwgfx: Release PRIME import in
* the BO destroy path" is applied, otherwise
* 8 / 8 (pinned until reboot)
*/
#define _GNU_SOURCE
#include <errno.h>
#include <fcntl.h>
#include <stdio.h>
#include <string.h>
#include <unistd.h>
#include <sys/ioctl.h>
#include <sys/mman.h>
#include <linux/udmabuf.h>
#include <drm/drm.h>
#include <drm/vmwgfx_drm.h>
#define DRM_IOCTL_VMW_GB_SURFACE_REF_EXT \
DRM_IOWR(DRM_COMMAND_BASE + DRM_VMW_GB_SURFACE_REF_EXT, \
union drm_vmw_gb_surface_reference_ext_arg)
#define N 8
#define SIZE (6UL << 20)
/* udmabufs of @size listed in dma_buf/bufinfo */
static int count_udmabufs(unsigned long size)
{
FILE *f = fopen("/sys/kernel/debug/dma_buf/bufinfo", "r");
char line[512];
int n = 0;
if (!f) {
perror("/sys/kernel/debug/dma_buf/bufinfo");
return -1;
}
while (fgets(line, sizeof(line), f)) {
unsigned long sz;
if (sscanf(line, "%lu", &sz) == 1 && sz == size &&
strstr(line, "udmabuf"))
n++;
}
fclose(f);
return n;
}
int main(void)
{
int render = open("/dev/dri/renderD128", O_RDWR | O_CLOEXEC);
int udm = open("/dev/udmabuf", O_RDWR | O_CLOEXEC);
int before, einval = 0;
if (render < 0 || udm < 0) {
perror("open");
return 1;
}
before = count_udmabufs(SIZE);
for (int i = 0; i < N; i++) {
int memfd = memfd_create("surfref", MFD_ALLOW_SEALING |
MFD_CLOEXEC);
struct udmabuf_create create = {
.memfd = memfd, .flags = UDMABUF_FLAGS_CLOEXEC, .size =
SIZE,
};
union drm_vmw_gb_surface_reference_ext_arg arg;
int dmabuf;
if (memfd < 0 || ftruncate(memfd, SIZE) ||
fcntl(memfd, F_ADD_SEALS, F_SEAL_SHRINK)) {
perror("memfd");
return 1;
}
dmabuf = ioctl(udm, UDMABUF_CREATE, &create);
if (dmabuf < 0) {
perror("UDMABUF_CREATE");
return 1;
}
memset(&arg, 0, sizeof(arg));
arg.req.sid = dmabuf;
arg.req.handle_type = DRM_VMW_HANDLE_PRIME;
if (ioctl(render, DRM_IOCTL_VMW_GB_SURFACE_REF_EXT, &arg) &&
errno == EINVAL)
einval++;
close(dmabuf);
close(memfd);
}
close(udm);
usleep(500000);
printf("GB_SURFACE_REF_EXT with a udmabuf fd: %d/%d calls failed with
EINVAL\n",
einval, N);
printf("6 MiB udmabufs left, render node open: %d (before: %d)\n",
count_udmabufs(SIZE), before);
close(render);
usleep(500000);
printf("6 MiB udmabufs left, render node closed: %d\n",
count_udmabufs(SIZE));
return 0;
}
drivers/gpu/drm/vmwgfx/vmwgfx_surface.c | 48 ++++++++++++++-----------
1 file changed, 28 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
index bd0563741..27f68fd9c 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
@@ -16,8 +16,11 @@
#include "device_include/svga3d_surfacedefs.h"
#include <drm/drm_dumb_buffers.h>
+#include <drm/drm_prime.h>
#include <drm/ttm/ttm_placement.h>
+#include <linux/dma-buf.h>
+
#define SVGA3D_FLAGS_64(upper32, lower32) (((uint64_t)upper32 << 32) | lower32)
/**
@@ -931,33 +934,37 @@ u32 vmw_lookup_surface_handle_for_buffer(struct
vmw_private *vmw,
static int vmw_buffer_prime_to_surface_base(struct vmw_private *dev_priv,
struct drm_file *file_priv,
- u32 fd, u32 *handle,
+ u32 fd,
struct ttm_base_object **base_p)
{
struct ttm_base_object *base;
- struct vmw_bo *bo;
+ struct dma_buf *dma_buf;
struct ttm_object_file *tfile = vmw_fpriv(file_priv)->tfile;
struct vmw_user_surface *user_srf;
int ret;
- ret = drm_gem_prime_fd_to_handle(&dev_priv->drm, file_priv, fd, handle);
- if (ret) {
- drm_warn(&dev_priv->drm,
- "Wasn't able to find user buffer for fd = %u.\n", fd);
- return ret;
- }
+ dma_buf = dma_buf_get(fd);
+ if (IS_ERR(dma_buf))
+ return PTR_ERR(dma_buf);
- ret = vmw_user_bo_lookup(file_priv, *handle, &bo);
- if (ret) {
- drm_warn(&dev_priv->drm,
- "Wasn't able to lookup user buffer for handle =
%u.\n", *handle);
- return ret;
+ /*
+ * Only buffers exported by this device can have a user surface.
+ * Look the buffer up through the dma-buf, which holds a reference
+ * to it, instead of importing the fd into file_priv: the GEM handle
+ * that would create is never returned to userspace, so nothing
+ * would release it (or a foreign dma-buf) until the file is closed.
+ */
+ if (!drm_gem_is_prime_exported_dma_buf(&dev_priv->drm, dma_buf)) {
+ ret = -EINVAL;
+ goto out;
}
- user_srf = vmw_lookup_user_surface_for_buffer(dev_priv, bo, *handle);
- if (WARN_ON(!user_srf)) {
- drm_warn(&dev_priv->drm,
- "User surface fd %d (handle %d) is null.\n", fd,
*handle);
+ user_srf = vmw_lookup_user_surface_for_buffer(dev_priv,
+ to_vmw_bo(dma_buf->priv),
+ fd);
+ if (!user_srf) {
+ drm_dbg_driver(&dev_priv->drm,
+ "No user surface for buffer fd %d.\n", fd);
ret = -EINVAL;
goto out;
}
@@ -966,13 +973,15 @@ static int vmw_buffer_prime_to_surface_base(struct
vmw_private *dev_priv,
ret = ttm_ref_object_add(tfile, base, NULL, false);
if (ret) {
drm_warn(&dev_priv->drm,
- "Couldn't add an object ref for the buffer (%d).\n",
*handle);
+ "Couldn't add an object ref for buffer fd %d (%d).\n",
+ fd, ret);
+ ttm_base_object_unref(&base);
goto out;
}
*base_p = base;
out:
- vmw_user_bo_unref(&bo);
+ dma_buf_put(dma_buf);
return ret;
}
@@ -996,7 +1005,6 @@ vmw_surface_handle_reference(struct vmw_private *dev_priv,
return vmw_buffer_prime_to_surface_base(dev_priv,
file_priv,
u_handle,
- &handle,
base_p);
} else {
handle = u_handle;
--
2.55.0
