From d3ca35c64d48daf3451851043cffe2bda3913648 Mon Sep 17 00:00:00 2001 From: Stanislav Fomichev Date: Thu, 7 Nov 2024 10:12:04 -0800 Subject: [PATCH 01/16] selftests: ncdevmem: Remove default arguments To make it clear what's required and what's not. Also, some of the values don't seem like a good defaults; for example eth1. Move the invocation comment to the top, add missing -s to the client and cleanup the client invocation a bit to make more readable. Reviewed-by: Mina Almasry Reviewed-by: Joe Damato Signed-off-by: Stanislav Fomichev Link: https://patch.msgid.link/20241107181211.3934153-6-sdf@fomichev.me Signed-off-by: Jakub Kicinski --- tools/testing/selftests/net/ncdevmem.c | 61 ++++++++++++++++---------- 1 file changed, 39 insertions(+), 22 deletions(-) diff --git a/tools/testing/selftests/net/ncdevmem.c b/tools/testing/selftests/net/ncdevmem.c index faa9dce121c7..0feeca56c049 100644 --- a/tools/testing/selftests/net/ncdevmem.c +++ b/tools/testing/selftests/net/ncdevmem.c @@ -1,4 +1,31 @@ // SPDX-License-Identifier: GPL-2.0 +/* + * tcpdevmem netcat. Works similarly to netcat but does device memory TCP + * instead of regular TCP. Uses udmabuf to mock a dmabuf provider. + * + * Usage: + * + * On server: + * ncdevmem -s [-c ] -f eth1 -l -p 5201 + * + * On client: + * echo -n "hello\nworld" | nc -s 5201 -p 5201 + * + * Test data validation: + * + * On server: + * ncdevmem -s [-c ] -f eth1 -l -p 5201 -v 7 + * + * On client: + * yes $(echo -e \\x01\\x02\\x03\\x04\\x05\\x06) | \ + * tr \\n \\0 | \ + * head -c 5G | \ + * nc 5201 -p 5201 + * + * + * Note this is compatible with regular netcat. i.e. the sender or receiver can + * be replaced with regular netcat to test the RX or TX path in isolation. + */ #define _GNU_SOURCE #define __EXPORTED_HEADERS__ @@ -42,32 +69,13 @@ #define MSG_SOCK_DEVMEM 0x2000000 #endif -/* - * tcpdevmem netcat. Works similarly to netcat but does device memory TCP - * instead of regular TCP. Uses udmabuf to mock a dmabuf provider. - * - * Usage: - * - * On server: - * ncdevmem -s -c -f eth1 -l -p 5201 -v 7 - * - * On client: - * yes $(echo -e \\x01\\x02\\x03\\x04\\x05\\x06) | \ - * tr \\n \\0 | \ - * head -c 5G | \ - * nc 5201 -p 5201 - * - * Note this is compatible with regular netcat. i.e. the sender or receiver can - * be replaced with regular netcat to test the RX or TX path in isolation. - */ - -static char *server_ip = "192.168.1.4"; +static char *server_ip; static char *client_ip; -static char *port = "5201"; +static char *port; static size_t do_validation; static int start_queue = 8; static int num_queues = 8; -static char *ifname = "eth1"; +static char *ifname; static unsigned int ifindex; static unsigned int dmabuf_id; @@ -595,6 +603,15 @@ int main(int argc, char *argv[]) } } + if (!server_ip) + error(1, 0, "Missing -s argument\n"); + + if (!port) + error(1, 0, "Missing -p argument\n"); + + if (!ifname) + error(1, 0, "Missing -f argument\n"); + ifindex = if_nametoindex(ifname); for (; optind < argc; optind++) -- 2.51.0 From 933056357a8cf0c9b3fb2ecc4d2d8d142614f0a3 Mon Sep 17 00:00:00 2001 From: Stanislav Fomichev Date: Thu, 7 Nov 2024 10:12:05 -0800 Subject: [PATCH 02/16] selftests: ncdevmem: Switch to AF_INET6 Use dualstack socket to support both v4 and v6. v4-mapped-v6 address can be used to do v4. Reviewed-by: Mina Almasry Reviewed-by: Joe Damato Signed-off-by: Stanislav Fomichev Link: https://patch.msgid.link/20241107181211.3934153-7-sdf@fomichev.me Signed-off-by: Jakub Kicinski --- tools/testing/selftests/net/ncdevmem.c | 97 ++++++++++++++++++-------- 1 file changed, 68 insertions(+), 29 deletions(-) diff --git a/tools/testing/selftests/net/ncdevmem.c b/tools/testing/selftests/net/ncdevmem.c index 0feeca56c049..645ef0bb63ec 100644 --- a/tools/testing/selftests/net/ncdevmem.c +++ b/tools/testing/selftests/net/ncdevmem.c @@ -242,13 +242,26 @@ static int configure_channels(unsigned int rx, unsigned int tx) return run_command("sudo ethtool -L %s rx %u tx %u", ifname, rx, tx); } -static int configure_flow_steering(void) +static int configure_flow_steering(struct sockaddr_in6 *server_sin) { - return run_command("sudo ethtool -N %s flow-type tcp4 %s %s dst-ip %s %s %s dst-port %s queue %d >&2", + const char *type = "tcp6"; + const char *server_addr; + char buf[40]; + + inet_ntop(AF_INET6, &server_sin->sin6_addr, buf, sizeof(buf)); + server_addr = buf; + + if (IN6_IS_ADDR_V4MAPPED(&server_sin->sin6_addr)) { + type = "tcp4"; + server_addr = strrchr(server_addr, ':') + 1; + } + + return run_command("sudo ethtool -N %s flow-type %s %s %s dst-ip %s %s %s dst-port %s queue %d >&2", ifname, + type, client_ip ? "src-ip" : "", client_ip ?: "", - server_ip, + server_addr, client_ip ? "src-port" : "", client_ip ? port : "", port, start_queue); @@ -299,13 +312,51 @@ err_close: return -1; } +static void enable_reuseaddr(int fd) +{ + int opt = 1; + int ret; + + ret = setsockopt(fd, SOL_SOCKET, SO_REUSEPORT, &opt, sizeof(opt)); + if (ret) + error(1, errno, "%s: [FAIL, SO_REUSEPORT]\n", TEST_PREFIX); + + ret = setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, &opt, sizeof(opt)); + if (ret) + error(1, errno, "%s: [FAIL, SO_REUSEADDR]\n", TEST_PREFIX); +} + +static int parse_address(const char *str, int port, struct sockaddr_in6 *sin6) +{ + int ret; + + sin6->sin6_family = AF_INET6; + sin6->sin6_port = htons(port); + + ret = inet_pton(sin6->sin6_family, str, &sin6->sin6_addr); + if (ret != 1) { + /* fallback to plain IPv4 */ + ret = inet_pton(AF_INET, str, &sin6->sin6_addr.s6_addr32[3]); + if (ret != 1) + return -1; + + /* add ::ffff prefix */ + sin6->sin6_addr.s6_addr32[0] = 0; + sin6->sin6_addr.s6_addr32[1] = 0; + sin6->sin6_addr.s6_addr16[4] = 0; + sin6->sin6_addr.s6_addr16[5] = 0xffff; + } + + return 0; +} + int do_server(struct memory_buffer *mem) { char ctrl_data[sizeof(int) * 20000]; struct netdev_queue_id *queues; size_t non_page_aligned_frags = 0; - struct sockaddr_in client_addr; - struct sockaddr_in server_sin; + struct sockaddr_in6 client_addr; + struct sockaddr_in6 server_sin; size_t page_aligned_frags = 0; size_t total_received = 0; socklen_t client_addr_len; @@ -317,9 +368,12 @@ int do_server(struct memory_buffer *mem) int socket_fd; int client_fd; size_t i = 0; - int opt = 1; int ret; + ret = parse_address(server_ip, atoi(port), &server_sin); + if (ret < 0) + error(1, 0, "parse server address"); + if (reset_flow_steering()) error(1, 0, "Failed to reset flow steering\n"); @@ -328,7 +382,7 @@ int do_server(struct memory_buffer *mem) error(1, 0, "Failed to configure rss\n"); /* Flow steer our devmem flows to start_queue */ - if (configure_flow_steering()) + if (configure_flow_steering(&server_sin)) error(1, 0, "Failed to configure flow steering\n"); sleep(1); @@ -349,29 +403,14 @@ int do_server(struct memory_buffer *mem) if (!tmp_mem) error(1, ENOMEM, "malloc failed"); - server_sin.sin_family = AF_INET; - server_sin.sin_port = htons(atoi(port)); - - ret = inet_pton(server_sin.sin_family, server_ip, &server_sin.sin_addr); - if (ret < 0) - error(1, errno, "%s: [FAIL, create socket]\n", TEST_PREFIX); - - socket_fd = socket(server_sin.sin_family, SOCK_STREAM, 0); + socket_fd = socket(AF_INET6, SOCK_STREAM, 0); if (socket_fd < 0) error(1, errno, "%s: [FAIL, create socket]\n", TEST_PREFIX); - ret = setsockopt(socket_fd, SOL_SOCKET, SO_REUSEPORT, &opt, - sizeof(opt)); - if (ret) - error(1, errno, "%s: [FAIL, set sock opt]\n", TEST_PREFIX); - - ret = setsockopt(socket_fd, SOL_SOCKET, SO_REUSEADDR, &opt, - sizeof(opt)); - if (ret) - error(1, errno, "%s: [FAIL, set sock opt]\n", TEST_PREFIX); + enable_reuseaddr(socket_fd); fprintf(stderr, "binding to address %s:%d\n", server_ip, - ntohs(server_sin.sin_port)); + ntohs(server_sin.sin6_port)); ret = bind(socket_fd, &server_sin, sizeof(server_sin)); if (ret) @@ -383,16 +422,16 @@ int do_server(struct memory_buffer *mem) client_addr_len = sizeof(client_addr); - inet_ntop(server_sin.sin_family, &server_sin.sin_addr, buffer, + inet_ntop(AF_INET6, &server_sin.sin6_addr, buffer, sizeof(buffer)); fprintf(stderr, "Waiting or connection on %s:%d\n", buffer, - ntohs(server_sin.sin_port)); + ntohs(server_sin.sin6_port)); client_fd = accept(socket_fd, &client_addr, &client_addr_len); - inet_ntop(client_addr.sin_family, &client_addr.sin_addr, buffer, + inet_ntop(AF_INET6, &client_addr.sin6_addr, buffer, sizeof(buffer)); fprintf(stderr, "Got connection from %s:%d\n", buffer, - ntohs(client_addr.sin_port)); + ntohs(client_addr.sin6_port)); while (1) { struct iovec iov = { .iov_base = iobuf, -- 2.51.0 From e3c09623a53b8d11ff9e3c0f435ce1e8f52134ba Mon Sep 17 00:00:00 2001 From: Stanislav Fomichev Date: Thu, 7 Nov 2024 10:12:06 -0800 Subject: [PATCH 03/16] selftests: ncdevmem: Properly reset flow steering ntuple off/on might be not enough to do it on all NICs. Add a bunch of shell crap to explicitly remove the rules. Reviewed-by: Mina Almasry Reviewed-by: Joe Damato Signed-off-by: Stanislav Fomichev Link: https://patch.msgid.link/20241107181211.3934153-8-sdf@fomichev.me Signed-off-by: Jakub Kicinski --- tools/testing/selftests/net/ncdevmem.c | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/tools/testing/selftests/net/ncdevmem.c b/tools/testing/selftests/net/ncdevmem.c index 645ef0bb63ec..ad6de8e0e97b 100644 --- a/tools/testing/selftests/net/ncdevmem.c +++ b/tools/testing/selftests/net/ncdevmem.c @@ -217,13 +217,18 @@ void validate_buffer(void *line, size_t size) static int reset_flow_steering(void) { - int ret = 0; - - ret = run_command("sudo ethtool -K %s ntuple off >&2", ifname); - if (ret) - return ret; - - return run_command("sudo ethtool -K %s ntuple on >&2", ifname); + /* Depending on the NIC, toggling ntuple off and on might not + * be allowed. Additionally, attempting to delete existing filters + * will fail if no filters are present. Therefore, do not enforce + * the exit status. + */ + + run_command("sudo ethtool -K %s ntuple off >&2", ifname); + run_command("sudo ethtool -K %s ntuple on >&2", ifname); + run_command( + "sudo ethtool -n %s | grep 'Filter:' | awk '{print $2}' | xargs -n1 ethtool -N %s delete >&2", + ifname, ifname); + return 0; } static int configure_headersplit(bool on) -- 2.51.0 From 798d822e5d34ffe3f25b66b2573928962a5d3c11 Mon Sep 17 00:00:00 2001 From: Stanislav Fomichev Date: Thu, 7 Nov 2024 10:12:07 -0800 Subject: [PATCH 04/16] selftests: ncdevmem: Use YNL to enable TCP header split In the next patch the hard-coded queue numbers are gonna be removed. So introduce some initial support for ethtool YNL and use it to enable header split. Also, tcp-data-split requires latest ethtool which is unlikely to be present in the distros right now. (ideally, we should not shell out to ethtool at all). Reviewed-by: Mina Almasry Reviewed-by: Joe Damato Signed-off-by: Stanislav Fomichev Link: https://patch.msgid.link/20241107181211.3934153-9-sdf@fomichev.me Signed-off-by: Jakub Kicinski --- tools/testing/selftests/net/Makefile | 2 +- tools/testing/selftests/net/ncdevmem.c | 57 +++++++++++++++++++++++++- 2 files changed, 56 insertions(+), 3 deletions(-) diff --git a/tools/testing/selftests/net/Makefile b/tools/testing/selftests/net/Makefile index 8c4db5199a42..61cce028f105 100644 --- a/tools/testing/selftests/net/Makefile +++ b/tools/testing/selftests/net/Makefile @@ -112,7 +112,7 @@ TEST_INCLUDES := forwarding/lib.sh include ../lib.mk # YNL build -YNL_GENS := netdev +YNL_GENS := ethtool netdev include ynl.mk $(OUTPUT)/epoll_busy_poll: LDLIBS += -lcap diff --git a/tools/testing/selftests/net/ncdevmem.c b/tools/testing/selftests/net/ncdevmem.c index ad6de8e0e97b..9ca2da3a2f63 100644 --- a/tools/testing/selftests/net/ncdevmem.c +++ b/tools/testing/selftests/net/ncdevmem.c @@ -55,10 +55,12 @@ #include #include #include +#include #include #include #include "netdev-user.h" +#include "ethtool-user.h" #include #define PAGE_SHIFT 12 @@ -231,10 +233,58 @@ static int reset_flow_steering(void) return 0; } +static const char *tcp_data_split_str(int val) +{ + switch (val) { + case 0: + return "off"; + case 1: + return "auto"; + case 2: + return "on"; + default: + return "?"; + } +} + static int configure_headersplit(bool on) { - return run_command("sudo ethtool -G %s tcp-data-split %s >&2", ifname, - on ? "on" : "off"); + struct ethtool_rings_get_req *get_req; + struct ethtool_rings_get_rsp *get_rsp; + struct ethtool_rings_set_req *req; + struct ynl_error yerr; + struct ynl_sock *ys; + int ret; + + ys = ynl_sock_create(&ynl_ethtool_family, &yerr); + if (!ys) { + fprintf(stderr, "YNL: %s\n", yerr.msg); + return -1; + } + + req = ethtool_rings_set_req_alloc(); + ethtool_rings_set_req_set_header_dev_index(req, ifindex); + /* 0 - off, 1 - auto, 2 - on */ + ethtool_rings_set_req_set_tcp_data_split(req, on ? 2 : 0); + ret = ethtool_rings_set(ys, req); + if (ret < 0) + fprintf(stderr, "YNL failed: %s\n", ys->err.msg); + ethtool_rings_set_req_free(req); + + if (ret == 0) { + get_req = ethtool_rings_get_req_alloc(); + ethtool_rings_get_req_set_header_dev_index(get_req, ifindex); + get_rsp = ethtool_rings_get(ys, get_req); + ethtool_rings_get_req_free(get_req); + if (get_rsp) + fprintf(stderr, "TCP header split: %s\n", + tcp_data_split_str(get_rsp->tcp_data_split)); + ethtool_rings_get_rsp_free(get_rsp); + } + + ynl_sock_destroy(ys); + + return ret; } static int configure_rss(void) @@ -382,6 +432,9 @@ int do_server(struct memory_buffer *mem) if (reset_flow_steering()) error(1, 0, "Failed to reset flow steering\n"); + if (configure_headersplit(1)) + error(1, 0, "Failed to enable TCP header split\n"); + /* Configure RSS to divert all traffic from our devmem queues */ if (configure_rss()) error(1, 0, "Failed to configure rss\n"); -- 2.51.0 From d4ef05d211315395974fa846308c693ab2ea1ff2 Mon Sep 17 00:00:00 2001 From: Stanislav Fomichev Date: Thu, 7 Nov 2024 10:12:08 -0800 Subject: [PATCH 05/16] selftests: ncdevmem: Remove hard-coded queue numbers Use single last queue of the device and probe it dynamically. Reviewed-by: Mina Almasry Reviewed-by: Joe Damato Signed-off-by: Stanislav Fomichev Link: https://patch.msgid.link/20241107181211.3934153-10-sdf@fomichev.me Signed-off-by: Jakub Kicinski --- tools/testing/selftests/net/ncdevmem.c | 40 ++++++++++++++++++++++++-- 1 file changed, 38 insertions(+), 2 deletions(-) diff --git a/tools/testing/selftests/net/ncdevmem.c b/tools/testing/selftests/net/ncdevmem.c index 9ca2da3a2f63..1ea62c129ddc 100644 --- a/tools/testing/selftests/net/ncdevmem.c +++ b/tools/testing/selftests/net/ncdevmem.c @@ -75,8 +75,8 @@ static char *server_ip; static char *client_ip; static char *port; static size_t do_validation; -static int start_queue = 8; -static int num_queues = 8; +static int start_queue = -1; +static int num_queues = 1; static char *ifname; static unsigned int ifindex; static unsigned int dmabuf_id; @@ -208,6 +208,33 @@ void validate_buffer(void *line, size_t size) fprintf(stdout, "Validated buffer\n"); } +static int rxq_num(int ifindex) +{ + struct ethtool_channels_get_req *req; + struct ethtool_channels_get_rsp *rsp; + struct ynl_error yerr; + struct ynl_sock *ys; + int num = -1; + + ys = ynl_sock_create(&ynl_ethtool_family, &yerr); + if (!ys) { + fprintf(stderr, "YNL: %s\n", yerr.msg); + return -1; + } + + req = ethtool_channels_get_req_alloc(); + ethtool_channels_get_req_set_header_dev_index(req, ifindex); + rsp = ethtool_channels_get(ys, req); + if (rsp) + num = rsp->rx_count + rsp->combined_count; + ethtool_channels_get_req_free(req); + ethtool_channels_get_rsp_free(rsp); + + ynl_sock_destroy(ys); + + return num; +} + #define run_command(cmd, ...) \ ({ \ char command[256]; \ @@ -711,6 +738,15 @@ int main(int argc, char *argv[]) ifindex = if_nametoindex(ifname); + if (start_queue < 0) { + start_queue = rxq_num(ifindex) - 1; + + if (start_queue < 0) + error(1, 0, "couldn't detect number of queues\n"); + + fprintf(stderr, "using queues %d..%d\n", start_queue, start_queue + num_queues); + } + for (; optind < argc; optind++) fprintf(stderr, "extra arguments: %s\n", argv[optind]); -- 2.51.0 From 77f870a000165f364082e06bfd8fd16d331219d8 Mon Sep 17 00:00:00 2001 From: Stanislav Fomichev Date: Thu, 7 Nov 2024 10:12:09 -0800 Subject: [PATCH 06/16] selftests: ncdevmem: Run selftest when none of the -s or -c has been provided This will be used as a 'probe' mode in the selftest to check whether the device supports the devmem or not. Use hard-coded queue layout (two last queues) and prevent user from passing custom -q and/or -t. Reviewed-by: Mina Almasry Reviewed-by: Joe Damato Signed-off-by: Stanislav Fomichev Link: https://patch.msgid.link/20241107181211.3934153-11-sdf@fomichev.me Signed-off-by: Jakub Kicinski --- tools/testing/selftests/net/ncdevmem.c | 49 ++++++++++++++++++++------ 1 file changed, 39 insertions(+), 10 deletions(-) diff --git a/tools/testing/selftests/net/ncdevmem.c b/tools/testing/selftests/net/ncdevmem.c index 1ea62c129ddc..8e502a1f8f9b 100644 --- a/tools/testing/selftests/net/ncdevmem.c +++ b/tools/testing/selftests/net/ncdevmem.c @@ -76,7 +76,7 @@ static char *client_ip; static char *port; static size_t do_validation; static int start_queue = -1; -static int num_queues = 1; +static int num_queues = -1; static char *ifname; static unsigned int ifindex; static unsigned int dmabuf_id; @@ -727,19 +727,38 @@ int main(int argc, char *argv[]) } } - if (!server_ip) - error(1, 0, "Missing -s argument\n"); - - if (!port) - error(1, 0, "Missing -p argument\n"); - if (!ifname) error(1, 0, "Missing -f argument\n"); ifindex = if_nametoindex(ifname); - if (start_queue < 0) { - start_queue = rxq_num(ifindex) - 1; + if (!server_ip && !client_ip) { + if (start_queue < 0 && num_queues < 0) { + num_queues = rxq_num(ifindex); + if (num_queues < 0) + error(1, 0, "couldn't detect number of queues\n"); + if (num_queues < 2) + error(1, 0, + "number of device queues is too low\n"); + /* make sure can bind to multiple queues */ + start_queue = num_queues / 2; + num_queues /= 2; + } + + if (start_queue < 0 || num_queues < 0) + error(1, 0, "Both -t and -q are required\n"); + + run_devmem_tests(); + return 0; + } + + if (start_queue < 0 && num_queues < 0) { + num_queues = rxq_num(ifindex); + if (num_queues < 2) + error(1, 0, "number of device queues is too low\n"); + + num_queues = 1; + start_queue = rxq_num(ifindex) - num_queues; if (start_queue < 0) error(1, 0, "couldn't detect number of queues\n"); @@ -750,7 +769,17 @@ int main(int argc, char *argv[]) for (; optind < argc; optind++) fprintf(stderr, "extra arguments: %s\n", argv[optind]); - run_devmem_tests(); + if (start_queue < 0) + error(1, 0, "Missing -t argument\n"); + + if (num_queues < 0) + error(1, 0, "Missing -q argument\n"); + + if (!server_ip) + error(1, 0, "Missing -s argument\n"); + + if (!port) + error(1, 0, "Missing -p argument\n"); mem = provider->alloc(getpagesize() * NUM_PAGES); ret = is_server ? do_server(mem) : 1; -- 2.51.0 From be43a6b2382983c89b59166ba2c32ec0f1092cfe Mon Sep 17 00:00:00 2001 From: Stanislav Fomichev Date: Thu, 7 Nov 2024 10:12:10 -0800 Subject: [PATCH 07/16] selftests: ncdevmem: Move ncdevmem under drivers/net/hw This is where all the tests that depend on the HW functionality live in and this is where the automated test is gonna be added in the next patch. Reviewed-by: Mina Almasry Signed-off-by: Stanislav Fomichev Link: https://patch.msgid.link/20241107181211.3934153-12-sdf@fomichev.me Signed-off-by: Jakub Kicinski --- tools/testing/selftests/drivers/net/hw/.gitignore | 1 + tools/testing/selftests/drivers/net/hw/Makefile | 8 ++++++++ .../testing/selftests/{net => drivers/net/hw}/ncdevmem.c | 0 tools/testing/selftests/net/.gitignore | 1 - tools/testing/selftests/net/Makefile | 8 -------- 5 files changed, 9 insertions(+), 9 deletions(-) create mode 100644 tools/testing/selftests/drivers/net/hw/.gitignore rename tools/testing/selftests/{net => drivers/net/hw}/ncdevmem.c (100%) diff --git a/tools/testing/selftests/drivers/net/hw/.gitignore b/tools/testing/selftests/drivers/net/hw/.gitignore new file mode 100644 index 000000000000..e9fe6ede681a --- /dev/null +++ b/tools/testing/selftests/drivers/net/hw/.gitignore @@ -0,0 +1 @@ +ncdevmem diff --git a/tools/testing/selftests/drivers/net/hw/Makefile b/tools/testing/selftests/drivers/net/hw/Makefile index c9f2f48fc30f..182348f4bd40 100644 --- a/tools/testing/selftests/drivers/net/hw/Makefile +++ b/tools/testing/selftests/drivers/net/hw/Makefile @@ -26,4 +26,12 @@ TEST_INCLUDES := \ ../../../net/forwarding/tc_common.sh \ # +# YNL files, must be before "include ..lib.mk" +YNL_GEN_FILES := ncdevmem +TEST_GEN_FILES += $(YNL_GEN_FILES) + include ../../../lib.mk + +# YNL build +YNL_GENS := ethtool netdev +include ../../../net/ynl.mk diff --git a/tools/testing/selftests/net/ncdevmem.c b/tools/testing/selftests/drivers/net/hw/ncdevmem.c similarity index 100% rename from tools/testing/selftests/net/ncdevmem.c rename to tools/testing/selftests/drivers/net/hw/ncdevmem.c diff --git a/tools/testing/selftests/net/.gitignore b/tools/testing/selftests/net/.gitignore index 217d8b7a7365..a78debbd1fe7 100644 --- a/tools/testing/selftests/net/.gitignore +++ b/tools/testing/selftests/net/.gitignore @@ -18,7 +18,6 @@ ipv6_flowlabel_mgr log.txt msg_oob msg_zerocopy -ncdevmem nettest psock_fanout psock_snd diff --git a/tools/testing/selftests/net/Makefile b/tools/testing/selftests/net/Makefile index 61cce028f105..9322b904ad00 100644 --- a/tools/testing/selftests/net/Makefile +++ b/tools/testing/selftests/net/Makefile @@ -98,10 +98,6 @@ TEST_PROGS += vlan_hw_filter.sh TEST_PROGS += bpf_offload.py TEST_PROGS += ipv6_route_update_soft_lockup.sh -# YNL files, must be before "include ..lib.mk" -YNL_GEN_FILES := ncdevmem -TEST_GEN_FILES += $(YNL_GEN_FILES) - TEST_FILES := settings TEST_FILES += in_netns.sh lib.sh net_helper.sh setup_loopback.sh setup_veth.sh @@ -111,10 +107,6 @@ TEST_INCLUDES := forwarding/lib.sh include ../lib.mk -# YNL build -YNL_GENS := ethtool netdev -include ynl.mk - $(OUTPUT)/epoll_busy_poll: LDLIBS += -lcap $(OUTPUT)/reuseport_bpf_numa: LDLIBS += -lnuma $(OUTPUT)/tcp_mmap: LDLIBS += -lpthread -lcrypto -- 2.51.0 From 80230864b7b0fd9b54b294ab08a28f01d4193aa2 Mon Sep 17 00:00:00 2001 From: Stanislav Fomichev Date: Thu, 7 Nov 2024 10:12:11 -0800 Subject: [PATCH 08/16] selftests: ncdevmem: Add automated test Only RX side for now and small message to test the setup. In the future, we can extend it to TX side and to testing both sides with a couple of megs of data. make \ -C tools/testing/selftests \ TARGETS="drivers/hw/net" \ install INSTALL_PATH=~/tmp/ksft scp ~/tmp/ksft ${HOST}: scp ~/tmp/ksft ${PEER}: cfg+="NETIF=${DEV}\n" cfg+="LOCAL_V6=${HOST_IP}\n" cfg+="REMOTE_V6=${PEER_IP}\n" cfg+="REMOTE_TYPE=ssh\n" cfg+="REMOTE_ARGS=root@${PEER}\n" echo -e "$cfg" | ssh root@${HOST} "cat > ksft/drivers/net/net.config" ssh root@${HOST} "cd ksft && ./run_kselftest.sh -t drivers/net:devmem.py" Reviewed-by: Mina Almasry Signed-off-by: Stanislav Fomichev Reviewed-by: Joe Damato Link: https://patch.msgid.link/20241107181211.3934153-13-sdf@fomichev.me Signed-off-by: Jakub Kicinski --- .../testing/selftests/drivers/net/hw/Makefile | 1 + .../selftests/drivers/net/hw/devmem.py | 45 +++++++++++++++++++ 2 files changed, 46 insertions(+) create mode 100755 tools/testing/selftests/drivers/net/hw/devmem.py diff --git a/tools/testing/selftests/drivers/net/hw/Makefile b/tools/testing/selftests/drivers/net/hw/Makefile index 182348f4bd40..1c6a77480923 100644 --- a/tools/testing/selftests/drivers/net/hw/Makefile +++ b/tools/testing/selftests/drivers/net/hw/Makefile @@ -3,6 +3,7 @@ TEST_PROGS = \ csum.py \ devlink_port_split.py \ + devmem.py \ ethtool.sh \ ethtool_extended_state.sh \ ethtool_mm.sh \ diff --git a/tools/testing/selftests/drivers/net/hw/devmem.py b/tools/testing/selftests/drivers/net/hw/devmem.py new file mode 100755 index 000000000000..1223f0f5c10c --- /dev/null +++ b/tools/testing/selftests/drivers/net/hw/devmem.py @@ -0,0 +1,45 @@ +#!/usr/bin/env python3 +# SPDX-License-Identifier: GPL-2.0 + +from lib.py import ksft_run, ksft_exit +from lib.py import ksft_eq, KsftSkipEx +from lib.py import NetDrvEpEnv +from lib.py import bkg, cmd, rand_port, wait_port_listen +from lib.py import ksft_disruptive + + +def require_devmem(cfg): + if not hasattr(cfg, "_devmem_probed"): + port = rand_port() + probe_command = f"./ncdevmem -f {cfg.ifname}" + cfg._devmem_supported = cmd(probe_command, fail=False, shell=True).ret == 0 + cfg._devmem_probed = True + + if not cfg._devmem_supported: + raise KsftSkipEx("Test requires devmem support") + + +@ksft_disruptive +def check_rx(cfg) -> None: + cfg.require_v6() + require_devmem(cfg) + + port = rand_port() + listen_cmd = f"./ncdevmem -l -f {cfg.ifname} -s {cfg.v6} -p {port}" + + with bkg(listen_cmd) as socat: + wait_port_listen(port) + cmd(f"echo -e \"hello\\nworld\"| socat -u - TCP6:[{cfg.v6}]:{port}", host=cfg.remote, shell=True) + + ksft_eq(socat.stdout.strip(), "hello\nworld") + + +def main() -> None: + with NetDrvEpEnv(__file__) as cfg: + ksft_run([check_rx], + args=(cfg, )) + ksft_exit() + + +if __name__ == "__main__": + main() -- 2.51.0 From 7a3bcd39ae1f0e3ab896d9df62339ab4297a0bfd Mon Sep 17 00:00:00 2001 From: Heiner Kallweit Date: Sat, 9 Nov 2024 23:12:12 +0100 Subject: [PATCH 09/16] r8169: use helper r8169_mod_reg8_cond to simplify rtl_jumbo_config Use recently added helper r8169_mod_reg8_cond() to simplify jumbo mode configuration. Signed-off-by: Heiner Kallweit Reviewed-by: Simon Horman Link: https://patch.msgid.link/3df1d484-a02e-46e7-8f75-db5b428e422e@gmail.com Signed-off-by: Jakub Kicinski --- drivers/net/ethernet/realtek/r8169_main.c | 77 ++++------------------- 1 file changed, 11 insertions(+), 66 deletions(-) diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c index 6578a9947b82..907a482c012f 100644 --- a/drivers/net/ethernet/realtek/r8169_main.c +++ b/drivers/net/ethernet/realtek/r8169_main.c @@ -2543,86 +2543,31 @@ static void rtl8169_init_ring_indexes(struct rtl8169_private *tp) tp->dirty_tx = tp->cur_tx = tp->cur_rx = 0; } -static void r8168c_hw_jumbo_enable(struct rtl8169_private *tp) -{ - RTL_W8(tp, Config3, RTL_R8(tp, Config3) | Jumbo_En0); - RTL_W8(tp, Config4, RTL_R8(tp, Config4) | Jumbo_En1); -} - -static void r8168c_hw_jumbo_disable(struct rtl8169_private *tp) -{ - RTL_W8(tp, Config3, RTL_R8(tp, Config3) & ~Jumbo_En0); - RTL_W8(tp, Config4, RTL_R8(tp, Config4) & ~Jumbo_En1); -} - -static void r8168dp_hw_jumbo_enable(struct rtl8169_private *tp) -{ - RTL_W8(tp, Config3, RTL_R8(tp, Config3) | Jumbo_En0); -} - -static void r8168dp_hw_jumbo_disable(struct rtl8169_private *tp) -{ - RTL_W8(tp, Config3, RTL_R8(tp, Config3) & ~Jumbo_En0); -} - -static void r8168e_hw_jumbo_enable(struct rtl8169_private *tp) -{ - RTL_W8(tp, MaxTxPacketSize, 0x24); - RTL_W8(tp, Config3, RTL_R8(tp, Config3) | Jumbo_En0); - RTL_W8(tp, Config4, RTL_R8(tp, Config4) | 0x01); -} - -static void r8168e_hw_jumbo_disable(struct rtl8169_private *tp) -{ - RTL_W8(tp, MaxTxPacketSize, 0x3f); - RTL_W8(tp, Config3, RTL_R8(tp, Config3) & ~Jumbo_En0); - RTL_W8(tp, Config4, RTL_R8(tp, Config4) & ~0x01); -} - -static void r8168b_1_hw_jumbo_enable(struct rtl8169_private *tp) -{ - RTL_W8(tp, Config4, RTL_R8(tp, Config4) | (1 << 0)); -} - -static void r8168b_1_hw_jumbo_disable(struct rtl8169_private *tp) -{ - RTL_W8(tp, Config4, RTL_R8(tp, Config4) & ~(1 << 0)); -} - static void rtl_jumbo_config(struct rtl8169_private *tp) { bool jumbo = tp->dev->mtu > ETH_DATA_LEN; int readrq = 4096; + if (jumbo && tp->mac_version >= RTL_GIGA_MAC_VER_17 && + tp->mac_version <= RTL_GIGA_MAC_VER_26) + readrq = 512; + rtl_unlock_config_regs(tp); switch (tp->mac_version) { case RTL_GIGA_MAC_VER_17: - if (jumbo) { - readrq = 512; - r8168b_1_hw_jumbo_enable(tp); - } else { - r8168b_1_hw_jumbo_disable(tp); - } + r8169_mod_reg8_cond(tp, Config4, BIT(0), jumbo); break; case RTL_GIGA_MAC_VER_18 ... RTL_GIGA_MAC_VER_26: - if (jumbo) { - readrq = 512; - r8168c_hw_jumbo_enable(tp); - } else { - r8168c_hw_jumbo_disable(tp); - } + r8169_mod_reg8_cond(tp, Config3, Jumbo_En0, jumbo); + r8169_mod_reg8_cond(tp, Config4, Jumbo_En1, jumbo); break; case RTL_GIGA_MAC_VER_28: - if (jumbo) - r8168dp_hw_jumbo_enable(tp); - else - r8168dp_hw_jumbo_disable(tp); + r8169_mod_reg8_cond(tp, Config3, Jumbo_En0, jumbo); break; case RTL_GIGA_MAC_VER_31 ... RTL_GIGA_MAC_VER_33: - if (jumbo) - r8168e_hw_jumbo_enable(tp); - else - r8168e_hw_jumbo_disable(tp); + RTL_W8(tp, MaxTxPacketSize, jumbo ? 0x24 : 0x3f); + r8169_mod_reg8_cond(tp, Config3, Jumbo_En0, jumbo); + r8169_mod_reg8_cond(tp, Config4, BIT(0), jumbo); break; default: break; -- 2.51.0 From d5ec8d91f82ef78405b506737952dec8af95a95b Mon Sep 17 00:00:00 2001 From: Kuniyuki Iwashima Date: Thu, 7 Nov 2024 16:48:14 -0800 Subject: [PATCH 10/16] rtnetlink: Remove __rtnl_link_unregister(). rtnl_link_unregister() holds RTNL and calls __rtnl_link_unregister(), where we call synchronize_srcu() to wait inflight RTM_NEWLINK requests for per-netns RTNL. We put synchronize_srcu() in __rtnl_link_unregister() due to ifb.ko and dummy.ko. However, rtnl_newlink() will acquire SRCU before RTNL later in this series. Then, lockdep will detect the deadlock: rtnl_link_unregister() rtnl_newlink() ---- ---- lock(rtnl_mutex); lock(&ops->srcu); lock(rtnl_mutex); sync(&ops->srcu); To avoid the problem, we must call synchronize_srcu() before RTNL in rtnl_link_unregister(). As a preparation, let's remove __rtnl_link_unregister(). Signed-off-by: Kuniyuki Iwashima Link: https://patch.msgid.link/20241108004823.29419-2-kuniyu@amazon.com Signed-off-by: Jakub Kicinski --- drivers/net/dummy.c | 6 +++++- drivers/net/ifb.c | 6 +++++- include/net/rtnetlink.h | 1 - net/core/rtnetlink.c | 32 ++++++++++---------------------- 4 files changed, 20 insertions(+), 25 deletions(-) diff --git a/drivers/net/dummy.c b/drivers/net/dummy.c index e9c5e1e11fa0..72618b6af44e 100644 --- a/drivers/net/dummy.c +++ b/drivers/net/dummy.c @@ -166,6 +166,7 @@ err: static int __init dummy_init_module(void) { + bool need_unregister = false; int i, err = 0; down_write(&pernet_ops_rwsem); @@ -179,12 +180,15 @@ static int __init dummy_init_module(void) cond_resched(); } if (err < 0) - __rtnl_link_unregister(&dummy_link_ops); + need_unregister = true; out: rtnl_unlock(); up_write(&pernet_ops_rwsem); + if (need_unregister) + rtnl_link_unregister(&dummy_link_ops); + return err; } diff --git a/drivers/net/ifb.c b/drivers/net/ifb.c index 2c1b5def4a0b..a4b9ec4e8f30 100644 --- a/drivers/net/ifb.c +++ b/drivers/net/ifb.c @@ -424,6 +424,7 @@ err: static int __init ifb_init_module(void) { + bool need_unregister = false; int i, err; down_write(&pernet_ops_rwsem); @@ -437,12 +438,15 @@ static int __init ifb_init_module(void) cond_resched(); } if (err) - __rtnl_link_unregister(&ifb_link_ops); + need_unregister = true; out: rtnl_unlock(); up_write(&pernet_ops_rwsem); + if (need_unregister) + rtnl_link_unregister(&ifb_link_ops); + return err; } diff --git a/include/net/rtnetlink.h b/include/net/rtnetlink.h index b260c0cc9671..3ebfcc6e56fd 100644 --- a/include/net/rtnetlink.h +++ b/include/net/rtnetlink.h @@ -165,7 +165,6 @@ struct rtnl_link_ops { }; int __rtnl_link_register(struct rtnl_link_ops *ops); -void __rtnl_link_unregister(struct rtnl_link_ops *ops); int rtnl_link_register(struct rtnl_link_ops *ops); void rtnl_link_unregister(struct rtnl_link_ops *ops); diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c index a5c386a45501..f0246ecec7fa 100644 --- a/net/core/rtnetlink.c +++ b/net/core/rtnetlink.c @@ -568,27 +568,6 @@ static void __rtnl_kill_links(struct net *net, struct rtnl_link_ops *ops) unregister_netdevice_many(&list_kill); } -/** - * __rtnl_link_unregister - Unregister rtnl_link_ops from rtnetlink. - * @ops: struct rtnl_link_ops * to unregister - * - * The caller must hold the rtnl_mutex and guarantee net_namespace_list - * integrity (hold pernet_ops_rwsem for writing to close the race - * with setup_net() and cleanup_net()). - */ -void __rtnl_link_unregister(struct rtnl_link_ops *ops) -{ - struct net *net; - - list_del_rcu(&ops->list); - synchronize_srcu(&ops->srcu); - cleanup_srcu_struct(&ops->srcu); - - for_each_net(net) - __rtnl_kill_links(net, ops); -} -EXPORT_SYMBOL_GPL(__rtnl_link_unregister); - /* Return with the rtnl_lock held when there are no network * devices unregistering in any network namespace. */ @@ -617,10 +596,19 @@ static void rtnl_lock_unregistering_all(void) */ void rtnl_link_unregister(struct rtnl_link_ops *ops) { + struct net *net; + /* Close the race with setup_net() and cleanup_net() */ down_write(&pernet_ops_rwsem); rtnl_lock_unregistering_all(); - __rtnl_link_unregister(ops); + + list_del_rcu(&ops->list); + synchronize_srcu(&ops->srcu); + cleanup_srcu_struct(&ops->srcu); + + for_each_net(net) + __rtnl_kill_links(net, ops); + rtnl_unlock(); up_write(&pernet_ops_rwsem); } -- 2.51.0 From 6b57ff21a3109b1dba2d286ff415463e6fb1fca3 Mon Sep 17 00:00:00 2001 From: Kuniyuki Iwashima Date: Thu, 7 Nov 2024 16:48:15 -0800 Subject: [PATCH 11/16] rtnetlink: Protect link_ops by mutex. rtnl_link_unregister() holds RTNL and calls synchronize_srcu(), but rtnl_newlink() will acquire SRCU frist and then RTNL. Then, we need to unlink ops and call synchronize_srcu() outside of RTNL to avoid the deadlock. rtnl_link_unregister() rtnl_newlink() ---- ---- lock(rtnl_mutex); lock(&ops->srcu); lock(rtnl_mutex); sync(&ops->srcu); Let's move as such and add a mutex to protect link_ops. Now, link_ops is protected by its dedicated mutex and rtnl_link_register() no longer needs to hold RTNL. While at it, we move the initialisation of ops->dellink and ops->srcu out of the mutex scope. Signed-off-by: Kuniyuki Iwashima Link: https://patch.msgid.link/20241108004823.29419-3-kuniyu@amazon.com Signed-off-by: Jakub Kicinski --- include/net/rtnetlink.h | 2 +- net/core/rtnetlink.c | 33 ++++++++++++++++++++------------- 2 files changed, 21 insertions(+), 14 deletions(-) diff --git a/include/net/rtnetlink.h b/include/net/rtnetlink.h index 3ebfcc6e56fd..7559020f760c 100644 --- a/include/net/rtnetlink.h +++ b/include/net/rtnetlink.h @@ -71,7 +71,7 @@ static inline int rtnl_msg_family(const struct nlmsghdr *nlh) /** * struct rtnl_link_ops - rtnetlink link operations * - * @list: Used internally, protected by RTNL and SRCU + * @list: Used internally, protected by link_ops_mutex and SRCU * @srcu: Used internally * @kind: Identifier * @netns_refund: Physical device, move to init_net on netns exit diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c index f0246ecec7fa..21154ef0048f 100644 --- a/net/core/rtnetlink.c +++ b/net/core/rtnetlink.c @@ -466,6 +466,7 @@ void __rtnl_unregister_many(const struct rtnl_msg_handler *handlers, int n) } EXPORT_SYMBOL_GPL(__rtnl_unregister_many); +static DEFINE_MUTEX(link_ops_mutex); static LIST_HEAD(link_ops); static struct rtnl_link_ops *rtnl_link_ops_get(const char *kind, int *srcu_index) @@ -508,14 +509,6 @@ int __rtnl_link_register(struct rtnl_link_ops *ops) struct rtnl_link_ops *tmp; int err; - /* When RTNL is removed, add lock for link_ops. */ - ASSERT_RTNL(); - - list_for_each_entry(tmp, &link_ops, list) { - if (!strcmp(ops->kind, tmp->kind)) - return -EEXIST; - } - /* The check for alloc/setup is here because if ops * does not have that filled up, it is not possible * to use the ops for creating device. So do not @@ -528,9 +521,20 @@ int __rtnl_link_register(struct rtnl_link_ops *ops) if (err) return err; + mutex_lock(&link_ops_mutex); + + list_for_each_entry(tmp, &link_ops, list) { + if (!strcmp(ops->kind, tmp->kind)) { + err = -EEXIST; + goto unlock; + } + } + list_add_tail_rcu(&ops->list, &link_ops); +unlock: + mutex_unlock(&link_ops_mutex); - return 0; + return err; } EXPORT_SYMBOL_GPL(__rtnl_link_register); @@ -598,14 +602,17 @@ void rtnl_link_unregister(struct rtnl_link_ops *ops) { struct net *net; - /* Close the race with setup_net() and cleanup_net() */ - down_write(&pernet_ops_rwsem); - rtnl_lock_unregistering_all(); - + mutex_lock(&link_ops_mutex); list_del_rcu(&ops->list); + mutex_unlock(&link_ops_mutex); + synchronize_srcu(&ops->srcu); cleanup_srcu_struct(&ops->srcu); + /* Close the race with setup_net() and cleanup_net() */ + down_write(&pernet_ops_rwsem); + rtnl_lock_unregistering_all(); + for_each_net(net) __rtnl_kill_links(net, ops); -- 2.51.0 From 68297dbb967f87c3c92af9d2f652270f57c547c7 Mon Sep 17 00:00:00 2001 From: Kuniyuki Iwashima Date: Thu, 7 Nov 2024 16:48:16 -0800 Subject: [PATCH 12/16] rtnetlink: Remove __rtnl_link_register() link_ops is protected by link_ops_mutex and no longer needs RTNL, so we have no reason to have __rtnl_link_register() separately. Let's remove it and call rtnl_link_register() from ifb.ko and dummy.ko. Note that both modules' init() work on init_net only, so we need not export pernet_ops_rwsem and can use rtnl_net_lock() there. Signed-off-by: Kuniyuki Iwashima Link: https://patch.msgid.link/20241108004823.29419-4-kuniyu@amazon.com Signed-off-by: Jakub Kicinski --- drivers/net/dummy.c | 17 ++++++----------- drivers/net/ifb.c | 17 ++++++----------- include/net/rtnetlink.h | 2 -- net/core/net_namespace.c | 1 - net/core/rtnetlink.c | 35 +++++++---------------------------- 5 files changed, 19 insertions(+), 53 deletions(-) diff --git a/drivers/net/dummy.c b/drivers/net/dummy.c index 72618b6af44e..005d79975f3b 100644 --- a/drivers/net/dummy.c +++ b/drivers/net/dummy.c @@ -166,27 +166,22 @@ err: static int __init dummy_init_module(void) { - bool need_unregister = false; int i, err = 0; - down_write(&pernet_ops_rwsem); - rtnl_lock(); - err = __rtnl_link_register(&dummy_link_ops); + err = rtnl_link_register(&dummy_link_ops); if (err < 0) - goto out; + return err; + + rtnl_net_lock(&init_net); for (i = 0; i < numdummies && !err; i++) { err = dummy_init_one(); cond_resched(); } - if (err < 0) - need_unregister = true; -out: - rtnl_unlock(); - up_write(&pernet_ops_rwsem); + rtnl_net_unlock(&init_net); - if (need_unregister) + if (err < 0) rtnl_link_unregister(&dummy_link_ops); return err; diff --git a/drivers/net/ifb.c b/drivers/net/ifb.c index a4b9ec4e8f30..67424888ff0a 100644 --- a/drivers/net/ifb.c +++ b/drivers/net/ifb.c @@ -424,27 +424,22 @@ err: static int __init ifb_init_module(void) { - bool need_unregister = false; int i, err; - down_write(&pernet_ops_rwsem); - rtnl_lock(); - err = __rtnl_link_register(&ifb_link_ops); + err = rtnl_link_register(&ifb_link_ops); if (err < 0) - goto out; + return err; + + rtnl_net_lock(&init_net); for (i = 0; i < numifbs && !err; i++) { err = ifb_init_one(i); cond_resched(); } - if (err) - need_unregister = true; -out: - rtnl_unlock(); - up_write(&pernet_ops_rwsem); + rtnl_net_unlock(&init_net); - if (need_unregister) + if (err) rtnl_link_unregister(&ifb_link_ops); return err; diff --git a/include/net/rtnetlink.h b/include/net/rtnetlink.h index 7559020f760c..ef7c11f0d74c 100644 --- a/include/net/rtnetlink.h +++ b/include/net/rtnetlink.h @@ -164,8 +164,6 @@ struct rtnl_link_ops { int *prividx, int attr); }; -int __rtnl_link_register(struct rtnl_link_ops *ops); - int rtnl_link_register(struct rtnl_link_ops *ops); void rtnl_link_unregister(struct rtnl_link_ops *ops); diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c index 809b48c0a528..157021ced442 100644 --- a/net/core/net_namespace.c +++ b/net/core/net_namespace.c @@ -56,7 +56,6 @@ static bool init_net_initialized; * outside. */ DECLARE_RWSEM(pernet_ops_rwsem); -EXPORT_SYMBOL_GPL(pernet_ops_rwsem); #define MIN_PERNET_OPS_ID \ ((sizeof(struct net_generic) + sizeof(void *) - 1) / sizeof(void *)) diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c index 21154ef0048f..e8357a3b9c7e 100644 --- a/net/core/rtnetlink.c +++ b/net/core/rtnetlink.c @@ -495,20 +495,21 @@ static void rtnl_link_ops_put(struct rtnl_link_ops *ops, int srcu_index) } /** - * __rtnl_link_register - Register rtnl_link_ops with rtnetlink. + * rtnl_link_register - Register rtnl_link_ops with rtnetlink. * @ops: struct rtnl_link_ops * to register * - * The caller must hold the rtnl_mutex. This function should be used - * by drivers that create devices during module initialization. It - * must be called before registering the devices. - * * Returns 0 on success or a negative error code. */ -int __rtnl_link_register(struct rtnl_link_ops *ops) +int rtnl_link_register(struct rtnl_link_ops *ops) { struct rtnl_link_ops *tmp; int err; + /* Sanity-check max sizes to avoid stack buffer overflow. */ + if (WARN_ON(ops->maxtype > RTNL_MAX_TYPE || + ops->slave_maxtype > RTNL_SLAVE_MAX_TYPE)) + return -EINVAL; + /* The check for alloc/setup is here because if ops * does not have that filled up, it is not possible * to use the ops for creating device. So do not @@ -536,28 +537,6 @@ unlock: return err; } -EXPORT_SYMBOL_GPL(__rtnl_link_register); - -/** - * rtnl_link_register - Register rtnl_link_ops with rtnetlink. - * @ops: struct rtnl_link_ops * to register - * - * Returns 0 on success or a negative error code. - */ -int rtnl_link_register(struct rtnl_link_ops *ops) -{ - int err; - - /* Sanity-check max sizes to avoid stack buffer overflow. */ - if (WARN_ON(ops->maxtype > RTNL_MAX_TYPE || - ops->slave_maxtype > RTNL_SLAVE_MAX_TYPE)) - return -EINVAL; - - rtnl_lock(); - err = __rtnl_link_register(ops); - rtnl_unlock(); - return err; -} EXPORT_SYMBOL_GPL(rtnl_link_register); static void __rtnl_kill_links(struct net *net, struct rtnl_link_ops *ops) -- 2.51.0 From cbaaa6326bc58e75239df437a8fdcdb2335d3b24 Mon Sep 17 00:00:00 2001 From: Kuniyuki Iwashima Date: Thu, 7 Nov 2024 16:48:17 -0800 Subject: [PATCH 13/16] rtnetlink: Introduce struct rtnl_nets and helpers. rtnl_newlink() needs to hold 3 per-netns RTNL: 2 for a new device and 1 for its peer. We will add rtnl_nets_lock() later, which performs the nested locking based on struct rtnl_nets, which has an array of struct net pointers. rtnl_nets_add() adds a net pointer to the array and sorts it so that rtnl_nets_lock() can simply acquire per-netns RTNL from array[0] to [2]. Before calling rtnl_nets_add(), get_net() must be called for the net, and rtnl_nets_destroy() will call put_net() for each. Let's apply the helpers to rtnl_newlink(). When CONFIG_DEBUG_NET_SMALL_RTNL is disabled, we do not call rtnl_net_lock() thus do not care about the array order, so rtnl_net_cmp_locks() returns -1 so that the loop in rtnl_nets_add() can be optimised to NOP. Signed-off-by: Kuniyuki Iwashima Reviewed-by: Eric Dumazet Reviewed-by: Nikolay Aleksandrov Link: https://patch.msgid.link/20241108004823.29419-5-kuniyu@amazon.com Signed-off-by: Jakub Kicinski --- net/core/rtnetlink.c | 70 ++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 67 insertions(+), 3 deletions(-) diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c index e8357a3b9c7e..960d9d2c6aec 100644 --- a/net/core/rtnetlink.c +++ b/net/core/rtnetlink.c @@ -258,8 +258,67 @@ bool lockdep_rtnl_net_is_held(struct net *net) return lockdep_rtnl_is_held() && lockdep_is_held(&net->rtnl_mutex); } EXPORT_SYMBOL(lockdep_rtnl_net_is_held); +#else +static int rtnl_net_cmp_locks(const struct net *net_a, const struct net *net_b) +{ + /* No need to swap */ + return -1; +} #endif +struct rtnl_nets { + /* ->newlink() needs to freeze 3 netns at most; + * 2 for the new device, 1 for its peer. + */ + struct net *net[3]; + unsigned char len; +}; + +static void rtnl_nets_init(struct rtnl_nets *rtnl_nets) +{ + memset(rtnl_nets, 0, sizeof(*rtnl_nets)); +} + +static void rtnl_nets_destroy(struct rtnl_nets *rtnl_nets) +{ + int i; + + for (i = 0; i < rtnl_nets->len; i++) { + put_net(rtnl_nets->net[i]); + rtnl_nets->net[i] = NULL; + } + + rtnl_nets->len = 0; +} + +/** + * rtnl_nets_add - Add netns to be locked before ->newlink(). + * + * @rtnl_nets: rtnl_nets pointer passed to ->get_peer_net(). + * @net: netns pointer with an extra refcnt held. + * + * The extra refcnt is released in rtnl_nets_destroy(). + */ +static void rtnl_nets_add(struct rtnl_nets *rtnl_nets, struct net *net) +{ + int i; + + DEBUG_NET_WARN_ON_ONCE(rtnl_nets->len == ARRAY_SIZE(rtnl_nets->net)); + + for (i = 0; i < rtnl_nets->len; i++) { + switch (rtnl_net_cmp_locks(rtnl_nets->net[i], net)) { + case 0: + put_net(net); + return; + case 1: + swap(rtnl_nets->net[i], net); + } + } + + rtnl_nets->net[i] = net; + rtnl_nets->len++; +} + static struct rtnl_link __rcu *__rcu *rtnl_msg_handlers[RTNL_FAMILY_MAX + 1]; static inline int rtm_msgindex(int msgtype) @@ -3767,6 +3826,7 @@ static int rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh, struct net *tgt_net, *link_net = NULL; struct rtnl_link_ops *ops = NULL; struct rtnl_newlink_tbs *tbs; + struct rtnl_nets rtnl_nets; int ops_srcu_index; int ret; @@ -3810,6 +3870,8 @@ static int rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh, #endif } + rtnl_nets_init(&rtnl_nets); + if (ops) { if (ops->maxtype > RTNL_MAX_TYPE) { ret = -EINVAL; @@ -3839,6 +3901,8 @@ static int rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh, goto put_ops; } + rtnl_nets_add(&rtnl_nets, tgt_net); + if (tb[IFLA_LINK_NETNSID]) { int id = nla_get_s32(tb[IFLA_LINK_NETNSID]); @@ -3849,6 +3913,8 @@ static int rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh, goto put_net; } + rtnl_nets_add(&rtnl_nets, link_net); + if (!netlink_ns_capable(skb, link_net->user_ns, CAP_NET_ADMIN)) { ret = -EPERM; goto put_net; @@ -3858,9 +3924,7 @@ static int rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh, ret = __rtnl_newlink(skb, nlh, ops, tgt_net, link_net, tbs, data, extack); put_net: - if (link_net) - put_net(link_net); - put_net(tgt_net); + rtnl_nets_destroy(&rtnl_nets); put_ops: if (ops) rtnl_link_ops_put(ops, ops_srcu_index); -- 2.51.0 From 28690e5361c05fd4ef0ca3a17d1c667cba790554 Mon Sep 17 00:00:00 2001 From: Kuniyuki Iwashima Date: Thu, 7 Nov 2024 16:48:18 -0800 Subject: [PATCH 14/16] rtnetlink: Add peer_type in struct rtnl_link_ops. In ops->newlink(), veth, vxcan, and netkit call rtnl_link_get_net() with a net pointer, which is the first argument of ->newlink(). rtnl_link_get_net() could return another netns based on IFLA_NET_NS_PID and IFLA_NET_NS_FD in the peer device's attributes. We want to get it and fill rtnl_nets->nets[] in advance in rtnl_newlink() for per-netns RTNL. All of the three get the peer netns in the same way: 1. Call rtnl_nla_parse_ifinfomsg() 2. Call ops->validate() (vxcan doesn't have) 3. Call rtnl_link_get_net_tb() Let's add a new field peer_type to struct rtnl_link_ops and prefetch netns in the peer ifla to add it to rtnl_nets in rtnl_newlink(). Signed-off-by: Kuniyuki Iwashima Reviewed-by: Eric Dumazet Reviewed-by: Nikolay Aleksandrov Link: https://patch.msgid.link/20241108004823.29419-6-kuniyu@amazon.com Signed-off-by: Jakub Kicinski --- include/net/rtnetlink.h | 2 ++ net/core/rtnetlink.c | 55 ++++++++++++++++++++++++++++++++++++++--- 2 files changed, 53 insertions(+), 4 deletions(-) diff --git a/include/net/rtnetlink.h b/include/net/rtnetlink.h index ef7c11f0d74c..bef76abcff8d 100644 --- a/include/net/rtnetlink.h +++ b/include/net/rtnetlink.h @@ -75,6 +75,7 @@ static inline int rtnl_msg_family(const struct nlmsghdr *nlh) * @srcu: Used internally * @kind: Identifier * @netns_refund: Physical device, move to init_net on netns exit + * @peer_type: Peer device specific netlink attribute number (e.g. VETH_INFO_PEER) * @maxtype: Highest device specific netlink attribute number * @policy: Netlink policy for device specific attribute validation * @validate: Optional validation function for netlink/changelink parameters @@ -116,6 +117,7 @@ struct rtnl_link_ops { void (*setup)(struct net_device *dev); bool netns_refund; + const u16 peer_type; unsigned int maxtype; const struct nla_policy *policy; int (*validate)(struct nlattr *tb[], diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c index 960d9d2c6aec..1af187a4a3f1 100644 --- a/net/core/rtnetlink.c +++ b/net/core/rtnetlink.c @@ -2492,9 +2492,10 @@ int rtnl_nla_parse_ifinfomsg(struct nlattr **tb, const struct nlattr *nla_peer, } EXPORT_SYMBOL(rtnl_nla_parse_ifinfomsg); -struct net *rtnl_link_get_net(struct net *src_net, struct nlattr *tb[]) +static struct net *rtnl_link_get_net_ifla(struct nlattr *tb[]) { - struct net *net; + struct net *net = NULL; + /* Examine the link attributes and figure out which * network namespace we are talking about. */ @@ -2502,8 +2503,17 @@ struct net *rtnl_link_get_net(struct net *src_net, struct nlattr *tb[]) net = get_net_ns_by_pid(nla_get_u32(tb[IFLA_NET_NS_PID])); else if (tb[IFLA_NET_NS_FD]) net = get_net_ns_by_fd(nla_get_u32(tb[IFLA_NET_NS_FD])); - else + + return net; +} + +struct net *rtnl_link_get_net(struct net *src_net, struct nlattr *tb[]) +{ + struct net *net = rtnl_link_get_net_ifla(tb); + + if (!net) net = get_net(src_net); + return net; } EXPORT_SYMBOL(rtnl_link_get_net); @@ -3765,6 +3775,37 @@ out_unregister: goto out; } +static int rtnl_add_peer_net(struct rtnl_nets *rtnl_nets, + const struct rtnl_link_ops *ops, + struct nlattr *data[], + struct netlink_ext_ack *extack) +{ + struct nlattr *tb[IFLA_MAX + 1]; + struct net *net; + int err; + + if (!data || !data[ops->peer_type]) + return 0; + + err = rtnl_nla_parse_ifinfomsg(tb, data[ops->peer_type], extack); + if (err < 0) + return err; + + if (ops->validate) { + err = ops->validate(tb, NULL, extack); + if (err < 0) + return err; + } + + net = rtnl_link_get_net_ifla(tb); + if (IS_ERR(net)) + return PTR_ERR(net); + if (net) + rtnl_nets_add(rtnl_nets, net); + + return 0; +} + static int __rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh, const struct rtnl_link_ops *ops, struct net *tgt_net, struct net *link_net, @@ -3893,12 +3934,18 @@ static int rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh, if (ret < 0) goto put_ops; } + + if (ops->peer_type) { + ret = rtnl_add_peer_net(&rtnl_nets, ops, data, extack); + if (ret < 0) + goto put_ops; + } } tgt_net = rtnl_link_get_net_capable(skb, sock_net(skb->sk), tb, CAP_NET_ADMIN); if (IS_ERR(tgt_net)) { ret = PTR_ERR(tgt_net); - goto put_ops; + goto put_net; } rtnl_nets_add(&rtnl_nets, tgt_net); -- 2.51.0 From 0eb87b02a7058f1dc64bcd6fa619d8556186de3d Mon Sep 17 00:00:00 2001 From: Kuniyuki Iwashima Date: Thu, 7 Nov 2024 16:48:19 -0800 Subject: [PATCH 15/16] veth: Set VETH_INFO_PEER to veth_link_ops.peer_type. For per-netns RTNL, we need to prefetch the peer device's netns. Let's set rtnl_link_ops.peer_type and accordingly remove duplicated validation in ->newlink(). Signed-off-by: Kuniyuki Iwashima Reviewed-by: Eric Dumazet Reviewed-by: Nikolay Aleksandrov Link: https://patch.msgid.link/20241108004823.29419-7-kuniyu@amazon.com Signed-off-by: Jakub Kicinski --- drivers/net/veth.c | 18 ++++-------------- 1 file changed, 4 insertions(+), 14 deletions(-) diff --git a/drivers/net/veth.c b/drivers/net/veth.c index 18148e068aa0..0d6d0d749d44 100644 --- a/drivers/net/veth.c +++ b/drivers/net/veth.c @@ -1781,19 +1781,11 @@ static int veth_newlink(struct net *src_net, struct net_device *dev, /* * create and register peer first */ - if (data != NULL && data[VETH_INFO_PEER] != NULL) { - struct nlattr *nla_peer; + if (data && data[VETH_INFO_PEER]) { + struct nlattr *nla_peer = data[VETH_INFO_PEER]; - nla_peer = data[VETH_INFO_PEER]; ifmp = nla_data(nla_peer); - err = rtnl_nla_parse_ifinfomsg(peer_tb, nla_peer, extack); - if (err < 0) - return err; - - err = veth_validate(peer_tb, NULL, extack); - if (err < 0) - return err; - + rtnl_nla_parse_ifinfomsg(peer_tb, nla_peer, extack); tbp = peer_tb; } else { ifmp = NULL; @@ -1809,9 +1801,6 @@ static int veth_newlink(struct net *src_net, struct net_device *dev, } net = rtnl_link_get_net(src_net, tbp); - if (IS_ERR(net)) - return PTR_ERR(net); - peer = rtnl_create_link(net, ifname, name_assign_type, &veth_link_ops, tbp, extack); if (IS_ERR(peer)) { @@ -1952,6 +1941,7 @@ static struct rtnl_link_ops veth_link_ops = { .newlink = veth_newlink, .dellink = veth_dellink, .policy = veth_policy, + .peer_type = VETH_INFO_PEER, .maxtype = VETH_INFO_MAX, .get_link_net = veth_get_link_net, .get_num_tx_queues = veth_get_num_queues, -- 2.51.0 From 6b84e558e95d95f3bb1139dfbb693bc22c760dad Mon Sep 17 00:00:00 2001 From: Kuniyuki Iwashima Date: Thu, 7 Nov 2024 16:48:20 -0800 Subject: [PATCH 16/16] vxcan: Set VXCAN_INFO_PEER to vxcan_link_ops.peer_type. For per-netns RTNL, we need to prefetch the peer device's netns. Let's set rtnl_link_ops.peer_type and accordingly remove duplicated validation in ->newlink(). Signed-off-by: Kuniyuki Iwashima Reviewed-by: Eric Dumazet Reviewed-by: Nikolay Aleksandrov Link: https://patch.msgid.link/20241108004823.29419-8-kuniyu@amazon.com Signed-off-by: Jakub Kicinski --- drivers/net/can/vxcan.c | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/drivers/net/can/vxcan.c b/drivers/net/can/vxcan.c index 9e1b7d41005f..da7c72105fb6 100644 --- a/drivers/net/can/vxcan.c +++ b/drivers/net/can/vxcan.c @@ -188,14 +188,10 @@ static int vxcan_newlink(struct net *net, struct net_device *dev, /* register peer device */ if (data && data[VXCAN_INFO_PEER]) { - struct nlattr *nla_peer; + struct nlattr *nla_peer = data[VXCAN_INFO_PEER]; - nla_peer = data[VXCAN_INFO_PEER]; ifmp = nla_data(nla_peer); - err = rtnl_nla_parse_ifinfomsg(peer_tb, nla_peer, extack); - if (err < 0) - return err; - + rtnl_nla_parse_ifinfomsg(peer_tb, nla_peer, extack); tbp = peer_tb; } @@ -208,9 +204,6 @@ static int vxcan_newlink(struct net *net, struct net_device *dev, } peer_net = rtnl_link_get_net(net, tbp); - if (IS_ERR(peer_net)) - return PTR_ERR(peer_net); - peer = rtnl_create_link(peer_net, ifname, name_assign_type, &vxcan_link_ops, tbp, extack); if (IS_ERR(peer)) { @@ -302,6 +295,7 @@ static struct rtnl_link_ops vxcan_link_ops = { .newlink = vxcan_newlink, .dellink = vxcan_dellink, .policy = vxcan_policy, + .peer_type = VXCAN_INFO_PEER, .maxtype = VXCAN_INFO_MAX, .get_link_net = vxcan_get_link_net, }; -- 2.51.0