Re: [PATCH 2/2] udp: support traceroute for IPv4

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 5314 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]