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 performedThe 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-javierm@xxxxxxxxxx/
[1] https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_fb_helper.c#L2146This 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 <patrik.r.jakobsson@xxxxxxxxx> Cc: <stable@xxxxxxxxxxxxxxx> Signed-off-by: Takashi Iwai <tiwai@xxxxxxx>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 <tzimmermann@xxxxxxx> Cc: Javier Martinez Canillas <javierm@xxxxxxxxxx> Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx> Cc: Maxime Ripard <mripard@xxxxxxxxxx> Cc: Zack Rusin <zackr@xxxxxxxxxx> Cc: VMware Graphics Reviewers <linux-graphics-maintainer@xxxxxxxxxx> Cc: Jaya Kumar <jayalk@xxxxxxxxxxxx> Cc: Daniel Vetter <daniel@xxxxxxxx> Cc: "K. Y. Srinivasan" <kys@xxxxxxxxxxxxx> Cc: Haiyang Zhang <haiyangz@xxxxxxxxxxxxx> Cc: Wei Liu <wei.liu@xxxxxxxxxx> Cc: Dexuan Cui <decui@xxxxxxxxxxxxx> Cc: Steve Glendinning <steve.glendinning@xxxxxxxxxxx> Cc: Bernie Thompson <bernie@xxxxxxxxxxxx> Cc: Helge Deller <deller@xxxxxx> Cc: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> Cc: Stephen Kitt <steve@xxxxxxx> Cc: Peter Suti <peter.suti@xxxxxxxxxxxxxxxxxxx> Cc: Sam Ravnborg <sam@xxxxxxxxxxxx> Cc: Geert Uytterhoeven <geert+renesas@xxxxxxxxx> Cc: ye xingchen <ye.xingchen@xxxxxxxxxx> Cc: Petr Mladek <pmladek@xxxxxxxx> Cc: John Ogness <john.ogness@xxxxxxxxxxxxx> Cc: Tom Rix <trix@xxxxxxxxxx> Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx Cc: linux-fbdev@xxxxxxxxxxxxxxx Cc: linux-hyperv@xxxxxxxxxxxxxxx Cc: <stable@xxxxxxxxxxxxxxx> # 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
Attachment:
OpenPGP_signature
Description: OpenPGP digital signature
