Re: [PATCH v5 1/2] tests/intel: Add gem_change_handle_race test suite

[Sebastian Brzezinka <sebastian.brzezinka@intel.com>]

Hi Vitaly,

I think it would be good to add this context to the commit message. It
wasn't obvious to me at first that this ioctl is currently wired to
drm_invalid_op apparently landing working IGT tests here is a listed
prerequisite for enabing it.

On Thu Aug 6, 2026 at 4:12 AM CEST, vitaly.prosyak wrote:
> From: Vitaly Prosyak <vitaly.prosyak@amd.com>
>
> This test suite validates concurrent operation handling in GEM handle
> management through the proposed DRM_IOCTL_GEM_CHANGE_HANDLE ioctl.
>
> The test is GPU-agnostic and works with both Intel i915 and AMD amdgpu
> drivers, focusing on proper locking and handle lifecycle management during
> concurrent GEM operations.
>
> Test coverage (7 race condition subtests):
>  - race-change-vs-close: CHANGE_HANDLE races against GEM_CLOSE
>  - race-change-vs-change: Two CHANGE_HANDLE ops race on same handle
>  - race-change-vs-prime: CHANGE_HANDLE races against PRIME_HANDLE_TO_FD
>  - race-aggressive-change-vs-close: High-iteration close vs change
>  - race-exploit-single-thread: Sequential swap+close pattern
>  - race-exploit-random-handles: Random handle stress test
>  - race-close-before-lock: Close-before-lock scenario with CPU pinning
>
> The test uses i915/gem.h infrastructure while remaining vendor-agnostic
> via DRM core, following IGT convention of placing all gem_* tests in
> tests/intel/ directory.
>
> Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Simona Vetter <simona@ffwll.ch>
> Signed-off-by: Vitaly Prosyak <vitaly.prosyak@amd.com>
> ---
> v5 changes (addressing Kamil Konieczny's review feedback):
>  - Replaced all non-ASCII characters (UTF-8 box-drawing, arrows, emoji)
>    with plain ASCII equivalents throughout
>  - Removed Change-Id from commit message
>  - Moved version changelog to after --- (not in git log)
>
> v4 changes (addressing Kamil Konieczny's review feedback):
>  - Removed running_under_gdb() function entirely
>  - Fixed all double newlines throughout
>  - Removed #define _GNU_SOURCE (already defined by meson build system)
>  - Added header comment explaining tests/intel/ location
>  - Fixed check_kernel_traces() brace style
>  - Enhanced pin_to_cpu() with error handling and ARM compatibility
>  - Fixed variable declarations (C89 style)
>  - Removed unnecessary braces in if statements
>  - Fixed static variable initialization
>  - Added meson.build entry
>
> v3 changes (addressing Kamil Konieczny's review feedback):
>  - Removed reference to external documentation
>  - Sanitized test descriptions and comments
>  - Renamed 'race-darknavy-cve' to 'race-close-before-lock'
>  - Renamed internal functions and variables for clarity
>
> v2 changes (addressing Kamil Konieczny's review feedback):
>  - Added header comment explaining why test is in tests/intel/ directory
>  - ARM compatibility fixes (graceful CPU pinning failure handling)
>  - Made pin_to_cpu() handle failures gracefully with igt_debug()
>
> v1:
>  - Initial submission with 7 race condition subtests
>
>  tests/intel/gem_change_handle_race.c | 1798 ++++++++++++++++++++++++++
>  tests/meson.build                    |    1 +
>  2 files changed, 1799 insertions(+)
>  create mode 100644 tests/intel/gem_change_handle_race.c
>
> diff --git a/tests/intel/gem_change_handle_race.c b/tests/intel/gem_change_handle_race.c
> new file mode 100644
> index 000000000..f4b00c65e
> --- /dev/null
> +++ b/tests/intel/gem_change_handle_race.c
> @@ -0,0 +1,1798 @@
> +// 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
> + *
> + * ARM compatibility: This test uses pthread_setaffinity_np() which requires
> + * _GNU_SOURCE and may behave differently on ARM due to different CPU topology.
> + * The test gracefully handles CPU pinning failures and continues execution.
> + */
> +
> +#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
> +
> +/* Helper to check for concurrent access traces in kernel log */
> +static int check_kernel_traces(void)
> +{
> +	FILE *fp;
> +	char line[1024];
> +	int trace_count = 0;
> +
> +	/*
> +	 * concurrent access manifests in kernel logs as:
> +	 * - KASAN reports (if CONFIG_KASAN=y)
> +	 * - Kernel oops/warnings in drm_gem_object_release_handle
> +	 * - Stack traces with drm_gem_object_handle_put_unlocked
> +	 *
> +	 * We grep for function names that appear in the concurrent access path.
> +	 */
> +	fp = popen("dmesg | grep -c -E '(KASAN.*(drm_gem|change_handle)|"
> +		   "drm_gem_object_release_handle|"
> +		   "drm_gem_object_handle_put_unlocked|"
> +		   "drm_gem_change_handle.*RIP)' 2>/dev/null", "r");
Since a lot of tests here rely on greping dmesg output for validation,
I think it's worth first checking that we actually have access to dmesg
It might be overkill, but otherwise I believe the tests will always pass.

I think igt should have already somthing like this. I'm not sure about
specific function, but maybe Kamil can recomend something.

> +	if (fp == NULL)
> +		return 0;
> +
> +	if (fgets(line, sizeof(line), fp) != NULL)
> +		trace_count = atoi(line);
> +	pclose(fp);
> +
> +	return trace_count;
> +}
> +
> +static void pin_to_cpu(int cpu)
> +{
> +	cpu_set_t cpuset;
> +	int ret;
> +
> +	CPU_ZERO(&cpuset);
> +	CPU_SET(cpu, &cpuset);
> +	ret = pthread_setaffinity_np(pthread_self(), sizeof(cpu_set_t), &cpuset);
> +	
tab

Other than this, please run checkpatch from the kernel scripts. This
file has multiple style problems.

-- 
Best regards,
Sebastian