Re: [PATCH v3 11/11] conf: Select addresses for DHCP and NDP distribution

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

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

On Fri, Jan 30, 2026 at 04:44:47PM -0500, Jon Maloy wrote:
> As a part of the multiple address support feature, we add a new
> algorithm for DHCP/DHCPv6/NDP address selection and refactor the
> conf_print() function accordingly.
> 
> The fwd_first_usable_addr() and fwd_select_addr() functions now take
> a context pointer and address family parameter to work with the
> unified address array using for_each_addr().
> 
> Signed-off-by: Jon Maloy <jmaloy@redhat.com>
> ---
>  conf.c   | 60 +++++++++++++++++++++++++----------------
>  dhcp.c   | 17 +++++++++---
>  dhcp.h   |  2 +-
>  dhcpv6.c | 17 ++++++++++--
>  fwd.c    | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  fwd.h    |  6 +++++
>  ndp.c    | 15 ++++++++---
>  ndp.h    |  4 +--
>  passt.h  |  3 +++
>  9 files changed, 171 insertions(+), 35 deletions(-)
> 
> diff --git a/conf.c b/conf.c
> index 6892cea..4c53d33 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -46,6 +46,7 @@
>  #include "lineread.h"
>  #include "isolation.h"
>  #include "log.h"
> +#include "fwd.h"
>  #include "vhost_user.h"
>  
>  #define NETNS_RUN_DIR	"/run/netns"
> @@ -1099,10 +1100,12 @@ enum passt_modes conf_mode(int argc, char *argv[])
>  static void conf_print(const struct ctx *c)
>  {
>  	char buf4[INET_ADDRSTRLEN], buf6[INET6_ADDRSTRLEN];
> -	char bufmac[ETH_ADDRSTRLEN], ifn[IFNAMSIZ];
> +	char bufmac[ETH_ADDRSTRLEN];
>  	int i;
>  
>  	if (c->ifi4 > 0 || c->ifi6 > 0) {
> +		char ifn[IFNAMSIZ];
> +
>  		info("Template interface: %s%s%s%s%s",
>  		     c->ifi4 > 0 ? if_indextoname(c->ifi4, ifn) : "",
>  		     c->ifi4 > 0 ? " (IPv4)" : "",
> @@ -1144,20 +1147,22 @@ static void conf_print(const struct ctx *c)
>  				       buf4, sizeof(buf4)));
>  
>  		if (!c->no_dhcp) {
> -			struct inany_addr_entry *e = first_v4(c);
> -			uint32_t mask;
> -
> -			mask = IN4_MASK(e->prefix_len);
> -
> -			info("DHCP:");
> -			info("    assign: %s",
> -			     inet_ntop(AF_INET, inany_v4(&e->addr),
> -				       buf4, sizeof(buf4)));
> -			info("    mask: %s",
> -			     inet_ntop(AF_INET, &mask,        buf4, sizeof(buf4)));
> -			info("    router: %s",
> -			     inet_ntop(AF_INET, &c->ip4.guest_gw,
> -				       buf4, sizeof(buf4)));
> +			const struct inany_addr_entry *e;
> +
> +			e = fwd_first_usable_addr(c, AF_INET,
> +						  INANY_ADDR_OBSERVED);

The fact that INANY_ADDR_OBSERVER is things to skip rather than things
to look for is not obvious here at the call site.

> +			if (e) {
> +				uint32_t mask = IN4_MASK(e->prefix_len);
> +
> +				info("DHCP:");
> +				info("    assign: %s",
> +				     inany_ntop(&e->addr, buf4, sizeof(buf4)));
> +				info("    mask: %s",
> +				     inet_ntop(AF_INET, &mask, buf4, sizeof(buf4)));
> +				info("    router: %s",
> +				     inet_ntop(AF_INET, &c->ip4.guest_gw,
> +					       buf4, sizeof(buf4)));
> +			}
>  		}
>  
>  		for (i = 0; !IN4_IS_ADDR_UNSPECIFIED(&c->ip4.dns[i]); i++) {
> @@ -1189,14 +1194,23 @@ static void conf_print(const struct ctx *c)
>  		else
>  			goto dns6;
>  
> -		info("    assign: %s",
> -		     inet_ntop(AF_INET6, &first_v6(c)->addr.a6,
> -			       buf6, sizeof(buf6)));
> -		info("    router: %s",
> -		     inet_ntop(AF_INET6, &c->ip6.guest_gw, buf6, sizeof(buf6)));
> -		info("    our link-local: %s",
> -		     inet_ntop(AF_INET6, &c->ip6.our_tap_ll,
> -			       buf6, sizeof(buf6)));
> +	{
> +			const struct inany_addr_entry *e;
> +
> +			e = fwd_first_usable_addr(c, AF_INET6,
> +						  INANY_ADDR_OBSERVED |
> +						  INANY_ADDR_LINKLOCAL);
> +			if (e) {
> +				info("    assign: %s",
> +				     inany_ntop(&e->addr, buf6, sizeof(buf6)));
> +				info("    router: %s",
> +				     inet_ntop(AF_INET6, &c->ip6.guest_gw,
> +					       buf6, sizeof(buf6)));
> +				info("    our link-local: %s",
> +				     inet_ntop(AF_INET6, &c->ip6.our_tap_ll,
> +					       buf6, sizeof(buf6)));
> +			}
> +		}
>  
>  dns6:
>  		for (i = 0; !IN6_IS_ADDR_UNSPECIFIED(&c->ip6.dns[i]); i++) {
> diff --git a/dhcp.c b/dhcp.c
> index 933a8cf..31ea4f1 100644
> --- a/dhcp.c
> +++ b/dhcp.c
> @@ -31,6 +31,7 @@
>  #include "passt.h"
>  #include "tap.h"
>  #include "log.h"
> +#include "fwd.h"
>  #include "dhcp.h"
>  
>  /**
> @@ -300,8 +301,9 @@ static void opt_set_dns_search(const struct ctx *c, size_t max_len)
>   *
>   * Return: 0 if it's not a DHCP message, 1 if handled, -1 on failure
>   */
> -int dhcp(const struct ctx *c, struct iov_tail *data)
> +int dhcp(struct ctx *c, struct iov_tail *data)
>  {
> +	union inany_addr yiaddr = { 0 };
>  	char macstr[ETH_ADDRSTRLEN];
>  	size_t mlen, dlen, opt_len;
>  	struct in_addr mask, dst;
> @@ -314,7 +316,7 @@ int dhcp(const struct ctx *c, struct iov_tail *data)
>  	struct msg m_storage;
>  	struct msg const *m;
>  	struct msg reply;
> -	unsigned int i;
> +	int i;
>  
>  	eh = IOV_REMOVE_HEADER(data, eh_storage);
>  	iph = IOV_PEEK_HEADER(data, iph_storage);
> @@ -344,6 +346,15 @@ int dhcp(const struct ctx *c, struct iov_tail *data)
>  	    m->op != BOOTREQUEST)
>  		return -1;
>  
> +	/* Select address to offer */
> +	{
> +		const struct inany_addr_entry *e;
> +
> +		e = fwd_select_addr(c, AF_INET, INANY_ADDR_DHCP,
> +				    INANY_ADDR_OBSERVED);

The fact we're using a different call here, where we actually select
the address, from the call where we print the address to use seems
fragile.

> +		if (e)
> +			yiaddr = e->addr;

.. and if !e?  I think you want to not respond to the DHCP at all,
rather than reply with an unspecified address.

> +	}
>  	reply.op		= BOOTREPLY;
>  	reply.htype		= m->htype;
>  	reply.hlen		= m->hlen;
> @@ -352,7 +363,7 @@ int dhcp(const struct ctx *c, struct iov_tail *data)
>  	reply.secs		= 0;
>  	reply.flags		= m->flags;
>  	reply.ciaddr		= m->ciaddr;
> -	reply.yiaddr		= *inany_v4(&first_v4(c)->addr);
> +	reply.yiaddr		= *inany_v4(&yiaddr);
>  	reply.siaddr		= 0;
>  	reply.giaddr		= m->giaddr;
>  	memcpy(&reply.chaddr,	m->chaddr,	sizeof(reply.chaddr));
> diff --git a/dhcp.h b/dhcp.h
> index cd50c99..7326c7d 100644
> --- a/dhcp.h
> +++ b/dhcp.h
> @@ -6,7 +6,7 @@
>  #ifndef DHCP_H
>  #define DHCP_H
>  
> -int dhcp(const struct ctx *c, struct iov_tail *data);
> +int dhcp(struct ctx *c, struct iov_tail *data);
>  void dhcp_init(void);
>  
>  #endif /* DHCP_H */
> diff --git a/dhcpv6.c b/dhcpv6.c
> index 67307d9..32494e9 100644
> --- a/dhcpv6.c
> +++ b/dhcpv6.c
> @@ -31,6 +31,7 @@
>  #include "passt.h"
>  #include "tap.h"
>  #include "log.h"
> +#include "fwd.h"
>  
>  /**
>   * struct opt_hdr - DHCPv6 option header
> @@ -564,6 +565,7 @@ int dhcpv6(struct ctx *c, struct iov_tail *data,
>  	struct opt_hdr client_id_storage;
>  	/* cppcheck-suppress [variableScope,unmatchedSuppression] */
>  	struct opt_ia_na ia_storage;
> +	struct in6_addr *ia_addr = NULL;
>  	const struct in6_addr *src;
>  	struct msg_hdr mh_storage;
>  	const struct msg_hdr *mh;
> @@ -612,6 +614,17 @@ 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;
>  
> +	/* Select address to assign */
> +	{
> +		struct inany_addr_entry *e;
> +
> +		e = fwd_select_addr(c, AF_INET6, INANY_ADDR_DHCP6,
> +				    INANY_ADDR_OBSERVED | INANY_ADDR_LINKLOCAL);
> +		if (e) {
> +			ia_addr = &e->addr.a6;
> +			resp.ia_addr.addr = *ia_addr;
> +		}
> +	}

Same comments as for DHCPv4.   But more generally, this seems like a
lot of work for an interim step.  Why not go straight to advertising
all the addresses with DHCPv6?

>  	resp.hdr.type = TYPE_REPLY;
>  	switch (mh->type) {
>  	case TYPE_REQUEST:
> @@ -624,7 +637,7 @@ int dhcpv6(struct ctx *c, struct iov_tail *data,
>  		if (mh->type == TYPE_CONFIRM && server_id)
>  			return -1;
>  
> -		if (dhcpv6_ia_notonlink(data, &first_v6(c)->addr.a6)) {
> +		if (dhcpv6_ia_notonlink(data, ia_addr)) {
>  
>  			dhcpv6_send_ia_notonlink(c, data, &client_id_base,
>  						 ntohs(client_id->l), mh->xid);
> @@ -701,5 +714,5 @@ void dhcpv6_init(const struct ctx *c)
>  	memcpy(resp_not_on_link.server_id.duid_lladdr,
>  	       c->our_tap_mac, sizeof(c->our_tap_mac));
>  
> -	resp.ia_addr.addr	= first_v6(c)->addr.a6;
> +	/* Address is set dynamically in dhcpv6() */
>  }
> diff --git a/fwd.c b/fwd.c
> index fc26eba..e41ae9b 100644
> --- a/fwd.c
> +++ b/fwd.c
> @@ -491,6 +491,88 @@ static bool is_dns_flow(uint8_t proto, const struct flowside *ini)
>  		((ini->oport == 53) || (ini->oport == 853));
>  }
>  
> +/**
> + * fwd_first_usable_addr() - Find first assignable address (read-only)
> + * @c:		Execution context
> + * @af:		Address family (AF_INET or AF_INET6)
> + * @skip:	Flags to skip (e.g., INANY_ADDR_LINKLOCAL | INANY_ADDR_OBSERVED)
> + *
> + * Prefers CONFIGURED over HOST addresses.

Not sure we explicitly need this.  CONFIGURED addresses should
generally go before HOST addresses anyway.  Once we allow dynamic
reconfiguration of addresses we'll need to re-examine our priority
policies anyway.  I think it will be cleaner to control priority by
controlling the order we insert / update them, rather than
prioritising when we look up.

> + * Return: pointer to first usable address, or NULL
> + */
> +const struct inany_addr_entry *
> +fwd_first_usable_addr(const struct ctx *c, int af, int skip)
> +{
> +	const struct inany_addr_entry *e;
> +
> +	for_each_addr(c, e, af) {
> +		if (e->flags & skip)
> +			continue;
> +		if (e->flags & INANY_ADDR_CONFIGURED)
> +			return e;
> +	}
> +
> +	for_each_addr(c, e, af) {
> +		if (e->flags & skip)
> +			continue;
> +		if (e->flags & INANY_ADDR_HOST)
> +			return e;
> +	}
> +
> +	return NULL;
> +}
> +
> +/**
> + * fwd_select_addr() - Select address for protocol assignment (DHCP/NDP/DHCPv6)
> + * @c:		Execution context
> + * @af:		Address family (AF_INET or AF_INET6)
> + * @assign_flag: Flag to set on selected address (INANY_ADDR_DHCP, etc.)
> + * @skip:	Flags to skip (e.g., INANY_ADDR_LINKLOCAL | INANY_ADDR_OBSERVED)
> + *
> + * Selection priority:
> + *   1. Address already having @assign_flag set
> + *   2. Address with INANY_ADDR_CONFIGURED (user -a option)
> + *   3. Address with INANY_ADDR_HOST (from template interface)
> + *
> + * Return: pointer to selected address entry, or NULL if none found
> + */
> +struct inany_addr_entry *fwd_select_addr(struct ctx *c, int af,
> +					 int assign_flag, int skip)
> +{
> +	struct inany_addr_entry *e;
> +
> +	/* First: find address already having assign_flag */
> +	for_each_addr(c, e, af) {
> +		if (e->flags & skip)
> +			continue;
> +		if (e->flags & assign_flag)
> +			return e;
> +	}
> +
> +	/* Second: find CONFIGURED address, set assign_flag */
> +	for_each_addr(c, e, af) {
> +		if (e->flags & skip)
> +			continue;
> +		if (e->flags & INANY_ADDR_CONFIGURED) {
> +			e->flags |= assign_flag;

What's the value of tagging with a persistent flag?

> +			return e;
> +		}
> +	}
> +
> +	/* Third: find HOST address, set assign_flag */
> +	for_each_addr(c, e, af) {
> +		if (e->flags & skip)
> +			continue;
> +		if (e->flags & INANY_ADDR_HOST) {
> +			e->flags |= assign_flag;
> +			return e;
> +		}
> +	}
> +
> +	return NULL;
> +}
> +
>  /**
>   * fwd_guest_addr4() - Get first observed IPv4 guest address
>   * @c:		Execution context
> diff --git a/fwd.h b/fwd.h
> index 7792582..91b15dc 100644
> --- a/fwd.h
> +++ b/fwd.h
> @@ -9,6 +9,7 @@
>  
>  union inany_addr;
>  struct flowside;
> +struct inany_addr_entry;
>  
>  /* Number of ports for both TCP and UDP */
>  #define	NUM_PORTS	(1U << 16)
> @@ -63,4 +64,9 @@ void fwd_neigh_mac_get(const struct ctx *c, const union inany_addr *addr,
>  		       uint8_t *mac);
>  void fwd_neigh_table_init(const struct ctx *c);
>  
> +const struct inany_addr_entry *fwd_first_usable_addr(const struct ctx *c,
> +						     int af, int skip);
> +struct inany_addr_entry *fwd_select_addr(struct ctx *c, int af,
> +					 int assign_flag, int skip);
> +
>  #endif /* FWD_H */
> diff --git a/ndp.c b/ndp.c
> index f60c298..2211f86 100644
> --- a/ndp.c
> +++ b/ndp.c
> @@ -32,6 +32,7 @@
>  #include "passt.h"
>  #include "tap.h"
>  #include "log.h"
> +#include "fwd.h"
>  
>  #define	RT_LIFETIME	65535
>  
> @@ -236,7 +237,7 @@ void ndp_unsolicited_na(const struct ctx *c, const struct in6_addr *addr)
>   * @c:		Execution context
>   * @dst:	IPv6 address to send the RA to
>   */
> -static void ndp_ra(const struct ctx *c, const struct in6_addr *dst)
> +static void ndp_ra(struct ctx *c, const struct in6_addr *dst)
>  {
>  	struct ndp_ra ra = {
>  		.ih = {
> @@ -257,7 +258,7 @@ static void ndp_ra(const struct ctx *c, const struct in6_addr *dst)
>  			.valid_lifetime		= ~0U,
>  			.pref_lifetime		= ~0U,
>  		},
> -		.prefix = first_v6(c)->addr.a6,
> +		.prefix = IN6ADDR_ANY_INIT,
>  		.source_ll = {
>  			.header = {
>  				.type		= OPT_SRC_L2_ADDR,
> @@ -265,8 +266,14 @@ static void ndp_ra(const struct ctx *c, const struct in6_addr *dst)
>  			},
>  		},
>  	};
> +	struct inany_addr_entry *e;
>  	unsigned char *ptr = NULL;
>  
> +	/* Select address/prefix to advertise */
> +	e = fwd_select_addr(c, AF_INET6, INANY_ADDR_NDP, INANY_ADDR_LINKLOCAL);
> +	if (e)
> +		ra.prefix = e->addr.a6;

As for DHCP if !e we just shouldn't send this RA.

As for DHPCPv6, having this interim step seems awkward - we want to
send RAs for all the addresses.

Btw, now that we're actually tracking prefix length, we should
probably avoid RAs if prefix_len != 64 - SLAAC address configuration
won't work if that's not the case.

> +
>  	ptr = &ra.var[0];
>  
>  	if (c->mtu) {
> @@ -353,7 +360,7 @@ static void ndp_ra(const struct ctx *c, const struct in6_addr *dst)
>   *
>   * Return: 0 if not handled here, 1 if handled, -1 on failure
>   */
> -int ndp(const struct ctx *c, const struct in6_addr *saddr,
> +int ndp(struct ctx *c, const struct in6_addr *saddr,
>  	struct iov_tail *data)
>  {
>  	struct icmp6hdr ih_storage;
> @@ -407,7 +414,7 @@ static time_t next_ra;
>   * @c:		Execution context
>   * @now:	Current (monotonic) time
>   */
> -void ndp_timer(const struct ctx *c, const struct timespec *now)
> +void ndp_timer(struct ctx *c, const struct timespec *now)
>  {
>  	time_t max_rtr_adv_interval = DEFAULT_MAX_RTR_ADV_INTERVAL;
>  	time_t min_rtr_adv_interval, interval;
> diff --git a/ndp.h b/ndp.h
> index 56b756d..726aef6 100644
> --- a/ndp.h
> +++ b/ndp.h
> @@ -8,9 +8,9 @@
>  
>  struct icmp6hdr;
>  
> -int ndp(const struct ctx *c, const struct in6_addr *saddr,
> +int ndp(struct ctx *c, const struct in6_addr *saddr,
>  	struct iov_tail *data);
> -void ndp_timer(const struct ctx *c, const struct timespec *now);
> +void ndp_timer(struct ctx *c, const struct timespec *now);
>  void ndp_send_init_req(const struct ctx *c);
>  void ndp_unsolicited_na(const struct ctx *c, const struct in6_addr *addr);
>  
> diff --git a/passt.h b/passt.h
> index 7040781..f5f8c68 100644
> --- a/passt.h
> +++ b/passt.h
> @@ -71,6 +71,9 @@ enum passt_modes {
>  #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 */
> +#define INANY_ADDR_DHCP		BIT(4)		/* IPv4: assigned via DHCP */
> +#define INANY_ADDR_DHCP6	BIT(5)		/* IPv6: assigned via DHCPv6 */
> +#define INANY_ADDR_NDP		BIT(6)		/* IPv6: prefix via NDP/RA */
>  
>  /**
>   * for_each_addr() - Iterate over addresses in unified array
> -- 
> 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 --]