[PATCH] drm: qxl: Don't clean up uninitialized fbdev framebuffer

Thu Mar 2 07:09:35 UTC 2017

On Wed, Mar 01, 2017 at 06:50:08PM -0300, Gabriel Krisman Bertazi wrote:
> Daniel Vetter <daniel at ffwll.ch> 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 at collabora.co.uk>
> ---
>  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