On Tue, 24 Mar 2026 16:24:05 -0400 Aaron Conole <[email protected]> wrote:
> Timothy Redaelli via dev <[email protected]> writes: > > > Add a new "pfd:" passive stream class that accepts a pre-opened file > > descriptor number. This is the core building block for systemd socket > > activation, where systemd opens and binds the listening socket before > > starting the service. > > > > The pfd_open() function validates that the file descriptor refers to > > a listening stream socket via getsockopt(SO_TYPE) and > > getsockopt(SO_ACCEPTCONN), sets it non-blocking, and wraps it in an > > fd_pstream. Unlike punix:, the unlink_path is NULL because the > > service does not own the socket file. > > > > For security, pfd: remotes are restricted to the command line > > (--remote=pfd:N). Runtime addition via ovsdb-server/add-remote > > or the database is rejected at three entry points > > (ovsdb_server_add_remote, add_manager_options, query_db_remotes), > > preventing an attacker with database write access from hijacking > > arbitrary file descriptors. > > > > Reported-at: https://issues.redhat.com/browse/FDP-3413 > > Co-authored-by: Lubomir Rintel <[email protected]> > > Signed-off-by: Lubomir Rintel <[email protected]> > > Signed-off-by: Timothy Redaelli <[email protected]> > > --- > > Documentation/ref/ovsdb.7.rst | 12 ++++++++ > > lib/stream-provider.h | 1 + > > lib/stream-unix.c | 53 +++++++++++++++++++++++++++++++++++ > > lib/stream.c | 5 ++++ > > ovsdb/ovsdb-server.c | 23 ++++++++++++++- > > 5 files changed, 93 insertions(+), 1 deletion(-) > > > > diff --git a/Documentation/ref/ovsdb.7.rst b/Documentation/ref/ovsdb.7.rst > > index 42541dd7e..cf1ef3736 100644 > > --- a/Documentation/ref/ovsdb.7.rst > > +++ b/Documentation/ref/ovsdb.7.rst > > @@ -709,6 +709,18 @@ punix:<file> > > <file> to mimic the behavior of a Unix domain socket. The ACLs of the > > named > > pipe include LocalSystem, Administrators, and Creator Owner. > > > > +pfd:<fd> > > + Listen on a pre-opened file descriptor <fd>. The file descriptor must > > + refer to a bound, listening Unix domain stream socket. This is > > intended > > + for use with systemd socket activation, where systemd opens the socket > > + and passes it to the service. > > + > > + For security, ``pfd:`` may only be specified on the command line > > + (``--remote=pfd:<fd>``). It is rejected if added at runtime via > > + ``ovsdb-server/add-remote`` or through the database. > > + > > + This connection method is not supported on Windows. > > + > > All IP-based connection methods accept IPv4 and IPv6 addresses. To > > specify an > > IPv6 address, wrap it in square brackets, e.g. ``ssl:[::1]:6640``. > > Passive > > IP-based connection methods by default listen for IPv4 connections only; > > use > > diff --git a/lib/stream-provider.h b/lib/stream-provider.h > > index 44e3c6431..ddd468b09 100644 > > --- a/lib/stream-provider.h > > +++ b/lib/stream-provider.h > > @@ -195,6 +195,7 @@ extern const struct pstream_class ptcp_pstream_class; > > #ifndef _WIN32 > > extern const struct stream_class unix_stream_class; > > extern const struct pstream_class punix_pstream_class; > > +extern const struct pstream_class pfd_pstream_class; > > #else > > extern const struct stream_class windows_stream_class; > > extern const struct pstream_class pwindows_pstream_class; > > diff --git a/lib/stream-unix.c b/lib/stream-unix.c > > index d265efb83..5cb4e93d2 100644 > > --- a/lib/stream-unix.c > > +++ b/lib/stream-unix.c > > @@ -136,3 +136,56 @@ const struct pstream_class punix_pstream_class = { > > NULL, > > }; > > > > +/* Pre-opened file descriptor passive stream. > > + * > > + * Used for systemd socket activation: systemd opens and binds the socket, > > + * then passes it to the service as a pre-opened file descriptor. */ > > + > > +static int > > +pfd_open(const char *name, char *suffix, > > + struct pstream **pstreamp, uint8_t dscp OVS_UNUSED) > > +{ > > + char *end; > > + long fd; > > + > > + fd = strtol(suffix, &end, 10); > > + if (*end != '\0' || fd < 0 || fd > INT_MAX) { > > + VLOG_ERR("%s: bad file descriptor", name); > > + return ERANGE; > > + } > > I would change this to: > > if (!str_to_long(suffix, 10, &fd) || fd < 0) { > VLOG_ERR("%s: bad file descriptor", name); > return EINVAL; > } done in v2, thanks. > > + /* Verify it is a listening stream socket. */ > > + int sock_type; > > + socklen_t len = sizeof sock_type; > > + if (getsockopt(fd, SOL_SOCKET, SO_TYPE, &sock_type, &len)) { > > + VLOG_ERR("%s: not a socket (%s)", name, ovs_strerror(errno)); > > + return errno; > > + } > > + if (sock_type != SOCK_STREAM) { > > + VLOG_ERR("%s: not a stream socket (type %d)", name, sock_type); > > + return EINVAL; > > + } > > + int listening; > > + len = sizeof listening; > > + if (getsockopt(fd, SOL_SOCKET, SO_ACCEPTCONN, &listening, &len) > > + || !listening) { > > + VLOG_ERR("%s: not a listening socket", name); > > + return EINVAL; > > + } > > + > > + int error = set_nonblocking(fd); > > + if (error) { > > + return error; > > + } > > + > > + return new_fd_pstream(xstrdup(name), fd, punix_accept, NULL, pstreamp); > > Should we also drain down the socket when it gets opened? What I mean > is, in case we crash and systemd restarts us does the socket that the > service hold actually drain us down? I guess that isn't clear to me and > I worry about some kind of invalid data sitting in the queue. because this is a listening socket (not a connected one), there is no data sitting in a queue — only pending connections. Those pending connections are valid clients that connected while ovsdb-server was restarting, so we want to accept() them, not discard them. That is one of the main benefits of socket activation: clients queue on the listening socket instead of getting ECONNREFUSED [1]. > I also wonder if we should also do something like > recv(,MSG_DONTWAIT|MSG_PEEK) and make sure that the socket is still > open. The manpage from systemd seems to imply that if the socket gets > closed the fd itself is no longer considered in a good state. I'm not > sure how the systemd service works for it. it would fail with ENOTCONN on a listening socket since recv(2) requires a connected socket [2]. The getsockopt(SO_TYPE) + getsockopt(SO_ACCEPTCONN) checks already validate that the fd is a listening stream socket in a usable state. If systemd had closed it, getsockopt would fail with EBADF and we would log "not a socket". [...] [1] https://0pointer.de/blog/projects/socket-activation.html [2] recv(2): ENOTCONN - "The socket is associated with a connection-oriented protocol and has not been connected" _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
