From ffaa906cd1bccc1eff1c263ec83d305cdb49f92f Mon Sep 17 00:00:00 2001 From: Kurt Himebauch <136133082+xXJSONDeruloXx@users.noreply.github.com> Date: Tue, 6 Oct 2026 11:52:16 -0400 Subject: [PATCH] fix: catch KGSL GEM_CLOSE at ioctl instead of exporting drmIoctl (#305) * fix: avoid reusing retired KGSL GEM handles * fix: defer closing GEM_CLOSE'd KGSL dma-buf descriptors * Revert "fix: defer closing GEM_CLOSE'd KGSL dma-buf descriptors" This reverts commit 191bf4b6f2701bbe1602609a7ce4b49d963f9db1. * test: interpose drmIoctl without releasing GEM_CLOSE handles * fix: catch KGSL GEM_CLOSE at ioctl instead of exporting drmIoctl --- .github/workflows/build.yml | 6 +- tools/linuxfs/preload/drm.c | 123 ++++++++++++++++++++++++++-------- tools/linuxfs/preload/netif.c | 2 + 3 files changed, 103 insertions(+), 28 deletions(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index eb17e9eb0..3c0dac30d 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -363,9 +363,13 @@ jobs: echo "::error::libfakeinput.so exports the C++ runtime"; exit 1 fi SESSION=$(syms "$D/libblsession.so") - for sym in socket bind getsockname setsockopt statfs statvfs syscall shm_open shm_unlink mmap mmap64 munmap drmPrimeFDToHandle drmCloseBufferHandle drmIoctl; do + for sym in socket bind getsockname setsockopt statfs statvfs syscall shm_open shm_unlink mmap mmap64 munmap drmPrimeFDToHandle drmCloseBufferHandle; do grep -qx "$sym" <<<"$SESSION" || { echo "::error::libblsession.so does not export $sym"; exit 1; } done + # Zink's GEM_CLOSE is caught at ioctl(); an exported drmIoctl made NMS replay old frames. + if grep -qx drmIoctl <<<"$SESSION"; then + echo "::error::libblsession.so exports drmIoctl"; exit 1 + fi SSBS=$(syms "$D/libssbs.so") for sym in sigaction signal bsd_signal sysv_signal; do grep -qx "$sym" <<<"$SSBS" || { echo "::error::libssbs.so does not export $sym"; exit 1; } diff --git a/tools/linuxfs/preload/drm.c b/tools/linuxfs/preload/drm.c index 82b072b1b..bb4374be0 100644 --- a/tools/linuxfs/preload/drm.c +++ b/tools/linuxfs/preload/drm.c @@ -14,6 +14,7 @@ #include #include #include +#include #include #define MAX_HANDLES 4096 @@ -25,8 +26,15 @@ struct gem_close { }; #define GEM_CLOSE _IOW('d', 0x09, struct gem_close) +struct handle_entry { + uint32_t handle; + int dma_fd; + int drm_fd; +}; + static pthread_mutex_t lock = PTHREAD_MUTEX_INITIALIZER; -static int handle_fds[MAX_HANDLES]; +static struct handle_entry handle_entries[MAX_HANDLES]; +static uint32_t next_handle = 1; static dev_t kgsl_dev; static int kgsl_state; /* 0 unknown, 1 known, -1 absent */ @@ -49,6 +57,58 @@ static void *real(const char *name) { return fn; } +static int same_file_description(int a, int b) { + if (a == b) { + return 1; + } +#ifdef SYS_kcmp + int saved_errno = errno; + long ret = syscall(SYS_kcmp, getpid(), getpid(), 0 /* KCMP_FILE */, a, b); + errno = saved_errno; + return ret == 0; +#else + return 0; +#endif +} + +static int find_handle_locked(int fd, uint32_t handle) { + for (int i = 0; i < MAX_HANDLES; i++) { + if (handle_entries[i].handle == handle && + same_file_description(handle_entries[i].drm_fd, fd)) { + return i; + } + } + return -1; +} + +/* + * The old table used its slot index as the public handle. Once #275 started freeing slots, + * rapidly recreated swapchains immediately reused those numbers. Keep storage slots reusable but + * make the public token monotonic so a stale handle can fail instead of aliasing a newer buffer. + */ +static uint32_t alloc_handle_locked(void) { + for (int tries = 0; tries <= MAX_HANDLES; tries++) { + uint32_t candidate = next_handle++; + if (next_handle == 0) { + next_handle = 1; + } + if (candidate == 0) { + continue; + } + int used = 0; + for (int i = 0; i < MAX_HANDLES; i++) { + if (handle_entries[i].handle == candidate) { + used = 1; + break; + } + } + if (!used) { + return candidate; + } + } + return 0; +} + int drmPrimeFDToHandle(int fd, int prime_fd, uint32_t *handle) { if (!is_kgsl(fd)) { int (*fn)(int, int, uint32_t *) = (int (*)(int, int, uint32_t *)) real("drmPrimeFDToHandle"); @@ -60,10 +120,16 @@ int drmPrimeFDToHandle(int fd, int prime_fd, uint32_t *handle) { } int ret = -ENOMEM; pthread_mutex_lock(&lock); - for (uint32_t i = 1; i < MAX_HANDLES; i++) { - if (handle_fds[i] == 0) { - handle_fds[i] = dup_fd; - *handle = i; + for (int i = 0; i < MAX_HANDLES; i++) { + if (handle_entries[i].handle == 0) { + uint32_t token = alloc_handle_locked(); + if (!token) { + break; + } + handle_entries[i].handle = token; + handle_entries[i].dma_fd = dup_fd; + handle_entries[i].drm_fd = fd; + *handle = token; ret = 0; break; } @@ -82,21 +148,23 @@ int drmPrimeHandleToFD(int fd, uint32_t handle, uint32_t flags, int *prime_fd) { } int ret = -EINVAL; pthread_mutex_lock(&lock); - if (handle > 0 && handle < MAX_HANDLES && handle_fds[handle] > 0) { - *prime_fd = fcntl(handle_fds[handle], F_DUPFD_CLOEXEC, 0); + int index = find_handle_locked(fd, handle); + if (index >= 0) { + *prime_fd = fcntl(handle_entries[index].dma_fd, F_DUPFD_CLOEXEC, 0); ret = *prime_fd < 0 ? -errno : 0; } pthread_mutex_unlock(&lock); return ret; } -/* Drops a handle's descriptor; false when the handle was never given out. */ -static int release_handle(uint32_t handle) { +/* Drops only a handle belonging to this KGSL file description. */ +static int release_handle(int fd, uint32_t handle) { int found = 0; pthread_mutex_lock(&lock); - if (handle > 0 && handle < MAX_HANDLES && handle_fds[handle] > 0) { - close(handle_fds[handle]); - handle_fds[handle] = 0; + int index = find_handle_locked(fd, handle); + if (index >= 0) { + close(handle_entries[index].dma_fd); + handle_entries[index] = (struct handle_entry){0}; found = 1; } pthread_mutex_unlock(&lock); @@ -108,29 +176,30 @@ int drmCloseBufferHandle(int fd, uint32_t handle) { int (*fn)(int, uint32_t) = (int (*)(int, uint32_t)) real("drmCloseBufferHandle"); return fn ? fn(fd, handle) : -ENOSYS; } - return release_handle(handle) ? 0 : -EINVAL; + return release_handle(fd, handle) ? 0 : -EINVAL; } /* * Zink gives its handles back with the GEM_CLOSE ioctl itself, not drmCloseBufferHandle * (zink_bo.c, bo_destroy). KGSL refuses the ioctl, so the duplicated descriptor stayed open and - * kept the whole buffer alive: Xwayland asks for a handle for every buffer it imports, and a game - * whose swapchain was recreated on each resize piled up gigabytes of old swapchain images until - * Android killed the session. + * kept the whole buffer alive. Preserve #275's reclamation without letting another KGSL file + * description release the token or making a new buffer immediately inherit a retired token. + * + * Caught at ioctl() (netif.c), which libdrm's drmIoctl calls, rather than by exporting drmIoctl: + * every session process preloads this library, and exporting drmIoctl from it brought on a + * judder replaying old frames in No Man's Sky on an Adreno 840 even with no handle released. */ -int drmIoctl(int fd, unsigned long request, void *arg) { - if (request == GEM_CLOSE && arg && is_kgsl(fd)) { - if (release_handle(((struct gem_close *) arg)->handle)) { - return 0; - } +__attribute__((visibility("hidden"))) int bl_drm_gem_close(int fd, unsigned long request, void *arg, int *rc) { + if (request != GEM_CLOSE || !arg || !is_kgsl(fd)) { + return 0; + } + if (release_handle(fd, ((struct gem_close *) arg)->handle)) { + *rc = 0; + } else { errno = EINVAL; - return -1; + *rc = -1; } - int (*fn)(int, unsigned long, void *) = (int (*)(int, unsigned long, void *)) real("drmIoctl"); - if (!fn) { - return -1; - } - return fn(fd, request, arg); + return 1; } /* diff --git a/tools/linuxfs/preload/netif.c b/tools/linuxfs/preload/netif.c index 0de77eed5..a7b91a5bb 100644 --- a/tools/linuxfs/preload/netif.c +++ b/tools/linuxfs/preload/netif.c @@ -353,6 +353,7 @@ void bl_kgsl_chip_id_fixup(unsigned long request, void *arg, int rc); /* kgslid. int bl_kgsl_poll(int (*real)(int, unsigned long, void *), int fd, unsigned long request, void *arg, int *rc) __attribute__((visibility("hidden"))); int bl_ntsync_ioctl(int fd, unsigned long request, void *arg, int *rc) __attribute__((visibility("hidden"))); +int bl_drm_gem_close(int fd, unsigned long request, void *arg, int *rc) __attribute__((visibility("hidden"))); /* Every DRM and evdev call comes through here, so the lookup is done once. Two threads racing * to store it store the same pointer. */ @@ -366,6 +367,7 @@ int ioctl(int fd, unsigned long request, ...) { int rc; if (bl_ntsync_ioctl(fd, request, arg, &rc)) return rc; + if (bl_drm_gem_close(fd, request, arg, &rc)) return rc; if (real == NULL) real = (ioctl_fn) dlsym(RTLD_NEXT, "ioctl"); if (!bl_kgsl_poll(real, fd, request, arg, &rc)) rc = real(fd, request, arg); bl_kgsl_chip_id_fixup(request, arg, rc);