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

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

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

On Sat, Mar 21, 2026 at 08:43:28PM -0400, 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
> v6: - Refactored fwd_set_addr(), notably:
>       o Limited number of allowed observed addresses to four per protocol
>       o I kept the memmove() calls, since I find no more elegant way to
>         do this. Performance cost should be minimal, since these parts
>         of the code will execute only very exceptionally. Note that
>         removing the 'oldest' entry implicitly means removing the least
>         used one, since the latter will migrate to the highest position
>         after a few iterations of remove/add.
>       o Also kept the prefix_len update. Not sure about this, but I
>         cannot see how the current approach can cause any harm.
>     - Other changes suggested by David G, notably reversing some
>       residues after an accidental merge/re-split with the next
>       commit.
> ---
>  conf.c    |   4 --
>  fwd.c     | 132 +++++++++++++++++++++++++++++++++++++++++++++---------
>  fwd.h     |   4 ++
>  inany.h   |  10 +++++
>  migrate.c |  17 ++++++-
>  passt.h   |   6 +--
>  tap.c     |  20 +++++++--
>  7 files changed, 160 insertions(+), 33 deletions(-)
> 
> diff --git a/conf.c b/conf.c
> index 70435d3..1c9f07c 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -771,9 +771,6 @@ static unsigned int conf_ip4(struct ctx *c, unsigned int ifi)
>  		a = fwd_get_addr(c, AF_INET, CONF_ADDR_HOST, 0);
>  	}
>  
> -	if (a)
> -		ip4->addr_seen = *inany_v4(&a->addr);
> -
>  	ip4->our_tap_addr = ip4->guest_gw;
>  
>  	return ifi;
> @@ -787,7 +784,6 @@ static void conf_ip4_local(struct ctx *c)
>  {
>  	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;
>  	fwd_set_addr(c, &inany_from_v4(IP4_LL_GUEST_ADDR),
> diff --git a/fwd.c b/fwd.c
> index 2853c0e..28a721e 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"
> @@ -252,7 +253,7 @@ void fwd_neigh_table_init(const struct ctx *c)
>  /**
>   * fwd_set_addr() - Update address entry, adding one if needed
>   * @c:		Execution context
> - * @addr:	Address to add (IPv4-mapped or IPv6)
> + * @addr:	Address to add/alter

Nit: the new description was already more accurate, since you could at
least update flags with this function.

>   * @flags:	CONF_ADDR_* flags for this address
>   * @prefix_len:	Prefix length in IPv6/mapped format, 0-128
>   *
> @@ -260,23 +261,69 @@ void fwd_neigh_table_init(const struct ctx *c)
>  void fwd_set_addr(struct ctx *c, const union inany_addr *addr,
>  		  uint8_t flags, int prefix_len)
>  {
> -	struct guest_addr *a;
> +	struct guest_addr *a, *arr = &c->addrs[0], *rm = NULL;
> +	int count = c->addr_count;
> +	int af_cnt = 0;
>  
> -	for (int i = 0; i < c->addr_count; i++) {
> -		a = &c->addrs[i];
> -		if (inany_equals(addr, &a->addr))
> -			goto found;
> +	for_each_addr(a, c, inany_af(addr)) {

Checking the family is implicit in the inany_equals(), so passing the
address family here doesn't really accomplish anything.

> +		if (!inany_equals(&a->addr, addr))
> +			continue;
> +
> +		/* Update prefix_len if provided and applicable */
> +		if (prefix_len && !(a->flags & CONF_ADDR_USER))
> +			a->prefix_len = prefix_len;
> +
> +		/* Nothing more to change */
> +		if ((a->flags & flags) == flags)
> +			return;
> +
> +		if (!(flags & CONF_ADDR_OBSERVED)) {
> +			a->flags |= flags;

Put this before the if, and you can avoid duplicating it after the if.

> +			return;
> +		}
> +
> +		/* Observed address moves to position 0: remove, re-add later */
> +		flags |= a->flags;
> +		prefix_len = a->prefix_len;
> +		memmove(a, a + 1, (&arr[count] - (a + 1)) * sizeof(*a));
> +		c->addr_count = --count;
> +		break;
>  	}
>  
> -	if (c->addr_count >= MAX_GUEST_ADDRS)
> +	if (count >= MAX_GUEST_ADDRS) {
> +		debug("Address table full, can't add address");
>  		return;
> +	}
>  
> -	a = &c->addrs[c->addr_count++];
> -
> -found:
> +	/* Add to head or tail, depending on flag */
> +	if (flags & CONF_ADDR_OBSERVED) {
> +		a = &arr[0];
> +		memmove(&arr[1], a, count * sizeof(*a));
> +	} else {
> +		a = &arr[count];
> +	}
> +	c->addr_count = ++count;
>  	a->addr = *addr;
>  	a->prefix_len = prefix_len;
>  	a->flags = flags;
> +
> +	if (!(flags & CONF_ADDR_OBSERVED))
> +		return;
> +
> +	/* Remove oldest observed address of this protocol if too many */
> +	for (int i = count - 1; i >= 0; i--) {
> +		a = &arr[i];
> +		if ((!(a->flags & CONF_ADDR_OBSERVED)) ||
> +		    (inany_af(&a->addr)) != inany_af(addr))

This could delete any OBSERVED address, even if it's also USER or
HOST.  I think you only want to purge addresses that are _only_
OBSERVED.

> +			continue;
> +		if (!rm)
> +			rm = a;
> +		af_cnt++;
> +	}
> +	if (af_cnt > MAX_OBSERVED_ADDRS) {
> +		memmove(rm, rm + 1, (&arr[count] - (rm + 1)) * sizeof(*rm));
> +		c->addr_count--;
> +	}

I still don't think that tracking >1 observed address per family+scope
is useful.  There's two situations where we could get multiple
observed addresses:

1) The guest is periodically changing addresses (e.g. at reboots)
2) The guest is actively using multiple addresses

