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

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

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

On Sun, Feb 22, 2026 at 12:44:40PM -0500, Jon Maloy wrote:
> We remove the addr_seen and addr_ll_seen fields in struct ip6_ctx
> and replace them by setting CONF_ADDR_OBSERVED and CONF_ADDR_LINKLOCAL
> flags in the corresponding entry in the unified address array.
> 
> The observed IPv6 address is always added/moved to position 0
> in the array, improving chances for fast lookup.
> 
> This completes the unification of address storage for both IPv4 and
> IPv6, enabling future support for multiple guest addresses per family.
> 
> Signed-off-by: Jon Maloy <jmaloy@redhat.com>
> 
> ---
> v5: - Made to use same algorithm and function as IPv4 for inserting
>       observed into the array.
> ---
>  conf.c    |  2 --
>  dhcpv6.c  |  6 +-----
>  dhcpv6.h  |  2 +-
>  migrate.c | 27 +++++++++++++++++++-------
>  passt.h   |  4 ----
>  pasta.c   | 17 +++++++++++++----
>  tap.c     | 57 +++++++++++++++++++++++++++++++++----------------------
>  7 files changed, 69 insertions(+), 46 deletions(-)

As noted in the previous patch, there are several hunks from there
that ought to be in this patch.

> diff --git a/conf.c b/conf.c
> index faa60ec..f622424 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -816,8 +816,6 @@ static unsigned int conf_ip6(struct ctx *c, unsigned int ifi)
>  		e->flags = CONF_ADDR_HOST;
>  	}
>  
> -	ip6->addr_seen = e->addr.a6;
> -
>  	if (IN6_IS_ADDR_LINKLOCAL(&ip6->guest_gw))
>  		ip6->our_tap_ll = ip6->guest_gw;
>  
> diff --git a/dhcpv6.c b/dhcpv6.c
> index 55c17cf..ba200e5 100644
> --- a/dhcpv6.c
> +++ b/dhcpv6.c
> @@ -549,7 +549,7 @@ static size_t dhcpv6_client_fqdn_fill(const struct iov_tail *data,
>   * Return: 0 if it's not a DHCPv6 message, 1 if handled, -1 on failure
>   */
>  int dhcpv6(struct ctx *c, struct iov_tail *data,
> -	   const struct in6_addr *saddr, const struct in6_addr *daddr)
> +	   const struct in6_addr *daddr)
>  {
>  	const struct opt_server_id *server_id = NULL;
>  	struct inany_addr_entry *e = first_v6(c);
> @@ -591,8 +591,6 @@ int dhcpv6(struct ctx *c, struct iov_tail *data,
>  	if (mlen + sizeof(*uh) != ntohs(uh->len) || mlen < sizeof(*mh))
>  		return -1;
>  
> -	c->ip6.addr_ll_seen = *saddr;
> -

Removing this without a replacement doesn't look correct to me.
Possibly it's redundant anyway with code in tap.c, but if that's so
removing it in a separate patch with its own rationale would make it
clearer.

>  	src = &c->ip6.our_tap_ll;
>  
>  	mh = IOV_REMOVE_HEADER(data, mh_storage);
> @@ -683,8 +681,6 @@ int dhcpv6(struct ctx *c, struct iov_tail *data,
>  
>  	tap_udp6_send(c, src, 547, tap_ip6_daddr(c, src), 546,
>  		      mh->xid, &resp, n);
> -	if (e)
> -		c->ip6.addr_seen = e->addr.a6;
>  
>  	return 1;
>  }
> diff --git a/dhcpv6.h b/dhcpv6.h
> index c706dfd..8cbc769 100644
> --- a/dhcpv6.h
> +++ b/dhcpv6.h
> @@ -7,7 +7,7 @@
>  #define DHCPV6_H
>  
>  int dhcpv6(struct ctx *c, struct iov_tail *data,
> -	   struct in6_addr *saddr, struct in6_addr *daddr);
> +	   struct in6_addr *daddr);
>  void dhcpv6_init(const struct ctx *c);
>  
>  #endif /* DHCPV6_H */
> diff --git a/migrate.c b/migrate.c
> index f026e95..1e2830f 100644
> --- a/migrate.c
> +++ b/migrate.c
> @@ -56,10 +56,7 @@ struct migrate_seen_addrs_v1 {
>  static int seen_addrs_source_v1(struct ctx *c,
>  				const struct migrate_stage *stage, int fd)
>  {
> -	struct migrate_seen_addrs_v1 addrs = {
> -		.addr6 = c->ip6.addr_seen,
> -		.addr6_ll = c->ip6.addr_ll_seen,
> -	};
> +	struct migrate_seen_addrs_v1 addrs = { 0 };
>  	const struct inany_addr_entry *e;
>  
>  	(void)stage;
> @@ -69,6 +66,15 @@ static int seen_addrs_source_v1(struct ctx *c,
>  	if (e)
>  		addrs.addr4 = *inany_v4(&e->addr);
>  
> +	/* IPv6 observed address */
> +	e = fwd_get_addr(c, AF_INET6, CONF_ADDR_OBSERVED, 0);

As for v4, you need a fall back if there's no observed address.

> +	if (e) {
> +		if (e->flags & CONF_ADDR_LINKLOCAL)
> +			addrs.addr6_ll = e->addr.a6;
> +		else
> +			addrs.addr6 = e->addr.a6;
> +	}

This isn't right.  You need to populate *both* addr6 and addr6_ll, not
one or the other.

> +
>  	memcpy(addrs.mac, c->guest_mac, sizeof(addrs.mac));
>  
>  	if (write_all_buf(fd, &addrs, sizeof(addrs)))
> @@ -89,6 +95,7 @@ static int seen_addrs_target_v1(struct ctx *c,
>  				const struct migrate_stage *stage, int fd)
>  {
>  	struct migrate_seen_addrs_v1 addrs;
> +	struct in6_addr addr6, addr6_ll;
>  	struct in_addr addr4;
>  
>  	(void)stage;
> @@ -96,14 +103,20 @@ static int seen_addrs_target_v1(struct ctx *c,
>  	if (read_all_buf(fd, &addrs, sizeof(addrs)))
>  		return errno;
>  
> -	c->ip6.addr_seen = addrs.addr6;
> -	c->ip6.addr_ll_seen = addrs.addr6_ll;
> -
>  	/* Copy from packed struct to avoid alignment issues */
>  	addr4 = addrs.addr4;
> +	addr6 = addrs.addr6;
> +	addr6_ll = addrs.addr6_ll;
> +
>  	if (addr4.s_addr)
>  		fwd_set_addr(c, &inany_from_v4(addr4), CONF_ADDR_OBSERVED, 0);
>  
> +	/* Prefer global over link-local if both present */
> +	if (!IN6_IS_ADDR_UNSPECIFIED(&addr6))
> +		fwd_set_addr(c, &inany_from_v6(addr6), CONF_ADDR_OBSERVED, 0);
> +	else if (!IN6_IS_ADDR_UNSPECIFIED(&addr6_ll))
> +		fwd_set_addr(c, &inany_from_v6(addr6_ll), CONF_ADDR_OBSERVED, 0);

Likewise, you need to put both the LL and non-LL observed addresses
back into the table, not just one.

>  	memcpy(c->guest_mac, addrs.mac, sizeof(c->guest_mac));
>  
>  	return 0;
> diff --git a/passt.h b/passt.h
> index b808a19..e7489ca 100644
> --- a/passt.h
> +++ b/passt.h
> @@ -112,8 +112,6 @@ struct ip4_ctx {
>  
>  /**
>   * struct ip6_ctx - IPv6 execution context
> - * @addr_seen:		Latest IPv6 global/site address seen as source from tap
> - * @addr_ll_seen:	Latest IPv6 link-local address seen as source from tap
>   * @guest_gw:		IPv6 gateway as seen by the guest
>   * @map_host_loopback:	Outbound connections to this address are NATted to the
>   *                      host's [::1]
> @@ -129,8 +127,6 @@ struct ip4_ctx {
>   * @no_copy_addrs:	Don't copy all addresses when configuring namespace
>   */
>  struct ip6_ctx {
> -	struct in6_addr addr_seen;
> -	struct in6_addr addr_ll_seen;
>  	struct in6_addr guest_gw;
>  	struct in6_addr map_host_loopback;
>  	struct in6_addr map_guest_addr;
> diff --git a/pasta.c b/pasta.c
> index 7a081e7..56b2e0a 100644
> --- a/pasta.c
> +++ b/pasta.c
> @@ -46,6 +46,8 @@
>  
>  #include "util.h"
>  #include "passt.h"
> +#include "conf.h"
> +#include "fwd.h"
>  #include "isolation.h"
>  #include "netlink.h"
>  #include "log.h"
> @@ -384,12 +386,15 @@ void pasta_ns_conf(struct ctx *c)
>  ipv4_done:
>  
>  		if (c->ifi6) {
> -			rc = nl_addr_get_ll(nl_sock_ns, c->pasta_ifi,
> -					    &c->ip6.addr_ll_seen);
> -			if (rc < 0) {
> +			struct in6_addr addr_ll;
> +
> +			rc = nl_addr_get_ll(nl_sock_ns, c->pasta_ifi, &addr_ll);
> +			if (rc < 0)
>  				warn("Can't get LL address from namespace: %s",
>  				    strerror_(-rc));
> -			}
> +			else
> +				fwd_set_addr(c, &inany_from_v6(addr_ll),
> +					     CONF_ADDR_OBSERVED, 0);
>  
>  			rc = nl_addr_set_ll_nodad(nl_sock_ns, c->pasta_ifi);
>  			if (rc < 0) {
> @@ -403,6 +408,10 @@ ipv4_done:
>  
>  			if (c->ip6.no_copy_addrs) {
>  				for_each_addr(e, c, AF_INET6) {
> +					/* Skip, kernel auto-configures */
> +					if (e->flags & CONF_ADDR_LINKLOCAL)
> +						continue;

This seems like it belongs in an earlier patch - it's not really
related to observed addresses specifically.

>  					rc = nl_addr_set(nl_sock_ns,
>  							 c->pasta_ifi,
>  							 AF_INET6, &e->addr.a6,
> diff --git a/tap.c b/tap.c
> index 30e52f7..875c2bf 100644
> --- a/tap.c
> +++ b/tap.c
> @@ -174,6 +174,17 @@ static void tap_check_src_addr4(struct ctx *c, const struct in_addr *addr)
>  		fwd_set_addr(c, &inany_from_v4(*addr), CONF_ADDR_OBSERVED, 0);
>  }
>  
> +/**
> + * tap_check_src_addr6() - Note an IPv6 address seen in guest traffic
> + * @c:		Execution context
> + * @addr:	IPv6 address seen as source from guest
> + */
> +static void tap_check_src_addr6(struct ctx *c, const struct in6_addr *addr)
> +{
> +	if (!IN6_IS_ADDR_UNSPECIFIED(addr))
> +		fwd_set_addr(c, &inany_from_v6(*addr), CONF_ADDR_OBSERVED, 0);
> +}
> +
>  /**
>   * tap_ip6_daddr() - Normal IPv6 destination address for inbound packets
>   * @c:		Execution context
> @@ -184,9 +195,25 @@ static void tap_check_src_addr4(struct ctx *c, const struct in_addr *addr)
>  const struct in6_addr *tap_ip6_daddr(const struct ctx *c,
>  				     const struct in6_addr *src)
>  {
> -	if (IN6_IS_ADDR_LINKLOCAL(src))
> -		return &c->ip6.addr_ll_seen;
> -	return &c->ip6.addr_seen;
> +	const struct inany_addr_entry *e;
> +
> +	if (IN6_IS_ADDR_LINKLOCAL(src)) {
> +		/* Link-local: first LL address (observed is at front) */
> +		e = fwd_get_addr(c, AF_INET6, CONF_ADDR_LINKLOCAL, 0);
> +	} else {
> +		/* Global: observed non-LL first, then any non-LL */

Do you need this in addition to your logic to put observed addresses
at the beginning of the array?

> +		e = fwd_get_addr(c, AF_INET6, CONF_ADDR_OBSERVED,
> +				 CONF_ADDR_LINKLOCAL);
> +		if (!e)
> +			e = fwd_get_addr(c, AF_INET6, 0, CONF_ADDR_LINKLOCAL);
> +	}
> +
> +	if (e)
> +		return &e->addr.a6;
> +
> +	/* Last resort: first IPv6 address */
> +	e = first_v6(c);

I think we should fail rather than falling back further.  We really
mustn't mix linklocal and non-linklocal addresses in a packet.

> +	return e ? &e->addr.a6 : &in6addr_any;
>  }
>  
>  /**
> @@ -786,7 +813,7 @@ resume:
>  
>  		if (iph->saddr)
>  			tap_check_src_addr4(c,
> -					    (const struct in_addr *)&iph->saddr);
> +					   (const struct in_addr *)&iph->saddr);

Unrelated whitespace change

>  
>  		if (!iov_drop_header(&data, hlen))
>  			continue;
> @@ -958,24 +985,8 @@ resume:
>  			continue;
>  		}
>  
> -		if (IN6_IS_ADDR_LINKLOCAL(saddr)) {
> -			c->ip6.addr_ll_seen = *saddr;
> -
> -			if (IN6_IS_ADDR_UNSPECIFIED(&c->ip6.addr_seen)) {
> -				c->ip6.addr_seen = *saddr;
> -			}
> -
> -			if (!first_v6(c) && c->addr_count < INANY_MAX_ADDRS) {
> -				struct inany_addr_entry *e;
> -
> -				e = &c->addrs[c->addr_count++];
> -				e->addr.a6 = *saddr;
> -				e->prefix_len = 64;
> -				e->flags = CONF_ADDR_LINKLOCAL;
> -			}
> -		} else if (!IN6_IS_ADDR_UNSPECIFIED(saddr)){
> -			c->ip6.addr_seen = *saddr;
> -		}
> +		if (!IN6_IS_ADDR_UNSPECIFIED(saddr))

Redundant with the check inside tap_check_src_addr6().

> +			tap_check_src_addr6(c, saddr);
>  
>  		if (proto == IPPROTO_ICMPV6) {
>  			struct iov_tail ndp_data;
> @@ -1006,7 +1017,7 @@ resume:
>  		if (proto == IPPROTO_UDP) {
>  			struct iov_tail uh_data = data;
>  
> -			if (dhcpv6(c, &uh_data, saddr, daddr))
> +			if (dhcpv6(c, &uh_data, daddr))
>  				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 --]