[PATCH v9 0/4] Reconstruct incoming ICMP headers for failed UDP connect and forward back

newer
[PATCH v2 0/3] Send RST for guest...

Jon Maloy

4 Mar 2025 4 Mar '25

2:29 a.m.

v2: - Added patch breaking out udp header creation from function tap_udp4_send(). - Updated the ICMP creation by using the new function. - Added logics to find correct flow, depending on origin. - All done after feedback from David Gibson. v3: - More changes after feedback from David Gibson. v4: - Even more changes after feedback from D. Gibson v5: - Added corresponding patches for IPv6 v6: - Fixed some small nits after comments from D. Gibson. v7: - Added handling of all rejected ICMP messages - Returning correct user data amount if IPv6 as per RFC 4884. v8: - Added MTU to ICMPv4 ICMP_FRAG_NEEDED messages. - Added ASSERT() validation to message creation functions. v9: - Using real source address of ICMP to complement destination address for originial UDP message when needed. Jon Maloy (4): tap: break out building of udp header from tap_udp4_send function udp: create and send ICMPv4 to local peer when applicable tap: break out building of udp header from tap_udp6_send function udp: create and send ICMPv6 to local peer when applicable tap.c | 86 +++++++++++++++++++++++--------- tap.h | 15 ++++++ udp.c | 132 ++++++++++++++++++++++++++++++++++++++++++++----- udp_internal.h | 2 +- udp_vu.c | 4 +- 5 files changed, 200 insertions(+), 39 deletions(-) -- 2.48.1

Show replies by date

Jon Maloy

4 Mar 4 Mar

2:29 a.m.

New subject: [1/4] tap: break out building of udp header from tap_udp4_send function