We have no good way of telling the difference between those, and even
if we did it doesn't matter:

In case (1), we don't care about previous addresses, only the current
/ latest one.

In case (2), tracking all the observed addresses doesn't help us,
because we still don't know what the right one to send to is.
Handling this requires more stuff in the forwarding tables.  I have
plans to add that, but it might be a while before I get there.

Or to look at it another way: the users of @addr_seen require a unique
address.  So replacing it with something that doesn't give a unique
address isn't helpful.

>  }
>  
>  /**
> @@ -987,6 +1034,38 @@ static bool is_dns_flow(uint8_t proto, const struct flowside *ini)
>  		((ini->oport == 53) || (ini->oport == 853));
>  }
>  
> +/**
> + * fwd_select_addr() - Select address with priority-based search
> + * @c:		Execution context
> + * @af:		Address family (AF_INET or AF_INET6)
> + * @primary:	Primary flags to match (or 0 to skip)
> + * @secondary:	Secondary flags to match (or 0 to skip)
> + * @skip:	Flags to exclude from search
> + *
> + * Search for address entries in priority order.
> + *
> + * Return: pointer to selected address entry, or NULL if none found
> + */
> +const struct guest_addr *fwd_select_addr(const struct ctx *c, int af,
> +					 int primary, int secondary, int skip)
> +{
> +	const struct guest_addr *a;
> +
> +	if (primary) {
> +		a = fwd_get_addr(c, af, primary, skip);
> +		if (a)
> +			return a;
> +	}
> +
> +	if (secondary) {
> +		a = fwd_get_addr(c, af, secondary, skip);
> +		if (a)
> +			return a;
> +	}
> +
> +	return NULL;
> +}
> +
>  /**
>   * fwd_guest_accessible() - Is address guest-accessible
>   * @c:		Execution context
> @@ -1011,17 +1090,11 @@ static bool fwd_guest_accessible(const struct ctx *c,
>  	if (inany_is_unspecified(addr))
>  		return false;
>  
> -	/* Check against all configured guest addresses */
> +	/* Check against all known guest addresses */
>  	for_each_addr(a, c, AF_UNSPEC)
>  		if (inany_equals(addr, &a->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.
> @@ -1216,10 +1289,20 @@ 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 guest_addr *a;
> +
> +				a = fwd_select_addr(c, AF_INET,
> +						    CONF_ADDR_OBSERVED,
> +						    CONF_ADDR_USER |
> +						    CONF_ADDR_HOST, 0);
> +				if (!a)
> +					return PIF_NONE;
> +
> +				tgt->eaddr = a->addr;
> +			}
>  			tgt->oaddr = inany_any4;
>  		} else {
>  			if (c->host_lo_to_ns_lo)
> @@ -1254,7 +1337,14 @@ 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 guest_addr *a;
> +
> +		a = fwd_select_addr(c, AF_INET, CONF_ADDR_OBSERVED,
> +				    CONF_ADDR_USER | CONF_ADDR_HOST, 0);
> +		if (!a)
> +			return PIF_NONE;
> +
> +		tgt->eaddr = a->addr;
>  	} else {
>  		if (inany_is_linklocal6(&tgt->oaddr))
>  			tgt->eaddr.a6 = c->ip6.addr_ll_seen;
> diff --git a/fwd.h b/fwd.h
> index c5a1068..9893856 100644
> --- a/fwd.h
> +++ b/fwd.h
> @@ -25,6 +25,10 @@ void fwd_probe_ephemeral(void);
>  bool fwd_port_is_ephemeral(in_port_t port);
>  const struct guest_addr *fwd_get_addr(const struct ctx *c, sa_family_t af,
>  				      uint8_t incl, uint8_t excl);
> +const struct guest_addr *fwd_select_addr(const struct ctx *c, int af,
> +					 int primary, int secondary, int skip);
> +void fwd_set_addr(struct ctx *c, const union inany_addr *addr,
> +		  uint8_t flags, int prefix_len);
>  
>  /**
>   * struct fwd_rule - Forwarding rule governing a range of ports
> diff --git a/inany.h b/inany.h
> index 9891ed6..7b23cb0 100644
> --- a/inany.h
> +++ b/inany.h
> @@ -102,6 +102,16 @@ static inline struct in_addr *inany_v4(const union inany_addr *addr)
>  	return (struct in_addr *)&addr->v4mapped.a4;
>  }
>  
> +/** inany_af - Get address family of IPv[46] address
> + * @addr:	IPv4 or IPv6 address
> + *
> + * Return: AF_INET for IPv4, AF_INET6 for IPv6
> + */
> +static inline sa_family_t inany_af(const union inany_addr *addr)
> +{
> +	return inany_v4(addr) ? AF_INET : AF_INET6;
> +}
> +
>  /** inany_default_prefix_len() - Get default prefix length for address
>   * @addr:	IPv4 or iPv6 address
>   *
> diff --git a/migrate.c b/migrate.c
> index 1e8858a..1e02720 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,18 @@ static int seen_addrs_source_v2(struct ctx *c,
>  	struct migrate_seen_addrs_v2 addrs = {
>  		.addr6 = c->ip6.addr_seen,
>  		.addr6_ll = c->ip6.addr_ll_seen,
> -		.addr4 = c->ip4.addr_seen,
>  	};
> +	const struct guest_addr *a;
>  
>  	(void)stage;
>  
> +	/* IPv4 observed address, with fallback to configured address */
> +	a = fwd_select_addr(c, AF_INET, CONF_ADDR_OBSERVED,
> +			    CONF_ADDR_USER | CONF_ADDR_HOST,
> +			    CONF_ADDR_LINKLOCAL);
> +	if (a)
> +		addrs.addr4 = *inany_v4(&a->addr);
> +
>  	memcpy(addrs.mac, c->guest_mac, sizeof(addrs.mac));
>  
>  	if (write_all_buf(fd, &addrs, sizeof(addrs)))
> @@ -90,7 +99,11 @@ static int seen_addrs_target_v2(struct ctx *c,
>  
>  	c->ip6.addr_seen = addrs.addr6;
>  	c->ip6.addr_ll_seen = addrs.addr6_ll;
> -	c->ip4.addr_seen = addrs.addr4;
> +
> +	if (addrs.addr4.s_addr)
> +		fwd_set_addr(c, &inany_from_v4(addrs.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 6bca778..5452225 100644
> --- a/passt.h
> +++ b/passt.h
> @@ -64,8 +64,9 @@ enum passt_modes {
>  	MODE_VU,
>  };
>  
> -/* Maximum number of addresses in context address array */
> +/* Limits on number of addresses in context address array */
>  #define MAX_GUEST_ADDRS		32
> +#define MAX_OBSERVED_ADDRS	4
>  
>  /* Flags indicating origin and role of a guest address
>   * To be used in struct guest_addr
> @@ -73,6 +74,7 @@ enum passt_modes {
>  #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 */
>  
>  /**
>   * struct guest_addr - Unified IPv4/IPv6 address entry
> @@ -88,7 +90,6 @@ struct guest_addr {
>  
>  /**
>   * 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
> @@ -104,7 +105,6 @@ struct guest_addr {
>   * @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 78a494a..c75a4df 100644
> --- a/tap.c
> +++ b/tap.c
> @@ -47,6 +47,7 @@
>  #include "ip.h"
>  #include "iov.h"
>  #include "passt.h"
> +#include "fwd.h"
>  #include "arp.h"
>  #include "dhcp.h"
>  #include "ndp.h"
> @@ -161,6 +162,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)

This duplicates the iph->saddr check in the caller.

In fact, it's not clear to me that this function makes things any
shorter or clearer than just calling fwd_set_addr() directly from
tap4_handler().

> +		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
> @@ -723,6 +735,7 @@ static int tap4_handler(struct ctx *c, const struct pool *in,
>  resume:
>  	for (seq_count = 0, seq = NULL; i < in->count; i++) {
>  		size_t l3len, hlen, l4len;
> +		const struct in_addr *ia;
>  		struct ethhdr eh_storage;
>  		struct iphdr iph_storage;
>  		struct udphdr uh_storage;
> @@ -771,9 +784,10 @@ resume:
>  			continue;
>  		}
>  
> -		if (iph->saddr && c->ip4.addr_seen.s_addr != iph->saddr)
> -			c->ip4.addr_seen.s_addr = iph->saddr;
> -
> +		if (iph->saddr) {
> +			ia = (const struct in_addr *) &iph->saddr;
> +			tap_check_src_addr4(c, ia);
> +		}
>  		if (!iov_drop_header(&data, hlen))
>  			continue;
>  		if (iov_tail_size(&data) != l4len)
> -- 
> 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 --]