Re: [PATCH v6 13/13] ndp: Support advertising multiple prefixes in Router Advertisements

[Stefano Brivio <sbrivio@redhat.com>]

On Sat, 21 Mar 2026 20:43:33 -0400
Jon Maloy <jmaloy@redhat.com> wrote:

> We extend NDP to advertise all suitable IPv6 prefixes in Router
> Advertisements, per RFC 4861. Observed and link-local addresses,
> plus addresses with a prefix length != 64, are excluded.
> 
> Signed-off-by: Jon Maloy <jmaloy@redhat.com>
> 
> ---
> v6: Adapted to previous changes in series
> ---
>  conf.c    |  19 ++++++---
>  fwd.c     |   3 ++
>  migrate.c |   5 +++
>  ndp.c     | 121 ++++++++++++++++++++++++++++++++++++------------------
>  passt.h   |   1 +
>  5 files changed, 103 insertions(+), 46 deletions(-)
> 
> diff --git a/conf.c b/conf.c
> index de2fb7c..b8dd40a 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -1213,7 +1213,7 @@ static void conf_print(const struct ctx *c)
>  	}
>  
>  	if (c->ifi6) {
> -		bool has_dhcpv6 = false;
> +		bool has_ndp = false, has_dhcpv6 = false;
>  		const char *head;
>  
>  		if (!IN6_IS_ADDR_UNSPECIFIED(&c->ip6.map_host_loopback))
> @@ -1223,18 +1223,25 @@ static void conf_print(const struct ctx *c)
>  
>  		/* Check what we have to advertise */
>  		for_each_addr(a, c, AF_INET6) {
> +			if (a->flags & CONF_ADDR_NDP)
> +				has_ndp = true;
>  			if (a->flags & CONF_ADDR_DHCPV6)
>  				has_dhcpv6 = true;
>  		}
>  
> -		if (c->no_ndp && !has_dhcpv6)
> +		if (!has_ndp && !has_dhcpv6)
>  			goto dns6;
>  
> -		a = fwd_get_addr(c, AF_INET6, 0, CONF_ADDR_LINKLOCAL);
> -		if (!c->no_ndp && a) {
> +		if (has_ndp) {
>  			info("NDP:");
> -			inany_ntop(&a->addr, buf, sizeof(buf));
> -			info("    assign: %s", buf);
> +			head = "assign";

Same comment as for 12/13.

> +			for_each_addr(a, c, AF_INET6) {
> +				if (!(a->flags & CONF_ADDR_NDP))
> +					continue;
> +				inany_ntop(&a->addr, buf, sizeof(buf));
> +				info("    %s: %s/%d", head, buf, a->prefix_len);

It's a bit redundant to have the prefix length here as a variable given
that it's always /64, but it might make the code clearer, so I don't
really have a preference.

> +				head = "      ";
> +			}
>  		}
>  
>  		if (has_dhcpv6) {
> diff --git a/fwd.c b/fwd.c
> index f867398..fe6e9d4 100644
> --- a/fwd.c
> +++ b/fwd.c
> @@ -305,6 +305,9 @@ void fwd_set_addr(struct ctx *c, const union inany_addr *addr,
>  			/* DHCPv6 for IPv6 */
>  			if (!c->no_dhcpv6)
>  				flags |= CONF_ADDR_DHCPV6;
> +			/* NDP/RA only if /64 prefix, as required for SLAAC */

It's not "only" in the sense of "NDP only", rather in the "only if"
sense, this could be less ambiguous.

Regardless of that, I don't think it's a requirement, because we can
derive a prefix (a subnet prefix, that is, /64) from an address with a
longer or shorter prefix length.

If we don't, we're actually going to break functionality as we can't
assume that the guest will always have a DHCPv6 client running (but the
host might, and that would typically mean a /128 address).

In the case of prefix delegation, you'll have a shorter prefix length
on the host, but that doesn't mean we should totally ignore that prefix
while configuring the guest.

> +			if (!c->no_ndp && prefix_len == 64)
> +				flags |= CONF_ADDR_NDP;
>  		}
>  	}
>  
> diff --git a/migrate.c b/migrate.c
> index 105f624..98c2d7e 100644
> --- a/migrate.c
> +++ b/migrate.c
> @@ -54,6 +54,7 @@ struct migrate_seen_addrs_v2 {
>  #define MIGRATE_ADDR_OBSERVED	BIT(3)
>  #define MIGRATE_ADDR_DHCP	BIT(4)
>  #define MIGRATE_ADDR_DHCPV6	BIT(5)
> +#define MIGRATE_ADDR_NDP	BIT(6)

Nit: maybe we should call this SLAAC as it's the specific address
assignment mechanism (saying NDP is a bit like saying "UDP" to imply
DHCP). NDP is perfectly clear though.

>  
>  /**
>   * struct migrate_addr_v3 - Wire format for a single address entry
> @@ -89,6 +90,8 @@ static uint8_t flags_to_wire(uint8_t flags)
>  		wire |= MIGRATE_ADDR_DHCP;
>  	if (flags & CONF_ADDR_DHCPV6)
>  		wire |= MIGRATE_ADDR_DHCPV6;
> +	if (flags & CONF_ADDR_NDP)
> +		wire |= MIGRATE_ADDR_NDP;
>  
>  	return wire;
>  }
> @@ -115,6 +118,8 @@ static uint8_t flags_from_wire(uint8_t wire)
>  		flags |= CONF_ADDR_DHCP;
>  	if (wire & MIGRATE_ADDR_DHCPV6)
>  		flags |= CONF_ADDR_DHCPV6;
> +	if (wire & MIGRATE_ADDR_NDP)
> +		flags |= CONF_ADDR_NDP;
>  
>  	return flags;
>  }
> diff --git a/ndp.c b/ndp.c
> index 3750fc5..f47286e 100644
> --- a/ndp.c
> +++ b/ndp.c
> @@ -32,6 +32,8 @@
>  #include "passt.h"
>  #include "tap.h"
>  #include "log.h"
> +#include "fwd.h"
> +#include "conf.h"
>  
>  #define	RT_LIFETIME	65535
>  
> @@ -82,7 +84,7 @@ struct ndp_na {
>  } __attribute__((packed));
>  
>  /**
> - * struct opt_prefix_info - Prefix Information option
> + * struct opt_prefix_info - Prefix Information option header

This looks unrelated.

>   * @header:		Option header
>   * @prefix_len:		The number of leading bits in the Prefix that are valid
>   * @prefix_flags:	Flags associated with the prefix
> @@ -99,6 +101,16 @@ struct opt_prefix_info {
>  	uint32_t reserved;
>  } __attribute__((packed));
>  
> +/**
> + * struct ndp_prefix - Complete Prefix Information option with prefix

If it's complete, it'd better have a prefix...

> + * @info:		Prefix Information option header
> + * @prefix:		IPv6 prefix
> + */
> +struct ndp_prefix {
> +	struct opt_prefix_info info;
> +	struct in6_addr prefix;
> +} __attribute__((__packed__));
> +
>  /**
>   * struct opt_mtu - Maximum transmission unit (MTU) option
>   * @header:		Option header
> @@ -140,27 +152,23 @@ struct opt_dnssl {
>  } __attribute__((packed));
>  
>  /**
> - * struct ndp_ra - NDP Router Advertisement (RA) message
> + * struct ndp_ra_hdr - NDP Router Advertisement fixed header
>   * @ih:			ICMPv6 header
>   * @reachable:		Reachability time, after confirmation (ms)
>   * @retrans:		Time between retransmitted NS messages (ms)
> - * @prefix_info:	Prefix Information option
> - * @prefix:		IPv6 prefix
> - * @mtu:		MTU option
> - * @source_ll:		Target link-layer address
> - * @var:		Variable fields
>   */
> -struct ndp_ra {
> +struct ndp_ra_hdr {
>  	struct icmp6hdr ih;
>  	uint32_t reachable;
>  	uint32_t retrans;
> -	struct opt_prefix_info prefix_info;
> -	struct in6_addr prefix;
> -	struct opt_l2_addr source_ll;
> +} __attribute__((__packed__));
>  
> -	unsigned char var[sizeof(struct opt_mtu) + sizeof(struct opt_rdnss) +
> -			  sizeof(struct opt_dnssl)];
> -} __attribute__((packed, aligned(__alignof__(struct in6_addr))));
> +/* Maximum RA message size: hdr + prefixes + source_ll + mtu + rdnss + dnssl */
> +#define NDP_RA_MAX_SIZE	(sizeof(struct ndp_ra_hdr) + \
> +			 MAX_GUEST_ADDRS * sizeof(struct ndp_prefix) + \
> +			 sizeof(struct opt_l2_addr) + \
> +			 sizeof(struct opt_mtu) + sizeof(struct opt_rdnss) + \
> +			 sizeof(struct opt_dnssl))
>  
>  /**
>   * struct ndp_ns - NDP Neighbor Solicitation (NS) message
> @@ -231,6 +239,42 @@ void ndp_unsolicited_na(const struct ctx *c, const struct in6_addr *addr)
>  		ndp_na(c, &in6addr_ll_all_nodes, addr);
>  }
>  
> +/**
> + * ndp_prefix_fill() - Fill prefix options for all suitable addresses
> + * @c:		Execution context
> + * @buf:	Buffer to write prefix options into
> + *
> + * Fills buffer with Prefix Information options for all non-linklocal,
> + * non-observed addresses with prefix_len == 64 (required for SLAAC).

Not really... the only requirement is that we _offer_ a subnet prefix,
not that we have one.

> + *
> + * Return: number of bytes written
> + */
> +static size_t ndp_prefix_fill(const struct ctx *c, unsigned char *buf)
> +{
> +	const struct guest_addr *a;
> +	struct ndp_prefix *p;
> +	size_t offset = 0;
> +
> +	for_each_addr(a, c, AF_INET6) {
> +		if (!(a->flags & CONF_ADDR_NDP))
> +			continue;
> +
> +		p = (struct ndp_prefix *)(buf + offset);
> +		p->info.header.type = OPT_PREFIX_INFO;
> +		p->info.header.len = 4;  /* 4 * 8 = 32 bytes */
> +		p->info.prefix_len = 64;
> +		p->info.prefix_flags = 0xc0;  /* L, A flags */
> +		p->info.valid_lifetime = ~0U;
> +		p->info.pref_lifetime = ~0U;
> +		p->info.reserved = 0;
> +		p->prefix = a->addr.a6;
> +
> +		offset += sizeof(struct ndp_prefix);
> +	}
> +
> +	return offset;
> +}
> +
>  /**
>   * ndp_ra() - Send an NDP Router Advertisement (RA) message
>   * @c:		Execution context
> @@ -238,7 +282,15 @@ void ndp_unsolicited_na(const struct ctx *c, const struct in6_addr *addr)
>   */
>  static void ndp_ra(const struct ctx *c, const struct in6_addr *dst)
>  {
> -	struct ndp_ra ra = {
> +	unsigned char buf[NDP_RA_MAX_SIZE]
> +		__attribute__((__aligned__(__alignof__(struct in6_addr))));
> +	struct ndp_ra_hdr *hdr = (struct ndp_ra_hdr *)buf;
> +	struct opt_l2_addr *source_ll;
> +	unsigned char *ptr;
> +	size_t prefix_len;
> +
> +	/* Build RA header */
> +	*hdr = (struct ndp_ra_hdr){
>  		.ih = {
>  			.icmp6_type		= RA,
>  			.icmp6_code		= 0,
> @@ -247,31 +299,22 @@ static void ndp_ra(const struct ctx *c, const struct in6_addr *dst)
>  			.icmp6_rt_lifetime	= htons_constant(RT_LIFETIME),
>  			.icmp6_addrconf_managed	= 1,
>  		},
> -		.prefix_info = {
> -			.header = {
> -				.type		= OPT_PREFIX_INFO,
> -				.len		= 4,
> -			},
> -			.prefix_len		= 64,
> -			.prefix_flags		= 0xc0,	/* prefix flags: L, A */
> -			.valid_lifetime		= ~0U,
> -			.pref_lifetime		= ~0U,
> -		},
> -		.source_ll = {
> -			.header = {
> -				.type		= OPT_SRC_L2_ADDR,
> -				.len		= 1,
> -			},
> -		},
>  	};
> -	const struct guest_addr *a = fwd_get_addr(c, AF_INET6, 0, 0);
> -	unsigned char *ptr = NULL;
> -
> -	ASSERT(a);
>  
> -	ra.prefix = a->addr.a6;
> +	/* Fill prefix options */
> +	prefix_len = ndp_prefix_fill(c, (unsigned char *)(hdr + 1));
> +	if (prefix_len == 0) {
> +		/* No suitable prefixes to advertise */
> +		return;
> +	}
>  
> -	ptr = &ra.var[0];
> +	/* Add source link-layer address option */

Commit c16141eda5e8 ("ndp.c: Turn NDP responder into more declarative
implementation") finally turned this into something more readable, and
I think we could keep that by assigning this to a struct and then
copying the whole thing into the message.

By the way nothing should prevent us from moving this as first option
and keeping the fields assigned in the initialiser.

> +	ptr = (unsigned char *)(hdr + 1) + prefix_len;
> +	source_ll = (struct opt_l2_addr *)ptr;
> +	source_ll->header.type = OPT_SRC_L2_ADDR;
> +	source_ll->header.len = 1;
> +	memcpy(source_ll->mac, c->our_tap_mac, ETH_ALEN);
> +	ptr += sizeof(struct opt_l2_addr);
>  
>  	if (c->mtu) {
>  		struct opt_mtu *mtu = (struct opt_mtu *)ptr;
> @@ -345,10 +388,8 @@ static void ndp_ra(const struct ctx *c, const struct in6_addr *dst)
>  		}
>  	}
>  
> -	memcpy(&ra.source_ll.mac, c->our_tap_mac, ETH_ALEN);
> -
>  	/* NOLINTNEXTLINE(clang-analyzer-security.PointerSub) */
> -	ndp_send(c, dst, &ra, ptr - (unsigned char *)&ra);
> +	ndp_send(c, dst, buf, ptr - buf);
>  }
>  
>  /**
> diff --git a/passt.h b/passt.h
> index c4c1f04..c6f4406 100644
> --- a/passt.h
> +++ b/passt.h
> @@ -77,6 +77,7 @@ enum passt_modes {
>  #define CONF_ADDR_OBSERVED	BIT(3)		/* Seen in guest traffic */
>  #define CONF_ADDR_DHCP		BIT(4)		/* Advertise via DHCP (IPv4) */
>  #define CONF_ADDR_DHCPV6	BIT(5)		/* Advertise via DHCPv6 (IPv6) */
> +#define CONF_ADDR_NDP		BIT(6)		/* Advertise via NDP/RA (IPv6, /64) */

Same as above, I think "SLAAC" is more fitting here (even though "NDP"
is perfectly clear).

>  
>  /**
>   * struct guest_addr - Unified IPv4/IPv6 address entry

-- 
Stefano