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

[Tim Gardner]

On 09/01/2015 02:05 AM, Andy Whitcroft wrote:
> 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
>

Good catch. Will repost with the scaffold patch which makes them both 
clean cherry-picks.

-- 
Tim Gardner tim.gardner at canonical.com