Re: [PATCH] tap: don't let overheard traffic move addr_seen when serving a fixed address

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

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

On Fri, Jun 05, 2026 at 01:57:41PM +0200, Stefano Brivio wrote:
> EJ, thanks for the detailed patch and commit message, and apologies for
> the delayed response.
> 
> Some remarks:
> 
> On Tue, 2 Jun 2026 18:15:24 -0700
> EJ Campbell <ej.campbell@gmail.com> wrote:
> 
> > The latest source address seen on the tap interface (ip4.addr_seen) is
> > taken to be the guest's and is used as the target for inbound port
> > forwarding. The update accepts any source address.
> > 
> > On a point-to-point tap that is always correct. But when the tap sits on
> > a shared L2 segment -- for example when it is bridged together with other
> > hosts -- pasta also sees frames from those other hosts.
> 
> That's a interesting use case, it's the first time I see pasta used
> like this. Thanks for sharing.

Likewise, it's the first I've heard of someone using it specifically
like this, but it is a use model I've had in mind for a while.  So
this is a known limitation I've been thinking about for a while.  It
hasn't reached the top of my priority list because while your very
specific case almost works already, the more general case of a bridge
on the guest side requires a bunch more infrastructure.  That's in
progress or at least planned, but it's going to be a while.

Fwiw, I really dislike the addr_seen mechanism even in cases where it
"works": it allows poorly configured guests to work at the expense of
substantial complexity which impacts all cases.  However, it's
established behaviour, so Stefano is understandably against simply
removing it.

> > With a static
> > configuration (-a / address set, DHCP disabled), an overheard frame from
> > another host silently moves addr_seen to that host, and every subsequent
> > inbound forwarded connection is sent there instead of to the guest, so
> > connections are reset or hang.
> > 
> > I hit this running pasta with its tap enslaved to a Linux bridge that also
> > carries other traffic: inbound port forwarding to the guest broke as soon
> > as another address was seen on the segment.
> 
> Oops. We should definitely fix this, and maybe your patch is good enough
> for the moment (or better than the alternative), but I have a concern
> here:
> 
> > Restrict the update so that, when DHCP is disabled (no_dhcp), only frames
> > sourced from the configured guest address may move addr_seen. With DHCP
> > enabled -- the default -- behaviour is unchanged, since the guest's
> > address is whatever pasta leases and tracking it is still correct.
> 
> ...inferring possibly but not necessarily related information from
> whether DHCP is disabled or enabled looks a bit risky to me:
> 
> - somebody might have DHCP enabled (just because it's the default
>   option), but one single connected container / guest not using DHCP at
>   all, and picking an address that we didn't expect. Your patch would
>   break this (perhaps unlikely?) usage
> 
> - somebody might have DHCP enabled but just the intended container /
>   guest using it, and your same problem. Your patch wouldn't fix that
>   (definitely less likely) usage

Right.  I agree with Stefano that basing this of --no-dhcp is not a
good idea.  It happens to work for your case, but it doesn't
generalise well.  In addition to the cases Stefano mentions, for the
more general case of a guest-side bridge, we'd eventually like to have
our DHCP implementation serve multiple address, in which case we
wouldn't want addr_seen, even though DHCP is enabled.

> So I've been thinking: shouldn't we rather use as a condition the fact
> that -a / --address was explicitly given? We don't track that from
> conf() at the moment but it would be a small change.
> 
> If -a / --address is given, I'd say the user is very much expecting the
> container / guest to be using that address, and in that case I think
> it's safer to ignore other addresses.

I think having an explcit -a is a sufficient condition to disable
addr_seen, but maybe not a necessary one.  I think requiring the guest
to use the assigned address would be a reasonable request, even if
that address is taken from the host.

Stefano and others talked me out of an explicit --no-address-snooping
option, on the grounds of option proliferation at our last call.
However, thinking about it again, I'm still wondering if it might be
useful: we can think of addr_seen as a (not very good) address
assignment mechanism, along with DHCP, NDP, DHCPv6 and --config-net.
All of those (better) mechanisms can be explicitly disabled, so why
should we not be able to disable the worst one.

