pty leak or corruption w/ openpty + dup2?

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

pty leak or corruption w/ openpty + dup2?

Martin Pieuchot
Program below is the smaller version of a syzkaller report [0].  After
running it one is left without usable console.  A second execution will
make openpty(3) pick a different "/dev/tty*" node:

  50361 crash    CALL  ioctl(3,PTMGET,0x7f7ffffeda80)
  50361 crash    NAMI  "/dev/ptypd"
  50361 crash    NAMI  "/dev/ttypd"
  50361 crash    NAMI  "/dev/ttypd"
  50361 crash    RET   ioctl 0

After some more tries:

  65559 crash    CALL  ioctl(3,PTMGET,0x7f7ffffc36a0)
  65559 crash    NAMI  "/dev/ptypm"
  65559 crash    NAMI  "/dev/ttypm"
  65559 crash    NAMI  "/dev/ttypm"
  65559 crash    RET   ioctl 0

[0] https://syzkaller.appspot.com/bug?id=a74718ca902617e6aa7327aa008b25844eccf2d3

----- crash.c -----

#include <unistd.h>
#include <util.h>

int
main(void)
{
        char garbage[100];
        int master, slave;

        if (openpty(&master, &slave, NULL, NULL, NULL) == -1)
                return -1;
        if (dup2(master, master + 100) != -1)
                close(master);

        write(slave, garbage, 99);

        return 0;
}

Reply | Threaded
Open this post in threaded view
|

Re: pty leak or corruption w/ openpty + dup2?

Theo de Raadt-2
Or does this have nothing to do with ptm, but dup2 broken when it
was unlocked?

Martin Pieuchot <[hidden email]> wrote:

> Program below is the smaller version of a syzkaller report [0].  After
> running it one is left without usable console.  A second execution will
> make openpty(3) pick a different "/dev/tty*" node:
>
>   50361 crash    CALL  ioctl(3,PTMGET,0x7f7ffffeda80)
>   50361 crash    NAMI  "/dev/ptypd"
>   50361 crash    NAMI  "/dev/ttypd"
>   50361 crash    NAMI  "/dev/ttypd"
>   50361 crash    RET   ioctl 0
>
> After some more tries:
>
>   65559 crash    CALL  ioctl(3,PTMGET,0x7f7ffffc36a0)
>   65559 crash    NAMI  "/dev/ptypm"
>   65559 crash    NAMI  "/dev/ttypm"
>   65559 crash    NAMI  "/dev/ttypm"
>   65559 crash    RET   ioctl 0
>
> [0] https://syzkaller.appspot.com/bug?id=a74718ca902617e6aa7327aa008b25844eccf2d3
>
> ----- crash.c -----
>
> #include <unistd.h>
> #include <util.h>
>
> int
> main(void)
> {
> char garbage[100];
> int master, slave;
>
> if (openpty(&master, &slave, NULL, NULL, NULL) == -1)
> return -1;
> if (dup2(master, master + 100) != -1)
> close(master);
>
> write(slave, garbage, 99);
>
> return 0;
> }
>

Reply | Threaded
Open this post in threaded view
|

Re: pty leak or corruption w/ openpty + dup2?

Anton Lindqvist-2
In reply to this post by Martin Pieuchot
On Wed, Apr 29, 2020 at 11:12:37AM +0200, Martin Pieuchot wrote:

> Program below is the smaller version of a syzkaller report [0].  After
> running it one is left without usable console.  A second execution will
> make openpty(3) pick a different "/dev/tty*" node:
>
>   50361 crash    CALL  ioctl(3,PTMGET,0x7f7ffffeda80)
>   50361 crash    NAMI  "/dev/ptypd"
>   50361 crash    NAMI  "/dev/ttypd"
>   50361 crash    NAMI  "/dev/ttypd"
>   50361 crash    RET   ioctl 0
>
> After some more tries:
>
>   65559 crash    CALL  ioctl(3,PTMGET,0x7f7ffffc36a0)
>   65559 crash    NAMI  "/dev/ptypm"
>   65559 crash    NAMI  "/dev/ttypm"
>   65559 crash    NAMI  "/dev/ttypm"
>   65559 crash    RET   ioctl 0
>
> [0] https://syzkaller.appspot.com/bug?id=a74718ca902617e6aa7327aa008b25844eccf2d3
>
> ----- crash.c -----
>
> #include <unistd.h>
> #include <util.h>
>
> int
> main(void)
> {
> char garbage[100];
> int master, slave;
>
> if (openpty(&master, &slave, NULL, NULL, NULL) == -1)
> return -1;
> if (dup2(master, master + 100) != -1)
> close(master);
>
> write(slave, garbage, 99);
>
> return 0;
> }
>

