From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=none (p=none dis=none) header.from=gibson.dropbear.id.au Authentication-Results: passt.top; dkim=pass (2048-bit key; secure) header.d=gibson.dropbear.id.au header.i=@gibson.dropbear.id.au header.a=rsa-sha256 header.s=202606 header.b=piekbJE3; dkim-atps=neutral Received: from mail.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id A2C8D5A0262 for ; Tue, 09 Jun 2026 03:55:03 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202606; t=1780970100; bh=Pe5Yf77+V+bzhaPvR00IKlRwdQjOOPwixhctHfJcLtU=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=piekbJE3IIMgEmqNZVlw0+jw5TMqJFmtdRUsbcmZuuaKa40MINbC60fpxHoc9AYyX Mosbn5BfADjHdtDH0JYzNT032R0+if2bv/12CPBcNfh+VYCBMnyMvxMSiPwDFJXsVv UnM0ZLTGdv443JZzNRsIFQWPoH52m9lyq2Qo0tfBkKTkxd18ouNmdxNfwRWPS+EmCg m/mV4OnkXVu63yk0MaZEfD4sLH9WapxRFPllk2A6/NbI5Fq7HLTfek5ZjlrEs0Qqr5 wJhwmFSSOBSPWtxOruXyN2Ep5HbSJiV785I7E6ZXerhr6n1ZUk8Qss0KFHGSMOOVZP CaWExjs8EPGRw== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4gZBn013WDz4wLm; Tue, 09 Jun 2026 11:55:00 +1000 (AEST) Date: Tue, 9 Jun 2026 11:54:55 +1000 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH] tap: don't let overheard traffic move addr_seen when serving a fixed address Message-ID: References: <20260605135741.60d87874@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="9Qbv+YeLY4J0j16I" Content-Disposition: inline In-Reply-To: <20260605135741.60d87874@elisabeth> Message-ID-Hash: 54TNI5SUFBZX4BSTZ4YYXSPAT24LCTC5 X-Message-ID-Hash: 54TNI5SUFBZX4BSTZ4YYXSPAT24LCTC5 X-MailFrom: dgibson@gandalf.ozlabs.org X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: EJ Campbell , ej@campbell.name, passt-dev@passt.top, Jon Maloy X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: --9Qbv+YeLY4J0j16I Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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. >=20 > Some remarks: >=20 > On Tue, 2 Jun 2026 18:15:24 -0700 > EJ Campbell wrote: >=20 > > 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. > >=20 > > 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 oth= er > > hosts -- pasta also sees frames from those other hosts. >=20 > 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. > >=20 > > I hit this running pasta with its tap enslaved to a Linux bridge that a= lso > > carries other traffic: inbound port forwarding to the guest broke as so= on > > as another address was seen on the segment. >=20 > 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: >=20 > > Restrict the update so that, when DHCP is disabled (no_dhcp), only fram= es > > 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. >=20 > ...inferring possibly but not necessarily related information from > whether DHCP is disabled or enabled looks a bit risky to me: >=20 > - 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 >=20 > - 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. >=20 > 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. >=20 > For context, Jon is working on a series adding support for multiple > addresses (assigned, used, or observed): >=20 > https://archives.passt.top/passt-dev/20260413005319.3295910-1-jmaloy@re= dhat.com/ >=20 > and patch 8/13: >=20 > https://archives.passt.top/passt-dev/20260413005319.3295910-9-jmaloy@re= dhat.com/ >=20 > refactors the same exact part your patch is touching, here: >=20 > https://archives.passt.top/passt-dev/7f04e12/s/?b=3Dtap.c#n764 >=20 > with no functional change. >=20 > By the way, if you want to try things out on top of that series, you > can apply it with: >=20 > b4 shazam > https://archives.passt.top/passt-dev/20260413005319.3295910-1-jmaloy@re= dhat.com/ >=20 > ...you might want to check out a slightly older revision to apply > cleanly without conflicts. >=20 > The main purpose of this work is actually to implement a netlink > monitor to track changes in host addresses: >=20 > https://bugs.passt.top/show_bug.cgi?id=3D141 > https://pad.passt.top/p/netlinkMonitor >=20 > ...but it might also be convenient for this kind of tracking. For > guests, we could establish some preference rules, for example: >=20 > - 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 >=20 > - if we haven't seen any DHCP / DHCPv6 request, but we have seen a > single address, default to that one as "guest" address >=20 > and so on. >=20 > 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). >=20 > Thoughts? Jon? >=20 > If this is all too complicated for the moment, would a condition based > on explicit -a / --address option work for you? >=20 > > 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. >=20 > I think we should definitely take care of IPv6 as well, I'm just not > sure how. >=20 > A couple of comments about the patch itself: >=20 > > Signed-off-by: EJ Campbell > > --- > > tap.c | 14 +++++++++++++- > > 1 file changed, 13 insertions(+), 1 deletion(-) > >=20 > > diff --git a/tap.c b/tap.c > > index 4cba4c7..514379e 100644 > > --- a/tap.c > > +++ b/tap.c > > @@ -770,7 +770,19 @@ resume: > > continue; >=20 > It looks like the patch itself was copied and pasted from a terminal, > because all tabs became spaces, and the patch doesn't apply. >=20 > 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. >=20 > > } > >=20 > > - if (iph->saddr && c->ip4.addr_seen.s_addr !=3D iph->saddr) > > + /* The latest source address seen on the tap is taken to = be the > > + * guest's, and becomes the target for inbound forwarding= =2E On a > > + * point-to-point tap that's always correct. But if the t= ap sits > > + * on a shared L2 segment (for example bridged together w= ith > > + * other hosts) and we serve a fixed address (DHCP disabl= ed), a > > + * frame overheard from another host on that segment must= not > > + * move addr_seen and silently retarget forwarded connect= ions > > + * away from the guest. When DHCP is disabled, only the > > + * configured guest address may update addr_seen; with DH= CP > > + * enabled (the default) behaviour is unchanged. >=20 > 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. >=20 > Other than that it's pretty well described, thanks. Let's see if we can > find a better solution though. >=20 > > + */ > > + if (iph->saddr && c->ip4.addr_seen.s_addr !=3D iph->saddr= && > > + (!c->no_dhcp || iph->saddr =3D=3D c->ip4.addr.s_addr)) > > c->ip4.addr_seen.s_addr =3D iph->saddr; > >=20 > > if (!iov_drop_header(&data, hlen)) > >=20 > > base-commit: 4b2823784aab04a70dfc295b16fd6f0592955790 > > -- > > 2.53.0 > >=20 >=20 > --=20 > Stefano >=20 >=20 --=20 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 --9Qbv+YeLY4J0j16I Content-Type: application/pgp-signature; name=signature.asc -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmoncm4ACgkQzQJF27ox 2GcTgQ//ZNd1ezjNVzA3EkKGlczL3QmjksWqG2vSEmaSUIEjJ72tSO/3y+pZn7mh yIaFYlnIlwoubbaMMmFXZFyd0k8PMIa6LyFEsgeldRxf4RxXh8T1gr6vgFIIiOiq Eo6VmPVmosmWAgNteymRWr5jDeuKXF1MqsaRNSIVwIQBVKInsSveLEfGu5o+eE3w sL7DS/vGfUF77A3dEd7+dIMR4yDeVktnMWRASfeHEWwzjOCx5VdTmZQSGKtsut5z h8S43fhbE9Rh7kiGtyKKlXTFBL/Eyl7nXRb73W9h2l2E9apRMmlxP15f7STLvHQn ZmxC67RCNohlOyR5hDvnmHqiD/S27M+jrh+LDdUhbD5IisTylX0yYvJSG+88UCPA 660kN7BmVcPGKT0qLT2s71tSTLThPUUCadV/eqi8Y3YNIlzzvHUGeGtZffwtRzoh L2xDEZeJ5PtUCsyifnairP1rCak3LHdbOhmY5UnboBoymCPTzNY8fcirv+u7EKXH 51Ulcsnbni3t0HWPRuk3h7sUAWh8h8kdtxihS0kCB21oHabJ6fETQZaObMnPOcFg uRtT24/4yfrIWZGVa7JILPApsoplSf2D+bSy1H27yRgAZuVbp2R/LvLWsHTQdcme 06qHzp+9tl8QSzkma3S3wER0yQNzmZTaemvm8jbsQ9pBcxv6H2w= =KmcK -----END PGP SIGNATURE----- --9Qbv+YeLY4J0j16I--