We will need to build the UDP header at other locations than in function tap_udp4_send(), so we break that part out to a separate function. Reviewed-by: David Gibson Signed-off-by: Jon Maloy --- tap.c | 32 +++++++++++++++++++++++++------- tap.h | 5 +++++ 2 files changed, 30 insertions(+), 7 deletions(-) diff --git a/tap.c b/tap.c index 44b0fc0..d5c8edd 100644 --- a/tap.c +++ b/tap.c @@ -163,7 +163,7 @@ static void *tap_push_ip4h(struct iphdr *ip4h, struct in_addr src, } /** - * tap_udp4_send() - Send UDP over IPv4 packet + * tap_push_uh4() - Build UDPv4 header with checksum * @c: Execution context * @src: IPv4 source address * @sport: UDP source port @@ -172,15 +172,11 @@ static void *tap_push_ip4h(struct iphdr *ip4h, struct in_addr src, * @in: UDP payload contents (not including UDP header) * @dlen: UDP payload length (not including UDP header) */ -void tap_udp4_send(const struct ctx *c, struct in_addr src, in_port_t sport, +void *tap_push_uh4(struct udphdr *uh, struct in_addr src, in_port_t sport, struct in_addr dst, in_port_t dport, const void *in, size_t dlen) { size_t l4len = dlen + sizeof(struct udphdr); - char buf[USHRT_MAX]; - struct iphdr *ip4h = tap_push_l2h(c, buf, ETH_P_IP); - struct udphdr *uh = tap_push_ip4h(ip4h, src, dst, l4len, IPPROTO_UDP); - char *data = (char *)(uh + 1); const struct iovec iov = { .iov_base = (void *)in, .iov_len = dlen @@ -191,8 +187,30 @@ void tap_udp4_send(const struct ctx *c, struct in_addr src, in_port_t sport, uh->dest = htons(dport); uh->len = htons(l4len); csum_udp4(uh, src, dst, &payload); - memcpy(data, in, dlen); + return uh + 1; +} +/** + * tap_udp4_send() - Send UDP over IPv4 packet + * @c: Execution context + * @src: IPv4 source address + * @sport: UDP source port + * @dst: IPv4 destination address + * @dport: UDP destination port + * @in: UDP payload contents (not including UDP header) + * @dlen: UDP payload length (not including UDP header) + */ +void tap_udp4_send(const struct ctx *c, struct in_addr src, in_port_t sport, + struct in_addr dst, in_port_t dport, + const void *in, size_t dlen) +{ + size_t l4len = dlen + sizeof(struct udphdr); + char buf[USHRT_MAX]; + struct iphdr *ip4h = tap_push_l2h(c, buf, ETH_P_IP); + struct udphdr *uh = tap_push_ip4h(ip4h, src, dst, l4len, IPPROTO_UDP); + char *data = tap_push_uh4(uh, src, sport, dst, dport, in, dlen); + + memcpy(data, in, dlen); tap_send_single(c, buf, dlen + (data - buf)); } diff --git a/tap.h b/tap.h index a476a12..0b3eb93 100644 --- a/tap.h +++ b/tap.h @@ -6,6 +6,8 @@ #ifndef TAP_H #define TAP_H +struct udphdr; + /** * struct tap_hdr - tap backend specific headers * @vnet_len: Frame length (for qemu socket transport) @@ -42,6 +44,9 @@ static inline void tap_hdr_update(struct tap_hdr *thdr, size_t l2len) thdr->vnet_len = htonl(l2len); } +void *tap_push_uh4(struct udphdr *uh, struct in_addr src, in_port_t sport, + struct in_addr dst, in_port_t dport, + const void *in, size_t dlen); void tap_udp4_send(const struct ctx *c, struct in_addr src, in_port_t sport, struct in_addr dst, in_port_t dport, const void *in, size_t dlen); -- 2.48.1

Jon Maloy

2:29 a.m.

New subject: [2/4] udp: create and send ICMPv4 to local peer when applicable

When a local peer sends a UDP message to a non-existing port on an existing remote host, that host will return an ICMP message containing the error code ICMP_PORT_UNREACH, plus the header and the first eight bytes of the original message. If the sender socket has been connected, it uses this message to issue a "Connection Refused" event to the user. Until now, we have only read such events from the externally facing socket, but we don't forward them back to the local sender because we cannot read the ICMP message directly to user space. Because of this, the local peer will hang and wait for a response that never arrives. We now fix this for IPv4 by recreating and forwarding a correct ICMP message back to the internal sender. We synthesize the message based on the information in the extended error structure, plus the returned part of the original message body. Note that for the sake of completeness, we even produce ICMP messages for other error codes. We have noticed that at least ICMP_PROT_UNREACH is propagated as an error event back to the user. Reviewed-by: David Gibson Signed-off-by: Jon Maloy --- v2: - Updated the ICMP creation to use the new function tap_push_uh4(). - Added logics to find correct flow, depending on origin. - All done after feedback from David Gibson. v3: - Passing parameter 'now' along with call to udp_sock_errs() call. - Corrected lookup of flow from listener socket - All done after feedback from David Gibson. v4: - Eliminate any attempt to handle ICMPs from listener sockets. - After feedback from David Gibson. v5: - Small reshuffle in anticipation of following commits. v6: - Introduced macro ICMP4_MAX_DLEN v7: - Introduced ASSERT() on data lenght size - Added MTU for ICMPv4 ICMP_FRAG_NEEDED messages v8: - Distinguishing between ICMP source address and origininal UDP destination address. --- tap.c | 4 +-- tap.h | 2 ++ udp.c | 86 +++++++++++++++++++++++++++++++++++++++++++------- udp_internal.h | 2 +- udp_vu.c | 4 +-- 5 files changed, 81 insertions(+), 17 deletions(-) diff --git a/tap.c b/tap.c index d5c8edd..97416ac 100644 --- a/tap.c +++ b/tap.c @@ -143,8 +143,8 @@ static void *tap_push_l2h(const struct ctx *c, void *buf, uint16_t proto) * * Return: pointer at which to write the packet's payload */ -static void *tap_push_ip4h(struct iphdr *ip4h, struct in_addr src, - struct in_addr dst, size_t l4len, uint8_t proto) +void *tap_push_ip4h(struct iphdr *ip4h, struct in_addr src, + struct in_addr dst, size_t l4len, uint8_t proto) { uint16_t l3len = l4len + sizeof(*ip4h); diff --git a/tap.h b/tap.h index 0b3eb93..37da21d 100644 --- a/tap.h +++ b/tap.h @@ -47,6 +47,8 @@ static inline void tap_hdr_update(struct tap_hdr *thdr, size_t l2len) void *tap_push_uh4(struct udphdr *uh, struct in_addr src, in_port_t sport, struct in_addr dst, in_port_t dport, const void *in, size_t dlen); +void *tap_push_ip4h(struct iphdr *ip4h, struct in_addr src, + struct in_addr dst, size_t l4len, uint8_t proto); void tap_udp4_send(const struct ctx *c, struct in_addr src, in_port_t sport, struct in_addr dst, in_port_t dport, const void *in, size_t dlen); diff --git a/udp.c b/udp.c index 923cc38..c34eb42 100644 --- a/udp.c +++ b/udp.c @@ -87,6 +87,7 @@ #include #include #include +#include #include #include #include @@ -112,6 +113,9 @@ #include "udp_internal.h" #include "udp_vu.h" +/* Maximum UDP data to be returned in ICMP messages */ +#define ICMP4_MAX_DLEN 8 + /* "Spliced" sockets indexed by bound port (host order) */ static int udp_splice_ns [IP_VERSIONS][NUM_PORTS]; static int udp_splice_init[IP_VERSIONS][NUM_PORTS]; @@ -402,25 +406,75 @@ static void udp_tap_prepare(const struct mmsghdr *mmh, (*tap_iov)[UDP_IOV_PAYLOAD].iov_len = l4len; } +/** + * udp_send_conn_fail_icmp4() - Construct and send ICMPv4 to local peer + * @c: Execution context + * @ee: Extended error descriptor + * @toside: Destination side of flow + * @saddr: Address of ICMP generating node + * @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, + void *in, size_t dlen) +{ + struct in_addr oaddr = toside->oaddr.v4mapped.a4; + struct in_addr eaddr = toside->eaddr.v4mapped.a4; + in_port_t eport = toside->eport; + in_port_t oport = toside->oport; + struct { + struct icmphdr icmp4h; + struct iphdr ip4h; + struct udphdr uh; + char data[ICMP4_MAX_DLEN]; + } __attribute__((packed, aligned(__alignof__(max_align_t)))) msg; + size_t msglen = sizeof(msg) - sizeof(msg.data) + dlen; + + ASSERT(dlen <= ICMP4_MAX_DLEN); + memset(&msg, 0, sizeof(msg)); + msg.icmp4h.type = ee->ee_type; + msg.icmp4h.code = ee->ee_code; + if (ee->ee_type == ICMP_DEST_UNREACH && ee->ee_code == ICMP_FRAG_NEEDED) + msg.icmp4h.un.frag.mtu = htons((uint16_t) ee->ee_info); + + /* Reconstruct the original headers as returned in the ICMP message */ + tap_push_ip4h(&msg.ip4h, eaddr, oaddr, dlen, IPPROTO_UDP); + tap_push_uh4(&msg.uh, eaddr, eport, oaddr, oport, in, dlen); + memcpy(&msg.data, in, dlen); + + tap_icmp4_send(c, saddr, eaddr, &msg, msglen); +} + /** * udp_sock_recverr() - Receive and clear an error from a socket - * @s: Socket to receive from + * @c: Execution context + * @ref: epoll reference * * Return: 1 if error received and processed, 0 if no more errors in queue, < 0 * if there was an error reading the queue * * #syscalls recvmsg */ -static int udp_sock_recverr(int s) +static int udp_sock_recverr(const struct ctx *c, union epoll_ref ref) { const struct sock_extended_err *ee; const struct cmsghdr *hdr; + union sockaddr_inany saddr; char buf[CMSG_SPACE(sizeof(*ee))]; + char data[ICMP4_MAX_DLEN]; + int s = ref.fd; + struct iovec iov = { + .iov_base = data, + .iov_len = sizeof(data) + }; struct msghdr mh = { - .msg_name = NULL, - .msg_namelen = 0, - .msg_iov = NULL, - .msg_iovlen = 0, + .msg_name = &saddr, + .msg_namelen = sizeof(saddr), + .msg_iov = &iov, + .msg_iovlen = 1, .msg_control = buf, .msg_controllen = sizeof(buf), }; @@ -450,8 +504,15 @@ static int udp_sock_recverr(int s) } ee = (const struct sock_extended_err *)CMSG_DATA(hdr); + if (ref.type == EPOLL_TYPE_UDP_REPLY) { + flow_sidx_t sidx = flow_sidx_opposite(ref.flowside); + const struct flowside *toside = flowside_at_sidx(sidx); - /* TODO: When possible propagate and otherwise handle errors */ + udp_send_conn_fail_icmp4(c, ee, toside, saddr.sa4.sin_addr, + data, rc); + } else { + trace("Ignoring received IP_RECVERR cmsg on listener socket"); + } debug("%s error on UDP socket %i: %s", str_ee_origin(ee), s, strerror_(ee->ee_errno)); @@ -461,15 +522,16 @@ static int udp_sock_recverr(int s) /** * udp_sock_errs() - Process errors on a socket * @c: Execution context - * @s: Socket to receive from + * @ref: epoll reference * @events: epoll events bitmap * * Return: Number of errors handled, or < 0 if we have an unrecoverable error */ -int udp_sock_errs(const struct ctx *c, int s, uint32_t events) +int udp_sock_errs(const struct ctx *c, union epoll_ref ref, uint32_t events) { unsigned n_err = 0; socklen_t errlen; + int s = ref.fd; int rc, err; ASSERT(!c->no_udp); @@ -478,7 +540,7 @@ int udp_sock_errs(const struct ctx *c, int s, uint32_t events) return 0; /* Nothing to do */ /* Empty the error queue */ - while ((rc = udp_sock_recverr(s)) > 0) + while ((rc = udp_sock_recverr(c, ref)) > 0) n_err += rc; if (rc < 0) @@ -558,7 +620,7 @@ static void udp_buf_listen_sock_handler(const struct ctx *c, const socklen_t sasize = sizeof(udp_meta[0].s_in); int n, i; - if (udp_sock_errs(c, ref.fd, events) < 0) { + if (udp_sock_errs(c, ref, 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? */ @@ -661,7 +723,7 @@ static void udp_buf_reply_sock_handler(const struct ctx *c, union epoll_ref ref, from_s = uflow->s[ref.flowside.sidei]; - if (udp_sock_errs(c, from_s, events) < 0) { + if (udp_sock_errs(c, ref, events) < 0) { flow_err(uflow, "Unrecoverable error on reply socket"); flow_err_details(uflow); udp_flow_close(c, uflow); diff --git a/udp_internal.h b/udp_internal.h index cc80e30..3b081f5 100644 --- a/udp_internal.h +++ b/udp_internal.h @@ -30,5 +30,5 @@ size_t udp_update_hdr4(struct iphdr *ip4h, struct udp_payload_t *bp, size_t udp_update_hdr6(struct ipv6hdr *ip6h, struct udp_payload_t *bp, const struct flowside *toside, size_t dlen, bool no_udp_csum); -int udp_sock_errs(const struct ctx *c, int s, uint32_t events); +int udp_sock_errs(const struct ctx *c, union epoll_ref ref, uint32_t events); #endif /* UDP_INTERNAL_H */ diff --git a/udp_vu.c b/udp_vu.c index 4123510..c26a223 100644 --- a/udp_vu.c +++ b/udp_vu.c @@ -227,7 +227,7 @@ void udp_vu_listen_sock_handler(const struct ctx *c, union epoll_ref ref, struct vu_virtq *vq = &vdev->vq[VHOST_USER_RX_QUEUE]; int i; - if (udp_sock_errs(c, ref.fd, events) < 0) { + if (udp_sock_errs(c, ref, events) < 0) { err("UDP: Unrecoverable error on listening socket:" " (%s port %hu)", pif_name(ref.udp.pif), ref.udp.port); return; @@ -302,7 +302,7 @@ void udp_vu_reply_sock_handler(const struct ctx *c, union epoll_ref ref, ASSERT(!c->no_udp); - if (udp_sock_errs(c, from_s, events) < 0) { + if (udp_sock_errs(c, ref, events) < 0) { flow_err(uflow, "Unrecoverable error on reply socket"); flow_err_details(uflow); udp_flow_close(c, uflow); -- 2.48.1

Jon Maloy

2:29 a.m.

New subject: [3/4] tap: break out building of udp header from tap_udp6_send function

We will need to build the UDP header at other locations than in function tap_udp6_send(), so we break that part out to a separate function. Reviewed-by: David Gibson Signed-off-by: Jon Maloy --- tap.c | 40 ++++++++++++++++++++++++++++++---------- tap.h | 4 ++++ 2 files changed, 34 insertions(+), 10 deletions(-) diff --git a/tap.c b/tap.c index 97416ac..8aa47bd 100644 --- a/tap.c +++ b/tap.c @@ -266,27 +266,22 @@ static void *tap_push_ip6h(struct ipv6hdr *ip6h, } /** - * tap_udp6_send() - Send UDP over IPv6 packet + * tap_push_uh6() - Build UDPv6 header with checksum * @c: Execution context * @src: IPv6 source address * @sport: UDP source port * @dst: IPv6 destination address * @dport: UDP destination port * @flow: Flow label - * @in: UDP payload contents (not including UDP header) + * @in: UDP payload contents (not including UDP header) * @dlen: UDP payload length (not including UDP header) */ -void tap_udp6_send(const struct ctx *c, +void *tap_push_uh6(struct udphdr *uh, const struct in6_addr *src, in_port_t sport, const struct in6_addr *dst, in_port_t dport, - uint32_t flow, void *in, size_t dlen) + void *in, size_t dlen) { size_t l4len = dlen + sizeof(struct udphdr); - char buf[USHRT_MAX]; - struct ipv6hdr *ip6h = tap_push_l2h(c, buf, ETH_P_IPV6); - struct udphdr *uh = tap_push_ip6h(ip6h, src, dst, - l4len, IPPROTO_UDP, flow); - char *data = (char *)(uh + 1); const struct iovec iov = { .iov_base = in, .iov_len = dlen @@ -297,8 +292,33 @@ void tap_udp6_send(const struct ctx *c, uh->dest = htons(dport); uh->len = htons(l4len); csum_udp6(uh, src, dst, &payload); - memcpy(data, in, dlen); + return uh + 1; +} +/** + * tap_udp6_send() - Send UDP over IPv6 packet + * @c: Execution context + * @src: IPv6 source address + * @sport: UDP source port + * @dst: IPv6 destination address + * @dport: UDP destination port + * @flow: Flow label + * @in: UDP payload contents (not including UDP header) + * @dlen: UDP payload length (not including UDP header) + */ +void tap_udp6_send(const struct ctx *c, + const struct in6_addr *src, in_port_t sport, + const struct in6_addr *dst, in_port_t dport, + uint32_t flow, void *in, size_t dlen) +{ + size_t l4len = dlen + sizeof(struct udphdr); + char buf[USHRT_MAX]; + struct ipv6hdr *ip6h = tap_push_l2h(c, buf, ETH_P_IPV6); + struct udphdr *uh = tap_push_ip6h(ip6h, src, dst, + l4len, IPPROTO_UDP, flow); + char *data = tap_push_uh6(uh, src, sport, dst, dport, in, dlen); + + memcpy(data, in, dlen); tap_send_single(c, buf, dlen + (data - buf)); } diff --git a/tap.h b/tap.h index 37da21d..b7b8cef 100644 --- a/tap.h +++ b/tap.h @@ -47,6 +47,10 @@ static inline void tap_hdr_update(struct tap_hdr *thdr, size_t l2len) void *tap_push_uh4(struct udphdr *uh, struct in_addr src, in_port_t sport, struct in_addr dst, in_port_t dport, const void *in, size_t dlen); +void *tap_push_uh6(struct udphdr *uh, + const struct in6_addr *src, in_port_t sport, + const struct in6_addr *dst, in_port_t dport, + void *in, size_t dlen); void *tap_push_ip4h(struct iphdr *ip4h, struct in_addr src, struct in_addr dst, size_t l4len, uint8_t proto); void tap_udp4_send(const struct ctx *c, struct in_addr src, in_port_t sport, -- 2.48.1

Jon Maloy

2:29 a.m.

New subject: [4/4] udp: create and send ICMPv6 to local peer when applicable

When a local peer sends a UDP message to a non-existing port on an existing remote host, that host will return an ICMPv6 message containing the error code ICMP6_DST_UNREACH_NOPORT, plus the IPv6 header, UDP header and the first 1232 bytes of the original message, if any. If the sender socket has been connected, it uses this message to issue a "Connection Refused" event to the user. Until now, we have only read such events from the externally facing socket, but we don't forward them back to the local sender because we cannot read the ICMP message directly to user space. Because of this, the local peer will hang and wait for a response that never arrives. We now fix this for IPv6 by recreating and forwarding a correct ICMP message back to the internal sender. We synthesize the message based on the information in the extended error structure, plus the returned part of the original message body. Note that for the sake of completeness, we even produce ICMP messages for other error types and codes. We have noticed that at least ICMP_PROT_UNREACH is propagated as an error event back to the user. Reviewed-by: David Gibson Signed-off-by: Jon Maloy --- v2: - Handling all types or ICMP types and codes - Returning up to 1232 bytes of user data as per RFC 4884. Both suggested by anonymous PASST user. v3: - Added ASSERT() in the ICMPv6 message creation function. Suggested by David Gibson. v4: - Using real source address of ICMP to complement destination address for originial UDP message. --- tap.c | 8 ++++---- tap.h | 4 ++++ udp.c | 65 +++++++++++++++++++++++++++++++++++++++++++++++++++++++---- 3 files changed, 69 insertions(+), 8 deletions(-) diff --git a/tap.c b/tap.c index 8aa47bd..7e4bc00 100644 --- a/tap.c +++ b/tap.c @@ -247,10 +247,10 @@ void tap_icmp4_send(const struct ctx *c, struct in_addr src, struct in_addr dst, * * Return: pointer at which to write the packet's payload */ -static void *tap_push_ip6h(struct ipv6hdr *ip6h, - const struct in6_addr *src, - const struct in6_addr *dst, - size_t l4len, uint8_t proto, uint32_t flow) +void *tap_push_ip6h(struct ipv6hdr *ip6h, + const struct in6_addr *src, + const struct in6_addr *dst, + size_t l4len, uint8_t proto, uint32_t flow) { ip6h->payload_len = htons(l4len); ip6h->priority = 0; diff --git a/tap.h b/tap.h index b7b8cef..67aa9e6 100644 --- a/tap.h +++ b/tap.h @@ -53,6 +53,10 @@ void *tap_push_uh6(struct udphdr *uh, void *in, size_t dlen); void *tap_push_ip4h(struct iphdr *ip4h, struct in_addr src, struct in_addr dst, size_t l4len, uint8_t proto); +void *tap_push_ip6h(struct ipv6hdr *ip6h, + const struct in6_addr *src, + const struct in6_addr *dst, + size_t l4len, uint8_t proto, uint32_t flow); void tap_udp4_send(const struct ctx *c, struct in_addr src, in_port_t sport, struct in_addr dst, in_port_t dport, const void *in, size_t dlen); diff --git a/udp.c b/udp.c index c34eb42..b7962e9 100644 --- a/udp.c +++ b/udp.c @@ -88,6 +88,7 @@ #include #include #include +#include #include #include #include @@ -115,6 +116,9 @@ /* Maximum UDP data to be returned in ICMP messages */ #define ICMP4_MAX_DLEN 8 +#define ICMP6_MAX_DLEN (IPV6_MIN_MTU \ + - sizeof(struct udphdr) \ + - sizeof(struct ipv6hdr)) /* "Spliced" sockets indexed by bound port (host order) */ static int udp_splice_ns [IP_VERSIONS][NUM_PORTS]; @@ -448,6 +452,51 @@ static void udp_send_conn_fail_icmp4(const struct ctx *c, tap_icmp4_send(c, saddr, eaddr, &msg, msglen); } + +/** + * udp_send_conn_fail_icmp6() - Construct and send ICMPv6 to local peer + * @c: Execution context + * @ee: Extended error descriptor + * @toside: Destination side of flow + * @saddr: Address of ICMP generating node + * @in: First bytes (max 1232) of original UDP message body + * @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, + 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; + in_port_t eport = toside->eport; + in_port_t oport = toside->oport; + struct { + struct icmp6_hdr icmp6h; + struct ipv6hdr ip6h; + struct udphdr uh; + char data[ICMP6_MAX_DLEN]; + } __attribute__((packed, aligned(__alignof__(max_align_t)))) msg; + size_t msglen = sizeof(msg) - sizeof(msg.data) + dlen; + size_t l4len = dlen + sizeof(struct udphdr); + + ASSERT(dlen <= ICMP6_MAX_DLEN); + memset(&msg, 0, sizeof(msg)); + msg.icmp6h.icmp6_type = ee->ee_type; + msg.icmp6h.icmp6_code = ee->ee_code; + if (ee->ee_type == ICMP6_PACKET_TOO_BIG) + msg.icmp6h.icmp6_dataun.icmp6_un_data32[0] = htonl(ee->ee_info); + + /* Reconstruct the original headers as returned in the ICMP message */ + tap_push_ip6h(&msg.ip6h, eaddr, oaddr, l4len, IPPROTO_UDP, flow); + tap_push_uh6(&msg.uh, eaddr, eport, oaddr, oport, in, dlen); + memcpy(&msg.data, in, dlen); + + tap_icmp6_send(c, saddr, eaddr, &msg, msglen); +} + /** * udp_sock_recverr() - Receive and clear an error from a socket * @c: Execution context @@ -464,7 +513,7 @@ static int udp_sock_recverr(const struct ctx *c, union epoll_ref ref) const struct cmsghdr *hdr; union sockaddr_inany saddr; char buf[CMSG_SPACE(sizeof(*ee))]; - char data[ICMP4_MAX_DLEN]; + char data[ICMP6_MAX_DLEN]; int s = ref.fd; struct iovec iov = { .iov_base = data, @@ -507,9 +556,17 @@ static int udp_sock_recverr(const struct ctx *c, union epoll_ref ref) if (ref.type == EPOLL_TYPE_UDP_REPLY) { flow_sidx_t sidx = flow_sidx_opposite(ref.flowside); const struct flowside *toside = flowside_at_sidx(sidx); - - udp_send_conn_fail_icmp4(c, ee, toside, saddr.sa4.sin_addr, - data, rc); + size_t dlen = rc; + + 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); + } else if (hdr->cmsg_level == IPPROTO_IPV6) { + udp_send_conn_fail_icmp6(c, ee, toside, + &saddr.sa6.sin6_addr, + data, dlen, sidx.flowi); + } } else { trace("Ignoring received IP_RECVERR cmsg on listener socket"); } -- 2.48.1

David Gibson

5:46 a.m.

On Mon, Mar 03, 2025 at 08:29:11PM -0500, Jon Maloy wrote:

...

v2: - Added patch breaking out udp header creation from function tap_udp4_send(). - Updated the ICMP creation by using the new function. - Added logics to find correct flow, depending on origin. - All done after feedback from David Gibson. v3: - More changes after feedback from David Gibson. v4: - Even more changes after feedback from D. Gibson v5: - Added corresponding patches for IPv6 v6: - Fixed some small nits after comments from D. Gibson. v7: - Added handling of all rejected ICMP messages - Returning correct user data amount if IPv6 as per RFC 4884. v8: - Added MTU to ICMPv4 ICMP_FRAG_NEEDED messages. - Added ASSERT() validation to message creation functions. v9: - Using real source address of ICMP to complement destination address for originial UDP message when needed.

I think the changes for this are fine as far as they go. It does raise an additional wrinkle for when we try to do this for "listening" sockets: since the ICMP may be coming from somewhere other than the destination of the triggering message, we can't rely on the source address to find the correct flow. I think we'll need to use EE_OFFENDER() to get the information we need, which will also mean extended our cmsg buffers a bit: looks like the kernel puts a sockaddr after the extended_err structure. -- David Gibson (he or they) | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you, not the other way | around. http://www.ozlabs.org/~dgibson

Stefano Brivio

1:05 p.m.

On Mon, 3 Mar 2025 20:29:11 -0500 Jon Maloy wrote:

...

v2: - Added patch breaking out udp header creation from function tap_udp4_send(). - Updated the ICMP creation by using the new function. - Added logics to find correct flow, depending on origin. - All done after feedback from David Gibson. v3: - More changes after feedback from David Gibson. v4: - Even more changes after feedback from D. Gibson v5: - Added corresponding patches for IPv6 v6: - Fixed some small nits after comments from D. Gibson. v7: - Added handling of all rejected ICMP messages - Returning correct user data amount if IPv6 as per RFC 4884. v8: - Added MTU to ICMPv4 ICMP_FRAG_NEEDED messages. - Added ASSERT() validation to message creation functions. v9: - Using real source address of ICMP to complement destination address for originial UDP message when needed.

Jon Maloy (4): tap: break out building of udp header from tap_udp4_send function udp: create and send ICMPv4 to local peer when applicable tap: break out building of udp header from tap_udp6_send function udp: create and send ICMPv6 to local peer when applicable

I was about to apply those, then I realised that Coverity Scan isn't happy about a few things, listed below. I didn't check if those are false positives (I can have a look later or within a couple of days unless you get to it first). 1. --- /home/sbrivio/passt/udp.c:448:2: Type: Out-of-bounds access (ARRAY_VS_SINGLETON) /home/sbrivio/passt/udp.c:440:2: 1. path: Condition "!(dlen <= 8)", taking false branch. /home/sbrivio/passt/udp.c:444:2: 2. path: Condition "ee->ee_type == 3", taking true branch. /home/sbrivio/passt/udp.c:444:2: 3. path: Condition "ee->ee_code == 4", taking true branch. /home/sbrivio/passt/udp.c:448:2: 4. address_of: Taking address with "&msg.ip4h" yields a singleton pointer. /home/sbrivio/passt/udp.c:448:2: 5. callee_ptr_arith: Passing "&msg.ip4h" to function "tap_push_ip4h" which uses it as an array. This might corrupt or misinterpret adjacent memory locations. /home/sbrivio/passt/tap.c:162:2: 5.1. ptr_arith: Performing pointer arithmetic on "ip4h" in expression "ip4h + 1". --- 2. --- /home/sbrivio/passt/udp.c:493:2: Type: Out-of-bounds access (ARRAY_VS_SINGLETON) /home/sbrivio/passt/udp.c:485:2: 1. path: Condition "!(dlen <= 1232UL /* 1280 - sizeof (struct udphdr) - sizeof (struct ipv6hdr) */)", taking false branch. /home/sbrivio/passt/udp.c:489:2: 2. path: Condition "ee->ee_type == 2", taking true branch. /home/sbrivio/passt/udp.c:493:2: 3. address_of: Taking address with "&msg.ip6h" yields a singleton pointer. /home/sbrivio/passt/udp.c:493:2: 4. callee_ptr_arith: Passing "&msg.ip6h" to function "tap_push_ip6h" which uses it as an array. This might corrupt or misinterpret adjacent memory locations. /home/sbrivio/passt/tap.c:265:2: 4.1. ptr_arith: Performing pointer arithmetic on "ip6h" in expression "ip6h + 1". --- 3. --- /home/sbrivio/passt/udp.c:449:2: Type: Out-of-bounds access (ARRAY_VS_SINGLETON) /home/sbrivio/passt/udp.c:440:2: 1. path: Condition "!(dlen <= 8)", taking false branch. /home/sbrivio/passt/udp.c:444:2: 2. path: Condition "ee->ee_type == 3", taking true branch. /home/sbrivio/passt/udp.c:444:2: 3. path: Condition "ee->ee_code == 4", taking true branch. /home/sbrivio/passt/udp.c:449:2: 4. address_of: Taking address with "&msg.uh" yields a singleton pointer. /home/sbrivio/passt/udp.c:449:2: 5. callee_ptr_arith: Passing "&msg.uh" to function "tap_push_uh4" which uses it as an array. This might corrupt or misinterpret adjacent memory locations. /home/sbrivio/passt/tap.c:190:2: 5.1. ptr_arith: Performing pointer arithmetic on "uh" in expression "uh + 1". --- 4. --- /home/sbrivio/passt/udp.c:494:2: Type: Out-of-bounds access (ARRAY_VS_SINGLETON) /home/sbrivio/passt/udp.c:485:2: 1. path: Condition "!(dlen <= 1232UL /* 1280 - sizeof (struct udphdr) - sizeof (struct ipv6hdr) */)", taking false branch. /home/sbrivio/passt/udp.c:489:2: 2. path: Condition "ee->ee_type == 2", taking true branch. /home/sbrivio/passt/udp.c:494:2: 3. address_of: Taking address with "&msg.uh" yields a singleton pointer. /home/sbrivio/passt/udp.c:494:2: 4. callee_ptr_arith: Passing "&msg.uh" to function "tap_push_uh6" which uses it as an array. This might corrupt or misinterpret adjacent memory locations. /home/sbrivio/passt/tap.c:295:2: 4.1. ptr_arith: Performing pointer arithmetic on "uh" in expression "uh + 1". --- -- Stefano

Jon Maloy

11:44 p.m.

On 2025-03-04 07:05, Stefano Brivio wrote:

...

On Mon, 3 Mar 2025 20:29:11 -0500 Jon Maloy wrote:

...
v2: - Added patch breaking out udp header creation from function tap_udp4_send(). - Updated the ICMP creation by using the new function. - Added logics to find correct flow, depending on origin. - All done after feedback from David Gibson. v3: - More changes after feedback from David Gibson. v4: - Even more changes after feedback from D. Gibson v5: - Added corresponding patches for IPv6 v6: - Fixed some small nits after comments from D. Gibson. v7: - Added handling of all rejected ICMP messages - Returning correct user data amount if IPv6 as per RFC 4884. v8: - Added MTU to ICMPv4 ICMP_FRAG_NEEDED messages. - Added ASSERT() validation to message creation functions. v9: - Using real source address of ICMP to complement destination address for originial UDP message when needed.

Jon Maloy (4): tap: break out building of udp header from tap_udp4_send function udp: create and send ICMPv4 to local peer when applicable tap: break out building of udp header from tap_udp6_send function udp: create and send ICMPv6 to local peer when applicable

I was about to apply those, then I realised that Coverity Scan isn't happy about a few things, listed below. I didn't check if those are false positives (I can have a look later or within a couple of days unless you get to it first).

1. --- /home/sbrivio/passt/udp.c:448:2: Type: Out-of-bounds access (ARRAY_VS_SINGLETON)

/home/sbrivio/passt/udp.c:440:2: 1. path: Condition "!(dlen <= 8)", taking false branch. /home/sbrivio/passt/udp.c:444:2: 2. path: Condition "ee->ee_type == 3", taking true branch. /home/sbrivio/passt/udp.c:444:2: 3. path: Condition "ee->ee_code == 4", taking true branch. /home/sbrivio/passt/udp.c:448:2: 4. address_of: Taking address with "&msg.ip4h" yields a singleton pointer. /home/sbrivio/passt/udp.c:448:2: 5. callee_ptr_arith: Passing "&msg.ip4h" to function "tap_push_ip4h" which uses it as an array. This might corrupt or misinterpret adjacent memory locations. /home/sbrivio/passt/tap.c:162:2: 5.1. ptr_arith: Performing pointer arithmetic on "ip4h" in expression "ip4h + 1". ---

[...]

...

3. --- /home/sbrivio/passt/udp.c:449:2: Type: Out-of-bounds access (ARRAY_VS_SINGLETON)

/home/sbrivio/passt/udp.c:440:2: 1. path: Condition "!(dlen <= 8)", taking false branch. /home/sbrivio/passt/udp.c:444:2: 2. path: Condition "ee->ee_type == 3", taking true branch. /home/sbrivio/passt/udp.c:444:2: 3. path: Condition "ee->ee_code == 4", taking true branch. /home/sbrivio/passt/udp.c:449:2: 4. address_of: Taking address with "&msg.uh" yields a singleton pointer. /home/sbrivio/passt/udp.c:449:2: 5. callee_ptr_arith: Passing "&msg.uh" to function "tap_push_uh4" which uses it as an array. This might corrupt or misinterpret adjacent memory locations. /home/sbrivio/passt/tap.c:190:2: 5.1. ptr_arith: Performing pointer arithmetic on "uh" in expression "uh + 1". ---

[...] I installed coverity and tried it, of course with the same result. These are clearly false positives, and the first one is already in the upstream code, not added by me. I can probably get rid of them with some pointer gymnastics, but is it really worth it? BTW, I discovered a bug in patch #2 which I just fixed. I will post a v10 of my patches shortly. ///jon

Stefano Brivio

5 Mar 5 Mar

9:59 a.m.

[Dropping Laurent from Cc:, I'm not sure why he's Cc'ed here, and fixing David's email] On Tue, 4 Mar 2025 17:44:29 -0500 Jon Maloy wrote:

...

On 2025-03-04 07:05, Stefano Brivio wrote:

...
On Mon, 3 Mar 2025 20:29:11 -0500 Jon Maloy wrote:

...
v2: - Added patch breaking out udp header creation from function tap_udp4_send(). - Updated the ICMP creation by using the new function. - Added logics to find correct flow, depending on origin. - All done after feedback from David Gibson. v3: - More changes after feedback from David Gibson. v4: - Even more changes after feedback from D. Gibson v5: - Added corresponding patches for IPv6 v6: - Fixed some small nits after comments from D. Gibson. v7: - Added handling of all rejected ICMP messages - Returning correct user data amount if IPv6 as per RFC 4884. v8: - Added MTU to ICMPv4 ICMP_FRAG_NEEDED messages. - Added ASSERT() validation to message creation functions. v9: - Using real source address of ICMP to complement destination address for originial UDP message when needed.

Jon Maloy (4): tap: break out building of udp header from tap_udp4_send function udp: create and send ICMPv4 to local peer when applicable tap: break out building of udp header from tap_udp6_send function udp: create and send ICMPv6 to local peer when applicable

I was about to apply those, then I realised that Coverity Scan isn't happy about a few things, listed below. I didn't check if those are false positives (I can have a look later or within a couple of days unless you get to it first).

1. --- /home/sbrivio/passt/udp.c:448:2: Type: Out-of-bounds access (ARRAY_VS_SINGLETON)

/home/sbrivio/passt/udp.c:440:2: 1. path: Condition "!(dlen <= 8)", taking false branch. /home/sbrivio/passt/udp.c:444:2: 2. path: Condition "ee->ee_type == 3", taking true branch. /home/sbrivio/passt/udp.c:444:2: 3. path: Condition "ee->ee_code == 4", taking true branch. /home/sbrivio/passt/udp.c:448:2: 4. address_of: Taking address with "&msg.ip4h" yields a singleton pointer. /home/sbrivio/passt/udp.c:448:2: 5. callee_ptr_arith: Passing "&msg.ip4h" to function "tap_push_ip4h" which uses it as an array. This might corrupt or misinterpret adjacent memory locations. /home/sbrivio/passt/tap.c:162:2: 5.1. ptr_arith: Performing pointer arithmetic on "ip4h" in expression "ip4h + 1". ---

[...]

...
3. --- /home/sbrivio/passt/udp.c:449:2: Type: Out-of-bounds access (ARRAY_VS_SINGLETON)

/home/sbrivio/passt/udp.c:440:2: 1. path: Condition "!(dlen <= 8)", taking false branch. /home/sbrivio/passt/udp.c:444:2: 2. path: Condition "ee->ee_type == 3", taking true branch. /home/sbrivio/passt/udp.c:444:2: 3. path: Condition "ee->ee_code == 4", taking true branch. /home/sbrivio/passt/udp.c:449:2: 4. address_of: Taking address with "&msg.uh" yields a singleton pointer. /home/sbrivio/passt/udp.c:449:2: 5. callee_ptr_arith: Passing "&msg.uh" to function "tap_push_uh4" which uses it as an array. This might corrupt or misinterpret adjacent memory locations. /home/sbrivio/passt/tap.c:190:2: 5.1. ptr_arith: Performing pointer arithmetic on "uh" in expression "uh + 1". ---

[...] I installed coverity and tried it, of course with the same result.

These are clearly false positives, and the first one is already in the upstream code, not added by me.

Not really, that one: /home/sbrivio/passt/udp.c:448:2: 5. callee_ptr_arith: Passing "&msg.ip4h" to function "tap_push_ip4h" which uses it as an array. This might corrupt or misinterpret adjacent memory locations. /home/sbrivio/passt/tap.c:162:2: 5.1. ptr_arith: Performing pointer arithmetic on "ip4h" in expression "ip4h + 1". comes from patch 2/4 in this series: + /* Reconstruct the original headers as returned in the ICMP message */ + tap_push_ip4h(&msg.ip4h, eaddr, oaddr, l4len, IPPROTO_UDP); + tap_push_uh4(&msg.uh, eaddr, eport, oaddr, oport, in, dlen);

...

I can probably get rid of them with some pointer gymnastics, but is it really worth it?

You didn't mention why they're false positives, so I checked. First off, I noticed that you forgot to update function comments: you don't say what tap_push_uh4() and tap_push_uh6() return now. Hint: look at the function comments for tap_push_ip4h() and tap_push_ip6h(). By the way, in both comments: * @in: UDP payload contents (not including UDP header) needs two tabs to be aligned with the other arguments. Back to the warning: the fact is that "ip4h + 1", "ipv6h + 1", and "uh + 1" are seen, per se, as array usage. It's kind of arbitrary what "uses it as an array" means: one could even say that simply taking what's after an element (header) qualifies as array usage because you have one element and something after it in an ordered set. So yes, the check is overzealous and not helpful here, but strictly speaking I wouldn't call it a false positive. I think it's worth it because as part of my maintainer duties I routinely use that checker, and keeping warnings to a minimum decreases the noise I have to go through, regardless of the fact that warnings make no sense as it's the case here. Static checkers are otherwise very valuable. Besides, the workaround is perhaps a bit annoying but not significantly less readable, and it took me approximately 30 seconds: -- diff --git a/tap.c b/tap.c index 7e4bc00..48cca00 100644 --- a/tap.c +++ b/tap.c @@ -159,7 +159,8 @@ void *tap_push_ip4h(struct iphdr *ip4h, struct in_addr src, ip4h->saddr = src.s_addr; ip4h->daddr = dst.s_addr; ip4h->check = csum_ip4_header(l3len, proto, src, dst); - return ip4h + 1; + + return (char *)ip4h + sizeof(*ip4h); } /** @@ -187,7 +188,8 @@ void *tap_push_uh4(struct udphdr *uh, struct in_addr src, in_port_t sport, uh->dest = htons(dport); uh->len = htons(l4len); csum_udp4(uh, src, dst, &payload); - return uh + 1; + + return (char *)uh + sizeof(*uh); } /** @@ -262,7 +264,8 @@ void *tap_push_ip6h(struct ipv6hdr *ip6h, ip6h->flow_lbl[0] = (flow >> 16) & 0xf; ip6h->flow_lbl[1] = (flow >> 8) & 0xff; ip6h->flow_lbl[2] = (flow >> 0) & 0xff; - return ip6h + 1; + + return (char *)ip6h + sizeof(*ip6h); } /** @@ -292,7 +295,8 @@ void *tap_push_uh6(struct udphdr *uh, uh->dest = htons(dport); uh->len = htons(l4len); csum_udp6(uh, src, dst, &payload); - return uh + 1; + + return (char *)uh + sizeof(*uh); } /** -- -- Stefano

Age (days ago)

Last active (days ago)

List overview

Download

8 comments

3 participants

participants (3)

David Gibson
Jon Maloy
Stefano Brivio

[PATCH v9 0/4] Reconstruct incoming ICMP headers for failed UDP connect and forward back

tags

participants (3)