On 2025-03-25 20:15, David Gibson wrote:

Recently we added support for detecting ICMP triggered errors on UDP sockets and forwarding them to the tap interface. However, in udp_sock_recverr() where this is handled we don't know for certain that the tap interface is the other side of the UDP flow. It could be a spliced connection with another socket on the other side.

To forward errors in that case, we'd need to force the other side's socket to trigger issue an ICMP error. I'm not sure if there's a way to do that; probably not for an arbitrary ICMP but it might be possible for certain error conditions.

Nonetheless what we do now - synthesise an ICMP on the tap interface - is certainly wrong. It's probably harmless; for a spliced connection it will have loopback addresses meaning we can expect the guest to discard it. But, correct this for now, by not attempting to propagate errors when the other side of the flow is a socket.

Fixes: 55431f007 ("udp: create and send ICMPv4 to local peer when...") Fixes: 68b04182e ("udp: create and send ICMPv6 to local peer when...")

Signed-off-by: David Gibson --- udp.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/udp.c b/udp.c index 80520cbd..a706fed9 100644 --- a/udp.c +++ b/udp.c @@ -559,7 +559,10 @@ static int udp_sock_recverr(const struct ctx *c, union epoll_ref ref) const struct flowside *toside = flowside_at_sidx(sidx); size_t dlen = rc;

- if (hdr->cmsg_level == IPPROTO_IP) { + if (pif_is_socket(pif_at_sidx(sidx))) { + /* XXX Is there any way to propagate ICMPs from socket + * to socket? */ + } else if (hdr->cmsg_level == IPPROTO_IP) { dlen = MIN(dlen, ICMP4_MAX_DLEN); udp_send_conn_fail_icmp4(c, ee, toside, saddr.sa4.sin_addr, data, dlen);

I never thought about this. I'll try to think of a solution, if there even is any. In the meantime: Acked-by: Jon Maloy

On 2025-03-25 20:15, David Gibson wrote:

udp_send_conn_fail_icmp[46]() aren't actually specific to connections failing: they can propagate a variety of ICMP errors, which might or might not break a "connection". They are, however, specific to sending ICMP errors to the tap connection, not splice or host. Rename them to better reflect that.

Signed-off-by: David Gibson --- udp.c | 33 ++++++++++++++++----------------- 1 file changed, 16 insertions(+), 17 deletions(-)

diff --git a/udp.c b/udp.c index a706fed9..88128181 100644 --- a/udp.c +++ b/udp.c @@ -411,7 +411,7 @@ static void udp_tap_prepare(const struct mmsghdr *mmh, }

/** - * udp_send_conn_fail_icmp4() - Construct and send ICMPv4 to local peer + * udp_send_tap_icmp4() - Construct and send ICMPv4 to local peer * @c: Execution context * @ee: Extended error descriptor * @toside: Destination side of flow @@ -419,11 +419,11 @@ static void udp_tap_prepare(const struct mmsghdr *mmh, * @in: First bytes (max 8) of original UDP message body * @dlen: Length of the read part of original UDP message body */ -static void udp_send_conn_fail_icmp4(const struct ctx *c, - const struct sock_extended_err *ee, - const struct flowside *toside, - struct in_addr saddr, - const void *in, size_t dlen) +static void udp_send_tap_icmp4(const struct ctx *c, + const struct sock_extended_err *ee, + const struct flowside *toside, + struct in_addr saddr, + const void *in, size_t dlen) { struct in_addr oaddr = toside->oaddr.v4mapped.a4; struct in_addr eaddr = toside->eaddr.v4mapped.a4; @@ -455,7 +455,7 @@ static void udp_send_conn_fail_icmp4(const struct ctx *c,

/** - * udp_send_conn_fail_icmp6() - Construct and send ICMPv6 to local peer + * udp_send_tap_icmp6() - Construct and send ICMPv6 to local peer * @c: Execution context * @ee: Extended error descriptor * @toside: Destination side of flow @@ -464,11 +464,11 @@ static void udp_send_conn_fail_icmp4(const struct ctx *c, * @dlen: Length of the read part of original UDP message body * @flow: IPv6 flow identifier */ -static void udp_send_conn_fail_icmp6(const struct ctx *c, - const struct sock_extended_err *ee, - const struct flowside *toside, - const struct in6_addr *saddr, - void *in, size_t dlen, uint32_t flow) +static void udp_send_tap_icmp6(const struct ctx *c, + const struct sock_extended_err *ee, + const struct flowside *toside, + const struct in6_addr *saddr, + void *in, size_t dlen, uint32_t flow) { const struct in6_addr *oaddr = &toside->oaddr.a6; const struct in6_addr *eaddr = &toside->eaddr.a6; @@ -564,12 +564,11 @@ static int udp_sock_recverr(const struct ctx *c, union epoll_ref ref) * to socket? */ } else if (hdr->cmsg_level == IPPROTO_IP) { dlen = MIN(dlen, ICMP4_MAX_DLEN); - udp_send_conn_fail_icmp4(c, ee, toside, saddr.sa4.sin_addr, - data, dlen); + udp_send_tap_icmp4(c, ee, toside, saddr.sa4.sin_addr, + data, dlen); } else if (hdr->cmsg_level == IPPROTO_IPV6) { - udp_send_conn_fail_icmp6(c, ee, toside, - &saddr.sa6.sin6_addr, - data, dlen, sidx.flowi); + udp_send_tap_icmp6(c, ee, toside, &saddr.sa6.sin6_addr, + data, dlen, sidx.flowi); } } else { trace("Ignoring received IP_RECVERR cmsg on listener socket");

Acked-by: Jon Maloy