diff mbox series

fbdev: Fix incorrect page mapping clearance at fb_deferred_io_release()

Message ID 20230308063628.15233-1-tiwai@suse.de
State New
Headers
Series fbdev: Fix incorrect page mapping clearance at fb_deferred_io_release() |

Commit Message

Takashi Iwai March 8, 2023, 6:36 a.m. UTC 
  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@vger.kernel.org>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/video/fbdev/core/fb_defio.c | 16 +++++++++++++---
 include/linux/fb.h                  |  1 +
 2 files changed, 14 insertions(+), 3 deletions(-)

Comments

Patrik Jakobsson March 8, 2023, 9:08 a.m. UTC | #1 
On Wed, Mar 8, 2023 at 7:36 AM Takashi Iwai <tiwai@suse.de> 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@vger.kernel.org>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>  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?

>  {
>         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()?

>         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.

> +}
>  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.
Also, don't we need it to be atomic_t?

>         struct mutex lock; /* mutex that protects the pageref list */
>         struct list_head pagereflist; /* list of pagerefs for touched pages */
>         /* callback */
> --
> 2.35.3
>
Takashi Iwai March 8, 2023, 9:14 a.m. UTC | #2 
On Wed, 08 Mar 2023 10:08:24 +0100,
Patrik Jakobsson wrote:
> 
> On Wed, Mar 8, 2023 at 7:36 AM Takashi Iwai <tiwai@suse.de> 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@vger.kernel.org>
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > ---
> >  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
Patrik Jakobsson March 8, 2023, 9:34 a.m. UTC | #3 
On Wed, Mar 8, 2023 at 10:14 AM Takashi Iwai <tiwai@suse.de> wrote:
>
> On Wed, 08 Mar 2023 10:08:24 +0100,
> Patrik Jakobsson wrote:
> >
> > On Wed, Mar 8, 2023 at 7:36 AM Takashi Iwai <tiwai@suse.de> 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@vger.kernel.org>
> > > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > > ---
> > >  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.

I agree. It's rather pointless since it's already checked in fb_release().

>
> > >         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.

Ah, yes, fb_release() locks around everything. Then we are fine. A
comment would be nice.

>
> > > +}
> > >  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.

Yes

>
>
> thanks,
>
> Takashi
kernel test robot March 8, 2023, 10:54 a.m. UTC | #4 
Hi Takashi,

I love your patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on v6.3-rc1 next-20230308]
[cannot apply to drm-misc/drm-misc-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Takashi-Iwai/fbdev-Fix-incorrect-page-mapping-clearance-at-fb_deferred_io_release/20230308-143749
patch link:    https://lore.kernel.org/r/20230308063628.15233-1-tiwai%40suse.de
patch subject: [PATCH] fbdev: Fix incorrect page mapping clearance at fb_deferred_io_release()
config: x86_64-allyesconfig (https://download.01.org/0day-ci/archive/20230308/202303081843.yyoyPjaN-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/fe94468e5ddd91231f1624559f861fb8110163c3
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Takashi-Iwai/fbdev-Fix-incorrect-page-mapping-clearance-at-fb_deferred_io_release/20230308-143749
        git checkout fe94468e5ddd91231f1624559f861fb8110163c3
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=x86_64 olddefconfig
        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/video/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303081843.yyoyPjaN-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/video/fbdev/core/fb_defio.c: In function 'fb_deferred_io_release_internal':
>> drivers/video/fbdev/core/fb_defio.c:317:32: warning: unused variable 'fbdefio' [-Wunused-variable]
     317 |         struct fb_deferred_io *fbdefio = info->fbdefio;
         |                                ^~~~~~~


vim +/fbdefio +317 drivers/video/fbdev/core/fb_defio.c

0b78f8bcf4951af drivers/video/fbdev/core/fb_defio.c Matthew Wilcox 2021-06-01  314  
fe94468e5ddd912 drivers/video/fbdev/core/fb_defio.c Takashi Iwai   2023-03-08  315  static void fb_deferred_io_release_internal(struct fb_info *info)
60b59beafba875a drivers/video/fb_defio.c            Jaya Kumar     2007-05-08  316  {
60b59beafba875a drivers/video/fb_defio.c            Jaya Kumar     2007-05-08 @317  	struct fb_deferred_io *fbdefio = info->fbdefio;
0b78f8bcf4951af drivers/video/fbdev/core/fb_defio.c Matthew Wilcox 2021-06-01  318  	struct page *page;
0b78f8bcf4951af drivers/video/fbdev/core/fb_defio.c Matthew Wilcox 2021-06-01  319  	int i;
60b59beafba875a drivers/video/fb_defio.c            Jaya Kumar     2007-05-08  320  
181b74ef794e198 drivers/video/fb_defio.c            Tejun Heo      2011-06-15  321  	cancel_delayed_work_sync(&info->deferred_work);
0b78f8bcf4951af drivers/video/fbdev/core/fb_defio.c Matthew Wilcox 2021-06-01  322  
0b78f8bcf4951af drivers/video/fbdev/core/fb_defio.c Matthew Wilcox 2021-06-01  323  	/* clear out the mapping that we setup */
0b78f8bcf4951af drivers/video/fbdev/core/fb_defio.c Matthew Wilcox 2021-06-01  324  	for (i = 0 ; i < info->fix.smem_len; i += PAGE_SIZE) {
0b78f8bcf4951af drivers/video/fbdev/core/fb_defio.c Matthew Wilcox 2021-06-01  325  		page = fb_deferred_io_page(info, i);
0b78f8bcf4951af drivers/video/fbdev/core/fb_defio.c Matthew Wilcox 2021-06-01  326  		page->mapping = NULL;
0b78f8bcf4951af drivers/video/fbdev/core/fb_defio.c Matthew Wilcox 2021-06-01  327  	}
3efc61d95259956 drivers/video/fbdev/core/fb_defio.c Takashi Iwai   2023-01-29  328  }
fe94468e5ddd912 drivers/video/fbdev/core/fb_defio.c Takashi Iwai   2023-03-08  329
diff mbox series

Patch

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)
 {
 	struct fb_deferred_io *fbdefio = info->fbdefio;
 	struct page *page;
 	int i;
 
-	BUG_ON(!fbdefio);
 	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);
+}
 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 */
 	struct mutex lock; /* mutex that protects the pageref list */
 	struct list_head pagereflist; /* list of pagerefs for touched pages */
 	/* callback */