Re: [PATCH v4] udp: support traceroute

[Jon Maloy <jmaloy@redhat.com>]

On 2025-04-04 07:50, Stefano Brivio wrote:
> Jon, I wasn't actually suggesting that you would drop *all* the Cc:'s.
> :) I just saw no reason to specifically spam Laurent with this.
> 
> On Fri, 4 Apr 2025 10:31:29 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
>> On Thu, Apr 03, 2025 at 04:27:12PM -0400, Jon Maloy wrote:
>>>
>>>
>>> On 2025-04-03 11:48, Stefano Brivio wrote:
>>>> The implementation looks solid to me, a list of nits (or a bit
>>>> more) below.
>>>>
>>>> By the way, I don't think you need to Cc: people who are already on
>>>> this list unless you specifically want their attention.
>>>>
>>>> On Wed,  2 Apr 2025 22:22:29 -0400
>>>> Jon Maloy <jmaloy@redhat.com> 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  in this commit.
>>>>>
>>>>> Signed-off-by: Jon Maloy <jmaloy@redhat.com>
>>>>
>>>> This fixes https://bugs.passt.top/show_bug.cgi?id=64 ("Link:" tag) if I
>>>> understood correctly.
>>>>    
>>>>> ---
>>>>> v2: - Using ancillary data instead of setsockopt to transfer outgoing
>>>>>         TTL.
>>>>>       - Support IPv6
>>>>> v3: - Storing ttl per packet instead of per flow. This may not be
>>>>>         elegant, but much less intrusive than changing the flow
>>>
>>> [...]
>>>    
>>>>> @@ -11,6 +11,8 @@
>>>>>    /* Maximum size of a single packet stored in pool, including headers */
>>>>>    #define PACKET_MAX_LEN	((size_t)UINT16_MAX)
>>>>> +#define DEFAULT_TTL 64
>>>>
>>>> If I understood correctly, David's comment to this on v3:
>>>>
>>>>     https://archives.passt.top/passt-dev/Z-om3Ey-HR1Hj8UH@zatzit/
>>>>
>>>> was meant to imply that, as the default value can be changed via
>>>> sysctl, the value set via sysctl could be read at start-up. I'm fine
>>>> with 64 as well, by the way, with a slight preference for reading the
>>>> value via sysctl.
>>>
>>> I don't think the local host/container setting will have any effect
>>> if the sending guest is a VM.
>>
>> That's true, but..
>>
>>> The benefit is of this is dubious.
>>
>> .. uflow->ttl[] isn't so much representing what the guest set, as a
>> cache of what the socket is sending and that *does* depend on the host
>> value.
> 
> Right, my concern is that now we'll use the host value (whatever it is)
> if the value from the container / guest is 64.
> 
> So:
> 
> - guest uses 63, host has 255 configured: we use 63
> 
> - guest uses 64, host has 64 configured: we use 64
> 
> - ...but: guest uses 64, host has 255 configured: we use 255
> 
> ...and this might actually break traceroute itself in some extreme
> cases.
> 
> Let's say we have 255 configured on the host and you're in the middle
> of a traceroute:
> 
> - guest sends TTL 62, goes out with 62 -> 62nd hop replies
> - guest sends TTL 63, goes out with 63 -> 63rd hop replies
> - guest sends TTL 64, goes out with 255 -> destination replies
> - guest sends TTL 65, goes out with 65 -> 65th hop replies, traceroute broken

Conclusion is that we have to set TTL in the socket at the opening of
every new flow in direction tap->sock. I can do that in a separate patch.

