Re: [Intel-gfx] [PATCH] drm/i915/guc: Fix potential null pointer deref in GuC 'steal id' test

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

On Mon, Aug 07, 2023 at 12:46:46PM -0700, John Harrison wrote:
> On 8/3/2023 06:28, Andi Shyti wrote:
> > Hi John,
> > 
> > On Wed, Aug 02, 2023 at 11:49:40AM -0700, John.C.Harrison@Intel.com wrote:
> > > From: John Harrison <John.C.Harrison@Intel.com>
> > > 
> > > It was noticed that if the very first 'stealing' request failed to
> > > create for some reason then the 'steal all ids' loop would immediately
> > > exit with 'last' still being NULL. The test would attempt to continue
> > > but using a null pointer. Fix that by aborting the test if it fails to
> > > create any requests at all.
> > > 
> > > Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
> > > ---
> > >   drivers/gpu/drm/i915/gt/uc/selftest_guc.c | 6 +++---
> > >   1 file changed, 3 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/gt/uc/selftest_guc.c b/drivers/gpu/drm/i915/gt/uc/selftest_guc.c
> > > index 1fd760539f77b..bfb72143566f6 100644
> > > --- a/drivers/gpu/drm/i915/gt/uc/selftest_guc.c
> > > +++ b/drivers/gpu/drm/i915/gt/uc/selftest_guc.c
> > > @@ -204,9 +204,9 @@ static int intel_guc_steal_guc_ids(void *arg)
> > >   		if (IS_ERR(rq)) {
> > >   			ret = PTR_ERR(rq);
> > >   			rq = NULL;
> > > -			if (ret != -EAGAIN) {
> > > -				guc_err(guc, "Failed to create request %d: %pe\n",
> > > -					context_index, ERR_PTR(ret));
> > > +			if ((ret != -EAGAIN) || !last) {
> > isn't last alway NULL here?
> > 
> > Andi
> No, only on the first pass around the loop. When a request is successfully
> created, the else clause below assigns last to that new request. So if the
> failure to create only happens on pass 2 or later, last will be non-null.
> Which is the whole point of the code. It keeps creating all the
> contexts/requests that it can until it runs out of resources and gets an
> EAGAIN failure. At which point, last will be pointing to the last successful
> creation and the test continues to the next part of actually stealing an id.
> 
> But if the EAGAIN failure happens on the first pass then last will be null
> and it is not safe/valid to proceed so it needs to abort. And if anything
> other than EAGAIN is returned then something has gone wrong and it doesn't
> matter what last is set to, it needs to abort regardless.

Right! Thanks!

Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com> 

Andi