[PATCH 1/2] drivers/media/dvb-core/en50221: use kref to manage struct dvb_ca_private

[PATCH 1/2] drivers/media/dvb-core/en50221: use kref to manage struct dvb_ca_private

[Max Kellermann]

Don't free the object until the file handle has been closed.  Fixes
use-after-free bug which occurs when I disconnect my DVB-S received
while VDR is running.

This is a crash dump of such a use-after-free:

    general protection fault: 0000 [#1] SMP
    CPU: 0 PID: 2541 Comm: CI adapter on d Not tainted 4.7.0-rc1-hosting+ #49
    Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
    task: ffff880027d7ce00 ti: ffff88003d8f8000 task.ti: ffff88003d8f8000
    RIP: 0010:[<ffffffff812f3d1f>]  [<ffffffff812f3d1f>] dvb_ca_en50221_io_read_condition.isra.7+0x6f/0x150
    RSP: 0018:ffff88003d8fba98  EFLAGS: 00010206
    RAX: 0000000059534255 RBX: 000000753d470f90 RCX: ffff88003c74d181
    RDX: 00000001bea04ba9 RSI: ffff88003d8fbaf4 RDI: 3a3030a56d763fc0
    RBP: ffff88003d8fbae0 R08: ffff88003c74d180 R09: 0000000000000000
    R10: 0000000000000001 R11: 0000000000000000 R12: ffff88003c480e00
    R13: 00000000ffffffff R14: 0000000059534255 R15: 0000000000000000
    FS:  00007fb4209b4700(0000) GS:ffff88003fc00000(0000) knlGS:0000000000000000
    CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
    CR2: 00007f06445f4078 CR3: 000000003c55b000 CR4: 00000000000006b0
    Stack:
     ffff88003d8fbaf4 000000003c2170c0 0000000000004000 0000000000000000
     ffff88003c480e00 ffff88003d8fbc80 ffff88003c74d180 ffff88003d8fbb8c
     0000000000000000 ffff88003d8fbb10 ffffffff812f3e37 ffff88003d8fbb00
    Call Trace:
     [<ffffffff812f3e37>] dvb_ca_en50221_io_poll+0x37/0xa0
     [<ffffffff8113109b>] do_sys_poll+0x2db/0x520

This is a backtrace of the kernel attempting to lock a freed mutex:

    #0  0xffffffff81083d40 in rep_nop () at ./arch/x86/include/asm/processor.h:569
    #1  cpu_relax () at ./arch/x86/include/asm/processor.h:574
    #2  virt_spin_lock (lock=<optimized out>) at ./arch/x86/include/asm/qspinlock.h:57
    #3  native_queued_spin_lock_slowpath (lock=0xffff88003c480e90, val=761492029) at kernel/locking/qspinlock.c:304
    #4  0xffffffff810d1a06 in pv_queued_spin_lock_slowpath (val=<optimized out>, lock=<optimized out>) at ./arch/x86/include/asm/paravirt.h:669
    #5  queued_spin_lock_slowpath (val=<optimized out>, lock=<optimized out>) at ./arch/x86/include/asm/qspinlock.h:28
    #6  queued_spin_lock (lock=<optimized out>) at include/asm-generic/qspinlock.h:107
    #7  __mutex_lock_common (use_ww_ctx=<optimized out>, ww_ctx=<optimized out>, ip=<optimized out>, nest_lock=<optimized out>, subclass=<optimized out>,
        state=<optimized out>, lock=<optimized out>) at kernel/locking/mutex.c:526
    #8  mutex_lock_interruptible_nested (lock=0xffff88003c480e88, subclass=<optimized out>) at kernel/locking/mutex.c:647
    #9  0xffffffff812f49fe in dvb_ca_en50221_io_do_ioctl (file=<optimized out>, cmd=761492029, parg=0x1 <irq_stack_union+1>)
        at drivers/media/dvb-core/dvb_ca_en50221.c:1210
    #10 0xffffffff812ee660 in dvb_usercopy (file=<optimized out>, cmd=761492029, arg=<optimized out>, func=<optimized out>) at drivers/media/dvb-core/dvbdev.c:883
    #11 0xffffffff812f3410 in dvb_ca_en50221_io_ioctl (file=<optimized out>, cmd=<optimized out>, arg=<optimized out>) at drivers/media/dvb-core/dvb_ca_en50221.c:1284
    #12 0xffffffff8112eddd in vfs_ioctl (arg=<optimized out>, cmd=<optimized out>, filp=<optimized out>) at fs/ioctl.c:43
    #13 do_vfs_ioctl (filp=0xffff88003c480e90, fd=<optimized out>, cmd=<optimized out>, arg=<optimized out>) at fs/ioctl.c:674
    #14 0xffffffff8112f30c in SYSC_ioctl (arg=<optimized out>, cmd=<optimized out>, fd=<optimized out>) at fs/ioctl.c:689
    #15 SyS_ioctl (fd=6, cmd=2148298626, arg=140734533693696) at fs/ioctl.c:680
    #16 0xffffffff8103feb2 in entry_SYSCALL_64 () at arch/x86/entry/entry_64.S:207

