All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] tests/intel: Add edge case and race condition tests for GEM_CHANGE_HANDLE
@ 2026-10-07 14:12 David Francis
  2026-10-07 14:13 ` Francis, David
                   ` (3 more replies)
  0 siblings, 4 replies; 7+ messages in thread
From: David Francis @ 2026-10-07 14:12 UTC (permalink / raw)
  To: igt-dev
  Cc: tvrtko.ursulin, Vitaly Prosyak, Kamil Konieczny,
	Sebastian Brzezinka, Christian Konig, Simona Vetter, Alex Deucher,
	David Francis

From: Vitaly Prosyak <vitaly.prosyak@amd.com>

Add a GPU-agnostic GEM_CHANGE_HANDLE tests. These test for basic
functionality, edge cases, and race conditions of this ioctl against
itself, gem close, gem create, and prime import and export.

GEM_CHANGE_HANDLE is currently wired to drm_invalid_op in DRM core. This IGT
coverage is a prerequisite for validating the eventual kernel-side ioctl
implementation and makes fixing its security bugs safer.

Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
Cc: Sebastian Brzezinka <sebastian.brzezinka@intel.com>
Cc: Christian Konig <christian.koenig@amd.com>
Cc: Simona Vetter <simona@ffwll.ch>
Cc: Alex Deucher <alexander.deucher@amd.com>
Signed-off-by: Vitaly Prosyak <vitaly.prosyak@amd.com>
Signed-off-by: David Francis <David.Francis@amd.com>
---
 tests/intel/gem_change_handle.c | 747 ++++++++++++++++++++++++++++++++
 tests/meson.build               |   1 +
 2 files changed, 748 insertions(+)
 create mode 100644 tests/intel/gem_change_handle.c

diff --git a/tests/intel/gem_change_handle.c b/tests/intel/gem_change_handle.c
new file mode 100644
index 000000000..23f2f8b39
--- /dev/null
+++ b/tests/intel/gem_change_handle.c
@@ -0,0 +1,747 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright 2026 Advanced Micro Devices, Inc.
+ * Copyright 2026 Intel Corporation
+ *
+ * GPU-agnostic race condition tests for GEM_CHANGE_HANDLE ioctl
+ *
+ * NOTE: This test is located in tests/intel/ directory because:
+ * 1. All gem_* tests are traditionally placed in tests/intel/ regardless
+ *    of GPU vendor, following IGT convention for GEM-related tests
+ * 2. The test uses i915/gem.h and i915/gem_create.h for GEM object creation
+ *    on Intel platforms, though it works vendor-agnostically via DRM core
+ * 3. Some subtest designs (noop-same-handle, invalid-*, edge-*, functional-*)
+ *    were proposed by Simona Vetter (Intel), hence Intel copyright header
+ *
+ */
+
+#include <errno.h>
+#include <fcntl.h>
+#include <limits.h>
+#include <pthread.h>
+#include <sched.h>
+#include <semaphore.h>
+#include <signal.h>
+#include <string.h>
+#include <sys/stat.h>
+
+#include "igt.h"
+#include "igt_device.h"
+#include "i915/gem.h"
+#include "i915/gem_create.h"
+
+/* AMDGPU includes (if available) */
+#if __has_include("igt_amd.h")
+#include "lib/amdgpu/amd_memory.h"
+#include "igt_amd.h"
+#define HAS_AMDGPU 1
+#else
+#define HAS_AMDGPU 0
+#endif
+
+#define BO_SIZE (16 * 1024)
+#define RACE_DURATION_SEC 1
+
+static bool verbose_logging;
+
+/**
+ * TEST: gem change handle
+ * Description: tests for drm ioctl GEM_CHANGE_HANDLE
+ * Category: Core
+ * Mega feature: General Core features
+ * Sub-category: GEM handles
+ * Functionality: gem change handle
+ * Feature: gem
+ * Run type: FULL
+ *
+ * SUBTEST: sanity
+ * Description: Changing a handle to a new value works as intended
+ *
+ * SUBTEST: change_prime
+ * Description: Changing a handle affects the result of prime import
+ *
+ * SUBTEST: new_handle_zero
+ * Description: Changing a handle to 0 should fail
+ *
+ * SUBTEST: new_handle_exceeds_int_max
+ * Description: Changing a handle to > INT_MAX should fail
+ *
+ * SUBTEST: new_handle_equals_old_handle
+ * Description: Changing a handle to itself should fail
+ *
+ * SUBTEST: new_handle_taken
+ * Description: Changing a handle to a value already in use should fail
+ *
+ * SUBTEST: invalid_handle
+ * Description: Changing a non-existent handle should fail
+ *
+ * SUBTEST: race_change_change
+ * Description: Test for race conditions between two GEM_CHANGE_HANDLE
+ *
+ * SUBTEST: race_change_create_close
+ * Description: Test for race conditions between GEM_CHANGE_HANDLE, GEM_CREATE, and GEM_CLOSE
+ *
+ * SUBTEST: race_change_export_import
+ * Description: Test for race conditions between GEM_CHANGE_HANDLE, prime export, and prime import
+ */
+
+IGT_TEST_DESCRIPTION("Edge case and race condition tests for GEM_CHANGE_HANDLE");
+
+/* Structs */
+
+/* Define DRM_IOCTL_GEM_CHANGE_HANDLE if not available in kernel headers */
+#ifndef DRM_IOCTL_GEM_CHANGE_HANDLE
+struct drm_gem_change_handle {
+	__u32 handle;
+	__u32 new_handle;
+};
+
+#define DRM_IOCTL_GEM_CHANGE_HANDLE    DRM_IOWR(0xD2, struct drm_gem_change_handle)
+#endif
+
+/* Driver type detection */
+enum gpu_driver {
+	DRIVER_TYPE_UNKNOWN = 0,
+	DRIVER_TYPE_I915,
+	DRIVER_TYPE_AMDGPU,
+};
+
+/* GPU context structure */
+struct gpu_ctx {
+	int fd;
+	enum gpu_driver driver;
+};
+
+/* Helpers */
+
+static uint32_t gem_create_bo(struct gpu_ctx *ctx, uint64_t size)
+{
+	uint32_t handle = 0;
+
+	switch (ctx->driver) {
+	case DRIVER_TYPE_I915: {
+		struct drm_i915_gem_create create = { .size = size };
+
+		do_ioctl(ctx->fd, DRM_IOCTL_I915_GEM_CREATE, &create);
+		handle = create.handle;
+		break;
+	}
+	case DRIVER_TYPE_AMDGPU:
+#if HAS_AMDGPU
+		handle = igt_amd_create_bo(ctx->fd, size);
+#else
+		igt_require_f(0, "AMDGPU header not found for AMDGPU driver\n");
+#endif
+		break;
+	default:
+		igt_assert_f(0, "Unsupported driver\n");
+	}
+
+	igt_assert(handle != 0);
+	return handle;
+}
+
+static int gem_close_bo(struct gpu_ctx *ctx, uint32_t handle)
+{
+	struct drm_gem_close close_args = { .handle = handle };
+
+	return igt_ioctl(ctx->fd, DRM_IOCTL_GEM_CLOSE, &close_args);
+}
+
+static int gem_change_handle(struct gpu_ctx *ctx, uint32_t old_handle, uint32_t new_handle)
+{
+	struct drm_gem_change_handle args = {
+		.handle = old_handle,
+		.new_handle = new_handle,
+	};
+	return igt_ioctl(ctx->fd, DRM_IOCTL_GEM_CHANGE_HANDLE, &args);
+}
+
+static enum gpu_driver detect_driver(int fd)
+{
+	/* Check AMDGPU first (primary target) */
+	if (is_amdgpu_device(fd))
+		return DRIVER_TYPE_AMDGPU;
+	if (is_i915_device(fd))
+		return DRIVER_TYPE_I915;
+	return DRIVER_TYPE_UNKNOWN;
+}
+
+/* Sanity and Functionality tests */
+
+/**
+ * test_sanity - verify rename actually works
+ *
+ * After a successful rename:
+ * - GEM_CLOSE(old_handle) must FAIL (handle no longer exists)
+ * - GEM_CLOSE(new_handle) must SUCCEED (object is there)
+ */
+static void test_sanity(struct gpu_ctx *ctx)
+{
+	uint32_t handle, new_h;
+	int ret;
+
+	handle = gem_create_bo(ctx, 4096);
+	new_h = handle + 1000;
+
+	/* Perform the rename */
+	ret = gem_change_handle(ctx, handle, new_h);
+	igt_assert_f(ret == 0,
+		     "gem_change_handle(%u -> %u) failed: %s\n",
+		     handle, new_h, strerror(errno));
+
+	/* Old handle must be invalid now */
+	ret = gem_close_bo(ctx, handle);
+	igt_assert_f(ret < 0 && errno == EINVAL,
+		     "GEM_CLOSE(old_handle=%u) should fail with EINVAL, "
+		     "got ret=%d errno=%d (%s)\n",
+		     handle, ret, errno, strerror(errno));
+
+	/* New handle must be valid */
+	ret = gem_close_bo(ctx, new_h);
+	igt_assert_f(ret == 0,
+		     "GEM_CLOSE(new_handle=%u) should succeed, "
+		     "got ret=%d errno=%d (%s)\n",
+		     new_h, ret, errno, strerror(errno));
+}
+
+/**
+ * test_change_export - changing a handle affects prime import
+ *
+ * Importing a prime object that has had its handle changed since it was
+ * exported should get the new handle
+ */
+static void test_change_export(struct gpu_ctx *ctx)
+{
+	int ret, handle1, handle2, dmabuf_fd, imported_handle;
+
+	/* Create a handle and export it */
+	handle1 = gem_create_bo(ctx, 4096);
+	handle2 = handle1 + 1;
+	dmabuf_fd = prime_handle_to_fd(ctx->fd, handle1);
+
+	/* When you import, you get the same handle back */
+	imported_handle = prime_fd_to_handle(ctx->fd, dmabuf_fd);
+	igt_assert_eq(imported_handle, handle1);
+
+	/* Change the handle (should succeed) */
+	ret = gem_change_handle(ctx, handle1, handle2);
+	igt_assert_eq(ret, 0);
+
+	/* Import, should get the new handle */
+	imported_handle = prime_fd_to_handle(ctx->fd, dmabuf_fd);
+	igt_assert_eq(imported_handle, handle2);
+
+	/* Confirm gem object is at the new handle */
+	igt_assert_eq(gem_close_bo(ctx, handle2), 0);
+
+	ret = gem_close_bo(ctx, handle1);
+	igt_assert(ret < 0);
+	igt_assert_eq(errno, EINVAL);
+
+	close(dmabuf_fd);
+}
+
+/* Edge case and fail case tests */
+
+/**
+ * test_new_handle_zero - new_handle = 0 should fail
+ *
+ * 0 is not a valid gem handle.
+ */
+static void test_new_handle_zero(struct gpu_ctx *ctx)
+{
+	uint32_t handle;
+	int ret;
+
+	handle = gem_create_bo(ctx, 4096);
+
+	/* new_handle = 0 must fail with -EINVAL */
+	ret = gem_change_handle(ctx, handle, 0);
+	igt_assert(ret < 0);
+	igt_assert_eq(errno, EINVAL);
+
+	/* Original handle should still be valid */
+	igt_assert_eq(gem_close_bo(ctx, handle), 0);
+}
+
+/**
+ * test_new_handle_exceeds_int_max - new_handle > INT_MAX should fail
+ *
+ * The kernel enforces an idr_alloc() limitation: new_handle must be <= INT_MAX.
+ */
+static void test_new_handle_exceeds_int_max(struct gpu_ctx *ctx)
+{
+	uint32_t handle;
+	int ret;
+
+	handle = gem_create_bo(ctx, 4096);
+
+	/* new_handle > INT_MAX must fail with -EINVAL */
+	ret = gem_change_handle(ctx, handle, (uint32_t)INT_MAX + 1);
+	igt_assert(ret < 0);
+	igt_assert_eq(errno, EINVAL);
+
+	/* Also test 0xFFFFFFFF */
+	ret = gem_change_handle(ctx, handle, 0xFFFFFFFF);
+	igt_assert(ret < 0);
+	igt_assert_eq(errno, EINVAL);
+
+	/* Original handle should still be valid */
+	igt_assert_eq(gem_close_bo(ctx, handle), 0);
+}
+
+/**
+ * test_new_handle_equals_old_handle - new_handle = old_handle should fail
+ *
+ * The ioctl treats old_handle = new_handle as invalid input.
+ */
+static void test_new_handle_equals_old_handle(struct gpu_ctx *ctx)
+{
+	uint32_t handle;
+	int ret;
+
+	handle = gem_create_bo(ctx, 4096);
+
+	/* new_handle = old_handle must fail with -EINVAL */
+	ret = gem_change_handle(ctx, handle, handle);
+	igt_assert(ret < 0);
+	igt_assert_eq(errno, EINVAL);
+
+	/* same thing if handle doesn't exist */
+	ret = gem_change_handle(ctx, handle + 1, handle + 1);
+	igt_assert(ret < 0);
+	igt_assert_eq(errno, EINVAL);
+
+	/* Original handle should still be valid */
+	igt_assert_eq(gem_close_bo(ctx, handle), 0);
+}
+
+/**
+ * test_new_handle_taken - new_handle being the handle of another gem object must fail
+ *
+ * Not allowed to change a handle to a handle that's already taken
+ */
+static void test_new_handle_taken(struct gpu_ctx *ctx)
+{
+	uint32_t handle, handle_2;
+	int ret;
+
+	handle = gem_create_bo(ctx, 4096);
+	handle_2 = gem_create_bo(ctx, 4096);
+
+	/* new_handle is taken must fail with -ENOSPC (direct from idr_alloc) */
+	ret = gem_change_handle(ctx, handle, handle_2);
+	igt_assert(ret < 0);
+	igt_assert_eq(errno, ENOSPC);
+
+	/* And the other way */
+	ret = gem_change_handle(ctx, handle_2, handle);
+	igt_assert(ret < 0);
+	igt_assert_eq(errno, ENOSPC);
+
+	/* Original handles should still be valid */
+	igt_assert_eq(gem_close_bo(ctx, handle), 0);
+	igt_assert_eq(gem_close_bo(ctx, handle_2), 0);
+}
+
+/**
+ * test_invalid_handle - non-existent handle should fail
+ *
+ * Using a handle that doesn't exist should return -ENOENT.
+ */
+static void test_invalid_handle(struct gpu_ctx *ctx)
+{
+	int ret;
+
+	/* Use a handle that was never allocated */
+	ret = gem_change_handle(ctx, 0xDEAD, 0xBEEF);
+	igt_assert(ret < 0);
+	igt_assert_eq(errno, ENOENT);
+
+	/* Handle 0 is also invalid */
+	ret = gem_change_handle(ctx, 0, 100);
+	igt_assert(ret < 0);
+	igt_assert_eq(errno, ENOENT);
+}
+
+/* Race condition test structs and helpers */
+
+struct change_handle_thread_data {
+	struct gpu_ctx *ctx;
+	uint32_t handles[4];
+	bool running;
+	int unexpected_errno;
+	pthread_barrier_t barrier;
+};
+
+static void set_up_context(struct change_handle_thread_data *data, struct gpu_ctx *ctx)
+{
+	data->handles[0] = gem_create_bo(ctx, BO_SIZE);
+	igt_assert(data->handles[0] != 0);
+	data->handles[1] = gem_create_bo(ctx, BO_SIZE);
+	igt_assert(data->handles[1] != 0);
+
+	data->ctx = ctx;
+	data->handles[2] = max(data->handles[0], data->handles[1]) + 0x10;
+	data->handles[3] = max(data->handles[0], data->handles[1]) + 0x20;
+	data->unexpected_errno = 0;
+	__atomic_store_n(&data->running, true, __ATOMIC_RELEASE);
+
+	pthread_barrier_init(&data->barrier, NULL, 2);
+}
+
+/* Race condition test threads */
+
+static void *change_handle_thread(void *arg)
+{
+	struct change_handle_thread_data *data = arg;
+	uint32_t *h = data->handles;
+	int from[] = {h[0], h[2], h[0], h[3], h[3], h[1], h[1], h[3], h[2], h[1], h[0], h[2]};
+	int to[] =   {h[1], h[3], h[2], h[1], h[0], h[2], h[0], h[2], h[0], h[3], h[3], h[1]};
+	int ret;
+
+	pthread_barrier_wait(&data->barrier);
+
+	while (__atomic_load_n(&data->running, __ATOMIC_ACQUIRE)) {
+		for (int i = 0; i < 12; i++) {
+			ret = gem_change_handle(data->ctx, from[i], to[i]);
+			/*
+			 * Expected errors:
+			 * - ENOENT: old handle invalid
+			 * - ENOSPC: target handle occupied (collision detected during idr_alloc)
+			 */
+			if (ret != 0 && errno != ENOENT && errno != ENOSPC)
+				data->unexpected_errno = errno;
+		}
+
+		sched_yield();
+	}
+
+       return NULL;
+}
+
+static void *close_recreate_thread(void *arg)
+{
+	struct change_handle_thread_data *data = arg;
+	uint32_t *h = data->handles;
+	pthread_t tid = pthread_self();
+	int handle, i, ret;
+
+	pthread_barrier_wait(&data->barrier);
+
+	while (__atomic_load_n(&data->running, __ATOMIC_ACQUIRE)) {
+		/* Close a handle; keep trying until success */
+		i = 0;
+		while (true) {
+			ret = gem_close_bo(data->ctx, h[i]);
+			if (ret == 0) {
+				if (verbose_logging) {
+					igt_info("[CLOSE-TID:%lu] Closed handle %u\n",
+						 (unsigned long)tid, h[i]);
+				}
+				break;
+			}
+
+			i += 1;
+			if (i > 3)
+				i = 0;
+
+			if (!__atomic_load_n(&data->running, __ATOMIC_ACQUIRE))
+				return NULL;
+		}
+
+		/* Open a handle */
+		handle = gem_create_bo(data->ctx, BO_SIZE);
+		if (verbose_logging) {
+			igt_info("[CLOSE-TID:%lu] Created new BO with handle %u\n",
+				 (unsigned long)tid, handle);
+		}
+
+		/* If the opened handle isn't one of the four ones being
+		 * shuffled, change_handle it in. Keep trying until success.
+		 */
+		if (handle != h[0] && handle != h[1] && handle != h[2] && handle != h[3]) {
+			i = 0;
+			while (true) {
+				ret = gem_change_handle(data->ctx, handle, h[i]);
+				if (ret == 0) {
+					if (verbose_logging) {
+						igt_info("[CLOSE-TID:%lu] Change handle %u to %u\n",
+							 (unsigned long)tid, handle, h[i]);
+					}
+					break;
+				}
+
+				if (ret != 0 && errno != ENOENT && errno != ENOSPC)
+					data->unexpected_errno = errno;
+
+				i += 1;
+				if (i > 3)
+					i = 0;
+
+				if (!__atomic_load_n(&data->running, __ATOMIC_ACQUIRE))
+					return NULL;
+			}
+		}
+
+		sched_yield();
+	}
+
+	return NULL;
+}
+
+static void *export_import_thread(void *arg)
+{
+	struct change_handle_thread_data *data = arg;
+	struct drm_prime_handle prime_args;
+	uint32_t *h = data->handles;
+	pthread_t tid = pthread_self();
+	int handle, i, ret, dmabuf_fd;
+
+	pthread_barrier_wait(&data->barrier);
+
+	while (__atomic_load_n(&data->running, __ATOMIC_ACQUIRE)) {
+		/* Export a handle; keep trying until success */
+		i = 0;
+		while (true) {
+			prime_args.handle = h[i];
+			prime_args.flags = DRM_CLOEXEC | DRM_RDWR;
+			ret = igt_ioctl(data->ctx->fd, DRM_IOCTL_PRIME_HANDLE_TO_FD, &prime_args);
+			if (ret == 0) {
+				if (verbose_logging) {
+					igt_info("[PRIME-TID:%lu] Exported handle %u\n",
+						 (unsigned long)tid, h[i]);
+				}
+				break;
+			}
+
+			i += 1;
+			if (i > 3)
+				i = 0;
+
+			if (!__atomic_load_n(&data->running, __ATOMIC_ACQUIRE))
+				return NULL;
+		}
+
+		dmabuf_fd = prime_args.fd;
+
+		/* Import the handle; it should be one of the four handles in the shuffle */
+		prime_args.fd = dmabuf_fd;
+		prime_args.flags = 0;
+		ret = igt_ioctl(data->ctx->fd, DRM_IOCTL_PRIME_FD_TO_HANDLE, &prime_args);
+
+		igt_assert_eq(ret, 0);
+		handle = prime_args.handle;
+		if (verbose_logging) {
+			igt_info("[PRIME-TID:%lu] Imported; got handle %u\n",
+				 (unsigned long)tid, handle);
+		}
+		igt_assert(handle == h[0] || handle == h[1] || handle == h[2] || handle == h[3]);
+
+		close(dmabuf_fd);
+
+		sched_yield();
+	}
+
+	return NULL;
+}
+
+/* Race condition tests */
+
+/**
+ * test_race_change_change - Test for race conditions of change_handle against itself
+ *
+ * Start two threads, each of which constantly attempt changes between four different handles
+ * Try to catch race conditions such as changing same handle to two different handles,
+ * Changing two handles to the same handle, changing a handle to a handle itself being changed,
+ * etc.
+ */
+static void test_race_change_change(struct gpu_ctx *ctx)
+{
+	pthread_t thread_change_1, thread_change_2;
+	struct change_handle_thread_data data = {0};
+	int num_handles, ret;
+
+	set_up_context(&data, ctx);
+
+	/* Start both threads simultaneously */
+	igt_assert_eq(pthread_create(&thread_change_1, NULL, change_handle_thread, &data), 0);
+	igt_assert_eq(pthread_create(&thread_change_2, NULL, change_handle_thread, &data), 0);
+
+	/* Let them race */
+	sleep(RACE_DURATION_SEC);
+
+	__atomic_store_n(&data.running, false, __ATOMIC_RELEASE);
+	pthread_join(thread_change_1, NULL);
+	pthread_join(thread_change_2, NULL);
+
+	pthread_barrier_destroy(&data.barrier);
+
+	igt_assert_eq(data.unexpected_errno, 0);
+
+	/* Exactly two gem objects should still exist */
+	num_handles = 0;
+	for (int i = 0; i < 4; i++) {
+		ret = gem_close_bo(ctx, data.handles[i]);
+
+		if (ret == 0) {
+			num_handles += 1;
+		} else {
+			igt_assert_eq(errno, EINVAL);
+		}
+	}
+	igt_assert_eq(num_handles, 2);
+}
+
+/**
+ * test_race_change_create_close - Test for race conditions of change_handle against gem create / close
+ *
+ * Start two threads, one of which attempts to change between four handles while the other alternates
+ * closing a gem handle and creating new ones. Test for race conditions such as change a handle that's
+ * being closed or change a handle to a value that's being created.
+ */
+static void test_race_change_create_close(struct gpu_ctx *ctx)
+{
+	pthread_t thread_change, thread_create_close;
+	struct change_handle_thread_data data = {0};
+	int num_handles, ret;
+
+	set_up_context(&data, ctx);
+
+	/* Start both threads simultaneously */
+	igt_assert_eq(pthread_create(&thread_change, NULL, change_handle_thread, &data), 0);
+	igt_assert_eq(pthread_create(&thread_create_close, NULL, close_recreate_thread, &data), 0);
+
+	/* Let them race */
+	sleep(RACE_DURATION_SEC);
+
+	__atomic_store_n(&data.running, false, __ATOMIC_RELEASE);
+	pthread_join(thread_change, NULL);
+	pthread_join(thread_create_close, NULL);
+
+	pthread_barrier_destroy(&data.barrier);
+
+	igt_assert_eq(data.unexpected_errno, 0);
+
+	/* Depending on where create / close ended up, there can be 1 or 2 handles open */
+	num_handles = 0;
+	for (int i = 0; i < 4; i++) {
+		ret = gem_close_bo(ctx, data.handles[i]);
+
+		if (ret == 0) {
+			num_handles += 1;
+		} else {
+			igt_assert_eq(errno, EINVAL);
+		}
+	}
+	igt_assert(num_handles >= 1 && num_handles <= 2);
+}
+
+/**
+ * test_race_change_export_import - Test for race conditions of change_handle against prime export / import
+ *
+ * Start two threads, one of which attempts to change between four handles while the other exports
+ * handles, then imports them again. Check for race conditions such as changing a handle while exporting
+ * it or important a handle while changing it.
+ */
+static void test_race_change_export_import(struct gpu_ctx *ctx)
+{
+	pthread_t thread_change, thread_export_import;
+	struct change_handle_thread_data data = {0};
+	int num_handles, ret;
+
+	set_up_context(&data, ctx);
+
+	/* Start both threads simultaneously */
+	igt_assert_eq(pthread_create(&thread_change, NULL, change_handle_thread, &data), 0);
+	igt_assert_eq(pthread_create(&thread_export_import, NULL, export_import_thread, &data), 0);
+
+	/* Let them race */
+	sleep(RACE_DURATION_SEC);
+
+	__atomic_store_n(&data.running, false, __ATOMIC_RELEASE);
+	pthread_join(thread_change, NULL);
+	pthread_join(thread_export_import, NULL);
+
+	pthread_barrier_destroy(&data.barrier);
+
+	igt_assert_eq(data.unexpected_errno, 0);
+
+	/* Exporting a handle doesn't close it, so should still be exactly 2 */
+	num_handles = 0;
+	for (int i = 0; i < 4; i++) {
+		ret = gem_close_bo(ctx, data.handles[i]);
+
+		if (ret == 0) {
+			num_handles += 1;
+		} else {
+			igt_assert_eq(errno, EINVAL);
+		}
+	}
+	igt_assert_eq(num_handles, 2);
+}
+
+int igt_main()
+{
+	struct gpu_ctx ctx = { .fd = -1 };
+
+	igt_fixture() {
+		ctx.fd = drm_open_driver_render(DRIVER_ANY);
+		igt_require(ctx.fd >= 0);
+
+		ctx.driver = detect_driver(ctx.fd);
+		igt_require_f(ctx.driver != DRIVER_TYPE_UNKNOWN,
+			      "Unsupported GPU driver\n");
+
+		if (ctx.driver == DRIVER_TYPE_I915)
+			igt_require_gem(ctx.fd);
+	}
+
+	igt_describe("Sanity Test: Change handle actually changes handle");
+	igt_subtest("sanity")
+		test_sanity(&ctx);
+
+	igt_describe("Functinality Test: Change handle affects result of prime import");
+	igt_subtest("change_prime")
+		test_change_export(&ctx);
+
+
+	igt_describe("Edge Case: New handle = 0 should fail");
+	igt_subtest("new_handle_zero")
+		test_new_handle_zero(&ctx);
+
+	igt_describe("Edge Case: New handle > INT_MAX should fail");
+	igt_subtest("new_handle_exceeds_int_max")
+		test_new_handle_exceeds_int_max(&ctx);
+
+	igt_describe("Edge Case: Changing a handle to itself should fail");
+	igt_subtest("new_handle_equals_old_handle")
+		test_new_handle_equals_old_handle(&ctx);
+
+	igt_describe("Edge Case: Change a handle to a value which is taken should fail");
+	igt_subtest("new_handle_taken")
+		test_new_handle_taken(&ctx);
+
+	igt_describe("Edge Case: Changing a non-existent handle should fail");
+	igt_subtest("invalid_handle")
+		test_invalid_handle(&ctx);
+
+
+	igt_describe("Race: Changing handle vs. Changing handle");
+	igt_subtest("race_change_change")
+		test_race_change_change(&ctx);
+
+	igt_describe("Race: Changing handle vs. Create / Close");
+	igt_subtest("race_change_create_close")
+		test_race_change_create_close(&ctx);
+
+	igt_describe("Race: Changing handle vs. Export / Import");
+	igt_subtest("race_change_export_import")
+		test_race_change_export_import(&ctx);
+
+	igt_fixture() {
+		drm_close_driver(ctx.fd);
+	}
+
+}
\ No newline at end of file
diff --git a/tests/meson.build b/tests/meson.build
index 4f5c9be7b..ab083916d 100644
--- a/tests/meson.build
+++ b/tests/meson.build
@@ -104,6 +104,7 @@ intel_i915_progs = [
 	'gem_blits',
 	'gem_busy',
 	'gem_caching',
+	'gem_change_handle',
 	'gem_create',
 	'gem_ccs',
 	'gem_close',
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH] tests/intel: Add edge case and race condition tests for GEM_CHANGE_HANDLE
  2026-10-07 14:12 [PATCH] tests/intel: Add edge case and race condition tests for GEM_CHANGE_HANDLE David Francis
