On Sat, Mar 15, 2025 at 11:32:44AM -0400, Jon Maloy wrote:

While developing traceroute forwarding tap-to-sock we found that struct msghdr.msg_name for the ICMPs in the opposite direction always contains the destination address of the original UDP message, and not, as one might expect, the one of the host which created the error message.

Study of the kernel code reveals that this address instead is appended as extra data after the received struct sock_extended_err area.

We now change the ICMP receive code accordingly.

Fixes: 55431f0077b6 ("udp: create and send ICMPv4 to local peer when applicable") Fixes: 68b04182e07d ("udp: create and send ICMPv6 to local peer when applicable")

Signed-off-by: Jon Maloy

The idea looks good, a few minor warts in the implementation.

--- udp.c | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/udp.c b/udp.c index 80520cb..271e570 100644 --- a/udp.c +++ b/udp.c @@ -510,10 +510,13 @@ static void udp_send_conn_fail_icmp6(const struct ctx *c, */ static int udp_sock_recverr(const struct ctx *c, union epoll_ref ref) { - const struct sock_extended_err *ee; + struct errhdr { + struct sock_extended_err ee; + union sockaddr_inany saddr; + }; + const struct errhdr *eh; const struct cmsghdr *hdr; - union sockaddr_inany saddr; - char buf[CMSG_SPACE(sizeof(*ee))]; + char buf[CMSG_SPACE(sizeof(struct errhdr))]; char data[ICMP6_MAX_DLEN]; int s = ref.fd; struct iovec iov = { @@ -521,11 +524,11 @@ static int udp_sock_recverr(const struct ctx *c, union epoll_ref ref) .iov_len = sizeof(data) }; struct msghdr mh = { - .msg_name = &saddr, - .msg_namelen = sizeof(saddr), + .msg_name = 0, + .msg_namelen = 0,

I think you can just omit these and they'll be zero initialized.

.msg_iov = &iov, .msg_iovlen = 1, - .msg_control = buf, + .msg_control = buf,//(void *)&errhdr,

Looks like you have a leftover comment here.

.msg_controllen = sizeof(buf), }; ssize_t rc; @@ -553,7 +556,7 @@ static int udp_sock_recverr(const struct ctx *c, union epoll_ref ref) return -1; }

- ee = (const struct sock_extended_err *)CMSG_DATA(hdr); + eh = (const struct errhdr *)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); @@ -561,18 +564,19 @@ static int udp_sock_recverr(const struct ctx *c, union epoll_ref ref)

if (hdr->cmsg_level == IPPROTO_IP) { dlen = MIN(dlen, ICMP4_MAX_DLEN); - udp_send_conn_fail_icmp4(c, ee, toside, saddr.sa4.sin_addr, + udp_send_conn_fail_icmp4(c, &eh->ee, toside, + eh->saddr.sa4.sin_addr, data, dlen);

So, it looks from man ip(7) that you're supposed to use the SO_EE_OFFENDER() macro to get the socket address, rather than just assuming it's immediately after the struct sock_extended_err. Except... without assuming that, I can't really see how you're supposed to properly size the buffer

} else if (hdr->cmsg_level == IPPROTO_IPV6) { - udp_send_conn_fail_icmp6(c, ee, toside, - &saddr.sa6.sin6_addr, + udp_send_conn_fail_icmp6(c, &eh->ee, toside, + &eh->saddr.sa6.sin6_addr, data, dlen, sidx.flowi); } } 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)); + str_ee_origin(&eh->ee), s, strerror_(eh->ee.ee_errno));

return 1; }

-- 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

On Sat, Mar 15, 2025 at 11:32:45AM -0400, Jon Maloy wrote:

Now that ICMP pass-through from socket-to-tap is in place, it is easy to support UDP based traceroute functionality in direction tap-to-socket.

We fix that for IPv4 in this commit.

