* [PATCH bpf v2 0/4] bpf, sockmap: Fix infinite recursion in sock_map_close
@ 2023-01-21 12:41 Jakub Sitnicki
2023-01-21 12:41 ` [PATCH bpf v2 1/4] bpf, sockmap: Don't let sock_map_{close,destroy,unhash} call itself Jakub Sitnicki
` (4 more replies)
0 siblings, 5 replies; 10+ messages in thread
From: Jakub Sitnicki @ 2023-01-21 12:41 UTC (permalink / raw)
To: bpf
Cc: netdev, John Fastabend, Eric Dumazet, Daniel Borkmann,
Alexei Starovoitov, Andrii Nakryiko, kernel-team,
syzbot+04c21ed96d861dccc5cd
This patch set addresses the syzbot report in [1].
Patch #1 has been suggested by Eric [2]. I extended it to cover the rest of
sock_map proto callbacks. Otherwise we would still overflow the stack.
Patch #2 contains the actual fix and bug analysis.
Patches #3 & #4 add coverage to selftests to trigger the bug.
[1] https://lore.kernel.org/all/00000000000073b14905ef2e7401@google.com/
[2] https://lore.kernel.org/all/CANn89iK2UN1FmdUcH12fv_xiZkv2G+Nskvmq7fG6aA_6VKRf6g@mail.gmail.com/
---
v1 -> v2:
v1: https://lore.kernel.org/r/20230113-sockmap-fix-v1-0-d3cad092ee10@cloudflare.com
[v1 didn't hit bpf@ ML by mistake]
* pull in Eric's patch to protect against recursion loop bugs (Eric)
* add a macro helper to check if pointer is inside a memory range (Eric)
---
Jakub Sitnicki (4):
bpf, sockmap: Don't let sock_map_{close,destroy,unhash} call itself
bpf, sockmap: Check for any of tcp_bpf_prots when cloning a listener
selftests/bpf: Pass BPF skeleton to sockmap_listen ops tests
selftests/bpf: Cover listener cloning with progs attached to sockmap
include/linux/util_macros.h | 12 ++++
net/core/sock_map.c | 61 ++++++++--------
net/ipv4/tcp_bpf.c | 4 +-
.../selftests/bpf/prog_tests/sockmap_listen.c | 81 +++++++++++++++++-----
4 files changed, 111 insertions(+), 47 deletions(-)
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH bpf v2 1/4] bpf, sockmap: Don't let sock_map_{close,destroy,unhash} call itself 2023-01-21 12:41 [PATCH bpf v2 0/4] bpf, sockmap: Fix infinite recursion in sock_map_close Jakub Sitnicki @ 2023-01-21 12:41 ` Jakub Sitnicki 2023-01-25 5:20 ` John Fastabend 2023-01-21 12:41 ` [PATCH bpf v2 2/4] bpf, sockmap: Check for any of tcp_bpf_prots when cloning a listener Jakub Sitnicki ` (3 subsequent siblings) 4 siblings, 1 reply; 10+ messages in thread From: Jakub Sitnicki @ 2023-01-21 12:41 UTC (permalink / raw) To: bpf Cc: netdev, John Fastabend, Eric Dumazet, Daniel Borkmann, Alexei Starovoitov, Andrii Nakryiko, kernel-team sock_map proto callbacks should never call themselves by design. Protect against bugs like [1] and break out of the recursive loop to avoid a stack overflow in favor of a resource leak. [1] https://lore.kernel.org/all/00000000000073b14905ef2e7401@google.com/ Suggested-by: Eric Dumazet <edumazet@google.com> Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com> --- net/core/sock_map.c | 61 +++++++++++++++++++++++++++++------------------------ 1 file changed, 34 insertions(+), 27 deletions(-) diff --git a/net/core/sock_map.c b/net/core/sock_map.c index 22fa2c5bc6ec..a68a7290a3b2 100644 --- a/net/core/sock_map.c +++ b/net/core/sock_map.c @@ -1569,15 +1569,16 @@ void sock_map_unhash(struct sock *sk) psock = sk_psock(sk); if (unlikely(!psock)) { rcu_read_unlock(); - if (sk->sk_prot->unhash) - sk->sk_prot->unhash(sk); - return; + saved_unhash = READ_ONCE(sk->sk_prot)->unhash; + } else { + saved_unhash = psock->saved_unhash; + sock_map_remove_links(sk, psock); + rcu_read_unlock(); } - - saved_unhash = psock->saved_unhash; - sock_map_remove_links(sk, psock); - rcu_read_unlock(); - saved_unhash(sk); + if (WARN_ON_ONCE(saved_unhash == sock_map_unhash)) + return; + if (saved_unhash) + saved_unhash(sk); } EXPORT_SYMBOL_GPL(sock_map_unhash); @@ -1590,17 +1591,18 @@ void sock_map_destroy(struct sock *sk) psock = sk_psock_get(sk); if (unlikely(!psock)) { rcu_read_unlock(); - if (sk->sk_prot->destroy) - sk->sk_prot->destroy(sk); - return; + saved_destroy = READ_ONCE(sk->sk_prot)->destroy; + } else { + saved_destroy = psock->saved_destroy; + sock_map_remove_links(sk, psock); + rcu_read_unlock(); + sk_psock_stop(psock); + sk_psock_put(sk, psock); } - - saved_destroy = psock->saved_destroy; - sock_map_remove_links(sk, psock); - rcu_read_unlock(); - sk_psock_stop(psock); - sk_psock_put(sk, psock); - saved_destroy(sk); + if (WARN_ON_ONCE(saved_destroy == sock_map_destroy)) + return; + if (saved_destroy) + saved_destroy(sk); } EXPORT_SYMBOL_GPL(sock_map_destroy); @@ -1615,16 +1617,21 @@ void sock_map_close(struct sock *sk, long timeout) if (unlikely(!psock)) { rcu_read_unlock(); release_sock(sk); - return sk->sk_prot->close(sk, timeout); + saved_close = READ_ONCE(sk->sk_prot)->close; + } else { + saved_close = psock->saved_close; + sock_map_remove_links(sk, psock); + rcu_read_unlock(); + sk_psock_stop(psock); + release_sock(sk); + cancel_work_sync(&psock->work); + sk_psock_put(sk, psock); } - - saved_close = psock->saved_close; - sock_map_remove_links(sk, psock); - rcu_read_unlock(); - sk_psock_stop(psock); - release_sock(sk); - cancel_work_sync(&psock->work); - sk_psock_put(sk, psock); + /* Make sure we do not recurse. This is a bug. + * Leak the socket instead of crashing on a stack overflow. + */ + if (WARN_ON_ONCE(saved_close == sock_map_close)) + return; saved_close(sk, timeout); } EXPORT_SYMBOL_GPL(sock_map_close); -- 2.39.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* RE: [PATCH bpf v2 1/4] bpf, sockmap: Don't let sock_map_{close,destroy,unhash} call itself 2023-01-21 12:41 ` [PATCH bpf v2 1/4] bpf, sockmap: Don't let sock_map_{close,destroy,unhash} call itself Jakub Sitnicki @ 2023-01-25 5:20 ` John Fastabend 0 siblings, 0 replies; 10+ messages in thread From: John Fastabend @ 2023-01-25 5:20 UTC (permalink / raw) To: Jakub Sitnicki, bpf Cc: netdev, John Fastabend, Eric Dumazet, Daniel Borkmann, Alexei Starovoitov, Andrii Nakryiko, kernel-team Jakub Sitnicki wrote: > sock_map proto callbacks should never call themselves by design. Protect > against bugs like [1] and break out of the recursive loop to avoid a stack > overflow in favor of a resource leak. > > [1] https://lore.kernel.org/all/00000000000073b14905ef2e7401@google.com/ > > Suggested-by: Eric Dumazet <edumazet@google.com> > Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com> > --- LGTM and warn once logic should be fine IMO. Acked-by: John Fastabend <john.fastabend@gmail.com> ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH bpf v2 2/4] bpf, sockmap: Check for any of tcp_bpf_prots when cloning a listener 2023-01-21 12:41 [PATCH bpf v2 0/4] bpf, sockmap: Fix infinite recursion in sock_map_close Jakub Sitnicki 2023-01-21 12:41 ` [PATCH bpf v2 1/4] bpf, sockmap: Don't let sock_map_{close,destroy,unhash} call itself Jakub Sitnicki @ 2023-01-21 12:41 ` Jakub Sitnicki 2023-01-25 5:25 ` John Fastabend 2023-01-21 12:41 ` [PATCH bpf v2 3/4] selftests/bpf: Pass BPF skeleton to sockmap_listen ops tests Jakub Sitnicki ` (2 subsequent siblings) 4 siblings, 1 reply; 10+ messages in thread From: Jakub Sitnicki @ 2023-01-21 12:41 UTC (permalink / raw) To: bpf Cc: netdev, John Fastabend, Eric Dumazet, Daniel Borkmann, Alexei Starovoitov, Andrii Nakryiko, kernel-team, syzbot+04c21ed96d861dccc5cd A listening socket linked to a sockmap has its sk_prot overridden. It points to one of the struct proto variants in tcp_bpf_prots. The variant depends on the socket's family and which sockmap programs are attached. A child socket cloned from a TCP listener initially inherits their sk_prot. But before cloning is finished, we restore the child's proto to the listener's original non-tcp_bpf_prots one. This happens in tcp_create_openreq_child -> tcp_bpf_clone. Today, in tcp_bpf_clone we detect if the child's proto should be restored by checking only for the TCP_BPF_BASE proto variant. This is not correct. The sk_prot of listening socket linked to a sockmap can point to to any variant in tcp_bpf_prots. If the listeners sk_prot happens to be not the TCP_BPF_BASE variant, then the child socket unintentionally is left if the inherited sk_prot by tcp_bpf_clone. This leads to issues like infinite recursion on close [1], because the child state is otherwise not set up for use with tcp_bpf_prot operations. Adjust the check in tcp_bpf_clone to detect all of tcp_bpf_prots variants. Note that it wouldn't be sufficient to check the socket state when overriding the sk_prot in tcp_bpf_update_proto in order to always use the TCP_BPF_BASE variant for listening sockets. Since commit b8b8315e39ff ("bpf, sockmap: Remove unhash handler for BPF sockmap usage") it is possible for a socket to transition to TCP_LISTEN state while already linked to a sockmap, e.g. connect() -> insert into map -> connect(AF_UNSPEC) -> listen(). [1]: https://lore.kernel.org/all/00000000000073b14905ef2e7401@google.com/ Fixes: e80251555f0b ("tcp_bpf: Don't let child socket inherit parent protocol ops on copy") Reported-by: syzbot+04c21ed96d861dccc5cd@syzkaller.appspotmail.com Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com> --- include/linux/util_macros.h | 12 ++++++++++++ net/ipv4/tcp_bpf.c | 4 ++-- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/include/linux/util_macros.h b/include/linux/util_macros.h index 72299f261b25..43db6e47503c 100644 --- a/include/linux/util_macros.h +++ b/include/linux/util_macros.h @@ -38,4 +38,16 @@ */ #define find_closest_descending(x, a, as) __find_closest(x, a, as, >=) +/** + * is_insidevar - check if the @ptr points inside the @var memory range. + * @ptr: the pointer to a memory address. + * @var: the variable which address and size identify the memory range. + * + * Evaluates to true if the address in @ptr lies within the memory + * range allocated to @var. + */ +#define is_insidevar(ptr, var) \ + ((uintptr_t)(ptr) >= (uintptr_t)(var) && \ + (uintptr_t)(ptr) < (uintptr_t)(var) + sizeof(var)) + #endif diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c index 94aad3870c5f..cf26d65ca389 100644 --- a/net/ipv4/tcp_bpf.c +++ b/net/ipv4/tcp_bpf.c @@ -6,6 +6,7 @@ #include <linux/bpf.h> #include <linux/init.h> #include <linux/wait.h> +#include <linux/util_macros.h> #include <net/inet_common.h> #include <net/tls.h> @@ -639,10 +640,9 @@ EXPORT_SYMBOL_GPL(tcp_bpf_update_proto); */ void tcp_bpf_clone(const struct sock *sk, struct sock *newsk) { - int family = sk->sk_family == AF_INET6 ? TCP_BPF_IPV6 : TCP_BPF_IPV4; struct proto *prot = newsk->sk_prot; - if (prot == &tcp_bpf_prots[family][TCP_BPF_BASE]) + if (is_insidevar(prot, tcp_bpf_prots)) newsk->sk_prot = sk->sk_prot_creator; } #endif /* CONFIG_BPF_SYSCALL */ -- 2.39.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* RE: [PATCH bpf v2 2/4] bpf, sockmap: Check for any of tcp_bpf_prots when cloning a listener 2023-01-21 12:41 ` [PATCH bpf v2 2/4] bpf, sockmap: Check for any of tcp_bpf_prots when cloning a listener Jakub Sitnicki @ 2023-01-25 5:25 ` John Fastabend 0 siblings, 0 replies; 10+ messages in thread From: John Fastabend @ 2023-01-25 5:25 UTC (permalink / raw) To: Jakub Sitnicki, bpf Cc: netdev, John Fastabend, Eric Dumazet, Daniel Borkmann, Alexei Starovoitov, Andrii Nakryiko, kernel-team, syzbot+04c21ed96d861dccc5cd Jakub Sitnicki wrote: > A listening socket linked to a sockmap has its sk_prot overridden. It > points to one of the struct proto variants in tcp_bpf_prots. The variant > depends on the socket's family and which sockmap programs are attached. > > A child socket cloned from a TCP listener initially inherits their sk_prot. > But before cloning is finished, we restore the child's proto to the > listener's original non-tcp_bpf_prots one. This happens in > tcp_create_openreq_child -> tcp_bpf_clone. > > Today, in tcp_bpf_clone we detect if the child's proto should be restored > by checking only for the TCP_BPF_BASE proto variant. This is not > correct. The sk_prot of listening socket linked to a sockmap can point to > to any variant in tcp_bpf_prots. > > If the listeners sk_prot happens to be not the TCP_BPF_BASE variant, then > the child socket unintentionally is left if the inherited sk_prot by > tcp_bpf_clone. > > This leads to issues like infinite recursion on close [1], because the > child state is otherwise not set up for use with tcp_bpf_prot operations. > > Adjust the check in tcp_bpf_clone to detect all of tcp_bpf_prots variants. > > Note that it wouldn't be sufficient to check the socket state when > overriding the sk_prot in tcp_bpf_update_proto in order to always use the > TCP_BPF_BASE variant for listening sockets. Since commit > b8b8315e39ff ("bpf, sockmap: Remove unhash handler for BPF sockmap usage") > it is possible for a socket to transition to TCP_LISTEN state while already > linked to a sockmap, e.g. connect() -> insert into map -> > connect(AF_UNSPEC) -> listen(). > > [1]: https://lore.kernel.org/all/00000000000073b14905ef2e7401@google.com/ > > Fixes: e80251555f0b ("tcp_bpf: Don't let child socket inherit parent protocol ops on copy") > Reported-by: syzbot+04c21ed96d861dccc5cd@syzkaller.appspotmail.com > Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com> > --- Acked-by: John Fastabend <john.fastabend@gmail.com> ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH bpf v2 3/4] selftests/bpf: Pass BPF skeleton to sockmap_listen ops tests 2023-01-21 12:41 [PATCH bpf v2 0/4] bpf, sockmap: Fix infinite recursion in sock_map_close Jakub Sitnicki 2023-01-21 12:41 ` [PATCH bpf v2 1/4] bpf, sockmap: Don't let sock_map_{close,destroy,unhash} call itself Jakub Sitnicki 2023-01-21 12:41 ` [PATCH bpf v2 2/4] bpf, sockmap: Check for any of tcp_bpf_prots when cloning a listener Jakub Sitnicki @ 2023-01-21 12:41 ` Jakub Sitnicki 2023-01-25 5:27 ` John Fastabend 2023-01-21 12:41 ` [PATCH bpf v2 4/4] selftests/bpf: Cover listener cloning with progs attached to sockmap Jakub Sitnicki 2023-01-25 6:00 ` [PATCH bpf v2 0/4] bpf, sockmap: Fix infinite recursion in sock_map_close patchwork-bot+netdevbpf 4 siblings, 1 reply; 10+ messages in thread From: Jakub Sitnicki @ 2023-01-21 12:41 UTC (permalink / raw) To: bpf Cc: netdev, John Fastabend, Eric Dumazet, Daniel Borkmann, Alexei Starovoitov, Andrii Nakryiko, kernel-team Following patch extends the sockmap ops tests to cover the scenario when a sockmap with attached programs holds listening sockets. Pass the BPF skeleton to sockmap ops test so that the can access and attach the BPF programs. Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com> --- .../selftests/bpf/prog_tests/sockmap_listen.c | 55 +++++++++++++++------- 1 file changed, 37 insertions(+), 18 deletions(-) diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c index 2cf0c7a3fe23..499fba8f55b9 100644 --- a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c +++ b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c @@ -30,6 +30,8 @@ #define MAX_STRERR_LEN 256 #define MAX_TEST_NAME 80 +#define __always_unused __attribute__((__unused__)) + #define _FAIL(errnum, fmt...) \ ({ \ error_at_line(0, (errnum), __func__, __LINE__, fmt); \ @@ -321,7 +323,8 @@ static int socket_loopback(int family, int sotype) return socket_loopback_reuseport(family, sotype, -1); } -static void test_insert_invalid(int family, int sotype, int mapfd) +static void test_insert_invalid(struct test_sockmap_listen *skel __always_unused, + int family, int sotype, int mapfd) { u32 key = 0; u64 value; @@ -338,7 +341,8 @@ static void test_insert_invalid(int family, int sotype, int mapfd) FAIL_ERRNO("map_update: expected EBADF"); } -static void test_insert_opened(int family, int sotype, int mapfd) +static void test_insert_opened(struct test_sockmap_listen *skel __always_unused, + int family, int sotype, int mapfd) { u32 key = 0; u64 value; @@ -359,7 +363,8 @@ static void test_insert_opened(int family, int sotype, int mapfd) xclose(s); } -static void test_insert_bound(int family, int sotype, int mapfd) +static void test_insert_bound(struct test_sockmap_listen *skel __always_unused, + int family, int sotype, int mapfd) { struct sockaddr_storage addr; socklen_t len; @@ -386,7 +391,8 @@ static void test_insert_bound(int family, int sotype, int mapfd) xclose(s); } -static void test_insert(int family, int sotype, int mapfd) +static void test_insert(struct test_sockmap_listen *skel __always_unused, + int family, int sotype, int mapfd) { u64 value; u32 key; @@ -402,7 +408,8 @@ static void test_insert(int family, int sotype, int mapfd) xclose(s); } -static void test_delete_after_insert(int family, int sotype, int mapfd) +static void test_delete_after_insert(struct test_sockmap_listen *skel __always_unused, + int family, int sotype, int mapfd) { u64 value; u32 key; @@ -419,7 +426,8 @@ static void test_delete_after_insert(int family, int sotype, int mapfd) xclose(s); } -static void test_delete_after_close(int family, int sotype, int mapfd) +static void test_delete_after_close(struct test_sockmap_listen *skel __always_unused, + int family, int sotype, int mapfd) { int err, s; u64 value; @@ -442,7 +450,8 @@ static void test_delete_after_close(int family, int sotype, int mapfd) FAIL_ERRNO("map_delete: expected EINVAL/EINVAL"); } -static void test_lookup_after_insert(int family, int sotype, int mapfd) +static void test_lookup_after_insert(struct test_sockmap_listen *skel __always_unused, + int family, int sotype, int mapfd) { u64 cookie, value; socklen_t len; @@ -470,7 +479,8 @@ static void test_lookup_after_insert(int family, int sotype, int mapfd) xclose(s); } -static void test_lookup_after_delete(int family, int sotype, int mapfd) +static void test_lookup_after_delete(struct test_sockmap_listen *skel __always_unused, + int family, int sotype, int mapfd) { int err, s; u64 value; @@ -493,7 +503,8 @@ static void test_lookup_after_delete(int family, int sotype, int mapfd) xclose(s); } -static void test_lookup_32_bit_value(int family, int sotype, int mapfd) +static void test_lookup_32_bit_value(struct test_sockmap_listen *skel __always_unused, + int family, int sotype, int mapfd) { u32 key, value32; int err, s; @@ -523,7 +534,8 @@ static void test_lookup_32_bit_value(int family, int sotype, int mapfd) xclose(s); } -static void test_update_existing(int family, int sotype, int mapfd) +static void test_update_existing(struct test_sockmap_listen *skel __always_unused, + int family, int sotype, int mapfd) { int s1, s2; u64 value; @@ -551,7 +563,8 @@ static void test_update_existing(int family, int sotype, int mapfd) /* Exercise the code path where we destroy child sockets that never * got accept()'ed, aka orphans, when parent socket gets closed. */ -static void test_destroy_orphan_child(int family, int sotype, int mapfd) +static void test_destroy_orphan_child(struct test_sockmap_listen *skel __always_unused, + int family, int sotype, int mapfd) { struct sockaddr_storage addr; socklen_t len; @@ -585,7 +598,8 @@ static void test_destroy_orphan_child(int family, int sotype, int mapfd) /* Perform a passive open after removing listening socket from SOCKMAP * to ensure that callbacks get restored properly. */ -static void test_clone_after_delete(int family, int sotype, int mapfd) +static void test_clone_after_delete(struct test_sockmap_listen *skel __always_unused, + int family, int sotype, int mapfd) { struct sockaddr_storage addr; socklen_t len; @@ -621,7 +635,8 @@ static void test_clone_after_delete(int family, int sotype, int mapfd) * SOCKMAP, but got accept()'ed only after the parent has been removed * from SOCKMAP, gets cloned without parent psock state or callbacks. */ -static void test_accept_after_delete(int family, int sotype, int mapfd) +static void test_accept_after_delete(struct test_sockmap_listen *skel __always_unused, + int family, int sotype, int mapfd) { struct sockaddr_storage addr; const u32 zero = 0; @@ -675,7 +690,8 @@ static void test_accept_after_delete(int family, int sotype, int mapfd) /* Check that child socket that got created and accepted while parent * was in a SOCKMAP is cloned without parent psock state or callbacks. */ -static void test_accept_before_delete(int family, int sotype, int mapfd) +static void test_accept_before_delete(struct test_sockmap_listen *skel __always_unused, + int family, int sotype, int mapfd) { struct sockaddr_storage addr; const u32 zero = 0, one = 1; @@ -784,7 +800,8 @@ static void *connect_accept_thread(void *arg) return NULL; } -static void test_syn_recv_insert_delete(int family, int sotype, int mapfd) +static void test_syn_recv_insert_delete(struct test_sockmap_listen *skel __always_unused, + int family, int sotype, int mapfd) { struct connect_accept_ctx ctx = { 0 }; struct sockaddr_storage addr; @@ -847,7 +864,8 @@ static void *listen_thread(void *arg) return NULL; } -static void test_race_insert_listen(int family, int socktype, int mapfd) +static void test_race_insert_listen(struct test_sockmap_listen *skel __always_unused, + int family, int socktype, int mapfd) { struct connect_accept_ctx ctx = { 0 }; const u32 zero = 0; @@ -1473,7 +1491,8 @@ static void test_ops(struct test_sockmap_listen *skel, struct bpf_map *map, int family, int sotype) { const struct op_test { - void (*fn)(int family, int sotype, int mapfd); + void (*fn)(struct test_sockmap_listen *skel, + int family, int sotype, int mapfd); const char *name; int sotype; } tests[] = { @@ -1520,7 +1539,7 @@ static void test_ops(struct test_sockmap_listen *skel, struct bpf_map *map, if (!test__start_subtest(s)) continue; - t->fn(family, sotype, map_fd); + t->fn(skel, family, sotype, map_fd); test_ops_cleanup(map); } } -- 2.39.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* RE: [PATCH bpf v2 3/4] selftests/bpf: Pass BPF skeleton to sockmap_listen ops tests 2023-01-21 12:41 ` [PATCH bpf v2 3/4] selftests/bpf: Pass BPF skeleton to sockmap_listen ops tests Jakub Sitnicki @ 2023-01-25 5:27 ` John Fastabend 0 siblings, 0 replies; 10+ messages in thread From: John Fastabend @ 2023-01-25 5:27 UTC (permalink / raw) To: Jakub Sitnicki, bpf Cc: netdev, John Fastabend, Eric Dumazet, Daniel Borkmann, Alexei Starovoitov, Andrii Nakryiko, kernel-team Jakub Sitnicki wrote: > Following patch extends the sockmap ops tests to cover the scenario when a > sockmap with attached programs holds listening sockets. > > Pass the BPF skeleton to sockmap ops test so that the can access and attach > the BPF programs. > > Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com> > --- Acked-by: John Fastabend <john.fastabend@gmail.com> ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH bpf v2 4/4] selftests/bpf: Cover listener cloning with progs attached to sockmap 2023-01-21 12:41 [PATCH bpf v2 0/4] bpf, sockmap: Fix infinite recursion in sock_map_close Jakub Sitnicki ` (2 preceding siblings ...) 2023-01-21 12:41 ` [PATCH bpf v2 3/4] selftests/bpf: Pass BPF skeleton to sockmap_listen ops tests Jakub Sitnicki @ 2023-01-21 12:41 ` Jakub Sitnicki 2023-01-25 5:28 ` John Fastabend 2023-01-25 6:00 ` [PATCH bpf v2 0/4] bpf, sockmap: Fix infinite recursion in sock_map_close patchwork-bot+netdevbpf 4 siblings, 1 reply; 10+ messages in thread From: Jakub Sitnicki @ 2023-01-21 12:41 UTC (permalink / raw) To: bpf Cc: netdev, John Fastabend, Eric Dumazet, Daniel Borkmann, Alexei Starovoitov, Andrii Nakryiko, kernel-team Today we test if a child socket is cloned properly from a listening socket inside a sockmap only when there are no BPF programs attached to the map. A bug has been reported [1] for the case when sockmap has a verdict program attached. So cover this case as well to prevent regressions. [1]: https://lore.kernel.org/r/00000000000073b14905ef2e7401@google.com Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com> --- .../selftests/bpf/prog_tests/sockmap_listen.c | 30 ++++++++++++++++++++-- 1 file changed, 28 insertions(+), 2 deletions(-) diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c index 499fba8f55b9..567e07c19ecc 100644 --- a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c +++ b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c @@ -563,8 +563,7 @@ static void test_update_existing(struct test_sockmap_listen *skel __always_unuse /* Exercise the code path where we destroy child sockets that never * got accept()'ed, aka orphans, when parent socket gets closed. */ -static void test_destroy_orphan_child(struct test_sockmap_listen *skel __always_unused, - int family, int sotype, int mapfd) +static void do_destroy_orphan_child(int family, int sotype, int mapfd) { struct sockaddr_storage addr; socklen_t len; @@ -595,6 +594,33 @@ static void test_destroy_orphan_child(struct test_sockmap_listen *skel __always_ xclose(s); } +static void test_destroy_orphan_child(struct test_sockmap_listen *skel, + int family, int sotype, int mapfd) +{ + int msg_verdict = bpf_program__fd(skel->progs.prog_msg_verdict); + int skb_verdict = bpf_program__fd(skel->progs.prog_skb_verdict); + const struct test { + int progfd; + enum bpf_attach_type atype; + } tests[] = { + { -1, -1 }, + { msg_verdict, BPF_SK_MSG_VERDICT }, + { skb_verdict, BPF_SK_SKB_VERDICT }, + }; + const struct test *t; + + for (t = tests; t < tests + ARRAY_SIZE(tests); t++) { + if (t->progfd != -1 && + xbpf_prog_attach(t->progfd, mapfd, t->atype, 0) != 0) + return; + + do_destroy_orphan_child(family, sotype, mapfd); + + if (t->progfd != -1) + xbpf_prog_detach2(t->progfd, mapfd, t->atype); + } +} + /* Perform a passive open after removing listening socket from SOCKMAP * to ensure that callbacks get restored properly. */ -- 2.39.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* RE: [PATCH bpf v2 4/4] selftests/bpf: Cover listener cloning with progs attached to sockmap 2023-01-21 12:41 ` [PATCH bpf v2 4/4] selftests/bpf: Cover listener cloning with progs attached to sockmap Jakub Sitnicki @ 2023-01-25 5:28 ` John Fastabend 0 siblings, 0 replies; 10+ messages in thread From: John Fastabend @ 2023-01-25 5:28 UTC (permalink / raw) To: Jakub Sitnicki, bpf Cc: netdev, John Fastabend, Eric Dumazet, Daniel Borkmann, Alexei Starovoitov, Andrii Nakryiko, kernel-team Jakub Sitnicki wrote: > Today we test if a child socket is cloned properly from a listening socket > inside a sockmap only when there are no BPF programs attached to the map. > > A bug has been reported [1] for the case when sockmap has a verdict program > attached. So cover this case as well to prevent regressions. > > [1]: https://lore.kernel.org/r/00000000000073b14905ef2e7401@google.com > > Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com> > --- Acked-by: John Fastabend <john.fastabend@gmail.com> ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH bpf v2 0/4] bpf, sockmap: Fix infinite recursion in sock_map_close 2023-01-21 12:41 [PATCH bpf v2 0/4] bpf, sockmap: Fix infinite recursion in sock_map_close Jakub Sitnicki ` (3 preceding siblings ...) 2023-01-21 12:41 ` [PATCH bpf v2 4/4] selftests/bpf: Cover listener cloning with progs attached to sockmap Jakub Sitnicki @ 2023-01-25 6:00 ` patchwork-bot+netdevbpf 4 siblings, 0 replies; 10+ messages in thread From: patchwork-bot+netdevbpf @ 2023-01-25 6:00 UTC (permalink / raw) To: Jakub Sitnicki Cc: bpf, netdev, john.fastabend, edumazet, daniel, ast, andrii, kernel-team, syzbot+04c21ed96d861dccc5cd Hello: This series was applied to bpf/bpf.git (master) by Alexei Starovoitov <ast@kernel.org>: On Sat, 21 Jan 2023 13:41:42 +0100 you wrote: > This patch set addresses the syzbot report in [1]. > > Patch #1 has been suggested by Eric [2]. I extended it to cover the rest of > sock_map proto callbacks. Otherwise we would still overflow the stack. > > Patch #2 contains the actual fix and bug analysis. > Patches #3 & #4 add coverage to selftests to trigger the bug. > > [...] Here is the summary with links: - [bpf,v2,1/4] bpf, sockmap: Don't let sock_map_{close,destroy,unhash} call itself https://git.kernel.org/bpf/bpf/c/5b4a79ba65a1 - [bpf,v2,2/4] bpf, sockmap: Check for any of tcp_bpf_prots when cloning a listener https://git.kernel.org/bpf/bpf/c/ddce1e091757 - [bpf,v2,3/4] selftests/bpf: Pass BPF skeleton to sockmap_listen ops tests https://git.kernel.org/bpf/bpf/c/b4ea530d024c - [bpf,v2,4/4] selftests/bpf: Cover listener cloning with progs attached to sockmap https://git.kernel.org/bpf/bpf/c/c88ea16a8f89 You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2023-01-25 6:00 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-01-21 12:41 [PATCH bpf v2 0/4] bpf, sockmap: Fix infinite recursion in sock_map_close Jakub Sitnicki
2023-01-21 12:41 ` [PATCH bpf v2 1/4] bpf, sockmap: Don't let sock_map_{close,destroy,unhash} call itself Jakub Sitnicki
2023-01-25 5:20 ` John Fastabend
2023-01-21 12:41 ` [PATCH bpf v2 2/4] bpf, sockmap: Check for any of tcp_bpf_prots when cloning a listener Jakub Sitnicki
2023-01-25 5:25 ` John Fastabend
2023-01-21 12:41 ` [PATCH bpf v2 3/4] selftests/bpf: Pass BPF skeleton to sockmap_listen ops tests Jakub Sitnicki
2023-01-25 5:27 ` John Fastabend
2023-01-21 12:41 ` [PATCH bpf v2 4/4] selftests/bpf: Cover listener cloning with progs attached to sockmap Jakub Sitnicki
2023-01-25 5:28 ` John Fastabend
2023-01-25 6:00 ` [PATCH bpf v2 0/4] bpf, sockmap: Fix infinite recursion in sock_map_close patchwork-bot+netdevbpf
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).