From: Thomas Hebb Date: Thu, 29 Sep 2022 06:40:35 +0000 (-0700) Subject: network: don't forget old RAs when a new one arrives X-Git-Tag: v252-rc1~31 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=2ccada8dc4a3571468a335808fd6fe49b8c6c6dd;p=thirdparty%2Fsystemd.git network: don't forget old RAs when a new one arrives IPv6 Neighbor Discovery lets us autoconfigure a link's IPv6 addresses, routes, DNS servers, and DNS search domains by listening for Router Advertisement (RA) packets broadcast by one or more routers on the link. Each RA can contain zero or more "options," each describing one piece of configuration (e.g. a single route). Currently, when we receive an RA from a router, we delete any addresses, routes, etc. that originated from that router's previous RAs unless they're also present as options in the new RA. That behavior is a violation of RFC 4861[1]. In Section 9, the RFC states that Senders MAY send a subset of options in different packets. ... Thus, a receiver MUST NOT associate any action with the absence of an option in a particular packet. This protocol specifies that receivers should only act on the expiration of timers and on the information that is received in the packets. Several other passages in the RFC reiterate this. Section 6.2.3: A router MAY choose not to include some or all options when sending unsolicited Router Advertisements. Section 6.3.4: Hosts accept the union of all received information; the receipt of a Router Advertisement MUST NOT invalidate all information received in a previous advertisement or from another source. At least one consumer router in production today, the Google Nest Wifi, often sends RAs that omit its global IPv6 prefix. When current versions of systemd-networkd receive those RAs, they immediately delete the interface's global IPv6 address, which breaks IPv6 connectivity. Fix the issue by removing the invalidation logic entirely. It's not needed at all, since we already invalidate addresses, routes, and DNS configuration when the interface goes down or their lifetimes expire. This fix does have the side effect of preventing changes to the .network file (e.g. denylisted prefixes, whether to add routes from RAs) from taking effect as soon as a new RA arrives. Instead, a full interface reconfiguration is needed. But triggering those changes on RA receipt was already rather arbitrary and out of the administrator's control, so I think this change is fine. commit 69203fba700e ("network: ndisc: remove old addresses and routes after at least one SLAAC address becomes ready") introduced this behavior. commit 50550722e3ba fixed it partially, by preventing one router's RAs from invalidating another router's configuration. [1] https://www.rfc-editor.org/rfc/rfc4861 Fixes: 69203fba700e ("network: ndisc: remove old addresses and routes after at least one SLAAC address becomes ready") --- diff --git a/src/network/networkd-address.c b/src/network/networkd-address.c index d7333b634ee..bb1eedc6f1d 100644 --- a/src/network/networkd-address.c +++ b/src/network/networkd-address.c @@ -160,7 +160,7 @@ bool address_is_ready(const Address *a) { return true; } -void link_mark_addresses(Link *link, NetworkConfigSource source, const struct in6_addr *router) { +void link_mark_addresses(Link *link, NetworkConfigSource source) { Address *a; assert(link); @@ -169,10 +169,6 @@ void link_mark_addresses(Link *link, NetworkConfigSource source, const struct in if (a->source != source) continue; - if (source == NETWORK_CONFIG_SOURCE_NDISC && - router && !in6_addr_equal(router, &a->provider.in6)) - continue; - address_mark(a); } } diff --git a/src/network/networkd-address.h b/src/network/networkd-address.h index d1eff9090a9..ef29caf4e78 100644 --- a/src/network/networkd-address.h +++ b/src/network/networkd-address.h @@ -120,7 +120,7 @@ int address_compare_func(const Address *a1, const Address *a2); DEFINE_NETWORK_CONFIG_STATE_FUNCTIONS(Address, address); -void link_mark_addresses(Link *link, NetworkConfigSource source, const struct in6_addr *router); +void link_mark_addresses(Link *link, NetworkConfigSource source); CONFIG_PARSER_PROTOTYPE(config_parse_address); CONFIG_PARSER_PROTOTYPE(config_parse_broadcast); diff --git a/src/network/networkd-dhcp-prefix-delegation.c b/src/network/networkd-dhcp-prefix-delegation.c index a78a7e5789e..66c5e979d9f 100644 --- a/src/network/networkd-dhcp-prefix-delegation.c +++ b/src/network/networkd-dhcp-prefix-delegation.c @@ -572,8 +572,8 @@ static int dhcp_pd_prepare(Link *link) { if (link_radv_enabled(link) && link->network->dhcp_pd_announce && !link->radv) return 0; - link_mark_addresses(link, NETWORK_CONFIG_SOURCE_DHCP_PD, NULL); - link_mark_routes(link, NETWORK_CONFIG_SOURCE_DHCP_PD, NULL); + link_mark_addresses(link, NETWORK_CONFIG_SOURCE_DHCP_PD); + link_mark_routes(link, NETWORK_CONFIG_SOURCE_DHCP_PD); return 1; } diff --git a/src/network/networkd-dhcp4.c b/src/network/networkd-dhcp4.c index afe80460a0f..c5bddb60fd9 100644 --- a/src/network/networkd-dhcp4.c +++ b/src/network/networkd-dhcp4.c @@ -899,8 +899,8 @@ static int dhcp4_request_address_and_routes(Link *link, bool announce) { assert(link); - link_mark_addresses(link, NETWORK_CONFIG_SOURCE_DHCP4, NULL); - link_mark_routes(link, NETWORK_CONFIG_SOURCE_DHCP4, NULL); + link_mark_addresses(link, NETWORK_CONFIG_SOURCE_DHCP4); + link_mark_routes(link, NETWORK_CONFIG_SOURCE_DHCP4); r = dhcp4_request_address(link, announce); if (r < 0) diff --git a/src/network/networkd-dhcp6.c b/src/network/networkd-dhcp6.c index 086a6f4b616..789868c26aa 100644 --- a/src/network/networkd-dhcp6.c +++ b/src/network/networkd-dhcp6.c @@ -304,8 +304,8 @@ static int dhcp6_lease_ip_acquired(sd_dhcp6_client *client, Link *link) { sd_dhcp6_lease *lease; int r; - link_mark_addresses(link, NETWORK_CONFIG_SOURCE_DHCP6, NULL); - link_mark_routes(link, NETWORK_CONFIG_SOURCE_DHCP6, NULL); + link_mark_addresses(link, NETWORK_CONFIG_SOURCE_DHCP6); + link_mark_routes(link, NETWORK_CONFIG_SOURCE_DHCP6); r = sd_dhcp6_client_get_lease(client, &lease); if (r < 0) diff --git a/src/network/networkd-ndisc.c b/src/network/networkd-ndisc.c index d0508f93c04..2250235d4eb 100644 --- a/src/network/networkd-ndisc.c +++ b/src/network/networkd-ndisc.c @@ -72,72 +72,6 @@ void network_adjust_ipv6_accept_ra(Network *network) { network->ndisc_deny_listed_route_prefix = set_free_free(network->ndisc_deny_listed_route_prefix); } -static int ndisc_remove(Link *link, struct in6_addr *router) { - bool updated = false; - NDiscDNSSL *dnssl; - NDiscRDNSS *rdnss; - Address *address; - Route *route; - int k, r = 0; - - assert(link); - - SET_FOREACH(route, link->routes) { - if (route->source != NETWORK_CONFIG_SOURCE_NDISC) - continue; - if (!route_is_marked(route)) - continue; - if (router && !in6_addr_equal(router, &route->provider.in6)) - continue; - - k = route_remove(route); - if (k < 0) - r = k; - - route_cancel_request(route, link); - } - - SET_FOREACH(address, link->addresses) { - if (address->source != NETWORK_CONFIG_SOURCE_NDISC) - continue; - if (!address_is_marked(address)) - continue; - if (router && !in6_addr_equal(router, &address->provider.in6)) - continue; - - k = address_remove(address); - if (k < 0) - r = k; - - address_cancel_request(address); - } - - SET_FOREACH(rdnss, link->ndisc_rdnss) { - if (!rdnss->marked) - continue; - if (router && !in6_addr_equal(router, &rdnss->router)) - continue; - - free(set_remove(link->ndisc_rdnss, rdnss)); - updated = true; - } - - SET_FOREACH(dnssl, link->ndisc_dnssl) { - if (!dnssl->marked) - continue; - if (router && !in6_addr_equal(router, &dnssl->router)) - continue; - - free(set_remove(link->ndisc_dnssl, dnssl)); - updated = true; - } - - if (updated) - link_dirty(link); - - return r; -} - static int ndisc_check_ready(Link *link); static int ndisc_address_ready_callback(Address *address) { @@ -156,7 +90,6 @@ static int ndisc_address_ready_callback(Address *address) { static int ndisc_check_ready(Link *link) { bool found = false, ready = false; Address *address; - int r; assert(link); @@ -189,10 +122,6 @@ static int ndisc_check_ready(Link *link) { link->ndisc_configured = true; log_link_debug(link, "SLAAC addresses and routes set."); - r = ndisc_remove(link, NULL); - if (r < 0) - return r; - link_check_ready(link); return 0; } @@ -216,7 +145,6 @@ static int ndisc_route_handler(sd_netlink *rtnl, sd_netlink_message *m, Request static int ndisc_request_route(Route *in, Link *link, sd_ndisc_router *rt) { _cleanup_(route_freep) Route *route = in; struct in6_addr router; - Route *existing; int r; assert(route); @@ -236,10 +164,8 @@ static int ndisc_request_route(Route *in, Link *link, sd_ndisc_router *rt) { if (!route->protocol_set) route->protocol = RTPROT_RA; - if (route_get(NULL, link, route, &existing) < 0) + if (route_get(NULL, link, route, NULL) < 0) link->ndisc_configured = false; - else - route_unmark(existing); return link_request_route(link, TAKE_PTR(route), true, &link->ndisc_messages, ndisc_route_handler, NULL); @@ -264,7 +190,6 @@ static int ndisc_address_handler(sd_netlink *rtnl, sd_netlink_message *m, Reques static int ndisc_request_address(Address *in, Link *link, sd_ndisc_router *rt) { _cleanup_(address_freep) Address *address = in; struct in6_addr router; - Address *existing; int r; assert(address); @@ -282,10 +207,8 @@ static int ndisc_request_address(Address *in, Link *link, sd_ndisc_router *rt) { if (r < 0) return r; - if (address_get(link, address, &existing) < 0) + if (address_get(link, address, NULL) < 0) link->ndisc_configured = false; - else - address_unmark(existing); return link_request_address(link, TAKE_PTR(address), true, &link->ndisc_messages, ndisc_address_handler, NULL); @@ -723,7 +646,6 @@ static int ndisc_router_process_rdnss(Link *link, sd_ndisc_router *rt) { rdnss = set_get(link->ndisc_rdnss, &d); if (rdnss) { - rdnss->marked = false; rdnss->router = router; rdnss->lifetime_usec = lifetime_usec; continue; @@ -822,7 +744,6 @@ static int ndisc_router_process_dnssl(Link *link, sd_ndisc_router *rt) { dnssl = set_get(link->ndisc_dnssl, s); if (dnssl) { - dnssl->marked = false; dnssl->router = router; dnssl->lifetime_usec = lifetime_usec; continue; @@ -893,25 +814,6 @@ static int ndisc_router_process_options(Link *link, sd_ndisc_router *rt) { } } -static void ndisc_mark(Link *link, const struct in6_addr *router) { - NDiscRDNSS *rdnss; - NDiscDNSSL *dnssl; - - assert(link); - assert(router); - - link_mark_addresses(link, NETWORK_CONFIG_SOURCE_NDISC, router); - link_mark_routes(link, NETWORK_CONFIG_SOURCE_NDISC, router); - - SET_FOREACH(rdnss, link->ndisc_rdnss) - if (in6_addr_equal(&rdnss->router, router)) - rdnss->marked = true; - - SET_FOREACH(dnssl, link->ndisc_dnssl) - if (in6_addr_equal(&dnssl->router, router)) - dnssl->marked = true; -} - static int ndisc_start_dhcp6_client(Link *link, sd_ndisc_router *rt) { int r; @@ -978,8 +880,6 @@ static int ndisc_router_handler(Link *link, sd_ndisc_router *rt) { return 0; } - ndisc_mark(link, &router); - r = ndisc_start_dhcp6_client(link, rt); if (r < 0) return r; @@ -992,13 +892,9 @@ static int ndisc_router_handler(Link *link, sd_ndisc_router *rt) { if (r < 0) return r; - if (link->ndisc_messages == 0) { + if (link->ndisc_messages == 0) link->ndisc_configured = true; - - r = ndisc_remove(link, &router); - if (r < 0) - return r; - } else + else log_link_debug(link, "Setting SLAAC addresses and router."); if (!link->ndisc_configured) diff --git a/src/network/networkd-ndisc.h b/src/network/networkd-ndisc.h index f88a002c73e..bdbe2f41f1d 100644 --- a/src/network/networkd-ndisc.h +++ b/src/network/networkd-ndisc.h @@ -16,8 +16,6 @@ typedef enum IPv6AcceptRAStartDHCP6Client { } IPv6AcceptRAStartDHCP6Client; typedef struct NDiscRDNSS { - /* Used when GC'ing old DNS servers when configuration changes. */ - bool marked; struct in6_addr router; /* This is an absolute point in time, and NOT a timespan/duration. * Must be specified with clock_boottime_or_monotonic(). */ @@ -26,8 +24,6 @@ typedef struct NDiscRDNSS { } NDiscRDNSS; typedef struct NDiscDNSSL { - /* Used when GC'ing old domains when configuration changes. */ - bool marked; struct in6_addr router; /* This is an absolute point in time, and NOT a timespan/duration. * Must be specified with clock_boottime_or_monotonic(). */ diff --git a/src/network/networkd-route.c b/src/network/networkd-route.c index 6b3708573c1..3f460e0ba4b 100644 --- a/src/network/networkd-route.c +++ b/src/network/networkd-route.c @@ -534,7 +534,7 @@ static int route_convert(Manager *manager, const Route *route, ConvertedRoutes * return 1; } -void link_mark_routes(Link *link, NetworkConfigSource source, const struct in6_addr *router) { +void link_mark_routes(Link *link, NetworkConfigSource source) { Route *route; assert(link); @@ -543,10 +543,6 @@ void link_mark_routes(Link *link, NetworkConfigSource source, const struct in6_a if (route->source != source) continue; - if (source == NETWORK_CONFIG_SOURCE_NDISC && - router && !in6_addr_equal(router, &route->provider.in6)) - continue; - route_mark(route); } } diff --git a/src/network/networkd-route.h b/src/network/networkd-route.h index fd4433eae4e..071803cefbd 100644 --- a/src/network/networkd-route.h +++ b/src/network/networkd-route.h @@ -109,7 +109,7 @@ int network_add_default_route_on_device(Network *network); void network_drop_invalid_routes(Network *network); DEFINE_NETWORK_CONFIG_STATE_FUNCTIONS(Route, route); -void link_mark_routes(Link *link, NetworkConfigSource source, const struct in6_addr *router); +void link_mark_routes(Link *link, NetworkConfigSource source); CONFIG_PARSER_PROTOTYPE(config_parse_gateway); CONFIG_PARSER_PROTOTYPE(config_parse_preferred_src);