Signed-off-by: Jon Maloy --- packet.h | 2 ++ tap.c | 8 ++++++-- udp.c | 10 +++++++++- udp.h | 3 ++- 4 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/packet.h b/packet.h index d099f02..e406c6d 100644 --- a/packet.h +++ b/packet.h @@ -9,6 +9,8 @@ /* Maximum size of a single packet stored in pool, including headers */ #define PACKET_MAX_LEN UINT16_MAX

+#define DEFAULT_TTL 64 + /** * struct pool - Generic pool of packets stored in a buffer * @buf: Buffer storing packet descriptors, diff --git a/tap.c b/tap.c index 182a115..7d0970d 100644 --- a/tap.c +++ b/tap.c @@ -544,6 +544,7 @@ PACKET_POOL_DECL(pool_l4, UIO_MAXIOV, pkt_buf); * @dest: Destination port * @saddr: Source address * @daddr: Destination address + * @ttl: Time to live * @msg: Array of messages that can be handled in a single call */ static struct tap4_l4_t { @@ -555,6 +556,8 @@ static struct tap4_l4_t { struct in_addr saddr; struct in_addr daddr;

+ uint8_t ttl; +

Putting the TTL in here kind of implies that it's the same for all the packets in the pool below, but AFAICT you don't ensure that's the case...

struct pool_l4_t p; } tap4_l4[TAP_SEQS /* Arbitrary: TAP_MSGS in theory, so limit in users */];

@@ -776,6 +779,7 @@ resume: (seq)->dest = (uh)->dest; \ (seq)->saddr.s_addr = (iph)->saddr; \ (seq)->daddr.s_addr = (iph)->daddr; \ + (seq)->ttl = (iph)->ttl; \ } while (0)

... to do so, you'd need to update the L4_MATCH macro as well as the L4_SET macro. That said... grouping the packets by TTL seems a slightly odd thing to do. But to avoid doing so we'd need to pass additional information (say the whole IP header) per packet, which is a somewhat wider rework.

if (seq && L4_MATCH(iph, uh, seq) && seq->p.count < UIO_MAXIOV) @@ -824,7 +828,7 @@ append: for (k = 0; k < p->count; ) k += udp_tap_handler(c, PIF_TAP, AF_INET, &seq->saddr, &seq->daddr, - p, k, now); + seq->ttl, p, k, now); } }

@@ -1007,7 +1011,7 @@ append: for (k = 0; k < p->count; ) k += udp_tap_handler(c, PIF_TAP, AF_INET6, &seq->saddr, &seq->daddr, - p, k, now); + DEFAULT_TTL, p, k, now);

I believe there's an equivalent to TTL for IPv6. hop_limit?

} }

