diff mbox

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

Message ID 1440776869-15259-1-git-send-email-tim.gardner@canonical.com
State New
Headers show

Commit Message

Tim Gardner Aug. 28, 2015, 3:47 p.m. UTC 
From: Francesco Ruggeri <fruggeri@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@arista.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
(back ported from commit c4dc304677e8d566572c4738d95c48be150c6606)
Signed-off-by: Tim Gardner <tim.gardner@canonical.com>

Conflicts:
	drivers/tty/n_tty.c
---
 drivers/tty/n_tty.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

Comments

Stefan Bader Aug. 31, 2015, 12:13 p.m. UTC | #1
Andy Whitcroft Sept. 1, 2015, 8:05 a.m. UTC | #2 
On Fri, Aug 28, 2015 at 09:47:49AM -0600, tim.gardner@canonical.com wrote:
> From: Francesco Ruggeri <fruggeri@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@arista.com>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> (back ported from commit c4dc304677e8d566572c4738d95c48be150c6606)
> Signed-off-by: Tim Gardner <tim.gardner@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@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
Tim Gardner Sept. 1, 2015, 3:49 p.m. UTC | #3 
On 09/01/2015 02:05 AM, Andy Whitcroft wrote:
> On Fri, Aug 28, 2015 at 09:47:49AM -0600, tim.gardner@canonical.com wrote:
>> From: Francesco Ruggeri <fruggeri@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@arista.com>
>> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> (back ported from commit c4dc304677e8d566572c4738d95c48be150c6606)
>> Signed-off-by: Tim Gardner <tim.gardner@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@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.
diff mbox

Patch

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;
+	else if (mask & POLLHUP) {
+		tty_flush_to_ldisc(tty);
+		if (input_available_p(tty, 1))
+			mask |= POLLIN | POLLRDNORM;
+	}
 	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))) {