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 <jmaloy(a)redhat.com> 
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 <jmaloy(a)redhat.com>
 ---
  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