[PATCH] drm/i915: Return NULL on error in active_instance

[PATCH] drm/i915: Return NULL on error in active_instance

[Joonas Lahtinen]

Avoid returning &node->base when node is NULL due to OOM
during GFP_ATOMIC allocation.

Discovered using AI-assisted static analysis confirmed by
Intel Product Security.

Reported-by: Martin Hodo <martin.hodo@intel.com>
Fixes: bfaae47db3c0 ("drm/i915: make lockdep slightly happier about execbuf.")
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
Cc: Simona Vetter <simona.vetter@ffwll.ch>
Cc: <stable@vger.kernel.org> # v5.13+
Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_active.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_active.c b/drivers/gpu/drm/i915/i915_active.c
index 5cb7a72774a0..aa77def0bc0d 100644
--- a/drivers/gpu/drm/i915/i915_active.c
+++ b/drivers/gpu/drm/i915/i915_active.c
@@ -318,7 +318,7 @@ active_instance(struct i915_active *ref, u64 idx)
 	 */
 	node = kmem_cache_alloc(slab_cache, GFP_ATOMIC);
 	if (!node)
-		goto out;
+		goto err;
 
 	__i915_active_fence_init(&node->base, NULL, node_retire);
 	node->ref = ref;
@@ -332,6 +332,11 @@ active_instance(struct i915_active *ref, u64 idx)
 	spin_unlock_irq(&ref->tree_lock);
 
 	return &node->base;
