Re: [PATCH v8 01/14] dhcpv6: Fix reply destination to match client's source address

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

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

On Thu, Jun 25, 2026 at 10:45:06PM -0400, Jon Maloy wrote:
> tap_ip6_daddr() selects the reply destination based on our source
> address type (link-local), so it always returns addr_ll_seen. But if
> the client sent from a global address, we would reply to an address
> different from what the client is expecting. Since RFC 9915 (section
> 18.3.10) allows clients to use global addresses for DHCPv6, we now
> correct this, and always respond to the address the client was using.
> 
> We also remove a redundant addr_ll_seen assignment, since this is
> already done by tap.c when processing IPv6 packets.
> 
> Note: if the client uses a global source address, our reply will
> still have a link-local source, creating a scope mismatch. Fixing
> this properly would require a mechanism to allocate a global address
> for the DHCPv6 server, which we currently don't have. Responding to
> the client's actual source address is still a net improvement over the
> previous behavior of replying to an unrelated cached address.

Right.  This is a bit messy - mixing scopes can work in some
circumstances, but exactly when is pretty tricky to figure out.
Nonetheless I agree that this approach makes more sense than the
existing one - and also gets us a step closer to allowing our DHCP
server to answer queries from multiple clients.

> Note 2: This commit isn't actually a fix to an observed problem, but
> rather an answer to a theoretical issue, adding completeness to the
> mechanism and simplifying subsequent changes in this series.
> 
> Signed-off-by: Jon Maloy <jmaloy@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> 
> ---
> v8: -Updated commit log, acknowledging concerns expressed by David and
>      Stefano.
>     -Some minor changes also addressing feedback from the same persons.
> ---
>  dhcpv6.c | 16 ++++++++--------
>  dhcpv6.h |  2 +-
>  tap.c    | 15 ---------------
>  tap.h    |  2 --
>  4 files changed, 9 insertions(+), 26 deletions(-)
> 
> diff --git a/dhcpv6.c b/dhcpv6.c
> index 97c04e2c..29c7e320 100644
> --- a/dhcpv6.c
> +++ b/dhcpv6.c
> @@ -370,12 +370,14 @@ notonlink:
>  /**
>   * dhcpv6_send_ia_notonlink() - Send NotOnLink status
>   * @c:			Execution context
> + * @caddr:		Source address of client message (reply destination)
>   * @ia_base:		Non-appropriate IA_NA or IA_TA base
>   * @client_id_base:	Client ID message option base
>   * @len:		Client ID length
>   * @xid:		Transaction ID for message exchange
>   */
>  static void dhcpv6_send_ia_notonlink(struct ctx *c,
> +				     const struct in6_addr *caddr,
>  				     const struct iov_tail *ia_base,
>  				     const struct iov_tail *client_id_base,
>  				     int len, uint32_t xid)
> @@ -405,8 +407,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,
> -		      xid, &resp_not_on_link, n);
> +	tap_udp6_send(c, src, 547, caddr, 546, xid, &resp_not_on_link, n);
>  }
>  
>  /**
> @@ -590,8 +591,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;
> -
>  	src = &c->ip6.our_tap_ll;
>  
>  	mh = IOV_REMOVE_HEADER(data, mh_storage);
> @@ -630,8 +629,10 @@ int dhcpv6(struct ctx *c, struct iov_tail *data,
>  
>  		if (dhcpv6_ia_notonlink(data, &c->ip6.addr)) {
>  
> -			dhcpv6_send_ia_notonlink(c, data, &client_id_base,
> -						 ntohs(client_id->l), mh->xid);
> +			dhcpv6_send_ia_notonlink(c, saddr, data,
> +						 &client_id_base,
> +						 ntohs(client_id->l),
> +						 mh->xid);
>  
>  			return 1;
>  		}
> @@ -680,8 +681,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);
> +	tap_udp6_send(c, src, 547, saddr, 546, mh->xid, &resp, n);
>  	c->ip6.addr_seen = c->ip6.addr;
>  
>  	return 1;
> diff --git a/dhcpv6.h b/dhcpv6.h
> index c706dfdb..1015a1a7 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 *saddr, const struct in6_addr *daddr);
>  void dhcpv6_init(const struct ctx *c);
>  
>  #endif /* DHCPV6_H */
> diff --git a/tap.c b/tap.c
> index 6d93c7ce..d4189617 100644
> --- a/tap.c
> +++ b/tap.c
> @@ -160,21 +160,6 @@ void tap_send_single(const struct ctx *c, const void *data, size_t l2len)
>  	}
>  }
>  
> -/**
> - * tap_ip6_daddr() - Normal IPv6 destination address for inbound packets
> - * @c:		Execution context
> - * @src:	Source address
> - *
> - * Return: pointer to IPv6 address
> - */
> -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;
> -}
> -
>  /**
>   * tap_push_l2h() - Build an L2 header for an inbound packet
>   * @c:		Execution context
> diff --git a/tap.h b/tap.h
> index 07ca0965..b335933f 100644
> --- a/tap.h
> +++ b/tap.h
> @@ -96,8 +96,6 @@ void tap_udp4_send(const struct ctx *c, struct in_addr src, in_port_t sport,
>  		   const void *in, size_t dlen);
>  void tap_icmp4_send(const struct ctx *c, struct in_addr src, struct in_addr dst,
>  		    const void *in, const void *src_mac, size_t l4len);
> -const struct in6_addr *tap_ip6_daddr(const struct ctx *c,
> -				     const struct in6_addr *src);
>  void *tap_push_ip6h(struct ipv6hdr *ip6h,
>  		    const struct in6_addr *src, const struct in6_addr *dst,
>  		    size_t l4len, uint8_t proto, uint32_t flow);
> -- 
> 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 --]