Re: [PATCH v2] fbdev: Fix invalid page access after closing deferred I/O devices
Added to drm-misc-fixes.
Am 29.01.23 um 09:28 schrieb Takashi Iwai:
When a fbdev with deferred I/O is once opened and closed, the dirty
pages still remain queued in the pageref list, and eventually later
those may be processed in the delayed work. This may lead to a
corruption of pages, hitting an Oops.
This patch makes sure to cancel the delayed work and clean up the
pageref list at closing the device for addressing the bug. A part of
the cleanup code is factored out as a new helper function that is
called from the common fb_release().
Reviewed-by: Patrik Jakobsson
Cc:
Signed-off-by: Takashi Iwai
---
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);
+
+void fb_deferred_io_cleanup(struct fb_info *info)
+{
+ struct fb_deferred_io *fbdefio = info->fbdefio;
+
+ fb_deferred_io_release(info);
kvfree(info->pagerefs);
mutex_destroy(&fbdefio->lock);
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index 3a6c8458eb8d..ab3545a00abc 100644
--- 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);
diff --git a/include/linux/fb.h b/include/linux/fb.h
index 96b96323e9cb..73eb1f85ea8e 100644
--- 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);
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
OpenPGP_signature
Description: OpenPGP digital signature
Re: [PATCH v2] fbdev: Fix invalid page access after closing deferred I/O devices
Am 29.01.23 um 09:28 schrieb Takashi Iwai:
When a fbdev with deferred I/O is once opened and closed, the dirty
pages still remain queued in the pageref list, and eventually later
those may be processed in the delayed work. This may lead to a
corruption of pages, hitting an Oops.
This patch makes sure to cancel the delayed work and clean up the
pageref list at closing the device for addressing the bug. A part of
the cleanup code is factored out as a new helper function that is
called from the common fb_release().
Reviewed-by: Patrik Jakobsson
Cc:
Signed-off-by: Takashi Iwai
Reviewed-by: Thomas Zimmermann
---
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);
+
+void fb_deferred_io_cleanup(struct fb_info *info)
+{
+ struct fb_deferred_io *fbdefio = info->fbdefio;
+
+ fb_deferred_io_release(info);
kvfree(info->pagerefs);
mutex_destroy(&fbdefio->lock);
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index 3a6c8458eb8d..ab3545a00abc 100644
--- 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);
diff --git a/include/linux/fb.h b/include/linux/fb.h
index 96b96323e9cb..73eb1f85ea8e 100644
--- 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);
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
OpenPGP_signature
Description: OpenPGP digital signature
Re: [PATCH v2] fbdev: Fix invalid page access after closing deferred I/O devices
Hi,
this bug could be a symptom of the problem reported at [1].
Best regards
Thomas
[1]
https://lore.kernel.org/dri-devel/CAM0jSHOcvZoyv-y6bnvFaUybRQsDx_mfOydL1uaNM4T4PgcA=a...@mail.gmail.com/T/#mfbef4df9b49fc5fda9bcfa26db5ca13cdaef6d7e
Am 29.01.23 um 09:28 schrieb Takashi Iwai:
When a fbdev with deferred I/O is once opened and closed, the dirty
pages still remain queued in the pageref list, and eventually later
those may be processed in the delayed work. This may lead to a
corruption of pages, hitting an Oops.
This patch makes sure to cancel the delayed work and clean up the
pageref list at closing the device for addressing the bug. A part of
the cleanup code is factored out as a new helper function that is
called from the common fb_release().
Reviewed-by: Patrik Jakobsson
Cc:
Signed-off-by: Takashi Iwai
---
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);
+
+void fb_deferred_io_cleanup(struct fb_info *info)
+{
+ struct fb_deferred_io *fbdefio = info->fbdefio;
+
+ fb_deferred_io_release(info);
kvfree(info->pagerefs);
mutex_destroy(&fbdefio->lock);
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index 3a6c8458eb8d..ab3545a00abc 100644
--- 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);
diff --git a/include/linux/fb.h b/include/linux/fb.h
index 96b96323e9cb..73eb1f85ea8e 100644
--- 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);
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
OpenPGP_signature
Description: OpenPGP digital signature
Re: [PATCH v2] fbdev: Fix invalid page access after closing deferred I/O devices
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
> > > Cc:
> > > Signed-off-by: Takashi Iwai
> >
> > 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
> > Cc: Javier Martinez Canillas
> > Cc: Maarten Lankhorst
> > Cc: Maxime Ripard
> > Cc: Zack Rusin
> > Cc: VMware Graphics Reviewers
> > Cc: Jaya Kumar
> > Cc: Daniel Vetter
> > Cc: "K. Y. Srinivasan"
> > Cc: Haiyang Zhang
> > Cc: Wei Liu
> > Cc: Dexuan Cui
> > Cc: Steve Glendinning
> > Cc: Bernie Thompson
> > Cc: Helge Deller
> > Cc: Andy Shevchenko
> > Cc: Greg Kroah-Hartman
> > Cc: Stephen Kitt
> > Cc: Peter Suti
> > Cc: Sam Ravnborg
> > Cc: Geert Uytterhoeven
> > Cc: ye xingchen
> > Cc: Petr Mladek
> > Cc: John Ogness
> > Cc: Tom Rix
> > Cc: dri-devel@lists.freedesktop.org
> > Cc: linux-fb...@vger.kernel.org
> > Cc: linux-hyp...@vger.kernel.org
> > Cc: # 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(
Re: [PATCH v2] fbdev: Fix invalid page access after closing deferred I/O devices
On Mon, 30 Jan 2023 10:28:16 +0100, Thomas Zimmermann wrote: > > Hi > > Am 30.01.23 um 09:52 schrieb Takashi Iwai: > > On Mon, 30 Jan 2023 09:28:36 +0100, > > Thomas Zimmermann wrote: > >> > >> Hi > >> > >> Am 29.01.23 um 09:28 schrieb Takashi Iwai: > >>> When a fbdev with deferred I/O is once opened and closed, the dirty > >>> pages still remain queued in the pageref list, and eventually later > >>> those may be processed in the delayed work. This may lead to a > >>> corruption of pages, hitting an Oops. > >> > >> Do you have more information on this problem? > > > > The details are in SUSE bugzilla, but that's an internal bug entry > > (and you know the number :) It happens at the following at least: > > > > - A VM is started with VGA console, no fb, on the installer > > - VM is switched to bochs drm > > - Start fbiterm on VT1, switching to the graphics mode on VT > > - Exit fbiterm, going back to the text mode on VT; > >at this moment, it gets Oops like: > > > > [ 42.338319][ T122] BUG: unable to handle page fault for address: > > e570c130 > > [ 42.340063][ T122] #PF: supervisor read access in kernel mode > > [ 42.340519][ T122] #PF: error_code(0x) - not-present page > > [ 42.340979][ T122] PGD 34c38067 P4D 34c38067 PUD 34c37067 PMD 0 > > [ 42.341456][ T122] Oops: [#1] PREEMPT SMP NOPTI > > [ 42.341853][ T122] CPU: 1 PID: 122 Comm: kworker/1:2 Not tainted > > 5.14.21-150500.5.g2ad24ee-default #1 SLE15-SP5 (unreleased) > > b7a28d028376a517e888a7ff28c5e5dede93267c > > [ 42.343000][ T122] Hardware name: QEMU Standard PC (i440FX + PIIX, > > 1996), > > BIOS rel-1.16.0-0-gd239552-rebuilt.opensuse.org 04/01/2014 > > [ 42.343929][ T122] Workqueue: events fb_deferred_io_work > > [ 42.344355][ T122] RIP: 0010:page_mapped+0x5e/0x90 > > [ 42.344743][ T122] Code: a8 01 75 d7 8b 47 30 f7 d0 c1 e8 1f c3 cc cc > > cc cc > > 48 89 df e8 33 9c 05 00 89 c1 31 c0 85 c9 74 13 eb d3 48 c1 e2 06 48 01 da > > <8b> > > 42 30 85 c0 79 c0 83 c1 01 48 8b 33 48 63 d1 b8 01 00 00 00 f7 > > [ 42.346285][ T122] RSP: 0018:b68640207e08 EFLAGS: 00010286 > > [ 42.346749][ T122] RAX: b3aea8f0 RBX: e570c0f0 RCX: > > 4000 > > [ 42.347355][ T122] RDX: e570c100 RSI: 000fc0010009 RDI: > > e570c0f0 > > [ 42.347960][ T122] RBP: c0503050 R08: R09: > > 0001 > > [ 42.348568][ T122] R10: R11: b68640207c88 R12: > > c0503020 > > [ 42.349180][ T122] R13: 921281dcdc00 R14: 9212bcf08000 R15: > > e570c0f0 > > [ 42.349789][ T122] FS: () GS:9212b3b0() > > knlGS: > > [ 42.350471][ T122] CS: 0010 DS: ES: CR0: 80050033 > > [ 42.350975][ T122] CR2: e570c130 CR3: 1b81 CR4: > > 06e0 > > [ 42.351588][ T122] Call Trace: > > [ 42.351845][ T122] > > [ 42.352069][ T122] page_mkclean+0x6e/0xc0 > > [ 42.352400][ T122] ? page_referenced_one+0x190/0x190 > > [ 42.353714][ T122] ? pmdp_collapse_flush+0x60/0x60 > > [ 42.354106][ T122] fb_deferred_io_work+0x13d/0x190 > > [ 42.354496][ T122] process_one_work+0x267/0x440 > > [ 42.354866][ T122] ? process_one_work+0x440/0x440 > > [ 42.355247][ T122] worker_thread+0x2d/0x3d0 > > [ 42.355590][ T122] ? process_one_work+0x440/0x440 > > [ 42.355972][ T122] kthread+0x156/0x180 > > [ 42.356281][ T122] ? set_kthread_struct+0x50/0x50 > > [ 42.356662][ T122] ret_from_fork+0x22/0x30 > > [ 42.357006][ T122] > > > > The page info shows that it's a compound page but it's somehow > > broken. On VM, it's triggered reliably with the scenario above, > > always at the same position. > > > > FWIW, the Oops is hit even if there is no rewrite on the screen. > > That is, another procedure is: > > - Start VM, run fbiterm on VT1 > > - Switch to VT2, text mode > > - On VT2, kill fbiterm; the crash still happens even if no screen > >change is performed > > > >> The mmap'ed buffer of the fbdev device comes from a vmalloc call. That > >> memory's location never changes; even across pairs of open/close on > >> the device file. I'm surprised that a page entry becomes invalid. > >> > >> In drm_fbdev_cleanup(), we first remove the fbdefio at [1] and then > >> vfree() the shadow buffer. So the memory should still be around until > >> fbdevio is gone. > > > > Yes, that's the puzzling part, too. Also, another thing is that the > > bug couldn't be triggered easily when the fb is started in a different > > way. e.g. when you run fbiterm & exit on the VM that had efifb, it > > didn't hit. > > > > So, overall, it might be that I'm scratching a wrong surface. But at > > least it "fixes" the problem above apparently, and the deferred io > > base code itself has certainly the potential problem in general as my > > patch suggests. > > Are there multiple graphics devices? There's just
Re: [PATCH v2] fbdev: Fix invalid page access after closing deferred I/O devices
Hi Am 30.01.23 um 09:52 schrieb Takashi Iwai: On Mon, 30 Jan 2023 09:28:36 +0100, Thomas Zimmermann wrote: Hi Am 29.01.23 um 09:28 schrieb Takashi Iwai: When a fbdev with deferred I/O is once opened and closed, the dirty pages still remain queued in the pageref list, and eventually later those may be processed in the delayed work. This may lead to a corruption of pages, hitting an Oops. Do you have more information on this problem? The details are in SUSE bugzilla, but that's an internal bug entry (and you know the number :) It happens at the following at least: - A VM is started with VGA console, no fb, on the installer - VM is switched to bochs drm - Start fbiterm on VT1, switching to the graphics mode on VT - Exit fbiterm, going back to the text mode on VT; at this moment, it gets Oops like: [ 42.338319][ T122] BUG: unable to handle page fault for address: e570c130 [ 42.340063][ T122] #PF: supervisor read access in kernel mode [ 42.340519][ T122] #PF: error_code(0x) - not-present page [ 42.340979][ T122] PGD 34c38067 P4D 34c38067 PUD 34c37067 PMD 0 [ 42.341456][ T122] Oops: [#1] PREEMPT SMP NOPTI [ 42.341853][ T122] CPU: 1 PID: 122 Comm: kworker/1:2 Not tainted 5.14.21-150500.5.g2ad24ee-default #1 SLE15-SP5 (unreleased) b7a28d028376a517e888a7ff28c5e5dede93267c [ 42.343000][ T122] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.16.0-0-gd239552-rebuilt.opensuse.org 04/01/2014 [ 42.343929][ T122] Workqueue: events fb_deferred_io_work [ 42.344355][ T122] RIP: 0010:page_mapped+0x5e/0x90 [ 42.344743][ T122] Code: a8 01 75 d7 8b 47 30 f7 d0 c1 e8 1f c3 cc cc cc cc 48 89 df e8 33 9c 05 00 89 c1 31 c0 85 c9 74 13 eb d3 48 c1 e2 06 48 01 da <8b> 42 30 85 c0 79 c0 83 c1 01 48 8b 33 48 63 d1 b8 01 00 00 00 f7 [ 42.346285][ T122] RSP: 0018:b68640207e08 EFLAGS: 00010286 [ 42.346749][ T122] RAX: b3aea8f0 RBX: e570c0f0 RCX: 4000 [ 42.347355][ T122] RDX: e570c100 RSI: 000fc0010009 RDI: e570c0f0 [ 42.347960][ T122] RBP: c0503050 R08: R09: 0001 [ 42.348568][ T122] R10: R11: b68640207c88 R12: c0503020 [ 42.349180][ T122] R13: 921281dcdc00 R14: 9212bcf08000 R15: e570c0f0 [ 42.349789][ T122] FS: () GS:9212b3b0() knlGS: [ 42.350471][ T122] CS: 0010 DS: ES: CR0: 80050033 [ 42.350975][ T122] CR2: e570c130 CR3: 1b81 CR4: 06e0 [ 42.351588][ T122] Call Trace: [ 42.351845][ T122] [ 42.352069][ T122] page_mkclean+0x6e/0xc0 [ 42.352400][ T122] ? page_referenced_one+0x190/0x190 [ 42.353714][ T122] ? pmdp_collapse_flush+0x60/0x60 [ 42.354106][ T122] fb_deferred_io_work+0x13d/0x190 [ 42.354496][ T122] process_one_work+0x267/0x440 [ 42.354866][ T122] ? process_one_work+0x440/0x440 [ 42.355247][ T122] worker_thread+0x2d/0x3d0 [ 42.355590][ T122] ? process_one_work+0x440/0x440 [ 42.355972][ T122] kthread+0x156/0x180 [ 42.356281][ T122] ? set_kthread_struct+0x50/0x50 [ 42.356662][ T122] ret_from_fork+0x22/0x30 [ 42.357006][ T122] The page info shows that it's a compound page but it's somehow broken. On VM, it's triggered reliably with the scenario above, always at the same position. FWIW, the Oops is hit even if there is no rewrite on the screen. That is, another procedure is: - Start VM, run fbiterm on VT1 - Switch to VT2, text mode - On VT2, kill fbiterm; the crash still happens even if no screen change is performed The mmap'ed buffer of the fbdev device comes from a vmalloc call. That memory's location never changes; even across pairs of open/close on the device file. I'm surprised that a page entry becomes invalid. In drm_fbdev_cleanup(), we first remove the fbdefio at [1] and then vfree() the shadow buffer. So the memory should still be around until fbdevio is gone. Yes, that's the puzzling part, too. Also, another thing is that the bug couldn't be triggered easily when the fb is started in a different way. e.g. when you run fbiterm & exit on the VM that had efifb, it didn't hit. So, overall, it might be that I'm scratching a wrong surface. But at least it "fixes" the problem above apparently, and the deferred io base code itself has certainly the potential problem in general as my patch suggests. Are there multiple graphics devices? There's just recently been a bugfix where graphics devices accidentally shared the same list of deferred pages. See https://lore.kernel.org/dri-devel/20230121192418.2814955-4-javi...@redhat.com/ [1] https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_fb_helper.c#L2146 This patch makes sure to cancel the delayed work and clean up the pageref list at closing the device for addressing the bug. A part of the cleanup code is factored out as a new helper function that is cal
Re: [PATCH v2] fbdev: Fix invalid page access after closing deferred I/O devices
On Mon, 30 Jan 2023 09:28:36 +0100, Thomas Zimmermann wrote: > > Hi > > Am 29.01.23 um 09:28 schrieb Takashi Iwai: > > When a fbdev with deferred I/O is once opened and closed, the dirty > > pages still remain queued in the pageref list, and eventually later > > those may be processed in the delayed work. This may lead to a > > corruption of pages, hitting an Oops. > > Do you have more information on this problem? The details are in SUSE bugzilla, but that's an internal bug entry (and you know the number :) It happens at the following at least: - A VM is started with VGA console, no fb, on the installer - VM is switched to bochs drm - Start fbiterm on VT1, switching to the graphics mode on VT - Exit fbiterm, going back to the text mode on VT; at this moment, it gets Oops like: [ 42.338319][ T122] BUG: unable to handle page fault for address: e570c130 [ 42.340063][ T122] #PF: supervisor read access in kernel mode [ 42.340519][ T122] #PF: error_code(0x) - not-present page [ 42.340979][ T122] PGD 34c38067 P4D 34c38067 PUD 34c37067 PMD 0 [ 42.341456][ T122] Oops: [#1] PREEMPT SMP NOPTI [ 42.341853][ T122] CPU: 1 PID: 122 Comm: kworker/1:2 Not tainted 5.14.21-150500.5.g2ad24ee-default #1 SLE15-SP5 (unreleased) b7a28d028376a517e888a7ff28c5e5dede93267c [ 42.343000][ T122] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.16.0-0-gd239552-rebuilt.opensuse.org 04/01/2014 [ 42.343929][ T122] Workqueue: events fb_deferred_io_work [ 42.344355][ T122] RIP: 0010:page_mapped+0x5e/0x90 [ 42.344743][ T122] Code: a8 01 75 d7 8b 47 30 f7 d0 c1 e8 1f c3 cc cc cc cc 48 89 df e8 33 9c 05 00 89 c1 31 c0 85 c9 74 13 eb d3 48 c1 e2 06 48 01 da <8b> 42 30 85 c0 79 c0 83 c1 01 48 8b 33 48 63 d1 b8 01 00 00 00 f7 [ 42.346285][ T122] RSP: 0018:b68640207e08 EFLAGS: 00010286 [ 42.346749][ T122] RAX: b3aea8f0 RBX: e570c0f0 RCX: 4000 [ 42.347355][ T122] RDX: e570c100 RSI: 000fc0010009 RDI: e570c0f0 [ 42.347960][ T122] RBP: c0503050 R08: R09: 0001 [ 42.348568][ T122] R10: R11: b68640207c88 R12: c0503020 [ 42.349180][ T122] R13: 921281dcdc00 R14: 9212bcf08000 R15: e570c0f0 [ 42.349789][ T122] FS: () GS:9212b3b0() knlGS: [ 42.350471][ T122] CS: 0010 DS: ES: CR0: 80050033 [ 42.350975][ T122] CR2: e570c130 CR3: 1b81 CR4: 06e0 [ 42.351588][ T122] Call Trace: [ 42.351845][ T122] [ 42.352069][ T122] page_mkclean+0x6e/0xc0 [ 42.352400][ T122] ? page_referenced_one+0x190/0x190 [ 42.353714][ T122] ? pmdp_collapse_flush+0x60/0x60 [ 42.354106][ T122] fb_deferred_io_work+0x13d/0x190 [ 42.354496][ T122] process_one_work+0x267/0x440 [ 42.354866][ T122] ? process_one_work+0x440/0x440 [ 42.355247][ T122] worker_thread+0x2d/0x3d0 [ 42.355590][ T122] ? process_one_work+0x440/0x440 [ 42.355972][ T122] kthread+0x156/0x180 [ 42.356281][ T122] ? set_kthread_struct+0x50/0x50 [ 42.356662][ T122] ret_from_fork+0x22/0x30 [ 42.357006][ T122] The page info shows that it's a compound page but it's somehow broken. On VM, it's triggered reliably with the scenario above, always at the same position. FWIW, the Oops is hit even if there is no rewrite on the screen. That is, another procedure is: - Start VM, run fbiterm on VT1 - Switch to VT2, text mode - On VT2, kill fbiterm; the crash still happens even if no screen change is performed > The mmap'ed buffer of the fbdev device comes from a vmalloc call. That > memory's location never changes; even across pairs of open/close on > the device file. I'm surprised that a page entry becomes invalid. > > In drm_fbdev_cleanup(), we first remove the fbdefio at [1] and then > vfree() the shadow buffer. So the memory should still be around until > fbdevio is gone. Yes, that's the puzzling part, too. Also, another thing is that the bug couldn't be triggered easily when the fb is started in a different way. e.g. when you run fbiterm & exit on the VM that had efifb, it didn't hit. So, overall, it might be that I'm scratching a wrong surface. But at least it "fixes" the problem above apparently, and the deferred io base code itself has certainly the potential problem in general as my patch suggests. > [1] > https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_fb_helper.c#L2146 > > > > > This patch makes sure to cancel the delayed work and clean up the > > pageref list at closing the device for addressing the bug. A part of > > the cleanup code is factored out as a new helper function that is > > called from the common fb_release(). > > The delayed work is required to copy the framebuffer to the device > output. So if it's just canceled, could this result in missing > updates? > > There's a call to cancel_delayed_work_sync()
Re: [PATCH v2] fbdev: Fix invalid page access after closing deferred I/O devices
Hi
Am 29.01.23 um 09:28 schrieb Takashi Iwai:
When a fbdev with deferred I/O is once opened and closed, the dirty
pages still remain queued in the pageref list, and eventually later
those may be processed in the delayed work. This may lead to a
corruption of pages, hitting an Oops.
Do you have more information on this problem?
The mmap'ed buffer of the fbdev device comes from a vmalloc call. That
memory's location never changes; even across pairs of open/close on the
device file. I'm surprised that a page entry becomes invalid.
In drm_fbdev_cleanup(), we first remove the fbdefio at [1] and then
vfree() the shadow buffer. So the memory should still be around until
fbdevio is gone.
[1]
https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_fb_helper.c#L2146
This patch makes sure to cancel the delayed work and clean up the
pageref list at closing the device for addressing the bug. A part of
the cleanup code is factored out as a new helper function that is
called from the common fb_release().
The delayed work is required to copy the framebuffer to the device
output. So if it's just canceled, could this result in missing updates?
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.
Reviewed-by: Patrik Jakobsson
Cc:
Signed-off-by: Takashi Iwai
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")
Cc: Thomas Zimmermann
Cc: Javier Martinez Canillas
Cc: Maarten Lankhorst
Cc: Maxime Ripard
Cc: Zack Rusin
Cc: VMware Graphics Reviewers
Cc: Jaya Kumar
Cc: Daniel Vetter
Cc: "K. Y. Srinivasan"
Cc: Haiyang Zhang
Cc: Wei Liu
Cc: Dexuan Cui
Cc: Steve Glendinning
Cc: Bernie Thompson
Cc: Helge Deller
Cc: Andy Shevchenko
Cc: Greg Kroah-Hartman
Cc: Stephen Kitt
Cc: Peter Suti
Cc: Sam Ravnborg
Cc: Geert Uytterhoeven
Cc: ye xingchen
Cc: Petr Mladek
Cc: John Ogness
Cc: Tom Rix
Cc: dri-devel@lists.freedesktop.org
Cc: linux-fb...@vger.kernel.org
Cc: linux-hyp...@vger.kernel.org
Cc: # v5.19+
---
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.
Best regards
Thomas
+
+void fb_deferred_io_cleanup(struct fb_info *info)
+{
+ struct fb_deferred_io *fbdefio = info->fbdefio;
+
+ fb_deferred_io_release(info);
kvfree(info->pagerefs);
mutex_destroy(&fbdefio->lock);
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index 3a6c8458eb8d..ab3545a00abc 100644
--- 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);
diff --git a/include/linux/fb.h b/include/linux/fb.h
index 96b96323e9cb..73eb1f85ea8e 100644
--- 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);
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
OpenPGP_signature
Description: OpenPGP digital signature
Re: [PATCH v2] fbdev: Fix invalid page access after closing deferred I/O devices
On Sun, 2023-01-29 at 09:28 +0100, Takashi Iwai wrote:
> When a fbdev with deferred I/O is once opened and closed, the dirty
> pages still remain queued in the pageref list, and eventually later
> those may be processed in the delayed work. This may lead to a
> corruption of pages, hitting an Oops.
>
> This patch makes sure to cancel the delayed work and clean up the
> pageref list at closing the device for addressing the bug. A part of
> the cleanup code is factored out as a new helper function that is
> called from the common fb_release().
>
> Reviewed-by: Patrik Jakobsson
> Cc:
> Signed-off-by: Takashi Iwai
> ---
> 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);
> +
> +void fb_deferred_io_cleanup(struct fb_info *info)
> +{
> + struct fb_deferred_io *fbdefio = info->fbdefio;
> +
> + fb_deferred_io_release(info);
>
> kvfree(info->pagerefs);
> mutex_destroy(&fbdefio->lock);
> diff --git a/drivers/video/fbdev/core/fbmem.c
> b/drivers/video/fbdev/core/fbmem.c
> index 3a6c8458eb8d..ab3545a00abc 100644
> --- 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);
> diff --git a/include/linux/fb.h b/include/linux/fb.h
> index 96b96323e9cb..73eb1f85ea8e 100644
> --- 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);
Tested-by: Miko Larsson
--
~miko
[PATCH v2] fbdev: Fix invalid page access after closing deferred I/O devices
When a fbdev with deferred I/O is once opened and closed, the dirty
pages still remain queued in the pageref list, and eventually later
those may be processed in the delayed work. This may lead to a
corruption of pages, hitting an Oops.
This patch makes sure to cancel the delayed work and clean up the
pageref list at closing the device for addressing the bug. A part of
the cleanup code is factored out as a new helper function that is
called from the common fb_release().
Reviewed-by: Patrik Jakobsson
Cc:
Signed-off-by: Takashi Iwai
---
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);
+
+void fb_deferred_io_cleanup(struct fb_info *info)
+{
+ struct fb_deferred_io *fbdefio = info->fbdefio;
+
+ fb_deferred_io_release(info);
kvfree(info->pagerefs);
mutex_destroy(&fbdefio->lock);
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index 3a6c8458eb8d..ab3545a00abc 100644
--- 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);
diff --git a/include/linux/fb.h b/include/linux/fb.h
index 96b96323e9cb..73eb1f85ea8e 100644
--- 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
