Re: [PATCH 1/1] pty, n_tty: continue processing input until the tty_buffer chain is flushed

[Peter Hurley <peter@hurleysoftware.com>]

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: <stable@vger.kernel.org> # 3.19+
> Signed-off-by: Andy Whitcroft <apw@canonical.com>
> ---
>  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 */
>