Re: [PATCH v5 19/19] flow, icmp: Use general flow forwarding rules for ICMP

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

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

On Sat, May 18, 2024 at 12:14:08AM +0200, Stefano Brivio wrote:
> On Tue, 14 May 2024 11:03:37 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > Current ICMP hard codes its forwarding rules, and never applies any
> > translations.  Change it to use the flow_forward() function, so that
> > it's translated the same as TCP (excluding TCP specific port
> > redirection).
> > 
> > This means that gw mapping now applies to ICMP so "ping <gw address>" will
> > now ping the host's loopback instead of the actual gw machine.  This
> > removes the surprising behaviour that the target you ping might not be the
> > same as you connect to with TCP.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  flow.c |  1 +
> >  icmp.c | 14 ++++++++++++--
> >  2 files changed, 13 insertions(+), 2 deletions(-)
> > 
> > diff --git a/flow.c b/flow.c
> > index a6afe39..b43a079 100644
> > --- a/flow.c
> > +++ b/flow.c
> > @@ -285,6 +285,7 @@ const struct flowside *flow_initiate_sa(union flow *flow, uint8_t pif,
> >   *
> >   * Return: pointer to the forwarded flowside information
> >   */
> > +/* cppcheck-suppress unusedFunction */
> >  const struct flowside *flow_forward_af(union flow *flow, uint8_t pif,
> >  				       sa_family_t af,
> >  				       const void *saddr, in_port_t sport,
> > diff --git a/icmp.c b/icmp.c
> > index 0112fd9..6310178 100644
> > --- a/icmp.c
> > +++ b/icmp.c
> > @@ -153,6 +153,7 @@ static struct icmp_ping_flow *icmp_ping_new(const struct ctx *c,
> >  					    sa_family_t af, uint16_t id,
> >  					    const void *saddr, const void *daddr)
> >  {
> > +	uint8_t proto = af == AF_INET ? IPPROTO_ICMP : IPPROTO_ICMPV6;
> >  	uint8_t flowtype = af == AF_INET ? FLOW_PING4 : FLOW_PING6;
> >  	union epoll_ref ref = { .type = EPOLL_TYPE_PING };
> >  	union flow *flow = flow_alloc();
> > @@ -163,9 +164,18 @@ static struct icmp_ping_flow *icmp_ping_new(const struct ctx *c,
> >  	if (!flow)
> >  		return NULL;
> >  
> > -
> >  	flow_initiate_af(flow, PIF_TAP, af, saddr, id, daddr, id);
> > -	flow_forward_af(flow, PIF_HOST,	af, NULL, 0, daddr, 0);
> > +	if (!flow_forward(c, flow, proto))
> > +		goto cancel;
> > +
> > +	if (flow->f.pif[FWDSIDE] != PIF_HOST) {
> > +		flow_err(flow, "No support for forwarding %s from %s to %s",
> > +			 proto == IPPROTO_ICMP ? "ICMP" : "ICMPv6",
> 
> Which brings me to two remarks:
> 
> - having the protocol name also in the flow_err() message printed in
>   flow_forward() could be helpful

It would, and I've thought about it, but haven't seen a great way to
go about it.

 * The flow type is not set at this point, so we can't use that.  We
   can't trivially move setting the type earlier, because for TCP at
   least we need the information from flow_froward() to determine if
   we're spliced and set the type based on that.

 * Including both flow type and protocol in flow_common is annoyingly
   redundant, as well as adding a full 32-bits to the structure
   because of padding.

 * We could possibly eliminate flow type and make it implicit based on
   the protocol+pifs: regular TCP flow is (TCP, HOST, TAP) or (TCP,
   TAP, HOST), whereas TCP spliced is (TCP, HOST, SPLICE) or (TCP,
   SPLICE, HOST).  Type is the selector field for which of the union
   variants is valid, and I don't love that being something with a
   kind of complicated calculation behind it.

 * We could make the type field hold the protocol until
   FLOW_SET_TYPE(), but I don't love semantics of a field changing
   like that.

 * We could just pass either a protocol number or a string to
   flow_forward() etc., but that seems a bit awkward.

Hrm... actually thinking on that last one.  It might make sense to add
a descriptive string to flow_initiate(), not just the protocol but
something like "TCP SYN" versus "TCP accept()" or the like.  That
wouldn't directly help flow_forward(), but the info line from
flow_initiate() is likely to be in close proximity, so it would help.


> - then, perhaps, we should re-introduce ip_proto_str[] which was dropped
>   with 340164445341 ("epoll: Generalize epoll_ref to cover things other
>   than sockets")

Maybe.  I guess the standard way to do that is with getprotobyname(3),
but that probably won't work in our isolated namespace, I guess.

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