Re: [PATCH v5 07/13] ip: Track observed guest IPv4 addresses in unified address array

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

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

On Sun, Feb 22, 2026 at 12:44:39PM -0500, Jon Maloy wrote:
> We remove the addr_seen field in struct ip4_ctx and replace it by
> setting a new CONF_ADDR_OBSERVED flag in the corresponding entry
> in the unified address array.
> 
> The observed IPv4 address is always added at or moved to position 0,
> increasing chances for a fast lookup.
> 
> Signed-off-by: Jon Maloy <jmaloy@redhat.com>
> 
> ---
> v4: - Removed migration protocol update, to be added in later commit
>     - Allow only one OBSERVED address at a time
>     - Some other changes based on feedback from David G
> v5: - Allowing multiple observed IPv4 addresses

As I said before v4, with no mechanism to *remove* observed addresses,
this seems like a bad idea to me.  Given that the guest using an
address we didn't assign to it is already kind of weird, I don't see a
lot of reason to assume it will always pick the same address.  If
e.g. a passt guest picked a different address on each reboot, we could
chew up our entire table with stale observed addresses on a long lived
VM.

> ---
>  conf.c    |   2 -
>  conf.h    |   1 +
>  fwd.c     | 223 +++++++++++++++++++++++++++++++++++++++++++++++-------
>  fwd.h     |   9 +++
>  inany.h   |   3 +
>  migrate.c |  17 ++++-
>  passt.h   |   2 -
>  tap.c     |  17 ++++-
>  8 files changed, 239 insertions(+), 35 deletions(-)
> 
> diff --git a/conf.c b/conf.c
> index 35ef994..faa60ec 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -747,7 +747,6 @@ static unsigned int conf_ip4(struct ctx *c, unsigned int ifi)
>  		e->addr = inany_from_v4(addr);
>  		e->prefix_len = prefix_len + 96;
>  		e->flags = CONF_ADDR_HOST;
> -		ip4->addr_seen = addr;
>  	}
>  
>  	ip4->our_tap_addr = ip4->guest_gw;
> @@ -764,7 +763,6 @@ static void conf_ip4_local(struct ctx *c)
>  	struct inany_addr_entry *e = &c->addrs[c->addr_count++];
>  	struct ip4_ctx *ip4 = &c->ip4;
>  
> -	ip4->addr_seen = IP4_LL_GUEST_ADDR;
>  	ip4->our_tap_addr = ip4->guest_gw = IP4_LL_GUEST_GW;
>  	ip4->no_copy_addrs = ip4->no_copy_routes = true;
>  	e->addr = inany_from_v4(IP4_LL_GUEST_ADDR);
> diff --git a/conf.h b/conf.h
> index bfad36f..8b10ac6 100644
> --- a/conf.h
> +++ b/conf.h
> @@ -12,6 +12,7 @@
>  #define CONF_ADDR_USER		BIT(0)		/* User set via -a */
>  #define CONF_ADDR_HOST		BIT(1)		/* From host interface */
>  #define CONF_ADDR_LINKLOCAL	BIT(2)		/* Link-local address */
> +#define CONF_ADDR_OBSERVED	BIT(3)		/* Seen in guest traffic */
>  
>  enum passt_modes conf_mode(int argc, char *argv[]);
>  void conf(struct ctx *c, int argc, char **argv);
> diff --git a/fwd.c b/fwd.c
> index edf6a6b..9141e37 100644
> --- a/fwd.c
> +++ b/fwd.c
> @@ -28,6 +28,7 @@
>  #include "inany.h"
>  #include "fwd.h"
>  #include "passt.h"
> +#include "conf.h"
>  #include "lineread.h"
>  #include "flow_table.h"
>  #include "netlink.h"
> @@ -879,6 +880,159 @@ static bool is_dns_flow(uint8_t proto, const struct flowside *ini)
>  		((ini->oport == 53) || (ini->oport == 853));
>  }
>  
> +/**
> + * fwd_get_addr() - Get guest address entry matching criteria
> + * @c:		Execution context
> + * @af:		Address family (AF_INET, AF_INET6, or 0 for any)
> + * @incl:	Flags that must be present (any-match)
> + * @excl:	Flags that must not be present
> + *
> + * Return: first address entry matching criteria, or NULL
> + */
> +const struct inany_addr_entry *fwd_get_addr(const struct ctx *c,
> +					      sa_family_t af,
> +					      uint8_t incl, uint8_t excl)
> +{
> +	const struct inany_addr_entry *e;
> +
> +	for_each_addr(e, c, af) {
> +		if (incl && !(e->flags & incl))
> +			continue;
> +		if (e->flags & excl)
> +			continue;
> +		return e;
> +	}
> +
> +	return NULL;
> +}
> +
> +/**
> + * fwd_select_addr() - Select address with priority-based search
> + * @c:		Execution context
> + * @af:		Address family (AF_INET or AF_INET6)
> + * @first:	First priority flags to match (or 0 to skip)
> + * @second:	Second priority flags to match (or 0 to skip)
> + * @third:	Third priority flags to match (or 0 to skip)

This structuring seems kind of clunky and arbitrary, but I guess I'll
see how it's used.

> + * @skip:	Flags to exclude from search
> + *
> + * Search for address entries in priority order.
> + *
> + * Return: pointer to selected address entry, or NULL if none found
> + */
> +struct inany_addr_entry *fwd_select_addr(const struct ctx *c, int af,
> +					 int first, int second, int third,
> +					 int skip)
> +{
> +	const struct inany_addr_entry *e;
> +
> +	if (first) {
> +		e = fwd_get_addr(c, af, first, skip);
> +		if (e)
> +			return (struct inany_addr_entry *)e;
> +	}
> +
> +	if (second) {
> +		e = fwd_get_addr(c, af, second, skip);
> +		if (e)
> +			return (struct inany_addr_entry *)e;
> +	}
> +
> +	if (third) {
> +		e = fwd_get_addr(c, af, third, skip);
> +		if (e)
> +			return (struct inany_addr_entry *)e;
> +	}
> +
> +	return NULL;
> +}
> +
> +/**
> + * fwd_set_addr() - Update address entry, adding one if needed
> + * @c:		Execution context
> + * @addr:	Address to add and/or set flags/prefix on
> + * @flags:	Flags to set
> + * @prefix_len:	Prefix length (0 to leave unchanged, if any)
> + *
> + * If CONF_ADDR_OBSERVED is in @flags, insert at position 0.
> + */
> +void fwd_set_addr(struct ctx *c, const union inany_addr *addr,
> +		  uint8_t flags, int prefix_len)
> +{
> +	struct inany_addr_entry *e;
> +	int i;
> +
> +	for (i = 0; i < c->addr_count; i++) {
> +		e = &c->addrs[i];

Can you use foreach_address?

> +		if (!inany_equals(&e->addr, addr))
> +			continue;
> +
> +		/* Update prefix_len if provided and applicable */
> +		if (prefix_len && !(e->flags & CONF_ADDR_USER))
> +			e->prefix_len = prefix_len;

Hmm.  Does updating prefix length ever make sense, even for non-user
addresses?  I'm not certain, but I think a "change" of prefix length
on the host would only occur via removing the old prefix and adding a
new one with the same address portion.  Or even add then del - on the
kernel side it is possible to have two different prefixes with the
same address at the same time:

2: dummy0: <BROADCAST,NOARP> mtu 1500 qdisc noop state DOWN group default qlen 1000
    link/ether ee:15:24:34:28:a3 brd ff:ff:ff:ff:ff:ff
    inet 10.1.2.3/16 scope global dummy0
       valid_lft forever preferred_lft forever
    inet 10.1.2.3/24 scope global dummy0
       valid_lft forever preferred_lft forever

So, I'm not really sure if we should be treating this as a list of
addresses, where prefix length and flags are attributes, or as a list
of prefixes, where flags are the only attributes.

> +
> +		/* Nothing more to change */
> +		if ((e->flags & flags) == flags)
> +			return;
> +
> +		/* OBSERVED address moves to position 0: remove, re-add later */
> +		if (flags & CONF_ADDR_OBSERVED) {
> +			flags |= e->flags;
> +			prefix_len = e->prefix_len;
> +			memmove(e, e + 1, (c->addr_count - i - 1) * sizeof(*e));
> +			c->addr_count--;
> +			break;
> +		}
> +
> +		e->flags |= flags;
> +		return;
> +	}
> +
> +	if (c->addr_count >= INANY_MAX_ADDRS) {
> +		debug("Address table full, can't add address");
> +		return;
> +	}
> +
> +	if (flags & CONF_ADDR_OBSERVED) {
> +		memmove(&c->addrs[1], &c->addrs[0], c->addr_count * sizeof(*e));

Moving the new entry to slot 0 is done with two memmove()s, one of
which is all of the whole existing table, and the other could be most
of it.  I guess the table isn't that large, but that does seem kind of
expensive.

Does the order of addresses matter?  Could you do this by swapping the
new OBSERVED address with entry 0, leaving the rest of the table in
place?

> +		e = &c->addrs[0];
> +	} else {
> +		e = &c->addrs[c->addr_count];
> +	}
> +	c->addr_count++;
> +	e->addr = *addr;
> +	e->prefix_len = prefix_len;
> +	e->flags = flags;
> +}
> +
> +/**
> + * fwd_remove_addr() - Remove an address from the unified array
> + * @c:		Execution context
> + * @addr:	Address to remove
> + *
> + * User-configured addresses (CONF_ADDR_USER) are not removed.
> + *
> + * Return: true if removed, false if not found or user-configured
> + */
> +bool fwd_remove_addr(struct ctx *c, const union inany_addr *addr)
> +{
> +	int i;
> +
> +	for (i = 0; i < c->addr_count; i++) {
> +		if (!inany_equals(&c->addrs[i].addr, addr))
> +			continue;
> +
> +		if (c->addrs[i].flags & CONF_ADDR_USER)
> +			return false;
> +
> +		c->addr_count--;
> +		memmove(&c->addrs[i], &c->addrs[i + 1],
> +			(c->addr_count - i) * sizeof(c->addrs[0]));

I'm not certain if memmove() is guaranteed to be a safe no-op if given
a zero length, we should double check that.

> +		return true;
> +	}
> +
> +	return false;
> +}
> +
>  /**
>   * fwd_guest_accessible() - Is address guest-accessible
>   * @c:		Execution context
> @@ -903,26 +1057,11 @@ static bool fwd_guest_accessible(const struct ctx *c,
>  	if (inany_is_unspecified4(addr))
>  		return false;
>  
> -	/* Check against all configured guest addresses */
> +	/* Check against all configured and observed guest addresses */