@ 2026-10-07 14:13 ` Francis, David
  2026-10-07 17:05   ` Francis, David
  2026-10-07 15:49 ` ✗ Fi.CI.BUILD: failure for tests/intel: Add edge case and race condition tests for GEM_CHANGE_HANDLE (rev2) Patchwork
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 7+ messages in thread
From: Francis, David @ 2026-10-07 14:13 UTC (permalink / raw)
  To: igt-dev@lists.freedesktop.org
  Cc: tvrtko.ursulin@igalia.com, Prosyak, Vitaly, Kamil Konieczny,
	Sebastian Brzezinka, Koenig, Christian, Simona Vetter,
	Deucher, Alexander

(Vitaly specifically requested that his Signed-off-by be preserved here, as this is a modification of his original tests)

David Francis

________________________________________
From: Francis, David <David.Francis@amd.com>
Sent: Wednesday, October 7, 2026 10:12 AM
To: igt-dev@lists.freedesktop.org
Cc: tvrtko.ursulin@igalia.com; Prosyak, Vitaly; Kamil Konieczny; Sebastian Brzezinka; Koenig, Christian; Simona Vetter; Deucher, Alexander; Francis, David
Subject: [PATCH] tests/intel: Add edge case and race condition tests for GEM_CHANGE_HANDLE

From: Vitaly Prosyak <vitaly.prosyak@amd.com>

Add a GPU-agnostic GEM_CHANGE_HANDLE tests. These test for basic
functionality, edge cases, and race conditions of this ioctl against
itself, gem close, gem create, and prime import and export.

GEM_CHANGE_HANDLE is currently wired to drm_invalid_op in DRM core. This IGT
coverage is a prerequisite for validating the eventual kernel-side ioctl
implementation and makes fixing its security bugs safer.

Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
Cc: Sebastian Brzezinka <sebastian.brzezinka@intel.com>
Cc: Christian Konig <christian.koenig@amd.com>
Cc: Simona Vetter <simona@ffwll.ch>
Cc: Alex Deucher <alexander.deucher@amd.com>
Signed-off-by: Vitaly Prosyak <vitaly.prosyak@amd.com>
Signed-off-by: David Francis <David.Francis@amd.com>
---
 tests/intel/gem_change_handle.c | 747 ++++++++++++++++++++++++++++++++
 tests/meson.build               |   1 +
 2 files changed, 748 insertions(+)
 create mode 100644 tests/intel/gem_change_handle.c

diff --git a/tests/intel/gem_change_handle.c b/tests/intel/gem_change_handle.c
new file mode 100644
index 000000000..23f2f8b39
--- /dev/null
+++ b/tests/intel/gem_change_handle.c
@@ -0,0 +1,747 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright 2026 Advanced Micro Devices, Inc.
+ * Copyright 2026 Intel Corporation
+ *
+ * GPU-agnostic race condition tests for GEM_CHANGE_HANDLE ioctl
+ *
+ * NOTE: This test is located in tests/intel/ directory because:
+ * 1. All gem_* tests are traditionally placed in tests/intel/ regardless
+ *    of GPU vendor, following IGT convention for GEM-related tests
+ * 2. The test uses i915/gem.h and i915/gem_create.h for GEM object creation
+ *    on Intel platforms, though it works vendor-agnostically via DRM core
+ * 3. Some subtest designs (noop-same-handle, invalid-*, edge-*, functional-*)
+ *    were proposed by Simona Vetter (Intel), hence Intel copyright header
+ *
+ */
+
+#include <errno.h>
+#include <fcntl.h>
+#include <limits.h>
+#include <pthread.h>
+#include <sched.h>
+#include <semaphore.h>
+#include <signal.h>
+#include <string.h>
+#include <sys/stat.h>
+
+#include "igt.h"
+#include "igt_device.h"
+#include "i915/gem.h"
+#include "i915/gem_create.h"
+
+/* AMDGPU includes (if available) */
+#if __has_include("igt_amd.h")
+#include "lib/amdgpu/amd_memory.h"
+#include "igt_amd.h"
+#define HAS_AMDGPU 1
+#else
+#define HAS_AMDGPU 0
+#endif
+
+#define BO_SIZE (16 * 1024)
+#define RACE_DURATION_SEC 1
+
+static bool verbose_logging;
+
+/**
+ * TEST: gem change handle
+ * Description: tests for drm ioctl GEM_CHANGE_HANDLE
+ * Category: Core
+ * Mega feature: General Core features
+ * Sub-category: GEM handles
+ * Functionality: gem change handle
+ * Feature: gem
+ * Run type: FULL
+ *
+ * SUBTEST: sanity
+ * Description: Changing a handle to a new value works as intended
+ *
+ * SUBTEST: change_prime
+ * Description: Changing a handle affects the result of prime import
+ *
+ * SUBTEST: new_handle_zero
+ * Description: Changing a handle to 0 should fail
+ *
+ * SUBTEST: new_handle_exceeds_int_max
+ * Description: Changing a handle to > INT_MAX should fail
+ *
+ * SUBTEST: new_handle_equals_old_handle
+ * Description: Changing a handle to itself should fail
+ *
+ * SUBTEST: new_handle_taken
+ * Description: Changing a handle to a value already in use should fail
+ *
+ * SUBTEST: invalid_handle
+ * Description: Changing a non-existent handle should fail
+ *
+ * SUBTEST: race_change_change
+ * Description: Test for race conditions between two GEM_CHANGE_HANDLE
+ *
+ * SUBTEST: race_change_create_close
+ * Description: Test for race conditions between GEM_CHANGE_HANDLE, GEM_CREATE, and GEM_CLOSE
+ *
+ * SUBTEST: race_change_export_import
+ * Description: Test for race conditions between GEM_CHANGE_HANDLE, prime export, and prime import
+ */
+
+IGT_TEST_DESCRIPTION("Edge case and race condition tests for GEM_CHANGE_HANDLE");
+
+/* Structs */
+
+/* Define DRM_IOCTL_GEM_CHANGE_HANDLE if not available in kernel headers */
+#ifndef DRM_IOCTL_GEM_CHANGE_HANDLE
+struct drm_gem_change_handle {
+       __u32 handle;
+       __u32 new_handle;
+};
+
+#define DRM_IOCTL_GEM_CHANGE_HANDLE    DRM_IOWR(0xD2, struct drm_gem_change_handle)
+#endif
+
+/* Driver type detection */
+enum gpu_driver {
+       DRIVER_TYPE_UNKNOWN = 0,
+       DRIVER_TYPE_I915,
+       DRIVER_TYPE_AMDGPU,
+};
+
+/* GPU context structure */
+struct gpu_ctx {
+       int fd;
+       enum gpu_driver driver;
+};
+
+/* Helpers */
+
+static uint32_t gem_create_bo(struct gpu_ctx *ctx, uint64_t size)
+{
+       uint32_t handle = 0;
+
+       switch (ctx->driver) {
+       case DRIVER_TYPE_I915: {
+               struct drm_i915_gem_create create = { .size = size };
+
+               do_ioctl(ctx->fd, DRM_IOCTL_I915_GEM_CREATE, &create);
+               handle = create.handle;
+               break;
+       }
+       case DRIVER_TYPE_AMDGPU:
+#if HAS_AMDGPU
+               handle = igt_amd_create_bo(ctx->fd, size);
+#else
+               igt_require_f(0, "AMDGPU header not found for AMDGPU driver\n");
+#endif
+               break;
+       default:
+               igt_assert_f(0, "Unsupported driver\n");
+       }
+
+       igt_assert(handle != 0);
+       return handle;
+}
+
+static int gem_close_bo(struct gpu_ctx *ctx, uint32_t handle)
+{
+       struct drm_gem_close close_args = { .handle = handle };
+
+       return igt_ioctl(ctx->fd, DRM_IOCTL_GEM_CLOSE, &close_args);
+}
+
+static int gem_change_handle(struct gpu_ctx *ctx, uint32_t old_handle, uint32_t new_handle)
+{
+       struct drm_gem_change_handle args = {
+               .handle = old_handle,
+               .new_handle = new_handle,
+       };
+       return igt_ioctl(ctx->fd, DRM_IOCTL_GEM_CHANGE_HANDLE, &args);
+}
+
+static enum gpu_driver detect_driver(int fd)
+{
+       /* Check AMDGPU first (primary target) */
+       if (is_amdgpu_device(fd))
+               return DRIVER_TYPE_AMDGPU;
+       if (is_i915_device(fd))
+               return DRIVER_TYPE_I915;
+       return DRIVER_TYPE_UNKNOWN;
+}
+
+/* Sanity and Functionality tests */
+
+/**
+ * test_sanity - verify rename actually works
+ *
+ * After a successful rename:
+ * - GEM_CLOSE(old_handle) must FAIL (handle no longer exists)
+ * - GEM_CLOSE(new_handle) must SUCCEED (object is there)
+ */
+static void test_sanity(struct gpu_ctx *ctx)
+{
+       uint32_t handle, new_h;
+       int ret;
+
+       handle = gem_create_bo(ctx, 4096);
+       new_h = handle + 1000;
+
+       /* Perform the rename */
+       ret = gem_change_handle(ctx, handle, new_h);
+       igt_assert_f(ret == 0,
+                    "gem_change_handle(%u -> %u) failed: %s\n",
+                    handle, new_h, strerror(errno));
+
+       /* Old handle must be invalid now */
+       ret = gem_close_bo(ctx, handle);
+       igt_assert_f(ret < 0 && errno == EINVAL,
+                    "GEM_CLOSE(old_handle=%u) should fail with EINVAL, "
+                    "got ret=%d errno=%d (%s)\n",
+                    handle, ret, errno, strerror(errno));
+
+       /* New handle must be valid */
+       ret = gem_close_bo(ctx, new_h);
+       igt_assert_f(ret == 0,
+                    "GEM_CLOSE(new_handle=%u) should succeed, "
+                    "got ret=%d errno=%d (%s)\n",
+                    new_h, ret, errno, strerror(errno));
+}
+
+/**
+ * test_change_export - changing a handle affects prime import
+ *
+ * Importing a prime object that has had its handle changed since it was
+ * exported should get the new handle
+ */
+static void test_change_export(struct gpu_ctx *ctx)
+{
+       int ret, handle1, handle2, dmabuf_fd, imported_handle;
+
+       /* Create a handle and export it */
+       handle1 = gem_create_bo(ctx, 4096);
+       handle2 = handle1 + 1;
+       dmabuf_fd = prime_handle_to_fd(ctx->fd, handle1);
+
+       /* When you import, you get the same handle back */
+       imported_handle = prime_fd_to_handle(ctx->fd, dmabuf_fd);
+       igt_assert_eq(imported_handle, handle1);
+
+       /* Change the handle (should succeed) */
+       ret = gem_change_handle(ctx, handle1, handle2);
+       igt_assert_eq(ret, 0);
+
+       /* Import, should get the new handle */
+       imported_handle = prime_fd_to_handle(ctx->fd, dmabuf_fd);
+       igt_assert_eq(imported_handle, handle2);
+
+       /* Confirm gem object is at the new handle */
+       igt_assert_eq(gem_close_bo(ctx, handle2), 0);
+
+       ret = gem_close_bo(ctx, handle1);
+       igt_assert(ret < 0);
+       igt_assert_eq(errno, EINVAL);
+
+       close(dmabuf_fd);
+}
+
+/* Edge case and fail case tests */
+
+/**
+ * test_new_handle_zero - new_handle = 0 should fail
+ *
+ * 0 is not a valid gem handle.
+ */
+static void test_new_handle_zero(struct gpu_ctx *ctx)
+{
+       uint32_t handle;
+       int ret;
+
+       handle = gem_create_bo(ctx, 4096);
+
+       /* new_handle = 0 must fail with -EINVAL */
+       ret = gem_change_handle(ctx, handle, 0);
+       igt_assert(ret < 0);
+       igt_assert_eq(errno, EINVAL);
+
+       /* Original handle should still be valid */
+       igt_assert_eq(gem_close_bo(ctx, handle), 0);
+}
+
+/**
+ * test_new_handle_exceeds_int_max - new_handle > INT_MAX should fail
+ *
+ * The kernel enforces an idr_alloc() limitation: new_handle must be <= INT_MAX.
+ */
+static void test_new_handle_exceeds_int_max(struct gpu_ctx *ctx)
+{
+       uint32_t handle;
+       int ret;
+
+       handle = gem_create_bo(ctx, 4096);
+
+       /* new_handle > INT_MAX must fail with -EINVAL */
+       ret = gem_change_handle(ctx, handle, (uint32_t)INT_MAX + 1);
+       igt_assert(ret < 0);
+       igt_assert_eq(errno, EINVAL);
+
+       /* Also test 0xFFFFFFFF */
+       ret = gem_change_handle(ctx, handle, 0xFFFFFFFF);
+       igt_assert(ret < 0);
+       igt_assert_eq(errno, EINVAL);
+
+       /* Original handle should still be valid */
+       igt_assert_eq(gem_close_bo(ctx, handle), 0);
+}
+
+/**
+ * test_new_handle_equals_old_handle - new_handle = old_handle should fail
+ *
+ * The ioctl treats old_handle = new_handle as invalid input.
+ */
+static void test_new_handle_equals_old_handle(struct gpu_ctx *ctx)
+{
+       uint32_t handle;
+       int ret;
+
+       handle = gem_create_bo(ctx, 4096);
+
+       /* new_handle = old_handle must fail with -EINVAL */
+       ret = gem_change_handle(ctx, handle, handle);
+       igt_assert(ret < 0);
+       igt_assert_eq(errno, EINVAL);
+
+       /* same thing if handle doesn't exist */
+       ret = gem_change_handle(ctx, handle + 1, handle + 1);
+       igt_assert(ret < 0);
+       igt_assert_eq(errno, EINVAL);
+
+       /* Original handle should still be valid */
+       igt_assert_eq(gem_close_bo(ctx, handle), 0);
+}
+
+/**
+ * test_new_handle_taken - new_handle being the handle of another gem object must fail
+ *
+ * Not allowed to change a handle to a handle that's already taken
+ */
+static void test_new_handle_taken(struct gpu_ctx *ctx)
+{
+       uint32_t handle, handle_2;
+       int ret;
+
+       handle = gem_create_bo(ctx, 4096);
+       handle_2 = gem_create_bo(ctx, 4096);
+
+       /* new_handle is taken must fail with -ENOSPC (direct from idr_alloc) */
+       ret = gem_change_handle(ctx, handle, handle_2);
+       igt_assert(ret < 0);
+       igt_assert_eq(errno, ENOSPC);
+
+       /* And the other way */
+       ret = gem_change_handle(ctx, handle_2, handle);
+       igt_assert(ret < 0);
+       igt_assert_eq(errno, ENOSPC);
+
+       /* Original handles should still be valid */
+       igt_assert_eq(gem_close_bo(ctx, handle), 0);
+       igt_assert_eq(gem_close_bo(ctx, handle_2), 0);
+}
+
+/**
+ * test_invalid_handle - non-existent handle should fail
+ *
+ * Using a handle that doesn't exist should return -ENOENT.
+ */
+static void test_invalid_handle(struct gpu_ctx *ctx)
+{
+       int ret;
+
+       /* Use a handle that was never allocated */
+       ret = gem_change_handle(ctx, 0xDEAD, 0xBEEF);
+       igt_assert(ret < 0);
+       igt_assert_eq(errno, ENOENT);
+
+       /* Handle 0 is also invalid */
+       ret = gem_change_handle(ctx, 0, 100);
+       igt_assert(ret < 0);
+       igt_assert_eq(errno, ENOENT);
+}
+
+/* Race condition test structs and helpers */
+
+struct change_handle_thread_data {
+       struct gpu_ctx *ctx;
+       uint32_t handles[4];
+       bool running;
+       int unexpected_errno;
+       pthread_barrier_t barrier;
+};
+
+static void set_up_context(struct change_handle_thread_data *data, struct gpu_ctx *ctx)
+{
+       data->handles[0] = gem_create_bo(ctx, BO_SIZE);
+       igt_assert(data->handles[0] != 0);
+       data->handles[1] = gem_create_bo(ctx, BO_SIZE);
+       igt_assert(data->handles[1] != 0);
+
+       data->ctx = ctx;
+       data->handles[2] = max(data->handles[0], data->handles[1]) + 0x10;
+       data->handles[3] = max(data->handles[0], data->handles[1]) + 0x20;
+       data->unexpected_errno = 0;
+       __atomic_store_n(&data->running, true, __ATOMIC_RELEASE);
+
+       pthread_barrier_init(&data->barrier, NULL, 2);
+}
+
+/* Race condition test threads */
+
+static void *change_handle_thread(void *arg)
+{
+       struct change_handle_thread_data *data = arg;
+       uint32_t *h = data->handles;
+       int from[] = {h[0], h[2], h[0], h[3], h[3], h[1], h[1], h[3], h[2], h[1], h[0], h[2]};
+       int to[] =   {h[1], h[3], h[2], h[1], h[0], h[2], h[0], h[2], h[0], h[3], h[3], h[1]};
+       int ret;
+
+       pthread_barrier_wait(&data->barrier);
+
+       while (__atomic_load_n(&data->running, __ATOMIC_ACQUIRE)) {
+               for (int i = 0; i < 12; i++) {
+                       ret = gem_change_handle(data->ctx, from[i], to[i]);
+                       /*
+                        * Expected errors:
+                        * - ENOENT: old handle invalid
+                        * - ENOSPC: target handle occupied (collision detected during idr_alloc)
+                        */
+                       if (ret != 0 && errno != ENOENT && errno != ENOSPC)
+                               data->unexpected_errno = errno;
+               }
+
+               sched_yield();
+       }
+
+       return NULL;
+}
+
+static void *close_recreate_thread(void *arg)
+{
+       struct change_handle_thread_data *data = arg;
+       uint32_t *h = data->handles;
+       pthread_t tid = pthread_self();
+       int handle, i, ret;
+
+       pthread_barrier_wait(&data->barrier);
+
+       while (__atomic_load_n(&data->running, __ATOMIC_ACQUIRE)) {
+               /* Close a handle; keep trying until success */
+               i = 0;
+               while (true) {
+                       ret = gem_close_bo(data->ctx, h[i]);
+                       if (ret == 0) {
+                               if (verbose_logging) {
+                                       igt_info("[CLOSE-TID:%lu] Closed handle %u\n",
+                                                (unsigned long)tid, h[i]);
+                               }
+                               break;
+                       }
+
+                       i += 1;
+                       if (i > 3)
+                               i = 0;
+
+                       if (!__atomic_load_n(&data->running, __ATOMIC_ACQUIRE))
+                               return NULL;
+               }
+
+               /* Open a handle */
+               handle = gem_create_bo(data->ctx, BO_SIZE);
+               if (verbose_logging) {
+                       igt_info("[CLOSE-TID:%lu] Created new BO with handle %u\n",
+                                (unsigned long)tid, handle);
+               }
+
+               /* If the opened handle isn't one of the four ones being
+                * shuffled, change_handle it in. Keep trying until success.
+                */
+               if (handle != h[0] && handle != h[1] && handle != h[2] && handle != h[3]) {
+                       i = 0;
+                       while (true) {
+                               ret = gem_change_handle(data->ctx, handle, h[i]);
+                               if (ret == 0) {
+                                       if (verbose_logging) {
+                                               igt_info("[CLOSE-TID:%lu] Change handle %u to %u\n",
+                                                        (unsigned long)tid, handle, h[i]);
+                                       }
+                                       break;
+                               }
+
+                               if (ret != 0 && errno != ENOENT && errno != ENOSPC)
+                                       data->unexpected_errno = errno;
+
+                               i += 1;
+                               if (i > 3)
+                                       i = 0;
+
+                               if (!__atomic_load_n(&data->running, __ATOMIC_ACQUIRE))
+                                       return NULL;
+                       }
+               }
+
+               sched_yield();
+       }
+
+       return NULL;
+}
+
+static void *export_import_thread(void *arg)
+{
+       struct change_handle_thread_data *data = arg;
+       struct drm_prime_handle prime_args;
+       uint32_t *h = data->handles;
+       pthread_t tid = pthread_self();
+       int handle, i, ret, dmabuf_fd;
+
+       pthread_barrier_wait(&data->barrier);
+
+       while (__atomic_load_n(&data->running, __ATOMIC_ACQUIRE)) {
+               /* Export a handle; keep trying until success */
+               i = 0;
+               while (true) {
+                       prime_args.handle = h[i];
+                       prime_args.flags = DRM_CLOEXEC | DRM_RDWR;
+                       ret = igt_ioctl(data->ctx->fd, DRM_IOCTL_PRIME_HANDLE_TO_FD, &prime_args);
+                       if (ret == 0) {
+                               if (verbose_logging) {
+                                       igt_info("[PRIME-TID:%lu] Exported handle %u\n",
+                                                (unsigned long)tid, h[i]);
+                               }
+                               break;
+                       }
+
+                       i += 1;
+                       if (i > 3)
+                               i = 0;
+
+                       if (!__atomic_load_n(&data->running, __ATOMIC_ACQUIRE))
+                               return NULL;
+               }
+
+               dmabuf_fd = prime_args.fd;
+
+               /* Import the handle; it should be one of the four handles in the shuffle */
+               prime_args.fd = dmabuf_fd;
+               prime_args.flags = 0;
+               ret = igt_ioctl(data->ctx->fd, DRM_IOCTL_PRIME_FD_TO_HANDLE, &prime_args);
+
+               igt_assert_eq(ret, 0);
+               handle = prime_args.handle;
+               if (verbose_logging) {
+                       igt_info("[PRIME-TID:%lu] Imported; got handle %u\n",
+                                (unsigned long)tid, handle);
+               }
+               igt_assert(handle == h[0] || handle == h[1] || handle == h[2] || handle == h[3]);
+
+               close(dmabuf_fd);
+
+               sched_yield();
+       }
+
+       return NULL;
+}
+
+/* Race condition tests */
+
+/**
+ * test_race_change_change - Test for race conditions of change_handle against itself
+ *
+ * Start two threads, each of which constantly attempt changes between four different handles
+ * Try to catch race conditions such as changing same handle to two different handles,
+ * Changing two handles to the same handle, changing a handle to a handle itself being changed,
+ * etc.
+ */
+static void test_race_change_change(struct gpu_ctx *ctx)
+{
+       pthread_t thread_change_1, thread_change_2;
+       struct change_handle_thread_data data = {0};
+       int num_handles, ret;
+
+       set_up_context(&data, ctx);
+
+       /* Start both threads simultaneously */
+       igt_assert_eq(pthread_create(&thread_change_1, NULL, change_handle_thread, &data), 0);
+       igt_assert_eq(pthread_create(&thread_change_2, NULL, change_handle_thread, &data), 0);
+
+       /* Let them race */
+       sleep(RACE_DURATION_SEC);
+
+       __atomic_store_n(&data.running, false, __ATOMIC_RELEASE);
+       pthread_join(thread_change_1, NULL);
+       pthread_join(thread_change_2, NULL);
+
+       pthread_barrier_destroy(&data.barrier);
+
+       igt_assert_eq(data.unexpected_errno, 0);
+
+       /* Exactly two gem objects should still exist */
+       num_handles = 0;
+       for (int i = 0; i < 4; i++) {
+               ret = gem_close_bo(ctx, data.handles[i]);
+
+               if (ret == 0) {
+                       num_handles += 1;
+               } else {
+                       igt_assert_eq(errno, EINVAL);
+               }
+       }
+       igt_assert_eq(num_handles, 2);
+}
+
+/**
+ * test_race_change_create_close - Test for race conditions of change_handle against gem create / close
+ *
+ * Start two threads, one of which attempts to change between four handles while the other alternates
+ * closing a gem handle and creating new ones. Test for race conditions such as change a handle that's
+ * being closed or change a handle to a value that's being created.
+ */
+static void test_race_change_create_close(struct gpu_ctx *ctx)
+{
+       pthread_t thread_change, thread_create_close;
+       struct change_handle_thread_data data = {0};
+       int num_handles, ret;
+
+       set_up_context(&data, ctx);
+
+       /* Start both threads simultaneously */
+       igt_assert_eq(pthread_create(&thread_change, NULL, change_handle_thread, &data), 0);
+       igt_assert_eq(pthread_create(&thread_create_close, NULL, close_recreate_thread, &data), 0);
+
+       /* Let them race */
+       sleep(RACE_DURATION_SEC);
+
+       __atomic_store_n(&data.running, false, __ATOMIC_RELEASE);
+       pthread_join(thread_change, NULL);
+       pthread_join(thread_create_close, NULL);
+
+       pthread_barrier_destroy(&data.barrier);
+
+       igt_assert_eq(data.unexpected_errno, 0);
+
+       /* Depending on where create / close ended up, there can be 1 or 2 handles open */
+       num_handles = 0;
+       for (int i = 0; i < 4; i++) {
+               ret = gem_close_bo(ctx, data.handles[i]);
+
+               if (ret == 0) {
+                       num_handles += 1;
+               } else {
+                       igt_assert_eq(errno, EINVAL);
+               }
+       }
+       igt_assert(num_handles >= 1 && num_handles <= 2);
+}
+
+/**
+ * test_race_change_export_import - Test for race conditions of change_handle against prime export / import
+ *
+ * Start two threads, one of which attempts to change between four handles while the other exports
+ * handles, then imports them again. Check for race conditions such as changing a handle while exporting
+ * it or important a handle while changing it.
+ */
+static void test_race_change_export_import(struct gpu_ctx *ctx)
+{
+       pthread_t thread_change, thread_export_import;
+       struct change_handle_thread_data data = {0};
+       int num_handles, ret;
+
+       set_up_context(&data, ctx);
+
+       /* Start both threads simultaneously */
+       igt_assert_eq(pthread_create(&thread_change, NULL, change_handle_thread, &data), 0);
+       igt_assert_eq(pthread_create(&thread_export_import, NULL, export_import_thread, &data), 0);
+
+       /* Let them race */
+       sleep(RACE_DURATION_SEC);
+
+       __atomic_store_n(&data.running, false, __ATOMIC_RELEASE);
+       pthread_join(thread_change, NULL);
+       pthread_join(thread_export_import, NULL);
+
+       pthread_barrier_destroy(&data.barrier);
+
+       igt_assert_eq(data.unexpected_errno, 0);
+
+       /* Exporting a handle doesn't close it, so should still be exactly 2 */
+       num_handles = 0;
+       for (int i = 0; i < 4; i++) {
+               ret = gem_close_bo(ctx, data.handles[i]);
+
+               if (ret == 0) {
+                       num_handles += 1;
+               } else {
+                       igt_assert_eq(errno, EINVAL);
+               }
+       }
+       igt_assert_eq(num_handles, 2);
+}
+
+int igt_main()
+{
+       struct gpu_ctx ctx = { .fd = -1 };
+
+       igt_fixture() {
+               ctx.fd = drm_open_driver_render(DRIVER_ANY);
+               igt_require(ctx.fd >= 0);
+
+               ctx.driver = detect_driver(ctx.fd);
+               igt_require_f(ctx.driver != DRIVER_TYPE_UNKNOWN,
+                             "Unsupported GPU driver\n");
+
+               if (ctx.driver == DRIVER_TYPE_I915)
+                       igt_require_gem(ctx.fd);
+       }
+
+       igt_describe("Sanity Test: Change handle actually changes handle");
+       igt_subtest("sanity")
+               test_sanity(&ctx);
+
+       igt_describe("Functinality Test: Change handle affects result of prime import");
+       igt_subtest("change_prime")
+               test_change_export(&ctx);
+
+
+       igt_describe("Edge Case: New handle = 0 should fail");
+       igt_subtest("new_handle_zero")
+               test_new_handle_zero(&ctx);
+
+       igt_describe("Edge Case: New handle > INT_MAX should fail");
+       igt_subtest("new_handle_exceeds_int_max")
+               test_new_handle_exceeds_int_max(&ctx);
+
+       igt_describe("Edge Case: Changing a handle to itself should fail");
+       igt_subtest("new_handle_equals_old_handle")
+               test_new_handle_equals_old_handle(&ctx);
+
+       igt_describe("Edge Case: Change a handle to a value which is taken should fail");
+       igt_subtest("new_handle_taken")
+               test_new_handle_taken(&ctx);
+
+       igt_describe("Edge Case: Changing a non-existent handle should fail");
+       igt_subtest("invalid_handle")
+               test_invalid_handle(&ctx);
+
+
+       igt_describe("Race: Changing handle vs. Changing handle");
+       igt_subtest("race_change_change")
+               test_race_change_change(&ctx);
+
+       igt_describe("Race: Changing handle vs. Create / Close");
+       igt_subtest("race_change_create_close")
+               test_race_change_create_close(&ctx);
+
+       igt_describe("Race: Changing handle vs. Export / Import");
+       igt_subtest("race_change_export_import")
+               test_race_change_export_import(&ctx);
+
+       igt_fixture() {
+               drm_close_driver(ctx.fd);
+       }
+
+}
\ No newline at end of file
diff --git a/tests/meson.build b/tests/meson.build
index 4f5c9be7b..ab083916d 100644
--- a/tests/meson.build
+++ b/tests/meson.build
@@ -104,6 +104,7 @@ intel_i915_progs = [
        'gem_blits',
        'gem_busy',
        'gem_caching',
+       'gem_change_handle',
        'gem_create',
        'gem_ccs',
        'gem_close',
--
2.34.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* ✗ Fi.CI.BUILD: failure for tests/intel: Add edge case and race condition tests for GEM_CHANGE_HANDLE (rev2)
  2026-10-07 14:12 [PATCH] tests/intel: Add edge case and race condition tests for GEM_CHANGE_HANDLE David Francis
  2026-10-07 14:13 ` Francis, David
