May 2025 - dev

[PATCH] udp: Actually discard datagrams we can't forward
by Stefano Brivio 03 May '25

03 May '25

Given that udp_sock_fwd() now loops on udp_peek_addr() to get endpoint addresses for datagrams, if we can't forward one of these datagrams, we need to make sure we actually discard it. Otherwise, with MSG_PEEK, we won't dequeue and loop on it forever. For example, if we fail to create a socket for a new flow, because, say, the destination of an inbound packet is multicast, and we can't bind() to a multicast address, the loop will look like this: 18.0563: Flow 0 (NEW): FREE -> NEW 18.0563: Flow 0 (INI): NEW -> INI 18.0563: Flow 0 (INI): HOST [127.0.0.1]:42487 -> [127.0.0.1]:9997 => ? 18.0563: Flow 0 (TGT): INI -> TGT 18.0563: Flow 0 (TGT): HOST [127.0.0.1]:42487 -> [ff02::c]:9997 => SPLICE [0.0.0.0]:42487 -> [88.198.0.164]:9997 18.0563: Flow 0 (UDP flow): TGT -> TYPED 18.0564: Flow 0 (UDP flow): HOST [127.0.0.1]:42487 -> [ff02::c]:9997 => SPLICE [0.0.0.0]:42487 -> [88.198.0.164]:9997 18.0564: Flow 0 (UDP flow): Couldn't open flow specific socket: Invalid argument 18.0564: Flow 0 (FREE): TYPED -> FREE 18.0564: Flow 0 (FREE): HOST [127.0.0.1]:42487 -> [ff02::c]:9997 => SPLICE [0.0.0.0]:42487 -> [88.198.0.164]:9997 18.0564: Discarding datagram without flow 18.0564: Flow 0 (NEW): FREE -> NEW 18.0564: Flow 0 (INI): NEW -> INI 18.0564: Flow 0 (INI): HOST [127.0.0.1]:42487 -> [127.0.0.1]:9997 => ? 18.0564: Flow 0 (TGT): INI -> TGT 18.0564: Flow 0 (TGT): HOST [127.0.0.1]:42487 -> [ff02::c]:9997 => SPLICE [0.0.0.0]:42487 -> [88.198.0.164]:9997 18.0564: Flow 0 (UDP flow): TGT -> TYPED 18.0564: Flow 0 (UDP flow): HOST [127.0.0.1]:42487 -> [ff02::c]:9997 => SPLICE [0.0.0.0]:42487 -> [88.198.0.164]:9997 18.0564: Flow 0 (UDP flow): Couldn't open flow specific socket: Invalid argument 18.0564: Flow 0 (FREE): TYPED -> FREE 18.0564: Flow 0 (FREE): HOST [127.0.0.1]:42487 -> [ff02::c]:9997 => SPLICE [0.0.0.0]:42487 -> [88.198.0.164]:9997 18.0564: Discarding datagram without flow and seen from strace: epoll_wait(3, [{events=EPOLLIN, data=0x1076c00000705}], 8, 1000) = 1 recvmsg(7, {msg_name={sa_family=AF_INET6, sin6_port=htons(55899), sin6_flowinfo=htonl(0), inet_pton(AF_INET6, "fe80::26e8:53ff:fef3:13b6", &sin6_addr), sin6_scope_id=if_nametoindex("wlp4s0")}, msg_namelen=28, msg_iov=NULL, msg_iovlen=0, msg_control=[{cmsg_len=36, cmsg_level=SOL_IPV6, cmsg_type=0x32, cmsg_data="\xff\x02\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x0c\x03\x00\x00\x00"}], msg_controllen=40, msg_flags=MSG_TRUNC}, MSG_PEEK|MSG_DONTWAIT) = 0 socket(AF_INET6, SOCK_DGRAM|SOCK_NONBLOCK, IPPROTO_UDP) = 12 setsockopt(12, SOL_IPV6, IPV6_V6ONLY, [1], 4) = 0 setsockopt(12, SOL_SOCKET, SO_REUSEADDR, [1], 4) = 0 setsockopt(12, SOL_IPV6, IPV6_RECVERR, [1], 4) = 0 setsockopt(12, SOL_IPV6, IPV6_RECVPKTINFO, [1], 4) = 0 bind(12, {sa_family=AF_INET6, sin6_port=htons(1900), sin6_flowinfo=htonl(0), inet_pton(AF_INET6, "ff02::c", &sin6_addr), sin6_scope_id=0}, 28) = -1 EINVAL (Invalid argument) close(12) = 0 recvmsg(7, {msg_name={sa_family=AF_INET6, sin6_port=htons(55899), sin6_flowinfo=htonl(0), inet_pton(AF_INET6, "fe80::26e8:53ff:fef3:13b6", &sin6_addr), sin6_scope_id=if_nametoindex("wlp4s0")}, msg_namelen=28, msg_iov=NULL, msg_iovlen=0, msg_control=[{cmsg_len=36, cmsg_level=SOL_IPV6, cmsg_type=0x32, cmsg_data="\xff\x02\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x0c\x03\x00\x00\x00"}], msg_controllen=40, msg_flags=MSG_TRUNC}, MSG_PEEK|MSG_DONTWAIT) = 0 socket(AF_INET6, SOCK_DGRAM|SOCK_NONBLOCK, IPPROTO_UDP) = 12 setsockopt(12, SOL_IPV6, IPV6_V6ONLY, [1], 4) = 0 setsockopt(12, SOL_SOCKET, SO_REUSEADDR, [1], 4) = 0 setsockopt(12, SOL_IPV6, IPV6_RECVERR, [1], 4) = 0 setsockopt(12, SOL_IPV6, IPV6_RECVPKTINFO, [1], 4) = 0 bind(12, {sa_family=AF_INET6, sin6_port=htons(1900), sin6_flowinfo=htonl(0), inet_pton(AF_INET6, "ff02::c", &sin6_addr), sin6_scope_id=0}, 28) = -1 EINVAL (Invalid argument) close(12) = 0 Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- udp.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/udp.c b/udp.c index f5a5cd1..ca28b37 100644 --- a/udp.c +++ b/udp.c @@ -828,6 +828,7 @@ void udp_sock_fwd(const struct ctx *c, int s, uint8_t frompif, int rc; while ((rc = udp_peek_addr(s, &src, &dst)) != 0) { + bool discard = false; flow_sidx_t tosidx; uint8_t topif; @@ -861,8 +862,17 @@ void udp_sock_fwd(const struct ctx *c, int s, uint8_t frompif, flow_err(uflow, "No support for forwarding UDP from %s to %s", pif_name(frompif), pif_name(topif)); + discard = true; } else { debug("Discarding datagram without flow"); + discard = true; + } + + if (discard) { + struct msghdr msg = { 0 }; + + if (recvmsg(s, &msg, MSG_DONTWAIT) < 0) + debug_perror("Failed to discard datagram"); } } } -- 2.43.0

