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