* [PATCH] Fix memory access issue due to variable block scope @ 2024-03-25 21:05 Peter Senna Tschudin 2024-04-10 5:40 ` [PATCH i-g-t v7] tests/intel/gem_exec_capture: Fix many-* subtests Peter Senna Tschudin 0 siblings, 1 reply; 3+ messages in thread From: Peter Senna Tschudin @ 2024-03-25 21:05 UTC (permalink / raw To: igt-dev; +Cc: Peter Senna Tschudin, kamil.konieczny, andi.shyti From: Peter Senna Tschudin <peter.senna@gmail.com> This patch fixes the tests gem_exec_capture@many-4k-incremental and gem_exec_capture@many-4k-zero that are currently failing with an invalid file descriptor error. struct intel_execution_engine2 * intel_get_current_engine(struct intel_engine_data *ed) When intel_get_current_engine is called from the macro for_each_ctx_cfg_engine(), the variable *ed is defined within a for loop. The scope of *ed is limited to that loop, leading to access violations when attempting to access its contents outside the loop. Before to this patch, intel_get_current_engine() would return an element of *ed and attempting to use it after the loop ended resulted in undefined behavior. This patch introduces a memcpy() to copy the contents of ed->current_engine to a memory area not confined by the loop's scope, ensuring safe access to the data. Signed-off-by: Peter Senna Tschudin <peter.senna@gmail.com> --- lib/i915/gem_engine_topology.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/lib/i915/gem_engine_topology.c b/lib/i915/gem_engine_topology.c index afb576afb..b3b809482 100644 --- a/lib/i915/gem_engine_topology.c +++ b/lib/i915/gem_engine_topology.c @@ -189,12 +189,24 @@ static int __query_engine_list(int fd, struct intel_engine_data *ed) struct intel_execution_engine2 * intel_get_current_engine(struct intel_engine_data *ed) { + struct intel_execution_engine2 *ret = NULL; + if (ed->n >= ed->nengines) ed->current_engine = NULL; else if (!ed->n) ed->current_engine = &ed->engines[0]; - return ed->current_engine; + // When called from the macro for_each_ctx_cfg_engine(), *ed is defined + // inside a for loop. In that case, not memcping ed->current_engine + // will lead to a memory access violation when trying to access the + // contents of ed->current_engine after the end of the for loop + if (ed->current_engine) { + ret = malloc(sizeof(*ret)); + if (ret) + memcpy(ret, ed->current_engine, sizeof(*ret)); + } + + return ret; } void intel_next_engine(struct intel_engine_data *ed) -- 2.34.1 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* [PATCH i-g-t v7] tests/intel/gem_exec_capture: Fix many-* subtests 2024-03-25 21:05 [PATCH] Fix memory access issue due to variable block scope Peter Senna Tschudin @ 2024-04-10 5:40 ` Peter Senna Tschudin 2024-04-10 16:38 ` Kamil Konieczny 0 siblings, 1 reply; 3+ messages in thread From: Peter Senna Tschudin @ 2024-04-10 5:40 UTC (permalink / raw To: igt-dev, kamil.konieczny, andi.shyti 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 by: 1. Creating a 'tmpe' variable for code clarity by hinting that this is a temporal variable. 1. Moving the call to configure_hangs() to inside the for loop, where there are no issues with scope and lifetime of 'ed'. 2. Assigning 'e' to a value of 'saved' with 'e = &saved.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 share the same scope and lifetime. Additionally, this patch: - moves the 'igt_assert(tmpe);' to between the assignment and first use of 'tmpe'. - sets 'e = NULL;' and checks if this changes to decide to skip the test. v7: unifies the two patches from v6; creates 'tmpe' for code clarity; sets 'e = NULL;' to decide to skip the the test if still NULL by the end of 'find_first_available_engine()'; places 'igt_assert(tmpe);' between assignment and first use of 'tmpe'; Signed-off-by: Peter Senna Tschudin <me@petersenna.com> --- tests/intel/gem_exec_capture.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/tests/intel/gem_exec_capture.c b/tests/intel/gem_exec_capture.c index 57b178f3e..b79268443 100644 --- a/tests/intel/gem_exec_capture.c +++ b/tests/intel/gem_exec_capture.c @@ -662,13 +662,19 @@ static bool needs_recoverable_ctx(int fd) #define find_first_available_engine(fd, ctx, e, saved) \ do { \ + struct intel_execution_engine2 *tmpe = NULL; \ + e = NULL; \ 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_ctx_engine(fd, ctx, tmpe) { \ + igt_assert(tmpe); \ + if(gem_class_can_store_dword(fd, tmpe->class)) { \ + saved = configure_hangs(fd, tmpe, ctx->id); \ + e = &saved.engine; \ break; \ - igt_assert(e); \ - saved = configure_hangs(fd, e, ctx->id); \ + } \ + } \ + igt_skip_on_f(e == NULL, "no capable engine found\n"); \ } while(0) static void many(int fd, int dir, uint64_t size, unsigned int flags) -- 2.44.0 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH i-g-t v7] tests/intel/gem_exec_capture: Fix many-* subtests 2024-04-10 5:40 ` [PATCH i-g-t v7] tests/intel/gem_exec_capture: Fix many-* subtests Peter Senna Tschudin @ 2024-04-10 16:38 ` Kamil Konieczny 0 siblings, 0 replies; 3+ messages in thread From: Kamil Konieczny @ 2024-04-10 16:38 UTC (permalink / raw To: igt-dev, andi.shyti; +Cc: Peter Senna Tschudin Hi Peter, On 2024-04-10 at 07:40:30 +0200, 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. That is good explanation. > > This patch fixes the issue by: > 1. Creating a 'tmpe' variable for code clarity by hinting that this is a > temporal variable. > 1. Moving the call to configure_hangs() to inside the for loop, where > there are no issues with scope and lifetime of 'ed'. > 2. Assigning 'e' to a value of 'saved' with 'e = &saved.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 > share the same scope and lifetime. Skip this second explanation points starting from 'Creating tmpe' as it can be readed from patch, just write that using 'e' outside for_each_ctx_engine() is incorrect and this patch fixes it. > > Additionally, this patch: > - moves the 'igt_assert(tmpe);' to between the assignment and first use > of 'tmpe'. > - sets 'e = NULL;' and checks if this changes to decide to skip the > test. And here just write that you changes assert into skip_on and why. > > v7: unifies the two patches from v6; creates 'tmpe' for code clarity; > sets 'e = NULL;' to decide to skip the the test if still NULL by > the end of 'find_first_available_engine()'; places > 'igt_assert(tmpe);' between assignment and first use of 'tmpe'; > > Signed-off-by: Peter Senna Tschudin <me@petersenna.com> > --- > tests/intel/gem_exec_capture.c | 14 ++++++++++---- > 1 file changed, 10 insertions(+), 4 deletions(-) > > diff --git a/tests/intel/gem_exec_capture.c b/tests/intel/gem_exec_capture.c > index 57b178f3e..b79268443 100644 > --- a/tests/intel/gem_exec_capture.c > +++ b/tests/intel/gem_exec_capture.c > @@ -662,13 +662,19 @@ static bool needs_recoverable_ctx(int fd) > > #define find_first_available_engine(fd, ctx, e, saved) \ > do { \ > + struct intel_execution_engine2 *tmpe = NULL; \ > + e = NULL; \ Add here newline to improve readability: \ With that added feel free to add my r-b. I do not see your patch on patchwork, maybe resend as a new one? Regards, Kamil > 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_ctx_engine(fd, ctx, tmpe) { \ > + igt_assert(tmpe); \ > + if(gem_class_can_store_dword(fd, tmpe->class)) { \ > + saved = configure_hangs(fd, tmpe, ctx->id); \ > + e = &saved.engine; \ > break; \ > - igt_assert(e); \ > - saved = configure_hangs(fd, e, ctx->id); \ > + } \ > + } \ > + igt_skip_on_f(e == NULL, "no capable engine found\n"); \ > } while(0) > > static void many(int fd, int dir, uint64_t size, unsigned int flags) > -- > 2.44.0 > ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-04-10 16:38 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-03-25 21:05 [PATCH] Fix memory access issue due to variable block scope Peter Senna Tschudin 2024-04-10 5:40 ` [PATCH i-g-t v7] tests/intel/gem_exec_capture: Fix many-* subtests Peter Senna Tschudin 2024-04-10 16:38 ` Kamil Konieczny
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.