The order in which the pty master/slave is closed seems to be the
trigger here. While not duping the master, it's closed before the slave.
In the opposite scenario, the slave is closed before the master. While
closing the slave, it ends up here expressed as a simplified backtrace:

  tsleep()
  ttysleep()
  ttywait()
  ttywflush()
  ttylclose()
  ptsclose()
  fdfree()
  exit1()

In order words, it ends up doing a tsleep(INFSLP) causing the thread to
hang. Note that this is not the case when the master is closed before
the slave since `tp->t_oproc == NULL' causing ttywait() to bail early.

NetBSD does a sleep with a timeout in ttywflush(). I've applied the same
approach in the diff below which does fix the hang.

Index: kern/kern_proc.c
===================================================================
RCS file: /cvs/src/sys/kern/kern_proc.c,v
retrieving revision 1.86
diff -u -p -r1.86 kern_proc.c
--- kern/kern_proc.c 30 Jan 2020 08:51:27 -0000 1.86
+++ kern/kern_proc.c 1 May 2020 10:11:12 -0000
@@ -410,7 +410,7 @@ killjobc(struct process *pr)
  if (sp->s_ttyp->t_session == sp) {
  if (sp->s_ttyp->t_pgrp)
  pgsignal(sp->s_ttyp->t_pgrp, SIGHUP, 1);
- ttywait(sp->s_ttyp);
+ ttywait(sp->s_ttyp, 0);
  /*
  * The tty could have been revoked
  * if we blocked.
Index: kern/tty.c
===================================================================
RCS file: /cvs/src/sys/kern/tty.c,v
retrieving revision 1.154
diff -u -p -r1.154 tty.c
--- kern/tty.c 7 Apr 2020 13:27:51 -0000 1.154
+++ kern/tty.c 1 May 2020 10:11:12 -0000
@@ -809,7 +809,7 @@ ttioctl(struct tty *tp, u_long cmd, cadd
  break;
  }
  case TIOCDRAIN: /* wait till output drained */
- if ((error = ttywait(tp)) != 0)
+ if ((error = ttywait(tp, 0)) != 0)
  return (error);
  break;
  case TIOCGETA: { /* get termios struct */
@@ -866,7 +866,7 @@ ttioctl(struct tty *tp, u_long cmd, cadd
 
  s = spltty();
  if (cmd == TIOCSETAW || cmd == TIOCSETAF) {
- if ((error = ttywait(tp)) != 0) {
+ if ((error = ttywait(tp, 0)) != 0) {
  splx(s);
  return (error);
  }
@@ -1205,7 +1205,7 @@ ttnread(struct tty *tp)
  * Wait for output to drain.
  */
 int
-ttywait(struct tty *tp)
+ttywait(struct tty *tp, int timo)
 {
  int error, s;
 
@@ -1219,7 +1219,8 @@ ttywait(struct tty *tp)
     (ISSET(tp->t_state, TS_CARR_ON) || ISSET(tp->t_cflag, CLOCAL))
     && tp->t_oproc) {
  SET(tp->t_state, TS_ASLEEP);
- error = ttysleep(tp, &tp->t_outq, TTOPRI | PCATCH, ttyout);
+ error = ttysleep_timo(tp, &tp->t_outq, TTOPRI | PCATCH,
+    ttyout, timo);
  if (error)
  break;
  } else
@@ -1237,7 +1238,8 @@ ttywflush(struct tty *tp)
 {
  int error;
 
- if ((error = ttywait(tp)) == 0)
+ error = ttywait(tp, 5 * hz);
+ if (error == 0 || error == EWOULDBLOCK)
  ttyflush(tp, FREAD);
  return (error);
 }
@@ -2281,11 +2283,18 @@ tputchar(int c, struct tty *tp)
 int
 ttysleep(struct tty *tp, void *chan, int pri, char *wmesg)
 {
+
+ return (ttysleep_timo(tp, chan, pri, wmesg, INFSLP));
+}
+
+int
+ttysleep_timo(struct tty *tp, void *chan, int pri, char *wmesg, int timo)
+{
  int error;
  short gen;
 
  gen = tp->t_gen;
- if ((error = tsleep_nsec(chan, pri, wmesg, INFSLP)) != 0)
+ if ((error = tsleep_nsec(chan, pri, wmesg, timo)) != 0)
  return (error);
  return (tp->t_gen == gen ? 0 : ERESTART);
 }
Index: sys/tty.h
===================================================================
RCS file: /cvs/src/sys/sys/tty.h,v
retrieving revision 1.38
diff -u -p -r1.38 tty.h
--- sys/tty.h 19 Jul 2019 00:17:16 -0000 1.38
+++ sys/tty.h 1 May 2020 10:11:12 -0000
@@ -293,7 +293,9 @@ void ttypend(struct tty *tp);
 void ttyretype(struct tty *tp);
 void ttyrub(int c, struct tty *tp);
 int ttysleep(struct tty *tp, void *chan, int pri, char *wmesg);
-int ttywait(struct tty *tp);
+int ttysleep_timo(struct tty *tp, void *chan, int pri, char *wmesg,
+    int timo);
+int ttywait(struct tty *tp, int timo);
 int ttywflush(struct tty *tp);
 void ttytstamp(struct tty *tp, int octs, int ncts, int odcd, int ndcd);
 

Reply | Threaded
Open this post in threaded view
|

Re: pty leak or corruption w/ openpty + dup2?

Martin Pieuchot
On 01/05/20(Fri) 12:13, Anton Lindqvist wrote:

> The order in which the pty master/slave is closed seems to be the
> trigger here. While not duping the master, it's closed before the slave.
> In the opposite scenario, the slave is closed before the master. While
> closing the slave, it ends up here expressed as a simplified backtrace:
>
>   tsleep()
>   ttysleep()
>   ttywait()
>   ttywflush()
>   ttylclose()
>   ptsclose()
>   fdfree()
>   exit1()
>
> In order words, it ends up doing a tsleep(INFSLP) causing the thread to
> hang. Note that this is not the case when the master is closed before
> the slave since `tp->t_oproc == NULL' causing ttywait() to bail early.

