[PATCH v2 0/3] More caution with NONBLOCK flag on Unix sockets

David Gibson

18 May 2026 18 May '26

5:22 a.m.

This is a revision of the one patch from my pesto series that wasn't merged: a rework to the handling of the blocking flag for both listening and accepted Unix sockets. This new version is split into several pieces to make the rationales clearer at each step. It's also a bit more cautious in what it does, so should avoid the problems with the original version. David Gibson (3): treewide: Add SOCK_CLOEXEC to accept() calls that are missing it tap: Report accept() errors conf, repair, tap: Document reasons for blocking Unix sockets conf.c | 6 ++++++ repair.c | 9 +++++++-- tap.c | 13 +++++++++++-- 3 files changed, 24 insertions(+), 4 deletions(-) -- 2.54.0

Show replies by date

David Gibson

18 May 18 May

5:22 a.m.

New subject: [PATCH v2 1/3] treewide: Add SOCK_CLOEXEC to accept() calls that are missing it

Generally we try to set the O_CLOEXEC flag on every fd we create. This seems to be generally accepted security best practice these days, and we never exec(), so certainly have no need to pass fds to exec()ed processes. A handful of accept4() calls on Unix sockets are missing the SOCK_CLOEXEC flag to set this though. Add the missing flag. Signed-off-by: David Gibson --- repair.c | 5 +++-- tap.c | 4 ++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/repair.c b/repair.c index 69c53077..3e0e3e0a 100644 --- a/repair.c +++ b/repair.c @@ -87,7 +87,7 @@ int repair_listen_handler(struct ctx *c, uint32_t events) /* Another client is already connected: accept and close right away. */ if (c->fd_repair != -1) { int discard = accept4(c->fd_repair_listen, NULL, NULL, - SOCK_NONBLOCK); + SOCK_NONBLOCK | SOCK_CLOEXEC); if (discard == -1) return errno; @@ -99,7 +99,8 @@ int repair_listen_handler(struct ctx *c, uint32_t events) return EEXIST; } - if ((c->fd_repair = accept4(c->fd_repair_listen, NULL, NULL, 0)) < 0) { + if ((c->fd_repair = accept4(c->fd_repair_listen, NULL, NULL, + SOCK_CLOEXEC)) < 0) { rc = errno; debug_perror("accept4() on TCP_REPAIR helper listening socket"); return rc; diff --git a/tap.c b/tap.c index 0920a325..e7cac9df 100644 --- a/tap.c +++ b/tap.c @@ -1477,7 +1477,7 @@ void tap_listen_handler(struct ctx *c, uint32_t events) /* Another client is already connected: accept and close right away. */ if (c->fd_tap != -1) { int discard = accept4(c->fd_tap_listen, NULL, NULL, - SOCK_NONBLOCK); + SOCK_NONBLOCK | SOCK_CLOEXEC); if (discard == -1) return; @@ -1490,7 +1490,7 @@ void tap_listen_handler(struct ctx *c, uint32_t events) return; } - c->fd_tap = accept4(c->fd_tap_listen, NULL, NULL, 0); + c->fd_tap = accept4(c->fd_tap_listen, NULL, NULL, SOCK_CLOEXEC); if (!getsockopt(c->fd_tap, SOL_SOCKET, SO_PEERCRED, &ucred, &len)) info("accepted connection from PID %i", ucred.pid); -- 2.54.0

David Gibson

5:22 a.m.

New subject: [PATCH v2 3/3] conf, repair, tap: Document reasons for blocking Unix sockets

Most of our operation is asynchronous, based on non-blocking fds handled in our epoll loop. However, our several Unix sockets (tap client, repair helper, control client) are all blocking fds after accept(). That is in fact correct, but for not especially obvious reasons that are slightly different in each case. Add explanatory comments to each of them. Signed-off-by: David Gibson --- conf.c | 6 ++++++ repair.c | 4 ++++ tap.c | 5 +++++ 3 files changed, 15 insertions(+) diff --git a/conf.c b/conf.c index 029b9c7c..5c7dfea1 100644 --- a/conf.c +++ b/conf.c @@ -2084,6 +2084,12 @@ static void conf_accept(struct ctx *c) int fd, rc; retry: + /* Currently we perform the configuration transaction more-or-less + * synchronously, so we want the accepted socket to be blocking. + * + * FIXME: We should make the configuration update asynchronous, like + * most of our operation, so a misbehaving configuration client can't + * block the main forwarding loop. */ fd = accept4(c->fd_control_listen, NULL, NULL, SOCK_CLOEXEC); if (fd < 0) { if (errno != EAGAIN) diff --git a/repair.c b/repair.c index 3e0e3e0a..f31cccee 100644 --- a/repair.c +++ b/repair.c @@ -99,6 +99,10 @@ int repair_listen_handler(struct ctx *c, uint32_t events) return EEXIST; } + /* We want the accepted socket to be blocking; we use it during + * migration which is a synchronous interruption to our normal + * non-blocking behaviour. + */ if ((c->fd_repair = accept4(c->fd_repair_listen, NULL, NULL, SOCK_CLOEXEC)) < 0) { rc = errno; diff --git a/tap.c b/tap.c index 660f1cb6..d73068e5 100644 --- a/tap.c +++ b/tap.c @@ -1490,6 +1490,11 @@ void tap_listen_handler(struct ctx *c, uint32_t events) return; } + /* Because we generally only access the accepted socket from epoll + * events, it usually doesn't matter if it's blocking or non-blocking. + * However, in rare cases when the socket buffer fills we need (briefly, + * we hope) blocking writes (write_remainder() in send_frames_passt()). + */ c->fd_tap = accept4(c->fd_tap_listen, NULL, NULL, SOCK_CLOEXEC); if (c->fd_tap < 0) { warn_perror("Error accepting tap client"); -- 2.54.0

Stefano Brivio

20 May 20 May

2:52 a.m.

New subject: [PATCH v2 3/3] conf, repair, tap: Document reasons for blocking Unix sockets

On Mon, 18 May 2026 13:22:43 +1000 David Gibson wrote:

...

Most of our operation is asynchronous, based on non-blocking fds handled in our epoll loop. However, our several Unix sockets (tap client, repair helper, control client) are all blocking fds after accept().

That is in fact correct, but for not especially obvious reasons that are slightly different in each case. Add explanatory comments to each of them.

Signed-off-by: David Gibson --- conf.c | 6 ++++++ repair.c | 4 ++++ tap.c | 5 +++++ 3 files changed, 15 insertions(+)

diff --git a/conf.c b/conf.c index 029b9c7c..5c7dfea1 100644 --- a/conf.c +++ b/conf.c @@ -2084,6 +2084,12 @@ static void conf_accept(struct ctx *c) int fd, rc;

retry: + /* Currently we perform the configuration transaction more-or-less + * synchronously, so we want the accepted socket to be blocking. + * + * FIXME: We should make the configuration update asynchronous, like + * most of our operation, so a misbehaving configuration client can't + * block the main forwarding loop. */

Nit I fixed on merge: * [...] loop. */ -- Stefano

Stefano Brivio

2:52 a.m.

On Mon, 18 May 2026 13:22:40 +1000 David Gibson wrote:

...

This is a revision of the one patch from my pesto series that wasn't merged: a rework to the handling of the blocking flag for both listening and accepted Unix sockets. This new version is split into several pieces to make the rationales clearer at each step. It's also a bit more cautious in what it does, so should avoid the problems with the original version.

David Gibson (3): treewide: Add SOCK_CLOEXEC to accept() calls that are missing it tap: Report accept() errors conf, repair, tap: Document reasons for blocking Unix sockets

Applied, with the nit I reported on 3/3 fixed up. -- Stefano

David Gibson

3:13 a.m.

New subject: [PATCH v2 3/3] conf, repair, tap: Document reasons for blocking Unix sockets

On Wed, May 20, 2026 at 02:52:14AM +0200, Stefano Brivio wrote:

...

On Mon, 18 May 2026 13:22:43 +1000 David Gibson wrote:

...
Most of our operation is asynchronous, based on non-blocking fds handled in our epoll loop. However, our several Unix sockets (tap client, repair helper, control client) are all blocking fds after accept().

That is in fact correct, but for not especially obvious reasons that are slightly different in each case. Add explanatory comments to each of them.

Signed-off-by: David Gibson --- conf.c | 6 ++++++ repair.c | 4 ++++ tap.c | 5 +++++ 3 files changed, 15 insertions(+)

diff --git a/conf.c b/conf.c index 029b9c7c..5c7dfea1 100644 --- a/conf.c +++ b/conf.c @@ -2084,6 +2084,12 @@ static void conf_accept(struct ctx *c) int fd, rc;

retry: + /* Currently we perform the configuration transaction more-or-less + * synchronously, so we want the accepted socket to be blocking. + * + * FIXME: We should make the configuration update asynchronous, like + * most of our operation, so a misbehaving configuration client can't + * block the main forwarding loop. */

Nit I fixed on merge:

* [...] loop. */

Oh, thanks. -- David Gibson (he or they) | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you, not the other way | around. http://www.ozlabs.org/~dgibson

Age (days ago)

Last active (days ago)

List overview

Download

5 comments

2 participants

participants (2)

David Gibson
Stefano Brivio