Re: [PATCH v3 12/15] icmp: Populate and use host side flow information

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

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

On Thu, Jan 18, 2024 at 04:43:05PM +0100, Stefano Brivio wrote:
> On Thu, 18 Jan 2024 12:22:29 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Wed, Jan 17, 2024 at 08:59:41PM +0100, Stefano Brivio wrote:
> > > On Thu, 21 Dec 2023 18:02:34 +1100
> > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > >   
> > > > Complete filling out the common flow information for "ping" flows by
> > > > storing the host side information for the ping socket.  We can only
> > > > retrieve this from the socket after we send the echo-request, because
> > > > that's when the kernel will assign an ID.
> > > > 
> > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > > ---
> > > >  icmp.c | 36 +++++++++++++++++++++++++++++++++---
> > > >  1 file changed, 33 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/icmp.c b/icmp.c
> > > > index 53ad087..917c810 100644
> > > > --- a/icmp.c
> > > > +++ b/icmp.c
> > > > @@ -310,11 +310,41 @@ int icmp_tap_handler(const struct ctx *c, uint8_t pif, int af,
> > > >  	if (sendto(pingf->sock, pkt, plen, MSG_NOSIGNAL, &sa.sa, sl) < 0) {
> > > >  		debug("%s: failed to relay request to socket: %s",
> > > >  		      pname, strerror(errno));
> > > > -	} else {
> > > > -		debug("%s: echo request to socket, ID: %"PRIu16", seq: %"PRIu16,
> > > > -		      pname, id, seq);
> > > > +		if (flow)
> > > > +			goto cancel;
> > > > +	}
> > > > +
> > > > +	debug("%s: echo request to socket, ID: %"PRIu16", seq: %"PRIu16,
> > > > +	      pname, id, seq);
> > > > +
> > > > +	if (!flow)
> > > > +		/* Nothing more to do for an existing flow */
> > > > +		return 1;
> > > > +
> > > > +	/* We need to wait until after the sendto() to fill in the SOCKSIDE
> > > > +	 * information, so that we can find out the host side id the kernel
> > > > +	 * assigned.  If there's no bind address specified, this will still have
> > > > +	 * 0.0.0.0 or :: as the host side forwarding address.  There's not
> > > > +	 * really anything we can do to fill that in, which means we can never
> > > > +	 * insert the SOCKSIDE of a ping flow into the hash table.
> > > > +	 */  
> > > 
> > > Well, if it was just because of that we could argue at some point that,
> > > with wildcards or suchlike, we could insert that in the hash table as
> > > well.  
> > 
> > If we allow wildcards, it's probably not a hash table any more, or at
> > least not a normal one.  Hashing is pretty inherently tied to a single
> > value lookup.
> 
> Well, unless you consider 0.0.0.0 / :: as the actual value (which makes
> it a wildcard, but not in the hash table sense). I'm not suggesting we
> should, though (at least for the moment being).

Right, ok.

So, I've been looking at what we need to do flow based NAT, and I am
now leaning towards revising the existing series so that it's less
insistent on "complete" flowside information, except in the case where
we really are hashing.  That should also let us remove many of the
getsockname() calls.

> > > But that wouldn't make sense anyway, right? That is, unless I'm missing
> > > something, we would have no use for a hash table lookup of the SOCKSIDE
> > > of a ping flow anyway.  
> > 
> > That's correct, which is why we can get away with this.  Likewise
> > SOCKSIDE of tcp flows is also never hashed.  However in some cases we
> > may want to hash the host sides of flows: I think we'll want to for
> > UDP, for example, because we can receive multiple flows via the same
> > bound socket.  That's why I want to be very clear about when and why
> > we're constructing a flowside that can't be hashed.
> 
> The comment makes this sound like a compromise on functionality, or a
> problem we have (and that's what mostly caught my attention: can we
> "solve" this?).
> 
> Maybe you could add something like "(not that there's any use for it)"
> at the end, or similar.

Right, the revision I'm thinking of should make that clearer, I hope.

-- 
David Gibson			| 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 --]