Hi Andy,
On 03/16/2015 10:30 AM, Andy Whitcroft wrote:
> In commit 52bce7f8d4fc ("pty, n_tty: Simplify input processing on
> final close") the tty_flush_to_ldisc() data flush was moved from the
> n_tty_input() path to the pty_close() path because the new locking ensured
> that the data had already been copied:
>
> However, this only guarantees that it will see _some_ of the pending
> data, we cannot guarantee that all of the queued data will fit into the
> output buffer. When the output buffer fills the flush worker triggered
> in pty_close will complete leaving the remaining data queued in the
> tty_buffer chain.
>
> When this occurs the reader will see the initial tranch of data correctly
> and consume it. As we return this to the caller we will spot the
> additional pending data and trigger another flush worker. So far so good.
>
> However, we now have a race, if the consumer gets back into pty_read
> before the flush worker is actually able to actually progress the queue,
> it will see the see the tty input buffer empty, the other end marked
> TTY_OTHER_CLOSED and return EIO to the caller even though data is en-route.
>
> Fix this by ignoring TTY_OTHER_CLOSED while there remain unflushed buffers.
>
> Fixes: 52bce7f8d4fc ("pty, n_tty: Simplify input processing on final close")
> BugLink: http://bugs.launchpad.net/bugs/1429756
> Cc: <[email protected]> # 3.19+
> Signed-off-by: Andy Whitcroft <[email protected]>
> ---
> drivers/tty/n_tty.c | 4 +++-
> drivers/tty/tty_buffer.c | 19 +++++++++++++++++++
> include/linux/tty_flip.h | 2 ++
> 3 files changed, 24 insertions(+), 1 deletion(-)
>
> This was found during self-tests for the upstart init system which
> tests log handling by pumping large chunks of /dev/zero through
> to various logs in parallel. This was failing with short files
> at random, which was traced back to this race.
>
> Build tested against v4.0-rc4, heavily run tested against v3.19.1.
>
> -apw
Thanks for discovering this bug.
I just managed to reproduce this problem with a test jig;
can you confirm that your self-tests also use >= 4096-byte read buffer
(which I think is necessary to trigger the worker race)?
Regards,
Peter Hurley
> diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
> index cf6e0f2..76b38f0 100644
> --- a/drivers/tty/n_tty.c
> +++ b/drivers/tty/n_tty.c
> @@ -38,6 +38,7 @@
> #include <linux/sched.h>
> #include <linux/interrupt.h>
> #include <linux/tty.h>
> +#include <linux/tty_flip.h>
> #include <linux/timer.h>
> #include <linux/ctype.h>
> #include <linux/mm.h>
> @@ -2236,7 +2237,8 @@ static ssize_t n_tty_read(struct tty_struct *tty,
> struct file *file,
> ldata->minimum_to_wake = (minimum - (b - buf));
>
> if (!input_available_p(tty, 0)) {
> - if (test_bit(TTY_OTHER_CLOSED, &tty->flags)) {
> + if (test_bit(TTY_OTHER_CLOSED, &tty->flags) &&
> + !tty_data_pending_to_ldisc(tty)) {
> retval = -EIO;
> break;
> }
> diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
> index 7566164..04cfabb 100644
> --- a/drivers/tty/tty_buffer.c
> +++ b/drivers/tty/tty_buffer.c
> @@ -424,6 +424,25 @@ receive_buf(struct tty_struct *tty, struct tty_buffer
> *head, int count)
> return count;
> }
>
> +int tty_data_pending_to_ldisc(struct tty_struct *tty)
> +{
> + struct tty_bufhead *buf = &tty->port->buf;
> + struct tty_buffer *head = buf->head;
> +
> + struct tty_buffer *next;
> + int count;
> +
> + next = head->next;
> + /* paired w/ barrier in __tty_buffer_request_room();
> + * ensures commit value read is not stale if the head
> + * is advancing to the next buffer
> + */
> + smp_rmb();
> + count = head->commit - head->read;
> +
> + return (count || next);
> +}
> +
> /**
> * flush_to_ldisc
> * @work: tty structure passed from work queue.
> diff --git a/include/linux/tty_flip.h b/include/linux/tty_flip.h
> index c28dd52..a896b94 100644
> --- a/include/linux/tty_flip.h
> +++ b/include/linux/tty_flip.h
> @@ -38,4 +38,6 @@ static inline int tty_insert_flip_string(struct tty_port
> *port,
> extern void tty_buffer_lock_exclusive(struct tty_port *port);
> extern void tty_buffer_unlock_exclusive(struct tty_port *port);
>
> +extern int tty_data_pending_to_ldisc(struct tty_struct *tty);
> +
> #endif /* _LINUX_TTY_FLIP_H */
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
