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

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

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