* Re: [PATCH] drm/gem: Fix a GEM leak in drm_gem_get_unmapped_area()
2026-01-06 16:49 [PATCH] drm/gem: Fix a GEM leak in drm_gem_get_unmapped_area() Boris Brezillon
@ 2026-01-07 11:12 ` Boris Brezillon
2026-01-08 13:25 ` Loïc Molinari
2026-01-08 13:18 ` Loïc Molinari
2026-01-09 12:02 ` Boris Brezillon
2 siblings, 1 reply; 12+ messages in thread
From: Boris Brezillon @ 2026-01-07 11:12 UTC (permalink / raw)
To: dri-devel, David Airlie, Simona Vetter, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann
Cc: kernel, Loïc Molinari
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.
> + else
> + ret = obj->filp->f_op->get_unmapped_area(obj->filp, uaddr, len, 0, flags);
>
> drm_gem_object_put(obj);
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] drm/gem: Fix a GEM leak in drm_gem_get_unmapped_area()
2026-01-07 11:12 ` Boris Brezillon
@ 2026-01-08 13:25 ` Loïc Molinari
2026-01-08 13:43 ` Boris Brezillon
0 siblings, 1 reply; 12+ messages in thread
From: Loïc Molinari @ 2026-01-08 13:25 UTC (permalink / raw)
To: Boris Brezillon, dri-devel, David Airlie, Simona Vetter,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann
Cc: kernel
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. So it wasn't 0 before but was
ignored anyway. I don't mind passing pgoff but I find it simpler like that.
Regards,
Loïc
>> + else
>> + ret = obj->filp->f_op->get_unmapped_area(obj->filp, uaddr, len, 0, flags);
>>
>> drm_gem_object_put(obj);
>>
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] drm/gem: Fix a GEM leak in drm_gem_get_unmapped_area()
2026-01-08 13:25 ` Loïc Molinari
@ 2026-01-08 13:43 ` Boris Brezillon
2026-01-08 14:04 ` Loïc Molinari
0 siblings, 1 reply; 12+ messages in thread
From: Boris Brezillon @ 2026-01-08 13:43 UTC (permalink / raw)
To: Loïc Molinari
Cc: dri-devel, David Airlie, Simona Vetter, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, kernel
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.
Regards,
Boris
[1]https://elixir.bootlin.com/linux/v6.18.3/source/arch/arm/mm/mmap.c#L30
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] drm/gem: Fix a GEM leak in drm_gem_get_unmapped_area()
2026-01-08 13:43 ` Boris Brezillon
@ 2026-01-08 14:04 ` Loïc Molinari
2026-01-08 14:45 ` Boris Brezillon
0 siblings, 1 reply; 12+ messages in thread
From: Loïc Molinari @ 2026-01-08 14:04 UTC (permalink / raw)
To: Boris Brezillon
Cc: dri-devel, David Airlie, Simona Vetter, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, kernel
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.
> Regards,
>
> Boris
>
> [1]https://elixir.bootlin.com/linux/v6.18.3/source/arch/arm/mm/mmap.c#L30
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] drm/gem: Fix a GEM leak in drm_gem_get_unmapped_area()
2026-01-08 14:04 ` Loïc Molinari
@ 2026-01-08 14:45 ` Boris Brezillon
2026-01-09 9:42 ` Loïc Molinari
0 siblings, 1 reply; 12+ messages in thread
From: Boris Brezillon @ 2026-01-08 14:45 UTC (permalink / raw)
To: Loïc Molinari
Cc: dri-devel, David Airlie, Simona Vetter, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, kernel
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?
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] drm/gem: Fix a GEM leak in drm_gem_get_unmapped_area()
2026-01-08 14:45 ` Boris Brezillon
@ 2026-01-09 9:42 ` Loïc Molinari
2026-01-09 9:50 ` Boris Brezillon
0 siblings, 1 reply; 12+ messages in thread
From: Loïc Molinari @ 2026-01-09 9:42 UTC (permalink / raw)
To: Boris Brezillon
Cc: dri-devel, David Airlie, Simona Vetter, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, kernel
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?
I'll take care of it. I'd better wait for that GEM leak fix to be pulled
first to avoid a conflict, right?
Regards,
Loïc
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] drm/gem: Fix a GEM leak in drm_gem_get_unmapped_area()
2026-01-09 9:42 ` Loïc Molinari
@ 2026-01-09 9:50 ` Boris Brezillon
0 siblings, 0 replies; 12+ messages in thread
From: Boris Brezillon @ 2026-01-09 9:50 UTC (permalink / raw)
To: Loïc Molinari
Cc: dri-devel, David Airlie, Simona Vetter, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, kernel
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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] drm/gem: Fix a GEM leak in drm_gem_get_unmapped_area()
2026-01-06 16:49 [PATCH] drm/gem: Fix a GEM leak in drm_gem_get_unmapped_area() Boris Brezillon
2026-01-07 11:12 ` Boris Brezillon
@ 2026-01-08 13:18 ` Loïc Molinari
2026-01-08 13:31 ` Boris Brezillon
2026-01-09 12:02 ` Boris Brezillon
2 siblings, 1 reply; 12+ messages in thread
From: Loïc Molinari @ 2026-01-08 13:18 UTC (permalink / raw)
To: Boris Brezillon, dri-devel, David Airlie, Simona Vetter,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann
Cc: kernel
Hi Boris,
On 06/01/2026 17:49, Boris Brezillon 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);
> + else
> + ret = obj->filp->f_op->get_unmapped_area(obj->filp, uaddr, len, 0, flags);
Apart maybe for this line exceeding 80 chars:
Reviewed-by: Loïc Molinari <loic.molinari@collabora.com>
Regards,
Loïc
>
> drm_gem_object_put(obj);
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] drm/gem: Fix a GEM leak in drm_gem_get_unmapped_area()
2026-01-08 13:18 ` Loïc Molinari
@ 2026-01-08 13:31 ` Boris Brezillon
2026-01-08 14:09 ` Loïc Molinari
0 siblings, 1 reply; 12+ messages in thread
From: Boris Brezillon @ 2026-01-08 13:31 UTC (permalink / raw)
To: Loïc Molinari
Cc: dri-devel, David Airlie, Simona Vetter, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, kernel
On Thu, 8 Jan 2026 14:18:26 +0100
Loïc Molinari <loic.molinari@collabora.com> wrote:
> Hi Boris,
>
> On 06/01/2026 17:49, Boris Brezillon 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);
> > + else
> > + ret = obj->filp->f_op->get_unmapped_area(obj->filp, uaddr, len, 0, flags);
>
> Apart maybe for this line exceeding 80 chars:
The limit has been bumped to 100 chars a while ago (checkpatch --strict
didn't complain), and for these single statements inside conditional
blocks, I prefer to have them on a single line when I can because
otherwise I tend to add curly braces to clearly flag the end of each
conditional block.
>
> Reviewed-by: Loïc Molinari <loic.molinari@collabora.com>
Thanks!
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] drm/gem: Fix a GEM leak in drm_gem_get_unmapped_area()
2026-01-08 13:31 ` Boris Brezillon
@ 2026-01-08 14:09 ` Loïc Molinari
0 siblings, 0 replies; 12+ messages in thread
From: Loïc Molinari @ 2026-01-08 14:09 UTC (permalink / raw)
To: Boris Brezillon
Cc: dri-devel, David Airlie, Simona Vetter, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, kernel
On 08/01/2026 14:31, Boris Brezillon wrote:
> On Thu, 8 Jan 2026 14:18:26 +0100
> Loïc Molinari <loic.molinari@collabora.com> wrote:
>
>> Hi Boris,
>>
>> On 06/01/2026 17:49, Boris Brezillon 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);
>>> + else
>>> + ret = obj->filp->f_op->get_unmapped_area(obj->filp, uaddr, len, 0, flags);
>>
>> Apart maybe for this line exceeding 80 chars:
>
> The limit has been bumped to 100 chars a while ago (checkpatch --strict
> didn't complain), and for these single statements inside conditional
> blocks, I prefer to have them on a single line when I can because
> otherwise I tend to add curly braces to clearly flag the end of each
> conditional block.
Yeah, just mentioned it for consistency within the file.
Regards,
Loïc
>
>>
>> Reviewed-by: Loïc Molinari <loic.molinari@collabora.com>
>
> Thanks!
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] drm/gem: Fix a GEM leak in drm_gem_get_unmapped_area()
2026-01-06 16:49 [PATCH] drm/gem: Fix a GEM leak in drm_gem_get_unmapped_area() Boris Brezillon
2026-01-07 11:12 ` Boris Brezillon
2026-01-08 13:18 ` Loïc Molinari
@ 2026-01-09 12:02 ` Boris Brezillon
2 siblings, 0 replies; 12+ messages in thread
From: Boris Brezillon @ 2026-01-09 12:02 UTC (permalink / raw)
To: dri-devel, David Airlie, Simona Vetter, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann
Cc: kernel, Loïc Molinari
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>
Queued to drm-misc-next.
> ---
> 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);
> + else
> + ret = obj->filp->f_op->get_unmapped_area(obj->filp, uaddr, len, 0, flags);
>
> drm_gem_object_put(obj);
>
^ permalink raw reply [flat|nested] 12+ messages in thread