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

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

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

On Sun, Apr 12, 2026 at 08:53:07PM -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.

I think there might have been more callers of tap_ip6_daddr() in the
past, which might have made this not true.

> But if
> the client sent from a global address, we would reply to an address
> different from what the client is expecting. Since RFC 8415 allows
> clients to use global addresses for DHCPv6, we now correct this, and
> always respond to the address the client was using.

Responding to the same address the client used is a good idea in
general.  However, for this specific case, I don't think it will quite
do what we want.  The problem is that we're still always using
our_tap_ll (link local) as the source address.  So if the client used
a global address we'll send a packet with mismatched address scopes.
AFAIU that won't usually work.

> 
> We also remove a redundant addr_ll_seen assignment, since this is
> already done by tap.c when processing IPv6 packets.
> 
> Signed-off-by: Jon Maloy <jmaloy@redhat.com>
> ---
>  dhcpv6.c | 14 ++++++--------
>  dhcpv6.h |  2 +-
>  tap.c    | 15 ---------------
>  tap.h    |  2 --
>  4 files changed, 7 insertions(+), 26 deletions(-)
> 
> diff --git a/dhcpv6.c b/dhcpv6.c
> index 97c04e2..2db0944 100644
> --- a/dhcpv6.c
> +++ b/dhcpv6.c
> @@ -370,12 +370,14 @@ notonlink:
>  /**
>   * dhcpv6_send_ia_notonlink() - Send NotOnLink status
>   * @c:			Execution context
> + * @saddr:		Source address of client message (reply destination)

@saddr is a bad name in this context, since it's the source address of
an earlier packet, not the one this function sends.  Maybe @caddr or
@client_addr?

>   * @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 *saddr,
>  				     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, saddr, 546, xid, &resp_not_on_link, n);
>  }
>  
>  /**
> @@ -543,7 +544,7 @@ static size_t dhcpv6_client_fqdn_fill(const struct iov_tail *data,
>   * dhcpv6() - Check if this is a DHCPv6 message, reply as needed
>   * @c:		Execution context
>   * @data:	Single packet starting from UDP header
> - * @saddr:	Source IPv6 address of original message
> + * @saddr:	Source IPv6 address of original message (for reply destination)

I don't know that the addition really adds anything useful.

>   * @daddr:	Destination IPv6 address of original message
>   *
>   * Return: 0 if it's not a DHCPv6 message, 1 if handled, -1 on failure
> @@ -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;

If the client is using a global address, I think we need to too.
Which is a bit of a problem, since we don't really have any way to
allocate one.

Have you seen a guest using a global address for DHCPv6 in practice,
or is it just a theoretical possibility?  I am wondering if that's
only something that would happen if we advertised a global address for
the DHCPv6 server via NDP (which we don't, and can't).

>  
>  	mh = IOV_REMOVE_HEADER(data, mh_storage);
> @@ -630,7 +629,7 @@ 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,
> +			dhcpv6_send_ia_notonlink(c, saddr, data, &client_id_base,
>  						 ntohs(client_id->l), mh->xid);
>  
>  			return 1;
> @@ -680,8 +679,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 c706dfd..1015a1a 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 eaa6111..59c45a3 100644
> --- a/tap.c
> +++ b/tap.c
> @@ -161,21 +161,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;
> -}
> -

Nice to see this ugly thing go away, though.

>  /**
>   * tap_push_l2h() - Build an L2 header for an inbound packet
>   * @c:		Execution context
> diff --git a/tap.h b/tap.h
> index 07ca096..b335933 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 --]