[PATCH v4] drm/i915/gem: Drop check for changed VM in EXECBUF

[PATCH v4] drm/i915/gem: Drop check for changed VM in EXECBUF

[Joonas Lahtinen]

Since the introduction of d4433c7600f7 ("drm/i915/gem: Use the proto-context
to handle create parameters (v5)") it has not been possible for VM to change
after context creation so the check will never fail.

Sima's analysis:

  This check was added in f7ce8639f6ff ("drm/i915/gem: Split the context's
  obj:vma lut into its own mutex") but without any hint in the commit
  message as to why. In another hunk of that commit there's a hint though in
  __eb_add_lut:

          /* user racing with ctx set-vm */

  This would mean that this bug was introduced in e0695db7298e ("drm/i915:
  Create/destroy VM (ppGTT) for use with contexts"), which allowed to change
  the gem_ctx->vm at runtime, opening up the race that was partially fixed
  in the earlier referenced commit about a year later.

  But it cannot be exploited anymore in anything remotely recent because
  with the introduction of proto-contexts we've made gem_ctx->vm invariant
  again, exactly to preemptively close all these potential issues.
  Specifically d4433c7600f7 ("drm/i915/gem: Use the proto-context to handle
  create parameters (v5)") is the vm specific part of the proto-context
  work.

v3:
- Include Sima's analysis and WARN_ON_ONCE

v4:
- Focus only on latest mainline codebase

References: https://lore.kernel.org/all/20260324151741.29338-1-sosohero200@gmail.com/
Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Linus Torvalds <torvalds@linuxfoundation.org>
Cc: Simona Vetter <simona.vetter@ffwll.ch>
Cc: Tvrtko Ursulin <tursulin@ursulin.net>
Cc: Andi Shyti <andi.shyti@kernel.org>
Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
index bd608cea396f..621f07a55c38 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
@@ -895,7 +895,7 @@ static struct i915_vma *eb_lookup_vma(struct i915_execbuffer *eb, u32 handle)
 
 		rcu_read_lock();
 		vma = radix_tree_lookup(&eb->gem_context->handles_vma, handle);
-		if (likely(vma && vma->vm == vm))
+		if (likely(vma))
 			vma = i915_vma_tryget(vma);
 		rcu_read_unlock();
 		if (likely(vma))
-- 
2.53.0

Re: [PATCH v4] drm/i915/gem: Drop check for changed VM in EXECBUF

[Tvrtko Ursulin]

On 09/04/2026 06:31, Joonas Lahtinen wrote:
> Since the introduction of d4433c7600f7 ("drm/i915/gem: Use the proto-context
> to handle create parameters (v5)") it has not been possible for VM to change
> after context creation so the check will never fail.
> 
> Sima's analysis:
> 
>    This check was added in f7ce8639f6ff ("drm/i915/gem: Split the context's
>    obj:vma lut into its own mutex") but without any hint in the commit
>    message as to why. In another hunk of that commit there's a hint though in
>    __eb_add_lut:
> 
>            /* user racing with ctx set-vm */
> 
>    This would mean that this bug was introduced in e0695db7298e ("drm/i915:
>    Create/destroy VM (ppGTT) for use with contexts"), which allowed to change
>    the gem_ctx->vm at runtime, opening up the race that was partially fixed
>    in the earlier referenced commit about a year later.
> 
>    But it cannot be exploited anymore in anything remotely recent because
>    with the introduction of proto-contexts we've made gem_ctx->vm invariant
>    again, exactly to preemptively close all these potential issues.
>    Specifically d4433c7600f7 ("drm/i915/gem: Use the proto-context to handle
>    create parameters (v5)") is the vm specific part of the proto-context
>    work.
> 
> v3:
> - Include Sima's analysis and WARN_ON_ONCE
> 
> v4:
> - Focus only on latest mainline codebase
> 
> References: https://lore.kernel.org/all/20260324151741.29338-1-sosohero200@gmail.com/
> Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Linus Torvalds <torvalds@linuxfoundation.org>
> Cc: Simona Vetter <simona.vetter@ffwll.ch>
> Cc: Tvrtko Ursulin <tursulin@ursulin.net>
> Cc: Andi Shyti <andi.shyti@kernel.org>
> Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> index bd608cea396f..621f07a55c38 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> @@ -895,7 +895,7 @@ static struct i915_vma *eb_lookup_vma(struct i915_execbuffer *eb, u32 handle)
>   
>   		rcu_read_lock();
>   		vma = radix_tree_lookup(&eb->gem_context->handles_vma, handle);
> -		if (likely(vma && vma->vm == vm))
> +		if (likely(vma))
>   			vma = i915_vma_tryget(vma);
>   		rcu_read_unlock();
>   		if (likely(vma))

Analysis reads correct to me.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>

Regards,

Tvrtko

Claude review: drm/i915/gem: Drop check for changed VM in EXECBUF

[Claude Code Review Bot]

Overall Series Review

Subject: drm/i915/gem: Drop check for changed VM in EXECBUF
Author: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Patches: 2
Reviewed: 2026-04-12T11:46:34.835044

---

This is a single, well-motivated cleanup patch that removes a dead VM-equality check in the execbuffer VMA lookup path. The commit message and Sima's included analysis clearly trace the history: the check was originally needed when `gem_ctx->vm` could change at runtime (introduced in e0695db7298e), but since the proto-context rework (d4433c7600f7), `ctx->vm` is set once at context creation and never modified afterward. The VMA entries in `handles_vma` are always created via `i915_vma_instance(obj, vm, NULL)` using the same `vm`, so the `vma->vm == vm` check is indeed tautologically true.

The change is correct and trivially safe. It removes one comparison per cached VMA lookup on the fast path of execbuffer, which is a minor but nice cleanup.

One observation: the v3 changelog mentions adding a `WARN_ON_ONCE`, but the v4 patch does not include one. The v4 changelog says "Focus only on latest mainline codebase," suggesting the WARN was intentionally dropped. This is worth noting — a `WARN_ON_ONCE(vma && vma->vm != vm)` would have provided runtime validation of the invariant while keeping the fast path essentially the same. However, if the maintainers are confident the invariant is well-established (and it has been since 2022), omitting the WARN to keep the code clean is a reasonable choice.

**Verdict:** The patch looks correct and ready to merge.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/i915/gem: Drop check for changed VM in EXECBUF

[Claude Code Review Bot]

Patch Review

**Correctness: Good.** I verified in the current tree:

- `ctx->vm` is only assigned during context creation (`i915_gem_context.c:1642`) and in mock test code. There is no runtime setter — the proto-context design made it immutable.
- All VMAs inserted into `handles_vma` are created via `i915_vma_instance(obj, vm, NULL)` at `i915_gem_execbuffer.c:927`, using the same `vm` that is checked. The radix tree is per-context, and the VM is per-context, so any VMA found in the tree will always have `vma->vm == vm`.

**The diff:**

```c
-		if (likely(vma && vma->vm == vm))
+		if (likely(vma))
 			vma = i915_vma_tryget(vma);
```

This correctly preserves the NULL check (the `radix_tree_lookup` can return NULL) while removing the redundant VM comparison. The `else vma = NULL` branch on line 901-902 is now only reached when `vma` is already NULL from the lookup, which is fine — the code still sets `vma = NULL` for the `else` case and falls through to the slow path.

**Minor observations:**

1. The `else vma = NULL` clause (lines 901-902, not shown in diff context but present in the tree) is now technically redundant since the only way to reach it is when `vma` is already NULL. It's harmless and keeps the code readable, but a pedantic follow-up could simplify the entire block to just `vma = radix_tree_lookup(...); if (vma) vma = i915_vma_tryget(vma);`. Not worth blocking on.

2. The local variable `struct i915_address_space *vm = eb->context->vm;` on line 890 becomes unused by this patch. The compiler should warn about this. However, `vm` is still used at line 927 (`i915_vma_instance(obj, vm, NULL)`), so it remains needed. No issue here.

3. As noted in the overall review, the v3 mentioned a `WARN_ON_ONCE` that was dropped in v4. If reviewers want belt-and-suspenders validation, something like `WARN_ON_ONCE(vma && vma->vm != vm)` before the `i915_vma_tryget` would catch any future regression where a VMA ends up in the wrong context's radix tree. But this is a stylistic/policy choice, not a correctness concern.

**Commit message quality:** Excellent. The historical analysis is thorough, references are provided, and the reasoning is clear.

---
Generated by Claude Code Patch Reviewer