[PATCH Trusty SRU] tty: Fix pty master poll() after slave closes v2

Andy Whitcroft apw at canonical.com
Tue Sep 1 08:05:33 UTC 2015


On Fri, Aug 28, 2015 at 09:47:49AM -0600, tim.gardner at canonical.com wrote:
> From: Francesco Ruggeri <fruggeri at aristanetworks.com>
> 
> BugLink: http://bugs.launchpad.net/bugs/1397976
> 
> Commit f95499c3030f ("n_tty: Don't wait for buffer work in read() loop")
> introduces a race window where a pty master can be signalled that the pty
> slave was closed before all the data that the slave wrote is delivered.
> Commit f8747d4a466a ("tty: Fix pty master read() after slave closes") fixed the
> problem in case of n_tty_read, but the problem still exists for n_tty_poll.
> This can be seen by running 'for ((i=0; i<100;i++));do ./test.py ;done'
> where test.py is:
> 
> import os, select, pty
> 
> (pid, pty_fd) = pty.fork()
> 
> if pid == 0:
>    os.write(1, 'This string should be received by parent')
> else:
>    poller = select.epoll()
>    poller.register( pty_fd, select.EPOLLIN )
>    ready = poller.poll( 1 * 1000 )
>    for fd, events in ready:
>       if not events & select.EPOLLIN:
>          print 'missed POLLIN event'
>       else:
>          print os.read(fd, 100)
>    poller.close()
> 
> The string from the slave is missed several times.
> This patch takes the same approach as the fix for read and special cases
> this condition for poll.
> Tested on 3.16.
> 
> Signed-off-by: Francesco Ruggeri <fruggeri at arista.com>
> Signed-off-by: Greg Kroah-Hartman <gregkh at linuxfoundation.org>
> (back ported from commit c4dc304677e8d566572c4738d95c48be150c6606)
> Signed-off-by: Tim Gardner <tim.gardner at canonical.com>
> 
> Conflicts:
> 	drivers/tty/n_tty.c
> ---
>  drivers/tty/n_tty.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
> index ba9270b..ca045db 100644
> --- a/drivers/tty/n_tty.c
> +++ b/drivers/tty/n_tty.c
> @@ -2477,12 +2477,17 @@ static unsigned int n_tty_poll(struct tty_struct *tty, struct file *file,
>  
>  	poll_wait(file, &tty->read_wait, wait);
>  	poll_wait(file, &tty->write_wait, wait);
> +	if (test_bit(TTY_OTHER_CLOSED, &tty->flags))
> +		mask |= POLLHUP;
>  	if (input_available_p(tty, TIME_CHAR(tty) ? 0 : MIN_CHAR(tty)))
>  		mask |= POLLIN | POLLRDNORM;

I believe the intent of this change to be such that on detecting a HUP
event we flush the line discipline buffers down and then recheck for input.
That it would trigger a repeat of this input_available_p() incantation
after a tty_flush_to_ldisc() ...

> +	else if (mask & POLLHUP) {
> +		tty_flush_to_ldisc(tty);
> +		if (input_available_p(tty, 1))
> +			mask |= POLLIN | POLLRDNORM;

... so I expecting this second ones parameters to match the first.

If I am reading the history right the semantics of input_available_p()
changes in the commit below from a size to a boolean "poll", which falls
between where we are at and the commit being backported:

  commit eafbe67f84761d787802e5113d895a316b6292fe
  Author: Peter Hurley <peter at hurleysoftware.com>
  Date:   Mon Dec 2 14:24:45 2013 -0500

    n_tty: Refactor input_available_p() by call site

> +	}
>  	if (tty->packet && tty->link->ctrl_status)
>  		mask |= POLLPRI | POLLIN | POLLRDNORM;
> -	if (test_bit(TTY_OTHER_CLOSED, &tty->flags))
> -		mask |= POLLHUP;
>  	if (tty_hung_up_p(file))
>  		mask |= POLLHUP;
>  	if (!(mask & (POLLHUP | POLLIN | POLLRDNORM))) {

-apw




More information about the kernel-team mailing list