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 <[email protected]>
Cc: <[email protected]>
Signed-off-by: Takashi Iwai <[email protected]>
---
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
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 <[email protected]>
> Cc: <[email protected]>
> Signed-off-by: Takashi Iwai <[email protected]>
> ---
> 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 <[email protected]>
--
~miko
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 <[email protected]>
> Cc: <[email protected]>
> Signed-off-by: Takashi Iwai <[email protected]>
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 <[email protected]>
Cc: Javier Martinez Canillas <[email protected]>
Cc: Maarten Lankhorst <[email protected]>
Cc: Maxime Ripard <[email protected]>
Cc: Zack Rusin <[email protected]>
Cc: VMware Graphics Reviewers <[email protected]>
Cc: Jaya Kumar <[email protected]>
Cc: Daniel Vetter <[email protected]>
Cc: "K. Y. Srinivasan" <[email protected]>
Cc: Haiyang Zhang <[email protected]>
Cc: Wei Liu <[email protected]>
Cc: Dexuan Cui <[email protected]>
Cc: Steve Glendinning <[email protected]>
Cc: Bernie Thompson <[email protected]>
Cc: Helge Deller <[email protected]>
Cc: Andy Shevchenko <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Stephen Kitt <[email protected]>
Cc: Peter Suti <[email protected]>
Cc: Sam Ravnborg <[email protected]>
Cc: Geert Uytterhoeven <[email protected]>
Cc: ye xingchen <[email protected]>
Cc: Petr Mladek <[email protected]>
Cc: John Ogness <[email protected]>
Cc: Tom Rix <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: <[email protected]> # 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
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:
ffffe570c1000030
[ 42.340063][ T122] #PF: supervisor read access in kernel mode
[ 42.340519][ T122] #PF: error_code(0x0000) - not-present page
[ 42.340979][ T122] PGD 34c38067 P4D 34c38067 PUD 34c37067 PMD 0
[ 42.341456][ T122] Oops: 0000 [#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:ffffb68640207e08 EFLAGS: 00010286
[ 42.346749][ T122] RAX: 00000000b3aea8f0 RBX: ffffe570c0f00000 RCX:
0000000000004000
[ 42.347355][ T122] RDX: ffffe570c1000000 RSI: 000fffffc0010009 RDI:
ffffe570c0f00000
[ 42.347960][ T122] RBP: ffffffffc0503050 R08: 0000000000000000 R09:
0000000000000001
[ 42.348568][ T122] R10: 0000000000000000 R11: ffffb68640207c88 R12:
ffffffffc0503020
[ 42.349180][ T122] R13: ffff921281dcdc00 R14: ffff9212bcf08000 R15:
ffffe570c0f00000
[ 42.349789][ T122] FS: 0000000000000000(0000) GS:ffff9212b3b00000(0000)
knlGS:0000000000000000
[ 42.350471][ T122] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 42.350975][ T122] CR2: ffffe570c1000030 CR3: 000000001b810000 CR4:
00000000000006e0
[ 42.351588][ T122] Call Trace:
[ 42.351845][ T122] <TASK>
[ 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] </TASK>
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() 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.
> > Reviewed-by: Patrik Jakobsson <[email protected]>
> > Cc: <[email protected]>
> > Signed-off-by: Takashi Iwai <[email protected]>
>
> 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 <[email protected]>
> Cc: Javier Martinez Canillas <[email protected]>
> Cc: Maarten Lankhorst <[email protected]>
> Cc: Maxime Ripard <[email protected]>
> Cc: Zack Rusin <[email protected]>
> Cc: VMware Graphics Reviewers <[email protected]>
> Cc: Jaya Kumar <[email protected]>
> Cc: Daniel Vetter <[email protected]>
> Cc: "K. Y. Srinivasan" <[email protected]>
> Cc: Haiyang Zhang <[email protected]>
> Cc: Wei Liu <[email protected]>
> Cc: Dexuan Cui <[email protected]>
> Cc: Steve Glendinning <[email protected]>
> Cc: Bernie Thompson <[email protected]>
> Cc: Helge Deller <[email protected]>
> Cc: Andy Shevchenko <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: Stephen Kitt <[email protected]>
> Cc: Peter Suti <[email protected]>
> Cc: Sam Ravnborg <[email protected]>
> Cc: Geert Uytterhoeven <[email protected]>
> Cc: ye xingchen <[email protected]>
> Cc: Petr Mladek <[email protected]>
> Cc: John Ogness <[email protected]>
> Cc: Tom Rix <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: <[email protected]> # 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
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:
> ffffe570c1000030
> [ 42.340063][ T122] #PF: supervisor read access in kernel mode
> [ 42.340519][ T122] #PF: error_code(0x0000) - not-present page
> [ 42.340979][ T122] PGD 34c38067 P4D 34c38067 PUD 34c37067 PMD 0
> [ 42.341456][ T122] Oops: 0000 [#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:ffffb68640207e08 EFLAGS: 00010286
> [ 42.346749][ T122] RAX: 00000000b3aea8f0 RBX: ffffe570c0f00000 RCX:
> 0000000000004000
> [ 42.347355][ T122] RDX: ffffe570c1000000 RSI: 000fffffc0010009 RDI:
> ffffe570c0f00000
> [ 42.347960][ T122] RBP: ffffffffc0503050 R08: 0000000000000000 R09:
> 0000000000000001
> [ 42.348568][ T122] R10: 0000000000000000 R11: ffffb68640207c88 R12:
> ffffffffc0503020
> [ 42.349180][ T122] R13: ffff921281dcdc00 R14: ffff9212bcf08000 R15:
> ffffe570c0f00000
> [ 42.349789][ T122] FS: 0000000000000000(0000) GS:ffff9212b3b00000(0000)
> knlGS:0000000000000000
> [ 42.350471][ T122] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 42.350975][ T122] CR2: ffffe570c1000030 CR3: 000000001b810000 CR4:
> 00000000000006e0
> [ 42.351588][ T122] Call Trace:
> [ 42.351845][ T122] <TASK>
> [ 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] </TASK>
>
> 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/[email protected]/
>
>> [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.
>
> I thought of that, but took a shorter path.
> OK, let's check whether this keeps working with that change.
I read that cancel_() is not enough and needs to be followed by a
flush_() to ensure quiescence. So maybe we should call that flush_
unconditionally.
>
>>> Reviewed-by: Patrik Jakobsson <[email protected]>
>>> Cc: <[email protected]>
>>> Signed-off-by: Takashi Iwai <[email protected]>
>>
>> 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?
That's not easily revertable.
>
>> Cc: Thomas Zimmermann <[email protected]>
>> Cc: Javier Martinez Canillas <[email protected]>
>> Cc: Maarten Lankhorst <[email protected]>
>> Cc: Maxime Ripard <[email protected]>
>> Cc: Zack Rusin <[email protected]>
>> Cc: VMware Graphics Reviewers <[email protected]>
>> Cc: Jaya Kumar <[email protected]>
>> Cc: Daniel Vetter <[email protected]>
>> Cc: "K. Y. Srinivasan" <[email protected]>
>> Cc: Haiyang Zhang <[email protected]>
>> Cc: Wei Liu <[email protected]>
>> Cc: Dexuan Cui <[email protected]>
>> Cc: Steve Glendinning <[email protected]>
>> Cc: Bernie Thompson <[email protected]>
>> Cc: Helge Deller <[email protected]>
>> Cc: Andy Shevchenko <[email protected]>
>> Cc: Greg Kroah-Hartman <[email protected]>
>> Cc: Stephen Kitt <[email protected]>
>> Cc: Peter Suti <[email protected]>
>> Cc: Sam Ravnborg <[email protected]>
>> Cc: Geert Uytterhoeven <[email protected]>
>> Cc: ye xingchen <[email protected]>
>> Cc: Petr Mladek <[email protected]>
>> Cc: John Ogness <[email protected]>
>> Cc: Tom Rix <[email protected]>
>> Cc: [email protected]
>> Cc: [email protected]
>> Cc: [email protected]
>> Cc: <[email protected]> # v5.19+
>
> Nah, please don't. Too many Cc's, literally a spam.
Ok.
>
>>> ---
>>> 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.
Your choice, but appreciated.
Best regards
Thomas
>
>
> thanks,
>
> Takashi
--
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
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:
> > ffffe570c1000030
> > [ 42.340063][ T122] #PF: supervisor read access in kernel mode
> > [ 42.340519][ T122] #PF: error_code(0x0000) - not-present page
> > [ 42.340979][ T122] PGD 34c38067 P4D 34c38067 PUD 34c37067 PMD 0
> > [ 42.341456][ T122] Oops: 0000 [#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:ffffb68640207e08 EFLAGS: 00010286
> > [ 42.346749][ T122] RAX: 00000000b3aea8f0 RBX: ffffe570c0f00000 RCX:
> > 0000000000004000
> > [ 42.347355][ T122] RDX: ffffe570c1000000 RSI: 000fffffc0010009 RDI:
> > ffffe570c0f00000
> > [ 42.347960][ T122] RBP: ffffffffc0503050 R08: 0000000000000000 R09:
> > 0000000000000001
> > [ 42.348568][ T122] R10: 0000000000000000 R11: ffffb68640207c88 R12:
> > ffffffffc0503020
> > [ 42.349180][ T122] R13: ffff921281dcdc00 R14: ffff9212bcf08000 R15:
> > ffffe570c0f00000
> > [ 42.349789][ T122] FS: 0000000000000000(0000) GS:ffff9212b3b00000(0000)
> > knlGS:0000000000000000
> > [ 42.350471][ T122] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > [ 42.350975][ T122] CR2: ffffe570c1000030 CR3: 000000001b810000 CR4:
> > 00000000000006e0
> > [ 42.351588][ T122] Call Trace:
> > [ 42.351845][ T122] <TASK>
> > [ 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] </TASK>
> >
> > 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/[email protected]/
No, it's a KVM with a single VGA.
> >> [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.
> >
> > I thought of that, but took a shorter path.
> > OK, let's check whether this keeps working with that change.
>
> I read that cancel_() is not enough and needs to be followed by a
> flush_() to ensure quiescence.
That's not the case, I believe. As long as the code after that cleans
up the pending pages, the cancel_sync alone must be fine.
(Otherwise so many other code paths would hit the problem.)
> So maybe we should call that flush_
> unconditionally.
>
> >>> Reviewed-by: Patrik Jakobsson <[email protected]>
> >>> Cc: <[email protected]>
> >>> Signed-off-by: Takashi Iwai <[email protected]>
> >>
> >> 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?
>
> That's not easily revertable.
Indeed.
> >> Cc: Thomas Zimmermann <[email protected]>
> >> Cc: Javier Martinez Canillas <[email protected]>
> >> Cc: Maarten Lankhorst <[email protected]>
> >> Cc: Maxime Ripard <[email protected]>
> >> Cc: Zack Rusin <[email protected]>
> >> Cc: VMware Graphics Reviewers <[email protected]>
> >> Cc: Jaya Kumar <[email protected]>
> >> Cc: Daniel Vetter <[email protected]>
> >> Cc: "K. Y. Srinivasan" <[email protected]>
> >> Cc: Haiyang Zhang <[email protected]>
> >> Cc: Wei Liu <[email protected]>
> >> Cc: Dexuan Cui <[email protected]>
> >> Cc: Steve Glendinning <[email protected]>
> >> Cc: Bernie Thompson <[email protected]>
> >> Cc: Helge Deller <[email protected]>
> >> Cc: Andy Shevchenko <[email protected]>
> >> Cc: Greg Kroah-Hartman <[email protected]>
> >> Cc: Stephen Kitt <[email protected]>
> >> Cc: Peter Suti <[email protected]>
> >> Cc: Sam Ravnborg <[email protected]>
> >> Cc: Geert Uytterhoeven <[email protected]>
> >> Cc: ye xingchen <[email protected]>
> >> Cc: Petr Mladek <[email protected]>
> >> Cc: John Ogness <[email protected]>
> >> Cc: Tom Rix <[email protected]>
> >> Cc: [email protected]
> >> Cc: [email protected]
> >> Cc: [email protected]
> >> Cc: <[email protected]> # v5.19+
> >
> > Nah, please don't. Too many Cc's, literally a spam.
>
> Ok.
>
> >
> >>> ---
> >>> 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.
>
> Your choice, but appreciated.
Rather a choice by the maintainer, I'd say.
thanks,
Takashi
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 <[email protected]>
> > > Cc: <[email protected]>
> > > Signed-off-by: Takashi Iwai <[email protected]>
> >
> > 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 <[email protected]>
> > Cc: Javier Martinez Canillas <[email protected]>
> > Cc: Maarten Lankhorst <[email protected]>
> > Cc: Maxime Ripard <[email protected]>
> > Cc: Zack Rusin <[email protected]>
> > Cc: VMware Graphics Reviewers <[email protected]>
> > Cc: Jaya Kumar <[email protected]>
> > Cc: Daniel Vetter <[email protected]>
> > Cc: "K. Y. Srinivasan" <[email protected]>
> > Cc: Haiyang Zhang <[email protected]>
> > Cc: Wei Liu <[email protected]>
> > Cc: Dexuan Cui <[email protected]>
> > Cc: Steve Glendinning <[email protected]>
> > Cc: Bernie Thompson <[email protected]>
> > Cc: Helge Deller <[email protected]>
> > Cc: Andy Shevchenko <[email protected]>
> > Cc: Greg Kroah-Hartman <[email protected]>
> > Cc: Stephen Kitt <[email protected]>
> > Cc: Peter Suti <[email protected]>
> > Cc: Sam Ravnborg <[email protected]>
> > Cc: Geert Uytterhoeven <[email protected]>
> > Cc: ye xingchen <[email protected]>
> > Cc: Petr Mladek <[email protected]>
> > Cc: John Ogness <[email protected]>
> > Cc: Tom Rix <[email protected]>
> > Cc: [email protected]
> > Cc: [email protected]
> > Cc: [email protected]
> > Cc: <[email protected]> # 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
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 <[email protected]>
> Cc: <[email protected]>
> Signed-off-by: Takashi Iwai <[email protected]>
> ---
> 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
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 <[email protected]>
> Cc: <[email protected]>
> Signed-off-by: Takashi Iwai <[email protected]>
Reviewed-by: Thomas Zimmermann <[email protected]>
> ---
> 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
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 <[email protected]>
> Cc: <[email protected]>
> Signed-off-by: Takashi Iwai <[email protected]>
> ---
> 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