Re: [PATCH] drm/gem: Fix a GEM leak in drm_gem_get_unmapped_area()

[Boris Brezillon <boris.brezillon@collabora.com>]

On Fri, 9 Jan 2026 10:42:21 +0100
Loïc Molinari <loic.molinari@collabora.com> wrote:

> Hi Boris,
> 
> On 08/01/2026 15:45, Boris Brezillon wrote:
> > On Thu, 8 Jan 2026 15:04:33 +0100
> > Loïc Molinari <loic.molinari@collabora.com> wrote:
> >   
> >> On 08/01/2026 14:43, Boris Brezillon wrote:  
> >>> On Thu, 8 Jan 2026 14:25:01 +0100
> >>> Loïc Molinari <loic.molinari@collabora.com> wrote:
> >>>      
> >>>> On 07/01/2026 12:12, Boris Brezillon wrote:  
> >>>>> On Tue,  6 Jan 2026 17:49:35 +0100
> >>>>> Boris Brezillon <boris.brezillon@collabora.com> wrote:
> >>>>>         
> >>>>>> drm_gem_object_lookup_at_offset() can return a valid object with
> >>>>>> filp or filp->f_op->get_unmapped_area set to NULL. Make sure we still
> >>>>>> release the ref we acquired on such objects.
> >>>>>>
> >>>>>> Cc: Loïc Molinari <loic.molinari@collabora.com>
> >>>>>> Fixes: 99bda20d6d4c ("drm/gem: Introduce drm_gem_get_unmapped_area() fop")
> >>>>>> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> >>>>>> ---
> >>>>>>     drivers/gpu/drm/drm_gem.c | 10 ++++++----
> >>>>>>     1 file changed, 6 insertions(+), 4 deletions(-)
> >>>>>>
> >>>>>> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> >>>>>> index 36c8af123877..f7cbf6e8d1e0 100644
> >>>>>> --- a/drivers/gpu/drm/drm_gem.c
> >>>>>> +++ b/drivers/gpu/drm/drm_gem.c
> >>>>>> @@ -1298,11 +1298,13 @@ unsigned long drm_gem_get_unmapped_area(struct file *filp, unsigned long uaddr,
> >>>>>>     	unsigned long ret;
> >>>>>>     
> >>>>>>     	obj = drm_gem_object_lookup_at_offset(filp, pgoff, len >> PAGE_SHIFT);
> >>>>>> -	if (IS_ERR(obj) || !obj->filp || !obj->filp->f_op->get_unmapped_area)
> >>>>>> -		return mm_get_unmapped_area(filp, uaddr, len, 0, flags);
> >>>>>> +	if (IS_ERR(obj))
> >>>>>> +		obj = NULL;
> >>>>>>     
> >>>>>> -	ret = obj->filp->f_op->get_unmapped_area(obj->filp, uaddr, len, 0,
> >>>>>> -						 flags);
> >>>>>> +	if (!obj || !obj->filp || !obj->filp->f_op->get_unmapped_area)
> >>>>>> +		ret = mm_get_unmapped_area(filp, uaddr, len, 0, flags);  
> >>>>>
> >>>>> Also, I'm wondering if we shouldn't pass pgoff instead of zero here to
> >>>>> have the exact same behavior that existed before
> >>>>> drm_gem_get_unmapped_area() was introduced.
> >>>>>         
> >>>>
> >>>> For mappings with a file (the DRM file in our case), if
> >>>> filp->f_op->get_unmapped_area isn't set then generic_get_unmapped_area()
> >>>> is used and it ignores the pgoff argument.  
> >>>
> >>> That's true for architectures that rely on the default implementation
> >>> (Arm64 for instance), but other architectures might have their own
> >>> implementation.
> >>>
> >>>      
> >>>> So it wasn't 0 before but was
> >>>> ignored anyway.  
> >>>
> >>> Didn't check all of them but the Arm implementation does take this
> >>> pgoff into account if I read the code correctly [1]. The fact this
> >>> argument is passed around makes me think other architectures might take
> >>> this into account too. I know this pgoff is just a fake offset into the
> >>> /dev/dri pseudo-file, but if we want to err on the safe side, we'd
> >>> rather do exactly what was done before drm_gem_get_unmapped_area() was
> >>> introduced for the ->filp==NULL case. That's just my 2 cts, of course.  
> >>
> >> You're right, I focused on arm64 here and there's obviously other archs
> >> so we'd better pass pgoff for the fallback case.  
> > 
> > Do you want me to send a patch doing that, or should I take care of it?

Was meant to be "should I let you take care of it?", but you got it
right :D.

> 
> I'll take care of it. I'd better wait for that GEM leak fix to be pulled 
> first to avoid a conflict, right?

Yep. I'll queue that one to drm-misc-next today.