diff --git a/udp.c b/udp.c index 271e570..1ee289c 100644 --- a/udp.c +++ b/udp.c @@ -844,6 +844,7 @@ void udp_reply_sock_handler(const struct ctx *c, union epoll_ref ref, * @af: Address family, AF_INET or AF_INET6 * @saddr: Source address * @daddr: Destination address + * @ttl: TTL for packets to be sent in this call * @p: Pool of UDP packets, with UDP headers * @idx: Index of first packet to process * @now: Current timestamp @@ -854,7 +855,8 @@ void udp_reply_sock_handler(const struct ctx *c, union epoll_ref ref, */ int udp_tap_handler(const struct ctx *c, uint8_t pif, sa_family_t af, const void *saddr, const void *daddr, - const struct pool *p, int idx, const struct timespec *now) + uint8_t ttl, const struct pool *p, int idx, + const struct timespec *now) { const struct flowside *toside; struct mmsghdr mm[UIO_MAXIOV]; @@ -933,6 +935,12 @@ int udp_tap_handler(const struct ctx *c, uint8_t pif, mm[i].msg_hdr.msg_controllen = 0; mm[i].msg_hdr.msg_flags = 0;

+ if (ttl <= 30) { + if (setsockopt(s, IPPROTO_IP, IP_TTL, + &ttl, sizeof(ttl)) < 0)

AFAIK this will set the TTL on every subsequent packet, not just the next one, so this isn't quite right. To use this I guess we'd have to store the correct TTL in the flow, and do the sockopt if it's different from the guest side one. Unless there's a way to set TTL via cmsg. Maybe there is, but I haven't spotted it in a quick glance.

+ perror("setsockopt (IP_TTL)"); + } + count++; }

diff --git a/udp.h b/udp.h index de2df6d..041fad4 100644 --- a/udp.h +++ b/udp.h @@ -15,7 +15,8 @@ void udp_reply_sock_handler(const struct ctx *c, union epoll_ref ref, uint32_t events, const struct timespec *now); int udp_tap_handler(const struct ctx *c, uint8_t pif, sa_family_t af, const void *saddr, const void *daddr, - const struct pool *p, int idx, const struct timespec *now); + uint8_t ttl, const struct pool *p, int idx, + const struct timespec *now); int udp_sock_init(const struct ctx *c, int ns, const union inany_addr *addr, const char *ifname, in_port_t port); int udp_init(struct ctx *c);

-- 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

On Mon, Mar 17, 2025 at 01:40:51PM -0400, Jon Maloy wrote:

On 2025-03-16 22:58, David Gibson wrote:

...

On Sat, Mar 15, 2025 at 11:32:45AM -0400, Jon Maloy wrote:

...

Now that ICMP pass-through from socket-to-tap is in place, it is easy to support UDP based traceroute functionality in direction tap-to-socket.

[...]

...

...

+ uint8_t ttl, const struct pool *p, int idx, + const struct timespec *now) { const struct flowside *toside; struct mmsghdr mm[UIO_MAXIOV]; @@ -933,6 +935,12 @@ int udp_tap_handler(const struct ctx *c, uint8_t pif, mm[i].msg_hdr.msg_controllen = 0; mm[i].msg_hdr.msg_flags = 0; + if (ttl <= 30) { + if (setsockopt(s, IPPROTO_IP, IP_TTL, + &ttl, sizeof(ttl)) < 0)

AFAIK this will set the TTL on every subsequent packet, not just the next one, so this isn't quite right. To use this I guess we'd have to store the correct TTL in the flow, and do the sockopt if it's different from the guest side one.

I forgot one thing at our discussion about this at our meeting. Traceroute works by sending three packets with ttl=1, then three with ttl=2, then three with ttl=3 and so forth. For each packet sent it steps the destination port number, starting at 33334, and in the most extreme case (assuming it stops at ttl=32, which linux traceroute does in practice) stopping at port number 33524. The whole port range [33434:33535] is unassigned by IANA, and no sane developer would use it for his application, for obvious reasons.

This means that every single traceroute/udp message will become a separate flow, and there is no need to complicate things by storing the ttl in the flow or use it as criteria. It also means that my current approach works fine. I don't even need to restore the ttl in the outgoing socket, since that, just like the associated flow, is a single-shot affair.

Urgh. I mean, yes it works for (the current implementation of) traceroute, but doing it like this really isn't correct for anything else that manipulates TTL. I mean, yes, traceroute is *probably* the only thing to do that, but I think we should endeavour to be more generally correct if it's not too complicated. And in this case, I don't think it's too complicated.

...

Unless there's a way to set TTL via cmsg. Maybe there is, but I haven't spotted it in a quick glance.

I seems I can add it as ancillary data if I add a control buffer to struct mmsghdr.msghdr. However, it is hard to see any benefit of this, given the above. It will cost more performance on the critical code path than the simple test I am doing now.

Thinking about this more, I don't actually think setting the TTL per-packet with a cmsg is particularly useful anyway. For traceroute to a small extent - and I suspect in the unlikely case of anything else manipulating TTL - it seems like the guest is likely to use the same TTL for multiple packets in a row. That means just calling the setsockopt() when it changes from the current value should work well. -- 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