Re: [PATCH v2] drm/i915/userptr: Fix user pages leak on get_pages failure

Adding Maarten and Matt for below discussion points.

Quoting ZhaoJinming (2026-07-28 08:41:52)
> When ____i915_gem_object_get_pages() fails inside
> i915_gem_object_userptr_submit_init(), the pvec containing pages
> pinned by pin_user_pages_fast() was never freed:
> 
> 1. obj->userptr.pvec was set to the pinned pages
> 2. The local pvec variable was NULLed
> 3. ____i915_gem_object_get_pages() failed, but its internal
>    i915_gem_object_userptr_drop_ref() only decremented page_ref
>    from 2 to 1, not triggering the pvec cleanup
> 4. The unconditional obj->userptr.page_ref-- brought page_ref
>    to 0, but the pinned pages remained referenced only by
>    obj->userptr.pvec with no path to reclaim them
> 
> Additionally, the cache hit path could return success on a
> subsequent call despite page_ref being 0, leading to a
> GEM_BUG_ON(obj->userptr.page_ref < 0) crash in drop_ref when
> the pages were eventually invalidated.

That's a very verbose way of saying that error handling is missing
for ____i915_gem_object_get_pages failures here.

> Fix by calling i915_gem_object_userptr_drop_ref() on the
> get_pages failure path, which properly decrements page_ref
> from 1 to 0, triggering the pvec cleanup. Make the page_ref--
> after the if block conditional on success, since drop_ref
> already handles the refcount on failure.

Not sure that is the right fix, seems that i915_gem_userptr_get_pages
is rigged in a way that in the intro page_ref >= 1 is required, and is
unconditionally increased (becomes >= 2), then on error path drop_ref is
called which will unconditionally decrease but never free because page_ref
always remains at least 1. I think that is the real bug which is being papered
over here. Maarten, am I missing something?

This seems to be introduced by commit:

	ed29c2691188 ("drm/i915: Fix userptr so we do not have to worry about obj->mm.lock, v7.")

As for the original report, lack of error handling seems to be introduced
by commit:

	b4b9731b02c3 ("drm/i915: Simplify userptr locking")

The commit message goes on to explain it a little bit, but this chunk from
original patch still seems quite odd?

-------------------------- 8< --------------------------  
@@ -341,12 +317,14 @@ int i915_gem_object_userptr_submit_init(struct drm_i915_gem_object *obj)
        if (!obj->userptr.page_ref++) {
                obj->userptr.pvec = pvec;
                obj->userptr.notifier_seq = notifier_seq;
-
                pvec = NULL;
+               ret = ____i915_gem_object_get_pages(obj);
        }
 
+       obj->userptr.page_ref--;
+
 out_unlock:
-       spin_unlock(&i915->mm.notifier_lock);
+       i915_gem_object_unlock(obj);
 
 out:
        if (pvec) {
-------------------------- 8< --------------------------  

Regards, Joonas

> 
> Signed-off-by: ZhaoJinming <[email protected]>
> ---
>  drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> index 043095f93ac6..7d2750528485 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> @@ -292,9 +292,12 @@ int i915_gem_object_userptr_submit_init(struct drm_i915_gem_object *obj)
>                 obj->userptr.notifier_seq = notifier_seq;
>                 pvec = NULL;
>                 ret = ____i915_gem_object_get_pages(obj);
> +               if (ret)
> +                       i915_gem_object_userptr_drop_ref(obj);
>         }
>  
> -       obj->userptr.page_ref--;
> +       if (!ret)
> +               obj->userptr.page_ref--;
>  
>  out_unlock:
>         i915_gem_object_unlock(obj);
> -- 
> 2.20.1
>