From 3fabebf45e268a0e1e8f9e7688b2428f8ee8a802 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 24 Nov 2021 23:50:07 +0100 Subject: [PATCH 1/3] socket: always pass socket, fd and SocketPeer ownership to service together Per-connection socket instances we currently maintain three fields related to the socket: a reference to the Socket unit, the connection fd, and a reference to the SocketPeer object that counts socket peers. Let's synchronize their lifetime, i.e. always set them all three together or unset them together, so that their reference counters stay synchronous. THis will in particuar ensure that we'll drop the SocketPeer reference whenever we leave an active state of the service unit, i.e. at the same time we close the fd for it. Fixes: #20685 --- src/core/service.c | 27 ++++++++++++++++++--------- src/core/service.h | 9 +++++---- src/core/socket.c | 4 +--- 3 files changed, 24 insertions(+), 16 deletions(-) diff --git a/src/core/service.c b/src/core/service.c index 17c19a2c4a..49579f7998 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -190,6 +190,8 @@ void service_close_socket_fd(Service *s) { socket_connection_unref(SOCKET(UNIT_DEREF(s->accept_socket))); unit_ref_unset(&s->accept_socket); } + + s->socket_peer = socket_peer_unref(s->socket_peer); } static void service_stop_watchdog(Service *s) { @@ -388,7 +390,6 @@ static void service_done(Unit *u) { s->usb_function_strings = mfree(s->usb_function_strings); service_close_socket_fd(s); - s->peer = socket_peer_unref(s->peer); unit_ref_unset(&s->accept_socket); @@ -1237,8 +1238,8 @@ static int service_coldplug(Unit *u) { /* Make a best-effort attempt at bumping the connection count */ if (socket_acquire_peer(socket, s->socket_fd, &peer) > 0) { - socket_peer_unref(s->peer); - s->peer = peer; + socket_peer_unref(s->socket_peer); + s->socket_peer = peer; } } } @@ -4285,8 +4286,14 @@ static void service_bus_name_owner_change(Unit *u, const char *new_owner) { } } -int service_set_socket_fd(Service *s, int fd, Socket *sock, bool selinux_context_net) { - _cleanup_free_ char *peer = NULL; +int service_set_socket_fd( + Service *s, + int fd, + Socket *sock, + SocketPeer *peer, + bool selinux_context_net) { + + _cleanup_free_ char *peer_text = NULL; int r; assert(s); @@ -4301,22 +4308,23 @@ int service_set_socket_fd(Service *s, int fd, Socket *sock, bool selinux_context if (s->socket_fd >= 0) return -EBUSY; + assert(!s->socket_peer); + if (s->state != SERVICE_DEAD) return -EAGAIN; - if (getpeername_pretty(fd, true, &peer) >= 0) { + if (getpeername_pretty(fd, true, &peer_text) >= 0) { if (UNIT(s)->description) { _cleanup_free_ char *a = NULL; - a = strjoin(UNIT(s)->description, " (", peer, ")"); + a = strjoin(UNIT(s)->description, " (", peer_text, ")"); if (!a) return -ENOMEM; r = unit_set_description(UNIT(s), a); } else - r = unit_set_description(UNIT(s), peer); - + r = unit_set_description(UNIT(s), peer_text); if (r < 0) return r; } @@ -4326,6 +4334,7 @@ int service_set_socket_fd(Service *s, int fd, Socket *sock, bool selinux_context return r; s->socket_fd = fd; + s->socket_peer = socket_peer_ref(peer); s->socket_fd_selinux_context_net = selinux_context_net; unit_ref_set(&s->accept_socket, UNIT(s), UNIT(sock)); diff --git a/src/core/service.h b/src/core/service.h index 70ce70fba5..778551d844 100644 --- a/src/core/service.h +++ b/src/core/service.h @@ -157,8 +157,11 @@ struct Service { DynamicCreds dynamic_creds; pid_t main_pid, control_pid; + + /* if we are a socket activated service instance, store information of the connection/peer/socket */ int socket_fd; - SocketPeer *peer; + SocketPeer *socket_peer; + UnitRef accept_socket; bool socket_fd_selinux_context_net; bool permissions_start_only; @@ -186,8 +189,6 @@ struct Service { char *status_text; int status_errno; - UnitRef accept_socket; - sd_event_source *timer_event_source; PathSpec *pid_file_pathspec; @@ -226,7 +227,7 @@ static inline usec_t service_get_watchdog_usec(Service *s) { extern const UnitVTable service_vtable; -int service_set_socket_fd(Service *s, int fd, struct Socket *socket, bool selinux_context_net); +int service_set_socket_fd(Service *s, int fd, struct Socket *socket, struct SocketPeer *peer, bool selinux_context_net); void service_close_socket_fd(Service *s); const char* service_restart_to_string(ServiceRestart i) _const_; diff --git a/src/core/socket.c b/src/core/socket.c index e6d168188a..d9db1edd3c 100644 --- a/src/core/socket.c +++ b/src/core/socket.c @@ -2409,7 +2409,7 @@ static void socket_enter_running(Socket *s, int cfd_in) { s->n_accepted++; - r = service_set_socket_fd(SERVICE(service), cfd, s, s->selinux_context_from_net); + r = service_set_socket_fd(SERVICE(service), cfd, s, p, s->selinux_context_from_net); if (ERRNO_IS_DISCONNECT(r)) return; if (r < 0) @@ -2418,8 +2418,6 @@ static void socket_enter_running(Socket *s, int cfd_in) { TAKE_FD(cfd); /* We passed ownership of the fd to the service now. Forget it here. */ s->n_connections++; - SERVICE(service)->peer = TAKE_PTR(p); /* Pass ownership of the peer reference */ - r = manager_add_job(UNIT(s)->manager, JOB_START, service, JOB_REPLACE, NULL, &error, NULL); if (r < 0) { /* We failed to activate the new service, but it still exists. Let's make sure the From 000b61b980abba1a69d7a5e2a2a073930eca5e08 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 24 Nov 2021 23:53:10 +0100 Subject: [PATCH 2/3] socket: various modernizations --- src/core/socket.c | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/src/core/socket.c b/src/core/socket.c index d9db1edd3c..6b5ec9d987 100644 --- a/src/core/socket.c +++ b/src/core/socket.c @@ -463,10 +463,6 @@ static int socket_load(Unit *u) { assert(u); assert(u->load_state == UNIT_STUB); - r = set_ensure_allocated(&s->peers_by_address, &peer_address_hash_ops); - if (r < 0) - return r; - r = unit_load_fragment_and_dropin(u, true); if (r < 0) return r; @@ -485,12 +481,13 @@ static int socket_load(Unit *u) { static SocketPeer *socket_peer_new(void) { SocketPeer *p; - p = new0(SocketPeer, 1); + p = new(SocketPeer, 1); if (!p) return NULL; - p->n_ref = 1; - + *p = (SocketPeer) { + .n_ref = 1, + }; return p; } @@ -507,14 +504,15 @@ DEFINE_TRIVIAL_REF_UNREF_FUNC(SocketPeer, socket_peer, socket_peer_free); int socket_acquire_peer(Socket *s, int fd, SocketPeer **p) { _cleanup_(socket_peer_unrefp) SocketPeer *remote = NULL; - SocketPeer sa = {}, *i; - socklen_t salen = sizeof(sa.peer); + SocketPeer sa = { + .peer_salen = sizeof(union sockaddr_union), + }, *i; int r; assert(fd >= 0); assert(s); - if (getpeername(fd, &sa.peer.sa, &salen) < 0) + if (getpeername(fd, &sa.peer.sa, &sa.peer_salen) < 0) return log_unit_error_errno(UNIT(s), errno, "getpeername failed: %m"); if (!IN_SET(sa.peer.sa.sa_family, AF_INET, AF_INET6, AF_VSOCK)) { @@ -522,6 +520,10 @@ int socket_acquire_peer(Socket *s, int fd, SocketPeer **p) { return 0; } + r = set_ensure_allocated(&s->peers_by_address, &peer_address_hash_ops); + if (r < 0) + return r; + i = set_get(s->peers_by_address, &sa); if (i) { *p = socket_peer_ref(i); @@ -533,7 +535,7 @@ int socket_acquire_peer(Socket *s, int fd, SocketPeer **p) { return log_oom(); remote->peer = sa.peer; - remote->peer_salen = salen; + remote->peer_salen = sa.peer_salen; r = set_put(s->peers_by_address, remote); if (r < 0) @@ -542,7 +544,6 @@ int socket_acquire_peer(Socket *s, int fd, SocketPeer **p) { remote->socket = s; *p = TAKE_PTR(remote); - return 1; } From c91d2b43521affe6c045a59ba8bc840f29a82563 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 25 Nov 2021 00:11:31 +0100 Subject: [PATCH 3/3] man: suggest usage of CollectMode= in Accept=yes services --- man/systemd.socket.xml | 48 +++++++++++++++++++----------------------- 1 file changed, 22 insertions(+), 26 deletions(-) diff --git a/man/systemd.socket.xml b/man/systemd.socket.xml index f977f11541..e4f1f544cb 100644 --- a/man/systemd.socket.xml +++ b/man/systemd.socket.xml @@ -392,36 +392,32 @@ Accept= - Takes a boolean argument. If yes, a service - instance is spawned for each incoming connection and only the - connection socket is passed to it. If no, all listening - sockets themselves are passed to the started service unit, and - only one service unit is spawned for all connections (also see - above). This value is ignored for datagram sockets and FIFOs - where a single service unit unconditionally handles all - incoming traffic. Defaults to . For - performance reasons, it is recommended to write new daemons - only in a way that is suitable for - . A daemon listening on an - AF_UNIX socket may, but does not need to, - call - close2 - on the received socket before exiting. However, it must not - unlink the socket from a file system. It should not invoke - shutdown2 - on sockets it got with Accept=no, but it - may do so for sockets it got with - Accept=yes set. Setting - Accept=yes is mostly useful to allow - daemons designed for usage with - inetd8 - to work unmodified with systemd socket - activation. + Takes a boolean argument. If yes, a service instance is spawned for each incoming + connection and only the connection socket is passed to it. If no, all listening sockets themselves + are passed to the started service unit, and only one service unit is spawned for all connections + (also see above). This value is ignored for datagram sockets and FIFOs where a single service unit + unconditionally handles all incoming traffic. Defaults to . For performance + reasons, it is recommended to write new daemons only in a way that is suitable for + . A daemon listening on an AF_UNIX socket may, but + does not need to, call + close2 on the + received socket before exiting. However, it must not unlink the socket from a file system. It should + not invoke + shutdown2 on + sockets it got with Accept=no, but it may do so for sockets it got with + Accept=yes set. Setting Accept=yes is mostly useful to allow + daemons designed for usage with inetd8 to work + unmodified with systemd socket activation. For IPv4 and IPv6 connections, the REMOTE_ADDR environment variable will contain the remote IP address, and REMOTE_PORT will contain the remote port. This is the same as the format used by CGI. For SOCK_RAW, the port is the IP - protocol. + protocol. + + It is recommended to set CollectMode=inactive-or-failed for service + instances activated via Accept=yes, to ensure that failed connection services are + cleaned up and released from memory, and do not accumulate.