Signed-off-by: Max Kellermann <max@duempel.org>
---
 drivers/media/dvb-core/dvb_ca_en50221.c |   24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/drivers/media/dvb-core/dvb_ca_en50221.c b/drivers/media/dvb-core/dvb_ca_en50221.c
index b1e3a26..b5b5b19 100644
--- a/drivers/media/dvb-core/dvb_ca_en50221.c
+++ b/drivers/media/dvb-core/dvb_ca_en50221.c
@@ -123,6 +123,7 @@ struct dvb_ca_slot {
 
 /* Private CA-interface information */
 struct dvb_ca_private {
+	struct kref refcount;
 
 	/* pointer back to the public data structure */
 	struct dvb_ca_en50221 *pub;
@@ -173,6 +174,22 @@ static void dvb_ca_private_free(struct dvb_ca_private *ca)
 	kfree(ca);
 }
 
+static void dvb_ca_private_release(struct kref *ref)
+{
+	struct dvb_ca_private *ca = container_of(ref, struct dvb_ca_private, refcount);
+	dvb_ca_private_free(ca);
+}
+
+static void dvb_ca_private_get(struct dvb_ca_private *ca)
+{
+	kref_get(&ca->refcount);
+}
+
+static void dvb_ca_private_put(struct dvb_ca_private *ca)
+{
+	kref_put(&ca->refcount, dvb_ca_private_release);
+}
+
 static void dvb_ca_en50221_thread_wakeup(struct dvb_ca_private *ca);
 static int dvb_ca_en50221_read_data(struct dvb_ca_private *ca, int slot, u8 * ebuf, int ecount);
 static int dvb_ca_en50221_write_data(struct dvb_ca_private *ca, int slot, u8 * ebuf, int ecount);
@@ -1570,6 +1587,8 @@ static int dvb_ca_en50221_io_open(struct inode *inode, struct file *file)
 	dvb_ca_en50221_thread_update_delay(ca);
 	dvb_ca_en50221_thread_wakeup(ca);
 
+	dvb_ca_private_get(ca);
+
 	return 0;
 }
 
@@ -1598,6 +1617,8 @@ static int dvb_ca_en50221_io_release(struct inode *inode, struct file *file)
 
 	module_put(ca->pub->owner);
 
+	dvb_ca_private_put(ca);
+
 	return err;
 }
 
@@ -1693,6 +1714,7 @@ int dvb_ca_en50221_init(struct dvb_adapter *dvb_adapter,
 		ret = -ENOMEM;
 		goto exit;
 	}
+	kref_init(&ca->refcount);
 	ca->pub = pubca;
 	ca->flags = flags;
 	ca->slot_count = slot_count;