s/configured and observed/known/ ?

>  	for_each_addr(e, c, 0)
>  		if (inany_equals(addr, &e->addr))
>  			return false;
>  
> -	/* Also check addr_seen: it tracks the address the guest is actually
> -	 * using, which may differ from configured addresses.
> -	 */
> -	if (inany_equals4(addr, &c->ip4.addr_seen))
> -		return false;
> -
> -	/* For IPv6, addr_seen starts unspecified, because we don't know what LL
> -	 * address the guest will take until we see it.  Only check against it
> -	 * if it has been set to a real address.
> -	 */
> -	if (!inany_v4(addr) &&
> -	    !IN6_IS_ADDR_UNSPECIFIED(&c->ip6.addr_seen) &&
> -	    inany_equals6(addr, &c->ip6.addr_seen))
> -		return false;

IPv6 addr_seen is not in the array yet, so you can't remove this
stanza until the next patch.

>  	return true;
>  }
>  
> @@ -1099,16 +1238,36 @@ uint8_t fwd_nat_from_host(const struct ctx *c,
>  		 * match.
>  		 */
>  		if (inany_v4(&ini->eaddr)) {
> -			if (c->host_lo_to_ns_lo)
> +			if (c->host_lo_to_ns_lo) {
>  				tgt->eaddr = inany_loopback4;
> -			else
> -				tgt->eaddr = inany_from_v4(c->ip4.addr_seen);
> +			} else {
> +				const struct inany_addr_entry *e;
> +
> +				e = fwd_select_addr(c, AF_INET,
> +						    CONF_ADDR_OBSERVED,
> +						    CONF_ADDR_USER |
> +						    CONF_ADDR_HOST, 0, 0);

Everything has at least one of OBSERVED | USER | HOST, so from
OBSERVED you could fall back immediately to "anything IPv4" - no need
for multiple layers.

> +				if (!e)
> +					return PIF_NONE;
> +
> +				tgt->eaddr = e->addr;
> +			}
>  			tgt->oaddr = inany_any4;
>  		} else {
> -			if (c->host_lo_to_ns_lo)
> +			if (c->host_lo_to_ns_lo) {
>  				tgt->eaddr = inany_loopback6;
> -			else
> -				tgt->eaddr.a6 = c->ip6.addr_seen;
> +			} else {
> +				const struct inany_addr_entry *e;
> +
> +				e = fwd_select_addr(c, AF_INET6,
> +						    CONF_ADDR_OBSERVED,
> +						    CONF_ADDR_USER |
> +						    CONF_ADDR_HOST,
> +						    0, CONF_ADDR_LINKLOCAL);
> +				if (!e)
> +					return PIF_NONE;
> +				tgt->eaddr = e->addr;

Same comment, plus this looks like it belongs in the next patch.

> +			}
>  			tgt->oaddr = inany_any6;
>  		}
>  
> @@ -1137,12 +1296,22 @@ uint8_t fwd_nat_from_host(const struct ctx *c,
>  	tgt->oport = ini->eport;
>  
>  	if (inany_v4(&tgt->oaddr)) {
> -		tgt->eaddr = inany_from_v4(c->ip4.addr_seen);
> +		const struct inany_addr_entry *e;
> +
> +		e = fwd_get_addr(c, AF_INET, CONF_ADDR_OBSERVED, 0);

We need a fall back here as well (in the old code this was implicit,
because we initialized addr_seen to addr).

> +		if (!e)
> +			return PIF_NONE;
> +		tgt->eaddr = e->addr;
>  	} else {
> -		if (inany_is_linklocal6(&tgt->oaddr))
> -			tgt->eaddr.a6 = c->ip6.addr_ll_seen;
> -		else
> -			tgt->eaddr.a6 = c->ip6.addr_seen;
> +		bool linklocal = inany_is_linklocal6(&tgt->oaddr);
> +		int excl = linklocal ? 0 : CONF_ADDR_LINKLOCAL;
> +		const struct inany_addr_entry *e;
> +
> +		e = fwd_select_addr(c, AF_INET6, CONF_ADDR_OBSERVED,
> +				    CONF_ADDR_USER | CONF_ADDR_HOST, 0, excl);

Again, belongs in the next patch.

But more importantly, AFAICT this excludes linklocal addresses when we
want a non-linklocal, but it doesn't exclude non-linklocal addresses
when we want a linklocal.

> +		if (!e)
> +			return PIF_NONE;
> +		tgt->eaddr = e->addr;
>  	}
>  
>  	return PIF_TAP;
> diff --git a/fwd.h b/fwd.h
> index 1607011..698b407 100644
> --- a/fwd.h
> +++ b/fwd.h
> @@ -15,6 +15,15 @@ struct flowside;
>  
>  void fwd_probe_ephemeral(void);
>  bool fwd_port_is_ephemeral(in_port_t port);
> +const struct inany_addr_entry *fwd_get_addr(const struct ctx *c,
> +					      sa_family_t af,
> +					      uint8_t incl, uint8_t excl);
> +struct inany_addr_entry *fwd_select_addr(const struct ctx *c, int af,
> +					 int first, int second, int third,
> +					 int skip);
> +void fwd_set_addr(struct ctx *c, const union inany_addr *addr,
> +		  uint8_t flags, int prefix_len);
> +bool fwd_remove_addr(struct ctx *c, const union inany_addr *addr);
>  
>  /**
>   * struct fwd_rule - Forwarding rule governing a range of ports
> diff --git a/inany.h b/inany.h
> index a6069de..fd913ae 100644
> --- a/inany.h
> +++ b/inany.h
> @@ -54,6 +54,9 @@ extern const union inany_addr inany_any4;
>  #define inany_from_v4(a4)	\
>  	((union inany_addr)INANY_INIT4((a4)))
>  
> +#define inany_from_v6(addr)	\
> +	((union inany_addr){ .a6 = (addr) })

This macro is unused in this patch.

>  /** union sockaddr_inany - Either a sockaddr_in or a sockaddr_in6
>   * @sa_family:	Address family, AF_INET or AF_INET6
>   * @sa:		Plain struct sockaddr (useful to avoid casts)
> diff --git a/migrate.c b/migrate.c
> index 13bacab..f026e95 100644
> --- a/migrate.c
> +++ b/migrate.c
> @@ -18,6 +18,8 @@
>  #include "util.h"
>  #include "ip.h"
>  #include "passt.h"
> +#include "conf.h"
> +#include "fwd.h"
>  #include "inany.h"
>  #include "flow.h"
>  #include "flow_table.h"
> @@ -57,11 +59,16 @@ static int seen_addrs_source_v1(struct ctx *c,
>  	struct migrate_seen_addrs_v1 addrs = {
>  		.addr6 = c->ip6.addr_seen,
>  		.addr6_ll = c->ip6.addr_ll_seen,
> -		.addr4 = c->ip4.addr_seen,
>  	};
> +	const struct inany_addr_entry *e;
>  
>  	(void)stage;
>  
> +	/* IPv4 observed address */
> +	e = fwd_get_addr(c, AF_INET, CONF_ADDR_OBSERVED, 0);
> +	if (e)
> +		addrs.addr4 = *inany_v4(&e->addr);
> +

To match behaviour with the old version this needs to fall back to the
configured address if there is no observed address.

>  	memcpy(addrs.mac, c->guest_mac, sizeof(addrs.mac));
>  
>  	if (write_all_buf(fd, &addrs, sizeof(addrs)))
> @@ -82,6 +89,7 @@ static int seen_addrs_target_v1(struct ctx *c,
>  				const struct migrate_stage *stage, int fd)
>  {
>  	struct migrate_seen_addrs_v1 addrs;
> +	struct in_addr addr4;
>  
>  	(void)stage;
>  
> @@ -90,7 +98,12 @@ static int seen_addrs_target_v1(struct ctx *c,
>  
>  	c->ip6.addr_seen = addrs.addr6;
>  	c->ip6.addr_ll_seen = addrs.addr6_ll;
> -	c->ip4.addr_seen = addrs.addr4;
> +
> +	/* Copy from packed struct to avoid alignment issues */
> +	addr4 = addrs.addr4;

I'm pretty sure if you can assign from it, you can reference it (at
least C-ishly strictly, maybe not always in practice).  If you're
worried about alignment, I think you need an explicit memcpy().

> +	if (addr4.s_addr)
> +		fwd_set_addr(c, &inany_from_v4(addr4), CONF_ADDR_OBSERVED, 0);
> +
>  	memcpy(c->guest_mac, addrs.mac, sizeof(c->guest_mac));
>  
>  	return 0;
> diff --git a/passt.h b/passt.h
> index bb56998..b808a19 100644
> --- a/passt.h
> +++ b/passt.h
> @@ -78,7 +78,6 @@ struct inany_addr_entry {
>  
>  /**
>   * struct ip4_ctx - IPv4 execution context
> - * @addr_seen:		Latest IPv4 address seen as source from tap
>   * @guest_gw:		IPv4 gateway as seen by the guest
>   * @map_host_loopback:	Outbound connections to this address are NATted to the
>   *                      host's 127.0.0.1
> @@ -94,7 +93,6 @@ struct inany_addr_entry {
>   * @no_copy_addrs:	Don't copy all addresses when configuring namespace
>   */
>  struct ip4_ctx {
> -	struct in_addr addr_seen;
>  	struct in_addr guest_gw;
>  	struct in_addr map_host_loopback;
>  	struct in_addr map_guest_addr;
> diff --git a/tap.c b/tap.c
> index 7f79add..30e52f7 100644
> --- a/tap.c
> +++ b/tap.c
> @@ -48,6 +48,7 @@
>  #include "iov.h"
>  #include "passt.h"
>  #include "conf.h"
> +#include "fwd.h"
>  #include "arp.h"
>  #include "dhcp.h"
>  #include "ndp.h"
> @@ -162,6 +163,17 @@ void tap_send_single(const struct ctx *c, const void *data, size_t l2len)
>  	}
>  }
>  
> +/**
> + * tap_check_src_addr4() - Note an IPv4 address seen in guest traffic
> + * @c:		Execution context
> + * @addr:	IPv4 address seen as source from guest
> + */
> +static void tap_check_src_addr4(struct ctx *c, const struct in_addr *addr)
> +{
> +	if (addr->s_addr)

Better to use IN4_IS_ADDR_UNSPECIFIED.

> +		fwd_set_addr(c, &inany_from_v4(*addr), CONF_ADDR_OBSERVED, 0);
> +}
> +
>  /**
>   * tap_ip6_daddr() - Normal IPv6 destination address for inbound packets
>   * @c:		Execution context
> @@ -772,8 +784,9 @@ resume:
>  			continue;
>  		}
>  
> -		if (iph->saddr && c->ip4.addr_seen.s_addr != iph->saddr)
> -			c->ip4.addr_seen.s_addr = iph->saddr;
> +		if (iph->saddr)

Redundant with the test inside tap_check_src_addr4().

> +			tap_check_src_addr4(c,
> +					    (const struct in_addr *)&iph->saddr);
>  
>  		if (!iov_drop_header(&data, hlen))
>  			continue;
> -- 
> 2.52.0
> 

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