Now that tty_ldisc_assign() is a one-line file-scoped function,
remove it and perform the simple assignment at its call sites.
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Merge get_ldisc() into its only call site.
Note how, after merging, the unsafe acquire of an ldisc reference
is obvious.
CPU 0 in tty_ldisc_try() | CPU 1 in tty_ldisc_halt()
|
test_bit(TTY_LDISC, &tty_flags) |
if (true) | clear_bit(TTY_LDISC, &tty_flags)
tty->ldisc != 0? | atomic_read(&tty->ldisc->users)
if (true) | ret_val == 1?
atomic_inc(&tty->ldisc->users) | if (false)
| wait
|
<goes on assuming safe ldisc use> | <doesn't wait - proceeds w/ close>
|
The spin lock in tty_ldisc_try() does nothing wrt synchronizing
the ldisc halt since it's not acquired as part of halting.
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
tty_ldisc_ref()/tty_ldisc_unref() have usage semantics
equivalent to down_read_trylock()/up_read(). Only
callers of tty_ldisc_put() are performing the additional
operations necessary for proper ldisc teardown, and then only
after ensuring no outstanding 'read lock' remains.
Thus, tty_ldisc_unref() should never be the last reference;
WARN if it is. Conversely, tty_ldisc_put() should never be
destructing if the use count != 1.
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
This comment is a victim of code migration from
"tty: Fix the ldisc hangup race"; re-parent it.
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
The tty core relies on the ldisc layer for synchronizing destruction
of the tty. Instead, the final tty release must wait for any pending tty
work to complete prior to tty destruction.
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Waiting for buffer work to complete is not required for safely
performing changes to the line discipline, once the line discipline
is halted. The buffer work routine, flush_to_ldisc(), will be
unable to acquire an ldisc ref and all existing references were
waited until released (so it can't already have one).
Ensure running buffer work which may reference the soon-to-be-gone
tty completes and any buffer work running after this point retrieves
a NULL tty.
Also, ensure all buffer work is cancelled on port destruction.
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
tty_ldisc_hangup() guarantees the ldisc is enabled (or that there
is no ldisc). Since __tty_hangup() was the only user, re-define
tty_ldisc_enable() in file-scope.
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
SAK work may schedule hangup work (if TTY_SOFT_SAK is defined), thus
SAK work must be flushed before hangup work.
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
The pty driver does not obtain an ldisc reference to the linked
tty when writing. When the ldiscs are sequentially halted, it
is possible for one ldisc to be halted, and before the second
ldisc can be halted, a concurrent write schedules buffer work on
the first ldisc. This can lead to an access-after-free error when
the scheduled buffer work starts on the closed ldisc.
Prevent subsequent use after halt by performing each stage
of the halt on both ttys.
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
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 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] [<ffffffff81058aff>] warn_slowpath_common+0x7f/0xc0
[ 88.350383] [<ffffffff81058bf6>] warn_slowpath_fmt+0x46/0x50
[ 88.351745] [<ffffffff81432bd4>] flush_to_ldisc+0x194/0x1d0
[ 88.353047] [<ffffffff816f7fe1>] ? _raw_spin_unlock_irq+0x21/0x50
[ 88.354190] [<ffffffff8108a809>] ? finish_task_switch+0x49/0xe0
[ 88.355436] [<ffffffff81077ad1>] process_one_work+0x121/0x490
[ 88.357674] [<ffffffff81432a40>] ? __tty_buffer_flush+0x90/0x90
[ 88.358954] [<ffffffff81078c84>] worker_thread+0x164/0x3e0
[ 88.360247] [<ffffffff81078b20>] ? manage_workers+0x120/0x120
[ 88.361282] [<ffffffff8107e230>] kthread+0xc0/0xd0
[ 88.362284] [<ffffffff816f0000>] ? cmos_do_probe+0x2eb/0x3bf
[ 88.363391] [<ffffffff8107e170>] ? flush_kthread_worker+0xb0/0xb0
[ 88.364797] [<ffffffff816fff6c>] ret_from_fork+0x7c/0xb0
[ 88.366087] [<ffffffff8107e170>] ? flush_kthread_worker+0xb0/0xb0
[ 88.367266] ---[ end trace 453a7c9f38fbfec0 ]---
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
tty_ldisc_halt() will use the file-scoped function, tty_ldisc_wait_idle(),
in the following patch.
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Flip buffer work cannot be cancelled until all outstanding ldisc
references have been released. Convert the ldisc ref wait into
a full ldisc halt with buffer work cancellation.
Note that the legacy mutex is not held while cancelling.
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Since the tty->ldisc is prevented from being changed by tty_set_ldisc()
when a tty is being hung up, re-testing the ldisc user count is
unnecessary -- ie, it cannot be a different ldisc and the user count
cannot have increased (assuming the caller meets the precondition that
TTY_LDISC flag is cleared)
Removal of the 'early-out' locking optimization is necessary for
the subsequent patch 'tty: Fix ldisc halt sequence on hangup'.
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Refactor tty_ldisc_hangup() to extract standalone function,
tty_ldisc_hangup_wait_idle(), to wait for ldisc references
to be released.
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Flip buffer work must not be scheduled by the line discipline
after the line discipline has been halted; issue warning.
Note: drivers can still schedule flip buffer work.
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Since commit 89c8d91e31 ("tty: localise the lock") I see a dead lock
in one of my dummy_hcd + g_nokia test cases. The first run was usually
okay, the second often resulted in a splat by lockdep and the third was
usually a dead lock.
Lockdep complained about tty->hangup_work and tty->legacy_mutex taken
both ways:
| ======================================================
| [ INFO: possible circular locking dependency detected ]
| 3.7.0-rc6+ #204 Not tainted
| -------------------------------------------------------
| kworker/2:1/35 is trying to acquire lock:
| (&tty->legacy_mutex){+.+.+.}, at: [<c14051e6>] tty_lock_nested+0x36/0x80
|
| but task is already holding lock:
| ((&tty->hangup_work)){+.+...}, at: [<c104f6e4>] process_one_work+0x124/0x5e0
|
| which lock already depends on the new lock.
|
| the existing dependency chain (in reverse order) is:
|
| -> #2 ((&tty->hangup_work)){+.+...}:
| [<c107fe74>] lock_acquire+0x84/0x190
| [<c104d82d>] flush_work+0x3d/0x240
| [<c12e6986>] tty_ldisc_flush_works+0x16/0x30
| [<c12e7861>] tty_ldisc_release+0x21/0x70
| [<c12e0dfc>] tty_release+0x35c/0x470
| [<c1105e28>] __fput+0xd8/0x270
| [<c1105fcd>] ____fput+0xd/0x10
| [<c1051dd9>] task_work_run+0xb9/0xf0
| [<c1002a51>] do_notify_resume+0x51/0x80
| [<c140550a>] work_notifysig+0x35/0x3b
|
| -> #1 (&tty->legacy_mutex/1){+.+...}:
| [<c107fe74>] lock_acquire+0x84/0x190
| [<c140276c>] mutex_lock_nested+0x6c/0x2f0
| [<c14051e6>] tty_lock_nested+0x36/0x80
| [<c1405279>] tty_lock_pair+0x29/0x70
| [<c12e0bb8>] tty_release+0x118/0x470
| [<c1105e28>] __fput+0xd8/0x270
| [<c1105fcd>] ____fput+0xd/0x10
| [<c1051dd9>] task_work_run+0xb9/0xf0
| [<c1002a51>] do_notify_resume+0x51/0x80
| [<c140550a>] work_notifysig+0x35/0x3b
|
| -> #0 (&tty->legacy_mutex){+.+.+.}:
| [<c107f3c9>] __lock_acquire+0x1189/0x16a0
| [<c107fe74>] lock_acquire+0x84/0x190
| [<c140276c>] mutex_lock_nested+0x6c/0x2f0
| [<c14051e6>] tty_lock_nested+0x36/0x80
| [<c140523f>] tty_lock+0xf/0x20
| [<c12df8e4>] __tty_hangup+0x54/0x410
| [<c12dfcb2>] do_tty_hangup+0x12/0x20
| [<c104f763>] process_one_work+0x1a3/0x5e0
| [<c104fec9>] worker_thread+0x119/0x3a0
| [<c1055084>] kthread+0x94/0xa0
| [<c140ca37>] ret_from_kernel_thread+0x1b/0x28
|
|other info that might help us debug this:
|
|Chain exists of:
| &tty->legacy_mutex --> &tty->legacy_mutex/1 --> (&tty->hangup_work)
|
| Possible unsafe locking scenario:
|
| CPU0 CPU1
| ---- ----
| lock((&tty->hangup_work));
| lock(&tty->legacy_mutex/1);
| lock((&tty->hangup_work));
| lock(&tty->legacy_mutex);
|
| *** DEADLOCK ***
Before the path mentioned tty_ldisc_release() look like this:
| tty_ldisc_halt(tty);
| tty_ldisc_flush_works(tty);
| tty_lock();
As it can be seen, it first flushes the workqueue and then grabs the
tty_lock. Now we grab the lock first:
| tty_lock_pair(tty, o_tty);
| tty_ldisc_halt(tty);
| tty_ldisc_flush_works(tty);
so lockdep's complaint seems valid.
The earlier version of this patch took the ldisc_mutex since the other
user of tty_ldisc_flush_works() (tty_set_ldisc()) did this.
Peter Hurley then said that it is should not be requried. Since it
wasn't done earlier, I dropped this part.
The code under tty_ldisc_kill() was executed earlier with the tty lock
taken so it is taken again.
I was able to reproduce the deadlock on v3.8-rc1, this patch fixes the
problem in my testcase. I didn't notice any problems so far.
Cc: Alan Cox <alan@linux.intel.com>
Cc: Peter Hurley <peter@hurleysoftware.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Before waking up the tty line discipline idle queue first check if the queue is
active (non empty). This prevents unnecessary entering the critical section in
the wake_up() function and therefore avoid needless scheduling overhead on a
PREEMPT_RT system caused by two processes being in the same critical section.
Signed-off-by: Ivo Sieben <meltedpianoman@gmail.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
The global "normal" spin lock that guards the line discipline
administration is replaced by a raw spin lock. On a PREEMPT_RT system this
prevents unwanted scheduling overhead around the line discipline administration.
On a 200 MHz AT91SAM9261 processor setup this fixes about 100us of scheduling
overhead on a TTY read or write call.
Signed-off-by: Ivo Sieben <meltedpianoman@gmail.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
So this is it. The big step why we did all the work over the past
kernel releases. Now everything is prepared, so nothing protects us
from doing that big step.
| | \ \ nnnn/^l | |
| | \ / / | |
| '-,.__ => \/ ,-` => | '-,.__
| O __.´´) ( .` | O __.´´)
~~~ ~~ `` ~~~ ~~
The buffers are now in the tty_port structure and we can start
teaching the buffer helpers (insert char/string, flip etc.) to use
tty_port instead of tty_struct all around.
Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Acked-by: Alan Cox <alan@linux.intel.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
We reintroduced tty_ldisc_wait_idle in 100eeae2c5 (TTY: restore
tty_ldisc_wait_idle) and used in set_ldisc. Then we added it also to
the hangup path in 92f6fa09bd (TTY: ldisc, do not close until there
are readers). And we noted that there is one more path:
~ Before 65b770468e tty_ldisc_wait_idle was called also from
~ tty_ldisc_release. It is called from tty_release, so I don't think
~ we need to restore that one.
Well, I was wrong. There might still be holders of an ldisc
reference. Not from userspace, but drivers. If they take a reference
and a user closes the device immediately after that, we have a
problem. ldisc is halted and closed by TTY, but the driver still may
call some ldisc's operation and cause a crash.
So restore the tty_ldisc_wait_idle call also to the third location
where it was before 65b770468e (tty-ldisc: turn ldisc user count
into a proper refcount). Now we should be safe with respect to the
ldisc reference counting as all* tty_ldisc_close paths are safely
called with reference count of one.
* Not the one in tty_ldisc_setup's fail path. But that is called
before the first open finishes. So userspace does not see it yet.
Even thought the driver is given the TTY already via ->install, it
should not take a reference to the ldisc yet. If some driver is to
do this, we should put one tty_ldisc_wait_idle also in the setup.
Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Acked-by: Alan Cox <alan@linux.intel.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Pull workqueue changes from Tejun Heo:
"This is workqueue updates for v3.7-rc1. A lot of activities this
round including considerable API and behavior cleanups.
* delayed_work combines a timer and a work item. The handling of the
timer part has always been a bit clunky leading to confusing
cancelation API with weird corner-case behaviors. delayed_work is
updated to use new IRQ safe timer and cancelation now works as
expected.
* Another deficiency of delayed_work was lack of the counterpart of
mod_timer() which led to cancel+queue combinations or open-coded
timer+work usages. mod_delayed_work[_on]() are added.
These two delayed_work changes make delayed_work provide interface
and behave like timer which is executed with process context.
* A work item could be executed concurrently on multiple CPUs, which
is rather unintuitive and made flush_work() behavior confusing and
half-broken under certain circumstances. This problem doesn't
exist for non-reentrant workqueues. While non-reentrancy check
isn't free, the overhead is incurred only when a work item bounces
across different CPUs and even in simulated pathological scenario
the overhead isn't too high.
All workqueues are made non-reentrant. This removes the
distinction between flush_[delayed_]work() and
flush_[delayed_]_work_sync(). The former is now as strong as the
latter and the specified work item is guaranteed to have finished
execution of any previous queueing on return.
* In addition to the various bug fixes, Lai redid and simplified CPU
hotplug handling significantly.
* Joonsoo introduced system_highpri_wq and used it during CPU
hotplug.
There are two merge commits - one to pull in IRQ safe timer from
tip/timers/core and the other to pull in CPU hotplug fixes from
wq/for-3.6-fixes as Lai's hotplug restructuring depended on them."
Fixed a number of trivial conflicts, but the more interesting conflicts
were silent ones where the deprecated interfaces had been used by new
code in the merge window, and thus didn't cause any real data conflicts.
Tejun pointed out a few of them, I fixed a couple more.
* 'for-3.7' of git://git.kernel.org/pub/scm/linux/kernel/git/tj/wq: (46 commits)
workqueue: remove spurious WARN_ON_ONCE(in_irq()) from try_to_grab_pending()
workqueue: use cwq_set_max_active() helper for workqueue_set_max_active()
workqueue: introduce cwq_set_max_active() helper for thaw_workqueues()
workqueue: remove @delayed from cwq_dec_nr_in_flight()
workqueue: fix possible stall on try_to_grab_pending() of a delayed work item
workqueue: use hotcpu_notifier() for workqueue_cpu_down_callback()
workqueue: use __cpuinit instead of __devinit for cpu callbacks
workqueue: rename manager_mutex to assoc_mutex
workqueue: WORKER_REBIND is no longer necessary for idle rebinding
workqueue: WORKER_REBIND is no longer necessary for busy rebinding
workqueue: reimplement idle worker rebinding
workqueue: deprecate __cancel_delayed_work()
workqueue: reimplement cancel_delayed_work() using try_to_grab_pending()
workqueue: use mod_delayed_work() instead of __cancel + queue
workqueue: use irqsafe timer for delayed_work
workqueue: clean up delayed_work initializers and add missing one
workqueue: make deferrable delayed_work initializer names consistent
workqueue: cosmetic whitespace updates for macro definitions
workqueue: deprecate system_nrt[_freezable]_wq
workqueue: deprecate flush[_delayed]_work_sync()
...
Peter Hurley
Sebastian Andrzej Siewior
Ivo Sieben
Jiri Slaby
Linus Torvalds
Tejun Heo