Re: [PATCH v3 10/11] ip: Track observed guest IPv6 addresses in unified address array

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

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

On Fri, Jan 30, 2026 at 04:44:46PM -0500, Jon Maloy wrote:
> We remove the addr_seen and addr_ll_seen fields in struct ip6_ctx
> and replace them by setting INANY_ADDR_OBSERVED and INANY_ADDR_LINKLOCAL
> flags in the corresponding entry in the unified address array.
> If the seen address is not present in the array we add it first.
> 
> 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>

A number of the comments on 9/11 will also apply here.

> ---
> v3: - Adapted to previous changes in this series
> ---
>  conf.c    |  5 ----
>  dhcpv6.c  |  6 ++---
>  dhcpv6.h  |  2 +-
>  fwd.c     | 62 +++++++++++++++++++++++++++++++------------
>  migrate.c | 64 +++++++++++++++++++++++++++++++++++++++------
>  passt.h   |  5 +---
>  pasta.c   | 15 ++++++++++-
>  tap.c     | 78 +++++++++++++++++++++++++++++++++++++++++++++----------
>  8 files changed, 185 insertions(+), 52 deletions(-)
> 
> diff --git a/conf.c b/conf.c
> index c5818a8..6892cea 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -764,7 +764,6 @@ static void conf_ip4_local(struct ctx *c)
>   */
>  static unsigned int conf_ip6(unsigned int ifi, struct ctx *c)
>  {
> -	struct inany_addr_entry *e;
>  	union inany_addr addr = {0,};
>  	int prefix_len = 0;
>  	int rc;
> @@ -800,10 +799,6 @@ static unsigned int conf_ip6(unsigned int ifi, struct ctx *c)
>  		c->addr_count++;
>  	}
>  
> -	e = first_v6(c);
> -	if (e)
> -		c->ip6.addr_seen = e->addr.a6;
> -
>  	if (IN6_IS_ADDR_LINKLOCAL(&c->ip6.guest_gw))
>  		c->ip6.our_tap_ll = c->ip6.guest_gw;
>  
> diff --git a/dhcpv6.c b/dhcpv6.c
> index a1e5f15..67307d9 100644
> --- a/dhcpv6.c
> +++ b/dhcpv6.c
> @@ -546,10 +546,11 @@ 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;
>  	const struct opt_hdr *client_id = NULL;
> +

Unrelated whitespace change.

