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

[Stefano Brivio <sbrivio@redhat.com>]

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

> +		 * 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"?

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.

-- 
Stefano