+
+err:
+	spin_unlock_irq(&ref->tree_lock);
+
+	return NULL;
 }
 
 void __i915_active_init(struct i915_active *ref,
-- 
2.54.0

Re: [PATCH] drm/i915: Return NULL on error in active_instance

[Sebastian Brzezinka]

Hi Joonas,

On Wed Jun 24, 2026 at 11:09 AM CEST, Joonas Lahtinen wrote:
> Avoid returning &node->base when node is NULL due to OOM
> during GFP_ATOMIC allocation.
>
> Discovered using AI-assisted static analysis confirmed by
> Intel Product Security.
>
> Reported-by: Martin Hodo <martin.hodo@intel.com>
> Fixes: bfaae47db3c0 ("drm/i915: make lockdep slightly happier about execbuf.")
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> Cc: Simona Vetter <simona.vetter@ffwll.ch>
> Cc: <stable@vger.kernel.org> # v5.13+
> Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
Looks good.
Reviewed-by: Sebastian Brzezinka <sebastian.brzezinka@intel.com>

-- 
Best regards,
Sebastian

Re: [PATCH] drm/i915: Return NULL on error in active_instance

[Sebastian Brzezinka]

Hi,
On Wed Jun 24, 2026 at 11:09 AM CEST, Joonas Lahtinen wrote:
> Avoid returning &node->base when node is NULL due to OOM
> during GFP_ATOMIC allocation.
>
> Discovered using AI-assisted static analysis confirmed by
> Intel Product Security.
>
> Reported-by: Martin Hodo <martin.hodo@intel.com>
> Fixes: bfaae47db3c0 ("drm/i915: make lockdep slightly happier about execbuf.")
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> Cc: Simona Vetter <simona.vetter@ffwll.ch>
> Cc: <stable@vger.kernel.org> # v5.13+
> Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_active.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_active.c b/drivers/gpu/drm/i915/i915_active.c
> index 5cb7a72774a0..aa77def0bc0d 100644
> --- a/drivers/gpu/drm/i915/i915_active.c
> +++ b/drivers/gpu/drm/i915/i915_active.c
> @@ -318,7 +318,7 @@ active_instance(struct i915_active *ref, u64 idx)
>  	 */
>  	node = kmem_cache_alloc(slab_cache, GFP_ATOMIC);
>  	if (!node)
> -		goto out;
> +		goto err;
just a nit: this jump is not neccesery, you could return early.

-- 
Best regards,
Sebastian

Re: [PATCH] drm/i915: Return NULL on error in active_instance

[Maarten Lankhorst]

Hi,

On 6/24/26 11:09, Joonas Lahtinen wrote:
> Avoid returning &node->base when node is NULL due to OOM
> during GFP_ATOMIC allocation.
> 
> Discovered using AI-assisted static analysis confirmed by
> Intel Product Security.
> 
> Reported-by: Martin Hodo <martin.hodo@intel.com>
> Fixes: bfaae47db3c0 ("drm/i915: make lockdep slightly happier about execbuf.")
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> Cc: Simona Vetter <simona.vetter@ffwll.ch>
> Cc: <stable@vger.kernel.org> # v5.13+
> Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

>  drivers/gpu/drm/i915/i915_active.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_active.c b/drivers/gpu/drm/i915/i915_active.c
> index 5cb7a72774a0..aa77def0bc0d 100644
> --- a/drivers/gpu/drm/i915/i915_active.c
> +++ b/drivers/gpu/drm/i915/i915_active.c
> @@ -318,7 +318,7 @@ active_instance(struct i915_active *ref, u64 idx)
>  	 */
>  	node = kmem_cache_alloc(slab_cache, GFP_ATOMIC);
>  	if (!node)
> -		goto out;
> +		goto err;
>  
>  	__i915_active_fence_init(&node->base, NULL, node_retire);
>  	node->ref = ref;
> @@ -332,6 +332,11 @@ active_instance(struct i915_active *ref, u64 idx)
>  	spin_unlock_irq(&ref->tree_lock);
>  
>  	return &node->base;
> +
> +err:
> +	spin_unlock_irq(&ref->tree_lock);
> +
> +	return NULL;
>  }
>  
>  void __i915_active_init(struct i915_active *ref,

Re: [PATCH] drm/i915: Return NULL on error in active_instance

[Joonas Lahtinen]

Quoting Sebastian Brzezinka (2026-06-24 12:27:07)
> Hi,
> On Wed Jun 24, 2026 at 11:09 AM CEST, Joonas Lahtinen wrote:
> > Avoid returning &node->base when node is NULL due to OOM
> > during GFP_ATOMIC allocation.
> >
> > Discovered using AI-assisted static analysis confirmed by
> > Intel Product Security.

<SNIP>

> > +++ b/drivers/gpu/drm/i915/i915_active.c
> > @@ -318,7 +318,7 @@ active_instance(struct i915_active *ref, u64 idx)
> >        */
> >       node = kmem_cache_alloc(slab_cache, GFP_ATOMIC);
> >       if (!node)
> > -             goto out;
> > +             goto err;
> just a nit: this jump is not neccesery, you could return early.

We specifically want to embrace the onion error handling idiom with goto
rather than doing the spinlock release inline here.

Preferred error handling should look more like:

	if (!try_lock(lock))
		goto err;

	mem = alloc();
	if (!mem)
		goto err_lock;

	if (!bla_bla(mem, bar))
		goto err_mem;

	...

err_mem:
	free(mem);
err_lock:
	unlock(lock);
err:
	return ret;

Rather than:

	if (!try_lock(lock))
		return ret;

	mem = alloc();
	if (!mem)
	{
		unlock(lock);
		return ret;
	}

	if (!bla_bla(mem, bar))
	{
		free(mem);
		unlock(lock);
		return ret;
	}

	...

Regards, Joonas

Re: [PATCH] drm/i915: Return NULL on error in active_instance

[Sebastian Brzezinka]

On Wed Jun 24, 2026 at 12:24 PM CEST, Joonas Lahtinen wrote:
> Quoting Sebastian Brzezinka (2026-06-24 12:27:07)
>> Hi,
>> On Wed Jun 24, 2026 at 11:09 AM CEST, Joonas Lahtinen wrote:
>> > Avoid returning &node->base when node is NULL due to OOM
>> > during GFP_ATOMIC allocation.
>> >
>> > Discovered using AI-assisted static analysis confirmed by
>> > Intel Product Security.
>
> <SNIP>
>
>> > +++ b/drivers/gpu/drm/i915/i915_active.c
>> > @@ -318,7 +318,7 @@ active_instance(struct i915_active *ref, u64 idx)
>> >        */
>> >       node = kmem_cache_alloc(slab_cache, GFP_ATOMIC);
>> >       if (!node)
>> > -             goto out;
>> > +             goto err;
>> just a nit: this jump is not neccesery, you could return early.
>
> We specifically want to embrace the onion error handling idiom with goto
> rather than doing the spinlock release inline here.
>
> Preferred error handling should look more like:
>
> 	if (!try_lock(lock))
> 		goto err;
>
> 	mem = alloc();
> 	if (!mem)
> 		goto err_lock;
>
> 	if (!bla_bla(mem, bar))
> 		goto err_mem;
>
> 	...
>
> err_mem:
> 	free(mem);
> err_lock:
> 	unlock(lock);
> err:
> 	return ret;
>
> Rather than:
>
> 	if (!try_lock(lock))
> 		return ret;
>
> 	mem = alloc();
> 	if (!mem)
> 	{
> 		unlock(lock);
> 		return ret;
> 	}
>
> 	if (!bla_bla(mem, bar))
> 	{
> 		free(mem);
> 		unlock(lock);
> 		return ret;
> 	}
>
> 	...
>
> Regards, Joonas
Good to know, thanks for the explanation.

-- 
Best regards,
Sebastian

Re: [PATCH] drm/i915: Return NULL on error in active_instance

[Joonas Lahtinen]

Pushed to drm-intel-gt-next, thanks for the reviews.

Regards, Joonas

Re: [PATCH] drm/i915: Return NULL on error in active_instance

[Andi Shyti]

Hi Joonas,

On Wed, Jun 24, 2026 at 02:25:19PM +0300, Joonas Lahtinen wrote:
> Pushed to drm-intel-gt-next, thanks for the reviews.

please, next time:

- Give people more time to review the patch. Only two hours
  passed between posting it and pushing it (during lunch time,
  BTW).

- There were BAT failures. They were unrelated, but so far we
  have generally held back patches until BAT was green, even for
  the most obvious changes.

Thanks,
Andi

Re: [PATCH] drm/i915: Return NULL on error in active_instance

[Joonas Lahtinen]

Quoting Andi Shyti (2026-06-24 15:59:30)
> Hi Joonas,
> 
> On Wed, Jun 24, 2026 at 02:25:19PM +0300, Joonas Lahtinen wrote:
> > Pushed to drm-intel-gt-next, thanks for the reviews.
> 
> please, next time:
> 
> - Give people more time to review the patch. Only two hours
>   passed between posting it and pushing it

And why exactly is that a problem? I got the review from the original
patch author and a yet another person on top while it's a very
uncontroversial and trivial patch. Two reviewers per patch is already
quite a high bar to clear if you look at git history.

> (during lunch time, BTW).

Sorry, I did not know there is a universally agreed 2 hour lunch window
in UTC timezone that I should follow. I've missed that memo.

> - There were BAT failures. They were unrelated, but so far we
>   have generally held back patches until BAT was green, even for
>   the most obvious changes.

Strong disagree here. That'd have caused the patch to miss -next-fixes
PR just due to random noise of CI.

If there was a reasonable doubt about the impact of the patch on the
failure, that'd of course be different, but here there was absolutely
none in this case.

As per patchwork automated mail reply:

> If you think the reported changes have nothing to do with the changes
> introduced in Patchwork_169089v1, please notify your bug team

That's exactly what was done here. That's a fair ask, but asking for
maintainers not to merge any code because of false positives is simply
not.

Regards, Joonas

> 
> Thanks,
> Andi

Re: [PATCH] drm/i915: Return NULL on error in active_instanceg

[Andi Shyti]

Hi Joonas,

On Thu, Jun 25, 2026 at 09:03:32AM +0300, Joonas Lahtinen wrote:
> Quoting Andi Shyti (2026-06-24 15:59:30)
> > On Wed, Jun 24, 2026 at 02:25:19PM +0300, Joonas Lahtinen wrote:
> > > Pushed to drm-intel-gt-next, thanks for the reviews.
> > 
> > please, next time:
> > 
> > - Give people more time to review the patch. Only two hours
> >   passed between posting it and pushing it
> 
> And why exactly is that a problem? I got the review from the original
> patch author and a yet another person on top while it's a very
> uncontroversial and trivial patch. Two reviewers per patch is already
> quite a high bar to clear if you look at git history.
> 
> > (during lunch time, BTW).
> 
> Sorry, I did not know there is a universally agreed 2 hour lunch window
> in UTC timezone that I should follow. I've missed that memo.

It's not a matter of how many reviewers there are or how
controversial a patch may be. A patch needs to stay on the list
long enough for the whole community to have a chance to review
it. Give it one or two days.

Even if you and the other reviewers don't see anything wrong,
someone else might. As submitters and maintainers, we need to
give people a reasonable amount of time to look at every patch.
Two hours, at any time of the day, are definitely not enough.

We often complain when people send new revisions too early for
exactly the same reason.

> > - There were BAT failures. They were unrelated, but so far we
> >   have generally held back patches until BAT was green, even for
> >   the most obvious changes.
> 
> Strong disagree here. That'd have caused the patch to miss -next-fixes
> PR just due to random noise of CI.

We shouldn't care. The rules are the same for everyone, including
maintainers. Very often I've been told to wait until the next
cycle and very often I've asked others to do the same.

Besides, it's unfair to keep a patch on the list for only two
hours just to avoid missing the current cycle, while everyone
else is expected to wait.

> If there was a reasonable doubt about the impact of the patch on the
> failure, that'd of course be different, but here there was absolutely
> none in this case.
> 
> As per patchwork automated mail reply:
> 
> > If you think the reported changes have nothing to do with the changes
> > introduced in Patchwork_169089v1, please notify your bug team

The bug team is a different matter. Here we're talking about the
review process.

> That's exactly what was done here. That's a fair ask, but asking for
> maintainers not to merge any code because of false positives is simply
> not.

False positive or not, controversial or not, easy or difficult,
patches have *always* been blocked when BAT was red. The shard
tests don't even start if BAT is red.

Otherwise, we might as well stop running automatic tests for
patches considered "non-controversial" and save CI resources.

Andi

> Regards, Joonas
> 
> > 
> > Thanks,
> > Andi

Re: [PATCH] drm/i915: Return NULL on error in active_instanceg

[Joonas Lahtinen]

Quoting Andi Shyti (2026-06-25 11:50:50)
> On Thu, Jun 25, 2026 at 09:03:32AM +0300, Joonas Lahtinen wrote:

<SNIP>

> The bug team is a different matter. Here we're talking about the
> review process.

What comes to the review process, you're entitled to express your
opinion how things should be.

If you do a rundown of average time spent on mailing list after R-b
before merge split per subsystem / driver and show some stats for
patches that are less than 10 lines, and we can maybe talk further.

Don't see the point otherwise. You do you, and me do me.

> > That's exactly what was done here. That's a fair ask, but asking for
> > maintainers not to merge any code because of false positives is simply
> > not.
> 
> False positive or not, controversial or not, easy or difficult,
> patches have *always* been blocked when BAT was red.

There's a very clear difference between random noise in BAT and something
being wrong in BAT due to the patch. The whole reason for BAT to be established
was to have a fast litmus test if the patch would break the world.

Original motivation was to avoid spinning up the more expensive shard
runs if the litmus test doesn't pass. And it was recognized back then
that we absolutely can't have false positives in BAT for things to work
out reasonably.

> Otherwise, we might as well stop running automatic tests for
> patches considered "non-controversial" and save CI resources.

Now that you bring it up, that's exactly what we should be targeting.

There is zero point in forcibly attaching test results to an individual
patch if that patch has exactly zero coverage in the test set.

So for example here, if we would have some nice automated code coverage
analysis that would have declared our tests don't have any coverage for
the patch in question and skipped shards, that would have absolutely
made sense (always allowing user to force the run, still).

The shard test run for this specific patch offers no other value than
an completely disconnected idle run of the test set. A completely
disconnected idle run would probably be even more valuable as it
wouldn't connect false positives to unrelated patch.

But I don't think we're there yet. So in the meanwhile let's try to
live with limitations of reality and fix some bugs.

Regards, Joonas