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

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

[ZhaoJinming]

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.

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.

Signed-off-by: ZhaoJinming <zhaojinming@uniontech.com>
---
 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

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

[Krzysztof Karas]

Hi ZhaoJinming,

On 2026-07-28 at 13:41:52 +0800, ZhaoJinming wrote:
> 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.
> 
> 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.
Huh, so there was no cleanup on failure of
____i915_gem_object_get_pages, but the page_ref still got
decremented. I think the changes below should be enough to
resolve that issue, but we still need to run this through CI.

> 
> Signed-off-by: ZhaoJinming <zhaojinming@uniontech.com>
> ---
>  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
> 

-- 
Best Regards,
Krzysztof

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

[Joonas Lahtinen]

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 <zhaojinming@uniontech.com>
> ---
>  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
>

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

[Maarten Lankhorst]

Hey,

On 8/5/26 10:50, Joonas Lahtinen wrote:
> 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 <zhaojinming@uniontech.com>
>> ---
>>  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
>>

I completely and entirely forgot everything related to i915 userpointer code, so I'm not much of help here.

It seems that i915_gem_userptr_get_pages() checks if page-ref is 0, then increases the page ref on success, which we then use to decrease our original page ref.

The patch by ZhaoJinming removes correct handling of the case where page_ref is already >= 0

Might be better to do change it to

	if (!obj->userptr.page_ref) {
		obj->userptr.pvec = pvec;
		obj->userptr.notifier_seq = notifier_seq;
		pvec = NULL;
		ret = ____i915_gem_object_get_pages(obj);
	}

and in i915_gem_userptr_get_pages, look for !userptr.pvec instead of !obj->userptr.page_ref

Kind regards,
~Maarten Lankhorst