> It would be even better if we could track our designated container /
> guest among a number of them, only if there are more than one.
> 
> For context, Jon is working on a series adding support for multiple
> addresses (assigned, used, or observed):
> 
>   https://archives.passt.top/passt-dev/20260413005319.3295910-1-jmaloy@redhat.com/
> 
> and patch 8/13:
> 
>   https://archives.passt.top/passt-dev/20260413005319.3295910-9-jmaloy@redhat.com/
> 
> refactors the same exact part your patch is touching, here:
> 
>   https://archives.passt.top/passt-dev/7f04e12/s/?b=tap.c#n764
> 
> with no functional change.
> 
> By the way, if you want to try things out on top of that series, you
> can apply it with:
> 
>   b4 shazam
>   https://archives.passt.top/passt-dev/20260413005319.3295910-1-jmaloy@redhat.com/
> 
> ...you might want to check out a slightly older revision to apply
> cleanly without conflicts.
> 
> The main purpose of this work is actually to implement a netlink
> monitor to track changes in host addresses:
> 
>   https://bugs.passt.top/show_bug.cgi?id=141
>   https://pad.passt.top/p/netlinkMonitor
> 
> ...but it might also be convenient for this kind of tracking. For
> guests, we could establish some preference rules, for example:
> 
> - if any guest made a DHCP / DHCPv6 request, select its address (I
>   guess we should eventually implement guest-side MAC address tracking
>   for a full solution but just preferring addresses we assigned might
>   be quite good). I'm not sure how we would handle SLAAC
> 
> - if we haven't seen any DHCP / DHCPv6 request, but we have seen a
>   single address, default to that one as "guest" address
> 
> and so on.
> 
> For containers, we actually know which namespace we're associated to,
> so we could, on any new observed address, check the source MAC address
> of the frame and see if it corresponds to our namespace (querying via
> netlink).
> 
> Thoughts? Jon?
> 
> If this is all too complicated for the moment, would a condition based
> on explicit -a / --address option work for you?
> 
> > The same reasoning applies to ip6.addr_seen on a shared segment; I left
> > IPv6 alone for now since I only exercise IPv4 in this setup, and would be
> > glad to extend it the same way if you prefer.
> 
> I think we should definitely take care of IPv6 as well, I'm just not
> sure how.
> 
> A couple of comments about the patch itself:
> 
> > Signed-off-by: EJ Campbell <ej.campbell@gmail.com>
> > ---
> >  tap.c | 14 +++++++++++++-
> >  1 file changed, 13 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tap.c b/tap.c
> > index 4cba4c7..514379e 100644
> > --- a/tap.c
> > +++ b/tap.c
> > @@ -770,7 +770,19 @@ resume:
> >                       continue;
> 
> It looks like the patch itself was copied and pasted from a terminal,
> because all tabs became spaces, and the patch doesn't apply.
> 
> It's not a problem for small patches like this one as I tend to fix
> things up manually, but it would be good to have it directly from git
> send-email instead.
> 
> >               }
> > 
> > -             if (iph->saddr && c->ip4.addr_seen.s_addr != iph->saddr)
> > +             /* The latest source address seen on the tap is taken to be the
> > +              * guest's, and becomes the target for inbound forwarding. On a
> > +              * point-to-point tap that's always correct. But if the tap sits
> > +              * on a shared L2 segment (for example bridged together with
> > +              * other hosts) and we serve a fixed address (DHCP disabled), a
> > +              * frame overheard from another host on that segment must not
> > +              * move addr_seen and silently retarget forwarded connections
> > +              * away from the guest. When DHCP is disabled, only the
> > +              * configured guest address may update addr_seen; with DHCP
> > +              * enabled (the default) behaviour is unchanged.
> 
> The fact that the behaviour is _unchanged_ (by your patch) belongs in
> the commit message rather than the code, because if one reads the code
> they aren't explicitly reading your change and it's not clear to what
> previous behaviour you would be referring to.
> 
> Other than that it's pretty well described, thanks. Let's see if we can
> find a better solution though.
> 
> > +              */
> > +             if (iph->saddr && c->ip4.addr_seen.s_addr != iph->saddr &&
> > +                 (!c->no_dhcp || iph->saddr == c->ip4.addr.s_addr))
> >                       c->ip4.addr_seen.s_addr = iph->saddr;
> > 
> >               if (!iov_drop_header(&data, hlen))
> > 
> > base-commit: 4b2823784aab04a70dfc295b16fd6f0592955790
> > --
> > 2.53.0
> > 
> 
> -- 
> Stefano
> 
> 

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