[Patch] Fix a race condition in pty.c

[Patch] Fix a race condition in pty.c

[Zou, Nanhai]

[-- Attachment #1: Type: text/plain, Size: 445 bytes --]

 <<pty_close-race-fix.patch>> There is a race condition int pty.c 
when pty_close wakes up waiter on its pair device before set
TTY_OTHER_CLOSED flag.

It is possible on SMP or preempt kernel, waiter wakes up too early that
it will not get TTY_OTHER_CLOSED flag then fall into sleep again.

Lu hong jiu report this bug will hang some expect scripts on SMP
machines.

Signed-off-by:	Zou Nan hai <Nanhai.zou@intel.com>

Zou Nan hai


[-- Attachment #2: pty_close-race-fix.patch --]
[-- Type: application/octet-stream, Size: 525 bytes --]

--- linux-2.6.10-rc3-mm1/drivers/char/pty.c	2004-12-16 01:32:56.761886848 -0500
+++ b/drivers/char/pty.c	2004-12-16 01:33:32.217941101 -0500
@@ -55,9 +55,9 @@
 	if (!tty->link)
 		return;
 	tty->link->packet = 0;
+	set_bit(TTY_OTHER_CLOSED, &tty->link->flags);
 	wake_up_interruptible(&tty->link->read_wait);
 	wake_up_interruptible(&tty->link->write_wait);
-	set_bit(TTY_OTHER_CLOSED, &tty->link->flags);
 	if (tty->driver->subtype == PTY_TYPE_MASTER) {
 		set_bit(TTY_OTHER_CLOSED, &tty->flags);
 #ifdef CONFIG_UNIX98_PTYS

Re: [Patch] Fix a race condition in pty.c

[Andrew Morton]

"Zou, Nanhai" <nanhai.zou@intel.com> wrote:
>
>  <<pty_close-race-fix.patch>> There is a race condition int pty.c 
>  when pty_close wakes up waiter on its pair device before set
>  TTY_OTHER_CLOSED flag.
> 
>  It is possible on SMP or preempt kernel, waiter wakes up too early that
>  it will not get TTY_OTHER_CLOSED flag then fall into sleep again.
> 
>  Lu hong jiu report this bug will hang some expect scripts on SMP
>  machines.

The patch looks good (apart from the application/octet-stream encoding.  grr.)

But on my test box it still doesn't fix the testcase which hj attached to
bug 3894:

select (3) returns: 1
Read -1 characters.
errno: Input/output error
334
select (3) returns: 1
Read -1 characters.
errno: Input/output error
335

In fact the same happens on a non-preempt uniprocessor kernel, so I must be
seeing something different.

#include <sys/types.h> 
#include <sys/select.h>
#include <stdio.h>
#include <unistd.h>
#include <pty.h>

int
main ()
{
  pid_t pid;
  int fd;
  char slave_name [20];
  char buffer [1000];
  int count;
  fd_set readfds;
  int ret;

  pid = forkpty (&fd, slave_name, NULL, NULL);
  switch (pid)
    {
    case 0: /* Child process. */     
      if (execlp ("true", "true", NULL))
	perror ("execlp");
      return 1;
      break;

    default: /* Parent process. */
      break;
    } 

  FD_ZERO (&readfds);
  FD_SET (fd, &readfds);
  ret = select (1024, &readfds, NULL, NULL, NULL);
  printf ("select (%d) returns: %d\n", fd, ret);
  if (ret < 0)
    perror ("select");
  else if (FD_ISSET (fd, &readfds))
    {
      count = read (fd, buffer, 999);
      printf ("Read %d characters.\n", count);
      perror("errno");
    }
  return 0;
}

RE: [Patch] Fix a race condition in pty.c

[Lu, Hongjiu]

Sorry I didn't mention that my testcase hang:

11540 pts/0    S      0:00 ./pty
11541 ?        Zs     0:00 [true] <defunct>

Both parent and child are sleeping. They will never wake up. I updated
my bug report

http://bugzilla.kernel.org/show_bug.cgi?id=3894

with the config file for P4 SMP machine.


H.J. Lu
Intel Corporation


>-----Original Message-----
>From: Andrew Morton [mailto:akpm@osdl.org]
>Sent: Friday, December 17, 2004 12:36 AM
>To: Zou, Nanhai
>Cc: linux-kernel@vger.kernel.org; Lu, Hongjiu
>Subject: Re: [Patch] Fix a race condition in pty.c
>
>"Zou, Nanhai" <nanhai.zou@intel.com> wrote:
>>
>>  <<pty_close-race-fix.patch>> There is a race condition int pty.c
>>  when pty_close wakes up waiter on its pair device before set
>>  TTY_OTHER_CLOSED flag.
>>
>>  It is possible on SMP or preempt kernel, waiter wakes up too early
>that
>>  it will not get TTY_OTHER_CLOSED flag then fall into sleep again.
>>
>>  Lu hong jiu report this bug will hang some expect scripts on SMP
>>  machines.
>
>The patch looks good (apart from the application/octet-stream encoding.
>grr.)
>
>But on my test box it still doesn't fix the testcase which hj attached
>to
>bug 3894:
>
>select (3) returns: 1
>Read -1 characters.
>errno: Input/output error
>334
>select (3) returns: 1
>Read -1 characters.
>errno: Input/output error
>335
>
>In fact the same happens on a non-preempt uniprocessor kernel, so I
>must be
>seeing something different.
>
>#include <sys/types.h>
>#include <sys/select.h>
>#include <stdio.h>
>#include <unistd.h>
>#include <pty.h>
>
>int
>main ()
>{
>  pid_t pid;
>  int fd;
>  char slave_name [20];
>  char buffer [1000];
>  int count;
>  fd_set readfds;
>  int ret;
>
>  pid = forkpty (&fd, slave_name, NULL, NULL);
>  switch (pid)
>    {
>    case 0: /* Child process. */
>      if (execlp ("true", "true", NULL))
>	perror ("execlp");
>      return 1;
>      break;
>
>    default: /* Parent process. */
>      break;
>    }
>
>  FD_ZERO (&readfds);
>  FD_SET (fd, &readfds);
>  ret = select (1024, &readfds, NULL, NULL, NULL);
>  printf ("select (%d) returns: %d\n", fd, ret);
>  if (ret < 0)
>    perror ("select");
>  else if (FD_ISSET (fd, &readfds))
>    {
>      count = read (fd, buffer, 999);
>      printf ("Read %d characters.\n", count);
>      perror("errno");
>    }
>  return 0;
>}