Re: [PATCH] fbdev: Fix incorrect page mapping clearance at fb_deferred_io_release()

[Takashi Iwai]

On Wed, 08 Mar 2023 10:08:24 +0100,
Patrik Jakobsson wrote:
> 
> On Wed, Mar 8, 2023 at 7:36 AM Takashi Iwai <tiwai@xxxxxxx> wrote:
> >
> > The recent fix for the deferred I/O by the commit
> >   3efc61d95259 ("fbdev: Fix invalid page access after closing deferred I/O devices")
> > caused a regression when the same fb device is opened/closed while
> > it's being used.  It resulted in a frozen screen even if something
> > is redrawn there after the close.  The breakage is because the patch
> > was made under a wrong assumption of a single open; in the current
> > code, fb_deferred_io_release() cleans up the page mapping of the
> > pageref list and it calls cancel_delayed_work_sync() unconditionally,
> > where both are no correct behavior for multiple opens.
> >
> > This patch adds a refcount for the opens of the device, and applies
> > the cleanup only when all files get closed.
> >
> > Fixes: 3efc61d95259 ("fbdev: Fix invalid page access after closing deferred I/O devices")
> > Cc: <stable@xxxxxxxxxxxxxxx>
> > Signed-off-by: Takashi Iwai <tiwai@xxxxxxx>
> > ---
> >  drivers/video/fbdev/core/fb_defio.c | 16 +++++++++++++---
> >  include/linux/fb.h                  |  1 +
> >  2 files changed, 14 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/video/fbdev/core/fb_defio.c b/drivers/video/fbdev/core/fb_defio.c
> > index aa5f059d0222..9dcec9e020b6 100644
> > --- a/drivers/video/fbdev/core/fb_defio.c
> > +++ b/drivers/video/fbdev/core/fb_defio.c
> > @@ -305,17 +305,19 @@ void fb_deferred_io_open(struct fb_info *info,
> >                          struct inode *inode,
> >                          struct file *file)
> >  {
> > +       struct fb_deferred_io *fbdefio = info->fbdefio;
> > +
> >         file->f_mapping->a_ops = &fb_deferred_io_aops;
> > +       fbdefio->opens++;
> >  }
> >  EXPORT_SYMBOL_GPL(fb_deferred_io_open);
> >
> > -void fb_deferred_io_release(struct fb_info *info)
> > +static void fb_deferred_io_release_internal(struct fb_info *info)
> 
> Maybe a better name would be fb_deferred_io_lastclose() to be more in
> line with DRM?

Sounds good.

> >  {
> >         struct fb_deferred_io *fbdefio = info->fbdefio;
> >         struct page *page;
> >         int i;
> >
> > -       BUG_ON(!fbdefio);
> 
> Should the BUG_ON be put back into fb_deferred_io_release()?

It can be, but honestly speaking, such a BUG_ON() is utterly useless.
It should be WARN_ON() and return, if the sanity check is inevitably
needed.

> >         cancel_delayed_work_sync(&info->deferred_work);
> >
> >         /* clear out the mapping that we setup */
> > @@ -324,13 +326,21 @@ void fb_deferred_io_release(struct fb_info *info)
> >                 page->mapping = NULL;
> >         }
> >  }
> > +
> > +void fb_deferred_io_release(struct fb_info *info)
> > +{
> > +       struct fb_deferred_io *fbdefio = info->fbdefio;
> > +
> > +       if (!--fbdefio->opens)
> > +               fb_deferred_io_release_internal(info);
> 
> I think this can race so we need locking.

This one is fine, as it's always called inside the fb lock in the
caller side.  Maybe worth to comment in the code.

> > +}
> >  EXPORT_SYMBOL_GPL(fb_deferred_io_release);
> >
> >  void fb_deferred_io_cleanup(struct fb_info *info)
> >  {
> >         struct fb_deferred_io *fbdefio = info->fbdefio;
> >
> > -       fb_deferred_io_release(info);
> > +       fb_deferred_io_release_internal(info);
> >
> >         kvfree(info->pagerefs);
> >         mutex_destroy(&fbdefio->lock);
> > diff --git a/include/linux/fb.h b/include/linux/fb.h
> > index d8d20514ea05..29674a29d1c4 100644
> > --- a/include/linux/fb.h
> > +++ b/include/linux/fb.h
> > @@ -212,6 +212,7 @@ struct fb_deferred_io {
> >         /* delay between mkwrite and deferred handler */
> >         unsigned long delay;
> >         bool sort_pagereflist; /* sort pagelist by offset */
> > +       int opens; /* number of opened files */
> 
> I would prefer the name num_opens (or open_count as in DRM) instead of
> opens since it can be interpreted as a verb.

I don't mind either way.  I'd choose the latter.

> Also, don't we need it to be atomic_t?

It's always in the fb lock, so that should be fine with the standard
int.


thanks,

Takashi