From mboxrd@z Thu Jan 1 00:00:00 1970 From: Peter Hurley Subject: [PATCH v2 09/11] tty: Strengthen no-subsequent-use guarantee of tty_ldisc_halt() Date: Fri, 14 Dec 2012 13:22:48 -0500 Message-ID: <1355509370-5883-10-git-send-email-peter@hurleysoftware.com> References: <1355509370-5883-1-git-send-email-peter@hurleysoftware.com> Return-path: Received: from mailout39.mail01.mtsvc.net ([216.70.64.83]:54471 "EHLO n12.mail01.mtsvc.net" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1756980Ab2LNSXj (ORCPT ); Fri, 14 Dec 2012 13:23:39 -0500 In-Reply-To: <1355509370-5883-1-git-send-email-peter@hurleysoftware.com> Sender: linux-serial-owner@vger.kernel.org List-Id: linux-serial@vger.kernel.org To: Alan Cox , Jiri Slaby Cc: linux-serial@vger.kernel.org, Greg Kroah-Hartman , Ilya Zykov , Sasha Levin , linux-kernel@vger.kernel.org, Peter Hurley In preparation for destructing and freeing the tty, the line discipline must first be brought to an inactive state before it can be destructed. This line discipline shutdown must: - disallow new users of the ldisc - wait for existing ldisc users to finish - only then, cancel/flush their pending/running work Factor tty_ldisc_wait_idle() from tty_set_ldisc() and tty_ldisc_kill() to ensure this shutdown order. Failure to provide this guarantee can result in scheduled work running after the tty has already been freed, as indicated in the following log message: [ 88.331234] WARNING: at /home/peter/src/kernels/next/drivers/tty/tty_buffer.c:435 flush_to_ldisc+0x194/0x1d0() [ 88.334505] Hardware name: Bochs [ 88.335618] tty is bad=-1 [ 88.335703] Modules linked in: netconsole configfs bnep rfcomm bluetooth ...... [ 88.345272] Pid: 39, comm: kworker/1:1 Tainted: G W 3.7.0-next-20121129+ttydebug-xeon #20121129+ttydebug [ 88.347736] Call Trace: [ 88.349024] [] warn_slowpath_common+0x7f/0xc0 [ 88.350383] [] warn_slowpath_fmt+0x46/0x50 [ 88.351745] [] flush_to_ldisc+0x194/0x1d0 [ 88.353047] [] ? _raw_spin_unlock_irq+0x21/0x50 [ 88.354190] [] ? finish_task_switch+0x49/0xe0 [ 88.355436] [] process_one_work+0x121/0x490 [ 88.357674] [] ? __tty_buffer_flush+0x90/0x90 [ 88.358954] [] worker_thread+0x164/0x3e0 [ 88.360247] [] ? manage_workers+0x120/0x120 [ 88.361282] [] kthread+0xc0/0xd0 [ 88.362284] [] ? cmos_do_probe+0x2eb/0x3bf [ 88.363391] [] ? flush_kthread_worker+0xb0/0xb0 [ 88.364797] [] ret_from_fork+0x7c/0xb0 [ 88.366087] [] ? flush_kthread_worker+0xb0/0xb0 [ 88.367266] ---[ end trace 453a7c9f38fbfec0 ]--- Signed-off-by: Peter Hurley --- drivers/tty/tty_ldisc.c | 35 +++++++++++++++++++++-------------- 1 file changed, 21 insertions(+), 14 deletions(-) diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c index c3dec37..9f4c7b0 100644 --- a/drivers/tty/tty_ldisc.c +++ b/drivers/tty/tty_ldisc.c @@ -528,24 +528,38 @@ static int tty_ldisc_wait_idle(struct tty_struct *tty, long timeout) /** * tty_ldisc_halt - shut down the line discipline * @tty: tty device + * @pending: returns true if work was scheduled when cancelled + * (can be set to NULL) + * @timeout: # of jiffies to wait for ldisc refs to be released * * Shut down the line discipline and work queue for this tty device. * The TTY_LDISC flag being cleared ensures no further references can * be obtained while the delayed work queue halt ensures that no more * data is fed to the ldisc. * + * Furthermore, guarantee that existing ldisc references have been + * released, which in turn, guarantees that no future buffer work + * can be rescheduled. + * * You need to do a 'flush_scheduled_work()' (outside the ldisc_mutex) * in order to make sure any currently executing ldisc work is also * flushed. */ -static int tty_ldisc_halt(struct tty_struct *tty) +static int tty_ldisc_halt(struct tty_struct *tty, int *pending, long timeout) { - int scheduled; + int scheduled, retval; + clear_bit(TTY_LDISC, &tty->flags); + retval = tty_ldisc_wait_idle(tty, timeout); + if (retval) + return retval; + scheduled = cancel_work_sync(&tty->port->buf.work); set_bit(TTY_LDISC_HALTED, &tty->flags); - return scheduled; + if (pending) + *pending = scheduled; + return 0; } /** @@ -687,9 +701,9 @@ int tty_set_ldisc(struct tty_struct *tty, int ldisc) * parallel to the change and re-referencing the tty. */ - work = tty_ldisc_halt(tty); + retval = tty_ldisc_halt(tty, &work, 5 * HZ); if (o_tty) - o_work = tty_ldisc_halt(o_tty); + tty_ldisc_halt(o_tty, &o_work, 0); /* * Wait for ->hangup_work and ->buf.work handlers to terminate. @@ -700,8 +714,6 @@ int tty_set_ldisc(struct tty_struct *tty, int ldisc) tty_ldisc_flush_works(tty); - retval = tty_ldisc_wait_idle(tty, 5 * HZ); - tty_lock(tty); mutex_lock(&tty->ldisc_mutex); @@ -920,11 +932,6 @@ int tty_ldisc_setup(struct tty_struct *tty, struct tty_struct *o_tty) static void tty_ldisc_kill(struct tty_struct *tty) { - /* There cannot be users from userspace now. But there still might be - * drivers holding a reference via tty_ldisc_ref. Do not steal them the - * ldisc until they are done. */ - tty_ldisc_wait_idle(tty, MAX_SCHEDULE_TIMEOUT); - mutex_lock(&tty->ldisc_mutex); /* * Now kill off the ldisc @@ -958,10 +965,10 @@ void tty_ldisc_release(struct tty_struct *tty, struct tty_struct *o_tty) */ tty_lock_pair(tty, o_tty); - tty_ldisc_halt(tty); + tty_ldisc_halt(tty, NULL, MAX_SCHEDULE_TIMEOUT); tty_ldisc_flush_works(tty); if (o_tty) { - tty_ldisc_halt(o_tty); + tty_ldisc_halt(o_tty, NULL, MAX_SCHEDULE_TIMEOUT); tty_ldisc_flush_works(o_tty); } -- 1.8.0.1