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

[Stefano Brivio <sbrivio@redhat.com>]

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

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

-- 
Stefano