Re: [PATCH v6 12/13] dhcpv6: Select addresses for DHCPv6 distribution

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

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

On Sat, Mar 21, 2026 at 08:43:32PM -0400, Jon Maloy wrote:
> We introduce a CONF_ADDR_DHCP flag to mark if an added address is
> eligible for DHCP advertisement. By doing this once and for all

Copypasta from previous commit's message DHCP instead of DHCPv6.

> in the fwd_set_addr() function, the DHCPv6 code only needs to check
> for this flag to know that all criteria for advertisement are fulfilled.
> 
> We update the code in dhcpv6.c both to use the new flag and to make
> it possible to send multiple addresses in a single reply message,
> per RFC 8415.
> 
> We also let the conf_print() function use this flag to identify and
> print the eligible addresses.
> 
> Signed-off-by: Jon Maloy <jmaloy@redhat.com>
> 
> ---
> v6: -Refactored the DHCPv6 response structure to use a variable-length
>      buffer for IA_ADDR options, hopefully making this part of the code
>      slightly clearer.
> ---
>  conf.c    | 36 +++++++++++++++------
>  dhcpv6.c  | 97 ++++++++++++++++++++++++++++++++-----------------------
>  fwd.c     |  4 +++
>  migrate.c |  5 +++
>  passt.h   |  1 +
>  5 files changed, 94 insertions(+), 49 deletions(-)
> 
> diff --git a/conf.c b/conf.c
> index 512fa38..de2fb7c 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -1213,24 +1213,42 @@ static void conf_print(const struct ctx *c)
>  	}
>  
>  	if (c->ifi6) {
> +		bool has_dhcpv6 = false;
> +		const char *head;
> +
>  		if (!IN6_IS_ADDR_UNSPECIFIED(&c->ip6.map_host_loopback))
>  			info("    NAT to host ::1: %s",
>  			     inet_ntop(AF_INET6, &c->ip6.map_host_loopback,
>  				       buf, sizeof(buf)));
>  
> -		if (!c->no_ndp && !c->no_dhcpv6)
> -			info("NDP/DHCPv6:");
> -		else if (!c->no_dhcpv6)
> -			info("DHCPv6:");
> -		else if (!c->no_ndp)
> -			info("NDP:");
> -		else
> +		/* Check what we have to advertise */
> +		for_each_addr(a, c, AF_INET6) {
> +			if (a->flags & CONF_ADDR_DHCPV6)
> +				has_dhcpv6 = true;
> +		}
> +
> +		if (c->no_ndp && !has_dhcpv6)
>  			goto dns6;
>  
>  		a = fwd_get_addr(c, AF_INET6, 0, CONF_ADDR_LINKLOCAL);
> -		if (a)
> +		if (!c->no_ndp && a) {
> +			info("NDP:");
>  			inany_ntop(&a->addr, buf, sizeof(buf));
> -		info("    assign: %s", !a ? "" : buf);
> +			info("    assign: %s", buf);
> +		}
> +
> +		if (has_dhcpv6) {
> +			info("DHCPv6:");
> +			head = "assign";
> +			for_each_addr(a, c, AF_INET6) {
> +				if (!(a->flags & CONF_ADDR_DHCPV6))
> +					continue;
> +				inany_ntop(&a->addr, buf, sizeof(buf));
> +				info("    %s: %s/%d", head, buf, a->prefix_len);
> +				head = "      ";
> +			}
> +		}
> +
>  		inet_ntop(AF_INET6, &c->ip6.guest_gw, buf, sizeof(buf));
>  		info("    router: %s", buf);
>  		inet_ntop(AF_INET6, &c->ip6.our_tap_ll, buf, sizeof(buf));
> diff --git a/dhcpv6.c b/dhcpv6.c
> index 313c243..7c16da4 100644
> --- a/dhcpv6.c
> +++ b/dhcpv6.c
> @@ -31,6 +31,8 @@
>  #include "passt.h"
>  #include "tap.h"
>  #include "log.h"
> +#include "fwd.h"
> +#include "conf.h"
>  
>  /**
>   * struct opt_hdr - DHCPv6 option header
> @@ -202,56 +204,35 @@ struct msg_hdr {
>  	uint32_t xid:24;
>  } __attribute__((__packed__));
>  
> +/* Maximum variable part size: ia_addrs + client_id + dns + search + fqdn */
> +#define RESP_VAR_MAX	(MAX_GUEST_ADDRS * sizeof(struct opt_ia_addr) + \
> +			 sizeof(struct opt_client_id) + \
> +			 sizeof(struct opt_dns_servers) + \
> +			 sizeof(struct opt_dns_search) + \
> +			 sizeof(struct opt_client_fqdn))
> +
>  /**
>   * struct resp_t - Normal advertise and reply message
>   * @hdr:		DHCP message header
>   * @server_id:		Server Identifier option
>   * @ia_na:		Non-temporary Address option
> - * @ia_addr:		Address for IA_NA
> - * @client_id:		Client Identifier, variable length
> - * @dns_servers:	DNS Recursive Name Server, here just for storage size
> - * @dns_search:		Domain Search List, here just for storage size
> - * @client_fqdn:	Client FQDN, variable length
> + * @var:		Variable part: IA_ADDRs, client_id, dns, search, fqdn
>   */
>  static struct resp_t {
>  	struct msg_hdr hdr;
>  
>  	struct opt_server_id server_id;
>  	struct opt_ia_na ia_na;
> -	struct opt_ia_addr ia_addr;
> -	struct opt_client_id client_id;
> -	struct opt_dns_servers dns_servers;
> -	struct opt_dns_search dns_search;
> -	struct opt_client_fqdn client_fqdn;
> +	uint8_t var[RESP_VAR_MAX];
>  } __attribute__((__packed__)) resp = {
>  	{ 0 },
>  	SERVER_ID,
>  
> -	{ { OPT_IA_NA,		OPT_SIZE_CONV(sizeof(struct opt_ia_na) +
> -					      sizeof(struct opt_ia_addr) -
> -					      sizeof(struct opt_hdr)) },
> +	{ { OPT_IA_NA,		0 },  /* Length set dynamically */
>  	  1, (uint32_t)~0U, (uint32_t)~0U
>  	},
>  
> -	{ { OPT_IAAADR,		OPT_SIZE(ia_addr) },
> -	  IN6ADDR_ANY_INIT, (uint32_t)~0U, (uint32_t)~0U
> -	},
> -
> -	{ { OPT_CLIENTID,	0, },
> -	  { 0 }
> -	},
> -
> -	{ { OPT_DNS_SERVERS,	0, },
> -	  { IN6ADDR_ANY_INIT }
> -	},
> -
> -	{ { OPT_DNS_SEARCH,	0, },
> -	  { 0 },
> -	},
> -
> -	{ { OPT_CLIENT_FQDN, 0, },
> -	  0, { 0 },
> -	},
> +	{ 0 },  /* Variable part filled dynamically */
>  };
>  
>  static const struct opt_status_code sc_not_on_link = {
> @@ -543,6 +524,42 @@ static size_t dhcpv6_client_fqdn_fill(const struct iov_tail *data,
>  	return offset + sizeof(struct opt_hdr) + opt_len;
>  }
>  
> +/**
> + * dhcpv6_ia_addr_fill() - Fill IA_ADDR options for all suitable addresses
> + * @c:		Execution context
> + *
> + * Fills IA_ADDRs in resp.var with all non-linklocal, non-observed addresses

"non-observed" is no longer accurate, and this is potentially
misleading with the new flag based selection.

> + * and updates resp.ia_na.hdr.l with the correct length.
> + *
> + * Return: number of addresses filled
> + */
> +static int dhcpv6_ia_addr_fill(const struct ctx *c)
> +{
> +	struct opt_ia_addr *ia_addr = (struct opt_ia_addr *)resp.var;
> +	const struct guest_addr *e;
> +	int count = 0;
> +
> +	for_each_addr(e, c, AF_INET6) {
> +		if (!(e->flags & CONF_ADDR_DHCPV6))
> +			continue;
> +
> +		ia_addr[count].hdr.t = OPT_IAAADR;
> +		ia_addr[count].hdr.l = htons(sizeof(struct opt_ia_addr) -
> +					     sizeof(struct opt_hdr));
> +		ia_addr[count].addr = e->addr.a6;
> +		ia_addr[count].pref_lifetime = (uint32_t)~0U;
> +		ia_addr[count].valid_lifetime = (uint32_t)~0U;
> +		count++;
> +	}
> +
> +	/* Update IA_NA length: header fields + all IA_ADDRs */
> +	resp.ia_na.hdr.l = htons(sizeof(struct opt_ia_na) -
> +				 sizeof(struct opt_hdr) +
> +				 count * sizeof(struct opt_ia_addr));
> +
> +	return count;
> +}
> +
>  /**
>   * dhcpv6() - Check if this is a DHCPv6 message, reply as needed
>   * @c:		Execution context
> @@ -570,12 +587,14 @@ int dhcpv6(struct ctx *c, struct iov_tail *data,
>  	struct opt_hdr client_id_storage;
>  	/* cppcheck-suppress [variableScope,unmatchedSuppression] */
>  	const struct in6_addr *src, *dst;
> +	/* cppcheck-suppress [variableScope,unmatchedSuppression] */

You only need this because you unintentially moved the suppression
away from ia_storage in the earlier patch.

>  	struct opt_ia_na ia_storage;
>  	const struct guest_addr *a;
>  	struct msg_hdr mh_storage;
>  	const struct msg_hdr *mh;
>  	struct udphdr uh_storage;
>  	const struct udphdr *uh;
> +	int addr_count;
>  	size_t mlen, n;
>  
>  	a = fwd_get_addr(c, AF_INET6, 0, CONF_ADDR_LINKLOCAL);
> @@ -626,6 +645,7 @@ int dhcpv6(struct ctx *c, struct iov_tail *data,
>  	if (ia && ntohs(ia->hdr.l) < MIN(OPT_VSIZE(ia_na), OPT_VSIZE(ia_ta)))
>  		return -1;
>  
> +	addr_count = dhcpv6_ia_addr_fill(c);
>  	resp.hdr.type = TYPE_REPLY;
>  	switch (mh->type) {
>  	case TYPE_REQUEST:
> @@ -679,12 +699,14 @@ int dhcpv6(struct ctx *c, struct iov_tail *data,
>  	if (ia)
>  		resp.ia_na.iaid = ((struct opt_ia_na *)ia)->iaid;
>  
> +	/* Client_id goes right after the used IA_ADDRs */
> +	n = offsetof(struct resp_t, var) +
> +	    addr_count * sizeof(struct opt_ia_addr);
>  	iov_to_buf(&client_id_base.iov[0], client_id_base.cnt,
> -		   client_id_base.off, &resp.client_id,
> +		   client_id_base.off, (char *)&resp + n,
>  		   ntohs(client_id->l) + sizeof(struct opt_hdr));
>  
> -	n = offsetof(struct resp_t, client_id) +
> -	    sizeof(struct opt_hdr) + ntohs(client_id->l);
> +	n += sizeof(struct opt_hdr) + ntohs(client_id->l);
>  	n = dhcpv6_dns_fill(c, (char *)&resp, n);
>  	n = dhcpv6_client_fqdn_fill(data, c, (char *)&resp, n);
>  
> @@ -701,7 +723,6 @@ int dhcpv6(struct ctx *c, struct iov_tail *data,
>   */
>  void dhcpv6_init(const struct ctx *c)
>  {
> -	const struct guest_addr *a;
>  	time_t y2k = 946684800; /* Epoch to 2000-01-01T00:00:00Z, no mktime() */
>  	uint32_t duid_time;
>  
> @@ -714,8 +735,4 @@ void dhcpv6_init(const struct ctx *c)
>  	       c->our_tap_mac, sizeof(c->our_tap_mac));
>  	memcpy(resp_not_on_link.server_id.duid_lladdr,
>  	       c->our_tap_mac, sizeof(c->our_tap_mac));
> -
> -	a = fwd_get_addr(c, AF_INET6, 0, CONF_ADDR_LINKLOCAL);
> -	if (a)
> -		resp.ia_addr.addr = a->addr.a6;
>  }
> diff --git a/fwd.c b/fwd.c
> index e1c85dd..f867398 100644
> --- a/fwd.c
> +++ b/fwd.c
> @@ -301,6 +301,10 @@ void fwd_set_addr(struct ctx *c, const union inany_addr *addr,
>  		if (inany_v4(addr)) {
>  			if (!c->no_dhcp)
>  				flags |= CONF_ADDR_DHCP;
> +		} else {
> +			/* DHCPv6 for IPv6 */
> +			if (!c->no_dhcpv6)
> +				flags |= CONF_ADDR_DHCPV6;
>  		}
>  	}
>  
> diff --git a/migrate.c b/migrate.c
> index 1d1e0e6..105f624 100644
> --- a/migrate.c
> +++ b/migrate.c
> @@ -53,6 +53,7 @@ struct migrate_seen_addrs_v2 {
>  #define MIGRATE_ADDR_LINKLOCAL	BIT(2)
>  #define MIGRATE_ADDR_OBSERVED	BIT(3)
>  #define MIGRATE_ADDR_DHCP	BIT(4)
> +#define MIGRATE_ADDR_DHCPV6	BIT(5)
>  
>  /**
>   * struct migrate_addr_v3 - Wire format for a single address entry
> @@ -86,6 +87,8 @@ static uint8_t flags_to_wire(uint8_t flags)
>  		wire |= MIGRATE_ADDR_OBSERVED;
>  	if (flags & CONF_ADDR_DHCP)
>  		wire |= MIGRATE_ADDR_DHCP;
> +	if (flags & CONF_ADDR_DHCPV6)
> +		wire |= MIGRATE_ADDR_DHCPV6;
>  
>  	return wire;
>  }
> @@ -110,6 +113,8 @@ static uint8_t flags_from_wire(uint8_t wire)
>  		flags |= CONF_ADDR_OBSERVED;
>  	if (wire & MIGRATE_ADDR_DHCP)
>  		flags |= CONF_ADDR_DHCP;
> +	if (wire & MIGRATE_ADDR_DHCPV6)
> +		flags |= CONF_ADDR_DHCPV6;
>  
>  	return flags;
>  }
> diff --git a/passt.h b/passt.h
> index 5ea1715..c4c1f04 100644
> --- a/passt.h
> +++ b/passt.h
> @@ -76,6 +76,7 @@ enum passt_modes {
>  #define CONF_ADDR_LINKLOCAL	BIT(2)		/* Link-local address */
>  #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) */
>  
>  /**
>   * struct guest_addr - Unified IPv4/IPv6 address entry
> -- 
> 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 --]