This is just in case. While it works, I consider it to be diagnostic
data for those unfortunate enough to be intimate with tty locking :)
---
V1 (925bb1ce47f4) changelog:
tty_insert_flip_string_fixed_flag() is racy against itself when called
from the ioctl(TCXONC, TCION/TCIOFF) path [1] and the flush_to_ldisc()
workqueue path [2].
The problem is that port->buf.tail->used is modified without consistent
locking; the ioctl path takes tty->atomic_write_lock, whereas the workqueue
path takes ldata->output_lock.
We cannot simply take ldata->output_lock, since that is specific to the
N_TTY line discipline.
It might seem natural to try to take port->buf.lock inside
tty_insert_flip_string_fixed_flag() and friends (where port->buf is
actually used/modified), but this creates problems for flush_to_ldisc()
which takes it before grabbing tty->ldisc_sem, o_tty->termios_rwsem,
and ldata->output_lock.
Therefore, the simplest solution for now seems to be to take
tty->atomic_write_lock inside tty_port_default_receive_buf(). This lock
is also used in the write path [3] with a consistent ordering.
[1]: Call Trace:
tty_insert_flip_string_fixed_flag
pty_write
tty_send_xchar // down_read(&o_tty->termios_rwsem)
// mutex_lock(&tty->atomic_write_lock)
n_tty_ioctl_helper
n_tty_ioctl
tty_ioctl // down_read(&tty->ldisc_sem)
do_vfs_ioctl
SyS_ioctl
[2]: Workqueue: events_unbound flush_to_ldisc
Call Trace:
tty_insert_flip_string_fixed_flag
pty_write
tty_put_char
__process_echoes
commit_echoes // mutex_lock(&ldata->output_lock)
n_tty_receive_buf_common
n_tty_receive_buf2
tty_ldisc_receive_buf // down_read(&o_tty->termios_rwsem)
tty_port_default_receive_buf // down_read(&tty->ldisc_sem)
flush_to_ldisc // mutex_lock(&port->buf.lock)
process_one_work
[3]: Call Trace:
tty_insert_flip_string_fixed_flag
pty_write
n_tty_write // mutex_lock(&ldata->output_lock)
// down_read(&tty->termios_rwsem)
do_tty_write (inline) // mutex_lock(&tty->atomic_write_lock)
tty_write // down_read(&tty->ldisc_sem)
__vfs_write
vfs_write
SyS_write
The bug can result in about a dozen different crashes depending on what
exactly gets corrupted when port->buf.tail->used points outside the
buffer.
The patch passes my LOCKDEP/PROVE_LOCKING testing but more testing is
always welcome.
Found using syzkaller.
V2: The V1 solution induced an ordering issue, holding buf->lock while
acquiring tty->atomic_write_lock. Resolve it by moving acquisition to
flush_to_ldisc(), prior to acquisition of buf->lock.
Credit to Vegard Nossum for problem analysis/resolution, blame to me for
trivial adaptation thereof.
Signed-off-by: Mike Galbraith <efa...@gmx.de>
Cc: Vegard Nossum <vegard.nos...@oracle.com>
Cc: <sta...@vger.kernel.org>
---
drivers/tty/tty_buffer.c | 10 ++++++++++
1 file changed, 10 insertions(+)
--- a/drivers/tty/tty_buffer.c
+++ b/drivers/tty/tty_buffer.c
@@ -465,7 +465,13 @@ static void flush_to_ldisc(struct work_s
{
struct tty_port *port = container_of(work, struct tty_port, buf.work);
struct tty_bufhead *buf = &port->buf;
+ struct tty_struct *tty = READ_ONCE(port->itty);
+ struct tty_ldisc *disc = NULL;
+ if (tty)
+ disc = tty_ldisc_ref(tty);
+ if (disc)
+ mutex_lock(&tty->atomic_write_lock);
mutex_lock(&buf->lock);
while (1) {
@@ -501,6 +507,10 @@ static void flush_to_ldisc(struct work_s
}
mutex_unlock(&buf->lock);
+ if (disc) {
+ mutex_unlock(&tty->atomic_write_lock);
+ tty_ldisc_deref(disc);
+ }
}