Why is the sleeper never awaken?  Does that mean a ttwakeup() is missing?

> NetBSD does a sleep with a timeout in ttywflush(). I've applied the same
> approach in the diff below which does fix the hang.

This seems like a racy workaround for a bug that we do not fully
understand.  If this is a proper solution I'd be happy to understand
why.  If we go with such fix we should be using a value in "nsecs"
instead of ticks and INFSLP should be used instead of 0.  We should
refrain from introducing new usages of `hz' ;)

Reply | Threaded
Open this post in threaded view
|

Re: pty leak or corruption w/ openpty + dup2?

Anton Lindqvist-2
On Fri, May 01, 2020 at 05:17:36PM +0200, Martin Pieuchot wrote:

> On 01/05/20(Fri) 12:13, Anton Lindqvist wrote:
> > The order in which the pty master/slave is closed seems to be the
> > trigger here. While not duping the master, it's closed before the slave.
> > In the opposite scenario, the slave is closed before the master. While
> > closing the slave, it ends up here expressed as a simplified backtrace:
> >
> >   tsleep()
> >   ttysleep()
> >   ttywait()
> >   ttywflush()
> >   ttylclose()
> >   ptsclose()
> >   fdfree()
> >   exit1()
> >
> > In order words, it ends up doing a tsleep(INFSLP) causing the thread to
> > hang. Note that this is not the case when the master is closed before
> > the slave since `tp->t_oproc == NULL' causing ttywait() to bail early.
>
> Why is the sleeper never awaken?  Does that mean a ttwakeup() is missing?

In this case, the process is single threaded, about to exit and the only
consumer of the pty. I don't see how it could be any other process
responsibility to perform the wakeup.

