netdev_bind_rx takes ownership of the queue array passed as parameter and frees it, so a queue array buffer cannot be reused across multiple netdev_bind_rx calls.
This commit fixes that by always passing in a newly created queue array to all netdev_bind_rx calls in ncdevmem.
Fixes: 85585b4bc8d8 ("selftests: add ncdevmem, netcat for devmem TCP") Signed-off-by: Cosmin Ratiu cratiu@nvidia.com --- .../selftests/drivers/net/hw/ncdevmem.c | 55 ++++++++----------- 1 file changed, 22 insertions(+), 33 deletions(-)
diff --git a/tools/testing/selftests/drivers/net/hw/ncdevmem.c b/tools/testing/selftests/drivers/net/hw/ncdevmem.c index 2bf14ac2b8c6..9d48004ff1a1 100644 --- a/tools/testing/selftests/drivers/net/hw/ncdevmem.c +++ b/tools/testing/selftests/drivers/net/hw/ncdevmem.c @@ -431,6 +431,22 @@ static int parse_address(const char *str, int port, struct sockaddr_in6 *sin6) return 0; }
+static struct netdev_queue_id *create_queues(void) +{ + struct netdev_queue_id *queues; + size_t i = 0; + + queues = calloc(num_queues, sizeof(*queues)); + for (i = 0; i < num_queues; i++) { + queues[i]._present.type = 1; + queues[i]._present.id = 1; + queues[i].type = NETDEV_QUEUE_TYPE_RX; + queues[i].id = start_queue + i; + } + + return queues; +} + int do_server(struct memory_buffer *mem) { char ctrl_data[sizeof(int) * 20000]; @@ -448,7 +464,6 @@ int do_server(struct memory_buffer *mem) char buffer[256]; int socket_fd; int client_fd; - size_t i = 0; int ret;
ret = parse_address(server_ip, atoi(port), &server_sin); @@ -471,16 +486,7 @@ int do_server(struct memory_buffer *mem)
sleep(1);
- queues = malloc(sizeof(*queues) * num_queues); - - for (i = 0; i < num_queues; i++) { - queues[i]._present.type = 1; - queues[i]._present.id = 1; - queues[i].type = NETDEV_QUEUE_TYPE_RX; - queues[i].id = start_queue + i; - } - - if (bind_rx_queue(ifindex, mem->fd, queues, num_queues, &ys)) + if (bind_rx_queue(ifindex, mem->fd, create_queues(), num_queues, &ys)) error(1, 0, "Failed to bind\n");
tmp_mem = malloc(mem->size); @@ -545,7 +551,6 @@ int do_server(struct memory_buffer *mem) goto cleanup; }
- i++; for (cm = CMSG_FIRSTHDR(&msg); cm; cm = CMSG_NXTHDR(&msg, cm)) { if (cm->cmsg_level != SOL_SOCKET || (cm->cmsg_type != SCM_DEVMEM_DMABUF && @@ -630,10 +635,8 @@ int do_server(struct memory_buffer *mem)
void run_devmem_tests(void) { - struct netdev_queue_id *queues; struct memory_buffer *mem; struct ynl_sock *ys; - size_t i = 0;
mem = provider->alloc(getpagesize() * NUM_PAGES);
@@ -641,38 +644,24 @@ void run_devmem_tests(void) if (configure_rss()) error(1, 0, "rss error\n");
- queues = calloc(num_queues, sizeof(*queues)); - if (configure_headersplit(1)) error(1, 0, "Failed to configure header split\n");
- if (!bind_rx_queue(ifindex, mem->fd, queues, num_queues, &ys)) + if (!bind_rx_queue(ifindex, mem->fd, + calloc(num_queues, sizeof(struct netdev_queue_id)), + num_queues, &ys)) error(1, 0, "Binding empty queues array should have failed\n");
- for (i = 0; i < num_queues; i++) { - queues[i]._present.type = 1; - queues[i]._present.id = 1; - queues[i].type = NETDEV_QUEUE_TYPE_RX; - queues[i].id = start_queue + i; - } - if (configure_headersplit(0)) error(1, 0, "Failed to configure header split\n");
- if (!bind_rx_queue(ifindex, mem->fd, queues, num_queues, &ys)) + if (!bind_rx_queue(ifindex, mem->fd, create_queues(), num_queues, &ys)) error(1, 0, "Configure dmabuf with header split off should have failed\n");
if (configure_headersplit(1)) error(1, 0, "Failed to configure header split\n");
- for (i = 0; i < num_queues; i++) { - queues[i]._present.type = 1; - queues[i]._present.id = 1; - queues[i].type = NETDEV_QUEUE_TYPE_RX; - queues[i].id = start_queue + i; - } - - if (bind_rx_queue(ifindex, mem->fd, queues, num_queues, &ys)) + if (bind_rx_queue(ifindex, mem->fd, create_queues(), num_queues, &ys)) error(1, 0, "Failed to bind\n");
/* Deactivating a bound queue should not be legal */
On 05/08, Cosmin Ratiu wrote:
netdev_bind_rx takes ownership of the queue array passed as parameter and frees it, so a queue array buffer cannot be reused across multiple netdev_bind_rx calls.
This commit fixes that by always passing in a newly created queue array to all netdev_bind_rx calls in ncdevmem.
Fixes: 85585b4bc8d8 ("selftests: add ncdevmem, netcat for devmem TCP") Signed-off-by: Cosmin Ratiu cratiu@nvidia.com
Acked-by: Stanislav Fomichev sdf@fomichev.me
On Thu, May 08, 2025 at 11:44:34AM +0300, Cosmin Ratiu wrote:
netdev_bind_rx takes ownership of the queue array passed as parameter and frees it, so a queue array buffer cannot be reused across multiple netdev_bind_rx calls.
This commit fixes that by always passing in a newly created queue array to all netdev_bind_rx calls in ncdevmem.
Fixes: 85585b4bc8d8 ("selftests: add ncdevmem, netcat for devmem TCP") Signed-off-by: Cosmin Ratiu cratiu@nvidia.com
.../selftests/drivers/net/hw/ncdevmem.c | 55 ++++++++----------- 1 file changed, 22 insertions(+), 33 deletions(-)
diff --git a/tools/testing/selftests/drivers/net/hw/ncdevmem.c b/tools/testing/selftests/drivers/net/hw/ncdevmem.c index 2bf14ac2b8c6..9d48004ff1a1 100644 --- a/tools/testing/selftests/drivers/net/hw/ncdevmem.c +++ b/tools/testing/selftests/drivers/net/hw/ncdevmem.c @@ -431,6 +431,22 @@ static int parse_address(const char *str, int port, struct sockaddr_in6 *sin6)
- queues = calloc(num_queues, sizeof(*queues));
- queues = malloc(sizeof(*queues) * num_queues);
- if (!bind_rx_queue(ifindex, mem->fd,
calloc(num_queues, sizeof(struct netdev_queue_id)),
Nit: it looks like in the original we didn't care about malloc potentially failing. Do we care about checking for that now with this cleanup?
Otherwise:
Reviewed-by: Joe Damato jdamato@fastly.com
On Thu, 2025-05-08 at 11:31 -0700, Joe Damato wrote:
Nit: it looks like in the original we didn't care about malloc potentially failing. Do we care about checking for that now with this cleanup?
Thank you for the review, Joe.
I looked a bit into adding error messages and I think it wouldn't make sense just for these allocations, as there are others which do not check for malloc/calloc error (e.g. all generated ynl libs).
Furthermore, I am under the impression that on the kind of systems ncdevmem would run, malloc/calloc would almost never fail because of overcommit.
Finally, even if they did fail, the user program would just segfault (not very user friendly, but good enough).
Cosmin.
On Thu, May 8, 2025 at 1:45 AM Cosmin Ratiu cratiu@nvidia.com wrote:
netdev_bind_rx takes ownership of the queue array passed as parameter and frees it, so a queue array buffer cannot be reused across multiple netdev_bind_rx calls.
This commit fixes that by always passing in a newly created queue array to all netdev_bind_rx calls in ncdevmem.
Fixes: 85585b4bc8d8 ("selftests: add ncdevmem, netcat for devmem TCP") Signed-off-by: Cosmin Ratiu cratiu@nvidia.com
Thank you very much.
Reviewed-by: Mina Almasry almasrymina@google.com
Also, I think there was a discussion in v1 about increasing the amount of memory that ncdevmem uses by default (currently it's 64MB) as Stan suggested. I have it in my TODO list to implement that change but I don't think I'll get to it soon. If you (or anyone) gets to it before me, it's a welcome change. AFAIU it'll unblock you from running ncdevmem on your driver which expects much more dmabuf memory available.
But to be clear, that can be a follow up change. I think this is good as-is.
On Thu, 2025-05-08 at 13:42 -0700, Mina Almasry wrote:
Thank you very much.
Reviewed-by: Mina Almasry almasrymina@google.com
Thank you for the review.
Also, I think there was a discussion in v1 about increasing the amount of memory that ncdevmem uses by default (currently it's 64MB) as Stan suggested. I have it in my TODO list to implement that change but I don't think I'll get to it soon. If you (or anyone) gets to it before me, it's a welcome change. AFAIU it'll unblock you from running ncdevmem on your driver which expects much more dmabuf memory available.
But to be clear, that can be a follow up change. I think this is good as-is.
I do plan to follow-up on that topic, mlx5 will require some reengineering to not fail rx rings refill when the pool is too small, and most likely some touch-ups in ncdevmem/udmabuf to make this better.
Cosmin.
Hello:
This patch was applied to netdev/net.git (main) by Jakub Kicinski kuba@kernel.org:
On Thu, 8 May 2025 11:44:34 +0300 you wrote:
netdev_bind_rx takes ownership of the queue array passed as parameter and frees it, so a queue array buffer cannot be reused across multiple netdev_bind_rx calls.
This commit fixes that by always passing in a newly created queue array to all netdev_bind_rx calls in ncdevmem.
[...]
Here is the summary with links: - [net,v2] tests/ncdevmem: Fix double-free of queue array https://git.kernel.org/netdev/net/c/97c4e094a4b2
You are awesome, thank you!
linux-kselftest-mirror@lists.linaro.org
-
Cosmin Ratiu -
Joe Damato -
Mina Almasry -
patchwork-bot+netdevbpf＠kernel.org -
Stanislav Fomichev