* Re: [PATCH] tap: don't let overheard traffic move addr_seen when serving a fixed address
2026-06-03 1:15 [PATCH] tap: don't let overheard traffic move addr_seen when serving a fixed address EJ Campbell
@ 2026-06-05 11:57 ` Stefano Brivio
0 siblings, 0 replies; 2+ messages in thread
From: Stefano Brivio @ 2026-06-05 11:57 UTC (permalink / raw)
To: EJ Campbell; +Cc: ej, passt-dev, Jon Maloy, David Gibson
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.
> 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
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.
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
^ permalink raw reply [flat|nested] 2+ messages in thread