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

[Stefano Brivio <sbrivio@redhat.com>]

On Sat, 21 Mar 2026 20:43:29 -0400
Jon Maloy <jmaloy@redhat.com> 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.
> 
> v6: - Re-introduced code that by accident had been moved to the
>       previous commit.
>     - Some fixes based on feedback from David G.
> ---
>  conf.c    |  6 ------
>  dhcpv6.c  | 20 +++++++++++--------
>  dhcpv6.h  |  2 +-
>  fwd.c     | 38 ++++++++++++++++++++++--------------
>  inany.h   |  3 +++
>  migrate.c | 33 +++++++++++++++++++++++--------
>  passt.h   |  4 ----
>  pasta.c   |  7 +++++--
>  tap.c     | 58 ++++++++++++++++++++++++++++++++++++-------------------
>  9 files changed, 107 insertions(+), 64 deletions(-)
> 
> diff --git a/conf.c b/conf.c
> index 1c9f07c..320a9e4 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -767,8 +767,6 @@ static unsigned int conf_ip4(struct ctx *c, unsigned int ifi)
>  		}
>  		if (!rc || !fwd_get_addr(c, AF_INET, 0, 0))
>  			return 0;
> -
> -		a = fwd_get_addr(c, AF_INET, CONF_ADDR_HOST, 0);
>  	}
>  
>  	ip4->our_tap_addr = ip4->guest_gw;
> @@ -829,7 +827,6 @@ static unsigned int conf_ip6(struct ctx *c, unsigned int ifi)
>  			      strerror_(-rc));
>  			return 0;
>  		}
> -		a = fwd_get_addr(c, AF_INET6, CONF_ADDR_HOST, 0);
>  	} else {
>  		rc = nl_addr_get_ll(nl_sock, ifi, &ip6->our_tap_ll);
>  		if (rc < 0) {
> @@ -838,9 +835,6 @@ static unsigned int conf_ip6(struct ctx *c, unsigned int ifi)
>  		}
>  	}
>  
> -	if (a)
> -		ip6->addr_seen = a->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 3a007bf..313c243 100644
> --- a/dhcpv6.c
> +++ b/dhcpv6.c
> @@ -382,8 +382,12 @@ static void dhcpv6_send_ia_notonlink(struct ctx *c,
>  {
>  	const struct in6_addr *src = &c->ip6.our_tap_ll;
>  	struct opt_hdr *ia = (struct opt_hdr *)resp_not_on_link.var;
> +	const struct in6_addr *dst = tap_ip6_daddr(c, src);
>  	size_t n;
>  
> +	if (!dst)
> +		return;
> +
>  	info("DHCPv6: received CONFIRM with inappropriate IA,"
>  	     " sending NotOnLink status in REPLY");
>  
> @@ -405,7 +409,7 @@ static void dhcpv6_send_ia_notonlink(struct ctx *c,
>  
>  	resp_not_on_link.hdr.xid = xid;
>  
> -	tap_udp6_send(c, src, 547, tap_ip6_daddr(c, src), 546,
> +	tap_udp6_send(c, src, 547, dst, 546,
>  		      xid, &resp_not_on_link, n);
>  }
>  
> @@ -549,7 +553,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;
>  	const struct opt_hdr *client_id = NULL;
> @@ -565,9 +569,9 @@ int dhcpv6(struct ctx *c, struct iov_tail *data,
>  	/* cppcheck-suppress [variableScope,unmatchedSuppression] */
>  	struct opt_hdr client_id_storage;
>  	/* cppcheck-suppress [variableScope,unmatchedSuppression] */
> +	const struct in6_addr *src, *dst;
>  	struct opt_ia_na ia_storage;
>  	const struct guest_addr *a;
> -	const struct in6_addr *src;
>  	struct msg_hdr mh_storage;
>  	const struct msg_hdr *mh;
>  	struct udphdr uh_storage;
> @@ -593,9 +597,12 @@ 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;
> +	/* Guest LL address already recorded by tap_check_src_addr6() */

I spent a substantial amount of time on this part but I couldn't figure
out in detail one thing: why are we updating the observed link-local
address here? Is it really fine to just update it in tap6_handler()?

Some observations below, and the most plausible explanation I found
(which would confirm that it's a left-over and it's fine to change
it):

1. before commit 4aa8e54a303d ("passt: Introduce a DHCPv6 server"), we
   had this in tap6_handler():

	c->addr6_guest = ip6h->saddr;

2. and after that commit:

	if (ndp(c, eh, len) || dhcpv6(c, eh, len))
		return;
 
	l4h = ipv6_l4hdr(ip6h, &proto);

        c->addr6_guest = ip6h->saddr;

   with that assignment repeated in dhcpv6(), because the guest
   observed address was needed there to send the DHCPv6 response to.

   Until that point, I didn't want to have that assignment in ndp() or
   before it, because I thought it could be problematic that we would
   store a given address just before advertising a new prefix. On the
   other hand...

3. commit 7fa3e90290d1 ("ndp: Store link-local or global address on
   any NDP message received") adds this to ndp():

	if (IN6_IS_ADDR_LINKLOCAL(&ip6h->saddr))
		c->addr6_ll_seen = ip6h->saddr;
	else
		c->addr6_seen = ip6h->saddr;

4. so, again, why not just replace all this with an assignment before
   possibly calling ndp() or dhcpv6()?

   I guess it was simply a flawed reasoning of this type: NDP feels
   like ARP to me (the role is conceptually similar), so, I thought,
   we shouldn't look at IP stuff before handling NDP, because it's
   not IP. But it is, indeed, it's UDP over IPv6...

So far so good, no valid reason to keep any of that.

However, there's an hidden issue that this change might reveal: we
unconditionally used to set c->ip6.addr_ll_seen to the source address
of the DHCPv6 message, regardless of whether it's a link-local address
or not.

Note that RFC 8415 doesn't mandate using a link-local address as
source for client messages. Section 3 says:

   [...] The
   availability of these features means that a client can use its
   link-local address and a well-known multicast address to discover and
   communicate with DHCP servers or relay agents on its link.

"can use", and section 5 mentions:

   [...] The client uses a link-local address or
   addresses determined through other mechanisms for transmitting and
   receiving DHCP messages.

   A DHCP client sends most messages using a reserved, link-scoped
   multicast destination address so that the client need not be
   configured with the address or addresses of DHCP servers.

"other mechanisms" and "most messages", but this isn't really specified,
so, actually, any message could be sent with a DHCPv6-assigned address
as source, or an administratively set address, which could happily be a
global unicast address for example.

Section 17.1 doesn't restrict the address scope any further:

     17.1.  Source Address and Interface Selection for Address
     Assignment

     When a client sends a DHCP message to the
     All_DHCP_Relay_Agents_and_Servers multicast address, it SHOULD send
     the message through the interface for which configuration information
     (including the addresses) is being requested. [...]

The only restriction comes from Section 16:

     A server MUST discard any Solicit, Confirm, Rebind, or
     Information-request messages it receives with a Layer 3 unicast
     destination address.

but this is on the destination address only. The scope of the source
address doesn't need to match the scope of the destination.

As a result, the unconditional setting we had before guaranteed that we
would reply to the client message using the original source address as
destination. There's no explicit requirement to do so, in the RFC, as
far as I can tell, but it looks rather natural to me.

What happens now is this, though:

>  	src = &c->ip6.our_tap_ll;
> +	dst = tap_ip6_daddr(c, src);

if we have a link-local observed address, we'll use that to reply to
the client, regardless of the original source. And if we don't:

> +	if (!dst)
> +		return -1;

...we don't reply at all!

I think this could break things, and that we should change this
function (in this patch or in a separate patch, I don't have a strong
preference) to make sure we reply to the address that was used as
source, without calling tap_ip6_daddr() at all.

For that, you would need to reintroduce 'saddr' as argument I guess.

>  
>  	mh = IOV_REMOVE_HEADER(data, mh_storage);
>  	if (!mh)
> @@ -683,10 +690,7 @@ int dhcpv6(struct ctx *c, struct iov_tail *data,
>  
>  	resp.hdr.xid = mh->xid;
>  
> -	tap_udp6_send(c, src, 547, tap_ip6_daddr(c, src), 546,
> -		      mh->xid, &resp, n);
> -	if (a)
> -		c->ip6.addr_seen = a->addr.a6;
> +	tap_udp6_send(c, src, 547, dst, 546, mh->xid, &resp, n);
>  
>  	return 1;
>  }
> diff --git a/dhcpv6.h b/dhcpv6.h
> index c706dfd..eda133f 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);
> +	   const struct in6_addr *daddr);
>  void dhcpv6_init(const struct ctx *c);
>  
>  #endif /* DHCPV6_H */
> diff --git a/fwd.c b/fwd.c
> index 28a721e..b3f5dc0 100644
> --- a/fwd.c
> +++ b/fwd.c
> @@ -1095,14 +1095,6 @@ static bool fwd_guest_accessible(const struct ctx *c,
>  		if (inany_equals(addr, &a->addr))
>  			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) &&
> -	    inany_equals6(addr, &c->ip6.addr_seen))
> -		return false;
> -

Maybe it should be obvious for some reason, but if it's not this would
belong to the commit message: why can we skip this now?

>  	return true;
>  }
>  
> @@ -1305,10 +1297,20 @@ uint8_t fwd_nat_from_host(const struct ctx *c,
>  			}
>  			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 guest_addr *a;
> +
> +				a = fwd_select_addr(c, AF_INET6,
> +						    CONF_ADDR_OBSERVED,
> +						    CONF_ADDR_USER |
> +						    CONF_ADDR_HOST,
> +						    CONF_ADDR_LINKLOCAL);
> +				if (!a)
> +					return PIF_NONE;
> +				tgt->eaddr = a->addr;
> +			}
>  			tgt->oaddr = inany_any6;
>  		}
>  
> @@ -1346,10 +1348,16 @@ uint8_t fwd_nat_from_host(const struct ctx *c,
>  
>  		tgt->eaddr = a->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;

This should be uint8_t I guess.

> +		const struct guest_addr *a;
> +
> +		a = fwd_select_addr(c, AF_INET6, CONF_ADDR_OBSERVED,
> +				    CONF_ADDR_USER | CONF_ADDR_HOST, excl);
> +		if (!a)
> +			return PIF_NONE;
> +
> +		tgt->eaddr = a->addr;
>  	}
>  
>  	return PIF_TAP;
> diff --git a/inany.h b/inany.h
> index 7b23cb0..82dd102 100644
> --- a/inany.h
> +++ b/inany.h
> @@ -60,6 +60,9 @@ extern const union inany_addr inany_any4;
>  #define inany_from_v4(a4)	\
>  	((union inany_addr)INANY_INIT4((a4)))
>  
> +#define inany_from_v6(v6)	\
> +	((union inany_addr){ .a6 = (v6) })
> +
>  /** 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 1e02720..a92301b 100644
> --- a/migrate.c
> +++ b/migrate.c
> @@ -56,21 +56,30 @@ struct migrate_seen_addrs_v2 {
>  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 guest_addr *a;
>  
>  	(void)stage;
>  
> -	/* IPv4 observed address, with fallback to configured address */
> +	/* IPv4 observed address, with fallback to any other non-LL 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);
>  
> +	/* IPv6 observed address, with fallback to any other non-LL address */
> +	a = fwd_select_addr(c, AF_INET6, CONF_ADDR_OBSERVED,
> +			    CONF_ADDR_USER | CONF_ADDR_HOST,
> +			    CONF_ADDR_LINKLOCAL);
> +	if (a)
> +		addrs.addr6 = a->addr.a6;
> +
> +	/* IPv6 link-local address */
> +	a = fwd_get_addr(c, AF_INET6, CONF_ADDR_LINKLOCAL, 0);
> +	if (a)
> +		addrs.addr6_ll = a->addr.a6;
> +
>  	memcpy(addrs.mac, c->guest_mac, sizeof(addrs.mac));
>  
>  	if (write_all_buf(fd, &addrs, sizeof(addrs)))
> @@ -91,19 +100,27 @@ 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;
>  
>  	(void)stage;
>  
>  	if (read_all_buf(fd, &addrs, sizeof(addrs)))
>  		return errno;
>  
> -	c->ip6.addr_seen = addrs.addr6;
> -	c->ip6.addr_ll_seen = addrs.addr6_ll;
> -
>  	if (addrs.addr4.s_addr)
>  		fwd_set_addr(c, &inany_from_v4(addrs.addr4),
>  			     CONF_ADDR_OBSERVED, 0);
>  
> +	addr6 = addrs.addr6;
> +	if (!IN6_IS_ADDR_UNSPECIFIED(&addr6))