@@ -1772,6 +1794,6 @@ void dvb_ca_en50221_release(struct dvb_ca_en50221 *pubca)
 	for (i = 0; i < ca->slot_count; i++) {
 		dvb_ca_en50221_slot_shutdown(ca, i);
 	}
-	dvb_ca_private_free(ca);
+	dvb_ca_private_put(ca);
 	pubca->private = NULL;
 }

[PATCH 2/2] dvb_frontend: eliminate blocking wait in dvb_unregister_frontend()

[Max Kellermann]

The wait_event() call in dvb_unregister_frontend() waits synchronously
for other tasks to free a file descriptor, but it does that while
holding several mutexes.  That alone is a bad idea, but if one user
process happens to keep a (defunct) file descriptor open indefinitely,
the kernel will correctly detect a hung task:

    INFO: task kworker/0:1:314 blocked for more than 30 seconds.
          Not tainted 4.7.0-rc1-hosting+ #50
    "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
    kworker/0:1     D ffff88003daf7a50     0   314      2 0x00000000
    Workqueue: usb_hub_wq hub_event
     ffff88003daf7a50 0000000000000296 ffff88003daf7a30 ffff88003fc13f98
     ffff88003dadce00 ffff88003daf8000 ffff88003e3fc010 ffff88003d48d4f8
     ffff88003e3b5030 ffff88003e3f8898 ffff88003daf7a68 ffffffff810cf860
    Call Trace:
     [<ffffffff810cf860>] schedule+0x30/0x80
     [<ffffffff812f88d3>] dvb_unregister_frontend+0x93/0xc0
     [<ffffffff8107a000>] ? __wake_up_common+0x80/0x80
     [<ffffffff813019c7>] dvb_usb_adapter_frontend_exit+0x37/0x70
     [<ffffffff81300614>] dvb_usb_exit+0x34/0xb0
     [<ffffffff81300d4a>] dvb_usb_device_exit+0x3a/0x50
     [<ffffffff81302dc2>] pctv452e_usb_disconnect+0x52/0x60
     [<ffffffff81295a07>] usb_unbind_interface+0x67/0x1e0
     [<ffffffff810609f3>] ? __blocking_notifier_call_chain+0x53/0x70
     [<ffffffff8127ba67>] __device_release_driver+0x77/0x110
     [<ffffffff8127c2d3>] device_release_driver+0x23/0x30
     [<ffffffff8127ab5d>] bus_remove_device+0x10d/0x150
     [<ffffffff8127879b>] device_del+0x13b/0x260
     [<ffffffff81299dea>] ? usb_remove_ep_devs+0x1a/0x30
     [<ffffffff8129468e>] usb_disable_device+0x9e/0x1e0
     [<ffffffff8128bb09>] usb_disconnect+0x89/0x260
     [<ffffffff8128db8d>] hub_event+0x30d/0xfc0
     [<ffffffff81059475>] process_one_work+0x1c5/0x4a0
     [<ffffffff8105940c>] ? process_one_work+0x15c/0x4a0
     [<ffffffff81059799>] worker_thread+0x49/0x480
     [<ffffffff81059750>] ? process_one_work+0x4a0/0x4a0
     [<ffffffff81059750>] ? process_one_work+0x4a0/0x4a0
     [<ffffffff8105f65e>] kthread+0xee/0x110
     [<ffffffff810400bf>] ret_from_fork+0x1f/0x40
     [<ffffffff8105f570>] ? __kthread_unpark+0x70/0x70
    5 locks held by kworker/0:1/314:
     #0:  ("usb_hub_wq"){......}, at: [<ffffffff8105940c>] process_one_work+0x15c/0x4a0
     #1:  ((&hub->events)){......}, at: [<ffffffff8105940c>] process_one_work+0x15c/0x4a0
     #2:  (&dev->mutex){......}, at: [<ffffffff8128d8cb>] hub_event+0x4b/0xfc0
     #3:  (&dev->mutex){......}, at: [<ffffffff8128bad2>] usb_disconnect+0x52/0x260
     #4:  (&dev->mutex){......}, at: [<ffffffff8127c2cb>] device_release_driver+0x1b/0x30

