Re: [patch] tty: fix port buffer locking V2

[Mike Galbraith]

On Mon, 2017-06-05 at 08:48 +0200, Greg Kroah-Hartman wrote:
> 
> Can you redo this against 4.12-rc4 which has the original tty locking
> patch reverted?

It's against 4.12-rc4.

-Mike

Re: [patch] tty: fix port buffer locking V2

[Mike Galbraith]

On Mon, 2017-06-05 at 08:48 +0200, Greg Kroah-Hartman wrote:
> 
> Can you redo this against 4.12-rc4 which has the original tty locking
> patch reverted?

It's against 4.12-rc4.

-Mike

Re: [patch] tty: fix port buffer locking V2

[Greg Kroah-Hartman]

On Mon, Jun 05, 2017 at 06:52:27AM +0200, Mike Galbraith wrote:
> 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 :)

Can you redo this against 4.12-rc4 which has the original tty locking
patch reverted?

thanks,

greg k-h

Re: [patch] tty: fix port buffer locking V2

[Greg Kroah-Hartman]

On Mon, Jun 05, 2017 at 06:52:27AM +0200, Mike Galbraith wrote:
> 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 :)

Can you redo this against 4.12-rc4 which has the original tty locking
patch reverted?

thanks,

greg k-h

[patch] tty: fix port buffer locking V2

[Mike Galbraith]

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(_tty->termios_rwsem)
// mutex_lock(>atomic_write_lock)
 n_tty_ioctl_helper
 n_tty_ioctl
 tty_ioctl  // down_read(>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(>output_lock)
 n_tty_receive_buf_common
 n_tty_receive_buf2
 tty_ldisc_receive_buf  // down_read(_tty->termios_rwsem)
 tty_port_default_receive_buf   // down_read(>ldisc_sem)
 flush_to_ldisc // mutex_lock(>buf.lock)
 process_one_work

[3]: Call Trace:
 tty_insert_flip_string_fixed_flag
 pty_write
 n_tty_write// mutex_lock(>output_lock)
// down_read(>termios_rwsem)
 do_tty_write (inline)  // mutex_lock(>atomic_write_lock)
 tty_write  // down_read(>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 
Cc: Vegard Nossum 
Cc: 
---
 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 = >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(>atomic_write_lock);
mutex_lock(>lock);
 
while (1) {
@@ -501,6 +507,10 @@ static void flush_to_ldisc(struct work_s
}
 
mutex_unlock(>lock);
+   if (disc) {
+   mutex_unlock(>atomic_write_lock);
+   tty_ldisc_deref(disc);
+   }
 
 }
 

[patch] tty: fix port buffer locking V2

[Mike Galbraith]

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(_tty->termios_rwsem)
// mutex_lock(>atomic_write_lock)
 n_tty_ioctl_helper
 n_tty_ioctl
 tty_ioctl  // down_read(>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(>output_lock)
 n_tty_receive_buf_common
 n_tty_receive_buf2
 tty_ldisc_receive_buf  // down_read(_tty->termios_rwsem)
 tty_port_default_receive_buf   // down_read(>ldisc_sem)
 flush_to_ldisc // mutex_lock(>buf.lock)
 process_one_work

[3]: Call Trace:
 tty_insert_flip_string_fixed_flag
 pty_write
 n_tty_write// mutex_lock(>output_lock)
// down_read(>termios_rwsem)
 do_tty_write (inline)  // mutex_lock(>atomic_write_lock)
 tty_write  // down_read(>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 
Cc: Vegard Nossum 
Cc: 
---
 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 = >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(>atomic_write_lock);
mutex_lock(>lock);
 
while (1) {
@@ -501,6 +507,10 @@ static void flush_to_ldisc(struct work_s
}
 
mutex_unlock(>lock);
+   if (disc) {
+   mutex_unlock(>atomic_write_lock);
+   tty_ldisc_deref(disc);
+   }
 
 }