NAK: [SRU][Trusty][PATCH 1/1] tty: fix stall caused by missing memory barrier in drivers/tty/n_tty.c
Tim Gardner
tim.gardner at canonical.com
Thu Nov 12 20:16:53 UTC 2015
On 11/12/2015 11:24 AM, Joseph Salisbury wrote:
> From: Kosuke Tatsukawa <tatsu at ab.jp.nec.com>
>
> BugLink: http://bugs.launchpad.net/bugs/1512815
>
> My colleague ran into a program stall on a x86_64 server, where
> n_tty_read() was waiting for data even if there was data in the buffer
> in the pty. kernel stack for the stuck process looks like below.
> #0 [ffff88303d107b58] __schedule at ffffffff815c4b20
> #1 [ffff88303d107bd0] schedule at ffffffff815c513e
> #2 [ffff88303d107bf0] schedule_timeout at ffffffff815c7818
> #3 [ffff88303d107ca0] wait_woken at ffffffff81096bd2
> #4 [ffff88303d107ce0] n_tty_read at ffffffff8136fa23
> #5 [ffff88303d107dd0] tty_read at ffffffff81368013
> #6 [ffff88303d107e20] __vfs_read at ffffffff811a3704
> #7 [ffff88303d107ec0] vfs_read at ffffffff811a3a57
> #8 [ffff88303d107f00] sys_read at ffffffff811a4306
> #9 [ffff88303d107f50] entry_SYSCALL_64_fastpath at ffffffff815c86d7
>
> There seems to be two problems causing this issue.
>
> First, in drivers/tty/n_tty.c, __receive_buf() stores the data and
> updates ldata->commit_head using smp_store_release() and then checks
> the wait queue using waitqueue_active(). However, since there is no
> memory barrier, __receive_buf() could return without calling
> wake_up_interactive_poll(), and at the same time, n_tty_read() could
> start to wait in wait_woken() as in the following chart.
>
> __receive_buf() n_tty_read()
> ------------------------------------------------------------------------
> if (waitqueue_active(&tty->read_wait))
> /* Memory operations issued after the
> RELEASE may be completed before the
> RELEASE operation has completed */
> add_wait_queue(&tty->read_wait, &wait);
> ...
> if (!input_available_p(tty, 0)) {
> smp_store_release(&ldata->commit_head,
> ldata->read_head);
> ...
> timeout = wait_woken(&wait,
> TASK_INTERRUPTIBLE, timeout);
> ------------------------------------------------------------------------
>
> The second problem is that n_tty_read() also lacks a memory barrier
> call and could also cause __receive_buf() to return without calling
> wake_up_interactive_poll(), and n_tty_read() to wait in wait_woken()
> as in the chart below.
>
> __receive_buf() n_tty_read()
> ------------------------------------------------------------------------
> spin_lock_irqsave(&q->lock, flags);
> /* from add_wait_queue() */
> ...
> if (!input_available_p(tty, 0)) {
> /* Memory operations issued after the
> RELEASE may be completed before the
> RELEASE operation has completed */
> smp_store_release(&ldata->commit_head,
> ldata->read_head);
> if (waitqueue_active(&tty->read_wait))
> __add_wait_queue(q, wait);
> spin_unlock_irqrestore(&q->lock,flags);
> /* from add_wait_queue() */
> ...
> timeout = wait_woken(&wait,
> TASK_INTERRUPTIBLE, timeout);
> ------------------------------------------------------------------------
>
> There are also other places in drivers/tty/n_tty.c which have similar
> calls to waitqueue_active(), so instead of adding many memory barrier
> calls, this patch simply removes the call to waitqueue_active(),
> leaving just wake_up*() behind.
>
> This fixes both problems because, even though the memory access before
> or after the spinlocks in both wake_up*() and add_wait_queue() can
> sneak into the critical section, it cannot go past it and the critical
> section assures that they will be serialized (please see "INTER-CPU
> ACQUIRING BARRIER EFFECTS" in Documentation/memory-barriers.txt for a
> better explanation). Moreover, the resulting code is much simpler.
>
> Latency measurement using a ping-pong test over a pty doesn't show any
> visible performance drop.
>
> Signed-off-by: Kosuke Tatsukawa <tatsu at ab.jp.nec.com>
> Cc: stable at vger.kernel.org
> Signed-off-by: Greg Kroah-Hartman <gregkh at linuxfoundation.org>
> (backported from commit e81107d4c6bd098878af9796b24edc8d4a9524fd)
> Signed-off-by: Joseph Salisbury <joseph.salisbury at canonical.com>
> ---
> drivers/tty/n_tty.c | 11 +++++------
> 1 file changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
> index 84dcdf4..2d4088d 100644
> --- a/drivers/tty/n_tty.c
> +++ b/drivers/tty/n_tty.c
> @@ -363,6 +363,7 @@ static void n_tty_packet_mode_flush(struct tty_struct *tty)
> spin_lock_irqsave(&tty->ctrl_lock, flags);
> if (tty->link->packet) {
> tty->ctrl_status |= TIOCPKT_FLUSHREAD;
> + spin_unlock_irqrestore(&tty->ctrl_lock, flags);
> wake_up_interruptible(&tty->link->read_wait);
> }
> spin_unlock_irqrestore(&tty->ctrl_lock, flags);
This doesn't look right. I think spin lock/unlock have to be balanced,
plus it was not part of the original patch.
> @@ -1173,7 +1174,7 @@ static void n_tty_receive_break(struct tty_struct *tty)
> put_tty_queue('\0', ldata);
> }
> put_tty_queue('\0', ldata);
> - wake_up_interruptible(&tty->read_wait);
> + wake_up_interruptible_poll(&tty->read_wait, POLLIN);
> }
>
I'm also a little leery of using wake_up_interruptible_poll() when the
original code was wake_up_interruptible(), though upstream is currently
using wake_up_interruptible_poll(). However, test results appear to be
positive, so meh.
See attached backport.
rtg
--
Tim Gardner tim.gardner at canonical.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-tty-fix-stall-caused-by-missing-memory-barrier-in-dr.patch
Type: text/x-patch
Size: 5709 bytes
Desc: not available
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20151112/30080427/attachment.bin>
More information about the kernel-team
mailing list