[PATCH v2] fbdev: Fix invalid page access after closing deferred I/O devices

[Takashi Iwai]

On Mon, 30 Jan 2023 09:52:43 +0100,
Takashi Iwai wrote:
> 
> > There's a call to cancel_delayed_work_sync() in the new helper
> > fb_deferred_io_release(). Is this the right function? Maybe
> > flush_delayed_work() is a better choice.
> 
> I thought of that, but took a shorter path.
> OK, let's check whether this keeps working with that change.

Interestingly, this still makes the bug happening at the very same
place.  (The tested patch is below.)
So, more looking and testing, more confusing the problem becomes :-<


Takashi

-- 8< --
--- a/drivers/video/fbdev/core/fb_defio.c
+++ b/drivers/video/fbdev/core/fb_defio.c
@@ -313,20 +313,31 @@ void fb_deferred_io_open(struct fb_info *info,
 }
 EXPORT_SYMBOL_GPL(fb_deferred_io_open);
 
-void fb_deferred_io_cleanup(struct fb_info *info)
+/* clear out the mapping that we setup */
+static void fb_deferred_io_clear_mapping(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 */
 	for (i = 0 ; i < info->fix.smem_len; i += PAGE_SIZE) {
 		page = fb_deferred_io_page(info, i);
 		page->mapping = NULL;
 	}
+}
+
+void fb_deferred_io_release(struct fb_info *info)
+{
+	flush_delayed_work(&info->deferred_work);
+	fb_deferred_io_clear_mapping(info);
+}
+
+void fb_deferred_io_cleanup(struct fb_info *info)
+{
+	struct fb_deferred_io *fbdefio = info->fbdefio;
+
+	BUG_ON(!fbdefio);
+	cancel_delayed_work_sync(&info->deferred_work);
+	fb_deferred_io_clear_mapping(info);
 
 	kvfree(info->pagerefs);
 	mutex_destroy(&fbdefio->lock);
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -1454,6 +1454,10 @@ __releases(&info->lock)
 	struct fb_info * const info = file->private_data;
 
 	lock_fb_info(info);
+#if IS_ENABLED(CONFIG_FB_DEFERRED_IO)
+	if (info->fbdefio)
+		fb_deferred_io_release(info);
+#endif
 	if (info->fbops->fb_release)
 		info->fbops->fb_release(info,1);
 	module_put(info->fbops->owner);
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -662,6 +662,7 @@ extern int  fb_deferred_io_init(struct fb_info *info);
 extern void fb_deferred_io_open(struct fb_info *info,
 				struct inode *inode,
 				struct file *file);
+extern void fb_deferred_io_release(struct fb_info *info);
 extern void fb_deferred_io_cleanup(struct fb_info *info);
 extern int fb_deferred_io_fsync(struct file *file, loff_t start,
 				loff_t end, int datasync);
-- 
2.35.3



> 
> > > Reviewed-by: Patrik Jakobsson <patrik.r.jakobsson at gmail.com>
> > > Cc: <stable at vger.kernel.org>
> > > Signed-off-by: Takashi Iwai <tiwai at suse.de>
> > 
> > This could use a Fixes tag. It's not exactly clear to me when this
> > problem got originally introduced, but the recent refactoring seems a
> > candidate.
> > 
> > Fixes: 56c134f7f1b5 ("fbdev: Track deferred-I/O pages in pageref struct")
> 
> Hrm, this might be.  Maybe Patrik can test with the revert of this?
> 
> > Cc: Thomas Zimmermann <tzimmermann at suse.de>
> > Cc: Javier Martinez Canillas <javierm at redhat.com>
> > Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> > Cc: Maxime Ripard <mripard at kernel.org>
> > Cc: Zack Rusin <zackr at vmware.com>
> > Cc: VMware Graphics Reviewers <linux-graphics-maintainer at vmware.com>
> > Cc: Jaya Kumar <jayalk at intworks.biz>
> > Cc: Daniel Vetter <daniel at ffwll.ch>
> > Cc: "K. Y. Srinivasan" <kys at microsoft.com>
> > Cc: Haiyang Zhang <haiyangz at microsoft.com>
> > Cc: Wei Liu <wei.liu at kernel.org>
> > Cc: Dexuan Cui <decui at microsoft.com>
> > Cc: Steve Glendinning <steve.glendinning at shawell.net>
> > Cc: Bernie Thompson <bernie at plugable.com>
> > Cc: Helge Deller <deller at gmx.de>
> > Cc: Andy Shevchenko <andriy.shevchenko at linux.intel.com>
> > Cc: Greg Kroah-Hartman <gregkh at linuxfoundation.org>
> > Cc: Stephen Kitt <steve at sk2.org>
> > Cc: Peter Suti <peter.suti at streamunlimited.com>
> > Cc: Sam Ravnborg <sam at ravnborg.org>
> > Cc: Geert Uytterhoeven <geert+renesas at glider.be>
> > Cc: ye xingchen <ye.xingchen at zte.com.cn>
> > Cc: Petr Mladek <pmladek at suse.com>
> > Cc: John Ogness <john.ogness at linutronix.de>
> > Cc: Tom Rix <trix at redhat.com>
> > Cc: dri-devel at lists.freedesktop.org
> > Cc: linux-fbdev at vger.kernel.org
> > Cc: linux-hyperv at vger.kernel.org
> > Cc: <stable at vger.kernel.org> # v5.19+
> 
> Nah, please don't.  Too many Cc's, literally a spam.
> 
> > > ---
> > > v1->v2: Fix build error without CONFIG_FB_DEFERRED_IO
> > > 
> > >   drivers/video/fbdev/core/fb_defio.c | 10 +++++++++-
> > >   drivers/video/fbdev/core/fbmem.c    |  4 ++++
> > >   include/linux/fb.h                  |  1 +
> > >   3 files changed, 14 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/video/fbdev/core/fb_defio.c b/drivers/video/fbdev/core/fb_defio.c
> > > index c730253ab85c..583cbcf09446 100644
> > > --- a/drivers/video/fbdev/core/fb_defio.c
> > > +++ b/drivers/video/fbdev/core/fb_defio.c
> > > @@ -313,7 +313,7 @@ void fb_deferred_io_open(struct fb_info *info,
> > >   }
> > >   EXPORT_SYMBOL_GPL(fb_deferred_io_open);
> > >   -void fb_deferred_io_cleanup(struct fb_info *info)
> > > +void fb_deferred_io_release(struct fb_info *info)
> > >   {
> > >   	struct fb_deferred_io *fbdefio = info->fbdefio;
> > >   	struct page *page;
> > > @@ -327,6 +327,14 @@ void fb_deferred_io_cleanup(struct fb_info *info)
> > >   		page = fb_deferred_io_page(info, i);
> > >   		page->mapping = NULL;
> > >   	}
> > > +}
> > > +EXPORT_SYMBOL_GPL(fb_deferred_io_release);
> > 
> > It's all in the same module. No need to export this symbol.
> 
> I noticed it, too, but just keep the same style as other functions :)
> That said, the other exported symbols are also useless.  I can prepare
> another patch to clean it up.
> 
> 
> thanks,
> 
> Takashi