[PATCH i-g-t] lib|tests: Don't dereference engine outside for_each_ctx_engine macro

[Zbigniew Kempczyński]

On Tue, Aug 13, 2024 at 03:36:50PM +0200, Peter Senna Tschudin wrote:
> Please do not apply this patch. I am working on a proper fix.

Applying fix stops you from working on a your fix?

--
Zbigniew

> 
> On 13.08.2024 14:45, Zbigniew Kempczyński wrote:
> > Engine used inside for_each_ctx_engine() macro is valid only inside its
> > block as it is accessing local stack memory. Using outside the block works
> > by an accident if luckily nothing will overwrite this stack before use.
> > Fix found risky usages by using engine only inside the block or do its
> > copy.
> > 
> > Signed-off-by: Zbigniew Kempczyński <zbigniew.kempczynski at intel.com>
> > Cc: Andi Shyti <andi.shyti at linux.intel.com>
> > ---
> >  lib/igt_fb.c                    |  8 ++++----
> >  tests/intel/gem_lmem_swapping.c | 14 ++++++++++----
> >  2 files changed, 14 insertions(+), 8 deletions(-)
> > 
> > diff --git a/lib/igt_fb.c b/lib/igt_fb.c
> > index ab162a5b74..addff2cfba 100644
> > --- a/lib/igt_fb.c
> > +++ b/lib/igt_fb.c
> > @@ -3050,13 +3050,13 @@ static void blitcopy(const struct igt_fb *dst_fb,
> >  						   dst_fb->size);
> >  		} else if (ahnd && block_copy_ok(src_fb) && block_copy_ok(dst_fb)) {
> >  			for_each_ctx_engine(src_fb->fd, ictx, e) {
> > -				if (gem_engine_can_block_copy(src_fb->fd, e))
> > +				if (gem_engine_can_block_copy(src_fb->fd, e)) {
> > +					do_block_copy(src_fb, dst_fb, mem_region, i, ahnd,
> > +						      bb, bb_size, ictx, e);
> >  					break;
> > +				}
> >  			}
> >  			igt_assert_f(e, "No block copy capable engine found!\n");
> > -
> > -			do_block_copy(src_fb, dst_fb, mem_region, i, ahnd,
> > -				      bb, bb_size, ictx, e);
> >  		} else {
> >  			igt_blitter_src_copy(dst_fb->fd,
> >  					     ahnd, ctx, NULL,
> > diff --git a/tests/intel/gem_lmem_swapping.c b/tests/intel/gem_lmem_swapping.c
> > index b8f24742b8..b125261519 100644
> > --- a/tests/intel/gem_lmem_swapping.c
> > +++ b/tests/intel/gem_lmem_swapping.c
> > @@ -189,6 +189,7 @@ init_object_ccs(int i915, struct object *obj, struct blt_copy_object *tmp,
> >  {
> >  	struct blt_block_copy_data_ext ext = {}, *pext = &ext;
> >  	const struct intel_execution_engine2 *e;
> > +	struct intel_execution_engine2 ec;
> >  	struct blt_copy_data blt = {};
> >  	struct blt_copy_batch *cmd;
> >  	uint64_t size = 4096;
> > @@ -196,8 +197,10 @@ init_object_ccs(int i915, struct object *obj, struct blt_copy_object *tmp,
> >  
> >  	obj->seed = seed;
> >  	for_each_ctx_engine(i915, ctx, e) {
> > -		if (gem_engine_can_block_copy(i915, e))
> > +		if (gem_engine_can_block_copy(i915, e)) {
> > +			ec = *e;
> >  			break;
> > +		}
> >  	}
> >  	igt_assert_f(e, "Ctx don't have blt engine\n");
> >  
> > @@ -224,7 +227,7 @@ init_object_ccs(int i915, struct object *obj, struct blt_copy_object *tmp,
> >  	blt_set_object_ext(&ext.dst, 0, obj->blt_obj->x2, obj->blt_obj->y2,
> >  			   SURFACE_TYPE_2D);
> >  
> > -	blt_block_copy(i915, ctx, e, ahnd, &blt, pext);
> > +	blt_block_copy(i915, ctx, &ec, ahnd, &blt, pext);
> >  	free(cmd);
> >  }
> >  
> > @@ -255,14 +258,17 @@ verify_object_ccs(int i915, const struct object *obj,
> >  {
> >  	struct blt_block_copy_data_ext ext = {}, *pext = &ext;
> >  	const struct intel_execution_engine2 *e;
> > +	struct intel_execution_engine2 ec;
> >  	struct blt_copy_data blt = {};
> >  	struct blt_copy_batch *cmd;
> >  	uint64_t size = 4096;
> >  	unsigned long j, val, *buf;
> >  
> >  	for_each_ctx_engine(i915, ctx, e) {
> > -		if (gem_engine_can_block_copy(i915, e))
> > +		if (gem_engine_can_block_copy(i915, e)) {
> > +			ec = *e;
> >  			break;
> > +		}
> >  	}
> >  	igt_assert_f(e, "Ctx don't have blt engine\n");
> >  
> > @@ -284,7 +290,7 @@ verify_object_ccs(int i915, const struct object *obj,
> >  	blt_set_object_ext(&ext.src, 0, obj->blt_obj->x2, obj->blt_obj->y2,
> >  			   SURFACE_TYPE_2D);
> >  	blt_set_object_ext(&ext.dst, 0, tmp->x2, tmp->y2, SURFACE_TYPE_2D);
> > -	blt_block_copy(i915, ctx, e, ahnd, &blt, pext);
> > +	blt_block_copy(i915, ctx, &ec, ahnd, &blt, pext);
> >  
> >  	buf = gem_mmap__device_coherent(i915, tmp->handle, 0,
> >  					obj->size, PROT_READ);