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

[Peter Senna Tschudin <peter.senna@linux.intel.com>]

On 13.08.2024 15:56, Zbigniew Kempczyński wrote:
> 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?

Please do not fix a pothole just before I replace the road.

Nacked-by: Peter Senna Tschudin <peter.senna@linux.intel.com>

> 
> --
> 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@intel.com>
>>> Cc: Andi Shyti <andi.shyti@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);