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

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 --- 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