>
> > NetBSD does a sleep with a timeout in ttywflush(). I've applied the same
> > approach in the diff below which does fix the hang.
>
> This seems like a racy workaround for a bug that we do not fully
> understand.  If this is a proper solution I'd be happy to understand
> why.  If we go with such fix we should be using a value in "nsecs"
> instead of ticks and INFSLP should be used instead of 0.  We should
> refrain from introducing new usages of `hz' ;)

This is corresponding commit message[1] from NetBSD which describes what
we're seeing:

  commit a5bbe319b2ea24c4c2133a51eac8a0285dc074e4
  Author: gson <[hidden email]>
  Date:   Wed Aug 19 12:02:55 2015 +0000

      When closing a tty, limit the amount of time spent waiting for the
      output to drain to five seconds so that exiting processes with
      buffered output for a serial port blocked by flow control or a pty
      that is not being read do not hang indefinitely.  Should fix PRs
      kern/12534 and kern/17171.  This is an updated version of the change
      of tty.c 1.263.

Updated diff favoring INFSLP and SEC_TO_NSEC().

[1] https://github.com/IIJ-NetBSD/netbsd-src/commit/a5bbe319b2ea

Index: kern/kern_proc.c
===================================================================
RCS file: /cvs/src/sys/kern/kern_proc.c,v
retrieving revision 1.86
diff -u -p -r1.86 kern_proc.c
--- kern/kern_proc.c 30 Jan 2020 08:51:27 -0000 1.86
+++ kern/kern_proc.c 2 May 2020 08:35:03 -0000
@@ -410,7 +410,7 @@ killjobc(struct process *pr)
  if (sp->s_ttyp->t_session == sp) {
  if (sp->s_ttyp->t_pgrp)
  pgsignal(sp->s_ttyp->t_pgrp, SIGHUP, 1);
- ttywait(sp->s_ttyp);
+ ttywait(sp->s_ttyp, INFSLP);
  /*
  * The tty could have been revoked
  * if we blocked.
Index: kern/tty.c
===================================================================
RCS file: /cvs/src/sys/kern/tty.c,v
retrieving revision 1.154
diff -u -p -r1.154 tty.c
--- kern/tty.c 7 Apr 2020 13:27:51 -0000 1.154
+++ kern/tty.c 2 May 2020 08:35:03 -0000
@@ -809,7 +809,7 @@ ttioctl(struct tty *tp, u_long cmd, cadd
  break;
  }
  case TIOCDRAIN: /* wait till output drained */
- if ((error = ttywait(tp)) != 0)
+ if ((error = ttywait(tp, INFSLP)) != 0)
  return (error);
  break;
  case TIOCGETA: { /* get termios struct */
@@ -866,7 +866,7 @@ ttioctl(struct tty *tp, u_long cmd, cadd
 
  s = spltty();
  if (cmd == TIOCSETAW || cmd == TIOCSETAF) {
- if ((error = ttywait(tp)) != 0) {
+ if ((error = ttywait(tp, INFSLP)) != 0) {
  splx(s);
  return (error);
  }
@@ -1205,7 +1205,7 @@ ttnread(struct tty *tp)
  * Wait for output to drain.
  */
 int
-ttywait(struct tty *tp)
+ttywait(struct tty *tp, int timo)
 {
  int error, s;
 
@@ -1219,7 +1219,8 @@ ttywait(struct tty *tp)
     (ISSET(tp->t_state, TS_CARR_ON) || ISSET(tp->t_cflag, CLOCAL))
     && tp->t_oproc) {
  SET(tp->t_state, TS_ASLEEP);
- error = ttysleep(tp, &tp->t_outq, TTOPRI | PCATCH, ttyout);
+ error = ttysleep_timo(tp, &tp->t_outq, TTOPRI | PCATCH,
+    ttyout, timo);
  if (error)
  break;
  } else
@@ -1237,7 +1238,8 @@ ttywflush(struct tty *tp)
 {
  int error;
 
- if ((error = ttywait(tp)) == 0)
+ error = ttywait(tp, SEC_TO_NSEC(5));
+ if (error == 0 || error == EWOULDBLOCK)
  ttyflush(tp, FREAD);
  return (error);
 }
@@ -2281,11 +2283,18 @@ tputchar(int c, struct tty *tp)
 int
 ttysleep(struct tty *tp, void *chan, int pri, char *wmesg)
 {
+
+ return (ttysleep_timo(tp, chan, pri, wmesg, INFSLP));
+}
+
+int
+ttysleep_timo(struct tty *tp, void *chan, int pri, char *wmesg, int timo)
+{
  int error;
  short gen;
 
  gen = tp->t_gen;
- if ((error = tsleep_nsec(chan, pri, wmesg, INFSLP)) != 0)
+ if ((error = tsleep_nsec(chan, pri, wmesg, timo)) != 0)
  return (error);
  return (tp->t_gen == gen ? 0 : ERESTART);
 }
Index: sys/tty.h
===================================================================
RCS file: /cvs/src/sys/sys/tty.h,v
retrieving revision 1.38
diff -u -p -r1.38 tty.h
--- sys/tty.h 19 Jul 2019 00:17:16 -0000 1.38
+++ sys/tty.h 2 May 2020 08:35:03 -0000
@@ -293,7 +293,9 @@ void ttypend(struct tty *tp);
 void ttyretype(struct tty *tp);
 void ttyrub(int c, struct tty *tp);
 int ttysleep(struct tty *tp, void *chan, int pri, char *wmesg);
-int ttywait(struct tty *tp);
+int ttysleep_timo(struct tty *tp, void *chan, int pri, char *wmesg,
+    int timo);
+int ttywait(struct tty *tp, int timo);
 int ttywflush(struct tty *tp);
 void ttytstamp(struct tty *tp, int octs, int ncts, int odcd, int ndcd);
 

Reply | Threaded
Open this post in threaded view
|

Re: pty leak or corruption w/ openpty + dup2?

Martin Pieuchot
On 02/05/20(Sat) 10:40, Anton Lindqvist wrote:

> On Fri, May 01, 2020 at 05:17:36PM +0200, Martin Pieuchot wrote:
> > On 01/05/20(Fri) 12:13, Anton Lindqvist wrote:
> > > The order in which the pty master/slave is closed seems to be the
> > > trigger here. While not duping the master, it's closed before the slave.
> > > In the opposite scenario, the slave is closed before the master. While
> > > closing the slave, it ends up here expressed as a simplified backtrace:
> > >
> > >   tsleep()
> > >   ttysleep()
> > >   ttywait()
> > >   ttywflush()
> > >   ttylclose()
> > >   ptsclose()
> > >   fdfree()
> > >   exit1()
> > >
> > > In order words, it ends up doing a tsleep(INFSLP) causing the thread to
> > > hang. Note that this is not the case when the master is closed before
> > > the slave since `tp->t_oproc == NULL' causing ttywait() to bail early.
> >
> > Why is the sleeper never awaken?  Does that mean a ttwakeup() is missing?
>
> In this case, the process is single threaded, about to exit and the only
> consumer of the pty. I don't see how it could be any other process
> responsibility to perform the wakeup.

