From 01a88375e3543d3ae6b19927cec23966aff39a1c Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Thu, 23 Nov 2023 04:53:43 +0900 Subject: [PATCH 1/7] network: split out common checks No functional change, just refactoring. Note, some checks may be redundant, but the cost is not heavy, so let's explicitly check for safety. --- src/network/networkd-sysctl.c | 113 +++++++++++++--------------------- 1 file changed, 43 insertions(+), 70 deletions(-) diff --git a/src/network/networkd-sysctl.c b/src/network/networkd-sysctl.c index 2ac6c3527bc28..24525a10c1af9 100644 --- a/src/network/networkd-sysctl.c +++ b/src/network/networkd-sysctl.c @@ -2,6 +2,7 @@ #include #include +#include #include "missing_network.h" #include "networkd-link.h" @@ -12,10 +13,25 @@ #include "string-table.h" #include "sysctl-util.h" -static int link_update_ipv6_sysctl(Link *link) { +static bool link_is_configured_for_family(Link *link, int family) { assert(link); + if (!link->network) + return false; + if (link->flags & IFF_LOOPBACK) + return false; + + if (family == AF_INET6 && !socket_ipv6_is_supported()) + return false; + + return true; +} + +static int link_update_ipv6_sysctl(Link *link) { + assert(link); + + if (!link_is_configured_for_family(link, AF_INET6)) return 0; if (!link_ipv6_enabled(link)) @@ -27,10 +43,7 @@ static int link_update_ipv6_sysctl(Link *link) { static int link_set_proxy_arp(Link *link) { assert(link); - if (link->flags & IFF_LOOPBACK) - return 0; - - if (!link->network) + if (!link_is_configured_for_family(link, AF_INET)) return 0; if (link->network->proxy_arp < 0) @@ -43,13 +56,7 @@ static bool link_ip_forward_enabled(Link *link, int family) { assert(link); assert(IN_SET(family, AF_INET, AF_INET6)); - if (family == AF_INET6 && !socket_ipv6_is_supported()) - return false; - - if (link->flags & IFF_LOOPBACK) - return false; - - if (!link->network) + if (!link_is_configured_for_family(link, family)) return false; return link->network->ip_forward & (family == AF_INET ? ADDRESS_FAMILY_IPV4 : ADDRESS_FAMILY_IPV6); @@ -92,10 +99,7 @@ static int link_set_ipv6_forward(Link *link) { static int link_set_ipv4_rp_filter(Link *link) { assert(link); - if (link->flags & IFF_LOOPBACK) - return 0; - - if (!link->network) + if (!link_is_configured_for_family(link, AF_INET)) return 0; if (link->network->ipv4_rp_filter < 0) @@ -110,13 +114,7 @@ static int link_set_ipv6_privacy_extensions(Link *link) { assert(link); assert(link->manager); - if (!socket_ipv6_is_supported()) - return 0; - - if (link->flags & IFF_LOOPBACK) - return 0; - - if (!link->network) + if (!link_is_configured_for_family(link, AF_INET6)) return 0; val = link->network->ipv6_privacy_extensions; @@ -133,14 +131,7 @@ static int link_set_ipv6_privacy_extensions(Link *link) { static int link_set_ipv6_accept_ra(Link *link) { assert(link); - /* Make this a NOP if IPv6 is not available */ - if (!socket_ipv6_is_supported()) - return 0; - - if (link->flags & IFF_LOOPBACK) - return 0; - - if (!link->network) + if (!link_is_configured_for_family(link, AF_INET6)) return 0; return sysctl_write_ip_property(AF_INET6, link->ifname, "accept_ra", "0"); @@ -149,14 +140,7 @@ static int link_set_ipv6_accept_ra(Link *link) { static int link_set_ipv6_dad_transmits(Link *link) { assert(link); - /* Make this a NOP if IPv6 is not available */ - if (!socket_ipv6_is_supported()) - return 0; - - if (link->flags & IFF_LOOPBACK) - return 0; - - if (!link->network) + if (!link_is_configured_for_family(link, AF_INET6)) return 0; if (link->network->ipv6_dad_transmits < 0) @@ -168,14 +152,7 @@ static int link_set_ipv6_dad_transmits(Link *link) { static int link_set_ipv6_hop_limit(Link *link) { assert(link); - /* Make this a NOP if IPv6 is not available */ - if (!socket_ipv6_is_supported()) - return 0; - - if (link->flags & IFF_LOOPBACK) - return 0; - - if (!link->network) + if (!link_is_configured_for_family(link, AF_INET6)) return 0; if (link->network->ipv6_hop_limit <= 0) @@ -189,13 +166,7 @@ static int link_set_ipv6_proxy_ndp(Link *link) { assert(link); - if (!socket_ipv6_is_supported()) - return 0; - - if (link->flags & IFF_LOOPBACK) - return 0; - - if (!link->network) + if (!link_is_configured_for_family(link, AF_INET6)) return 0; if (link->network->ipv6_proxy_ndp >= 0) @@ -211,14 +182,7 @@ int link_set_ipv6_mtu(Link *link) { assert(link); - /* Make this a NOP if IPv6 is not available */ - if (!socket_ipv6_is_supported()) - return 0; - - if (link->flags & IFF_LOOPBACK) - return 0; - - if (!link->network) + if (!link_is_configured_for_family(link, AF_INET6)) return 0; if (link->network->ipv6_mtu == 0) @@ -237,7 +201,7 @@ int link_set_ipv6_mtu(Link *link) { static int link_set_ipv4_accept_local(Link *link) { assert(link); - if (link->flags & IFF_LOOPBACK) + if (!link_is_configured_for_family(link, AF_INET)) return 0; if (link->network->ipv4_accept_local < 0) @@ -249,7 +213,7 @@ static int link_set_ipv4_accept_local(Link *link) { static int link_set_ipv4_route_localnet(Link *link) { assert(link); - if (link->flags & IFF_LOOPBACK) + if (!link_is_configured_for_family(link, AF_INET)) return 0; if (link->network->ipv4_route_localnet < 0) @@ -258,6 +222,20 @@ static int link_set_ipv4_route_localnet(Link *link) { return sysctl_write_ip_property_boolean(AF_INET, link->ifname, "route_localnet", link->network->ipv4_route_localnet > 0); } +static int link_set_ipv4_promote_secondaries(Link *link) { + assert(link); + + if (!link_is_configured_for_family(link, AF_INET)) + return 0; + + /* If promote_secondaries is not set, DHCP will work only as long as the IP address does not + * changes between leases. The kernel will remove all secondary IP addresses of an interface + * otherwise. The way systemd-networkd works is that the new IP of a lease is added as a + * secondary IP and when the primary one expires it relies on the kernel to promote the + * secondary IP. See also https://github.com/systemd/systemd/issues/7163 */ + return sysctl_write_ip_property_boolean(AF_INET, link->ifname, "promote_secondaries", true); +} + int link_set_sysctl(Link *link) { int r; @@ -321,12 +299,7 @@ int link_set_sysctl(Link *link) { if (r < 0) log_link_warning_errno(link, r, "Cannot set IPv4 reverse path filtering for interface, ignoring: %m"); - /* If promote_secondaries is not set, DHCP will work only as long as the IP address does not - * changes between leases. The kernel will remove all secondary IP addresses of an interface - * otherwise. The way systemd-networkd works is that the new IP of a lease is added as a - * secondary IP and when the primary one expires it relies on the kernel to promote the - * secondary IP. See also https://github.com/systemd/systemd/issues/7163 */ - r = sysctl_write_ip_property_boolean(AF_INET, link->ifname, "promote_secondaries", true); + r = link_set_ipv4_promote_secondaries(link); if (r < 0) log_link_warning_errno(link, r, "Cannot enable promote_secondaries for interface, ignoring: %m"); From f43ce810c408ee491b6edfc14c7680ee29274238 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Thu, 23 Nov 2023 04:56:58 +0900 Subject: [PATCH 2/7] network: do not try to update IP sysctl settings for CAN devices CAN devices do not support IP layer. Most of the functions below are never called for CAN devices, but link_set_ipv6_mtu() may be called after setting interface MTU, and warn about the failure. For safety, let's unconditionally check if the interface is not a CAN device. --- src/network/networkd-sysctl.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/network/networkd-sysctl.c b/src/network/networkd-sysctl.c index 24525a10c1af9..2b226b2e2a183 100644 --- a/src/network/networkd-sysctl.c +++ b/src/network/networkd-sysctl.c @@ -22,6 +22,12 @@ static bool link_is_configured_for_family(Link *link, int family) { if (link->flags & IFF_LOOPBACK) return false; + /* CAN devices do not support IP layer. Most of the functions below are never called for CAN devices, + * but link_set_ipv6_mtu() may be called after setting interface MTU, and warn about the failure. For + * safety, let's unconditionally check if the interface is not a CAN device. */ + if (IN_SET(family, AF_INET, AF_INET6) && link->iftype == ARPHRD_CAN) + return false; + if (family == AF_INET6 && !socket_ipv6_is_supported()) return false; From a0460dfed617a73f7dbf36a6eb7e474e887ae780 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Thu, 23 Nov 2023 05:03:43 +0900 Subject: [PATCH 3/7] parse-util: accept arbitrary MTU size when AF_UNSPEC When [Link] MTU= is specified in a .network file, we have no idea about that what kind of interface will be configured with the .network file. The maximum and minimum MTU size depend on the kind of interface. So, we should not filter MTU eagerly in the parser. Closes #30140. --- src/basic/parse-util.c | 15 +++++++++----- src/test/test-parse-util.c | 25 ++++++++++++++++++----- test/test-network-generator-conversion.sh | 2 +- 3 files changed, 31 insertions(+), 11 deletions(-) diff --git a/src/basic/parse-util.c b/src/basic/parse-util.c index dc868c9b8e9ac..0430e33e40df5 100644 --- a/src/basic/parse-util.c +++ b/src/basic/parse-util.c @@ -123,8 +123,7 @@ int parse_ifindex(const char *s) { } int parse_mtu(int family, const char *s, uint32_t *ret) { - uint64_t u; - size_t m; + uint64_t u, m; int r; r = parse_size(s, 1024, &u); @@ -134,10 +133,16 @@ int parse_mtu(int family, const char *s, uint32_t *ret) { if (u > UINT32_MAX) return -ERANGE; - if (family == AF_INET6) + switch (family) { + case AF_INET: + m = IPV4_MIN_MTU; /* This is 68 */ + break; + case AF_INET6: m = IPV6_MIN_MTU; /* This is 1280 */ - else - m = IPV4_MIN_MTU; /* For all other protocols, including 'unspecified' we assume the IPv4 minimal MTU */ + break; + default: + m = 0; + } if (u < m) return -ERANGE; diff --git a/src/test/test-parse-util.c b/src/test/test-parse-util.c index 7a485f390fb51..58d22b6cfeed0 100644 --- a/src/test/test-parse-util.c +++ b/src/test/test-parse-util.c @@ -914,15 +914,30 @@ TEST(parse_mtu) { assert_se(parse_mtu(AF_UNSPEC, "4294967295", &mtu) >= 0 && mtu == 4294967295); assert_se(parse_mtu(AF_UNSPEC, "500", &mtu) >= 0 && mtu == 500); assert_se(parse_mtu(AF_UNSPEC, "1280", &mtu) >= 0 && mtu == 1280); + assert_se(parse_mtu(AF_UNSPEC, "4294967296", &mtu) == -ERANGE); + assert_se(parse_mtu(AF_UNSPEC, "68", &mtu) >= 0 && mtu == 68); + assert_se(parse_mtu(AF_UNSPEC, "67", &mtu) >= 0 && mtu == 67); + assert_se(parse_mtu(AF_UNSPEC, "0", &mtu) >= 0 && mtu == 0); + assert_se(parse_mtu(AF_UNSPEC, "", &mtu) == -EINVAL); + + assert_se(parse_mtu(AF_INET, "1500", &mtu) >= 0 && mtu == 1500); + assert_se(parse_mtu(AF_INET, "1400", &mtu) >= 0 && mtu == 1400); + assert_se(parse_mtu(AF_INET, "65535", &mtu) >= 0 && mtu == 65535); + assert_se(parse_mtu(AF_INET, "65536", &mtu) >= 0 && mtu == 65536); + assert_se(parse_mtu(AF_INET, "4294967295", &mtu) >= 0 && mtu == 4294967295); + assert_se(parse_mtu(AF_INET, "500", &mtu) >= 0 && mtu == 500); + assert_se(parse_mtu(AF_INET, "1280", &mtu) >= 0 && mtu == 1280); + assert_se(parse_mtu(AF_INET, "4294967296", &mtu) == -ERANGE); + assert_se(parse_mtu(AF_INET, "68", &mtu) >= 0 && mtu == 68); + assert_se(parse_mtu(AF_INET, "67", &mtu) == -ERANGE); + assert_se(parse_mtu(AF_INET, "0", &mtu) == -ERANGE); + assert_se(parse_mtu(AF_INET, "", &mtu) == -EINVAL); + assert_se(parse_mtu(AF_INET6, "1280", &mtu) >= 0 && mtu == 1280); assert_se(parse_mtu(AF_INET6, "1279", &mtu) == -ERANGE); - assert_se(parse_mtu(AF_UNSPEC, "4294967296", &mtu) == -ERANGE); assert_se(parse_mtu(AF_INET6, "4294967296", &mtu) == -ERANGE); assert_se(parse_mtu(AF_INET6, "68", &mtu) == -ERANGE); - assert_se(parse_mtu(AF_UNSPEC, "68", &mtu) >= 0 && mtu == 68); - assert_se(parse_mtu(AF_UNSPEC, "67", &mtu) == -ERANGE); - assert_se(parse_mtu(AF_UNSPEC, "0", &mtu) == -ERANGE); - assert_se(parse_mtu(AF_UNSPEC, "", &mtu) == -EINVAL); + assert_se(parse_mtu(AF_INET6, "", &mtu) == -EINVAL); } TEST(parse_loadavg_fixed_point) { diff --git a/test/test-network-generator-conversion.sh b/test/test-network-generator-conversion.sh index 6224a4d04f96a..9a4732c981dc9 100755 --- a/test/test-network-generator-conversion.sh +++ b/test/test-network-generator-conversion.sh @@ -296,7 +296,7 @@ INVALID_COMMAND_LINES=( "ip=10.0.0.1:::255.255.255::foo99:off" "ip=10.0.0.1:::255.255.255.0:invalid_hostname:foo99:off" "ip=10.0.0.1:::255.255.255.0::verylonginterfacename:off" - "ip=:::::dhcp99:dhcp6:0" + "ip=:::::dhcp99:dhcp6:4294967296" "ip=:::::dhcp99:dhcp6:-1" "ip=:::::dhcp99:dhcp6:666:52:54:00" "ip=fdef:c400:bd01:1096::2::[fdef:c400:bd01:1096::1]:64::ipv6:off:[fdef:c400:bd01:1096::aaaa]" From a60cc587d456c83e8bf77bc3d7fe3c9ed10f3c40 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Thu, 23 Nov 2023 05:19:24 +0900 Subject: [PATCH 4/7] network: update MTU after CAN specific configs applied Changing FD mode may trigger change of MTU and maximum MTU size. See kernel documents about CAN FD mode: https://docs.kernel.org/networking/can.html#can-fd-flexible-data-rate-driver-support --- src/network/networkd-setlink.c | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/src/network/networkd-setlink.c b/src/network/networkd-setlink.c index 2b37c86d2351a..854699833d0b6 100644 --- a/src/network/networkd-setlink.c +++ b/src/network/networkd-setlink.c @@ -563,12 +563,20 @@ static int link_is_ready_to_set_link(Link *link, Request *req) { break; } case REQUEST_TYPE_SET_LINK_MTU: { - Request req_ipoib = { - .link = link, - .type = REQUEST_TYPE_SET_LINK_IPOIB, - }; + if (ordered_set_contains(link->manager->request_queue, + &(const Request) { + .link = link, + .type = REQUEST_TYPE_SET_LINK_IPOIB, + })) + return false; - return !ordered_set_contains(link->manager->request_queue, &req_ipoib); + /* Changing FD mode may affect MTU. */ + if (ordered_set_contains(link->manager->request_queue, + &(const Request) { + .link = link, + .type = REQUEST_TYPE_SET_LINK_CAN, + })) + return false; } default: break; From 15be80428204b57ca55272d2b45703047ad6f28d Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Thu, 23 Nov 2023 05:36:43 +0900 Subject: [PATCH 5/7] network: the maximum MTU size for CAN interface may be changed later So we should not reduce the requested size to the current maximum before applying CAN FD mode. --- src/network/networkd-setlink.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/src/network/networkd-setlink.c b/src/network/networkd-setlink.c index 854699833d0b6..38f90969ec498 100644 --- a/src/network/networkd-setlink.c +++ b/src/network/networkd-setlink.c @@ -866,7 +866,7 @@ int link_request_to_set_master(Link *link) { int link_request_to_set_mtu(Link *link, uint32_t mtu) { const char *origin; - uint32_t min_mtu; + uint32_t min_mtu, max_mtu; Request *req; int r; @@ -894,10 +894,19 @@ int link_request_to_set_mtu(Link *link, uint32_t mtu) { mtu = min_mtu; } - if (mtu > link->max_mtu) { + max_mtu = link->max_mtu; + if (link->iftype == ARPHRD_CAN) + /* The maximum MTU may be changed when FD mode is changed. + * See https://docs.kernel.org/networking/can.html#can-fd-flexible-data-rate-driver-support + * MTU = 16 (CAN_MTU) => Classical CAN device + * MTU = 72 (CANFD_MTU) => CAN FD capable device + * So, even if the current maximum is 16, we should not reduce the requested value now. */ + max_mtu = MAX(max_mtu, 72u); + + if (mtu > max_mtu) { log_link_warning(link, "Reducing the requested MTU %"PRIu32" to the interface's maximum MTU %"PRIu32".", - mtu, link->max_mtu); - mtu = link->max_mtu; + mtu, max_mtu); + mtu = max_mtu; } if (link->mtu == mtu) From 941f8e1399bcbac54c9cc862a227e0e63cebc538 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Thu, 7 Dec 2023 18:57:13 +0900 Subject: [PATCH 6/7] network: allow to configure interface MTU for CAN devices Previously, even if MTUBytes= is specified in matching .network file, the setting was ignored for CAN devices. --- src/network/networkd-link.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/network/networkd-link.c b/src/network/networkd-link.c index aad86eed99112..a063696c82b12 100644 --- a/src/network/networkd-link.c +++ b/src/network/networkd-link.c @@ -1049,6 +1049,10 @@ static int link_configure(Link *link) { if (r < 0) return r; + r = link_configure_mtu(link); + if (r < 0) + return r; + if (link->iftype == ARPHRD_CAN) { /* let's shortcut things for CAN which doesn't need most of what's done below. */ r = link_request_to_set_can(link); @@ -1082,10 +1086,6 @@ static int link_configure(Link *link) { if (r < 0) return r; - r = link_configure_mtu(link); - if (r < 0) - return r; - r = link_request_to_set_addrgen_mode(link); if (r < 0) return r; From 470a329d9849d108e28f72d00dd130d130cebb01 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 6 Dec 2023 14:55:03 +0900 Subject: [PATCH 7/7] test-network: add test for small MTU for vcan Prompted by https://github.com/systemd/systemd/issues/30140#issuecomment-1837973580. --- test/test-network/conf/25-vcan.netdev | 1 + test/test-network/conf/25-vcan98.netdev | 4 ++++ test/test-network/conf/25-vcan98.network | 6 ++++++ test/test-network/systemd-networkd-tests.py | 14 ++++++++++++-- 4 files changed, 23 insertions(+), 2 deletions(-) create mode 100644 test/test-network/conf/25-vcan98.netdev create mode 100644 test/test-network/conf/25-vcan98.network diff --git a/test/test-network/conf/25-vcan.netdev b/test/test-network/conf/25-vcan.netdev index 29bd98e5c9d08..2762dd2374735 100644 --- a/test/test-network/conf/25-vcan.netdev +++ b/test/test-network/conf/25-vcan.netdev @@ -2,3 +2,4 @@ [NetDev] Name=vcan99 Kind=vcan +MTUBytes=16 diff --git a/test/test-network/conf/25-vcan98.netdev b/test/test-network/conf/25-vcan98.netdev new file mode 100644 index 0000000000000..5333c82da4598 --- /dev/null +++ b/test/test-network/conf/25-vcan98.netdev @@ -0,0 +1,4 @@ +# SPDX-License-Identifier: LGPL-2.1-or-later +[NetDev] +Name=vcan98 +Kind=vcan diff --git a/test/test-network/conf/25-vcan98.network b/test/test-network/conf/25-vcan98.network new file mode 100644 index 0000000000000..97f824d2443d6 --- /dev/null +++ b/test/test-network/conf/25-vcan98.network @@ -0,0 +1,6 @@ +# SPDX-License-Identifier: LGPL-2.1-or-later +[Match] +Name=vcan98 + +[Link] +MTUBytes=16 diff --git a/test/test-network/systemd-networkd-tests.py b/test/test-network/systemd-networkd-tests.py index 8ffcda37ed9f0..a9fde69023ba1 100755 --- a/test/test-network/systemd-networkd-tests.py +++ b/test/test-network/systemd-networkd-tests.py @@ -1725,10 +1725,20 @@ def test_vrf(self): @expectedFailureIfModuleIsNotAvailable('vcan') def test_vcan(self): - copy_network_unit('25-vcan.netdev', '26-netdev-link-local-addressing-yes.network') + copy_network_unit('25-vcan.netdev', '26-netdev-link-local-addressing-yes.network', + '25-vcan98.netdev', '25-vcan98.network') start_networkd() - self.wait_online(['vcan99:carrier']) + self.wait_online(['vcan99:carrier', 'vcan98:carrier']) + + # https://github.com/systemd/systemd/issues/30140 + output = check_output('ip -d link show vcan99') + print(output) + self.assertIn('mtu 16 ', output) + + output = check_output('ip -d link show vcan98') + print(output) + self.assertIn('mtu 16 ', output) @expectedFailureIfModuleIsNotAvailable('vxcan') def test_vxcan(self):