| Message ID | 20240318114259.25614-1-fnordahl@ubuntu.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev] controller: Use multicast for IPv6 Prefix Delegation. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
Thanks Frode, looks good to me. Acked-by: Mark Michelson <mmichels@redhat.com> On 3/18/24 07:42, Frode Nordahl wrote: > The OVN Controller currently uses unicast messages when > communicating with the DHCPv6 server. > > This is at odds with RFC 3315 paragraph 1 in each of the sections > 18.2.1, 18.2.3, 18.2.6, and 18.2.7. > > The client is expected to use multicast, unless the server has > explicitly told it to use unicast using the Server Unicast > Option [0]. I contemplated adding support for this option, but > I found no server implementation that made use of it. > > Other supporting documentation is a change to ISC DHCP server > making it reject unicast Request, Renew, Decline and Release > messages [1]. The dnsmasq implementation also enforces the use > of multicast messages [2]. > > Fix the ovn-controller IPv6 Prefix Delegation implementation by > making it send requests using the all DHCP agents multicast > address [3] as expected. > > 0: https://datatracker.ietf.org/doc/html/rfc3315#section-22.12 > 1: https://github.com/isc-projects/dhcp/commit/b31fe1d552 > 2: https://github.com/imp/dnsmasq/blob/770bce967c/src/rfc3315.c#L347-L357 > 3: https://www.rfc-editor.org/rfc/rfc2375.html > Fixes: e3a398e9146e ("controller: Add ipv6 prefix delegation state machine") > Signed-off-by: Frode Nordahl <fnordahl@ubuntu.com> > --- > controller/pinctrl.c | 34 +++++++++------------------------- > lib/ovn-l7.c | 2 ++ > lib/ovn-l7.h | 6 ++++++ > tests/system-common-macros.at | 3 +-- > 4 files changed, 18 insertions(+), 27 deletions(-) > > diff --git a/controller/pinctrl.c b/controller/pinctrl.c > index b8091111a..721a737de 100644 > --- a/controller/pinctrl.c > +++ b/controller/pinctrl.c > @@ -758,9 +758,6 @@ struct ipv6_prefixd_state { > long long int next_announce; > long long int last_complete; > long long int last_used; > - /* IPv6 PD server info */ > - struct in6_addr server_addr; > - struct eth_addr sa; > /* server_id_info */ > struct { > uint8_t data[DHCPV6_MAX_DUID_LEN]; > @@ -917,12 +914,13 @@ pinctrl_parse_dhcpv6_advt(struct rconn *swconn, const struct flow *ip_flow, > struct dp_packet packet; > > dp_packet_use_stub(&packet, packet_stub, sizeof packet_stub); > - eth_compose(&packet, ip_flow->dl_src, ip_flow->dl_dst, ETH_TYPE_IPV6, > - IPV6_HEADER_LEN); > + eth_compose(&packet, (struct eth_addr) ETH_ADDR_C(33,33,00,01,00,02), > + ip_flow->dl_dst, ETH_TYPE_IPV6, IPV6_HEADER_LEN); > > struct udp_header *udp_h = compose_ipv6(&packet, IPPROTO_UDP, > &ip_flow->ipv6_dst, > - &ip_flow->ipv6_src, 0, 0, 255, > + &in6addr_all_dhcp_agents, > + 0, 0, 255, > len + UDP_HEADER_LEN + 4); > udp_h->udp_len = htons(len + UDP_HEADER_LEN + 4); > udp_h->udp_csum = 0; > @@ -976,7 +974,6 @@ out: > static void > pinctrl_prefixd_state_handler(const struct flow *ip_flow, > struct in6_addr addr, unsigned aid, > - struct eth_addr sa, struct in6_addr server_addr, > char prefix_len, unsigned t1, unsigned t2, > unsigned plife_time, unsigned vlife_time, > const uint8_t *uuid, uint8_t uuid_len) > @@ -986,8 +983,6 @@ pinctrl_prefixd_state_handler(const struct flow *ip_flow, > pfd = pinctrl_find_prefixd_state(ip_flow, aid); > if (pfd) { > pfd->state = PREFIX_PENDING; > - pfd->server_addr = server_addr; > - pfd->sa = sa; > memcpy(pfd->uuid.data, uuid, uuid_len); > pfd->uuid.len = uuid_len; > pfd->plife_time = plife_time * 1000; > @@ -1005,10 +1000,6 @@ pinctrl_parse_dhcpv6_reply(struct dp_packet *pkt_in, > const struct flow *ip_flow) > OVS_REQUIRES(pinctrl_mutex) > { > - struct eth_header *eth = dp_packet_eth(pkt_in); > - struct ovs_16aligned_ip6_hdr *in_ip = dp_packet_l3(pkt_in); > - struct in6_addr ip6_src; > - memcpy(&ip6_src, &in_ip->ip6_src, sizeof ip6_src); > struct udp_header *udp_in = dp_packet_l4(pkt_in); > unsigned char *in_dhcpv6_data = (unsigned char *)(udp_in + 1); > size_t dlen = MIN(ntohs(udp_in->udp_len), dp_packet_l4_size(pkt_in)); > @@ -1091,8 +1082,7 @@ pinctrl_parse_dhcpv6_reply(struct dp_packet *pkt_in, > VLOG_DBG_RL(&rl, "Received DHCPv6 reply from %s with prefix %s/%d" > " aid %d", ip6_s, prefix, prefix_len, aid); > } > - pinctrl_prefixd_state_handler(ip_flow, ipv6, aid, eth->eth_src, > - ip6_src, prefix_len, t1, t2, > + pinctrl_prefixd_state_handler(ip_flow, ipv6, aid, prefix_len, t1, t2, > plife_time, vlife_time, uuid, uuid_len); > } > } > @@ -1125,28 +1115,22 @@ pinctrl_handle_dhcp6_server(struct rconn *swconn, const struct flow *ip_flow, > static void > compose_prefixd_packet(struct dp_packet *b, struct ipv6_prefixd_state *pfd) > { > - struct in6_addr ipv6_dst; > - struct eth_addr eth_dst; > - > int payload = sizeof(struct dhcpv6_opt_server_id) + > sizeof(struct dhcpv6_opt_ia_na); > if (pfd->uuid.len) { > payload += pfd->uuid.len + sizeof(struct dhcpv6_opt_header); > - ipv6_dst = pfd->server_addr; > - eth_dst = pfd->sa; > - } else { > - eth_dst = (struct eth_addr) ETH_ADDR_C(33,33,00,01,00,02); > - ipv6_parse("ff02::1:2", &ipv6_dst); > } > if (ipv6_addr_is_set(&pfd->prefix)) { > payload += sizeof(struct dhcpv6_opt_ia_prefix); > } > > - eth_compose(b, eth_dst, pfd->ea, ETH_TYPE_IPV6, IPV6_HEADER_LEN); > + eth_compose(b, (struct eth_addr) ETH_ADDR_C(33,33,00,01,00,02), pfd->ea, > + ETH_TYPE_IPV6, IPV6_HEADER_LEN); > > int len = UDP_HEADER_LEN + 4 + payload; > struct udp_header *udp_h = compose_ipv6(b, IPPROTO_UDP, &pfd->ipv6_addr, > - &ipv6_dst, 0, 0, 255, len); > + &in6addr_all_dhcp_agents, > + 0, 0, 255, len); > udp_h->udp_len = htons(len); > udp_h->udp_csum = 0; > packet_set_udp_port(b, htons(546), htons(547)); > diff --git a/lib/ovn-l7.c b/lib/ovn-l7.c > index 3a5f3f3ec..2ddb68cb0 100644 > --- a/lib/ovn-l7.c > +++ b/lib/ovn-l7.c > @@ -18,6 +18,8 @@ > > #include "ovn-l7.h" > > +const struct in6_addr in6addr_all_dhcp_agents = IN6ADDR_ALL_DHCP_AGENTS_INIT; > + > bool > ipv6_addr_is_routable_multicast(const struct in6_addr *ip) > { > diff --git a/lib/ovn-l7.h b/lib/ovn-l7.h > index ad514a922..f4a30cc00 100644 > --- a/lib/ovn-l7.h > +++ b/lib/ovn-l7.h > @@ -305,6 +305,12 @@ BUILD_ASSERT_DECL(DHCP_OPT_HEADER_LEN == sizeof(struct dhcp_opt_header)); > #define DHCPV6_FQDN_FLAGS_O 1 << 1 > #define DHCPV6_FQDN_FLAGS_N 1 << 2 > > +extern const struct in6_addr in6addr_all_dhcp_agents; > +#define IN6ADDR_ALL_DHCP_AGENTS_INIT { { { 0xff,0x02,0x00,0x00,0x00,0x00, \ > + 0x00,0x00,0x00,0x00,0x00,0x00, \ > + 0x00,0x01,0x00,0x02 } } } > + > + > #define DHCP6_OPT_HEADER_LEN 4 > OVS_PACKED( > struct dhcpv6_opt_header { > diff --git a/tests/system-common-macros.at b/tests/system-common-macros.at > index 4c25afe68..f85ab2f44 100644 > --- a/tests/system-common-macros.at > +++ b/tests/system-common-macros.at > @@ -352,7 +352,7 @@ OVS_TRAFFIC_VSWITCHD_START() > ADD_BR([br-int]) > ADD_BR([br-ext]) > > -ovs-ofctl add-flow br-ext action=normal > +ovs-vsctl set-fail-mode br-ext standalone > # Set external-ids in br-int needed for ovn-controller > ovs-vsctl \ > -- set Open_vSwitch . external-ids:system-id=hv1 \ > @@ -426,7 +426,6 @@ ovn-nbctl --wait=hv sync > cat > /etc/dhcp/dhcpd.conf <<EOF > option dhcp-rebinding-time 10; > option dhcp-renewal-time 5; > -option dhcp6.unicast 2001:1db8:3333::1; > subnet6 2001:1db8:3333::/64 { > prefix6 2001:1db8:3333:100:: 2001:1db8:3333:111:: /96; > }
On 3/18/24 22:08, Mark Michelson wrote: > Thanks Frode, looks good to me. > > Acked-by: Mark Michelson <mmichels@redhat.com> > > On 3/18/24 07:42, Frode Nordahl wrote: >> The OVN Controller currently uses unicast messages when >> communicating with the DHCPv6 server. >> >> This is at odds with RFC 3315 paragraph 1 in each of the sections >> 18.2.1, 18.2.3, 18.2.6, and 18.2.7. >> >> The client is expected to use multicast, unless the server has >> explicitly told it to use unicast using the Server Unicast >> Option [0]. I contemplated adding support for this option, but >> I found no server implementation that made use of it. >> >> Other supporting documentation is a change to ISC DHCP server >> making it reject unicast Request, Renew, Decline and Release >> messages [1]. The dnsmasq implementation also enforces the use >> of multicast messages [2]. >> >> Fix the ovn-controller IPv6 Prefix Delegation implementation by >> making it send requests using the all DHCP agents multicast >> address [3] as expected. >> >> 0: https://datatracker.ietf.org/doc/html/rfc3315#section-22.12 >> 1: https://github.com/isc-projects/dhcp/commit/b31fe1d552 >> 2: https://github.com/imp/dnsmasq/blob/770bce967c/src/rfc3315.c#L347-L357 >> 3: https://www.rfc-editor.org/rfc/rfc2375.html >> Fixes: e3a398e9146e ("controller: Add ipv6 prefix delegation state >> machine") >> Signed-off-by: Frode Nordahl <fnordahl@ubuntu.com> >> --- Thanks, Frode and Mark! Applied to main and backported down to 23.06. Regards, Dumitru
diff --git a/controller/pinctrl.c b/controller/pinctrl.c index b8091111a..721a737de 100644 --- a/controller/pinctrl.c +++ b/controller/pinctrl.c @@ -758,9 +758,6 @@ struct ipv6_prefixd_state { long long int next_announce; long long int last_complete; long long int last_used; - /* IPv6 PD server info */ - struct in6_addr server_addr; - struct eth_addr sa; /* server_id_info */ struct { uint8_t data[DHCPV6_MAX_DUID_LEN]; @@ -917,12 +914,13 @@ pinctrl_parse_dhcpv6_advt(struct rconn *swconn, const struct flow *ip_flow, struct dp_packet packet; dp_packet_use_stub(&packet, packet_stub, sizeof packet_stub); - eth_compose(&packet, ip_flow->dl_src, ip_flow->dl_dst, ETH_TYPE_IPV6, - IPV6_HEADER_LEN); + eth_compose(&packet, (struct eth_addr) ETH_ADDR_C(33,33,00,01,00,02), + ip_flow->dl_dst, ETH_TYPE_IPV6, IPV6_HEADER_LEN); struct udp_header *udp_h = compose_ipv6(&packet, IPPROTO_UDP, &ip_flow->ipv6_dst, - &ip_flow->ipv6_src, 0, 0, 255, + &in6addr_all_dhcp_agents, + 0, 0, 255, len + UDP_HEADER_LEN + 4); udp_h->udp_len = htons(len + UDP_HEADER_LEN + 4); udp_h->udp_csum = 0; @@ -976,7 +974,6 @@ out: static void pinctrl_prefixd_state_handler(const struct flow *ip_flow, struct in6_addr addr, unsigned aid, - struct eth_addr sa, struct in6_addr server_addr, char prefix_len, unsigned t1, unsigned t2, unsigned plife_time, unsigned vlife_time, const uint8_t *uuid, uint8_t uuid_len) @@ -986,8 +983,6 @@ pinctrl_prefixd_state_handler(const struct flow *ip_flow, pfd = pinctrl_find_prefixd_state(ip_flow, aid); if (pfd) { pfd->state = PREFIX_PENDING; - pfd->server_addr = server_addr; - pfd->sa = sa; memcpy(pfd->uuid.data, uuid, uuid_len); pfd->uuid.len = uuid_len; pfd->plife_time = plife_time * 1000; @@ -1005,10 +1000,6 @@ pinctrl_parse_dhcpv6_reply(struct dp_packet *pkt_in, const struct flow *ip_flow) OVS_REQUIRES(pinctrl_mutex) { - struct eth_header *eth = dp_packet_eth(pkt_in); - struct ovs_16aligned_ip6_hdr *in_ip = dp_packet_l3(pkt_in); - struct in6_addr ip6_src; - memcpy(&ip6_src, &in_ip->ip6_src, sizeof ip6_src); struct udp_header *udp_in = dp_packet_l4(pkt_in); unsigned char *in_dhcpv6_data = (unsigned char *)(udp_in + 1); size_t dlen = MIN(ntohs(udp_in->udp_len), dp_packet_l4_size(pkt_in)); @@ -1091,8 +1082,7 @@ pinctrl_parse_dhcpv6_reply(struct dp_packet *pkt_in, VLOG_DBG_RL(&rl, "Received DHCPv6 reply from %s with prefix %s/%d" " aid %d", ip6_s, prefix, prefix_len, aid); } - pinctrl_prefixd_state_handler(ip_flow, ipv6, aid, eth->eth_src, - ip6_src, prefix_len, t1, t2, + pinctrl_prefixd_state_handler(ip_flow, ipv6, aid, prefix_len, t1, t2, plife_time, vlife_time, uuid, uuid_len); } } @@ -1125,28 +1115,22 @@ pinctrl_handle_dhcp6_server(struct rconn *swconn, const struct flow *ip_flow, static void compose_prefixd_packet(struct dp_packet *b, struct ipv6_prefixd_state *pfd) { - struct in6_addr ipv6_dst; - struct eth_addr eth_dst; - int payload = sizeof(struct dhcpv6_opt_server_id) + sizeof(struct dhcpv6_opt_ia_na); if (pfd->uuid.len) { payload += pfd->uuid.len + sizeof(struct dhcpv6_opt_header); - ipv6_dst = pfd->server_addr; - eth_dst = pfd->sa; - } else { - eth_dst = (struct eth_addr) ETH_ADDR_C(33,33,00,01,00,02); - ipv6_parse("ff02::1:2", &ipv6_dst); } if (ipv6_addr_is_set(&pfd->prefix)) { payload += sizeof(struct dhcpv6_opt_ia_prefix); } - eth_compose(b, eth_dst, pfd->ea, ETH_TYPE_IPV6, IPV6_HEADER_LEN); + eth_compose(b, (struct eth_addr) ETH_ADDR_C(33,33,00,01,00,02), pfd->ea, + ETH_TYPE_IPV6, IPV6_HEADER_LEN); int len = UDP_HEADER_LEN + 4 + payload; struct udp_header *udp_h = compose_ipv6(b, IPPROTO_UDP, &pfd->ipv6_addr, - &ipv6_dst, 0, 0, 255, len); + &in6addr_all_dhcp_agents, + 0, 0, 255, len); udp_h->udp_len = htons(len); udp_h->udp_csum = 0; packet_set_udp_port(b, htons(546), htons(547)); diff --git a/lib/ovn-l7.c b/lib/ovn-l7.c index 3a5f3f3ec..2ddb68cb0 100644 --- a/lib/ovn-l7.c +++ b/lib/ovn-l7.c @@ -18,6 +18,8 @@ #include "ovn-l7.h" +const struct in6_addr in6addr_all_dhcp_agents = IN6ADDR_ALL_DHCP_AGENTS_INIT; + bool ipv6_addr_is_routable_multicast(const struct in6_addr *ip) { diff --git a/lib/ovn-l7.h b/lib/ovn-l7.h index ad514a922..f4a30cc00 100644 --- a/lib/ovn-l7.h +++ b/lib/ovn-l7.h @@ -305,6 +305,12 @@ BUILD_ASSERT_DECL(DHCP_OPT_HEADER_LEN == sizeof(struct dhcp_opt_header)); #define DHCPV6_FQDN_FLAGS_O 1 << 1 #define DHCPV6_FQDN_FLAGS_N 1 << 2 +extern const struct in6_addr in6addr_all_dhcp_agents; +#define IN6ADDR_ALL_DHCP_AGENTS_INIT { { { 0xff,0x02,0x00,0x00,0x00,0x00, \ + 0x00,0x00,0x00,0x00,0x00,0x00, \ + 0x00,0x01,0x00,0x02 } } } + + #define DHCP6_OPT_HEADER_LEN 4 OVS_PACKED( struct dhcpv6_opt_header { diff --git a/tests/system-common-macros.at b/tests/system-common-macros.at index 4c25afe68..f85ab2f44 100644 --- a/tests/system-common-macros.at +++ b/tests/system-common-macros.at @@ -352,7 +352,7 @@ OVS_TRAFFIC_VSWITCHD_START() ADD_BR([br-int]) ADD_BR([br-ext]) -ovs-ofctl add-flow br-ext action=normal +ovs-vsctl set-fail-mode br-ext standalone # Set external-ids in br-int needed for ovn-controller ovs-vsctl \ -- set Open_vSwitch . external-ids:system-id=hv1 \ @@ -426,7 +426,6 @@ ovn-nbctl --wait=hv sync cat > /etc/dhcp/dhcpd.conf <<EOF option dhcp-rebinding-time 10; option dhcp-renewal-time 5; -option dhcp6.unicast 2001:1db8:3333::1; subnet6 2001:1db8:3333::/64 { prefix6 2001:1db8:3333:100:: 2001:1db8:3333:111:: /96; }
The OVN Controller currently uses unicast messages when communicating with the DHCPv6 server. This is at odds with RFC 3315 paragraph 1 in each of the sections 18.2.1, 18.2.3, 18.2.6, and 18.2.7. The client is expected to use multicast, unless the server has explicitly told it to use unicast using the Server Unicast Option [0]. I contemplated adding support for this option, but I found no server implementation that made use of it. Other supporting documentation is a change to ISC DHCP server making it reject unicast Request, Renew, Decline and Release messages [1]. The dnsmasq implementation also enforces the use of multicast messages [2]. Fix the ovn-controller IPv6 Prefix Delegation implementation by making it send requests using the all DHCP agents multicast address [3] as expected. 0: https://datatracker.ietf.org/doc/html/rfc3315#section-22.12 1: https://github.com/isc-projects/dhcp/commit/b31fe1d552 2: https://github.com/imp/dnsmasq/blob/770bce967c/src/rfc3315.c#L347-L357 3: https://www.rfc-editor.org/rfc/rfc2375.html Fixes: e3a398e9146e ("controller: Add ipv6 prefix delegation state machine") Signed-off-by: Frode Nordahl <fnordahl@ubuntu.com> --- controller/pinctrl.c | 34 +++++++++------------------------- lib/ovn-l7.c | 2 ++ lib/ovn-l7.h | 6 ++++++ tests/system-common-macros.at | 3 +-- 4 files changed, 18 insertions(+), 27 deletions(-)