[PATCH i-g-t v6 1/2] tests/intel/gem_exec_capture: Fix many-* subtests

[Kamil Konieczny]

Hi Peter,
On 2024-03-30 at 15:19:31 +0100, Peter Senna Tschudin wrote:
> Currently trying to run `gem_exec_capture --run-subtest
> many-4K-incremental` or `gem_exec_capture --run-subtest many-4K-zero`
> will fail with:
> 
>  (gem_exec_capture:81999) i915/gem_engine_topology-CRITICAL: Test
>  assertion failure function gem_engine_properties_configure, file
>  ../lib/i915/gem_engine_topology.c:577:
>  (gem_exec_capture:81999) i915/gem_engine_topology-CRITICAL: Failed assertion: ret == 1
>  (gem_exec_capture:81999) i915/gem_engine_topology-CRITICAL: Last errno: 9, Bad file descriptor
>  (gem_exec_capture:81999) i915/gem_engine_topology-CRITICAL: error: -1 != 1
> 
> This problem happens inside the macro find_first_available_engine()
> when:
>  1. for_each_ctx_engine() allocates an struct intel_engine_data 'ed'
>     inside a for loop. The core of the issue is that ed only exists
>     inside the for loop. As soon as the for loop ends, ed is out of scope
>     and after it's lifetime.
>  2. intel_get_current_engine() sets '*e' to an address of ed. This is ok
>     while inside the for loop, and is undefined behavior after the for
>     loop ends.
>  3. configure_hangs() uses '*e' after the lifetime of 'ed' has ended
>     leading to undefined behavior
>  4. After the call to find_first_available_engine() __captureN() will
>     fail as it expects '*e' to be valid. This is also undefined
>     behavior.
> 
> This patch fixes the issue in two steps:
>  1. Moves the call to configure_hangs() to inside the for loop, where
>     '*e' is valid because there are no issues with scope and lifetime of
>     'ed'.
>  2. Adds `e = &saved.engine;` to the end of
>     find_first_available_engine(). The reason why this works is twofold:
>     First 'saved' has the same content as e had when there were no
>     variable scope and lifetime issues. Second both '*e' and 'saved' are
>     defined in many() meaning that they both have the same scope and
>     lifetime.
> 
>  v6: new commit message; moves igt_assert(e); to before the call to
>      configure_hangs(). This check is there because '*e' is set by
>      intel_get_current_engine() which will return NULL if ed->n >=
>      ed->nengines.
> 
> Signed-off-by: Peter Senna Tschudin <me at petersenna.com>
> ---
>  tests/intel/gem_exec_capture.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/intel/gem_exec_capture.c b/tests/intel/gem_exec_capture.c
> index 57b178f3e..a8348f21b 100644
> --- a/tests/intel/gem_exec_capture.c
> +++ b/tests/intel/gem_exec_capture.c
> @@ -665,10 +665,12 @@ static bool needs_recoverable_ctx(int fd)
>  		ctx = intel_ctx_create_all_physical(fd); \
>  		igt_assert(ctx); \
>  		for_each_ctx_engine(fd, ctx, e) \
> -			for_each_if(gem_class_can_store_dword(fd, e->class)) \
> +			for_each_if(gem_class_can_store_dword(fd, e->class)) { \
> +				igt_assert(e); \
--------------- ^
Why do you assert here? If it will be NULL code would blow at line
above, where e->class is used.

Regards,
Kamil

> +				saved = configure_hangs(fd, e, ctx->id); \
>  				break; \
> -		igt_assert(e); \
> -		saved = configure_hangs(fd, e, ctx->id); \
> +			} \
> +		e = &saved.engine; \
>  	} while(0)
>  
>  static void many(int fd, int dir, uint64_t size, unsigned int flags)
> -- 
> 2.44.0
>