On Wed, Mar 01, 2017 at 06:50:08PM -0300, Gabriel Krisman Bertazi wrote:
> Daniel Vetter <daniel@xxxxxxxx> writes:
>
> Hi Daniel, thanks again for your review.
>
> > Hm, I bit silly that we need #ifdef here, all this stuff is meant to Just
> > Work. Can't we just fix the oops with suitable checks in the fini paths
> > instead?
>
> Heh. I expected someone would bring up this objection. My rationale is
> that on the driver side, and for many drivers, fbdev emulation code is
> always executed, even if it is only calling a bunch of NOP functions on
> the drm core, which is a bit silly. I wonder if struct drm_driver could
> have a new callback for fbdev_initialization, which we could avoid
> calling in drm_core (maybe at registering time) if FBDEV_EMULATION is
> disabled. (well, this would go a bit in the opposite direction of having
> open coded probe sequences).
>
> Anyway, for the Oops I mentioned, can you consider the patch below,
> instead?
lgtm.
-Daniel
>
> >
> > Also 0day has hit some oops in bochs, it might suffer from similar bugs.
>
> I can take a look at that too.
>
> -- >8 --
> Subject: [PATCH] drm: qxl: Don't clean up uninitialized fbdev framebuffer
>
> If fbdev emulation is disabled, the QXL shutdown path still tries to
> clean the framebuffer, which wasn't initialized, hitting the Oops below.
> We can check for the qfb->obj element to ensure the fb was initialized,
> because the qfb always has a bo object associated with it since
> initialization time.
>
> [ 24.284684] BUG: unable to handle kernel NULL pointer dereference at 00000000000002e0
> [ 24.285627] IP: mutex_lock+0x18/0x30
> [ 24.286049] PGD 78cdf067
> [ 24.286050] PUD 7940f067
> [ 24.286344] PMD 0
> [ 24.286649]
> [ 24.287072] Oops: 0002 [#1] SMP
> [ 24.287422] Modules linked in: qxl
> [ 24.287806] CPU: 0 PID: 2328 Comm: bash Not tainted 4.10.0-rc5+ #97
> [ 24.288515] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.9.3-20161025_171302-gandalf 04/01/2014
> [ 24.289681] task: ffff88007c4c0000 task.stack: ffffc90001b58000
> [ 24.290354] RIP: 0010:mutex_lock+0x18/0x30
> [ 24.290812] RSP: 0018:ffffc90001b5bcb0 EFLAGS: 00010246
> [ 24.291401] RAX: 0000000000000000 RBX: 00000000000002e0 RCX: 0000000000000000
> [ 24.292209] RDX: ffff88007c4c0000 RSI: 0000000000000001 RDI: 00000000000002e0
> [ 24.292987] RBP: ffffc90001b5bcb8 R08: fffffffffffffffe R09: 0000000000000001
> [ 24.293797] R10: ffff880078d80b80 R11: 0000000000011400 R12: 0000000000000000
> [ 24.294601] R13: 00000000000002e0 R14: ffffffffa0009c28 R15: 0000000000000060
> [ 24.295439] FS: 00007f30e3acbb40(0000) GS:ffff88007fc00000(0000) knlGS:0000000000000000
> [ 24.296364] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 24.296997] CR2: 00000000000002e0 CR3: 0000000078c7b000 CR4: 00000000000006f0
> [ 24.297813] Call Trace:
> [ 24.298097] drm_framebuffer_cleanup+0x1f/0x70
> [ 24.298612] qxl_fbdev_fini+0x68/0x90 [qxl]
> [ 24.299074] qxl_modeset_fini+0xd/0x30 [qxl]
> [ 24.299562] qxl_pci_remove+0x22/0x50 [qxl]
> [ 24.300025] pci_device_remove+0x34/0xb0
> [ 24.300507] device_release_driver_internal+0x150/0x200
> [ 24.301082] device_release_driver+0xd/0x10
> [ 24.301587] unbind_store+0x108/0x150
> [ 24.301993] drv_attr_store+0x20/0x30
> [ 24.302402] sysfs_kf_write+0x32/0x40
> [ 24.302827] kernfs_fop_write+0x108/0x190
> [ 24.303269] __vfs_write+0x23/0x120
> [ 24.303678] ? security_file_permission+0x36/0xb0
> [ 24.304193] ? rw_verify_area+0x49/0xb0
> [ 24.304636] vfs_write+0xb0/0x190
> [ 24.305004] SyS_write+0x41/0xa0
> [ 24.305362] entry_SYSCALL_64_fastpath+0x1a/0xa9
> [ 24.305887] RIP: 0033:0x7f30e31d9620
> [ 24.306285] RSP: 002b:00007ffc54b47e68 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
> [ 24.307128] RAX: ffffffffffffffda RBX: 00007f30e3497600 RCX: 00007f30e31d9620
> [ 24.307928] RDX: 000000000000000d RSI: 0000000000da2008 RDI: 0000000000000001
> [ 24.308727] RBP: 000000000070bc60 R08: 00007f30e3498760 R09: 00007f30e3acbb40
> [ 24.309504] R10: 0000000000000073 R11: 0000000000000246 R12: 0000000000000001
> [ 24.310295] R13: 0000000000000000 R14: 0000000000000000 R15: 00007ffc54b47f34
> [ 24.311095] Code: 0e 01 e9 7b fe ff ff 66 90 66 2e 0f 1f 84 00 00 00 00 00
> 55 48 89 e5 53 48 89 fb e8 83 e8 ff ff 65 48 8b 14 25 40 c4 00 00 31 c0 <3e>
> 48 0f b1 13 48 85 c0 74 08 48 89 df e8 66 fd ff ff 5b 5d c3
> [ 24.313182] RIP: mutex_lock+0x18/0x30 RSP: ffffc90001b5bcb0
> [ 24.313811] CR2: 00000000000002e0
> [ 24.314208] ---[ end trace 29669c1593cae14b ]---
>
> Signed-off-by: Gabriel Krisman Bertazi <krisman@xxxxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/qxl/qxl_fb.c | 11 +++++++----
> 1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/qxl/qxl_fb.c b/drivers/gpu/drm/qxl/qxl_fb.c
> index 35124737666e..4d6c311e8e5e 100644
> --- a/drivers/gpu/drm/qxl/qxl_fb.c
> +++ b/drivers/gpu/drm/qxl/qxl_fb.c
> @@ -351,13 +351,16 @@ static int qxl_fbdev_destroy(struct drm_device *dev, struct qxl_fbdev *qfbdev)
>
> drm_fb_helper_unregister_fbi(&qfbdev->helper);
>
> - if (qfb->obj) {
> + if (qfb->obj)
> qxlfb_destroy_pinned_object(qfb->obj);
> - qfb->obj = NULL;
> - }
> +
> drm_fb_helper_fini(&qfbdev->helper);
> vfree(qfbdev->shadow);
> - drm_framebuffer_cleanup(&qfb->base);
> +
> + if (qfb->obj)
> + drm_framebuffer_cleanup(&qfb->base);
btw if you're looking for more cleanup work: Embeddeding drm_framebuffer
like this is deprecated, since it wreaks the refcounting. There's comments
around the relevant functions, and iirc we also have a todo somewhere.
-Daniel
> +
> + qfb->obj = NULL;
>
> return 0;
> }
> --
> 2.11.0
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
