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

[PATCH 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.

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 | 38 ++++++++++++++++++++------------------
 1 file changed, 20 insertions(+), 18 deletions(-)

-- 
2.49.0

[PATCH 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>
---
 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);
-- 
@@ -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);
-- 
2.49.0

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

[Jon Maloy]

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 <david@gibson.dropbear.id.au>
> ---
>   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 <jmaloy@redhat.com>

[PATCH 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>
---
 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");
-- 
@@ -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");
-- 
2.49.0

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

[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 <david@gibson.dropbear.id.au>
> ---
>   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 <jmaloy@redhat.com>

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

[Stefano Brivio]

On Wed, 26 Mar 2025 11:14:59 +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.
> 
> David Gibson (2):
>   udp: Don't attempt to forward ICMP socket errors to other sockets
>   udp: Improve name of UDP related ICMP sending functions

Jon, just for clarity, I'll wait for your review on those.

Both look good to me by the way. I wasn't aware of the fact that one
can do if (...) { } else ... without even a semicolon in the body.

-- 
Stefano