@ 2026-10-07 15:49 ` Patchwork
  2026-10-07 18:17 ` ✗ Fi.CI.BUILD: failure for tests/intel: Add edge case and race condition tests for GEM_CHANGE_HANDLE (rev3) Patchwork
  2026-10-14  9:42 ` [PATCH] tests/intel: Add edge case and race condition tests for GEM_CHANGE_HANDLE Sebastian Brzezinka
  3 siblings, 0 replies; 7+ messages in thread
From: Patchwork @ 2026-10-07 15:49 UTC (permalink / raw)
  To: Francis, David; +Cc: igt-dev

== Series Details ==

Series: tests/intel: Add edge case and race condition tests for GEM_CHANGE_HANDLE (rev2)
URL   : https://patchwork.freedesktop.org/series/175696/
State : failure

== Summary ==

Applying: tests/intel: Add edge case and race condition tests for GEM_CHANGE_HANDLE
Using index info to reconstruct a base tree...
M	tests/meson.build
Patch failed at 0001 tests/intel: Add edge case and race condition tests for GEM_CHANGE_HANDLE
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] tests/intel: Add edge case and race condition tests for GEM_CHANGE_HANDLE
  2026-10-07 14:13 ` Francis, David
@ 2026-10-07 17:05   ` Francis, David
  0 siblings, 0 replies; 7+ messages in thread
From: Francis, David @ 2026-10-07 17:05 UTC (permalink / raw)
  To: igt-dev@lists.freedesktop.org
  Cc: tvrtko.ursulin@igalia.com, Prosyak, Vitaly, Kamil Konieczny,
	Sebastian Brzezinka, Koenig, Christian, Simona Vetter,
	Deucher, Alexander

Things that have changed since Vitaly's last version
- Most of Tvrtko's 31 comments have been taken into account, cleaning up the code overall
- The edge cases and race conditions have been combined into one patch, now gem_change_handle.c
- Removed the change-vs-driver-close tests (reasoning; in my testing these tests didn't work that well, and the race conditions raised during kernel discussion were vs. create and free)
- Removed a lot of info prints to match the style of the other gem tests (not sure what the standard is on these but it's easy to revise)
- The race tests now all use a setup with two active gem objects swapping around between four different handles; hopefully this can catch more complicated scenarios than were possible with only one object and two handles.
- The ioctl policy on new_handle = old_handle has changed in the latest kernel version (following a lot of feedback). It's now an error whether or not the handle is in use.



________________________________________
From: Francis, David <David.Francis@amd.com>
Sent: Wednesday, October 7, 2026 10:13 AM
To: igt-dev@lists.freedesktop.org
Cc: tvrtko.ursulin@igalia.com; Prosyak, Vitaly; Kamil Konieczny; Sebastian Brzezinka; Koenig, Christian; Simona Vetter; Deucher, Alexander
Subject: Re: [PATCH] tests/intel: Add edge case and race condition tests for GEM_CHANGE_HANDLE

(Vitaly specifically requested that his Signed-off-by be preserved here, as this is a modification of his original tests)

David Francis

________________________________________
From: Francis, David <David.Francis@amd.com>
Sent: Wednesday, October 7, 2026 10:12 AM
To: igt-dev@lists.freedesktop.org
Cc: tvrtko.ursulin@igalia.com; Prosyak, Vitaly; Kamil Konieczny; Sebastian Brzezinka; Koenig, Christian; Simona Vetter; Deucher, Alexander; Francis, David
Subject: [PATCH] tests/intel: Add edge case and race condition tests for GEM_CHANGE_HANDLE

From: Vitaly Prosyak <vitaly.prosyak@amd.com>

Add a GPU-agnostic GEM_CHANGE_HANDLE tests. These test for basic
functionality, edge cases, and race conditions of this ioctl against
itself, gem close, gem create, and prime import and export.

GEM_CHANGE_HANDLE is currently wired to drm_invalid_op in DRM core. This IGT
coverage is a prerequisite for validating the eventual kernel-side ioctl
implementation and makes fixing its security bugs safer.

Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
Cc: Sebastian Brzezinka <sebastian.brzezinka@intel.com>
Cc: Christian Konig <christian.koenig@amd.com>
Cc: Simona Vetter <simona@ffwll.ch>
Cc: Alex Deucher <alexander.deucher@amd.com>
Signed-off-by: Vitaly Prosyak <vitaly.prosyak@amd.com>
Signed-off-by: David Francis <David.Francis@amd.com>
---
 tests/intel/gem_change_handle.c | 747 ++++++++++++++++++++++++++++++++
 tests/meson.build               |   1 +
 2 files changed, 748 insertions(+)
 create mode 100644 tests/intel/gem_change_handle.c

diff --git a/tests/intel/gem_change_handle.c b/tests/intel/gem_change_handle.c
new file mode 100644
index 000000000..23f2f8b39
--- /dev/null
+++ b/tests/intel/gem_change_handle.c
@@ -0,0 +1,747 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright 2026 Advanced Micro Devices, Inc.
+ * Copyright 2026 Intel Corporation
+ *
+ * GPU-agnostic race condition tests for GEM_CHANGE_HANDLE ioctl
+ *
+ * NOTE: This test is located in tests/intel/ directory because:
+ * 1. All gem_* tests are traditionally placed in tests/intel/ regardless
+ *    of GPU vendor, following IGT convention for GEM-related tests
+ * 2. The test uses i915/gem.h and i915/gem_create.h for GEM object creation
+ *    on Intel platforms, though it works vendor-agnostically via DRM core
+ * 3. Some subtest designs (noop-same-handle, invalid-*, edge-*, functional-*)
+ *    were proposed by Simona Vetter (Intel), hence Intel copyright header
+ *
+ */
+
+#include <errno.h>
+#include <fcntl.h>
+#include <limits.h>
+#include <pthread.h>
+#include <sched.h>
+#include <semaphore.h>
+#include <signal.h>
+#include <string.h>
+#include <sys/stat.h>
+
+#include "igt.h"
+#include "igt_device.h"
+#include "i915/gem.h"
+#include "i915/gem_create.h"
+
+/* AMDGPU includes (if available) */
+#if __has_include("igt_amd.h")
+#include "lib/amdgpu/amd_memory.h"
+#include "igt_amd.h"
+#define HAS_AMDGPU 1
+#else
+#define HAS_AMDGPU 0
+#endif
+
+#define BO_SIZE (16 * 1024)
+#define RACE_DURATION_SEC 1
+
+static bool verbose_logging;
+
+/**
+ * TEST: gem change handle
+ * Description: tests for drm ioctl GEM_CHANGE_HANDLE
+ * Category: Core
+ * Mega feature: General Core features
+ * Sub-category: GEM handles
+ * Functionality: gem change handle
+ * Feature: gem
+ * Run type: FULL
+ *
+ * SUBTEST: sanity
+ * Description: Changing a handle to a new value works as intended
+ *
+ * SUBTEST: change_prime
+ * Description: Changing a handle affects the result of prime import
+ *
+ * SUBTEST: new_handle_zero
+ * Description: Changing a handle to 0 should fail
+ *
+ * SUBTEST: new_handle_exceeds_int_max
+ * Description: Changing a handle to > INT_MAX should fail
+ *
+ * SUBTEST: new_handle_equals_old_handle
+ * Description: Changing a handle to itself should fail
+ *
+ * SUBTEST: new_handle_taken
+ * Description: Changing a handle to a value already in use should fail
+ *
+ * SUBTEST: invalid_handle
+ * Description: Changing a non-existent handle should fail
+ *
+ * SUBTEST: race_change_change
+ * Description: Test for race conditions between two GEM_CHANGE_HANDLE
+ *
+ * SUBTEST: race_change_create_close
+ * Description: Test for race conditions between GEM_CHANGE_HANDLE, GEM_CREATE, and GEM_CLOSE
+ *
+ * SUBTEST: race_change_export_import
+ * Description: Test for race conditions between GEM_CHANGE_HANDLE, prime export, and prime import
+ */
+
+IGT_TEST_DESCRIPTION("Edge case and race condition tests for GEM_CHANGE_HANDLE");
+
+/* Structs */
+
+/* Define DRM_IOCTL_GEM_CHANGE_HANDLE if not available in kernel headers */
+#ifndef DRM_IOCTL_GEM_CHANGE_HANDLE
+struct drm_gem_change_handle {
+       __u32 handle;
+       __u32 new_handle;
+};
+
+#define DRM_IOCTL_GEM_CHANGE_HANDLE    DRM_IOWR(0xD2, struct drm_gem_change_handle)
+#endif
+
+/* Driver type detection */
+enum gpu_driver {
+       DRIVER_TYPE_UNKNOWN = 0,
+       DRIVER_TYPE_I915,
+       DRIVER_TYPE_AMDGPU,
+};
+
+/* GPU context structure */
+struct gpu_ctx {
+       int fd;
+       enum gpu_driver driver;
+};
+
+/* Helpers */
+
+static uint32_t gem_create_bo(struct gpu_ctx *ctx, uint64_t size)
+{
+       uint32_t handle = 0;
+
+       switch (ctx->driver) {
+       case DRIVER_TYPE_I915: {
+               struct drm_i915_gem_create create = { .size = size };
+
+               do_ioctl(ctx->fd, DRM_IOCTL_I915_GEM_CREATE, &create);
+               handle = create.handle;
+               break;
+       }
+       case DRIVER_TYPE_AMDGPU:
+#if HAS_AMDGPU
+               handle = igt_amd_create_bo(ctx->fd, size);
+#else
+               igt_require_f(0, "AMDGPU header not found for AMDGPU driver\n");
+#endif
+               break;
+       default:
+               igt_assert_f(0, "Unsupported driver\n");
+       }
+
+       igt_assert(handle != 0);
+       return handle;
+}
+
+static int gem_close_bo(struct gpu_ctx *ctx, uint32_t handle)
+{
+       struct drm_gem_close close_args = { .handle = handle };
+
+       return igt_ioctl(ctx->fd, DRM_IOCTL_GEM_CLOSE, &close_args);
+}
+
+static int gem_change_handle(struct gpu_ctx *ctx, uint32_t old_handle, uint32_t new_handle)
+{
+       struct drm_gem_change_handle args = {
+               .handle = old_handle,
+               .new_handle = new_handle,
+       };
+       return igt_ioctl(ctx->fd, DRM_IOCTL_GEM_CHANGE_HANDLE, &args);
+}
+
+static enum gpu_driver detect_driver(int fd)
+{
+       /* Check AMDGPU first (primary target) */
+       if (is_amdgpu_device(fd))
+               return DRIVER_TYPE_AMDGPU;
+       if (is_i915_device(fd))
+               return DRIVER_TYPE_I915;
+       return DRIVER_TYPE_UNKNOWN;
+}
+
+/* Sanity and Functionality tests */
+
+/**
+ * test_sanity - verify rename actually works
+ *
+ * After a successful rename:
+ * - GEM_CLOSE(old_handle) must FAIL (handle no longer exists)
+ * - GEM_CLOSE(new_handle) must SUCCEED (object is there)
+ */
+static void test_sanity(struct gpu_ctx *ctx)
+{
+       uint32_t handle, new_h;
+       int ret;
+
+       handle = gem_create_bo(ctx, 4096);
+       new_h = handle + 1000;
+
+       /* Perform the rename */
+       ret = gem_change_handle(ctx, handle, new_h);
+       igt_assert_f(ret == 0,
+                    "gem_change_handle(%u -> %u) failed: %s\n",
+                    handle, new_h, strerror(errno));
+
+       /* Old handle must be invalid now */
+       ret = gem_close_bo(ctx, handle);
+       igt_assert_f(ret < 0 && errno == EINVAL,
+                    "GEM_CLOSE(old_handle=%u) should fail with EINVAL, "
+                    "got ret=%d errno=%d (%s)\n",
+                    handle, ret, errno, strerror(errno));
+
+       /* New handle must be valid */
+       ret = gem_close_bo(ctx, new_h);
+       igt_assert_f(ret == 0,
+                    "GEM_CLOSE(new_handle=%u) should succeed, "
+                    "got ret=%d errno=%d (%s)\n",
+                    new_h, ret, errno, strerror(errno));
+}
+
+/**
+ * test_change_export - changing a handle affects prime import
+ *
+ * Importing a prime object that has had its handle changed since it was
+ * exported should get the new handle
+ */
+static void test_change_export(struct gpu_ctx *ctx)
+{
+       int ret, handle1, handle2, dmabuf_fd, imported_handle;
+
+       /* Create a handle and export it */
+       handle1 = gem_create_bo(ctx, 4096);
+       handle2 = handle1 + 1;
+       dmabuf_fd = prime_handle_to_fd(ctx->fd, handle1);
+
+       /* When you import, you get the same handle back */
+       imported_handle = prime_fd_to_handle(ctx->fd, dmabuf_fd);
+       igt_assert_eq(imported_handle, handle1);
+
+       /* Change the handle (should succeed) */
+       ret = gem_change_handle(ctx, handle1, handle2);
+       igt_assert_eq(ret, 0);
+
+       /* Import, should get the new handle */
+       imported_handle = prime_fd_to_handle(ctx->fd, dmabuf_fd);
+       igt_assert_eq(imported_handle, handle2);
+
+       /* Confirm gem object is at the new handle */
+       igt_assert_eq(gem_close_bo(ctx, handle2), 0);
+
+       ret = gem_close_bo(ctx, handle1);
+       igt_assert(ret < 0);
+       igt_assert_eq(errno, EINVAL);
+
+       close(dmabuf_fd);
+}
+
+/* Edge case and fail case tests */
+
+/**
+ * test_new_handle_zero - new_handle = 0 should fail
+ *
+ * 0 is not a valid gem handle.
+ */
+static void test_new_handle_zero(struct gpu_ctx *ctx)
+{
+       uint32_t handle;
+       int ret;
+
+       handle = gem_create_bo(ctx, 4096);
+
+       /* new_handle = 0 must fail with -EINVAL */
+       ret = gem_change_handle(ctx, handle, 0);
+       igt_assert(ret < 0);
+       igt_assert_eq(errno, EINVAL);
+
+       /* Original handle should still be valid */
+       igt_assert_eq(gem_close_bo(ctx, handle), 0);
+}
+
+/**
+ * test_new_handle_exceeds_int_max - new_handle > INT_MAX should fail
+ *
+ * The kernel enforces an idr_alloc() limitation: new_handle must be <= INT_MAX.
+ */
+static void test_new_handle_exceeds_int_max(struct gpu_ctx *ctx)
+{
+       uint32_t handle;
+       int ret;
+
+       handle = gem_create_bo(ctx, 4096);
+
+       /* new_handle > INT_MAX must fail with -EINVAL */
+       ret = gem_change_handle(ctx, handle, (uint32_t)INT_MAX + 1);
+       igt_assert(ret < 0);
+       igt_assert_eq(errno, EINVAL);
+
+       /* Also test 0xFFFFFFFF */
+       ret = gem_change_handle(ctx, handle, 0xFFFFFFFF);
+       igt_assert(ret < 0);
+       igt_assert_eq(errno, EINVAL);
+
+       /* Original handle should still be valid */
+       igt_assert_eq(gem_close_bo(ctx, handle), 0);
+}
+
+/**
+ * test_new_handle_equals_old_handle - new_handle = old_handle should fail
+ *
+ * The ioctl treats old_handle = new_handle as invalid input.
+ */
+static void test_new_handle_equals_old_handle(struct gpu_ctx *ctx)
+{
+       uint32_t handle;
+       int ret;
+
+       handle = gem_create_bo(ctx, 4096);
+
+       /* new_handle = old_handle must fail with -EINVAL */
+       ret = gem_change_handle(ctx, handle, handle);
+       igt_assert(ret < 0);
+       igt_assert_eq(errno, EINVAL);
+
+       /* same thing if handle doesn't exist */
+       ret = gem_change_handle(ctx, handle + 1, handle + 1);
+       igt_assert(ret < 0);
+       igt_assert_eq(errno, EINVAL);
+
+       /* Original handle should still be valid */
+       igt_assert_eq(gem_close_bo(ctx, handle), 0);
+}
+
+/**
+ * test_new_handle_taken - new_handle being the handle of another gem object must fail
+ *
+ * Not allowed to change a handle to a handle that's already taken
+ */
+static void test_new_handle_taken(struct gpu_ctx *ctx)
+{
+       uint32_t handle, handle_2;
+       int ret;
+
+       handle = gem_create_bo(ctx, 4096);
+       handle_2 = gem_create_bo(ctx, 4096);
+
+       /* new_handle is taken must fail with -ENOSPC (direct from idr_alloc) */
+       ret = gem_change_handle(ctx, handle, handle_2);
+       igt_assert(ret < 0);
+       igt_assert_eq(errno, ENOSPC);
+
+       /* And the other way */
+       ret = gem_change_handle(ctx, handle_2, handle);
+       igt_assert(ret < 0);
+       igt_assert_eq(errno, ENOSPC);
+
+       /* Original handles should still be valid */
+       igt_assert_eq(gem_close_bo(ctx, handle), 0);
+       igt_assert_eq(gem_close_bo(ctx, handle_2), 0);
+}
+
+/**
+ * test_invalid_handle - non-existent handle should fail
+ *
+ * Using a handle that doesn't exist should return -ENOENT.
+ */
+static void test_invalid_handle(struct gpu_ctx *ctx)
+{
+       int ret;
+
+       /* Use a handle that was never allocated */
+       ret = gem_change_handle(ctx, 0xDEAD, 0xBEEF);
+       igt_assert(ret < 0);
+       igt_assert_eq(errno, ENOENT);
+
+       /* Handle 0 is also invalid */
+       ret = gem_change_handle(ctx, 0, 100);
+       igt_assert(ret < 0);
+       igt_assert_eq(errno, ENOENT);
+}
+
+/* Race condition test structs and helpers */
+
+struct change_handle_thread_data {
+       struct gpu_ctx *ctx;
+       uint32_t handles[4];
+       bool running;
+       int unexpected_errno;
+       pthread_barrier_t barrier;
+};
+
+static void set_up_context(struct change_handle_thread_data *data, struct gpu_ctx *ctx)
+{
+       data->handles[0] = gem_create_bo(ctx, BO_SIZE);
+       igt_assert(data->handles[0] != 0);
+       data->handles[1] = gem_create_bo(ctx, BO_SIZE);
+       igt_assert(data->handles[1] != 0);
+
+       data->ctx = ctx;
+       data->handles[2] = max(data->handles[0], data->handles[1]) + 0x10;
+       data->handles[3] = max(data->handles[0], data->handles[1]) + 0x20;
+       data->unexpected_errno = 0;
+       __atomic_store_n(&data->running, true, __ATOMIC_RELEASE);
+
+       pthread_barrier_init(&data->barrier, NULL, 2);
+}
+
+/* Race condition test threads */
+
+static void *change_handle_thread(void *arg)
+{
+       struct change_handle_thread_data *data = arg;
+       uint32_t *h = data->handles;
+       int from[] = {h[0], h[2], h[0], h[3], h[3], h[1], h[1], h[3], h[2], h[1], h[0], h[2]};
+       int to[] =   {h[1], h[3], h[2], h[1], h[0], h[2], h[0], h[2], h[0], h[3], h[3], h[1]};
+       int ret;
+
+       pthread_barrier_wait(&data->barrier);
+
+       while (__atomic_load_n(&data->running, __ATOMIC_ACQUIRE)) {
+               for (int i = 0; i < 12; i++) {
+                       ret = gem_change_handle(data->ctx, from[i], to[i]);
+                       /*
+                        * Expected errors:
+                        * - ENOENT: old handle invalid
+                        * - ENOSPC: target handle occupied (collision detected during idr_alloc)
+                        */
+                       if (ret != 0 && errno != ENOENT && errno != ENOSPC)
+                               data->unexpected_errno = errno;
+               }
+
+               sched_yield();
+       }
+
+       return NULL;
+}
+
+static void *close_recreate_thread(void *arg)
+{
+       struct change_handle_thread_data *data = arg;
+       uint32_t *h = data->handles;
+       pthread_t tid = pthread_self();
+       int handle, i, ret;
+
+       pthread_barrier_wait(&data->barrier);
+
+       while (__atomic_load_n(&data->running, __ATOMIC_ACQUIRE)) {
+               /* Close a handle; keep trying until success */
+               i = 0;
+               while (true) {
+                       ret = gem_close_bo(data->ctx, h[i]);
+                       if (ret == 0) {
+                               if (verbose_logging) {
+                                       igt_info("[CLOSE-TID:%lu] Closed handle %u\n",
+                                                (unsigned long)tid, h[i]);
+                               }
+                               break;
+                       }
+
+                       i += 1;
+                       if (i > 3)
+                               i = 0;
+
+                       if (!__atomic_load_n(&data->running, __ATOMIC_ACQUIRE))
+                               return NULL;
+               }
+
+               /* Open a handle */
+               handle = gem_create_bo(data->ctx, BO_SIZE);
+               if (verbose_logging) {
+                       igt_info("[CLOSE-TID:%lu] Created new BO with handle %u\n",
+                                (unsigned long)tid, handle);
+               }
+
+               /* If the opened handle isn't one of the four ones being
+                * shuffled, change_handle it in. Keep trying until success.
+                */
+               if (handle != h[0] && handle != h[1] && handle != h[2] && handle != h[3]) {
+                       i = 0;
+                       while (true) {
+                               ret = gem_change_handle(data->ctx, handle, h[i]);
+                               if (ret == 0) {
+                                       if (verbose_logging) {
+                                               igt_info("[CLOSE-TID:%lu] Change handle %u to %u\n",
+                                                        (unsigned long)tid, handle, h[i]);
+                                       }
+                                       break;
+                               }
+
+                               if (ret != 0 && errno != ENOENT && errno != ENOSPC)
+                                       data->unexpected_errno = errno;
+
+                               i += 1;
+                               if (i > 3)
+                                       i = 0;
+
+                               if (!__atomic_load_n(&data->running, __ATOMIC_ACQUIRE))
+                                       return NULL;
+                       }
+               }
+
+               sched_yield();
+       }
+
+       return NULL;
+}
+
+static void *export_import_thread(void *arg)
+{
+       struct change_handle_thread_data *data = arg;
+       struct drm_prime_handle prime_args;
+       uint32_t *h = data->handles;
+       pthread_t tid = pthread_self();
+       int handle, i, ret, dmabuf_fd;
+
+       pthread_barrier_wait(&data->barrier);
+
+       while (__atomic_load_n(&data->running, __ATOMIC_ACQUIRE)) {
+               /* Export a handle; keep trying until success */
+               i = 0;
+               while (true) {
+                       prime_args.handle = h[i];
+                       prime_args.flags = DRM_CLOEXEC | DRM_RDWR;
+                       ret = igt_ioctl(data->ctx->fd, DRM_IOCTL_PRIME_HANDLE_TO_FD, &prime_args);
+                       if (ret == 0) {
+                               if (verbose_logging) {
+                                       igt_info("[PRIME-TID:%lu] Exported handle %u\n",
+                                                (unsigned long)tid, h[i]);
+                               }
+                               break;
+                       }
+
+                       i += 1;
+                       if (i > 3)
+                               i = 0;
+
+                       if (!__atomic_load_n(&data->running, __ATOMIC_ACQUIRE))
+                               return NULL;
+               }
+
+               dmabuf_fd = prime_args.fd;
+
+               /* Import the handle; it should be one of the four handles in the shuffle */
+               prime_args.fd = dmabuf_fd;
+               prime_args.flags = 0;
+               ret = igt_ioctl(data->ctx->fd, DRM_IOCTL_PRIME_FD_TO_HANDLE, &prime_args);
+
+               igt_assert_eq(ret, 0);
+               handle = prime_args.handle;
+               if (verbose_logging) {
+                       igt_info("[PRIME-TID:%lu] Imported; got handle %u\n",
+                                (unsigned long)tid, handle);
+               }
+               igt_assert(handle == h[0] || handle == h[1] || handle == h[2] || handle == h[3]);
+
+               close(dmabuf_fd);
+
+               sched_yield();
+       }
+
+       return NULL;
+}
+
+/* Race condition tests */
+
+/**
+ * test_race_change_change - Test for race conditions of change_handle against itself
+ *
+ * Start two threads, each of which constantly attempt changes between four different handles
+ * Try to catch race conditions such as changing same handle to two different handles,
+ * Changing two handles to the same handle, changing a handle to a handle itself being changed,
+ * etc.
+ */
+static void test_race_change_change(struct gpu_ctx *ctx)
+{
+       pthread_t thread_change_1, thread_change_2;
+       struct change_handle_thread_data data = {0};
+       int num_handles, ret;
+
+       set_up_context(&data, ctx);
+
+       /* Start both threads simultaneously */
+       igt_assert_eq(pthread_create(&thread_change_1, NULL, change_handle_thread, &data), 0);
+       igt_assert_eq(pthread_create(&thread_change_2, NULL, change_handle_thread, &data), 0);
+
+       /* Let them race */
+       sleep(RACE_DURATION_SEC);
+
+       __atomic_store_n(&data.running, false, __ATOMIC_RELEASE);
+       pthread_join(thread_change_1, NULL);
+       pthread_join(thread_change_2, NULL);
+
+       pthread_barrier_destroy(&data.barrier);
+
+       igt_assert_eq(data.unexpected_errno, 0);
+
+       /* Exactly two gem objects should still exist */
+       num_handles = 0;
+       for (int i = 0; i < 4; i++) {
+               ret = gem_close_bo(ctx, data.handles[i]);
+
+               if (ret == 0) {
+                       num_handles += 1;
+               } else {
+                       igt_assert_eq(errno, EINVAL);
+               }
+       }
+       igt_assert_eq(num_handles, 2);
+}
+
+/**
+ * test_race_change_create_close - Test for race conditions of change_handle against gem create / close
+ *
+ * Start two threads, one of which attempts to change between four handles while the other alternates
+ * closing a gem handle and creating new ones. Test for race conditions such as change a handle that's
+ * being closed or change a handle to a value that's being created.
+ */
+static void test_race_change_create_close(struct gpu_ctx *ctx)
+{
+       pthread_t thread_change, thread_create_close;
+       struct change_handle_thread_data data = {0};
+       int num_handles, ret;
+
+       set_up_context(&data, ctx);
+
+       /* Start both threads simultaneously */
+       igt_assert_eq(pthread_create(&thread_change, NULL, change_handle_thread, &data), 0);
+       igt_assert_eq(pthread_create(&thread_create_close, NULL, close_recreate_thread, &data), 0);
+
+       /* Let them race */
+       sleep(RACE_DURATION_SEC);
+
+       __atomic_store_n(&data.running, false, __ATOMIC_RELEASE);
+       pthread_join(thread_change, NULL);
+       pthread_join(thread_create_close, NULL);
+
+       pthread_barrier_destroy(&data.barrier);
+
+       igt_assert_eq(data.unexpected_errno, 0);
+
+       /* Depending on where create / close ended up, there can be 1 or 2 handles open */
+       num_handles = 0;
+       for (int i = 0; i < 4; i++) {
+               ret = gem_close_bo(ctx, data.handles[i]);
+
+               if (ret == 0) {
+                       num_handles += 1;
+               } else {
+                       igt_assert_eq(errno, EINVAL);
+               }
+       }
+       igt_assert(num_handles >= 1 && num_handles <= 2);
+}
+
+/**
+ * test_race_change_export_import - Test for race conditions of change_handle against prime export / import
+ *
+ * Start two threads, one of which attempts to change between four handles while the other exports
+ * handles, then imports them again. Check for race conditions such as changing a handle while exporting
+ * it or important a handle while changing it.
+ */
+static void test_race_change_export_import(struct gpu_ctx *ctx)
+{
+       pthread_t thread_change, thread_export_import;
+       struct change_handle_thread_data data = {0};
+       int num_handles, ret;
+
+       set_up_context(&data, ctx);
+
+       /* Start both threads simultaneously */
+       igt_assert_eq(pthread_create(&thread_change, NULL, change_handle_thread, &data), 0);
+       igt_assert_eq(pthread_create(&thread_export_import, NULL, export_import_thread, &data), 0);
+
+       /* Let them race */
+       sleep(RACE_DURATION_SEC);
+
+       __atomic_store_n(&data.running, false, __ATOMIC_RELEASE);
+       pthread_join(thread_change, NULL);
+       pthread_join(thread_export_import, NULL);
+
+       pthread_barrier_destroy(&data.barrier);
+
+       igt_assert_eq(data.unexpected_errno, 0);
+
+       /* Exporting a handle doesn't close it, so should still be exactly 2 */
+       num_handles = 0;
+       for (int i = 0; i < 4; i++) {
+               ret = gem_close_bo(ctx, data.handles[i]);
+
+               if (ret == 0) {
+                       num_handles += 1;
+               } else {
+                       igt_assert_eq(errno, EINVAL);
+               }
+       }
+       igt_assert_eq(num_handles, 2);
+}
+
+int igt_main()
+{
+       struct gpu_ctx ctx = { .fd = -1 };
+
+       igt_fixture() {
+               ctx.fd = drm_open_driver_render(DRIVER_ANY);
+               igt_require(ctx.fd >= 0);
+
+               ctx.driver = detect_driver(ctx.fd);
+               igt_require_f(ctx.driver != DRIVER_TYPE_UNKNOWN,
+                             "Unsupported GPU driver\n");
+
+               if (ctx.driver == DRIVER_TYPE_I915)
+                       igt_require_gem(ctx.fd);
+       }
+
+       igt_describe("Sanity Test: Change handle actually changes handle");
+       igt_subtest("sanity")
+               test_sanity(&ctx);
+
+       igt_describe("Functinality Test: Change handle affects result of prime import");
+       igt_subtest("change_prime")
+               test_change_export(&ctx);
+
+
+       igt_describe("Edge Case: New handle = 0 should fail");
+       igt_subtest("new_handle_zero")
+               test_new_handle_zero(&ctx);
+
+       igt_describe("Edge Case: New handle > INT_MAX should fail");
+       igt_subtest("new_handle_exceeds_int_max")
+               test_new_handle_exceeds_int_max(&ctx);
+
+       igt_describe("Edge Case: Changing a handle to itself should fail");
+       igt_subtest("new_handle_equals_old_handle")
+               test_new_handle_equals_old_handle(&ctx);
+
+       igt_describe("Edge Case: Change a handle to a value which is taken should fail");
+       igt_subtest("new_handle_taken")
+               test_new_handle_taken(&ctx);
+
+       igt_describe("Edge Case: Changing a non-existent handle should fail");
+       igt_subtest("invalid_handle")
+               test_invalid_handle(&ctx);
+
+
+       igt_describe("Race: Changing handle vs. Changing handle");
+       igt_subtest("race_change_change")
+               test_race_change_change(&ctx);
+
+       igt_describe("Race: Changing handle vs. Create / Close");
+       igt_subtest("race_change_create_close")
+               test_race_change_create_close(&ctx);
+
+       igt_describe("Race: Changing handle vs. Export / Import");
+       igt_subtest("race_change_export_import")
+               test_race_change_export_import(&ctx);
+
+       igt_fixture() {
+               drm_close_driver(ctx.fd);
+       }
+
+}
\ No newline at end of file
diff --git a/tests/meson.build b/tests/meson.build
index 4f5c9be7b..ab083916d 100644
--- a/tests/meson.build
+++ b/tests/meson.build
@@ -104,6 +104,7 @@ intel_i915_progs = [
        'gem_blits',
        'gem_busy',
        'gem_caching',
+       'gem_change_handle',
        'gem_create',
        'gem_ccs',
        'gem_close',
--
2.34.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* ✗ Fi.CI.BUILD: failure for tests/intel: Add edge case and race condition tests for GEM_CHANGE_HANDLE (rev3)
  2026-10-07 14:12 [PATCH] tests/intel: Add edge case and race condition tests for GEM_CHANGE_HANDLE David Francis
  2026-10-07 14:13 ` Francis, David
  2026-10-07 15:49 ` ✗ Fi.CI.BUILD: failure for tests/intel: Add edge case and race condition tests for GEM_CHANGE_HANDLE (rev2) Patchwork
@ 2026-10-07 18:17 ` Patchwork
  2026-10-14  9:42 ` [PATCH] tests/intel: Add edge case and race condition tests for GEM_CHANGE_HANDLE Sebastian Brzezinka
  3 siblings, 0 replies; 7+ messages in thread
From: Patchwork @ 2026-10-07 18:17 UTC (permalink / raw)
  To: Francis, David; +Cc: igt-dev

== Series Details ==

Series: tests/intel: Add edge case and race condition tests for GEM_CHANGE_HANDLE (rev3)
URL   : https://patchwork.freedesktop.org/series/175696/
State : failure

== Summary ==

Applying: tests/intel: Add edge case and race condition tests for GEM_CHANGE_HANDLE
Using index info to reconstruct a base tree...
M	tests/meson.build
Patch failed at 0001 tests/intel: Add edge case and race condition tests for GEM_CHANGE_HANDLE
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] tests/intel: Add edge case and race condition tests for GEM_CHANGE_HANDLE
  2026-10-07 14:12 [PATCH] tests/intel: Add edge case and race condition tests for GEM_CHANGE_HANDLE David Francis
                   ` (2 preceding siblings ...)
  2026-10-07 18:17 ` ✗ Fi.CI.BUILD: failure for tests/intel: Add edge case and race condition tests for GEM_CHANGE_HANDLE (rev3) Patchwork
@ 2026-10-14  9:42 ` Sebastian Brzezinka
  2026-10-14 13:25   ` Francis, David
  3 siblings, 1 reply; 7+ messages in thread
From: Sebastian Brzezinka @ 2026-10-14  9:42 UTC (permalink / raw)
  To: David Francis, igt-dev
  Cc: tvrtko.ursulin, Vitaly Prosyak, Kamil Konieczny,
	Sebastian Brzezinka, Christian Konig, Simona Vetter, Alex Deucher

Hi David,

On Wed Oct 7, 2026 at 4:12 PM CEST, David Francis wrote:
> From: Vitaly Prosyak <vitaly.prosyak@amd.com>
>
> Add a GPU-agnostic GEM_CHANGE_HANDLE tests. These test for basic
> functionality, edge cases, and race conditions of this ioctl against
> itself, gem close, gem create, and prime import and export.
>
> GEM_CHANGE_HANDLE is currently wired to drm_invalid_op in DRM core. This IGT
> coverage is a prerequisite for validating the eventual kernel-side ioctl
> implementation and makes fixing its security bugs safer.
>
> Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
> Cc: Sebastian Brzezinka <sebastian.brzezinka@intel.com>
> Cc: Christian Konig <christian.koenig@amd.com>
> Cc: Simona Vetter <simona@ffwll.ch>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Signed-off-by: Vitaly Prosyak <vitaly.prosyak@amd.com>
> Signed-off-by: David Francis <David.Francis@amd.com>
> ---
>  tests/intel/gem_change_handle.c | 747 ++++++++++++++++++++++++++++++++
>  tests/meson.build               |   1 +
>  2 files changed, 748 insertions(+)
>  create mode 100644 tests/intel/gem_change_handle.c
>
> diff --git a/tests/intel/gem_change_handle.c b/tests/intel/gem_change_handle.c
> new file mode 100644
> index 000000000..23f2f8b39
> --- /dev/null
> +++ b/tests/intel/gem_change_handle.c
> @@ -0,0 +1,747 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright 2026 Advanced Micro Devices, Inc.
> + * Copyright 2026 Intel Corporation
> + *
> + * GPU-agnostic race condition tests for GEM_CHANGE_HANDLE ioctl
> + *
> + * NOTE: This test is located in tests/intel/ directory because:
> + * 1. All gem_* tests are traditionally placed in tests/intel/ regardless
> + *    of GPU vendor, following IGT convention for GEM-related tests
> + * 2. The test uses i915/gem.h and i915/gem_create.h for GEM object creation
> + *    on Intel platforms, though it works vendor-agnostically via DRM core
> + * 3. Some subtest designs (noop-same-handle, invalid-*, edge-*, functional-*)
> + *    were proposed by Simona Vetter (Intel), hence Intel copyright header
> + *
> + */
> +
> +#include <errno.h>
> +#include <fcntl.h>
a
It looks like fcntl is not used anymore.

> +#include <limits.h>
> +#include <pthread.h>
> +#include <sched.h>
> +#include <semaphore.h>
same here

> +#include <signal.h>
and here
> +#include <string.h>
> +#include <sys/stat.h>
and here, please double check it, I may have missed some usage.

> +
> +#include "igt.h"
> +#include "igt_device.h"
> +#include "i915/gem.h"
> +#include "i915/gem_create.h"
> +
> +/* AMDGPU includes (if available) */
> +#if __has_include("igt_amd.h")
> +#include "lib/amdgpu/amd_memory.h"
> +#include "igt_amd.h"
> +#define HAS_AMDGPU 1
> +#else
> +#define HAS_AMDGPU 0
> +#endif
> +
> +#define BO_SIZE (16 * 1024)
> +#define RACE_DURATION_SEC 1
> +
> +static bool verbose_logging;
> +
> +/**
> + * TEST: gem change handle
> + * Description: tests for drm ioctl GEM_CHANGE_HANDLE
> + * Category: Core
> + * Mega feature: General Core features
> + * Sub-category: GEM handles
> + * Functionality: gem change handle
> + * Feature: gem
> + * Run type: FULL
> + *
> + * SUBTEST: sanity
> + * Description: Changing a handle to a new value works as intended
> + *
> + * SUBTEST: change_prime
> + * Description: Changing a handle affects the result of prime import
> + *
> + * SUBTEST: new_handle_zero
> + * Description: Changing a handle to 0 should fail
> + *
> + * SUBTEST: new_handle_exceeds_int_max
> + * Description: Changing a handle to > INT_MAX should fail
> + *
> + * SUBTEST: new_handle_equals_old_handle
> + * Description: Changing a handle to itself should fail
> + *
> + * SUBTEST: new_handle_taken
> + * Description: Changing a handle to a value already in use should fail
> + *
> + * SUBTEST: invalid_handle
> + * Description: Changing a non-existent handle should fail
> + *
> + * SUBTEST: race_change_change
> + * Description: Test for race conditions between two GEM_CHANGE_HANDLE
> + *
> + * SUBTEST: race_change_create_close
> + * Description: Test for race conditions between GEM_CHANGE_HANDLE, GEM_CREATE, and GEM_CLOSE
> + *
> + * SUBTEST: race_change_export_import
> + * Description: Test for race conditions between GEM_CHANGE_HANDLE, prime export, and prime import
> + */
> +
> +IGT_TEST_DESCRIPTION("Edge case and race condition tests for GEM_CHANGE_HANDLE");
> +
> +/* Structs */
> +
> +/* Define DRM_IOCTL_GEM_CHANGE_HANDLE if not available in kernel headers */
> +#ifndef DRM_IOCTL_GEM_CHANGE_HANDLE
> +struct drm_gem_change_handle {
> +	__u32 handle;
> +	__u32 new_handle;
> +};
> +
> +#define DRM_IOCTL_GEM_CHANGE_HANDLE    DRM_IOWR(0xD2, struct drm_gem_change_handle)
> +#endif
> +
> +/* Driver type detection */
> +enum gpu_driver {
> +	DRIVER_TYPE_UNKNOWN = 0,
> +	DRIVER_TYPE_I915,
> +	DRIVER_TYPE_AMDGPU,
> +};
> +
> +/* GPU context structure */
> +struct gpu_ctx {
> +	int fd;
> +	enum gpu_driver driver;
> +};
> +
> +/* Helpers */
> +
> +static uint32_t gem_create_bo(struct gpu_ctx *ctx, uint64_t size)
> +{
> +	uint32_t handle = 0;
> +
> +	switch (ctx->driver) {
> +	case DRIVER_TYPE_I915: {
> +		struct drm_i915_gem_create create = { .size = size };
> +
> +		do_ioctl(ctx->fd, DRM_IOCTL_I915_GEM_CREATE, &create);
> +		handle = create.handle;
> +		break;
> +	}
> +	case DRIVER_TYPE_AMDGPU:
> +#if HAS_AMDGPU
> +		handle = igt_amd_create_bo(ctx->fd, size);
> +#else
> +		igt_require_f(0, "AMDGPU header not found for AMDGPU driver\n");
> +#endif
> +		break;
> +	default:
> +		igt_assert_f(0, "Unsupported driver\n");
> +	}
> +
> +	igt_assert(handle != 0);
> +	return handle;
> +}
> +
> +static int gem_close_bo(struct gpu_ctx *ctx, uint32_t handle)
> +{
> +	struct drm_gem_close close_args = { .handle = handle };
> +
> +	return igt_ioctl(ctx->fd, DRM_IOCTL_GEM_CLOSE, &close_args);
> +}
> +
> +static int gem_change_handle(struct gpu_ctx *ctx, uint32_t old_handle, uint32_t new_handle)
> +{
> +	struct drm_gem_change_handle args = {
> +		.handle = old_handle,
> +		.new_handle = new_handle,
> +	};
> +	return igt_ioctl(ctx->fd, DRM_IOCTL_GEM_CHANGE_HANDLE, &args);
> +}
> +
> +static enum gpu_driver detect_driver(int fd)
> +{
> +	/* Check AMDGPU first (primary target) */
> +	if (is_amdgpu_device(fd))
> +		return DRIVER_TYPE_AMDGPU;
> +	if (is_i915_device(fd))
> +		return DRIVER_TYPE_I915;
> +	return DRIVER_TYPE_UNKNOWN;
> +}
> +
> +/* Sanity and Functionality tests */
> +
> +/**
> + * test_sanity - verify rename actually works
> + *
> + * After a successful rename:
> + * - GEM_CLOSE(old_handle) must FAIL (handle no longer exists)
> + * - GEM_CLOSE(new_handle) must SUCCEED (object is there)
> + */
> +static void test_sanity(struct gpu_ctx *ctx)
> +{
> +	uint32_t handle, new_h;
> +	int ret;
> +
> +	handle = gem_create_bo(ctx, 4096);
> +	new_h = handle + 1000;
> +
> +	/* Perform the rename */
> +	ret = gem_change_handle(ctx, handle, new_h);
> +	igt_assert_f(ret == 0,
> +		     "gem_change_handle(%u -> %u) failed: %s\n",
> +		     handle, new_h, strerror(errno));
> +
> +	/* Old handle must be invalid now */
> +	ret = gem_close_bo(ctx, handle);
> +	igt_assert_f(ret < 0 && errno == EINVAL,
> +		     "GEM_CLOSE(old_handle=%u) should fail with EINVAL, "
> +		     "got ret=%d errno=%d (%s)\n",
> +		     handle, ret, errno, strerror(errno));
> +
> +	/* New handle must be valid */
> +	ret = gem_close_bo(ctx, new_h);
> +	igt_assert_f(ret == 0,
> +		     "GEM_CLOSE(new_handle=%u) should succeed, "
> +		     "got ret=%d errno=%d (%s)\n",
> +		     new_h, ret, errno, strerror(errno));
> +}
> +
> +/**
> + * test_change_export - changing a handle affects prime import
> + *
> + * Importing a prime object that has had its handle changed since it was
> + * exported should get the new handle
> + */
> +static void test_change_export(struct gpu_ctx *ctx)
> +{
> +	int ret, handle1, handle2, dmabuf_fd, imported_handle;
> +
> +	/* Create a handle and export it */
> +	handle1 = gem_create_bo(ctx, 4096);
> +	handle2 = handle1 + 1;
> +	dmabuf_fd = prime_handle_to_fd(ctx->fd, handle1);
> +
> +	/* When you import, you get the same handle back */
> +	imported_handle = prime_fd_to_handle(ctx->fd, dmabuf_fd);
> +	igt_assert_eq(imported_handle, handle1);
> +
> +	/* Change the handle (should succeed) */
> +	ret = gem_change_handle(ctx, handle1, handle2);
> +	igt_assert_eq(ret, 0);
> +
> +	/* Import, should get the new handle */
> +	imported_handle = prime_fd_to_handle(ctx->fd, dmabuf_fd);
> +	igt_assert_eq(imported_handle, handle2);
> +
> +	/* Confirm gem object is at the new handle */
> +	igt_assert_eq(gem_close_bo(ctx, handle2), 0);
> +
> +	ret = gem_close_bo(ctx, handle1);
> +	igt_assert(ret < 0);
> +	igt_assert_eq(errno, EINVAL);
> +
> +	close(dmabuf_fd);
> +}
> +
> +/* Edge case and fail case tests */
> +
> +/**
> + * test_new_handle_zero - new_handle = 0 should fail
> + *
> + * 0 is not a valid gem handle.
> + */
> +static void test_new_handle_zero(struct gpu_ctx *ctx)
> +{
> +	uint32_t handle;
> +	int ret;
> +
> +	handle = gem_create_bo(ctx, 4096);
> +
> +	/* new_handle = 0 must fail with -EINVAL */
> +	ret = gem_change_handle(ctx, handle, 0);
> +	igt_assert(ret < 0);
> +	igt_assert_eq(errno, EINVAL);
> +
> +	/* Original handle should still be valid */
> +	igt_assert_eq(gem_close_bo(ctx, handle), 0);
> +}
> +
> +/**
> + * test_new_handle_exceeds_int_max - new_handle > INT_MAX should fail
> + *
> + * The kernel enforces an idr_alloc() limitation: new_handle must be <= INT_MAX.
> + */
> +static void test_new_handle_exceeds_int_max(struct gpu_ctx *ctx)
> +{
> +	uint32_t handle;
> +	int ret;
> +
> +	handle = gem_create_bo(ctx, 4096);
> +
> +	/* new_handle > INT_MAX must fail with -EINVAL */
> +	ret = gem_change_handle(ctx, handle, (uint32_t)INT_MAX + 1);
> +	igt_assert(ret < 0);
> +	igt_assert_eq(errno, EINVAL);
> +
> +	/* Also test 0xFFFFFFFF */
> +	ret = gem_change_handle(ctx, handle, 0xFFFFFFFF);
> +	igt_assert(ret < 0);
> +	igt_assert_eq(errno, EINVAL);
> +
> +	/* Original handle should still be valid */
> +	igt_assert_eq(gem_close_bo(ctx, handle), 0);
> +}
> +
> +/**
> + * test_new_handle_equals_old_handle - new_handle = old_handle should fail
> + *
> + * The ioctl treats old_handle = new_handle as invalid input.
> + */
> +static void test_new_handle_equals_old_handle(struct gpu_ctx *ctx)
> +{
> +	uint32_t handle;
> +	int ret;
> +
> +	handle = gem_create_bo(ctx, 4096);
> +
> +	/* new_handle = old_handle must fail with -EINVAL */
> +	ret = gem_change_handle(ctx, handle, handle);
> +	igt_assert(ret < 0);
> +	igt_assert_eq(errno, EINVAL);
> +
> +	/* same thing if handle doesn't exist */
> +	ret = gem_change_handle(ctx, handle + 1, handle + 1);
> +	igt_assert(ret < 0);
> +	igt_assert_eq(errno, EINVAL);
> +
> +	/* Original handle should still be valid */
> +	igt_assert_eq(gem_close_bo(ctx, handle), 0);
> +}
> +
> +/**
> + * test_new_handle_taken - new_handle being the handle of another gem object must fail
> + *
> + * Not allowed to change a handle to a handle that's already taken
> + */
> +static void test_new_handle_taken(struct gpu_ctx *ctx)
> +{
> +	uint32_t handle, handle_2;
> +	int ret;
> +
> +	handle = gem_create_bo(ctx, 4096);
> +	handle_2 = gem_create_bo(ctx, 4096);
> +
> +	/* new_handle is taken must fail with -ENOSPC (direct from idr_alloc) */
> +	ret = gem_change_handle(ctx, handle, handle_2);
> +	igt_assert(ret < 0);
> +	igt_assert_eq(errno, ENOSPC);
> +
> +	/* And the other way */
> +	ret = gem_change_handle(ctx, handle_2, handle);
> +	igt_assert(ret < 0);
> +	igt_assert_eq(errno, ENOSPC);
> +
> +	/* Original handles should still be valid */
> +	igt_assert_eq(gem_close_bo(ctx, handle), 0);
> +	igt_assert_eq(gem_close_bo(ctx, handle_2), 0);
> +}
> +
> +/**
> + * test_invalid_handle - non-existent handle should fail
> + *
> + * Using a handle that doesn't exist should return -ENOENT.
> + */
> +static void test_invalid_handle(struct gpu_ctx *ctx)
> +{
> +	int ret;
> +
> +	/* Use a handle that was never allocated */
> +	ret = gem_change_handle(ctx, 0xDEAD, 0xBEEF);
> +	igt_assert(ret < 0);
> +	igt_assert_eq(errno, ENOENT);
> +
> +	/* Handle 0 is also invalid */
> +	ret = gem_change_handle(ctx, 0, 100);
> +	igt_assert(ret < 0);
> +	igt_assert_eq(errno, ENOENT);
> +}
> +
> +/* Race condition test structs and helpers */
> +
> +struct change_handle_thread_data {
> +	struct gpu_ctx *ctx;
> +	uint32_t handles[4];
> +	bool running;
> +	int unexpected_errno;
> +	pthread_barrier_t barrier;
> +};
> +
> +static void set_up_context(struct change_handle_thread_data *data, struct gpu_ctx *ctx)
> +{
> +	data->handles[0] = gem_create_bo(ctx, BO_SIZE);
> +	igt_assert(data->handles[0] != 0);
> +	data->handles[1] = gem_create_bo(ctx, BO_SIZE);
> +	igt_assert(data->handles[1] != 0);
> +
> +	data->ctx = ctx;
> +	data->handles[2] = max(data->handles[0], data->handles[1]) + 0x10;
> +	data->handles[3] = max(data->handles[0], data->handles[1]) + 0x20;
> +	data->unexpected_errno = 0;
> +	__atomic_store_n(&data->running, true, __ATOMIC_RELEASE);
> +
> +	pthread_barrier_init(&data->barrier, NULL, 2);
> +}
> +
> +/* Race condition test threads */
> +
> +static void *change_handle_thread(void *arg)
> +{
> +	struct change_handle_thread_data *data = arg;
> +	uint32_t *h = data->handles;
> +	int from[] = {h[0], h[2], h[0], h[3], h[3], h[1], h[1], h[3], h[2], h[1], h[0], h[2]};
> +	int to[] =   {h[1], h[3], h[2], h[1], h[0], h[2], h[0], h[2], h[0], h[3], h[3], h[1]};
It may be a nit, but from[] and to[] are int while holding uint32_t
values, it would be good to unify the types.

> +	int ret;
> +
> +	pthread_barrier_wait(&data->barrier);
> +
> +	while (__atomic_load_n(&data->running, __ATOMIC_ACQUIRE)) {
> +		for (int i = 0; i < 12; i++) {
> +			ret = gem_change_handle(data->ctx, from[i], to[i]);
> +			/*
> +			 * Expected errors:
> +			 * - ENOENT: old handle invalid
> +			 * - ENOSPC: target handle occupied (collision detected during idr_alloc)
> +			 */
> +			if (ret != 0 && errno != ENOENT && errno != ENOSPC)
> +				data->unexpected_errno = errno;
> +		}
> +
> +		sched_yield();
> +	}
> +
> +       return NULL;
spaces instead of a tab

> +}
> +
> +static void *close_recreate_thread(void *arg)
> +{
> +	struct change_handle_thread_data *data = arg;
> +	uint32_t *h = data->handles;
> +	pthread_t tid = pthread_self();
> +	int handle, i, ret;
> +
> +	pthread_barrier_wait(&data->barrier);
> +
> +	while (__atomic_load_n(&data->running, __ATOMIC_ACQUIRE)) {
> +		/* Close a handle; keep trying until success */
> +		i = 0;
> +		while (true) {
> +			ret = gem_close_bo(data->ctx, h[i]);
> +			if (ret == 0) {
> +				if (verbose_logging) {
> +					igt_info("[CLOSE-TID:%lu] Closed handle %u\n",
> +						 (unsigned long)tid, h[i]);
> +				}
> +				break;
> +			}
> +
> +			i += 1;
> +			if (i > 3)
> +				i = 0;
> +
> +			if (!__atomic_load_n(&data->running, __ATOMIC_ACQUIRE))
> +				return NULL;
> +		}
> +
> +		/* Open a handle */
> +		handle = gem_create_bo(data->ctx, BO_SIZE);
> +		if (verbose_logging) {
> +			igt_info("[CLOSE-TID:%lu] Created new BO with handle %u\n",
> +				 (unsigned long)tid, handle);
> +		}
> +
> +		/* If the opened handle isn't one of the four ones being
> +		 * shuffled, change_handle it in. Keep trying until success.
> +		 */
> +		if (handle != h[0] && handle != h[1] && handle != h[2] && handle != h[3]) {
> +			i = 0;
> +			while (true) {
> +				ret = gem_change_handle(data->ctx, handle, h[i]);
> +				if (ret == 0) {
> +					if (verbose_logging) {
> +						igt_info("[CLOSE-TID:%lu] Change handle %u to %u\n",
> +							 (unsigned long)tid, handle, h[i]);
> +					}
> +					break;
> +				}
> +
> +				if (ret != 0 && errno != ENOENT && errno != ENOSPC)
> +					data->unexpected_errno = errno;
> +
> +				i += 1;
> +				if (i > 3)
> +					i = 0;
> +
> +				if (!__atomic_load_n(&data->running, __ATOMIC_ACQUIRE))
> +					return NULL;
> +			}
> +		}
> +
> +		sched_yield();
> +	}
> +
> +	return NULL;
> +}
> +
> +static void *export_import_thread(void *arg)
> +{
> +	struct change_handle_thread_data *data = arg;
> +	struct drm_prime_handle prime_args;
> +	uint32_t *h = data->handles;
> +	pthread_t tid = pthread_self();
> +	int handle, i, ret, dmabuf_fd;
> +
> +	pthread_barrier_wait(&data->barrier);
> +
> +	while (__atomic_load_n(&data->running, __ATOMIC_ACQUIRE)) {
> +		/* Export a handle; keep trying until success */
> +		i = 0;
> +		while (true) {
> +			prime_args.handle = h[i];
> +			prime_args.flags = DRM_CLOEXEC | DRM_RDWR;
> +			ret = igt_ioctl(data->ctx->fd, DRM_IOCTL_PRIME_HANDLE_TO_FD, &prime_args);
> +			if (ret == 0) {
> +				if (verbose_logging) {
> +					igt_info("[PRIME-TID:%lu] Exported handle %u\n",
> +						 (unsigned long)tid, h[i]);
> +				}
> +				break;
> +			}
> +
> +			i += 1;
> +			if (i > 3)
> +				i = 0;
> +
> +			if (!__atomic_load_n(&data->running, __ATOMIC_ACQUIRE))
> +				return NULL;
> +		}
> +
> +		dmabuf_fd = prime_args.fd;
> +
> +		/* Import the handle; it should be one of the four handles in the shuffle */
> +		prime_args.fd = dmabuf_fd;
> +		prime_args.flags = 0;
> +		ret = igt_ioctl(data->ctx->fd, DRM_IOCTL_PRIME_FD_TO_HANDLE, &prime_args);
> +
> +		igt_assert_eq(ret, 0);
I don't think you can assert in a worker thread. It doesn't return
cleanly.

> +		handle = prime_args.handle;
> +		if (verbose_logging) {
> +			igt_info("[PRIME-TID:%lu] Imported; got handle %u\n",
> +				 (unsigned long)tid, handle);
> +		}
> +		igt_assert(handle == h[0] || handle == h[1] || handle == h[2] || handle == h[3]);
Same here.

> +
> +		close(dmabuf_fd);
> +
> +		sched_yield();
> +	}
> +
> +	return NULL;
> +}
> +
> +/* Race condition tests */
> +
> +/**
> + * test_race_change_change - Test for race conditions of change_handle against itself
> + *
> + * Start two threads, each of which constantly attempt changes between four different handles
> + * Try to catch race conditions such as changing same handle to two different handles,
> + * Changing two handles to the same handle, changing a handle to a handle itself being changed,
> + * etc.
> + */
> +static void test_race_change_change(struct gpu_ctx *ctx)
> +{
> +	pthread_t thread_change_1, thread_change_2;
> +	struct change_handle_thread_data data = {0};
> +	int num_handles, ret;
> +
> +	set_up_context(&data, ctx);
> +
> +	/* Start both threads simultaneously */
> +	igt_assert_eq(pthread_create(&thread_change_1, NULL, change_handle_thread, &data), 0);
> +	igt_assert_eq(pthread_create(&thread_change_2, NULL, change_handle_thread, &data), 0);
> +
> +	/* Let them race */
> +	sleep(RACE_DURATION_SEC);
> +
> +	__atomic_store_n(&data.running, false, __ATOMIC_RELEASE);
> +	pthread_join(thread_change_1, NULL);
> +	pthread_join(thread_change_2, NULL);
> +
> +	pthread_barrier_destroy(&data.barrier);
> +
> +	igt_assert_eq(data.unexpected_errno, 0);
> +
> +	/* Exactly two gem objects should still exist */
> +	num_handles = 0;
> +	for (int i = 0; i < 4; i++) {
> +		ret = gem_close_bo(ctx, data.handles[i]);
> +
> +		if (ret == 0) {
> +			num_handles += 1;
> +		} else {
> +			igt_assert_eq(errno, EINVAL);
> +		}
> +	}
> +	igt_assert_eq(num_handles, 2);
> +}
> +
> +/**
> + * test_race_change_create_close - Test for race conditions of change_handle against gem create / close
> + *
> + * Start two threads, one of which attempts to change between four handles while the other alternates
> + * closing a gem handle and creating new ones. Test for race conditions such as change a handle that's
> + * being closed or change a handle to a value that's being created.
> + */
> +static void test_race_change_create_close(struct gpu_ctx *ctx)
> +{
> +	pthread_t thread_change, thread_create_close;
> +	struct change_handle_thread_data data = {0};
> +	int num_handles, ret;
> +
> +	set_up_context(&data, ctx);
> +
> +	/* Start both threads simultaneously */
> +	igt_assert_eq(pthread_create(&thread_change, NULL, change_handle_thread, &data), 0);
> +	igt_assert_eq(pthread_create(&thread_create_close, NULL, close_recreate_thread, &data), 0);
> +
> +	/* Let them race */
> +	sleep(RACE_DURATION_SEC);
> +
> +	__atomic_store_n(&data.running, false, __ATOMIC_RELEASE);
> +	pthread_join(thread_change, NULL);
> +	pthread_join(thread_create_close, NULL);
> +
> +	pthread_barrier_destroy(&data.barrier);
> +
> +	igt_assert_eq(data.unexpected_errno, 0);
> +
> +	/* Depending on where create / close ended up, there can be 1 or 2 handles open */
> +	num_handles = 0;
> +	for (int i = 0; i < 4; i++) {
> +		ret = gem_close_bo(ctx, data.handles[i]);
> +
> +		if (ret == 0) {
> +			num_handles += 1;
> +		} else {
> +			igt_assert_eq(errno, EINVAL);
> +		}
> +	}
> +	igt_assert(num_handles >= 1 && num_handles <= 2);
> +}
> +
> +/**
> + * test_race_change_export_import - Test for race conditions of change_handle against prime export / import
> + *
> + * Start two threads, one of which attempts to change between four handles while the other exports
> + * handles, then imports them again. Check for race conditions such as changing a handle while exporting
> + * it or important a handle while changing it.
> + */
> +static void test_race_change_export_import(struct gpu_ctx *ctx)
> +{
> +	pthread_t thread_change, thread_export_import;
> +	struct change_handle_thread_data data = {0};
> +	int num_handles, ret;
> +
> +	set_up_context(&data, ctx);
> +
> +	/* Start both threads simultaneously */
> +	igt_assert_eq(pthread_create(&thread_change, NULL, change_handle_thread, &data), 0);
> +	igt_assert_eq(pthread_create(&thread_export_import, NULL, export_import_thread, &data), 0);
> +
> +	/* Let them race */
> +	sleep(RACE_DURATION_SEC);
> +
> +	__atomic_store_n(&data.running, false, __ATOMIC_RELEASE);
> +	pthread_join(thread_change, NULL);
> +	pthread_join(thread_export_import, NULL);
> +
> +	pthread_barrier_destroy(&data.barrier);
> +
> +	igt_assert_eq(data.unexpected_errno, 0);
> +
> +	/* Exporting a handle doesn't close it, so should still be exactly 2 */
> +	num_handles = 0;
> +	for (int i = 0; i < 4; i++) {
> +		ret = gem_close_bo(ctx, data.handles[i]);
> +
> +		if (ret == 0) {
> +			num_handles += 1;
> +		} else {
> +			igt_assert_eq(errno, EINVAL);
> +		}
> +	}
> +	igt_assert_eq(num_handles, 2);
> +}
> +
> +int igt_main()
> +{
> +	struct gpu_ctx ctx = { .fd = -1 };
> +
> +	igt_fixture() {
> +		ctx.fd = drm_open_driver_render(DRIVER_ANY);
> +		igt_require(ctx.fd >= 0);
> +
> +		ctx.driver = detect_driver(ctx.fd);
> +		igt_require_f(ctx.driver != DRIVER_TYPE_UNKNOWN,
> +			      "Unsupported GPU driver\n");
> +
> +		if (ctx.driver == DRIVER_TYPE_I915)
> +			igt_require_gem(ctx.fd);
> +	}
> +
> +	igt_describe("Sanity Test: Change handle actually changes handle");
> +	igt_subtest("sanity")
> +		test_sanity(&ctx);
> +
> +	igt_describe("Functinality Test: Change handle affects result of prime import");
/Functinality/Functionality/

> +	igt_subtest("change_prime")
> +		test_change_export(&ctx);
> +
> +
> +	igt_describe("Edge Case: New handle = 0 should fail");
> +	igt_subtest("new_handle_zero")
> +		test_new_handle_zero(&ctx);
> +
> +	igt_describe("Edge Case: New handle > INT_MAX should fail");
> +	igt_subtest("new_handle_exceeds_int_max")
> +		test_new_handle_exceeds_int_max(&ctx);
> +
> +	igt_describe("Edge Case: Changing a handle to itself should fail");
> +	igt_subtest("new_handle_equals_old_handle")
> +		test_new_handle_equals_old_handle(&ctx);
> +
> +	igt_describe("Edge Case: Change a handle to a value which is taken should fail");
> +	igt_subtest("new_handle_taken")
> +		test_new_handle_taken(&ctx);
> +
> +	igt_describe("Edge Case: Changing a non-existent handle should fail");
> +	igt_subtest("invalid_handle")
> +		test_invalid_handle(&ctx);
> +
> +
I'm not sure the extra blank line here is needed

> +	igt_describe("Race: Changing handle vs. Changing handle");
> +	igt_subtest("race_change_change")
> +		test_race_change_change(&ctx);
> +
> +	igt_describe("Race: Changing handle vs. Create / Close");
> +	igt_subtest("race_change_create_close")
> +		test_race_change_create_close(&ctx);
> +
> +	igt_describe("Race: Changing handle vs. Export / Import");
> +	igt_subtest("race_change_export_import")
> +		test_race_change_export_import(&ctx);
> +
> +	igt_fixture() {
> +		drm_close_driver(ctx.fd);
> +	}
> +
> +}
> \ No newline at end of file
Add newline at the end of the file.

> diff --git a/tests/meson.build b/tests/meson.build
> index 4f5c9be7b..ab083916d 100644
> --- a/tests/meson.build
> +++ b/tests/meson.build
> @@ -104,6 +104,7 @@ intel_i915_progs = [
>  	'gem_blits',
>  	'gem_busy',
>  	'gem_caching',
> +	'gem_change_handle',
>  	'gem_create',
>  	'gem_ccs',
>
Overall, this series looks much better now. One open question for Kamil:
since these tests exercise functionality that's currently disabled in
the kernel (drm_invalid_op), and the IGT coverage is meant to be a
prerequisite for bringing this feature back, how do we properly test it
in the meantime?

Also can you look at patchwork this changes currently failed to git am
https://patchwork.freedesktop.org/series/175696/#rev3


-- 
Best regards,
Sebastian


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] tests/intel: Add edge case and race condition tests for GEM_CHANGE_HANDLE
  2026-10-14  9:42 ` [PATCH] tests/intel: Add edge case and race condition tests for GEM_CHANGE_HANDLE Sebastian Brzezinka
