On Thu, Oct 21, 2021 at 08:51:13PM +0200, Anton Lindqvist wrote:
> On Wed, Oct 20, 2021 at 05:14:08PM +0200, Martin Pieuchot wrote:
> > On 15/10/21(Fri) 16:33, Anton Lindqvist wrote:
> > > On Fri, Oct 15, 2021 at 03:30:40PM +0200, Alexander Bluhm wrote:
> > > > On Thu, Oct 14, 2021 at 11:32:49PM -0600, Theo de Raadt wrote:
> > > > > Obviously.
> > > > >
> > > > > Back it out, ASAP, then try to repair.
> > > > >
> > > > > It is quite surprising there aren't enough regression tests to catch
> > > > > something like this.
> > > >
> > > > I do not see any problem with this diff on my regress machines.
> > > > My latest amd64 snap is this one, does it contain the commit?
> > > >
> > > > OpenBSD 7.0-current (GENERIC.MP) #37: Thu Oct 14 12:29:37 MDT 2021
> > > > [email protected]:/usr/src/sys/arch/amd64/compile/GENERIC.MP
> > > >
> > > > Anton, is there something special with your setup?
> > >
> > > I don't think so, other than my machine being slower than yours.
> > >
> > > > Have you tried to backout this diff to confirm that it is related?
> > >
> > > Backing out the diff made all tests pass again.
> >
> > I wonder if you aren't hitting a corner case in the pipe's kqueue
> > handler which is returning EPIPE. This might be relevant to timing.
> >
> > Ironically the poll(2) part of my diff already handles EPIPE...
> >
> > >
> > > mpi@, let me know if I can help in anyway.
> >
> > Could you try the diff below? It's the same as before plus a special
> > check for EPIPE.
> >
> > If you can still reproduce the issue with it, please enter ddb(4) and set
> > `kpoll_debug' to 1 to get more debug informations.
>
> You're pointing in the right direction but the pipe semantics for kqueue
> and select are still different. With your diff, the t-scp3 test case
> hangs in kqread when the end of the pipe is gone.
>
> pipe_poll() returns successfully (i.e. POLLHUP) when the other end is
> gone causing select to flag the fd as ready. Interacting with the pipe
> fd will fail with EPIPE at this point.
>
> Instead of masking EPIPE in pselregister(), here's an attempt to
> preserve the same semantics. I've put together a deterministic
> reproducer, coming up in a separate reply.
diff --git regress/sys/kern/pipe/Makefile regress/sys/kern/pipe/Makefile
index b2ee4cfc406..8e61aec5077 100644
--- regress/sys/kern/pipe/Makefile
+++ regress/sys/kern/pipe/Makefile
@@ -7,6 +7,7 @@ SRCS+= test-close.c
SRCS+= test-kqueue.c
SRCS+= test-ping-pong.c
SRCS+= test-run-down.c
+SRCS+= test-select.c
SRCS+= test-thundering-herd.c
LDADD+= -lpthread
@@ -22,6 +23,7 @@ TESTS+= kqueue-write-eof
TESTS+= ping-pong
TESTS+= run-down-write-big
TESTS+= run-down-write-small
+TESTS+= select-hup
TESTS+= thundering-herd-read-signal
TESTS+= thundering-herd-read-wakeup
TESTS+= thundering-herd-write-signal
diff --git regress/sys/kern/pipe/pipe.c regress/sys/kern/pipe/pipe.c
index 1352b543897..52911f7a7a3 100644
--- regress/sys/kern/pipe/pipe.c
+++ regress/sys/kern/pipe/pipe.c
@@ -48,6 +48,7 @@ main(int argc, char *argv[])
{ "ping-pong", test_ping_pong },
{ "run-down-write-big", test_run_down_write_big
},
{ "run-down-write-small",
test_run_down_write_small },
+ { "select-hup", test_select_hup },
{ "thundering-herd-read-signal",
test_thundering_herd_read_signal },
{ "thundering-herd-read-wakeup",
test_thundering_herd_read_wakeup },
{ "thundering-herd-write-signal",
test_thundering_herd_write_signal },
diff --git regress/sys/kern/pipe/pipe.h regress/sys/kern/pipe/pipe.h
index 93b2f316661..88913065f7f 100644
--- regress/sys/kern/pipe/pipe.h
+++ regress/sys/kern/pipe/pipe.h
@@ -28,6 +28,7 @@ int test_kqueue_write_eof(void);
int test_ping_pong(void);
int test_run_down_write_big(void);
int test_run_down_write_small(void);
+int test_select_hup(void);
int test_thundering_herd_read_signal(void);
int test_thundering_herd_read_wakeup(void);
int test_thundering_herd_write_signal(void);
diff --git regress/sys/kern/pipe/test-select.c
regress/sys/kern/pipe/test-select.c
new file mode 100644
index 00000000000..438b664ce2f
--- /dev/null
+++ regress/sys/kern/pipe/test-select.c
@@ -0,0 +1,57 @@
+/* $OpenBSD$ */
+
+/*
+ * Copyright (c) 2021 Anton Lindqvist <[email protected]>
+ *
+ * Permission to use, copy, modify, and distribute this software for any
+ * purpose with or without fee is hereby granted, provided that the above
+ * copyright notice and this permission notice appear in all copies.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
+ * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
+ * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
+ * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
+ * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
+ * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
+ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+ */
+
+#include <sys/select.h>
+
+#include <err.h>
+#include <errno.h>
+#include <unistd.h>
+
+#include "pipe.h"
+
+/*
+ * Verify select(2) hangup semantics.
+ */
+int
+test_select_hup(void)
+{
+ fd_set writefds;
+ ssize_t n;
+ int pip[2];
+ int nready;
+ char c = 'c';
+
+ if (pipe(pip) == -1)
+ err(1, "pipe");
+ close(pip[0]);
+
+ FD_ZERO(&writefds);
+ FD_SET(pip[1], &writefds);
+ nready = select(pip[1] + 1, NULL, &writefds, NULL, NULL);
+ if (nready == -1)
+ err(1, "select");
+ if (nready != 1)
+ errx(1, "select: want 1, got %d", nready);
+ n = write(pip[1], &c, sizeof(c));
+ if (n != -1)
+ errx(1, "read: want -1, got %zd", n);
+ if (errno != EPIPE)
+ errx(1, "errno: want %d, got %d", EPIPE, errno);
+
+ return 0;
+}