This patch removes the blocking wait, and postpones the kfree() call
until all file handles have been closed by using struct kref.

Signed-off-by: Max Kellermann <max@duempel.org>
---
 drivers/media/dvb-core/dvb_frontend.c |   33 +++++++++++++++++++++++++--------
 1 file changed, 25 insertions(+), 8 deletions(-)

diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c
index c014261..be99c8d 100644
--- a/drivers/media/dvb-core/dvb_frontend.c
+++ b/drivers/media/dvb-core/dvb_frontend.c
@@ -99,6 +99,7 @@ MODULE_PARM_DESC(dvb_mfe_wait_time, "Wait up to <mfe_wait_time> seconds on open(
 static DEFINE_MUTEX(frontend_mutex);
 
 struct dvb_frontend_private {
+	struct kref refcount;
 
 	/* thread/frontend values */
 	struct dvb_device *dvbdev;
@@ -137,6 +138,23 @@ struct dvb_frontend_private {
 #endif
 };
 
+static void dvb_frontend_private_free(struct kref *ref)
+{
+	struct dvb_frontend_private *fepriv =
+		container_of(ref, struct dvb_frontend_private, refcount);
+	kfree(fepriv);
+}
+
+static void dvb_frontend_private_put(struct dvb_frontend_private *fepriv)
+{
+	kref_put(&fepriv->refcount, dvb_frontend_private_free);
+}
+
+static void dvb_frontend_private_get(struct dvb_frontend_private *fepriv)
+{
+	kref_get(&fepriv->refcount);
+}
+
 static void dvb_frontend_wakeup(struct dvb_frontend *fe);
 static int dtv_get_frontend(struct dvb_frontend *fe,
 			    struct dtv_frontend_properties *c,
@@ -2543,6 +2561,8 @@ static int dvb_frontend_open(struct inode *inode, struct file *file)
 		fepriv->events.eventr = fepriv->events.eventw = 0;
 	}
 
+	dvb_frontend_private_get(fepriv);
+
 	if (adapter->mfe_shared)
 		mutex_unlock (&adapter->mfe_lock);
 	return ret;
@@ -2591,6 +2611,8 @@ static int dvb_frontend_release(struct inode *inode, struct file *file)
 			fe->ops.ts_bus_ctrl(fe, 0);
 	}
 
+	dvb_frontend_private_put(fepriv);
+
 	return ret;
 }
 
@@ -2679,6 +2701,8 @@ int dvb_register_frontend(struct dvb_adapter* dvb,
 	}
 	fepriv = fe->frontend_priv;
 
+	kref_init(&fepriv->refcount);
+
 	sema_init(&fepriv->sem, 1);
 	init_waitqueue_head (&fepriv->wait_queue);
 	init_waitqueue_head (&fepriv->events.wait_queue);
@@ -2713,18 +2737,11 @@ int dvb_unregister_frontend(struct dvb_frontend* fe)
 
 	mutex_lock(&frontend_mutex);
 	dvb_frontend_stop (fe);
-	mutex_unlock(&frontend_mutex);
-
-	if (fepriv->dvbdev->users < -1)
-		wait_event(fepriv->dvbdev->wait_queue,
-				fepriv->dvbdev->users==-1);
-
-	mutex_lock(&frontend_mutex);
 	dvb_unregister_device (fepriv->dvbdev);
 
 	/* fe is invalid now */
-	kfree(fepriv);
 	mutex_unlock(&frontend_mutex);
+	dvb_frontend_private_put(fepriv);
 	return 0;
 }
 EXPORT_SYMBOL(dvb_unregister_frontend);