@ 2026-10-14 13:25   ` Francis, David
  0 siblings, 0 replies; 7+ messages in thread
From: Francis, David @ 2026-10-14 13:25 UTC (permalink / raw)
  To: Sebastian Brzezinka, igt-dev@lists.freedesktop.org
  Cc: tvrtko.ursulin@igalia.com, Prosyak, Vitaly, Kamil Konieczny,
	Koenig, Christian, Simona Vetter, Deucher, Alexander

> Overall, this series looks much better now. One open question for Kamil:
> since these tests exercise functionality that's currently disabled in
> the kernel (drm_invalid_op), and the IGT coverage is meant to be a
> prerequisite for bringing this feature back, how do we properly test it
> in the meantime?
There's a kernel patch on dri-devel
"[PATCH v2] Fix and enable CHANGE_HANDLE".
You'd need to apply that patch manually for testing.

Rest of feedback is appreciated and should be straightforward

Thanks,
David

________________________________________
From: Sebastian Brzezinka <sebastian.brzezinka@intel.com>
Sent: Wednesday, October 14, 2026 5:42 AM
To: Francis, David; igt-dev@lists.freedesktop.org
Cc: tvrtko.ursulin@igalia.com; Prosyak, Vitaly; Kamil Konieczny; Sebastian Brzezinka; Koenig, Christian; Simona Vetter; Deucher, Alexander
Subject: Re: [PATCH] tests/intel: Add edge case and race condition tests for GEM_CHANGE_HANDLE

Hi David,

On Wed Oct 7, 2026 at 4:12 PM CEST, David Francis wrote:
> From: Vitaly Prosyak <vitaly.prosyak@amd.com>
>
> Add a GPU-agnostic GEM_CHANGE_HANDLE tests. These test for basic
> functionality, edge cases, and race conditions of this ioctl against
> itself, gem close, gem create, and prime import and export.
>
> GEM_CHANGE_HANDLE is currently wired to drm_invalid_op in DRM core. This IGT
> coverage is a prerequisite for validating the eventual kernel-side ioctl
> implementation and makes fixing its security bugs safer.
>
> Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
> Cc: Sebastian Brzezinka <sebastian.brzezinka@intel.com>
> Cc: Christian Konig <christian.koenig@amd.com>
> Cc: Simona Vetter <simona@ffwll.ch>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Signed-off-by: Vitaly Prosyak <vitaly.prosyak@amd.com>
> Signed-off-by: David Francis <David.Francis@amd.com>
> ---
>  tests/intel/gem_change_handle.c | 747 ++++++++++++++++++++++++++++++++
>  tests/meson.build               |   1 +
>  2 files changed, 748 insertions(+)
>  create mode 100644 tests/intel/gem_change_handle.c
>
> diff --git a/tests/intel/gem_change_handle.c b/tests/intel/gem_change_handle.c
> new file mode 100644
> index 000000000..23f2f8b39
> --- /dev/null
> +++ b/tests/intel/gem_change_handle.c
> @@ -0,0 +1,747 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright 2026 Advanced Micro Devices, Inc.
> + * Copyright 2026 Intel Corporation
> + *
> + * GPU-agnostic race condition tests for GEM_CHANGE_HANDLE ioctl
> + *
> + * NOTE: This test is located in tests/intel/ directory because:
> + * 1. All gem_* tests are traditionally placed in tests/intel/ regardless
> + *    of GPU vendor, following IGT convention for GEM-related tests
> + * 2. The test uses i915/gem.h and i915/gem_create.h for GEM object creation
> + *    on Intel platforms, though it works vendor-agnostically via DRM core
> + * 3. Some subtest designs (noop-same-handle, invalid-*, edge-*, functional-*)
> + *    were proposed by Simona Vetter (Intel), hence Intel copyright header
> + *
> + */
> +
> +#include <errno.h>
> +#include <fcntl.h>
a
It looks like fcntl is not used anymore.

> +#include <limits.h>
> +#include <pthread.h>
> +#include <sched.h>
> +#include <semaphore.h>
same here

> +#include <signal.h>
and here
> +#include <string.h>
> +#include <sys/stat.h>
and here, please double check it, I may have missed some usage.

> +
> +#include "igt.h"
> +#include "igt_device.h"
> +#include "i915/gem.h"
> +#include "i915/gem_create.h"
> +
> +/* AMDGPU includes (if available) */
> +#if __has_include("igt_amd.h")
> +#include "lib/amdgpu/amd_memory.h"
> +#include "igt_amd.h"
> +#define HAS_AMDGPU 1
> +#else
> +#define HAS_AMDGPU 0
> +#endif
> +
> +#define BO_SIZE (16 * 1024)
> +#define RACE_DURATION_SEC 1
> +
> +static bool verbose_logging;
> +
> +/**
> + * TEST: gem change handle
> + * Description: tests for drm ioctl GEM_CHANGE_HANDLE
> + * Category: Core
> + * Mega feature: General Core features
> + * Sub-category: GEM handles
> + * Functionality: gem change handle
> + * Feature: gem
> + * Run type: FULL
> + *
> + * SUBTEST: sanity
> + * Description: Changing a handle to a new value works as intended
> + *
> + * SUBTEST: change_prime
> + * Description: Changing a handle affects the result of prime import
> + *
> + * SUBTEST: new_handle_zero
> + * Description: Changing a handle to 0 should fail
> + *
> + * SUBTEST: new_handle_exceeds_int_max
> + * Description: Changing a handle to > INT_MAX should fail
> + *
> + * SUBTEST: new_handle_equals_old_handle
> + * Description: Changing a handle to itself should fail
> + *
> + * SUBTEST: new_handle_taken
> + * Description: Changing a handle to a value already in use should fail
> + *
> + * SUBTEST: invalid_handle
> + * Description: Changing a non-existent handle should fail
> + *
> + * SUBTEST: race_change_change
> + * Description: Test for race conditions between two GEM_CHANGE_HANDLE
> + *
> + * SUBTEST: race_change_create_close
> + * Description: Test for race conditions between GEM_CHANGE_HANDLE, GEM_CREATE, and GEM_CLOSE
> + *
> + * SUBTEST: race_change_export_import
> + * Description: Test for race conditions between GEM_CHANGE_HANDLE, prime export, and prime import
> + */
> +
> +IGT_TEST_DESCRIPTION("Edge case and race condition tests for GEM_CHANGE_HANDLE");
> +
> +/* Structs */
> +
> +/* Define DRM_IOCTL_GEM_CHANGE_HANDLE if not available in kernel headers */
> +#ifndef DRM_IOCTL_GEM_CHANGE_HANDLE
> +struct drm_gem_change_handle {
> +     __u32 handle;
> +     __u32 new_handle;
> +};
> +
> +#define DRM_IOCTL_GEM_CHANGE_HANDLE    DRM_IOWR(0xD2, struct drm_gem_change_handle)
> +#endif
> +
> +/* Driver type detection */
> +enum gpu_driver {
> +     DRIVER_TYPE_UNKNOWN = 0,
> +     DRIVER_TYPE_I915,
> +     DRIVER_TYPE_AMDGPU,
> +};
> +
> +/* GPU context structure */
> +struct gpu_ctx {
> +     int fd;
> +     enum gpu_driver driver;
> +};
> +
> +/* Helpers */
> +
> +static uint32_t gem_create_bo(struct gpu_ctx *ctx, uint64_t size)
> +{
> +     uint32_t handle = 0;
> +
> +     switch (ctx->driver) {
> +     case DRIVER_TYPE_I915: {
> +             struct drm_i915_gem_create create = { .size = size };
> +
> +             do_ioctl(ctx->fd, DRM_IOCTL_I915_GEM_CREATE, &create);
> +             handle = create.handle;
> +             break;
> +     }
> +     case DRIVER_TYPE_AMDGPU:
> +#if HAS_AMDGPU
> +             handle = igt_amd_create_bo(ctx->fd, size);
> +#else
> +             igt_require_f(0, "AMDGPU header not found for AMDGPU driver\n");
> +#endif
> +             break;
> +     default:
> +             igt_assert_f(0, "Unsupported driver\n");
> +     }
> +
> +     igt_assert(handle != 0);
> +     return handle;
> +}
> +
> +static int gem_close_bo(struct gpu_ctx *ctx, uint32_t handle)
> +{
> +     struct drm_gem_close close_args = { .handle = handle };
> +
> +     return igt_ioctl(ctx->fd, DRM_IOCTL_GEM_CLOSE, &close_args);
> +}
> +
> +static int gem_change_handle(struct gpu_ctx *ctx, uint32_t old_handle, uint32_t new_handle)
> +{
> +     struct drm_gem_change_handle args = {
> +             .handle = old_handle,
> +             .new_handle = new_handle,
> +     };
> +     return igt_ioctl(ctx->fd, DRM_IOCTL_GEM_CHANGE_HANDLE, &args);
> +}
> +
> +static enum gpu_driver detect_driver(int fd)
> +{
> +     /* Check AMDGPU first (primary target) */
> +     if (is_amdgpu_device(fd))
> +             return DRIVER_TYPE_AMDGPU;
> +     if (is_i915_device(fd))
> +             return DRIVER_TYPE_I915;
> +     return DRIVER_TYPE_UNKNOWN;
> +}
> +
> +/* Sanity and Functionality tests */
> +
> +/**
> + * test_sanity - verify rename actually works
> + *
> + * After a successful rename:
> + * - GEM_CLOSE(old_handle) must FAIL (handle no longer exists)
> + * - GEM_CLOSE(new_handle) must SUCCEED (object is there)
> + */
> +static void test_sanity(struct gpu_ctx *ctx)
> +{
> +     uint32_t handle, new_h;
> +     int ret;
> +
> +     handle = gem_create_bo(ctx, 4096);
> +     new_h = handle + 1000;
> +
> +     /* Perform the rename */
> +     ret = gem_change_handle(ctx, handle, new_h);
> +     igt_assert_f(ret == 0,
> +                  "gem_change_handle(%u -> %u) failed: %s\n",
> +                  handle, new_h, strerror(errno));
> +
> +     /* Old handle must be invalid now */
> +     ret = gem_close_bo(ctx, handle);
> +     igt_assert_f(ret < 0 && errno == EINVAL,
> +                  "GEM_CLOSE(old_handle=%u) should fail with EINVAL, "
> +                  "got ret=%d errno=%d (%s)\n",
> +                  handle, ret, errno, strerror(errno));
> +
> +     /* New handle must be valid */
> +     ret = gem_close_bo(ctx, new_h);
> +     igt_assert_f(ret == 0,
> +                  "GEM_CLOSE(new_handle=%u) should succeed, "
> +                  "got ret=%d errno=%d (%s)\n",
> +                  new_h, ret, errno, strerror(errno));
> +}
> +
> +/**
> + * test_change_export - changing a handle affects prime import
> + *
> + * Importing a prime object that has had its handle changed since it was
> + * exported should get the new handle
> + */
> +static void test_change_export(struct gpu_ctx *ctx)
> +{
> +     int ret, handle1, handle2, dmabuf_fd, imported_handle;
> +
> +     /* Create a handle and export it */
> +     handle1 = gem_create_bo(ctx, 4096);
> +     handle2 = handle1 + 1;
> +     dmabuf_fd = prime_handle_to_fd(ctx->fd, handle1);
> +
> +     /* When you import, you get the same handle back */
> +     imported_handle = prime_fd_to_handle(ctx->fd, dmabuf_fd);
> +     igt_assert_eq(imported_handle, handle1);
> +
> +     /* Change the handle (should succeed) */
> +     ret = gem_change_handle(ctx, handle1, handle2);
> +     igt_assert_eq(ret, 0);
> +
> +     /* Import, should get the new handle */
> +     imported_handle = prime_fd_to_handle(ctx->fd, dmabuf_fd);
> +     igt_assert_eq(imported_handle, handle2);
> +
> +     /* Confirm gem object is at the new handle */
> +     igt_assert_eq(gem_close_bo(ctx, handle2), 0);
> +
> +     ret = gem_close_bo(ctx, handle1);
> +     igt_assert(ret < 0);
> +     igt_assert_eq(errno, EINVAL);
> +
> +     close(dmabuf_fd);
> +}
> +
> +/* Edge case and fail case tests */
> +
> +/**
> + * test_new_handle_zero - new_handle = 0 should fail
> + *
> + * 0 is not a valid gem handle.
> + */
> +static void test_new_handle_zero(struct gpu_ctx *ctx)
> +{
> +     uint32_t handle;
> +     int ret;
> +
> +     handle = gem_create_bo(ctx, 4096);
> +
> +     /* new_handle = 0 must fail with -EINVAL */
> +     ret = gem_change_handle(ctx, handle, 0);
> +     igt_assert(ret < 0);
> +     igt_assert_eq(errno, EINVAL);
> +
> +     /* Original handle should still be valid */
> +     igt_assert_eq(gem_close_bo(ctx, handle), 0);
> +}
> +
> +/**
> + * test_new_handle_exceeds_int_max - new_handle > INT_MAX should fail
> + *
> + * The kernel enforces an idr_alloc() limitation: new_handle must be <= INT_MAX.
> + */
> +static void test_new_handle_exceeds_int_max(struct gpu_ctx *ctx)
> +{
> +     uint32_t handle;
> +     int ret;
> +
> +     handle = gem_create_bo(ctx, 4096);
> +
> +     /* new_handle > INT_MAX must fail with -EINVAL */
> +     ret = gem_change_handle(ctx, handle, (uint32_t)INT_MAX + 1);
> +     igt_assert(ret < 0);
> +     igt_assert_eq(errno, EINVAL);
> +
> +     /* Also test 0xFFFFFFFF */
> +     ret = gem_change_handle(ctx, handle, 0xFFFFFFFF);
> +     igt_assert(ret < 0);
> +     igt_assert_eq(errno, EINVAL);
> +
> +     /* Original handle should still be valid */
> +     igt_assert_eq(gem_close_bo(ctx, handle), 0);
> +}
> +
> +/**
> + * test_new_handle_equals_old_handle - new_handle = old_handle should fail
> + *
> + * The ioctl treats old_handle = new_handle as invalid input.
> + */
> +static void test_new_handle_equals_old_handle(struct gpu_ctx *ctx)
> +{
> +     uint32_t handle;
> +     int ret;
> +
> +     handle = gem_create_bo(ctx, 4096);
> +
> +     /* new_handle = old_handle must fail with -EINVAL */
> +     ret = gem_change_handle(ctx, handle, handle);
> +     igt_assert(ret < 0);
> +     igt_assert_eq(errno, EINVAL);
> +
> +     /* same thing if handle doesn't exist */
> +     ret = gem_change_handle(ctx, handle + 1, handle + 1);
> +     igt_assert(ret < 0);
> +     igt_assert_eq(errno, EINVAL);
> +
> +     /* Original handle should still be valid */
> +     igt_assert_eq(gem_close_bo(ctx, handle), 0);
> +}
> +
> +/**
> + * test_new_handle_taken - new_handle being the handle of another gem object must fail
> + *
> + * Not allowed to change a handle to a handle that's already taken
> + */
> +static void test_new_handle_taken(struct gpu_ctx *ctx)
> +{
> +     uint32_t handle, handle_2;
> +     int ret;
> +
> +     handle = gem_create_bo(ctx, 4096);
> +     handle_2 = gem_create_bo(ctx, 4096);
> +
> +     /* new_handle is taken must fail with -ENOSPC (direct from idr_alloc) */
> +     ret = gem_change_handle(ctx, handle, handle_2);
> +     igt_assert(ret < 0);
> +     igt_assert_eq(errno, ENOSPC);
> +
> +     /* And the other way */
> +     ret = gem_change_handle(ctx, handle_2, handle);
> +     igt_assert(ret < 0);
> +     igt_assert_eq(errno, ENOSPC);
> +
> +     /* Original handles should still be valid */
> +     igt_assert_eq(gem_close_bo(ctx, handle), 0);
> +     igt_assert_eq(gem_close_bo(ctx, handle_2), 0);
> +}
> +
> +/**
> + * test_invalid_handle - non-existent handle should fail
> + *
> + * Using a handle that doesn't exist should return -ENOENT.
> + */
> +static void test_invalid_handle(struct gpu_ctx *ctx)
> +{
> +     int ret;
> +
> +     /* Use a handle that was never allocated */
> +     ret = gem_change_handle(ctx, 0xDEAD, 0xBEEF);
> +     igt_assert(ret < 0);
> +     igt_assert_eq(errno, ENOENT);
> +
> +     /* Handle 0 is also invalid */
> +     ret = gem_change_handle(ctx, 0, 100);
> +     igt_assert(ret < 0);
> +     igt_assert_eq(errno, ENOENT);
> +}
> +
> +/* Race condition test structs and helpers */
> +
> +struct change_handle_thread_data {
> +     struct gpu_ctx *ctx;
> +     uint32_t handles[4];
> +     bool running;
> +     int unexpected_errno;
> +     pthread_barrier_t barrier;
> +};
> +
> +static void set_up_context(struct change_handle_thread_data *data, struct gpu_ctx *ctx)
> +{
> +     data->handles[0] = gem_create_bo(ctx, BO_SIZE);
> +     igt_assert(data->handles[0] != 0);
> +     data->handles[1] = gem_create_bo(ctx, BO_SIZE);
> +     igt_assert(data->handles[1] != 0);
> +
> +     data->ctx = ctx;
> +     data->handles[2] = max(data->handles[0], data->handles[1]) + 0x10;
> +     data->handles[3] = max(data->handles[0], data->handles[1]) + 0x20;
> +     data->unexpected_errno = 0;
> +     __atomic_store_n(&data->running, true, __ATOMIC_RELEASE);
> +
> +     pthread_barrier_init(&data->barrier, NULL, 2);
> +}
> +
> +/* Race condition test threads */
> +
> +static void *change_handle_thread(void *arg)
> +{
> +     struct change_handle_thread_data *data = arg;
> +     uint32_t *h = data->handles;
> +     int from[] = {h[0], h[2], h[0], h[3], h[3], h[1], h[1], h[3], h[2], h[1], h[0], h[2]};
> +     int to[] =   {h[1], h[3], h[2], h[1], h[0], h[2], h[0], h[2], h[0], h[3], h[3], h[1]};
It may be a nit, but from[] and to[] are int while holding uint32_t
values, it would be good to unify the types.

> +     int ret;
> +
> +     pthread_barrier_wait(&data->barrier);
> +
> +     while (__atomic_load_n(&data->running, __ATOMIC_ACQUIRE)) {
> +             for (int i = 0; i < 12; i++) {
> +                     ret = gem_change_handle(data->ctx, from[i], to[i]);
> +                     /*
> +                      * Expected errors:
> +                      * - ENOENT: old handle invalid
> +                      * - ENOSPC: target handle occupied (collision detected during idr_alloc)
> +                      */
> +                     if (ret != 0 && errno != ENOENT && errno != ENOSPC)
> +                             data->unexpected_errno = errno;
> +             }
> +
> +             sched_yield();
> +     }
> +
> +       return NULL;
spaces instead of a tab

> +}
> +
> +static void *close_recreate_thread(void *arg)
> +{
> +     struct change_handle_thread_data *data = arg;
> +     uint32_t *h = data->handles;
> +     pthread_t tid = pthread_self();
> +     int handle, i, ret;
> +
> +     pthread_barrier_wait(&data->barrier);
> +
> +     while (__atomic_load_n(&data->running, __ATOMIC_ACQUIRE)) {
> +             /* Close a handle; keep trying until success */
> +             i = 0;
> +             while (true) {
> +                     ret = gem_close_bo(data->ctx, h[i]);
> +                     if (ret == 0) {
> +                             if (verbose_logging) {
> +                                     igt_info("[CLOSE-TID:%lu] Closed handle %u\n",
> +                                              (unsigned long)tid, h[i]);
> +                             }
> +                             break;
> +                     }
> +
> +                     i += 1;
> +                     if (i > 3)
> +                             i = 0;
> +
> +                     if (!__atomic_load_n(&data->running, __ATOMIC_ACQUIRE))
> +                             return NULL;
> +             }
> +
> +             /* Open a handle */
> +             handle = gem_create_bo(data->ctx, BO_SIZE);
> +             if (verbose_logging) {
> +                     igt_info("[CLOSE-TID:%lu] Created new BO with handle %u\n",
> +                              (unsigned long)tid, handle);
> +             }
> +
> +             /* If the opened handle isn't one of the four ones being
> +              * shuffled, change_handle it in. Keep trying until success.
> +              */
> +             if (handle != h[0] && handle != h[1] && handle != h[2] && handle != h[3]) {
> +                     i = 0;
> +                     while (true) {
> +                             ret = gem_change_handle(data->ctx, handle, h[i]);
> +                             if (ret == 0) {
> +                                     if (verbose_logging) {
> +                                             igt_info("[CLOSE-TID:%lu] Change handle %u to %u\n",
> +                                                      (unsigned long)tid, handle, h[i]);
> +                                     }
> +                                     break;
> +                             }
> +
> +                             if (ret != 0 && errno != ENOENT && errno != ENOSPC)
> +                                     data->unexpected_errno = errno;
> +
> +                             i += 1;
> +                             if (i > 3)
> +                                     i = 0;
> +
> +                             if (!__atomic_load_n(&data->running, __ATOMIC_ACQUIRE))
> +                                     return NULL;
> +                     }
> +             }
> +
> +             sched_yield();
> +     }
> +
> +     return NULL;
> +}
> +
> +static void *export_import_thread(void *arg)
> +{
> +     struct change_handle_thread_data *data = arg;
> +     struct drm_prime_handle prime_args;
> +     uint32_t *h = data->handles;
> +     pthread_t tid = pthread_self();
> +     int handle, i, ret, dmabuf_fd;
> +
> +     pthread_barrier_wait(&data->barrier);
> +
> +     while (__atomic_load_n(&data->running, __ATOMIC_ACQUIRE)) {
> +             /* Export a handle; keep trying until success */
> +             i = 0;
> +             while (true) {
> +                     prime_args.handle = h[i];
> +                     prime_args.flags = DRM_CLOEXEC | DRM_RDWR;
> +                     ret = igt_ioctl(data->ctx->fd, DRM_IOCTL_PRIME_HANDLE_TO_FD, &prime_args);
> +                     if (ret == 0) {
> +                             if (verbose_logging) {
> +                                     igt_info("[PRIME-TID:%lu] Exported handle %u\n",
> +                                              (unsigned long)tid, h[i]);
> +                             }
> +                             break;
> +                     }
> +
> +                     i += 1;
> +                     if (i > 3)
> +                             i = 0;
> +
> +                     if (!__atomic_load_n(&data->running, __ATOMIC_ACQUIRE))
> +                             return NULL;
> +             }
> +
> +             dmabuf_fd = prime_args.fd;
> +
> +             /* Import the handle; it should be one of the four handles in the shuffle */
> +             prime_args.fd = dmabuf_fd;
> +             prime_args.flags = 0;
> +             ret = igt_ioctl(data->ctx->fd, DRM_IOCTL_PRIME_FD_TO_HANDLE, &prime_args);
> +
> +             igt_assert_eq(ret, 0);
I don't think you can assert in a worker thread. It doesn't return
cleanly.

> +             handle = prime_args.handle;
> +             if (verbose_logging) {
> +                     igt_info("[PRIME-TID:%lu] Imported; got handle %u\n",
> +                              (unsigned long)tid, handle);
> +             }
> +             igt_assert(handle == h[0] || handle == h[1] || handle == h[2] || handle == h[3]);
Same here.

> +
> +             close(dmabuf_fd);
> +
> +             sched_yield();
> +     }
> +
> +     return NULL;
> +}
> +
> +/* Race condition tests */
> +
> +/**
> + * test_race_change_change - Test for race conditions of change_handle against itself
> + *
> + * Start two threads, each of which constantly attempt changes between four different handles
> + * Try to catch race conditions such as changing same handle to two different handles,
> + * Changing two handles to the same handle, changing a handle to a handle itself being changed,
> + * etc.
> + */
> +static void test_race_change_change(struct gpu_ctx *ctx)
> +{
> +     pthread_t thread_change_1, thread_change_2;
> +     struct change_handle_thread_data data = {0};
> +     int num_handles, ret;
> +
> +     set_up_context(&data, ctx);
> +
> +     /* Start both threads simultaneously */
> +     igt_assert_eq(pthread_create(&thread_change_1, NULL, change_handle_thread, &data), 0);
> +     igt_assert_eq(pthread_create(&thread_change_2, NULL, change_handle_thread, &data), 0);
> +
> +     /* Let them race */
> +     sleep(RACE_DURATION_SEC);
> +
> +     __atomic_store_n(&data.running, false, __ATOMIC_RELEASE);
> +     pthread_join(thread_change_1, NULL);
> +     pthread_join(thread_change_2, NULL);
> +
> +     pthread_barrier_destroy(&data.barrier);
> +
> +     igt_assert_eq(data.unexpected_errno, 0);
> +
> +     /* Exactly two gem objects should still exist */
> +     num_handles = 0;
> +     for (int i = 0; i < 4; i++) {
> +             ret = gem_close_bo(ctx, data.handles[i]);
> +
> +             if (ret == 0) {
> +                     num_handles += 1;
> +             } else {
> +                     igt_assert_eq(errno, EINVAL);
> +             }
> +     }
> +     igt_assert_eq(num_handles, 2);
> +}
> +
> +/**
> + * test_race_change_create_close - Test for race conditions of change_handle against gem create / close
> + *
> + * Start two threads, one of which attempts to change between four handles while the other alternates
> + * closing a gem handle and creating new ones. Test for race conditions such as change a handle that's
> + * being closed or change a handle to a value that's being created.
> + */
> +static void test_race_change_create_close(struct gpu_ctx *ctx)
> +{
> +     pthread_t thread_change, thread_create_close;
> +     struct change_handle_thread_data data = {0};
> +     int num_handles, ret;
> +
> +     set_up_context(&data, ctx);
> +
> +     /* Start both threads simultaneously */
> +     igt_assert_eq(pthread_create(&thread_change, NULL, change_handle_thread, &data), 0);
> +     igt_assert_eq(pthread_create(&thread_create_close, NULL, close_recreate_thread, &data), 0);
> +
> +     /* Let them race */
> +     sleep(RACE_DURATION_SEC);
> +
> +     __atomic_store_n(&data.running, false, __ATOMIC_RELEASE);
> +     pthread_join(thread_change, NULL);
> +     pthread_join(thread_create_close, NULL);
> +
> +     pthread_barrier_destroy(&data.barrier);
> +
> +     igt_assert_eq(data.unexpected_errno, 0);
> +
> +     /* Depending on where create / close ended up, there can be 1 or 2 handles open */
> +     num_handles = 0;
> +     for (int i = 0; i < 4; i++) {
> +             ret = gem_close_bo(ctx, data.handles[i]);
> +
> +             if (ret == 0) {
> +                     num_handles += 1;
> +             } else {
> +                     igt_assert_eq(errno, EINVAL);
> +             }
> +     }
> +     igt_assert(num_handles >= 1 && num_handles <= 2);
> +}
> +
> +/**
> + * test_race_change_export_import - Test for race conditions of change_handle against prime export / import
> + *
> + * Start two threads, one of which attempts to change between four handles while the other exports
> + * handles, then imports them again. Check for race conditions such as changing a handle while exporting
> + * it or important a handle while changing it.
> + */
> +static void test_race_change_export_import(struct gpu_ctx *ctx)
> +{
> +     pthread_t thread_change, thread_export_import;
> +     struct change_handle_thread_data data = {0};
> +     int num_handles, ret;
> +
> +     set_up_context(&data, ctx);
> +
> +     /* Start both threads simultaneously */
> +     igt_assert_eq(pthread_create(&thread_change, NULL, change_handle_thread, &data), 0);
> +     igt_assert_eq(pthread_create(&thread_export_import, NULL, export_import_thread, &data), 0);
> +
> +     /* Let them race */
> +     sleep(RACE_DURATION_SEC);
> +
> +     __atomic_store_n(&data.running, false, __ATOMIC_RELEASE);
> +     pthread_join(thread_change, NULL);
> +     pthread_join(thread_export_import, NULL);
> +
> +     pthread_barrier_destroy(&data.barrier);
> +
> +     igt_assert_eq(data.unexpected_errno, 0);
> +
> +     /* Exporting a handle doesn't close it, so should still be exactly 2 */
> +     num_handles = 0;
> +     for (int i = 0; i < 4; i++) {
> +             ret = gem_close_bo(ctx, data.handles[i]);
> +
> +             if (ret == 0) {
> +                     num_handles += 1;
> +             } else {
> +                     igt_assert_eq(errno, EINVAL);
> +             }
> +     }
> +     igt_assert_eq(num_handles, 2);
> +}
> +
> +int igt_main()
> +{
> +     struct gpu_ctx ctx = { .fd = -1 };
> +
> +     igt_fixture() {
> +             ctx.fd = drm_open_driver_render(DRIVER_ANY);
> +             igt_require(ctx.fd >= 0);
> +
> +             ctx.driver = detect_driver(ctx.fd);
> +             igt_require_f(ctx.driver != DRIVER_TYPE_UNKNOWN,
> +                           "Unsupported GPU driver\n");
> +
> +             if (ctx.driver == DRIVER_TYPE_I915)
> +                     igt_require_gem(ctx.fd);
> +     }
> +
> +     igt_describe("Sanity Test: Change handle actually changes handle");
> +     igt_subtest("sanity")
> +             test_sanity(&ctx);
> +
> +     igt_describe("Functinality Test: Change handle affects result of prime import");
/Functinality/Functionality/

> +     igt_subtest("change_prime")
> +             test_change_export(&ctx);
> +
> +
> +     igt_describe("Edge Case: New handle = 0 should fail");
> +     igt_subtest("new_handle_zero")
> +             test_new_handle_zero(&ctx);
> +
> +     igt_describe("Edge Case: New handle > INT_MAX should fail");
> +     igt_subtest("new_handle_exceeds_int_max")
> +             test_new_handle_exceeds_int_max(&ctx);
> +
> +     igt_describe("Edge Case: Changing a handle to itself should fail");
> +     igt_subtest("new_handle_equals_old_handle")
> +             test_new_handle_equals_old_handle(&ctx);
> +
> +     igt_describe("Edge Case: Change a handle to a value which is taken should fail");
> +     igt_subtest("new_handle_taken")
> +             test_new_handle_taken(&ctx);
> +
> +     igt_describe("Edge Case: Changing a non-existent handle should fail");
> +     igt_subtest("invalid_handle")
> +             test_invalid_handle(&ctx);
> +
> +
I'm not sure the extra blank line here is needed

> +     igt_describe("Race: Changing handle vs. Changing handle");
> +     igt_subtest("race_change_change")
> +             test_race_change_change(&ctx);
> +
> +     igt_describe("Race: Changing handle vs. Create / Close");
> +     igt_subtest("race_change_create_close")
> +             test_race_change_create_close(&ctx);
> +
> +     igt_describe("Race: Changing handle vs. Export / Import");
> +     igt_subtest("race_change_export_import")
> +             test_race_change_export_import(&ctx);
> +
> +     igt_fixture() {
> +             drm_close_driver(ctx.fd);
> +     }
> +
> +}
> \ No newline at end of file
Add newline at the end of the file.

> diff --git a/tests/meson.build b/tests/meson.build
> index 4f5c9be7b..ab083916d 100644
> --- a/tests/meson.build
> +++ b/tests/meson.build
> @@ -104,6 +104,7 @@ intel_i915_progs = [
>       'gem_blits',
>       'gem_busy',
>       'gem_caching',
> +     'gem_change_handle',
>       'gem_create',
>       'gem_ccs',
>
Overall, this series looks much better now. One open question for Kamil:
since these tests exercise functionality that's currently disabled in
the kernel (drm_invalid_op), and the IGT coverage is meant to be a
prerequisite for bringing this feature back, how do we properly test it
in the meantime?

Also can you look at patchwork this changes currently failed to git am
https://patchwork.freedesktop.org/series/175696/#rev3


--
Best regards,
Sebastian


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2026-10-14 13:26 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-10-07 14:12 [PATCH] tests/intel: Add edge case and race condition tests for GEM_CHANGE_HANDLE David Francis
2026-10-07 14:13 ` Francis, David
2026-10-07 17:05   ` Francis, David
2026-10-07 15:49 ` ✗ Fi.CI.BUILD: failure for tests/intel: Add edge case and race condition tests for GEM_CHANGE_HANDLE (rev2) Patchwork
2026-10-07 18:17 ` ✗ Fi.CI.BUILD: failure for tests/intel: Add edge case and race condition tests for GEM_CHANGE_HANDLE (rev3) Patchwork
2026-10-14  9:42 ` [PATCH] tests/intel: Add edge case and race condition tests for GEM_CHANGE_HANDLE Sebastian Brzezinka
2026-10-14 13:25   ` Francis, David

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.