On Wed, 25 Sep 2024 16:54:36 +1000 David Gibson <david(a)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(a)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 exposeinadvertently+ * 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