2 1

[PATCH] flow: fix podman issue #25959
by Laurent Vivier 02 May '25

02 May '25

While running piHole using podman, traffic can trigger the following assert: ASSSERTION FAILED in flow_alloc (flow.c:521): flow->f.state == FLOW_STATE_FREE Backtrace shows that this happens in flow_defer_handler(): #4 0x00005610d6f5b481 flow_alloc (passt + 0xb481) #5 0x00005610d6f74f86 udp_flow_from_sock (passt + 0x24f86) #6 0x00005610d6f737c3 udp_sock_fwd (passt + 0x237c3) #7 0x00005610d6f74c07 udp_flush_flow (passt + 0x24c07) #8 0x00005610d6f752c2 udp_flow_defer (passt + 0x252c2) #9 0x00005610d6f5bce1 flow_defer_handler (passt + 0xbce1) We are trying to allocate a new flow inside the loop freeing them. Inside the loop free_head points to the first free flow entry in the current cluster. But if we allocate a new entry during the loop, free_head is not updated and can point now to the entry we have just allocated. We can fix the problem by spliting the loop in two parts: - first part where we can close some of them and allocate some new flow entries, - second part where we free the entries closed in the previous loop and we aggregate the free entries to merge consecutive the clusters. Link: https://github.com/containers/podman/issues/25959 Signed-off-by: Laurent Vivier <lvivier(a)redhat.com> --- flow.c | 107 ++++++++++++++++++++++++++++++--------------------------- 1 file changed, 57 insertions(+), 50 deletions(-) diff --git a/flow.c b/flow.c index 3c81cb42f921..00c1b2cc316f 100644 --- a/flow.c +++ b/flow.c @@ -788,6 +788,7 @@ void flow_defer_handler(const struct ctx *c, const struct timespec *now) { struct flow_free_cluster *free_head = NULL; unsigned *last_next = &flow_first_free; + bool to_free[FLOW_MAX] = { 0 }; bool timer = false; union flow *flow; @@ -798,9 +799,44 @@ void flow_defer_handler(const struct ctx *c, const struct timespec *now) ASSERT(!flow_new_entry); /* Incomplete flow at end of cycle */ - flow_foreach_slot(flow) { + /* Check which flows we might need to close first, but don't free them + * yet as it's not safe to do that in the middle of flow_foreach(). + */ + flow_foreach(flow) { bool closed = false; + switch (flow->f.type) { + case FLOW_TYPE_NONE: + ASSERT(false); + break; + case FLOW_TCP: + closed = tcp_flow_defer(&flow->tcp); + break; + case FLOW_TCP_SPLICE: + closed = tcp_splice_flow_defer(&flow->tcp_splice); + if (!closed && timer) + tcp_splice_timer(c, &flow->tcp_splice); + break; + case FLOW_PING4: + case FLOW_PING6: + if (timer) + closed = icmp_ping_timer(c, &flow->ping, now); + break; + case FLOW_UDP: + closed = udp_flow_defer(c, &flow->udp, now); + if (!closed && timer) + closed = udp_flow_timer(c, &flow->udp, now); + break; + default: + /* Assume other flow types don't need any handling */ + ; + } + + to_free[FLOW_IDX(flow)] = closed; + } + + /* Second step: actually free the flows */ + flow_foreach_slot(flow) { switch (flow->f.state) { case FLOW_STATE_FREE: { unsigned skip = flow->free.n; @@ -833,60 +869,31 @@ void flow_defer_handler(const struct ctx *c, const struct timespec *now) break; case FLOW_STATE_ACTIVE: - /* Nothing to do */ + if (to_free[FLOW_IDX(flow)]) { + flow_set_state(&flow->f, FLOW_STATE_FREE); + memset(flow, 0, sizeof(*flow)); + + if (free_head) { + /* Add slot to current free cluster */ + ASSERT(FLOW_IDX(flow) == + FLOW_IDX(free_head) + free_head->n); + free_head->n++; + flow->free.n = flow->free.next = 0; + } else { + /* Create new free cluster */ + free_head = &flow->free; + free_head->n = 1; + *last_next = FLOW_IDX(flow); + last_next = &free_head->next; + } + } else { + free_head = NULL; + } break; default: ASSERT(false); } - - switch (flow->f.type) { - case FLOW_TYPE_NONE: - ASSERT(false); - break; - case FLOW_TCP: - closed = tcp_flow_defer(&flow->tcp); - break; - case FLOW_TCP_SPLICE: - closed = tcp_splice_flow_defer(&flow->tcp_splice); - if (!closed && timer) - tcp_splice_timer(c, &flow->tcp_splice); - break; - case FLOW_PING4: - case FLOW_PING6: - if (timer) - closed = icmp_ping_timer(c, &flow->ping, now); - break; - case FLOW_UDP: - closed = udp_flow_defer(c, &flow->udp, now); - if (!closed && timer) - closed = udp_flow_timer(c, &flow->udp, now); - break; - default: - /* Assume other flow types don't need any handling */ - ; - } - - if (closed) { - flow_set_state(&flow->f, FLOW_STATE_FREE); - memset(flow, 0, sizeof(*flow)); - - if (free_head) { - /* Add slot to current free cluster */ - ASSERT(FLOW_IDX(flow) == - FLOW_IDX(free_head) + free_head->n); - free_head->n++; - flow->free.n = flow->free.next = 0; - } else { - /* Create new free cluster */ - free_head = &flow->free; - free_head->n = 1; - *last_next = FLOW_IDX(flow); - last_next = &free_head->next; - } - } else { - free_head = NULL; - } } *last_next = FLOW_MAX; -- 2.49.0