Nit, here and below: the Linux "net" coding style we adopt uses curly
brackets for single multi-line statements as well.

> +		fwd_set_addr(c, &inany_from_v6(addr6),
> +			     CONF_ADDR_OBSERVED, 0);
> +
> +	addr6_ll = addrs.addr6_ll;
> +	if (!IN6_IS_ADDR_UNSPECIFIED(&addr6_ll))
> +		fwd_set_addr(c, &inany_from_v6(addr6_ll),
> +			     CONF_ADDR_OBSERVED | CONF_ADDR_LINKLOCAL, 0);
> +
>  	memcpy(c->guest_mac, addrs.mac, sizeof(c->guest_mac));
>  
>  	return 0;
> diff --git a/passt.h b/passt.h
> index 5452225..db2f10d 100644
> --- a/passt.h
> +++ b/passt.h
> @@ -124,8 +124,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]
> @@ -141,8 +139,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 b8d7cf4..fcb169a 100644
> --- a/pasta.c
> +++ b/pasta.c
> @@ -370,6 +370,7 @@ static int pasta_conf_routes(struct ctx *c, sa_family_t af, int ifi,
>  void pasta_ns_conf(struct ctx *c)
>  {
>  	unsigned int flags = IFF_UP;
> +	struct in6_addr addr_ll;
>  	int rc;
>  
>  	rc = nl_link_set_flags(nl_sock_ns, 1 /* lo */, IFF_UP, IFF_UP);
> @@ -417,11 +418,13 @@ void pasta_ns_conf(struct ctx *c)
>  	if (!c->ifi6)
>  		goto done;
>  
> -	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
> +		fwd_set_addr(c, &inany_from_v6(addr_ll),
> +			     CONF_ADDR_LINKLOCAL | CONF_ADDR_HOST, 0);

Same here about curly brackets.

>  
>  	rc = nl_addr_set_ll_nodad(nl_sock_ns, c->pasta_ifi);
>  	if (rc < 0)
> diff --git a/tap.c b/tap.c
> index c75a4df..07f92bb 100644
> --- a/tap.c
> +++ b/tap.c
> @@ -173,19 +173,51 @@ 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

Should this have "check" in its name, then? Maybe tap_new_src_addr6() or
tap_seen_src_addr6() would fit better.

> + * @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)
> +{
> +	uint8_t flags = CONF_ADDR_OBSERVED;
> +
> +	if (IN6_IS_ADDR_UNSPECIFIED(addr))
> +		return;
> +
> +	if (IN6_IS_ADDR_LINKLOCAL(addr))
> +		flags |= CONF_ADDR_LINKLOCAL;
> +
> +	fwd_set_addr(c, &inany_from_v6(*addr), flags, 0);
> +}
> +
>  /**
>   * tap_ip6_daddr() - Normal IPv6 destination address for inbound packets
>   * @c:		Execution context
>   * @src:	Source address
>   *
> - * Return: pointer to IPv6 address
> + * Return: pointer to IPv6 address, NULL if no suitable address found
>   */
>  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 guest_addr *a;
> +
> +	if (IN6_IS_ADDR_LINKLOCAL(src)) {
> +		/* Link-local: first LL address in array */
> +		a = fwd_get_addr(c, AF_INET6, CONF_ADDR_LINKLOCAL, 0);
> +	} else {
> +		/* Global: observed non-LL first, then any non-LL */
> +		a = fwd_select_addr(c, AF_INET6, CONF_ADDR_OBSERVED,
> +				    CONF_ADDR_USER | CONF_ADDR_HOST,
> +				    CONF_ADDR_LINKLOCAL);
> +	}
> +
> +	if (a)
> +		return &a->addr.a6;
> +
> +	debug("No suitable IPv6 guest address found");

This lacks context. What for? What were we trying to do? I'd rather
print nothing here and add appropriate messages in callers.

> +	return NULL;
>  }
>  
>  /**
> @@ -958,21 +990,7 @@ 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 (!fwd_get_addr(c, AF_INET6, 0, 0)) {
> -				union inany_addr addr = { .a6 = *saddr };
> -
> -				fwd_set_addr(c, &addr, CONF_ADDR_LINKLOCAL, 64);
> -			}
> -		} else if (!IN6_IS_ADDR_UNSPECIFIED(saddr)){
> -			c->ip6.addr_seen = *saddr;
> -		}
> +		tap_check_src_addr6(c, saddr);
>  
>  		if (proto == IPPROTO_ICMPV6) {
>  			struct iov_tail ndp_data;
> @@ -1003,7 +1021,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;
>  		}
>  

...I'm still reviewing the rest.

-- 
Stefano