Re: [PATCH 2/2] fwd: Direct inbound spliced forwards to the guest's external address

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

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

On Wed, Sep 25, 2024 at 10:29:44AM +0200, Stefano Brivio wrote:
> On Wed, 25 Sep 2024 16:54:36 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > In pasta mode, where addressing permits we "splice" connections, forwarding
> > directly from host socket to guest/container socket without any L2 or L3
> > processing.  This gives us a very large performance improvement when it's
> > possible.
> > 
> > Since the traffic is from a local socket within the guest, it will go over
> > the guest's 'lo' interface, and accordingly we set the guest side address
> > to be the loopback address.  However this has a surprising side effect:
> > sometimes guests will run services that are only supposed to be used within
> > the guest and are therefore bound to only 127.0.0.1 and/or ::1.  pasta's
> > forwarding exposes those services to the host, which isn't generally what
> > we want.
> > 
> > Correct this by instead forwarding inbound "splice" flows to the guest's
> > external address.
> > 
> > Link: https://github.com/containers/podman/issues/24045
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  fwd.c | 24 +++++++++++++++---------
> >  1 file changed, 15 insertions(+), 9 deletions(-)
> > 
> > diff --git a/fwd.c b/fwd.c
> > index a505098..d5149db 100644
> > --- a/fwd.c
> > +++ b/fwd.c
> > @@ -447,20 +447,26 @@ uint8_t fwd_nat_from_host(const struct ctx *c, uint8_t proto,
> >  	    (proto == IPPROTO_TCP || proto == IPPROTO_UDP)) {
> >  		/* spliceable */
> >  
> > -		/* Preserve the specific loopback adddress used, but let the
> > -		 * kernel pick a source port on the target side
> > +		/* The traffic will go over the guest's 'lo' interface, but use
> > +		 * its external address, so we don't inadvertendly expose
> 
> inadvertently

Oops, fixed.

> > +		 * services that listen only on the guest's loopback address.
> > +		 *
> > +		 * Let the kernel pick our address on PIF_SPLICE
> >  		 */
> > -		tgt->oaddr = ini->eaddr;
> > +		if (inany_v4(&ini->eaddr)) {
> > +			tgt->eaddr = inany_from_v4(c->ip4.addr_seen);
> > +			tgt->oaddr = inany_any4;
> > +		} else {
> > +			tgt->eaddr.a6 = c->ip6.addr_seen;
> > +			tgt->oaddr = inany_any6;
> > +		}
> > +
> > +		/* Let the kernel pick port */
> >  		tgt->oport = 0;
> >  		if (proto == IPPROTO_UDP)
> > -			/* But for UDP preserve the source port */
> > +			/* Except for UDP, preserve the source port */
> 
> It means the same thing, but it's less clear now: does it mean "Except
> for UDP: in that case preserve the source port" or "Except for UDP: in
> all other cases preserve the source port"?

Good point.  I was trying to make it a follow on from the earlier "Let
the kernel pick port" comment, but you're right the old version was
still clearer.  Reverted.

> I would keep the original version unless I'm missing something subtle
> that this patch would change regarding ports.
> 
> >  			tgt->oport = ini->eport;
> >  
> > -		if (inany_v4(&ini->eaddr))
> > -			tgt->eaddr = inany_loopback4;
> > -		else
> > -			tgt->eaddr = inany_loopback6;
> > -
> >  		return PIF_SPLICE;
> >  	}
> >  
> 
> The series looks good to me (yes, I know I still have to review one
> pending series from you), except for the concern I mentioned as a
> comment to the cover letter.
> 

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]