* [PATCH] drivers:tty:pty: Fix a race causing data loss on close
@ 2020-10-02 2:16 minyard
2020-10-02 12:01 ` Greg Kroah-Hartman
0 siblings, 1 reply; 2+ messages in thread
From: minyard @ 2020-10-02 2:16 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby; +Cc: LKML, Corey Minyard
From: Corey Minyard <cminyard@mvista.com>
If you write to a pty master an immediately close the pty master, the
receiver might get a chunk of data dropped, but then receive some later
data. That's obviously something rather unexpected for a user. It
certainly confused my test program.
It turns out that tty_vhangup() gets called from pty_close(), and that
causes the data on the slave side to be flushed, but due to races more
data can be copied into the slave side's buffer after that. Consider
the following sequence:
thread1 thread2 thread3
write data into buffer,
n_tty buffer is filled
pty_close()
tty_vhangup()
tty_ldisc_hangup()
n_tty_flush_buffer()
reset_buffer_flags()
n_tty_read()
up_read(&tty->termios_rwsem);
down_read(&tty->termios_rwsem);
clear n_tty buffer contents
up_read(&tty->termios_rwsem);
tty_buffer_flush_work()
schedules work calling
flush_to_ldisc()
flush_to_ldisc()
receive_buf()
tty_port_default_receive_buf()
tty_ldisc_receive_buf()
tty_ldisc_receive_buf()
n_tty_receive_buf2()
n_tty_receive_buf_common()
down_read(&tty->termios_rwsem);
__receive_buf()
copies data into n_tty buffer
up_read(&tty->termios_rwsem);
down_read(&tty->termios_rwsem);
copy buffer data to user
This change checks to see if the tty is being hung up before copying
anything in n_tty_receive_buf_common(). It has to be done after the
tty->termios_rwsem semaphore is claimed, for reasons that should be
apparent from the sequence above.
Signed-off-by: Corey Minyard <cminyard@mvista.com>
---
I sent a program to reproduce this, I extended it to prove it wasn't the
test program that caused the issue, and I've uploaded it to:
http://sourceforge.net/projects/ser2net/files/tmp/testpty.c
if you want to run it. It has a lot of comments that explain what is
going on.
This is not a very satisfying fix, though. It works reliably, but it
doesn't seem right to me. My inclination was to remove the up and down
semaphore around tty_buffer_flush_work() in n_tty_read(), as it just
schedules some work, no need to unlock for that. But that resulted
in a deadlock elsewhere, so that up/down on the semaphore is there for
another reason.
The locking in the tty code is really hard to follow. I believe this is
actually a locking problem, but fixing it looks daunting to me.
-corey
drivers/tty/n_tty.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index 1794d84e7bf6..1c33c26dc229 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -1704,6 +1704,9 @@ n_tty_receive_buf_common(struct tty_struct *tty, const unsigned char *cp,
down_read(&tty->termios_rwsem);
+ if (test_bit(TTY_HUPPING, &tty->flags))
+ goto out_upsem;
+
do {
/*
* When PARMRK is set, each input char may take up to 3 chars
@@ -1760,6 +1763,7 @@ n_tty_receive_buf_common(struct tty_struct *tty, const unsigned char *cp,
} else
n_tty_check_throttle(tty);
+out_upsem:
up_read(&tty->termios_rwsem);
return rcvd;
--
2.17.1
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] drivers:tty:pty: Fix a race causing data loss on close
2020-10-02 2:16 [PATCH] drivers:tty:pty: Fix a race causing data loss on close minyard
@ 2020-10-02 12:01 ` Greg Kroah-Hartman
0 siblings, 0 replies; 2+ messages in thread
From: Greg Kroah-Hartman @ 2020-10-02 12:01 UTC (permalink / raw)
To: minyard; +Cc: Jiri Slaby, LKML, Corey Minyard
On Thu, Oct 01, 2020 at 09:16:30PM -0500, minyard@acm.org wrote:
> From: Corey Minyard <cminyard@mvista.com>
>
> If you write to a pty master an immediately close the pty master, the
> receiver might get a chunk of data dropped, but then receive some later
> data. That's obviously something rather unexpected for a user. It
> certainly confused my test program.
>
> It turns out that tty_vhangup() gets called from pty_close(), and that
> causes the data on the slave side to be flushed, but due to races more
> data can be copied into the slave side's buffer after that. Consider
> the following sequence:
>
> thread1 thread2 thread3
> write data into buffer,
> n_tty buffer is filled
> pty_close()
> tty_vhangup()
> tty_ldisc_hangup()
> n_tty_flush_buffer()
> reset_buffer_flags()
> n_tty_read()
> up_read(&tty->termios_rwsem);
> down_read(&tty->termios_rwsem);
> clear n_tty buffer contents
> up_read(&tty->termios_rwsem);
> tty_buffer_flush_work()
> schedules work calling
> flush_to_ldisc()
> flush_to_ldisc()
> receive_buf()
> tty_port_default_receive_buf()
> tty_ldisc_receive_buf()
> tty_ldisc_receive_buf()
> n_tty_receive_buf2()
> n_tty_receive_buf_common()
> down_read(&tty->termios_rwsem);
> __receive_buf()
> copies data into n_tty buffer
> up_read(&tty->termios_rwsem);
> down_read(&tty->termios_rwsem);
> copy buffer data to user
Your text got line-wrapped here for this explaination and it doesn't
make much sense :(
Can you resend this?
> This change checks to see if the tty is being hung up before copying
> anything in n_tty_receive_buf_common(). It has to be done after the
> tty->termios_rwsem semaphore is claimed, for reasons that should be
> apparent from the sequence above.
That kind of makes sense, but it's tricky, if you resend with the above
fixed it might be more obvious...
thanks,
greg k-h
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2020-10-02 12:01 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-02 2:16 [PATCH] drivers:tty:pty: Fix a race causing data loss on close minyard
2020-10-02 12:01 ` Greg Kroah-Hartman
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.