Re: [PATCH] drm/i915/gt: Protect against overflow in active_engine()

[Andi Shyti <andi.shyti@linux.intel.com>]

Hi,

On Mon, Aug 04, 2025 at 07:57:08AM +0000, Krzysztof Karas wrote:
> It is unlikely, but possible for the first call to
> intel_context_create() to fail with -ENOMEM, which would result
> in entering the following code block and decrementing "count",
> when it is set to 0 (initial condition in the for loop).
> 
> Protect from overflowing the variable with additional count > 0
> check.
> 
> Signed-off-by: Krzysztof Karas <krzysztof.karas@intel.com>
> ---
>  drivers/gpu/drm/i915/gt/selftest_hangcheck.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/selftest_hangcheck.c b/drivers/gpu/drm/i915/gt/selftest_hangcheck.c
> index f057c16410e7..cc0798dd30d5 100644
> --- a/drivers/gpu/drm/i915/gt/selftest_hangcheck.c
> +++ b/drivers/gpu/drm/i915/gt/selftest_hangcheck.c
> @@ -904,8 +904,10 @@ static void active_engine(struct kthread_work *work)
>  			arg->result = PTR_ERR(ce[count]);
>  			pr_err("[%s] Create context #%ld failed: %d!\n",
>  			       engine->name, count, arg->result);
> -			while (--count)

Off topic:

This is one of the reasons why counters should always be signed,
I've been always fighting with people saying that "this is
impossible to be negative". It's called robust programming.

if (count == 0) is one case out of all possible values of count.
if (count >= 0) is covering half of the possible values rendering
                the check more robust, even if we swear that
		count will never be negative (we can also have
		cases of memory corruption).

Anyway...

> -				intel_context_put(ce[count]);
> +			if (likely(count > 0)) {

... no need for likely, if we are here, we are already in an
unlikely situation.

> +				while (--count)
> +					intel_context_put(ce[count]);
> +			}


How about using a do ... while()?

Andi

>  			return;
>  		}
>  	}
> -- 
> 2.34.1
> 
> -- 
> Best Regards,
> Krzysztof