> 
> See also the comment below.
> 
>>>> All this might go away, though, please read the comment to
>>>> udp_flow_new() below, first.
>>>>    
>>>>> +
>>>>>    /**
>>>>>     * struct pool - Generic pool of packets stored in a buffer
>>>>>     * @buf:	Buffer storing packet descriptors,
>>>>> diff --git a/tap.c b/tap.c
>>>>> index 3a6fcbe..e65d592 100644
>>>>> --- a/tap.c
>>>>> +++ b/tap.c
>>>>> @@ -563,6 +563,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 {
>>>>> @@ -574,6 +575,8 @@ static struct tap4_l4_t {
>>>>>    	struct in_addr saddr;
>>>>>    	struct in_addr daddr;
>>>>> +	uint8_t ttl;
>>>>
>>>> If you move this after 'protocol' you save 4 or 8 bytes depending on
>>>> the architecture and, perhaps more importantly, with 64-byte cachelines,
>>>> you can fit the set of fields involved in the L4_MATCH() comparison
>>>> four times instead of three. If you have a look with pahole(1):
>>>>    
>>> Good point. I  didn't notice.
>>>
>>>
>>> [...]
>>>>>    	const struct flowside *toside;
>>>>>    	struct mmsghdr mm[UIO_MAXIOV];
>>>>> @@ -938,6 +940,19 @@ 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 != uflow->ttl[tosidx.sidei]) {
>>>>> +			uflow->ttl[tosidx.sidei] = ttl;
>>>>> +			if (af == AF_INET) {
>>>>> +				if (setsockopt(s, IPPROTO_IP, IP_TTL,
>>>>> +					       &ttl, sizeof(ttl)) < 0)
>>>>> +					perror("setsockopt (IP_TTL)");
>>>>
>>>> This would print to file descriptor 2 even if it's a socket. It should
>>>> be err_perror() instead, but now we also have flow_perror() which
>>>> prints flow index and type, given 'uflow' here, say:
>>>>
>>>> 					flow_perror(uflow, "IP_TTL setsockopt");
>>>>    
>>>>> +			} else {
>>>>> +				if (setsockopt(s, IPPROTO_IPV6, IPV6_HOPLIMIT,
>>>>> +					       &ttl, sizeof(ttl)) < 0)
>>>>> +					perror("setsockopt (IP_TTL)");
>>>>
>>>> ...and this is IPV6_HOPLIMIT, not IP_TTL, so perhaps:
>>>>
>>>> 					flow_perror(uflow,
>>>> 						    "setsockopt IPV6_HOPLIMIT");
>>>>    
>>> Ok.
>>>    
>>>>> +			}
>>>>> +		}
>>>>> +
>>>>>    		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,
>>>>
>>>> Excess whitespace beetween 'uint8_t' and 'ttl'.
>>>>    
>>>>> +		    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);
>>>>> diff --git a/udp_flow.c b/udp_flow.c
>>>>> index bf4b896..39372c2 100644
>>>>> --- a/udp_flow.c
>>>>> +++ b/udp_flow.c
>>>>> @@ -137,6 +137,7 @@ static flow_sidx_t udp_flow_new(const struct ctx *c, union flow *flow,
>>>>>    	uflow = FLOW_SET_TYPE(flow, FLOW_UDP, udp);
>>>>>    	uflow->ts = now->tv_sec;
>>>>>    	uflow->s[INISIDE] = uflow->s[TGTSIDE] = -1;
>>>>> +	uflow->ttl[INISIDE] = uflow->ttl[TGTSIDE] = DEFAULT_TTL;
>>>>
>>>> By the way, instead of using a default value, what about fetching the
>>>> current value with getsockopt()?
>>>>
>>>> One additional system call per UDP flow doesn't feel like a lot of
>>>> overhead, and we can be sure it's correct, no matter if the user
>>>> configures a different value before or after we start.
>>>>    
>>> This patch fixes UDP messaging tap->socket, and TTL may have any
>>> value in the first arriving packet. Reading it from the socket here only
>>> makes sense when I add the same support in direction socket->tap.
>>> That is my next project.
> 
> Well, wait, the getsockopt() will not tell you the value the socket is
> receiving. It tells you the value that the socket would send, at least
> according to the documentation:

We do have IP_RECVTTL and IP6_RECVHOPLIMIT for that. When this option is 
set, we can catch the TTL on received packets by reading anillary data.

///jon

> 
>         IP_TTL (since Linux 1.0)
>                Set or retrieve the current time-to-live field that is

>