Unlike TCP, UDP has no in-band signalling for the end of a flow. So the only way we remove flows is on a timer if they have no activity for 180s. However, we've started to investigate some error conditions in which we want to prematurely abort / abandon a UDP flow. We can call udp_flow_close(), which will make the flow inert (sockets closed, no epoll events, can't be looked up in hash). However it will still wait 3 minutes to clear away the stale entry. Clean this up by adding an explicit 'closed' flag which will cause a flow to be more promptly cleaned up. We also publish udp_flow_close() so it can be called from other places to abort UDP flows(). Signed-off-by: David Gibson --- flow.c | 3 ++- udp_flow.c | 18 +++++++++++++++++- udp_flow.h | 4 ++++ 3 files changed, 23 insertions(+), 2 deletions(-) diff --git a/flow.c b/flow.c index a00e01d1..f2de0414 100644 --- a/flow.c +++ b/flow.c @@ -832,7 +832,8 @@ void flow_defer_handler(const struct ctx *c, const struct timespec *now) closed = icmp_ping_timer(c, &flow->ping, now); break; case FLOW_UDP: - if (timer) + closed = udp_flow_defer(&flow->udp); + if (!closed && timer) closed = udp_flow_timer(c, &flow->udp, now); break; default: diff --git a/udp_flow.c b/udp_flow.c index 1ff59a9b..b81be2cf 100644 --- a/udp_flow.c +++ b/udp_flow.c @@ -39,8 +39,11 @@ struct udp_flow *udp_at_sidx(flow_sidx_t sidx) * @c: Execution context * @uflow: UDP flow */ -static void udp_flow_close(const struct ctx *c, struct udp_flow *uflow) +void udp_flow_close(const struct ctx *c, struct udp_flow *uflow) { + if (uflow->closed) + return; /* Nothing to do */ + if (uflow->s[INISIDE] >= 0) { /* The listening socket needs to stay in epoll */ close(uflow->s[INISIDE]); @@ -56,6 +59,8 @@ static void udp_flow_close(const struct ctx *c, struct udp_flow *uflow) flow_hash_remove(c, FLOW_SIDX(uflow, INISIDE)); if (!pif_is_socket(uflow->f.pif[TGTSIDE])) flow_hash_remove(c, FLOW_SIDX(uflow, TGTSIDE)); + + uflow->closed = true; } /** @@ -256,6 +261,17 @@ flow_sidx_t udp_flow_from_tap(const struct ctx *c, return udp_flow_new(c, flow, -1, now); } +/** + * udp_flow_defer() - Deferred per-flow handling (clean up aborted flows) + * @uflow: Flow to handle + * + * Return: true if the connection is ready to free, false otherwise + */ +bool udp_flow_defer(const struct udp_flow *uflow) +{ + return uflow->closed; +} + /** * udp_flow_timer() - Handler for timed events related to a given flow * @c: Execution context diff --git a/udp_flow.h b/udp_flow.h index 12ddf039..9a1b059c 100644 --- a/udp_flow.h +++ b/udp_flow.h @@ -10,6 +10,7 @@ /** * struct udp - Descriptor for a flow of UDP packets * @f: Generic flow information + * @closed: Flow is already closed * @ts: Activity timestamp * @s: Socket fd (or -1) for each side of the flow */ @@ -17,6 +18,7 @@ struct udp_flow { /* Must be first element */ struct flow_common f; + bool closed :1; time_t ts; int s[SIDES]; }; @@ -30,6 +32,8 @@ flow_sidx_t udp_flow_from_tap(const struct ctx *c, const void *saddr, const void *daddr, in_port_t srcport, in_port_t dstport, const struct timespec *now); +void udp_flow_close(const struct ctx *c, struct udp_flow *uflow); +bool udp_flow_defer(const struct udp_flow *uflow); bool udp_flow_timer(const struct ctx *c, struct udp_flow *uflow, const struct timespec *now); -- 2.46.0

Currently udp_sock_recv() both attempts to clear socket errors and read a batch of datagrams for forwarding. That made sense initially, since both listening and reply sockets need to do this. However, we have certain error cases which will add additional complexity to the error processing. Furthermore, if we ever wanted to more thoroughly handle errors received here - e.g. by synthesising ICMP messages on the tap device - it will likely require different handling for the listening and reply socket cases. So, split handling of error events into its own udp_sock_errs() function. While we're there, allow it to report "unrecoverable errors". We don't have any of these so far, but some cases we're working on might require it. Signed-off-by: David Gibson --- udp.c | 46 ++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 40 insertions(+), 6 deletions(-) diff --git a/udp.c b/udp.c index aae6d142..fd91b539 100644 --- a/udp.c +++ b/udp.c @@ -436,6 +436,30 @@ static bool udp_sock_recverr(int s) return true; } +/** + * udp_sock_errs() - Process errors on a socket + * @c: Execution context + * @s: Socket to receive from + * @events: epoll events bitmap + * + * Return: Number of errors handled, or < 0 if we have an unrecoverable error + */ +static int udp_sock_errs(const struct ctx *c, int s, uint32_t events) +{ + unsigned n_err = 0; + + ASSERT(!c->no_udp); + + if (!(events & EPOLLERR)) + return 0; /* Nothing to do */ + + /* Empty the error queue */ + while (udp_sock_recverr(s)) + n_err++; + + return n_err; +} + /** * udp_sock_recv() - Receive datagrams from a socket * @c: Execution context @@ -443,6 +467,8 @@ static bool udp_sock_recverr(int s) * @events: epoll events bitmap * @mmh mmsghdr array to receive into * + * Return: Number of datagrams received + * * #syscalls recvmmsg arm:recvmmsg_time64 i686:recvmmsg_time64 */ static int udp_sock_recv(const struct ctx *c, int s, uint32_t events, @@ -459,12 +485,6 @@ static int udp_sock_recv(const struct ctx *c, int s, uint32_t events, ASSERT(!c->no_udp); - /* Clear any errors first */ - if (events & EPOLLERR) { - while (udp_sock_recverr(s)) - ; - } - if (!(events & EPOLLIN)) return 0; @@ -492,6 +512,13 @@ void udp_listen_sock_handler(const struct ctx *c, union epoll_ref ref, const socklen_t sasize = sizeof(udp_meta[0].s_in); int n, i; + if (udp_sock_errs(c, ref.fd, events) < 0) { + err("UDP: Unrecoverable error on listening socket:" + " (%s port %hu)", pif_name(ref.udp.pif), ref.udp.port); + /* FIXME: what now? close/re-open socket? */ + return; + } + if ((n = udp_sock_recv(c, ref.fd, events, udp_mh_recv)) <= 0) return; @@ -566,6 +593,13 @@ void udp_reply_sock_handler(const struct ctx *c, union epoll_ref ref, ASSERT(!c->no_udp && uflow); + if (udp_sock_errs(c, from_s, events) < 0) { + flow_err(uflow, "Unrecoverable error on reply socket"); + flow_err_details(uflow); + udp_flow_close(c, uflow); + return; + } + if ((n = udp_sock_recv(c, from_s, events, udp_mh_recv)) <= 0) return; -- 2.46.0

udp_sock_errs() reads out everything in the socket error queue. However we've seen some cases[0] where an EPOLLERR event is active, but there isn't anything in the queue. One possibility is that the error is reported instead by the SO_ERROR sockopt. Check for that case and report it as best we can. If we still get an EPOLLERR without visible error, we have no way to clear the error state, so treat it as an unrecoverable error. [0] https://github.com/containers/podman/issues/23686#issuecomment-2324945010 Link: https://bugs.passt.top/show_bug.cgi?id=95 Signed-off-by: David Gibson --- udp.c | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/udp.c b/udp.c index fe5df995..2ba00c9c 100644 --- a/udp.c +++ b/udp.c @@ -450,7 +450,8 @@ static int udp_sock_recverr(int s) static int udp_sock_errs(const struct ctx *c, int s, uint32_t events) { unsigned n_err = 0; - int rc; + socklen_t errlen; + int rc, err; ASSERT(!c->no_udp); @@ -464,6 +465,24 @@ static int udp_sock_errs(const struct ctx *c, int s, uint32_t events) if (rc < 0) return -1; /* error reading error, unrecoverable */ + errlen = sizeof(err); + if (getsockopt(s, SOL_SOCKET, SO_ERROR, &err, &errlen) < 0 || + errlen != sizeof(err)) { + err_perror("Error reading SO_ERROR"); + return -1; /* error reading error, unrecoverable */ + } + + if (err) { + debug("Unqueued error on UDP socket %i: %s", s, strerror(err)); + n_err++; + } + + if (!n_err) { + /* EPOLLERR, but no errors to clear !? */ + err("EPOLLERR event without reported errors on socket %i", s); + return -1; /* no way to clear, unrecoverable */ + } + return n_err; } -- 2.46.0