[Intel-gfx] [PATCH 1/3] drm: Restore driver.preclose() for all to use

[Tang, CQ]

> -----Original Message-----
> From: Daniel Vetter <daniel at ffwll.ch>
> Sent: Monday, July 27, 2020 12:33 PM
> To: Chris Wilson <chris at chris-wilson.co.uk>; Dave Airlie <airlied at redhat.com>
> Cc: intel-gfx <intel-gfx at lists.freedesktop.org>; stable
> <stable at vger.kernel.org>; Gustavo Padovan
> <gustavo.padovan at collabora.com>; Tang, CQ <cq.tang at intel.com>; dri-
> devel <dri-devel at lists.freedesktop.org>; Vetter, Daniel
> <daniel.vetter at intel.com>
> Subject: Re: [PATCH 1/3] drm: Restore driver.preclose() for all to use
> 
> On Thu, Jul 23, 2020 at 7:21 PM Chris Wilson <chris at chris-wilson.co.uk>
> wrote:
> >
> > An unfortunate sequence of events, but it turns out there is a valid
> > usecase for being able to free/decouple the driver objects before they
> > are freed by the DRM core. In particular, if we have a pointer into a
> > drm core object from inside a driver object, that pointer needs to be
> > nerfed *before* it is freed so that concurrent access (e.g. debugfs)
> > does not following the dangling pointer.
> >
> > The legacy marker was adding in the code movement from drp_fops.c to
> > drm_file.c
> 
> I might fumble a lot, but not this one:
> 
> commit 45c3d213a400c952ab7119f394c5293bb6877e6b
> Author: Daniel Vetter <daniel.vetter at ffwll.ch>
> Date:   Mon May 8 10:26:33 2017 +0200
> 
>     drm: Nerf the preclose callback for modern drivers
> 
> Also looking at the debugfs hook that has some rather adventurous stuff
> going on I think, feels a bit like a kitchensink with batteries included. If that's
> really all needed I'd say iterate the contexts by first going over files, then the
> ctx (which arent shared anyway) and the problem should also be gone.

Debugfs code can jump in after drm_gem_release() (where file->object_idr is destroyed), but before postclose(). At this window, everything is fine for debugfs context accessing except the file->object_idr.

--CQ

> -Daniel
> 
> > References: 9acdac68bcdc ("drm: rename drm_fops.c to drm_file.c")
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > Cc: Daniel Vetter <daniel.vetter at intel.com>
> > Cc: Gustavo Padovan <gustavo.padovan at collabora.com>
> > Cc: CQ Tang <cq.tang at intel.com>
> > Cc: <stable at vger.kernel.org> # v4.12+
> > ---
> >  drivers/gpu/drm/drm_file.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> > index 0ac4566ae3f4..7b4258d6f7cc 100644
> > --- a/drivers/gpu/drm/drm_file.c
> > +++ b/drivers/gpu/drm/drm_file.c
> > @@ -258,8 +258,7 @@ void drm_file_free(struct drm_file *file)
> >                   (long)old_encode_dev(file->minor->kdev->devt),
> >                   atomic_read(&dev->open_count));
> >
> > -       if (drm_core_check_feature(dev, DRIVER_LEGACY) &&
> > -           dev->driver->preclose)
> > +       if (dev->driver->preclose)
> >                 dev->driver->preclose(dev, file);
> >
> >         if (drm_core_check_feature(dev, DRIVER_LEGACY))
> > --
> > 2.20.1
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> 
> 
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch