[PATCH v2 0/2] udp: Fix some minor warts in the ICMP error propagation code

[PATCH v2 0/2] udp: Fix some minor warts in the ICMP error propagation code

[David Gibson]

While working on using connected sockets for both sides of UDP flows,
I spotted a couple of minor problems in the recently added ICMP error
propagation code.  Here are the fixes.

v2:
 * Rebase and fix conflict with Jon's recent fix in the same code

David Gibson (2):
  udp: Don't attempt to forward ICMP socket errors to other sockets
  udp: Improve name of UDP related ICMP sending functions

 udp.c | 40 +++++++++++++++++++++-------------------
 1 file changed, 21 insertions(+), 19 deletions(-)

-- 
2.49.0

[PATCH v2 1/2] udp: Don't attempt to forward ICMP socket errors to other sockets

[David Gibson]

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 <david@gibson.dropbear.id.au>
Acked-by: Jon Maloy <jmaloy@redhat.com>
---
 udp.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/udp.c b/udp.c
index 0c223b4e..e410f55e 100644
--- a/udp.c
+++ b/udp.c
@@ -560,7 +560,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, &eh->ee, toside,
 						 eh->saddr.sa4.sin_addr,
-- 
@@ -560,7 +560,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, &eh->ee, toside,
 						 eh->saddr.sa4.sin_addr,
-- 
2.49.0

[PATCH v2 2/2] udp: Improve name of UDP related ICMP sending functions

[David Gibson]

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 <david@gibson.dropbear.id.au>
Acked-by: Jon Maloy <jmaloy@redhat.com>
---
 udp.c | 35 +++++++++++++++++------------------
 1 file changed, 17 insertions(+), 18 deletions(-)

diff --git a/udp.c b/udp.c
index e410f55e..39431d72 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;
@@ -565,13 +565,12 @@ 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, &eh->ee, toside,
-						 eh->saddr.sa4.sin_addr,
-						 data, dlen);
+			udp_send_tap_icmp4(c, &eh->ee, toside,
+					   eh->saddr.sa4.sin_addr, data, dlen);
 		} else if (hdr->cmsg_level == IPPROTO_IPV6) {
-			udp_send_conn_fail_icmp6(c, &eh->ee, toside,
-						 &eh->saddr.sa6.sin6_addr,
-						 data, dlen, sidx.flowi);
+			udp_send_tap_icmp6(c, &eh->ee, toside,
+					   &eh->saddr.sa6.sin6_addr, data,
+					   dlen, sidx.flowi);
 		}
 	} else {
 		trace("Ignoring received IP_RECVERR cmsg on listener socket");
-- 
@@ -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;
@@ -565,13 +565,12 @@ 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, &eh->ee, toside,
-						 eh->saddr.sa4.sin_addr,
-						 data, dlen);
+			udp_send_tap_icmp4(c, &eh->ee, toside,
+					   eh->saddr.sa4.sin_addr, data, dlen);
 		} else if (hdr->cmsg_level == IPPROTO_IPV6) {
-			udp_send_conn_fail_icmp6(c, &eh->ee, toside,
-						 &eh->saddr.sa6.sin6_addr,
-						 data, dlen, sidx.flowi);
+			udp_send_tap_icmp6(c, &eh->ee, toside,
+					   &eh->saddr.sa6.sin6_addr, data,
+					   dlen, sidx.flowi);
 		}
 	} else {
 		trace("Ignoring received IP_RECVERR cmsg on listener socket");
-- 
2.49.0

Re: [PATCH v2 0/2] udp: Fix some minor warts in the ICMP error propagation code

[Stefano Brivio]

On Fri, 28 Mar 2025 14:34:13 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> While working on using connected sockets for both sides of UDP flows,
> I spotted a couple of minor problems in the recently added ICMP error
> propagation code.  Here are the fixes.
> 
> v2:
>  * Rebase and fix conflict with Jon's recent fix in the same code
> 
> David Gibson (2):
>   udp: Don't attempt to forward ICMP socket errors to other sockets
>   udp: Improve name of UDP related ICMP sending functions

Applied.

-- 
Stefano