On Fri, 9 Jan 2026 10:42:21 +0100 Loïc Molinari <[email protected]> wrote:
> Hi Boris, > > On 08/01/2026 15:45, Boris Brezillon wrote: > > On Thu, 8 Jan 2026 15:04:33 +0100 > > Loïc Molinari <[email protected]> wrote: > > > >> On 08/01/2026 14:43, Boris Brezillon wrote: > >>> On Thu, 8 Jan 2026 14:25:01 +0100 > >>> Loïc Molinari <[email protected]> wrote: > >>> > >>>> On 07/01/2026 12:12, Boris Brezillon wrote: > >>>>> On Tue, 6 Jan 2026 17:49:35 +0100 > >>>>> Boris Brezillon <[email protected]> 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 <[email protected]> > >>>>>> Fixes: 99bda20d6d4c ("drm/gem: Introduce drm_gem_get_unmapped_area() > >>>>>> fop") > >>>>>> Signed-off-by: Boris Brezillon <[email protected]> > >>>>>> --- > >>>>>> 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.
