| Message ID | 20230308063628.15233-1-tiwai@suse.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | fbdev: Fix incorrect page mapping clearance at fb_deferred_io_release() | expand |
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 >
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
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
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 --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 */
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(-)