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