2 1

[PATCH 1/1] selinx: Add getattr to class udp_socket
by Janne Grunau 02 May '25

02 May '25

Commit 59cc89f ("udp, udp_flow: Track our specific address on socket interfaces") added a getsockname() call in udp_flow_new(). This requires getattr. Fixes "Flow 0 (UDP flow): Unable to determine local address: Permission denied" errors in muvm/passt on Fedora Linux 42 with SELinux. The SELinux audit message is | type=AVC msg=audit(1746083799.606:235): avc: denied { getattr } for | pid=2961 comm="passt" laddr=127.0.0.1 lport=49221 | faddr=127.0.0.53 fport=53 | scontext=unconfined_u:unconfined_r:passt_t:s0-s0:c0.c1023 | tcontext=unconfined_u:unconfined_r:passt_t:s0-s0:c0.c1023 | tclass=udp_socket permissive=0 Fixes: 59cc89f ("udp, udp_flow: Track our specific address on socket interfaces") Signed-off-by: Janne Grunau <janne-psst(a)jannau.net> --- contrib/selinux/passt.te | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/contrib/selinux/passt.te b/contrib/selinux/passt.te index f8ea672..eb9ce72 100644 --- a/contrib/selinux/passt.te +++ b/contrib/selinux/passt.te @@ -49,7 +49,7 @@ require { type proc_net_t; type node_t; class tcp_socket { create accept listen name_bind name_connect getattr ioctl }; - class udp_socket { create accept listen }; + class udp_socket { create accept listen getattr }; class icmp_socket { bind create name_bind node_bind setopt read write }; class sock_file { create unlink write }; @@ -133,7 +133,7 @@ allow passt_t node_t:icmp_socket { name_bind node_bind }; allow passt_t port_t:icmp_socket name_bind; allow passt_t self:tcp_socket { create getopt setopt connect bind listen accept shutdown read write getattr ioctl }; -allow passt_t self:udp_socket { create getopt setopt connect bind read write }; +allow passt_t self:udp_socket { create getopt setopt connect bind read write getattr }; allow passt_t self:icmp_socket { bind create setopt read write }; allow passt_t user_tmp_t:dir { add_name write }; -- 2.49.0

2 1