>  	/* The _storage variables can't be local to the blocks they're used in,
>  	 * because IOV_*_HEADER() may return pointers to them which are
>  	 * dereferenced afterwards. Since we don't have Rust-like lifetime
> @@ -587,8 +588,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;
> -

It's stored differently, but we still want to update the observed
ip6_ll address here.

>  	src = &c->ip6.our_tap_ll;
>  
>  	mh = IOV_REMOVE_HEADER(data, mh_storage);
> @@ -679,7 +678,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);
> -	c->ip6.addr_seen = first_v6(c)->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/fwd.c b/fwd.c
> index 5af482d..fc26eba 100644
> --- a/fwd.c
> +++ b/fwd.c
> @@ -515,6 +515,36 @@ static const struct in_addr *fwd_guest_addr4(const struct ctx *c)
>  	return NULL;
>  }
>  
> +/**
> + * fwd_guest_addr6() - Get first observed IPv6 guest address
> + * @c:		Execution context
> + * @linklocal:	If true, find link-local address; if false, find global
> + *
> + * Return: first observed IPv6 address, if any,
> + *         or first configured, if any, or NULL
> + */
> +static const struct in6_addr *fwd_guest_addr6(const struct ctx *c,
> +					      bool linklocal)
> +{
> +	const struct inany_addr_entry *e;
> +
> +	/* Find first observed address of matching scope */
> +	for_each_addr(c, e, AF_INET6) {
> +		if (!!(e->flags & INANY_ADDR_LINKLOCAL) != linklocal)
> +			continue;
> +		if (e->flags & INANY_ADDR_OBSERVED)
> +			return &e->addr.a6;
> +	}
> +
> +	/* Fallback to first address of matching scope */
> +	for_each_addr(c, e, AF_INET6) {
> +		if (!!(e->flags & INANY_ADDR_LINKLOCAL) == linklocal)
> +			return &e->addr.a6;
> +	}
> +
> +	return NULL;
> +}
> +
>  /**
>   * fwd_guest_accessible4() - Is IPv4 address guest-accessible
>   * @c:		Execution context
> @@ -563,19 +593,11 @@ static bool fwd_guest_accessible6(const struct ctx *c,
>  	if (IN6_IS_ADDR_LOOPBACK(addr))
>  		return false;
>  
> -	/* Check against all configured guest addresses */
> +	/* Check against all guest addresses (configured, host, or observed) */
>  	for_each_addr(c, e, AF_INET6)
>  		if (IN6_ARE_ADDR_EQUAL(addr, &e->addr.a6))
>  			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 (!IN6_IS_ADDR_UNSPECIFIED(&c->ip6.addr_seen) &&
> -	    IN6_ARE_ADDR_EQUAL(addr, &c->ip6.addr_seen))
> -		return false;
> -
>  	return true;
>  }
>  
> @@ -798,10 +820,16 @@ uint8_t fwd_nat_from_host(const struct ctx *c, uint8_t proto,
>  			}
>  			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 in6_addr *guest_addr6;
> +
> +				guest_addr6 = fwd_guest_addr6(c, false);
> +				if (!guest_addr6)
> +					return PIF_NONE;
> +				tgt->eaddr.a6 = *guest_addr6;
> +			}
>  			tgt->oaddr = inany_any6;
>  		}
>  
> @@ -833,10 +861,12 @@ uint8_t fwd_nat_from_host(const struct ctx *c, uint8_t proto,
>  			return PIF_NONE;
>  		tgt->eaddr = inany_from_v4(*guest_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);
> +		const struct in6_addr *guest_addr6 = fwd_guest_addr6(c, linklocal);
> +
> +		if (!guest_addr6)
> +			return PIF_NONE;
> +		tgt->eaddr.a6 = *guest_addr6;
>  	}
>  
>  	return PIF_TAP;
> diff --git a/migrate.c b/migrate.c
> index a84af1c..20da1f7 100644
> --- a/migrate.c
> +++ b/migrate.c
> @@ -66,10 +66,7 @@ struct migrate_seen_addrs_v3 {
>  static int seen_addrs_source_v2(struct ctx *c,
>  				const struct migrate_stage *stage, int fd)
>  {
> -	struct migrate_seen_addrs_v2 addrs = {
> -		.addr6 = c->ip6.addr_seen,
> -		.addr6_ll = c->ip6.addr_ll_seen,
> -	};
> +	struct migrate_seen_addrs_v2 addrs = { 0 };
>  	const struct inany_addr_entry *e;
>  
>  	(void)stage;
> @@ -82,6 +79,23 @@ static int seen_addrs_source_v2(struct ctx *c,
>  		}
>  	}
>  
> +	/* Find first observed IPv6 addresses (global and link-local) */
> +	for_each_addr(c, e, AF_INET6) {
> +		struct in6_addr tmp;
> +
> +		if (!(e->flags & INANY_ADDR_OBSERVED))
> +			continue;
> +		if (e->flags & INANY_ADDR_LINKLOCAL) {
> +			tmp = addrs.addr6_ll;
> +			if (IN6_IS_ADDR_UNSPECIFIED(&tmp))
> +				addrs.addr6_ll = e->addr.a6;
> +		} else {
> +			tmp = addrs.addr6;
> +			if (IN6_IS_ADDR_UNSPECIFIED(&tmp))
> +				addrs.addr6 = e->addr.a6;
> +		}
> +	}

You could reuse fwd_guest_addr6() here.

> +
>  	memcpy(addrs.mac, c->guest_mac, sizeof(addrs.mac));
>  
>  	if (write_all_buf(fd, &addrs, sizeof(addrs)))
> @@ -117,6 +131,37 @@ static void migrate_observed_addr4(struct ctx *c, const struct in_addr *addr)
>  	}
>  }
>  
> +/**
> + * migrate_observed_addr6() - Add observed IPv6 address to address array
> + * @c:		Execution context
> + * @addr:	IPv6 address to add
> + */
> +static void migrate_observed_addr6(struct ctx *c, const struct in6_addr *addr)
> +{
> +	uint8_t flags = INANY_ADDR_OBSERVED;
> +	struct inany_addr_entry *e;
> +
> +	if (IN6_IS_ADDR_UNSPECIFIED(addr))
> +		return;
> +
> +	if (IN6_IS_ADDR_LINKLOCAL(addr))
> +		flags |= INANY_ADDR_LINKLOCAL;
> +
> +	for_each_addr(c, e, AF_INET6) {
> +		if (IN6_ARE_ADDR_EQUAL(addr, &e->addr.a6)) {
> +			e->flags |= flags;
> +			return;
> +		}
> +	}
> +
> +	if (c->addr_count < INANY_MAX_ADDRS) {
> +		c->addrs[c->addr_count].addr.a6 = *addr;
> +		c->addrs[c->addr_count].prefix_len = 0;
> +		c->addrs[c->addr_count].flags = flags;
> +		c->addr_count++;
> +	}

Similar comments to v4:
 * Can use a unified function for migration and non-migration updates
 * Better to stick to 1 LL and 1 non-LL observed address for now, I think.

> +}
> +
>  /**
>   * seen_addrs_target_v2() - Receive observed addresses on target
>   * @c:		Execution context
> @@ -129,6 +174,7 @@ static int seen_addrs_target_v2(struct ctx *c,
>  				const struct migrate_stage *stage, int fd)
>  {
>  	struct migrate_seen_addrs_v2 addrs;
> +	struct in6_addr addr6, addr6_ll;
>  	struct in_addr addr4;
>  
>  	(void)stage;
> @@ -136,12 +182,14 @@ static int seen_addrs_target_v2(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;
> -
> -	/* Avoid alignment warning */
> +	/* Copy from packed struct to avoid alignment issues */
>  	addr4 = addrs.addr4;
> +	addr6 = addrs.addr6;
> +	addr6_ll = addrs.addr6_ll;
> +
>  	migrate_observed_addr4(c, &addr4);
> +	migrate_observed_addr6(c, &addr6);
> +	migrate_observed_addr6(c, &addr6_ll);
>  
>  	memcpy(c->guest_mac, addrs.mac, sizeof(c->guest_mac));
>  
> diff --git a/passt.h b/passt.h
> index 73084ae..7040781 100644
> --- a/passt.h
> +++ b/passt.h
> @@ -70,6 +70,7 @@ enum passt_modes {
>  #define INANY_ADDR_CONFIGURED	BIT(0)		/* User set via -a */
>  #define INANY_ADDR_HOST		BIT(1)		/* From host interface */
>  #define INANY_ADDR_OBSERVED	BIT(2)		/* Seen in guest traffic */
> +#define INANY_ADDR_LINKLOCAL	BIT(3)		/* Link-local address */

Isn't this redundant with the contents of the address itself?

>  /**
>   * for_each_addr() - Iterate over addresses in unified array
> @@ -134,8 +135,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]
> @@ -151,8 +150,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 8cb5873..7c2efd8 100644
> --- a/pasta.c
> +++ b/pasta.c
> @@ -350,12 +350,19 @@ static void pasta_ns_conf_ip4(struct ctx *c)
>   */
>  static void pasta_ns_conf_ip6(struct ctx *c)
>  {
> +	struct in6_addr addr_ll;
>  	int rc = 0;
>  
> -	rc = nl_addr_get_ll(nl_sock_ns, c->pasta_ifi, &c->ip6.addr_ll_seen);
> +	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 if (c->addr_count < INANY_MAX_ADDRS) {
> +		c->addrs[c->addr_count].addr.a6 = addr_ll;
> +		c->addrs[c->addr_count].prefix_len = 64;
> +		c->addrs[c->addr_count].flags =
> +			INANY_ADDR_OBSERVED | INANY_ADDR_LINKLOCAL;
> +		c->addr_count++;
>  	}
>  
>  	rc = nl_addr_set_ll_nodad(nl_sock_ns, c->pasta_ifi);
> @@ -373,6 +380,11 @@ static void pasta_ns_conf_ip6(struct ctx *c)
>  		for_each_addr(c, e, AF_INET6) {
>  			if (IN6_IS_ADDR_UNSPECIFIED(&e->addr.a6))
>  				continue;
> +
> +			/* Skip link-local - kernel auto-configures */
> +			if (e->flags & INANY_ADDR_LINKLOCAL)
> +				continue;
> +

I'm not sure we need this.  The kernel _will_ auto-configure, but if
the user has specified a specific one, we should probably use it.
Usually I wouldn't expect there to be an LL address in the array at
this point

>  			rc = nl_addr_set(nl_sock_ns, c->pasta_ifi,
>  					 AF_INET6, &e->addr.a6, e->prefix_len);
>  			if (rc < 0)
> @@ -440,6 +452,7 @@ void pasta_ns_conf(struct ctx *c)
>  
>  		if (c->ifi6)
>  			pasta_ns_conf_ip6(c);
> +

Unrelated whitespace change.

>  	}
>  
>  	proto_update_l2_buf(c->guest_mac);
> diff --git a/tap.c b/tap.c
> index f2512f2..ba60f4b 100644
> --- a/tap.c
> +++ b/tap.c
> @@ -193,6 +193,41 @@ static void tap_check_src_addr4(struct ctx *c, const struct in_addr *addr)
>  	debug("Added new observed IPv4 address");
>  }
>  
> +/**
> + * tap_check_src_addr6() - Note an IPv6 address seen in guest traffic
> + * @c:		Execution context
> + * @addr:	IPv6 address seen as source from guest
> + *
> + * Add the address to addrs[] with OBSERVED flag if not already present.
> + * Link-local addresses are also marked with LINKLOCAL flag.
> + */
> +static void tap_check_src_addr6(struct ctx *c, const struct in6_addr *addr)
> +{
> +	uint8_t flags = INANY_ADDR_OBSERVED;
> +	struct inany_addr_entry *e;
> +
> +	if (IN6_IS_ADDR_LINKLOCAL(addr))
> +		flags |= INANY_ADDR_LINKLOCAL;
> +
> +	/* Check if already in array */
> +	for_each_addr(c, e, AF_INET6) {
> +		if (IN6_ARE_ADDR_EQUAL(addr, &e->addr.a6)) {
> +			e->flags |= flags;
> +			return;
> +		}
> +	}
> +
> +	/* Add new entry if space available */
> +	if (c->addr_count < INANY_MAX_ADDRS) {
> +		c->addrs[c->addr_count].addr.a6 = *addr;
> +		c->addrs[c->addr_count].prefix_len = 0;
> +		c->addrs[c->addr_count].flags = flags;
> +		c->addr_count++;
> +	} else {
> +		warn("Address table full, can't add IPv6 address");
> +	}
> +}
> +
>  /**
>   * tap_ip6_daddr() - Normal IPv6 destination address for inbound packets
>   * @c:		Execution context
> @@ -203,9 +238,33 @@ 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;
> +	bool want_ll = IN6_IS_ADDR_LINKLOCAL(src);
> +	const struct inany_addr_entry *e;
> +
> +	/* Find first observed address of matching scope */
> +	for_each_addr(c, e, AF_INET6) {
> +		bool is_ll = !!(e->flags & INANY_ADDR_LINKLOCAL);
> +
> +		if (is_ll != want_ll)
> +			continue;
> +		if (e->flags & INANY_ADDR_OBSERVED)
> +			return &e->addr.a6;
> +	}
> +
> +	/* Fallback to first address of matching scope */
> +	for_each_addr(c, e, AF_INET6) {
> +		bool is_ll = !!(e->flags & INANY_ADDR_LINKLOCAL);
> +
> +		if (is_ll == want_ll)
> +			return &e->addr.a6;
> +	}
> +
> +	/* Last resort: return first IPv6 address */

I think we have to match scope - an LL address is not meaningful to a
peer who's not in scope.  So I think all you can do is return NULL at
this point.

> +	e = first_v6(c);
> +	if (e)
> +		return &e->addr.a6;
> +
> +	return &in6addr_any;
>  }
>  
>  /**
> @@ -976,15 +1035,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;
> -			}
> -		} else if (!IN6_IS_ADDR_UNSPECIFIED(saddr)){
> -			c->ip6.addr_seen = *saddr;
> -		}
> +		if (!IN6_IS_ADDR_UNSPECIFIED(saddr))
> +			tap_check_src_addr6(c, saddr);
>  
>  		if (proto == IPPROTO_ICMPV6) {
>  			struct iov_tail ndp_data;
> @@ -1015,7 +1067,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 --]