A few unrelated devmem TCP fixes bundled in a series for some convenience (if that's ok).
Patch 1-2: fix naming and provide page_pool_alloc_netmem for fragged netmem.
Patch 3-4: fix issues with dma-buf dma addresses being potentially passed to dma_sync_for_* helpers.
Patch 5-6: fix syzbot SO_DEVMEM_DONTNEED issue and add test for this case.
Mina Almasry (6): net: page_pool: rename page_pool_alloc_netmem to *_netmems net: page_pool: create page_pool_alloc_netmem page_pool: disable sync for cpu for dmabuf memory provider netmem: add netmem_prefetch net: fix SO_DEVMEM_DONTNEED looping too long ncdevmem: add test for too many token_count
Samiullah Khawaja (1): page_pool: Set `dma_sync` to false for devmem memory provider
include/net/netmem.h | 7 ++++ include/net/page_pool/helpers.h | 50 ++++++++++++++++++-------- include/net/page_pool/types.h | 2 +- net/core/devmem.c | 9 +++-- net/core/page_pool.c | 11 +++--- net/core/sock.c | 46 ++++++++++++++---------- tools/testing/selftests/net/ncdevmem.c | 11 ++++++ 7 files changed, 93 insertions(+), 43 deletions(-)
Check we're going to free a reasonable number of frags in token_count before starting the loop, to prevent looping too long.
Also minor code cleanups: - Flip checks to reduce indentation. - Use sizeof(*tokens) everywhere for consistentcy.
Cc: Yi Lai yi1.lai@linux.intel.com
Signed-off-by: Mina Almasry almasrymina@google.com
--- net/core/sock.c | 46 ++++++++++++++++++++++++++++------------------ 1 file changed, 28 insertions(+), 18 deletions(-)
diff --git a/net/core/sock.c b/net/core/sock.c index 7f398bd07fb7..8603b8d87f2e 100644 --- a/net/core/sock.c +++ b/net/core/sock.c @@ -1047,11 +1047,12 @@ static int sock_reserve_memory(struct sock *sk, int bytes)
#ifdef CONFIG_PAGE_POOL
-/* This is the number of tokens that the user can SO_DEVMEM_DONTNEED in +/* This is the number of frags that the user can SO_DEVMEM_DONTNEED in * 1 syscall. The limit exists to limit the amount of memory the kernel - * allocates to copy these tokens. + * allocates to copy these tokens, and to prevent looping over the frags for + * too long. */ -#define MAX_DONTNEED_TOKENS 128 +#define MAX_DONTNEED_FRAGS 1024
static noinline_for_stack int sock_devmem_dontneed(struct sock *sk, sockptr_t optval, unsigned int optlen) @@ -1059,43 +1060,52 @@ sock_devmem_dontneed(struct sock *sk, sockptr_t optval, unsigned int optlen) unsigned int num_tokens, i, j, k, netmem_num = 0; struct dmabuf_token *tokens; netmem_ref netmems[16]; + u64 num_frags = 0; int ret = 0;
if (!sk_is_tcp(sk)) return -EBADF;
- if (optlen % sizeof(struct dmabuf_token) || - optlen > sizeof(*tokens) * MAX_DONTNEED_TOKENS) + if (optlen % sizeof(*tokens) || + optlen > sizeof(*tokens) * MAX_DONTNEED_FRAGS) return -EINVAL;
- tokens = kvmalloc_array(optlen, sizeof(*tokens), GFP_KERNEL); + num_tokens = optlen / sizeof(*tokens); + tokens = kvmalloc_array(num_tokens, sizeof(*tokens), GFP_KERNEL); if (!tokens) return -ENOMEM;
- num_tokens = optlen / sizeof(struct dmabuf_token); if (copy_from_sockptr(tokens, optval, optlen)) { kvfree(tokens); return -EFAULT; }
+ for (i = 0; i < num_tokens; i++) { + num_frags += tokens[i].token_count; + if (num_frags > MAX_DONTNEED_FRAGS) { + kvfree(tokens); + return -E2BIG; + } + } + xa_lock_bh(&sk->sk_user_frags); for (i = 0; i < num_tokens; i++) { for (j = 0; j < tokens[i].token_count; j++) { netmem_ref netmem = (__force netmem_ref)__xa_erase( &sk->sk_user_frags, tokens[i].token_start + j);
- if (netmem && - !WARN_ON_ONCE(!netmem_is_net_iov(netmem))) { - netmems[netmem_num++] = netmem; - if (netmem_num == ARRAY_SIZE(netmems)) { - xa_unlock_bh(&sk->sk_user_frags); - for (k = 0; k < netmem_num; k++) - WARN_ON_ONCE(!napi_pp_put_page(netmems[k])); - netmem_num = 0; - xa_lock_bh(&sk->sk_user_frags); - } - ret++; + if (!netmem || WARN_ON_ONCE(!netmem_is_net_iov(netmem))) + continue; + + netmems[netmem_num++] = netmem; + if (netmem_num == ARRAY_SIZE(netmems)) { + xa_unlock_bh(&sk->sk_user_frags); + for (k = 0; k < netmem_num; k++) + WARN_ON_ONCE(!napi_pp_put_page(netmems[k])); + netmem_num = 0; + xa_lock_bh(&sk->sk_user_frags); } + ret++; } }
-- 2.47.0.163.g1226f6d8fa-goog
On 10/29, Mina Almasry wrote:
Check we're going to free a reasonable number of frags in token_count before starting the loop, to prevent looping too long.
Also minor code cleanups:
- Flip checks to reduce indentation.
- Use sizeof(*tokens) everywhere for consistentcy.
Cc: Yi Lai yi1.lai@linux.intel.com
Signed-off-by: Mina Almasry almasrymina@google.com
net/core/sock.c | 46 ++++++++++++++++++++++++++++------------------ 1 file changed, 28 insertions(+), 18 deletions(-)
diff --git a/net/core/sock.c b/net/core/sock.c index 7f398bd07fb7..8603b8d87f2e 100644 --- a/net/core/sock.c +++ b/net/core/sock.c @@ -1047,11 +1047,12 @@ static int sock_reserve_memory(struct sock *sk, int bytes)
#ifdef CONFIG_PAGE_POOL
-/* This is the number of tokens that the user can SO_DEVMEM_DONTNEED in +/* This is the number of frags that the user can SO_DEVMEM_DONTNEED in
- 1 syscall. The limit exists to limit the amount of memory the kernel
- allocates to copy these tokens.
- allocates to copy these tokens, and to prevent looping over the frags for
*/
- too long.
-#define MAX_DONTNEED_TOKENS 128 +#define MAX_DONTNEED_FRAGS 1024
static noinline_for_stack int sock_devmem_dontneed(struct sock *sk, sockptr_t optval, unsigned int optlen) @@ -1059,43 +1060,52 @@ sock_devmem_dontneed(struct sock *sk, sockptr_t optval, unsigned int optlen) unsigned int num_tokens, i, j, k, netmem_num = 0; struct dmabuf_token *tokens; netmem_ref netmems[16];
u64 num_frags = 0; int ret = 0;
if (!sk_is_tcp(sk)) return -EBADF;
- if (optlen % sizeof(struct dmabuf_token) ||
optlen > sizeof(*tokens) * MAX_DONTNEED_TOKENS)
- if (optlen % sizeof(*tokens) ||
optlen > sizeof(*tokens) * MAX_DONTNEED_FRAGS)
- tokens = kvmalloc_array(optlen, sizeof(*tokens), GFP_KERNEL);
- num_tokens = optlen / sizeof(*tokens);
- tokens = kvmalloc_array(num_tokens, sizeof(*tokens), GFP_KERNEL); if (!tokens) return -ENOMEM;
- num_tokens = optlen / sizeof(struct dmabuf_token); if (copy_from_sockptr(tokens, optval, optlen)) { kvfree(tokens); return -EFAULT; }
- for (i = 0; i < num_tokens; i++) {
num_frags += tokens[i].token_count;if (num_frags > MAX_DONTNEED_FRAGS) {kvfree(tokens);return -E2BIG;}- }
- xa_lock_bh(&sk->sk_user_frags); for (i = 0; i < num_tokens; i++) { for (j = 0; j < tokens[i].token_count; j++) { netmem_ref netmem = (__force netmem_ref)__xa_erase( &sk->sk_user_frags, tokens[i].token_start + j);
if (netmem &&!WARN_ON_ONCE(!netmem_is_net_iov(netmem))) {netmems[netmem_num++] = netmem;if (netmem_num == ARRAY_SIZE(netmems)) {xa_unlock_bh(&sk->sk_user_frags);for (k = 0; k < netmem_num; k++)WARN_ON_ONCE(!napi_pp_put_page(netmems[k]));netmem_num = 0;xa_lock_bh(&sk->sk_user_frags);}ret++;
[..]
if (!netmem || WARN_ON_ONCE(!netmem_is_net_iov(netmem)))continue;
Any reason we are not returning explicit error to the callers here? That probably needs some mechanism to signal which particular one failed so the users can restart?
On Wed, Oct 30, 2024 at 7:33 AM Stanislav Fomichev stfomichev@gmail.com wrote:
On 10/29, Mina Almasry wrote:
Check we're going to free a reasonable number of frags in token_count before starting the loop, to prevent looping too long.
Also minor code cleanups:
- Flip checks to reduce indentation.
- Use sizeof(*tokens) everywhere for consistentcy.
Cc: Yi Lai yi1.lai@linux.intel.com
Signed-off-by: Mina Almasry almasrymina@google.com
net/core/sock.c | 46 ++++++++++++++++++++++++++++------------------ 1 file changed, 28 insertions(+), 18 deletions(-)
diff --git a/net/core/sock.c b/net/core/sock.c index 7f398bd07fb7..8603b8d87f2e 100644 --- a/net/core/sock.c +++ b/net/core/sock.c @@ -1047,11 +1047,12 @@ static int sock_reserve_memory(struct sock *sk, int bytes)
#ifdef CONFIG_PAGE_POOL
-/* This is the number of tokens that the user can SO_DEVMEM_DONTNEED in +/* This is the number of frags that the user can SO_DEVMEM_DONTNEED in
- 1 syscall. The limit exists to limit the amount of memory the kernel
- allocates to copy these tokens.
- allocates to copy these tokens, and to prevent looping over the frags for
*/
- too long.
-#define MAX_DONTNEED_TOKENS 128 +#define MAX_DONTNEED_FRAGS 1024
static noinline_for_stack int sock_devmem_dontneed(struct sock *sk, sockptr_t optval, unsigned int optlen) @@ -1059,43 +1060,52 @@ sock_devmem_dontneed(struct sock *sk, sockptr_t optval, unsigned int optlen) unsigned int num_tokens, i, j, k, netmem_num = 0; struct dmabuf_token *tokens; netmem_ref netmems[16];
u64 num_frags = 0; int ret = 0; if (!sk_is_tcp(sk)) return -EBADF;
if (optlen % sizeof(struct dmabuf_token) ||optlen > sizeof(*tokens) * MAX_DONTNEED_TOKENS)
if (optlen % sizeof(*tokens) ||optlen > sizeof(*tokens) * MAX_DONTNEED_FRAGS) return -EINVAL;
tokens = kvmalloc_array(optlen, sizeof(*tokens), GFP_KERNEL);
num_tokens = optlen / sizeof(*tokens);tokens = kvmalloc_array(num_tokens, sizeof(*tokens), GFP_KERNEL); if (!tokens) return -ENOMEM;
num_tokens = optlen / sizeof(struct dmabuf_token); if (copy_from_sockptr(tokens, optval, optlen)) { kvfree(tokens); return -EFAULT; }
for (i = 0; i < num_tokens; i++) {num_frags += tokens[i].token_count;if (num_frags > MAX_DONTNEED_FRAGS) {kvfree(tokens);return -E2BIG;}}xa_lock_bh(&sk->sk_user_frags); for (i = 0; i < num_tokens; i++) { for (j = 0; j < tokens[i].token_count; j++) { netmem_ref netmem = (__force netmem_ref)__xa_erase( &sk->sk_user_frags, tokens[i].token_start + j);
if (netmem &&!WARN_ON_ONCE(!netmem_is_net_iov(netmem))) {netmems[netmem_num++] = netmem;if (netmem_num == ARRAY_SIZE(netmems)) {xa_unlock_bh(&sk->sk_user_frags);for (k = 0; k < netmem_num; k++)WARN_ON_ONCE(!napi_pp_put_page(netmems[k]));netmem_num = 0;xa_lock_bh(&sk->sk_user_frags);}ret++;[..]
if (!netmem || WARN_ON_ONCE(!netmem_is_net_iov(netmem)))continue;Any reason we are not returning explicit error to the callers here? That probably needs some mechanism to signal which particular one failed so the users can restart?
Only because I can't think of a simple way to return an array of frags failed to DONTNEED to the user.
Also, this error should be extremely rare or never hit really. I don't know how we end up not finding a netmem here or the netmem is page. The only way is if the user is malicious (messing with the token ids passed to the kernel) or if a kernel bug is happening.
Also, the information is useless to the user. If the user sees 'frag 128 failed to free'. There is nothing really the user can do to recover at runtime. Only usefulness that could come is for the user to log the error. We already WARN_ON_ONCE on the error the user would not be able to trigger.
On 10/30, Mina Almasry wrote:
On Wed, Oct 30, 2024 at 7:33 AM Stanislav Fomichev stfomichev@gmail.com wrote:
On 10/29, Mina Almasry wrote:
Check we're going to free a reasonable number of frags in token_count before starting the loop, to prevent looping too long.
Also minor code cleanups:
- Flip checks to reduce indentation.
- Use sizeof(*tokens) everywhere for consistentcy.
Cc: Yi Lai yi1.lai@linux.intel.com
Signed-off-by: Mina Almasry almasrymina@google.com
net/core/sock.c | 46 ++++++++++++++++++++++++++++------------------ 1 file changed, 28 insertions(+), 18 deletions(-)
diff --git a/net/core/sock.c b/net/core/sock.c index 7f398bd07fb7..8603b8d87f2e 100644 --- a/net/core/sock.c +++ b/net/core/sock.c @@ -1047,11 +1047,12 @@ static int sock_reserve_memory(struct sock *sk, int bytes)
#ifdef CONFIG_PAGE_POOL
-/* This is the number of tokens that the user can SO_DEVMEM_DONTNEED in +/* This is the number of frags that the user can SO_DEVMEM_DONTNEED in
- 1 syscall. The limit exists to limit the amount of memory the kernel
- allocates to copy these tokens.
- allocates to copy these tokens, and to prevent looping over the frags for
*/
- too long.
-#define MAX_DONTNEED_TOKENS 128 +#define MAX_DONTNEED_FRAGS 1024
static noinline_for_stack int sock_devmem_dontneed(struct sock *sk, sockptr_t optval, unsigned int optlen) @@ -1059,43 +1060,52 @@ sock_devmem_dontneed(struct sock *sk, sockptr_t optval, unsigned int optlen) unsigned int num_tokens, i, j, k, netmem_num = 0; struct dmabuf_token *tokens; netmem_ref netmems[16];
u64 num_frags = 0; int ret = 0; if (!sk_is_tcp(sk)) return -EBADF;
if (optlen % sizeof(struct dmabuf_token) ||optlen > sizeof(*tokens) * MAX_DONTNEED_TOKENS)
if (optlen % sizeof(*tokens) ||optlen > sizeof(*tokens) * MAX_DONTNEED_FRAGS) return -EINVAL;
tokens = kvmalloc_array(optlen, sizeof(*tokens), GFP_KERNEL);
num_tokens = optlen / sizeof(*tokens);tokens = kvmalloc_array(num_tokens, sizeof(*tokens), GFP_KERNEL); if (!tokens) return -ENOMEM;
num_tokens = optlen / sizeof(struct dmabuf_token); if (copy_from_sockptr(tokens, optval, optlen)) { kvfree(tokens); return -EFAULT; }
for (i = 0; i < num_tokens; i++) {num_frags += tokens[i].token_count;if (num_frags > MAX_DONTNEED_FRAGS) {kvfree(tokens);return -E2BIG;}}xa_lock_bh(&sk->sk_user_frags); for (i = 0; i < num_tokens; i++) { for (j = 0; j < tokens[i].token_count; j++) { netmem_ref netmem = (__force netmem_ref)__xa_erase( &sk->sk_user_frags, tokens[i].token_start + j);
if (netmem &&!WARN_ON_ONCE(!netmem_is_net_iov(netmem))) {netmems[netmem_num++] = netmem;if (netmem_num == ARRAY_SIZE(netmems)) {xa_unlock_bh(&sk->sk_user_frags);for (k = 0; k < netmem_num; k++)WARN_ON_ONCE(!napi_pp_put_page(netmems[k]));netmem_num = 0;xa_lock_bh(&sk->sk_user_frags);}ret++;[..]
if (!netmem || WARN_ON_ONCE(!netmem_is_net_iov(netmem)))continue;Any reason we are not returning explicit error to the callers here? That probably needs some mechanism to signal which particular one failed so the users can restart?
Only because I can't think of a simple way to return an array of frags failed to DONTNEED to the user.
I'd expect the call to return as soon as it hits the invalid frag entry (plus the number of entries that it successfully refilled up to the invalid one). But too late I guess.
Also, this error should be extremely rare or never hit really. I don't know how we end up not finding a netmem here or the netmem is page. The only way is if the user is malicious (messing with the token ids passed to the kernel) or if a kernel bug is happening.
I do hit this error with 1500 mtu, so it would've been nice to understand which particular token triggered that. It might be something buggy on the driver side, I need to investigate. (it's super low prio because 1500)
Also, the information is useless to the user. If the user sees 'frag 128 failed to free'. There is nothing really the user can do to recover at runtime. Only usefulness that could come is for the user to log the error. We already WARN_ON_ONCE on the error the user would not be able to trigger.
I'm thinking from the pow of user application. It might have bugs as well and try to refill something that should not have been refilled. Having info about which particular token has failed (even just for the logging purposes) might have been nice.
Add test for fixed issue: user passing a token with a very large token_count. Expect an error in this case.
Signed-off-by: Mina Almasry almasrymina@google.com --- tools/testing/selftests/net/ncdevmem.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
diff --git a/tools/testing/selftests/net/ncdevmem.c b/tools/testing/selftests/net/ncdevmem.c index 64d6805381c5..3fd2aee461f3 100644 --- a/tools/testing/selftests/net/ncdevmem.c +++ b/tools/testing/selftests/net/ncdevmem.c @@ -391,6 +391,17 @@ int do_server(void) continue; }
+ token.token_start = dmabuf_cmsg->frag_token; + token.token_count = 8192; + + ret = setsockopt(client_fd, SOL_SOCKET, + SO_DEVMEM_DONTNEED, &token, + sizeof(token)); + if (ret >= 0) + error(1, 0, + "DONTNEED of too many frags should have failed. ret=%ld\n", + ret); + token.token_start = dmabuf_cmsg->frag_token; token.token_count = 1;
On Tue, 29 Oct 2024 20:55:20 +0000 Mina Almasry wrote:
A few unrelated devmem TCP fixes bundled in a series for some convenience (if that's ok).
These two should go to net I presume? It's missing input validation.
Either way you gotta repost either as two properly separate series, or combine them as one, cause right now they are neither and patchwork doesn't recognize they are related.
linux-kselftest-mirror@lists.linaro.org
-
Jakub Kicinski -
Mina Almasry -
Stanislav Fomichev