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?