Do we see that the issue is caused by the order in which descriptors are
closed in fdfree()?  The current deadlock occurs because the duped master
has a higher fd number than the slave which means it is still open when the
slave is closed.

But why would that be a problem?  By default *close() functions,
including ttylclose() are blocking.  So any exiting process might end up
hanging in fdfree().  Diff below illustrates that by forcing all *close()
during exit1() to be non-blocking, it also fix the issue.

Does it make sense to close fds as non-blocking when existing?  What
should a dying thread wait for?  What can be the cons of such approach?

Now regarding your fix, why does it make sense to wait 5sec instead of
indefinitely?  Did you look at r1.263 of NetBSD's kern/tty.c?  If we go
with this change could you please change the 'timo' suffix and variables
to 'nsec' and use uint64_t instead of int?

Index: kern/vfs_vnops.c
===================================================================
RCS file: /cvs/src/sys/kern/vfs_vnops.c,v
retrieving revision 1.114
diff -u -p -r1.114 vfs_vnops.c
--- kern/vfs_vnops.c 8 Apr 2020 08:07:51 -0000 1.114
+++ kern/vfs_vnops.c 2 May 2020 09:18:28 -0000
@@ -601,6 +601,7 @@ vn_closefile(struct file *fp, struct pro
 {
  struct vnode *vp = fp->f_data;
  struct flock lf;
+ unsigned int flag;
  int error;
 
  KERNEL_LOCK();
@@ -611,7 +612,10 @@ vn_closefile(struct file *fp, struct pro
  lf.l_type = F_UNLCK;
  (void) VOP_ADVLOCK(vp, (caddr_t)fp, F_UNLCK, &lf, F_FLOCK);
  }
- error = vn_close(vp, fp->f_flag, fp->f_cred, p);
+ flag = fp->f_flag;
+ if (p != NULL && p->p_flag & P_WEXIT)
+ flag |= O_NONBLOCK;
+ error = vn_close(vp, flag, fp->f_cred, p);
  KERNEL_UNLOCK();
  return (error);
 }

Reply | Threaded
Open this post in threaded view
|

Re: pty leak or corruption w/ openpty + dup2?

Mark Kettenis
> Date: Sat, 2 May 2020 11:33:17 +0200
> From: Martin Pieuchot <[hidden email]>
>
> On 02/05/20(Sat) 10:40, Anton Lindqvist wrote:
> > On Fri, May 01, 2020 at 05:17:36PM +0200, Martin Pieuchot wrote:
> > > On 01/05/20(Fri) 12:13, Anton Lindqvist wrote:
> > > > The order in which the pty master/slave is closed seems to be the
> > > > trigger here. While not duping the master, it's closed before the slave.
> > > > In the opposite scenario, the slave is closed before the master. While
> > > > closing the slave, it ends up here expressed as a simplified backtrace:
> > > >
> > > >   tsleep()
> > > >   ttysleep()
> > > >   ttywait()
> > > >   ttywflush()
> > > >   ttylclose()
> > > >   ptsclose()
> > > >   fdfree()
> > > >   exit1()
> > > >
> > > > In order words, it ends up doing a tsleep(INFSLP) causing the thread to
> > > > hang. Note that this is not the case when the master is closed before
> > > > the slave since `tp->t_oproc == NULL' causing ttywait() to bail early.
> > >
> > > Why is the sleeper never awaken?  Does that mean a ttwakeup() is missing?
> >
> > In this case, the process is single threaded, about to exit and the only
> > consumer of the pty. I don't see how it could be any other process
> > responsibility to perform the wakeup.
>
> Do we see that the issue is caused by the order in which descriptors are
> closed in fdfree()?  The current deadlock occurs because the duped master
> has a higher fd number than the slave which means it is still open when the
> slave is closed.

I'm sure we could construct an example where the file descriptors are
in a different oder.  So changing the order is not going to help.

> But why would that be a problem?  By default *close() functions,
> including ttylclose() are blocking.  So any exiting process might end up
> hanging in fdfree().  Diff below illustrates that by forcing all *close()
> during exit1() to be non-blocking, it also fix the issue.

I very much fear that is going to have unintended side-effects with
output not being flushed properly.  And the process could still
deadlock itself by using close(2) directly isn't it?

> Does it make sense to close fds as non-blocking when existing?  What
> should a dying thread wait for?  What can be the cons of such approach?
>
> Now regarding your fix, why does it make sense to wait 5sec instead of
> indefinitely?  Did you look at r1.263 of NetBSD's kern/tty.c?  If we go
> with this change could you please change the 'timo' suffix and variables
> to 'nsec' and use uint64_t instead of int?

r1.263 was reverted in r1.264.  Then r1.265 is the commit quoted by anton@.
There is also r2.267 which adds an additional fix to r1.265.

In ttywait(), NetBSD only calls ttyflush() if there is a timeout. That
makes sense, because we have ttywflush() to combine the wait and flush
so ttywait() shouldn't flush when there is no error.


> Index: kern/vfs_vnops.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/vfs_vnops.c,v
> retrieving revision 1.114
> diff -u -p -r1.114 vfs_vnops.c
> --- kern/vfs_vnops.c 8 Apr 2020 08:07:51 -0000 1.114
> +++ kern/vfs_vnops.c 2 May 2020 09:18:28 -0000
> @@ -601,6 +601,7 @@ vn_closefile(struct file *fp, struct pro
>  {
>   struct vnode *vp = fp->f_data;
>   struct flock lf;
> + unsigned int flag;
>   int error;
>  
>   KERNEL_LOCK();
> @@ -611,7 +612,10 @@ vn_closefile(struct file *fp, struct pro
>   lf.l_type = F_UNLCK;
>   (void) VOP_ADVLOCK(vp, (caddr_t)fp, F_UNLCK, &lf, F_FLOCK);
>   }
> - error = vn_close(vp, fp->f_flag, fp->f_cred, p);
> + flag = fp->f_flag;
> + if (p != NULL && p->p_flag & P_WEXIT)
> + flag |= O_NONBLOCK;
> + error = vn_close(vp, flag, fp->f_cred, p);
>   KERNEL_UNLOCK();
>   return (error);
>  }
>
>

Reply | Threaded
Open this post in threaded view
|

Re: pty leak or corruption w/ openpty + dup2?

Martin Pieuchot
On 02/05/20(Sat) 16:02, Mark Kettenis wrote:

> > Date: Sat, 2 May 2020 11:33:17 +0200
> > From: Martin Pieuchot <[hidden email]>
> > [...]
> > Do we see that the issue is caused by the order in which descriptors are
> > closed in fdfree()?  The current deadlock occurs because the duped master
> > has a higher fd number than the slave which means it is still open when the
> > slave is closed.
>
> I'm sure we could construct an example where the file descriptors are
> in a different oder.  So changing the order is not going to help.

Obviously :)

> > But why would that be a problem?  By default *close() functions,
> > including ttylclose() are blocking.  So any exiting process might end up
> > hanging in fdfree().  Diff below illustrates that by forcing all *close()
> > during exit1() to be non-blocking, it also fix the issue.
>
> I very much fear that is going to have unintended side-effects with
> output not being flushed properly.  And the process could still
> deadlock itself by using close(2) directly isn't it?

Indeed.

> > Does it make sense to close fds as non-blocking when existing?  What
> > should a dying thread wait for?  What can be the cons of such approach?
> >
> > Now regarding your fix, why does it make sense to wait 5sec instead of
> > indefinitely?  Did you look at r1.263 of NetBSD's kern/tty.c?  If we go
> > with this change could you please change the 'timo' suffix and variables
> > to 'nsec' and use uint64_t instead of int?
>
> r1.263 was reverted in r1.264.  Then r1.265 is the commit quoted by anton@.
> There is also r2.267 which adds an additional fix to r1.265.
>
> In ttywait(), NetBSD only calls ttyflush() if there is a timeout. That
> makes sense, because we have ttywflush() to combine the wait and flush
> so ttywait() shouldn't flush when there is no error.

Updated diff below reflecting those changes.  I'm still questioning the
5sec timeout, but it is without doubt an improvement over the current
behavior.

The previously mentioned test as well as a modified version closing the
slave before exit(2) now hang for 5 seconds instead of deadlocking
indefinitely.

I believe we want that for release, ok?

Index: kern/tty.c
===================================================================
RCS file: /cvs/src/sys/kern/tty.c,v
retrieving revision 1.154
diff -u -p -r1.154 tty.c
--- kern/tty.c 7 Apr 2020 13:27:51 -0000 1.154
+++ kern/tty.c 6 May 2020 07:44:53 -0000
@@ -80,6 +80,8 @@ void filt_ttyrdetach(struct knote *kn);
 int filt_ttywrite(struct knote *kn, long hint);
 void filt_ttywdetach(struct knote *kn);
 void ttystats_init(struct itty **, size_t *);
+int ttywait_nsec(struct tty *tp, uint64_t nsecs);
+int ttysleep_nsec(struct tty *, void *, int, char *, uint64_t);
 
 /* Symbolic sleep message strings. */
 char ttclos[] = "ttycls";
@@ -1202,10 +1204,10 @@ ttnread(struct tty *tp)
 }
 
 /*
- * Wait for output to drain.
+ * Wait for output to drain, or if this times out, flush it.
  */
 int
-ttywait(struct tty *tp)
+ttywait_nsec(struct tty *tp, uint64_t nsecs)
 {
  int error, s;
 
@@ -1219,7 +1221,10 @@ ttywait(struct tty *tp)
     (ISSET(tp->t_state, TS_CARR_ON) || ISSET(tp->t_cflag, CLOCAL))
     && tp->t_oproc) {
  SET(tp->t_state, TS_ASLEEP);
- error = ttysleep(tp, &tp->t_outq, TTOPRI | PCATCH, ttyout);
+ error = ttysleep_nsec(tp, &tp->t_outq, TTOPRI | PCATCH,
+    ttyout, nsecs);
+ if (error == EWOULDBLOCK)
+ ttyflush(tp, FWRITE);
  if (error)
  break;
  } else
@@ -1229,6 +1234,12 @@ ttywait(struct tty *tp)
  return (error);
 }
 
+int
+ttywait(struct tty *tp)
+{
+ return (ttywait_nsec(tp, INFSLP));
+}
+
 /*
  * Flush if successfully wait.
  */
@@ -1237,7 +1248,8 @@ ttywflush(struct tty *tp)
 {
  int error;
 
- if ((error = ttywait(tp)) == 0)
+ error = ttywait_nsec(tp, SEC_TO_NSEC(5));
+ if (error == 0 || error == EWOULDBLOCK)
  ttyflush(tp, FREAD);
  return (error);
 }
@@ -2281,11 +2293,18 @@ tputchar(int c, struct tty *tp)
 int
 ttysleep(struct tty *tp, void *chan, int pri, char *wmesg)
 {
+
+ return (ttysleep_nsec(tp, chan, pri, wmesg, INFSLP));
+}
+
+int
+ttysleep_nsec(struct tty *tp, void *chan, int pri, char *wmesg, uint64_t nsecs)
+{
  int error;
  short gen;
 
  gen = tp->t_gen;
- if ((error = tsleep_nsec(chan, pri, wmesg, INFSLP)) != 0)
+ if ((error = tsleep_nsec(chan, pri, wmesg, nsecs)) != 0)
  return (error);
  return (tp->t_gen == gen ? 0 : ERESTART);
 }

Reply | Threaded
Open this post in threaded view
|

Re: pty leak or corruption w/ openpty + dup2?

Mark Kettenis
> Date: Wed, 6 May 2020 09:53:56 +0200
> From: Martin Pieuchot <[hidden email]>
> Cc: [hidden email]
>
> On 02/05/20(Sat) 16:02, Mark Kettenis wrote:
> > > Date: Sat, 2 May 2020 11:33:17 +0200
> > > From: Martin Pieuchot <[hidden email]>
> > > [...]
> > > Do we see that the issue is caused by the order in which descriptors are
> > > closed in fdfree()?  The current deadlock occurs because the duped master
> > > has a higher fd number than the slave which means it is still open when the
> > > slave is closed.
> >
> > I'm sure we could construct an example where the file descriptors are
> > in a different oder.  So changing the order is not going to help.
>
> Obviously :)
>
> > > But why would that be a problem?  By default *close() functions,
> > > including ttylclose() are blocking.  So any exiting process might end up
> > > hanging in fdfree().  Diff below illustrates that by forcing all *close()
> > > during exit1() to be non-blocking, it also fix the issue.
> >
> > I very much fear that is going to have unintended side-effects with
> > output not being flushed properly.  And the process could still
> > deadlock itself by using close(2) directly isn't it?
>
> Indeed.
>
> > > Does it make sense to close fds as non-blocking when existing?  What
> > > should a dying thread wait for?  What can be the cons of such approach?
> > >
> > > Now regarding your fix, why does it make sense to wait 5sec instead of
> > > indefinitely?  Did you look at r1.263 of NetBSD's kern/tty.c?  If we go
> > > with this change could you please change the 'timo' suffix and variables
> > > to 'nsec' and use uint64_t instead of int?
> >
> > r1.263 was reverted in r1.264.  Then r1.265 is the commit quoted by anton@.
> > There is also r2.267 which adds an additional fix to r1.265.
> >
> > In ttywait(), NetBSD only calls ttyflush() if there is a timeout. That
> > makes sense, because we have ttywflush() to combine the wait and flush
> > so ttywait() shouldn't flush when there is no error.
>
> Updated diff below reflecting those changes.  I'm still questioning the
> 5sec timeout, but it is without doubt an improvement over the current
> behavior.
>
> The previously mentioned test as well as a modified version closing the
> slave before exit(2) now hang for 5 seconds instead of deadlocking
> indefinitely.
>
> I believe we want that for release, ok?

I'm not sure.  I can't really judge the impact of this on serial ports
for example.  I think it may take some time for any potential problems
to surface.  And I think this bug has been with us for many years.

My understanding is that the impact of the bug is rather limited.
Users can DOS themselves with this artificially constructed testcase,
but there are many other ways to do this.  I would wait with this
until after the release.


> Index: kern/tty.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/tty.c,v
> retrieving revision 1.154
> diff -u -p -r1.154 tty.c
> --- kern/tty.c 7 Apr 2020 13:27:51 -0000 1.154
> +++ kern/tty.c 6 May 2020 07:44:53 -0000
> @@ -80,6 +80,8 @@ void filt_ttyrdetach(struct knote *kn);
>  int filt_ttywrite(struct knote *kn, long hint);
>  void filt_ttywdetach(struct knote *kn);
>  void ttystats_init(struct itty **, size_t *);
> +int ttywait_nsec(struct tty *tp, uint64_t nsecs);
> +int ttysleep_nsec(struct tty *, void *, int, char *, uint64_t);
>  
>  /* Symbolic sleep message strings. */
>  char ttclos[] = "ttycls";
> @@ -1202,10 +1204,10 @@ ttnread(struct tty *tp)
>  }
>  
>  /*
> - * Wait for output to drain.
> + * Wait for output to drain, or if this times out, flush it.
>   */
>  int
> -ttywait(struct tty *tp)
> +ttywait_nsec(struct tty *tp, uint64_t nsecs)
>  {
>   int error, s;
>  
> @@ -1219,7 +1221,10 @@ ttywait(struct tty *tp)
>      (ISSET(tp->t_state, TS_CARR_ON) || ISSET(tp->t_cflag, CLOCAL))
>      && tp->t_oproc) {
>   SET(tp->t_state, TS_ASLEEP);
> - error = ttysleep(tp, &tp->t_outq, TTOPRI | PCATCH, ttyout);
> + error = ttysleep_nsec(tp, &tp->t_outq, TTOPRI | PCATCH,
> +    ttyout, nsecs);
> + if (error == EWOULDBLOCK)
> + ttyflush(tp, FWRITE);
>   if (error)
>   break;
>   } else
> @@ -1229,6 +1234,12 @@ ttywait(struct tty *tp)
>   return (error);
>  }
>  
> +int
> +ttywait(struct tty *tp)
> +{
> + return (ttywait_nsec(tp, INFSLP));
> +}
> +
>  /*
>   * Flush if successfully wait.
>   */
> @@ -1237,7 +1248,8 @@ ttywflush(struct tty *tp)
>  {
>   int error;
>  
> - if ((error = ttywait(tp)) == 0)
> + error = ttywait_nsec(tp, SEC_TO_NSEC(5));
> + if (error == 0 || error == EWOULDBLOCK)
>   ttyflush(tp, FREAD);
>   return (error);
>  }
> @@ -2281,11 +2293,18 @@ tputchar(int c, struct tty *tp)
>  int
>  ttysleep(struct tty *tp, void *chan, int pri, char *wmesg)
>  {
> +

Please don't add these silly NetBSD-style empty lines at the start of
functions without local variables.

> + return (ttysleep_nsec(tp, chan, pri, wmesg, INFSLP));
> +}
> +
> +int
> +ttysleep_nsec(struct tty *tp, void *chan, int pri, char *wmesg, uint64_t nsecs)
> +{
>   int error;
>   short gen;
>  
>   gen = tp->t_gen;
> - if ((error = tsleep_nsec(chan, pri, wmesg, INFSLP)) != 0)
> + if ((error = tsleep_nsec(chan, pri, wmesg, nsecs)) != 0)
>   return (error);
>   return (tp->t_gen == gen ? 0 : ERESTART);
>  }
>