On Fri, Apr 04, 2025 at 01:01:45PM +0200, Stefano Brivio wrote:
> On Fri, 4 Apr 2025 11:57:58 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Fri, Apr 04, 2025 at 10:40:27AM +1100, David Gibson wrote:
> > > On Thu, Apr 03, 2025 at 06:27:06PM -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  in this commit.
> > > > 
> > > > Link: https://bugs.passt.top/show_bug.cgi?id=64
> > > > Signed-off-by: Jon Maloy <jmaloy@redhat.com>  
> > > 
> > > Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> > > 
> > > One commont below.  
> > 
> > Oh.. wait.  I just realised this patch has a weird side effect, for
> > flows which are initiated from outside, rather than from the guest.
> > 
> > If the flow is initiated from outside, it's maybe a bit unlikely, but
> > we could still get a non-default TTL from the guest on reply
> > datagrams.  That will trigger this code and alter the socket's TTL.
> > But in this case the socket is not a flow specific socket, but the
> > listening socket which initiated the flow, which could be handling
> > packets on many flows.
> > 
> > The "cached" TTL is stored per-flow, not per-socket, so we won't look
> > at the right value when we process datagrams from other flows, so they
> > may go out with the wrong TTL.
> > 
> > I think the obvious way to address this is to stop using the
> > "listening" socket to send datagrams for a flow, using a connect()ed
> > socket instead.  We have other reasons to do that too, and I'm working
> > on implementing that right now.
> > 
> > The question is whether this is a serious enough problem to delay this
> > until the "both sides connect()ed sockets" change is merged.
> 
> On the other hand, this patch applies cleanly on top of your "Use
> connect()ed sockets for both sides of UDP flows" series, and also
> semantically I couldn't spot any particular change or integration
> that we would need as a result in this patch.

Ah.. yeah.  When I wrote the above I didn't think I'd finish that
series today :).

> I haven't reviewed the series yet, but I don't think that an eventual
> re-spin would change this, so... problem solved I guess? I can just
> wait and apply them together.

Yeah, I guess so.  Hooray!

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