sd-dhcp-server: refactoring for socket fd handling

This makes
- UDP socket fd is owned by IO event source,
- open RAW socket fd just before sending first packet,
- set TOS and socket priority,
- use AF_UNIX soxket pair in the unit test and fuzzer, so the unit test
  can now run by unprivileged user.

src/libsystemd-network/dhcp-server-internal.h

@@ -29,9 +29,10 @@ typedef struct sd_dhcp_server {
 
         sd_event *event;
         int event_priority;
-        sd_event_source *receive_message;
-        int fd;
-        int fd_raw;
+        sd_event_source *io_event_source;
+        uint8_t ip_service_type;
+        int socket_fd; /* socket fd set externally, used by unit tests */
+        int raw_socket_fd; /* send-only raw socket fd, used on sending L2 unicast message. */
 
         int ifindex;
         char *ifname;

src/libsystemd-network/dhcp-server-request.c

@@ -3,13 +3,13 @@
 #include "sd-event.h"
 
 #include "alloc-util.h"
-#include "dhcp-network.h"
 #include "dhcp-protocol.h"
 #include "dhcp-server-internal.h"
 #include "dhcp-server-lease-internal.h"
 #include "dhcp-server-request.h"
 #include "dhcp-server-send.h"
 #include "errno-util.h"
+#include "fd-util.h"
 #include "hashmap.h"
 #include "iovec-util.h"
 #include "memory-util.h"
@@ -495,38 +495,92 @@ static int server_receive_message(sd_event_source *s, int fd, uint32_t revents,
         return 0;
 }
 
+static int server_open_socket(sd_dhcp_server *server) {
+        int r;
+
+        assert(server);
+
+        _cleanup_close_ int fd = RET_NERRNO(socket(AF_INET, SOCK_DGRAM | SOCK_CLOEXEC | SOCK_NONBLOCK, 0));
+        if (fd < 0)
+                return fd;
+
+        r = socket_bind_to_ifindex(fd, server->ifindex);
+        if (r < 0)
+                return r;
+
+        r = setsockopt_int(fd, SOL_SOCKET, SO_TIMESTAMP, true);
+        if (r < 0)
+                return r;
+
+        r = setsockopt_int(fd, SOL_SOCKET, SO_REUSEADDR, true);
+        if (r < 0)
+                return r;
+
+        r = setsockopt_int(fd, SOL_SOCKET, SO_BROADCAST, true);
+        if (r < 0)
+                return r;
+
+        r = setsockopt_int(fd, SOL_SOCKET, SO_PRIORITY, tos_to_priority(server->ip_service_type));
+        if (r < 0)
+                return r;
+
+        r = setsockopt_int(fd, IPPROTO_IP, IP_TOS, server->ip_service_type);
+        if (r < 0)
+                return r;
+
+        r = setsockopt_int(fd, IPPROTO_IP, IP_PKTINFO, true);
+        if (r < 0)
+                return r;
+
+        union sockaddr_union sa = {
+                .in.sin_family = AF_INET,
+                .in.sin_port = htobe16(DHCP_PORT_SERVER),
+                .in.sin_addr.s_addr = htobe32(INADDR_ANY),
+        };
+
+        if (bind(fd, &sa.sa, sizeof(sa.in)) < 0)
+                return -errno;
+
+        return TAKE_FD(fd);
+}
+
 int dhcp_server_setup_io_event_source(sd_dhcp_server *server) {
         int r;
 
         assert(server);
         assert(server->event);
 
-        r = socket(AF_PACKET, SOCK_DGRAM | SOCK_CLOEXEC | SOCK_NONBLOCK, 0);
-        if (r < 0) {
-                r = -errno;
-                goto on_error;
+        _cleanup_close_ int fd_close = -EBADF;
+        int fd;
+        if (server->socket_fd >= 0)
+                /* When a socket fd is given externally, unconditionally use it and do not close the socket
+                 * even if we fail to set up the event source. */
+                fd = server->socket_fd;
+        else {
+                fd = fd_close = server_open_socket(server);
+                if (fd < 0)
+                        return fd;
         }
-        server->fd_raw = r;
 
-        r = dhcp_network_bind_udp_socket(server->ifindex, INADDR_ANY, DHCP_PORT_SERVER, -1);
+        _cleanup_(sd_event_source_unrefp) sd_event_source *s = NULL;
+        r = sd_event_add_io(server->event, &s, fd, EPOLLIN, server_receive_message, server);
         if (r < 0)
-                goto on_error;
-        server->fd = r;
+                return r;
 
-        r = sd_event_add_io(server->event, &server->receive_message,
-                            server->fd, EPOLLIN,
-                            server_receive_message, server);
+        r = sd_event_source_set_priority(s, server->event_priority);
         if (r < 0)
-                goto on_error;
+                return r;
 
-        r = sd_event_source_set_priority(server->receive_message,
-                                         server->event_priority);
-        if (r < 0)
-                goto on_error;
+        (void) sd_event_source_set_description(s, "dhcp-server-io");
 
+        if (fd_close >= 0) {
+                r = sd_event_source_set_io_fd_own(s, true);
+                if (r < 0)
+                        return r;
+                TAKE_FD(fd_close);
+        }
+
+        sd_event_source_disable_unref(server->io_event_source);
+        server->io_event_source = TAKE_PTR(s);
         return 0;
-
- on_error:
-        sd_dhcp_server_stop(server);
-        return r;
 }

src/libsystemd-network/dhcp-server-send.c

@@ -1,19 +1,47 @@
 /* SPDX-License-Identifier: LGPL-2.1-or-later */
 
+#include "sd-event.h"
+
 #include "alloc-util.h"
-#include "dhcp-network.h"
 #include "dhcp-option.h"
 #include "dhcp-packet.h"
 #include "dhcp-server-lease-internal.h"
 #include "dhcp-server-send.h"
 #include "dns-domain.h"
 #include "errno-util.h"
+#include "fd-util.h"
 #include "hashmap.h"
 #include "in-addr-util.h"
 #include "iovec-util.h"
 #include "iovec-wrapper.h"
 #include "socket-util.h"
 
+static int server_acquire_raw_socket(sd_dhcp_server *server) {
+        int r;
+
+        assert(server);
+
+        if (server->socket_fd >= 0)
+                /* When a socket fd is given externally, unconditionally use it and do not close the socket. */
+                return server->socket_fd;
+
+        if (server->raw_socket_fd >= 0)
+                /* Already opened. */
+                return server->raw_socket_fd;
+
+        /* This is a send-only socket, hence it is opened with protocol=0, and do not call bind().
+         * The interface binding will be done on send. */
+        _cleanup_close_ int fd = RET_NERRNO(socket(AF_PACKET, SOCK_DGRAM | SOCK_CLOEXEC | SOCK_NONBLOCK, 0));
+        if (fd < 0)
+                return fd;
+
+        r = setsockopt_int(fd, SOL_SOCKET, SO_PRIORITY, tos_to_priority(server->ip_service_type));
+        if (r < 0)
+                return r;
+
+        return server->raw_socket_fd = TAKE_FD(fd);
+}
+
 static int dhcp_server_send_unicast_raw(
                 sd_dhcp_server *server,
                 uint8_t hlen,
@@ -21,12 +49,6 @@ static int dhcp_server_send_unicast_raw(
                 DHCPPacket *packet,
                 size_t len) {
 
-        union sockaddr_union link = {
-                .ll.sll_family = AF_PACKET,
-                .ll.sll_protocol = htobe16(ETH_P_IP),
-                .ll.sll_ifindex = server->ifindex,
-                .ll.sll_halen = hlen,
-        };
         int r;
 
         assert(server);
@@ -37,11 +59,13 @@ static int dhcp_server_send_unicast_raw(
         assert(packet);
         assert(len > sizeof(DHCPPacket));
 
-        memcpy(link.ll.sll_addr, chaddr, hlen);
-
         if (len > UINT16_MAX)
                 return -EOVERFLOW;
 
+        int fd = server_acquire_raw_socket(server);
+        if (fd < 0)
+                return fd;
+
         r = dhcp_packet_append_ip_headers(
                         packet,
                         server->address,
@@ -49,63 +73,69 @@ static int dhcp_server_send_unicast_raw(
                         packet->dhcp.yiaddr,
                         DHCP_PORT_CLIENT,
                         len,
-                        /* ip_service_type= */ -1);
+                        server->ip_service_type);
         if (r < 0)
                 return r;
 
-        return dhcp_network_send_raw_socket(
-                        server->fd_raw,
-                        &link,
-                        &(struct iovec_wrapper) {
-                                .iovec = &IOVEC_MAKE(packet, len),
-                                .count = 1,
-                        });
+        union sockaddr_union sa = {
+                .ll.sll_family = AF_PACKET,
+                .ll.sll_protocol = htobe16(ETH_P_IP),
+                .ll.sll_ifindex = server->ifindex,
+                .ll.sll_halen = hlen,
+        };
+
+        memcpy(sa.ll.sll_addr, chaddr, hlen);
+
+        struct msghdr mh = {
+                .msg_name = &sa.sa,
+                .msg_namelen = sockaddr_ll_len(&sa.ll),
+                .msg_iov = &IOVEC_MAKE(packet, len),
+                .msg_iovlen = 1,
+        };
+
+        if (sendmsg(fd, &mh, MSG_NOSIGNAL) < 0)
+                return -errno;
+
+        return 0;
 }
 
 static int dhcp_server_send_udp(sd_dhcp_server *server, be32_t destination,
                                 uint16_t destination_port,
                                 DHCPMessage *message, size_t len) {
-        union sockaddr_union dest = {
+
+        assert(server);
+        assert(message);
+        assert(len >= sizeof(DHCPMessage));
+
+        int fd = sd_event_source_get_io_fd(server->io_event_source);
+        if (fd < 0)
+                return fd;
+
+        union sockaddr_union sa = {
                 .in.sin_family = AF_INET,
                 .in.sin_port = htobe16(destination_port),
                 .in.sin_addr.s_addr = destination,
         };
-        struct iovec iov = {
-                .iov_base = message,
-                .iov_len = len,
-        };
         CMSG_BUFFER_TYPE(CMSG_SPACE(sizeof(struct in_pktinfo))) control = {};
         struct msghdr msg = {
-                .msg_name = &dest,
-                .msg_namelen = sizeof(dest.in),
-                .msg_iov = &iov,
+                .msg_name = &sa,
+                .msg_namelen = sizeof(sa.in),
+                .msg_iov = &IOVEC_MAKE(message, len),
                 .msg_iovlen = 1,
+                .msg_control = &control,
+                .msg_controllen = sizeof(control),
         };
-        struct cmsghdr *cmsg;
-        struct in_pktinfo *pktinfo;
-
-        assert(server);
-        assert(server->fd >= 0);
-        assert(message);
-        assert(len >= sizeof(DHCPMessage));
-
-        msg.msg_control = &control;
-        msg.msg_controllen = sizeof(control);
-
-        cmsg = CMSG_FIRSTHDR(&msg);
-        assert(cmsg);
 
+        struct cmsghdr *cmsg = ASSERT_PTR(CMSG_FIRSTHDR(&msg));
         cmsg->cmsg_level = IPPROTO_IP;
         cmsg->cmsg_type = IP_PKTINFO;
         cmsg->cmsg_len = CMSG_LEN(sizeof(struct in_pktinfo));
 
-        pktinfo = CMSG_TYPED_DATA(cmsg, struct in_pktinfo);
-        assert(pktinfo);
-
+        struct in_pktinfo *pktinfo = ASSERT_PTR(CMSG_TYPED_DATA(cmsg, struct in_pktinfo));
         pktinfo->ipi_ifindex = server->ifindex;
         pktinfo->ipi_spec_dst.s_addr = server->address;
 
-        if (sendmsg(server->fd, &msg, 0) < 0)
+        if (sendmsg(fd, &msg, MSG_NOSIGNAL) < 0)
                 return -errno;
 
         return 0;

src/libsystemd-network/fuzz-dhcp-server.c

@@ -68,13 +68,16 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) {
         _cleanup_(rm_rf_physical_and_freep) char *tmpdir = NULL;
         _cleanup_close_ int dir_fd = ASSERT_OK(mkdtemp_open(NULL, 0, &tmpdir));
 
+        _cleanup_close_pair_ int socket_fd[2] = EBADF_PAIR;
+        ASSERT_OK_ERRNO(socketpair(AF_UNIX, SOCK_SEQPACKET | SOCK_CLOEXEC | SOCK_NONBLOCK, 0, socket_fd));
+
         _cleanup_(sd_event_unrefp) sd_event *event = NULL;
         ASSERT_OK(sd_event_new(&event));
 
         _cleanup_(sd_dhcp_server_unrefp) sd_dhcp_server *server = NULL;
         ASSERT_OK(sd_dhcp_server_new(&server, 1));
         ASSERT_OK(sd_dhcp_server_attach_event(server, event, SD_EVENT_PRIORITY_NORMAL));
-        server->fd = ASSERT_OK_ERRNO(open("/dev/null", O_RDWR|O_CLOEXEC|O_NOCTTY));
+        server->socket_fd = TAKE_FD(socket_fd[0]);
         ASSERT_OK(sd_dhcp_server_set_lease_file(server, dir_fd, "leases"));
         ASSERT_OK(sd_dhcp_server_configure_pool(server, &address, 24, 0, 0));
 
@@ -87,6 +90,7 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) {
         ASSERT_OK(add_static_lease(server, 4));
 
         _cleanup_free_ uint8_t *duped = ASSERT_NOT_NULL(memdup(data, size));
+        ASSERT_OK(sd_dhcp_server_start(server));
         (void) dhcp_server_handle_message(server, (DHCPMessage*) duped, size, NULL);
 
         ASSERT_OK(dhcp_server_save_leases(server));

src/libsystemd-network/sd-dhcp-server.c

@@ -3,6 +3,8 @@
   Copyright © 2013 Intel Corporation. All rights reserved.
 ***/
 
+#include <netinet/ip.h>
+
 #include "sd-dhcp-server.h"
 #include "sd-event.h"
 
@@ -99,10 +101,7 @@ int sd_dhcp_server_configure_pool(
 }
 
 int sd_dhcp_server_is_running(sd_dhcp_server *server) {
-        if (!server)
-                return false;
-
-        return !!server->receive_message;
+        return server && sd_event_source_get_enabled(server->io_event_source, /* ret= */ NULL) > 0;
 }
 
 static sd_dhcp_server* dhcp_server_free(sd_dhcp_server *server) {
@@ -111,6 +110,8 @@ static sd_dhcp_server* dhcp_server_free(sd_dhcp_server *server) {
         sd_dhcp_server_stop(server);
 
         sd_event_unref(server->event);
+        safe_close(server->socket_fd);
+        safe_close(server->raw_socket_fd);
 
         free(server->boot_server_name);
         free(server->boot_filename);
@@ -149,8 +150,9 @@ int sd_dhcp_server_new(sd_dhcp_server **ret, int ifindex) {
 
         *server = (sd_dhcp_server) {
                 .n_ref = 1,
-                .fd_raw = -EBADF,
-                .fd = -EBADF,
+                .ip_service_type = IPTOS_CLASS_CS6,
+                .socket_fd = -EBADF,
+                .raw_socket_fd = -EBADF,
                 .address = htobe32(INADDR_ANY),
                 .netmask = htobe32(INADDR_ANY),
                 .ifindex = ifindex,
@@ -262,17 +264,13 @@ int sd_dhcp_server_set_boot_filename(sd_dhcp_server *server, const char *filenam
 }
 
 int sd_dhcp_server_stop(sd_dhcp_server *server) {
-        bool running;
-
         if (!server)
                 return 0;
 
-        running = sd_dhcp_server_is_running(server);
+        bool running = sd_dhcp_server_is_running(server);
 
-        server->receive_message = sd_event_source_disable_unref(server->receive_message);
-
-        server->fd_raw = safe_close(server->fd_raw);
-        server->fd = safe_close(server->fd);
+        server->raw_socket_fd = safe_close(server->raw_socket_fd);
+        server->io_event_source = sd_event_source_disable_unref(server->io_event_source);
 
         if (running)
                 log_dhcp_server(server, "STOPPED");
@@ -327,15 +325,11 @@ int sd_dhcp_server_start(sd_dhcp_server *server) {
 
         assert_return(server, -EINVAL);
         assert_return(server->event, -EINVAL);
+        assert_return(server->address != INADDR_ANY, -EUNATCH);
 
         if (sd_dhcp_server_is_running(server))
                 return 0;
 
-        assert_return(!server->receive_message, -EBUSY);
-        assert_return(server->fd_raw < 0, -EBUSY);
-        assert_return(server->fd < 0, -EBUSY);
-        assert_return(server->address != htobe32(INADDR_ANY), -EUNATCH);
-
         dhcp_server_update_lease_servers(server);
 
         r = dhcp_server_setup_io_event_source(server);

src/libsystemd-network/test-dhcp-server.c

@@ -10,6 +10,7 @@
 
 #include "dhcp-server-internal.h"
 #include "dhcp-server-request.h"
+#include "fd-util.h"
 #include "tests.h"
 
 TEST(basic) {
@@ -19,14 +20,12 @@ TEST(basic) {
         struct in_addr address_any = {
                 .s_addr = htobe32(INADDR_ANY),
         };
-        int r;
 
         _cleanup_(sd_event_unrefp) sd_event *event = NULL;
         ASSERT_OK(sd_event_new(&event));
 
-        /* attach to loopback interface */
         _cleanup_(sd_dhcp_server_unrefp) sd_dhcp_server *server = NULL;
-        ASSERT_OK(sd_dhcp_server_new(&server, 1));
+        ASSERT_OK(sd_dhcp_server_new(&server, 4242));
         ASSERT_NOT_NULL(server);
 
         ASSERT_OK(sd_dhcp_server_attach_event(server, event, SD_EVENT_PRIORITY_NORMAL));
@@ -49,13 +48,12 @@ TEST(basic) {
         ASSERT_RETURN_EXPECTED(ASSERT_ERROR(sd_dhcp_server_configure_pool(server, &address_any, 8, 0, 1), EINVAL));
         ASSERT_OK(sd_dhcp_server_configure_pool(server, &address_lo, 8, 0, 1));
 
-        r = sd_dhcp_server_start(server);
-        /* skip test if running in an environment with no full networking support, CONFIG_PACKET not
-         * compiled in kernel, nor af_packet module available. */
-        if (IN_SET(r, -EPERM, -EAFNOSUPPORT))
-                return (void) log_tests_skipped_errno(r, "cannot start dhcp server");
-        ASSERT_OK(r);
+        _cleanup_close_pair_ int socket_fd[2] = EBADF_PAIR;
+        ASSERT_OK_ERRNO(socketpair(AF_UNIX, SOCK_SEQPACKET | SOCK_CLOEXEC | SOCK_NONBLOCK, 0, socket_fd));
 
+        server->socket_fd = TAKE_FD(socket_fd[0]);
+
+        ASSERT_OK(sd_dhcp_server_start(server));
         ASSERT_OK(sd_dhcp_server_start(server));
         ASSERT_OK(sd_dhcp_server_stop(server));
         ASSERT_OK(sd_dhcp_server_stop(server));
@@ -112,13 +110,15 @@ TEST(dhcp_server_handle_message) {
                 .s_addr = htobe32(INADDR_LOOPBACK + 42),
         };
         static uint8_t static_lease_client_id[7] = {0x01, 'A', 'B', 'C', 'D', 'E', 'G' };
-        int r;
+
+        _cleanup_close_pair_ int socket_fd[2] = EBADF_PAIR;
+        ASSERT_OK_ERRNO(socketpair(AF_UNIX, SOCK_SEQPACKET | SOCK_CLOEXEC | SOCK_NONBLOCK, 0, socket_fd));
 
         _cleanup_(sd_event_unrefp) sd_event *event = NULL;
         ASSERT_OK(sd_event_new(&event));
 
         _cleanup_(sd_dhcp_server_unrefp) sd_dhcp_server *server = NULL;
-        ASSERT_OK(sd_dhcp_server_new(&server, 1));
+        ASSERT_OK(sd_dhcp_server_new(&server, 4242));
         ASSERT_OK(sd_dhcp_server_configure_pool(server, &address_lo, 8, 0, 0));
         ASSERT_OK(sd_dhcp_server_set_static_lease(
                         server,
@@ -127,15 +127,10 @@ TEST(dhcp_server_handle_message) {
                         ELEMENTSOF(static_lease_client_id),
                         /* hostname= */ NULL));
         ASSERT_OK(sd_dhcp_server_attach_event(server, event, SD_EVENT_PRIORITY_NORMAL));
-        r = sd_dhcp_server_start(server);
-        if (IN_SET(r, -EPERM, -EAFNOSUPPORT))
-                return (void) log_tests_skipped_errno(r, "cannot start dhcp server");
-        ASSERT_OK(r);
+        server->socket_fd = TAKE_FD(socket_fd[0]);
+        ASSERT_OK(sd_dhcp_server_start(server));
 
-        r = dhcp_server_handle_message(server, (DHCPMessage*)&test, sizeof(test), NULL);
-        if (r == -ENETDOWN)
-                return (void) log_tests_skipped("Network is not available");
-        ASSERT_OK_EQ(r, DHCP_OFFER);
+        ASSERT_OK_EQ(dhcp_server_handle_message(server, (DHCPMessage*)&test, sizeof(test), NULL), DHCP_OFFER);
 
         test.end = 0;
         /* TODO, shouldn't this fail? */
@@ -273,7 +268,7 @@ TEST(dhcp_server_handle_message) {
 
 TEST(sd_dhcp_server_set_static_lease) {
         _cleanup_(sd_dhcp_server_unrefp) sd_dhcp_server *server = NULL;
-        ASSERT_OK(sd_dhcp_server_new(&server, 1));
+        ASSERT_OK(sd_dhcp_server_new(&server, 4242));
 
         ASSERT_OK(sd_dhcp_server_set_static_lease(
                         server,
@@ -351,7 +346,7 @@ TEST(sd_dhcp_server_set_static_lease) {
 
 TEST(sd_dhcp_server_set_domain_name) {
         _cleanup_(sd_dhcp_server_unrefp) sd_dhcp_server *server = NULL;
-        ASSERT_OK(sd_dhcp_server_new(&server, 1));
+        ASSERT_OK(sd_dhcp_server_new(&server, 4242));
 
         /* Test setting domain name */
         ASSERT_OK_POSITIVE(sd_dhcp_server_set_domain_name(server, "example.com"));