On Tue, Mar 04, 2025 at 05:45:30PM -0500, Jon Maloy wrote: > 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. > v9: - Fixed wrong l4len value given to tap_push_ip4h() Ah, yes. I also missed that in the previous version. Again, Reviewed-by: David Gibson > --- > tap.c | 4 +-- > tap.h | 2 ++ > udp.c | 87 +++++++++++++++++++++++++++++++++++++++++++------- > udp_internal.h | 2 +- > udp_vu.c | 4 +-- > 5 files changed, 82 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..c260e3d 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,76 @@ 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; > + size_t l4len = dlen + sizeof(struct udphdr); > + > + 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, l4len, 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 +505,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 +523,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 +541,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 +621,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 +724,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); -- 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