Message ID | 20230223215311.926899-4-aditi.ghag@isovalent.com (mailing list archive) |
---|---|
State | Changes Requested |
Delegated to: | BPF |
Headers | show |
Series | : Add socket destroy capability | expand |
On 02/23, Aditi Ghag wrote: > The test cases for TCP and UDP iterators mirror the intended usages of the > helper. > The destroy helpers set `ECONNABORTED` error code that we can validate in > the > test code with client sockets. But UDP sockets have an overriding error > code > from the disconnect called during abort, so the error code the validation > is > only done for TCP sockets. > The `struct sock` is redefined as vmlinux.h forward declares the struct, > and the > loader fails to load the program as it finds the BTF FWD type for the > struct > incompatible with the BTF STRUCT type. > Here are the snippets of the verifier error, and corresponding BTF output: > ``` > verifier error: extern (func ksym) ...: func_proto ... incompatible with > kernel > BTF for selftest prog binary: > [104] FWD 'sock' fwd_kind=struct > [70] PTR '(anon)' type_id=104 > [84] FUNC_PROTO '(anon)' ret_type_id=2 vlen=1 > '(anon)' type_id=70 > [85] FUNC 'bpf_sock_destroy' type_id=84 linkage=extern > -- > [96] DATASEC '.ksyms' size=0 vlen=1 > type_id=85 offset=0 size=0 (FUNC 'bpf_sock_destroy') > BTF for selftest vmlinux: > [74923] FUNC 'bpf_sock_destroy' type_id=48965 linkage=static > [48965] FUNC_PROTO '(anon)' ret_type_id=9 vlen=1 > 'sk' type_id=1340 > [1340] PTR '(anon)' type_id=2363 > [2363] STRUCT 'sock' size=1280 vlen=93 > ``` > Signed-off-by: Aditi Ghag <aditi.ghag@isovalent.com> > --- > .../selftests/bpf/prog_tests/sock_destroy.c | 125 ++++++++++++++++++ > .../selftests/bpf/progs/sock_destroy_prog.c | 110 +++++++++++++++ > 2 files changed, 235 insertions(+) > create mode 100644 tools/testing/selftests/bpf/prog_tests/sock_destroy.c > create mode 100644 tools/testing/selftests/bpf/progs/sock_destroy_prog.c > diff --git a/tools/testing/selftests/bpf/prog_tests/sock_destroy.c > b/tools/testing/selftests/bpf/prog_tests/sock_destroy.c > new file mode 100644 > index 000000000000..d9da9d3578e2 > --- /dev/null > +++ b/tools/testing/selftests/bpf/prog_tests/sock_destroy.c > @@ -0,0 +1,125 @@ > +// SPDX-License-Identifier: GPL-2.0 > +#include <test_progs.h> > + > +#include "sock_destroy_prog.skel.h" > +#include "network_helpers.h" > + > +#define ECONNABORTED 103 > + > +static int duration; > + > +static void start_iter_sockets(struct bpf_program *prog) > +{ > + struct bpf_link *link; > + char buf[16] = {}; > + int iter_fd, len; > + > + link = bpf_program__attach_iter(prog, NULL); > + if (!ASSERT_OK_PTR(link, "attach_iter")) > + return; > + > + iter_fd = bpf_iter_create(bpf_link__fd(link)); > + if (!ASSERT_GE(iter_fd, 0, "create_iter")) > + goto free_link; > + > + while ((len = read(iter_fd, buf, sizeof(buf))) > 0) > + ; > + CHECK(len < 0, "read", "read failed: %s\n", strerror(errno)); > + > + close(iter_fd); > + > +free_link: > + bpf_link__destroy(link); > +} > + > +void test_tcp(struct sock_destroy_prog *skel) > +{ > + int serv = -1, clien = -1, n = 0; > + > + serv = start_server(AF_INET6, SOCK_STREAM, NULL, 0, 0); > + if (CHECK(serv < 0, "start_server", "failed to start server\n")) > + goto cleanup_serv; > + > + clien = connect_to_fd(serv, 0); > + if (CHECK(clien < 0, "connect_to_fd", "errno %d\n", errno)) > + goto cleanup_serv; > + > + serv = accept(serv, NULL, NULL); > + if (CHECK(serv < 0, "accept", "errno %d\n", errno)) > + goto cleanup; > + > + n = send(clien, "t", 1, 0); > + if (CHECK(n < 0, "client_send", "client failed to send on socket\n")) > + goto cleanup; > + > + start_iter_sockets(skel->progs.iter_tcp6); > + > + n = send(clien, "t", 1, 0); > + if (CHECK(n > 0, "client_send after destroy", "succeeded on destroyed > socket\n")) > + goto cleanup; > + CHECK(errno != ECONNABORTED, "client_send", "unexpected error code on > destroyed socket\n"); > + > + > +cleanup: > + close(clien); > +cleanup_serv: > + close(serv); > +} > + > + > +void test_udp(struct sock_destroy_prog *skel) > +{ > + int serv = -1, clien = -1, n = 0; > + > + serv = start_server(AF_INET6, SOCK_DGRAM, NULL, 6161, 0); > + if (CHECK(serv < 0, "start_server", "failed to start server\n")) > + goto cleanup_serv; > + > + clien = connect_to_fd(serv, 0); > + if (CHECK(clien < 0, "connect_to_fd", "errno %d\n", errno)) > + goto cleanup_serv; > + > + n = send(clien, "t", 1, 0); > + if (CHECK(n < 0, "client_send", "client failed to send on socket\n")) > + goto cleanup; > + > + start_iter_sockets(skel->progs.iter_udp6); > + > + n = send(clien, "t", 1, 0); > + if (CHECK(n > 0, "client_send after destroy", "succeeded on destroyed > socket\n")) > + goto cleanup; > + // UDP sockets have an overriding error code after they are > disconnected. C++-style comments. > + > + > +cleanup: > + close(clien); > +cleanup_serv: > + close(serv); > +} > + > +void test_sock_destroy(void) > +{ > + int cgroup_fd = 0; > + struct sock_destroy_prog *skel; > + > + skel = sock_destroy_prog__open_and_load(); > + if (!ASSERT_OK_PTR(skel, "skel_open")) > + return; > + > + cgroup_fd = test__join_cgroup("/sock_destroy"); > + if (CHECK(cgroup_fd < 0, "join_cgroup", "cgroup creation failed\n")) > + goto close_cgroup_fd; > + > + skel->links.sock_connect = bpf_program__attach_cgroup( > + skel->progs.sock_connect, cgroup_fd); > + if (!ASSERT_OK_PTR(skel->links.sock_connect, "prog_attach")) > + goto close_cgroup_fd; > + > + test_tcp(skel); > + test_udp(skel); > + > + > +close_cgroup_fd: > + close(cgroup_fd); > + sock_destroy_prog__destroy(skel); > +} > diff --git a/tools/testing/selftests/bpf/progs/sock_destroy_prog.c > b/tools/testing/selftests/bpf/progs/sock_destroy_prog.c > new file mode 100644 > index 000000000000..c6805a9b7594 > --- /dev/null > +++ b/tools/testing/selftests/bpf/progs/sock_destroy_prog.c > @@ -0,0 +1,110 @@ > +// SPDX-License-Identifier: GPL-2.0 > + > +#define sock sock___not_used > +#include "vmlinux.h" > +#undef sock > + > +#include <bpf/bpf_helpers.h> > + > +#define AF_INET6 10 > + > +/* Redefine the struct: vmlinux.h forward declares it, and the loader > fails > + * to load the program as it finds the BTF FWD type for the struct > incompatible > + * with the BTF STRUCT type. > + */ > +struct sock { > + struct sock_common __sk_common; > +#define sk_family __sk_common.skc_family > +#define sk_cookie __sk_common.skc_cookie > +}; > + > +int bpf_sock_destroy(struct sock_common *sk) __ksym; > + > +struct { > + __uint(type, BPF_MAP_TYPE_ARRAY); > + __uint(max_entries, 1); > + __type(key, __u32); > + __type(value, __u64); > +} tcp_conn_sockets SEC(".maps"); > + > +struct { > + __uint(type, BPF_MAP_TYPE_ARRAY); > + __uint(max_entries, 1); > + __type(key, __u32); > + __type(value, __u64); > +} udp_conn_sockets SEC(".maps"); > + > +SEC("cgroup/connect6") > +int sock_connect(struct bpf_sock_addr *ctx) > +{ > + int key = 0; > + __u64 sock_cookie = 0; > + __u32 keyc = 0; > + > + if (ctx->family != AF_INET6 || ctx->user_family != AF_INET6) > + return 1; > + > + sock_cookie = bpf_get_socket_cookie(ctx); > + if (ctx->protocol == IPPROTO_TCP) > + bpf_map_update_elem(&tcp_conn_sockets, &key, &sock_cookie, 0); > + else if (ctx->protocol == IPPROTO_UDP) > + bpf_map_update_elem(&udp_conn_sockets, &keyc, &sock_cookie, 0); > + else > + return 1; > + > + return 1; > +} > + > +SEC("iter/tcp") > +int iter_tcp6(struct bpf_iter__tcp *ctx) > +{ > + struct sock_common *sk_common = ctx->sk_common; > + struct seq_file *seq = ctx->meta->seq; > + __u64 sock_cookie = 0; > + __u64 *val; > + int key = 0; > + > + if (!sk_common) > + return 0; > + > + if (sk_common->skc_family != AF_INET6) > + return 0; > + > + sock_cookie = bpf_get_socket_cookie(sk_common); > + val = bpf_map_lookup_elem(&tcp_conn_sockets, &key); > + > + if (!val) > + return 0; > + > + if (sock_cookie == *val) > + bpf_sock_destroy(sk_common); > + > + return 0; > +} > + > +SEC("iter/udp") > +int iter_udp6(struct bpf_iter__udp *ctx) > +{ > + struct seq_file *seq = ctx->meta->seq; > + struct udp_sock *udp_sk = ctx->udp_sk; > + struct sock *sk = (struct sock *) udp_sk; > + __u64 sock_cookie = 0; > + int key = 0; > + __u64 *val; > + > + if (!sk) > + return 0; > + > + sock_cookie = bpf_get_socket_cookie(sk); > + val = bpf_map_lookup_elem(&udp_conn_sockets, &key); > + > + if (!val) > + return 0; > + > + if (sock_cookie == *val) > + bpf_sock_destroy((struct sock_common *)sk); > + > + return 0; > +} > + > +char _license[] SEC("license") = "GPL"; > -- > 2.34.1
On Thu, Feb 23, 2023 at 2:05 PM Aditi Ghag <aditi.ghag@isovalent.com> wrote: > > The test cases for TCP and UDP iterators mirror the intended usages of the > helper. > > The destroy helpers set `ECONNABORTED` error code that we can validate in the > test code with client sockets. But UDP sockets have an overriding error code > from the disconnect called during abort, so the error code the validation is > only done for TCP sockets. > > The `struct sock` is redefined as vmlinux.h forward declares the struct, and the > loader fails to load the program as it finds the BTF FWD type for the struct > incompatible with the BTF STRUCT type. > > Here are the snippets of the verifier error, and corresponding BTF output: > > ``` > verifier error: extern (func ksym) ...: func_proto ... incompatible with kernel > > BTF for selftest prog binary: > > [104] FWD 'sock' fwd_kind=struct > [70] PTR '(anon)' type_id=104 > [84] FUNC_PROTO '(anon)' ret_type_id=2 vlen=1 > '(anon)' type_id=70 > [85] FUNC 'bpf_sock_destroy' type_id=84 linkage=extern > -- > [96] DATASEC '.ksyms' size=0 vlen=1 > type_id=85 offset=0 size=0 (FUNC 'bpf_sock_destroy') > > BTF for selftest vmlinux: > > [74923] FUNC 'bpf_sock_destroy' type_id=48965 linkage=static > [48965] FUNC_PROTO '(anon)' ret_type_id=9 vlen=1 > 'sk' type_id=1340 > [1340] PTR '(anon)' type_id=2363 > [2363] STRUCT 'sock' size=1280 vlen=93 > ``` > > Signed-off-by: Aditi Ghag <aditi.ghag@isovalent.com> > --- > .../selftests/bpf/prog_tests/sock_destroy.c | 125 ++++++++++++++++++ > .../selftests/bpf/progs/sock_destroy_prog.c | 110 +++++++++++++++ > 2 files changed, 235 insertions(+) > create mode 100644 tools/testing/selftests/bpf/prog_tests/sock_destroy.c > create mode 100644 tools/testing/selftests/bpf/progs/sock_destroy_prog.c > [...] > + n = send(clien, "t", 1, 0); > + if (CHECK(n < 0, "client_send", "client failed to send on socket\n")) > + goto cleanup; > + > + start_iter_sockets(skel->progs.iter_tcp6); > + > + n = send(clien, "t", 1, 0); > + if (CHECK(n > 0, "client_send after destroy", "succeeded on destroyed socket\n")) > + goto cleanup; > + CHECK(errno != ECONNABORTED, "client_send", "unexpected error code on destroyed socket\n"); > + please don't use CHECK() macros, prefere ASSERT_xxx() ones > + > +cleanup: > + close(clien); > +cleanup_serv: > + close(serv); > +} > + > + > +void test_udp(struct sock_destroy_prog *skel) are these meant to be subtests? If yes, model them as such? and in either case, make these funcs static > +{ > + int serv = -1, clien = -1, n = 0; > + > + serv = start_server(AF_INET6, SOCK_DGRAM, NULL, 6161, 0); > + if (CHECK(serv < 0, "start_server", "failed to start server\n")) > + goto cleanup_serv; > + [...]
On 2/23/23 1:53 PM, Aditi Ghag wrote: > The test cases for TCP and UDP iterators mirror the intended usages of the > helper. > > The destroy helpers set `ECONNABORTED` error code that we can validate in the > test code with client sockets. But UDP sockets have an overriding error code > from the disconnect called during abort, so the error code the validation is > only done for TCP sockets. > > The `struct sock` is redefined as vmlinux.h forward declares the struct, and the > loader fails to load the program as it finds the BTF FWD type for the struct > incompatible with the BTF STRUCT type. > > Here are the snippets of the verifier error, and corresponding BTF output: > > ``` > verifier error: extern (func ksym) ...: func_proto ... incompatible with kernel > > BTF for selftest prog binary: > > [104] FWD 'sock' fwd_kind=struct > [70] PTR '(anon)' type_id=104 > [84] FUNC_PROTO '(anon)' ret_type_id=2 vlen=1 > '(anon)' type_id=70 > [85] FUNC 'bpf_sock_destroy' type_id=84 linkage=extern > -- > [96] DATASEC '.ksyms' size=0 vlen=1 > type_id=85 offset=0 size=0 (FUNC 'bpf_sock_destroy') > > BTF for selftest vmlinux: > > [74923] FUNC 'bpf_sock_destroy' type_id=48965 linkage=static > [48965] FUNC_PROTO '(anon)' ret_type_id=9 vlen=1 > 'sk' type_id=1340 > [1340] PTR '(anon)' type_id=2363 > [2363] STRUCT 'sock' size=1280 vlen=93 > ``` > +int bpf_sock_destroy(struct sock_common *sk) __ksym; This does not match the bpf prog's BTF dump above which has pointer [70] pointing to FWD 'sock' [104] as the argument. It should be at least FWD 'sock_common' if not STRUCT 'sock_common'. I tried to change the func signature to 'struct sock *sk' but cannot reproduce the issue in my environment also. Could you confirm the BTF paste and 'incompatible with kernel" error in the commit message do match the bpf_sock_destroy declaration? If not, can you re-paste the BTF output and libbpf error message that matches the bpf_sock_destroy signature.
> On Feb 28, 2023, at 3:08 PM, Martin KaFai Lau <martin.lau@linux.dev> wrote: > > On 2/23/23 1:53 PM, Aditi Ghag wrote: >> The test cases for TCP and UDP iterators mirror the intended usages of the >> helper. >> The destroy helpers set `ECONNABORTED` error code that we can validate in the >> test code with client sockets. But UDP sockets have an overriding error code >> from the disconnect called during abort, so the error code the validation is >> only done for TCP sockets. >> The `struct sock` is redefined as vmlinux.h forward declares the struct, and the >> loader fails to load the program as it finds the BTF FWD type for the struct >> incompatible with the BTF STRUCT type. >> Here are the snippets of the verifier error, and corresponding BTF output: >> ``` >> verifier error: extern (func ksym) ...: func_proto ... incompatible with kernel >> BTF for selftest prog binary: >> [104] FWD 'sock' fwd_kind=struct >> [70] PTR '(anon)' type_id=104 >> [84] FUNC_PROTO '(anon)' ret_type_id=2 vlen=1 >> '(anon)' type_id=70 >> [85] FUNC 'bpf_sock_destroy' type_id=84 linkage=extern >> -- >> [96] DATASEC '.ksyms' size=0 vlen=1 >> type_id=85 offset=0 size=0 (FUNC 'bpf_sock_destroy') >> BTF for selftest vmlinux: >> [74923] FUNC 'bpf_sock_destroy' type_id=48965 linkage=static >> [48965] FUNC_PROTO '(anon)' ret_type_id=9 vlen=1 >> 'sk' type_id=1340 >> [1340] PTR '(anon)' type_id=2363 >> [2363] STRUCT 'sock' size=1280 vlen=93 >> ``` > >> +int bpf_sock_destroy(struct sock_common *sk) __ksym; > > This does not match the bpf prog's BTF dump above which has pointer [70] pointing to FWD 'sock' [104] as the argument. It should be at least FWD 'sock_common' if not STRUCT 'sock_common'. I tried to change the func signature to 'struct sock *sk' but cannot reproduce the issue in my environment also. > > Could you confirm the BTF paste and 'incompatible with kernel" error in the commit message do match the bpf_sock_destroy declaration? If not, can you re-paste the BTF output and libbpf error message that matches the bpf_sock_destroy signature. I don't think you'll be able to reproduce the issue with `sock_common`, as `struct sock_common` isn't forward declared in vmlinux.h. But I find it odd that you weren't able to reproduce it with `struct sock`. Just to confirm, did you remove the minimal `struct sock` definition from the program? Per the commit description, I added that because libbpf was throwing this error - `libbpf: extern (func ksym) 'bpf_sock_destroy': func_proto [83] incompatible with kernel [75285]` Sending the BTF snippet again (full BTF - https://pastebin.com/etkFyuJk) ``` 85] FUNC 'bpf_sock_destroy' type_id=84 linkage=extern type_id=85 offset=0 size=0 (FUNC 'bpf_sock_destroy') [84] FUNC_PROTO '(anon)' ret_type_id=2 vlen=1 '(anon)' type_id=70 [70] PTR '(anon)' type_id=104 [104] FWD 'sock' fwd_kind=struct ``` Compare this to the BTF snippet once I undef and define the struct in the test prog: ``` [87] FUNC 'bpf_sock_destroy' type_id=84 linkage=extern type_id=87 offset=0 size=0 (FUNC 'bpf_sock_destroy') [84] FUNC_PROTO '(anon)' ret_type_id=2 vlen=1 '(anon)' type_id=85 [85] PTR '(anon)' type_id=86 [86] STRUCT 'sock' size=136 vlen=1 '__sk_common' type_id=34 bits_offset=0 ``` (Anyway looks like I needed to define the struct in the test prog only when bpf_sock_destory had `struct sock` as the argument.)
On 2/28/23 6:17 PM, Aditi Ghag wrote: > > >> On Feb 28, 2023, at 3:08 PM, Martin KaFai Lau <martin.lau@linux.dev> wrote: >> >> On 2/23/23 1:53 PM, Aditi Ghag wrote: >>> The test cases for TCP and UDP iterators mirror the intended usages of the >>> helper. >>> The destroy helpers set `ECONNABORTED` error code that we can validate in the >>> test code with client sockets. But UDP sockets have an overriding error code >>> from the disconnect called during abort, so the error code the validation is >>> only done for TCP sockets. >>> The `struct sock` is redefined as vmlinux.h forward declares the struct, and the >>> loader fails to load the program as it finds the BTF FWD type for the struct >>> incompatible with the BTF STRUCT type. >>> Here are the snippets of the verifier error, and corresponding BTF output: >>> ``` >>> verifier error: extern (func ksym) ...: func_proto ... incompatible with kernel >>> BTF for selftest prog binary: >>> [104] FWD 'sock' fwd_kind=struct >>> [70] PTR '(anon)' type_id=104 >>> [84] FUNC_PROTO '(anon)' ret_type_id=2 vlen=1 >>> '(anon)' type_id=70 >>> [85] FUNC 'bpf_sock_destroy' type_id=84 linkage=extern >>> -- >>> [96] DATASEC '.ksyms' size=0 vlen=1 >>> type_id=85 offset=0 size=0 (FUNC 'bpf_sock_destroy') >>> BTF for selftest vmlinux: >>> [74923] FUNC 'bpf_sock_destroy' type_id=48965 linkage=static >>> [48965] FUNC_PROTO '(anon)' ret_type_id=9 vlen=1 >>> 'sk' type_id=1340 >>> [1340] PTR '(anon)' type_id=2363 >>> [2363] STRUCT 'sock' size=1280 vlen=93 >>> ``` >> >>> +int bpf_sock_destroy(struct sock_common *sk) __ksym; >> >> This does not match the bpf prog's BTF dump above which has pointer [70] pointing to FWD 'sock' [104] as the argument. It should be at least FWD 'sock_common' if not STRUCT 'sock_common'. I tried to change the func signature to 'struct sock *sk' but cannot reproduce the issue in my environment also. >> >> Could you confirm the BTF paste and 'incompatible with kernel" error in the commit message do match the bpf_sock_destroy declaration? If not, can you re-paste the BTF output and libbpf error message that matches the bpf_sock_destroy signature. > > I don't think you'll be able to reproduce the issue with `sock_common`, as `struct sock_common` isn't forward declared in vmlinux.h. But I find it odd that you weren't able to reproduce it with `struct sock`. Just to confirm, did you remove the minimal `struct sock` definition from the program? Per the commit description, I added that because libbpf was throwing this error - > `libbpf: extern (func ksym) 'bpf_sock_destroy': func_proto [83] incompatible with kernel [75285]` Yep, I changed the kfunc to 'struct sock *' and removed the define/undef dance. > > Sending the BTF snippet again (full BTF - https://pastebin.com/etkFyuJk) > > ``` > 85] FUNC 'bpf_sock_destroy' type_id=84 linkage=extern > type_id=85 offset=0 size=0 (FUNC 'bpf_sock_destroy') > [84] FUNC_PROTO '(anon)' ret_type_id=2 vlen=1 > '(anon)' type_id=70 > [70] PTR '(anon)' type_id=104 > [104] FWD 'sock' fwd_kind=struct > ``` > > Compare this to the BTF snippet once I undef and define the struct in the test prog: > > ``` > [87] FUNC 'bpf_sock_destroy' type_id=84 linkage=extern > type_id=87 offset=0 size=0 (FUNC 'bpf_sock_destroy') > [84] FUNC_PROTO '(anon)' ret_type_id=2 vlen=1 > '(anon)' type_id=85 > [85] PTR '(anon)' type_id=86 > [86] STRUCT 'sock' size=136 vlen=1 > '__sk_common' type_id=34 bits_offset=0 > ``` > > (Anyway looks like I needed to define the struct in the test prog only when bpf_sock_destory had `struct sock` as the argument.) Right, I also think it is orthogonal to your set if the kfunc is taking 'struct sock_common *' anyway. [although I do feel a kernel function taking a 'struct sock_common *' is rather odd] I was only asking and also trying myself because it looks pretty wrong if it can be reproduced and it is something that should be fixed regardless. It is pretty normal to have forward declaration within a bpf prog itself (not from vmlinux.h). From the paste, it feels like the kfunc bpf_sock_destroy btf is generated earlier than the 'struct sock'. Which llvm version are you using?
> On Mar 1, 2023, at 11:06 PM, Martin KaFai Lau <martin.lau@linux.dev> wrote: > > On 2/28/23 6:17 PM, Aditi Ghag wrote: >>> On Feb 28, 2023, at 3:08 PM, Martin KaFai Lau <martin.lau@linux.dev> wrote: >>> >>> On 2/23/23 1:53 PM, Aditi Ghag wrote: >>>> The test cases for TCP and UDP iterators mirror the intended usages of the >>>> helper. >>>> The destroy helpers set `ECONNABORTED` error code that we can validate in the >>>> test code with client sockets. But UDP sockets have an overriding error code >>>> from the disconnect called during abort, so the error code the validation is >>>> only done for TCP sockets. >>>> The `struct sock` is redefined as vmlinux.h forward declares the struct, and the >>>> loader fails to load the program as it finds the BTF FWD type for the struct >>>> incompatible with the BTF STRUCT type. >>>> Here are the snippets of the verifier error, and corresponding BTF output: >>>> ``` >>>> verifier error: extern (func ksym) ...: func_proto ... incompatible with kernel >>>> BTF for selftest prog binary: >>>> [104] FWD 'sock' fwd_kind=struct >>>> [70] PTR '(anon)' type_id=104 >>>> [84] FUNC_PROTO '(anon)' ret_type_id=2 vlen=1 >>>> '(anon)' type_id=70 >>>> [85] FUNC 'bpf_sock_destroy' type_id=84 linkage=extern >>>> -- >>>> [96] DATASEC '.ksyms' size=0 vlen=1 >>>> type_id=85 offset=0 size=0 (FUNC 'bpf_sock_destroy') >>>> BTF for selftest vmlinux: >>>> [74923] FUNC 'bpf_sock_destroy' type_id=48965 linkage=static >>>> [48965] FUNC_PROTO '(anon)' ret_type_id=9 vlen=1 >>>> 'sk' type_id=1340 >>>> [1340] PTR '(anon)' type_id=2363 >>>> [2363] STRUCT 'sock' size=1280 vlen=93 >>>> ``` >>> >>>> +int bpf_sock_destroy(struct sock_common *sk) __ksym; >>> >>> This does not match the bpf prog's BTF dump above which has pointer [70] pointing to FWD 'sock' [104] as the argument. It should be at least FWD 'sock_common' if not STRUCT 'sock_common'. I tried to change the func signature to 'struct sock *sk' but cannot reproduce the issue in my environment also. >>> >>> Could you confirm the BTF paste and 'incompatible with kernel" error in the commit message do match the bpf_sock_destroy declaration? If not, can you re-paste the BTF output and libbpf error message that matches the bpf_sock_destroy signature. >> I don't think you'll be able to reproduce the issue with `sock_common`, as `struct sock_common` isn't forward declared in vmlinux.h. But I find it odd that you weren't able to reproduce it with `struct sock`. Just to confirm, did you remove the minimal `struct sock` definition from the program? Per the commit description, I added that because libbpf was throwing this error - >> `libbpf: extern (func ksym) 'bpf_sock_destroy': func_proto [83] incompatible with kernel [75285]` > > Yep, I changed the kfunc to 'struct sock *' and removed the define/undef dance. > >> Sending the BTF snippet again (full BTF - https://pastebin.com/etkFyuJk) >> ``` >> 85] FUNC 'bpf_sock_destroy' type_id=84 linkage=extern >> type_id=85 offset=0 size=0 (FUNC 'bpf_sock_destroy') >> [84] FUNC_PROTO '(anon)' ret_type_id=2 vlen=1 >> '(anon)' type_id=70 >> [70] PTR '(anon)' type_id=104 >> [104] FWD 'sock' fwd_kind=struct >> ``` >> Compare this to the BTF snippet once I undef and define the struct in the test prog: >> ``` >> [87] FUNC 'bpf_sock_destroy' type_id=84 linkage=extern >> type_id=87 offset=0 size=0 (FUNC 'bpf_sock_destroy') >> [84] FUNC_PROTO '(anon)' ret_type_id=2 vlen=1 >> '(anon)' type_id=85 >> [85] PTR '(anon)' type_id=86 >> [86] STRUCT 'sock' size=136 vlen=1 >> '__sk_common' type_id=34 bits_offset=0 >> ``` >> (Anyway looks like I needed to define the struct in the test prog only when bpf_sock_destory had `struct sock` as the argument.) > > Right, I also think it is orthogonal to your set if the kfunc is taking 'struct sock_common *' anyway. [although I do feel a kernel function taking a 'struct sock_common *' is rather odd] Yes, this wasn't a problem with the helper taking `struct sock` as the argument in v1 patch. I'm all ears if we can have a similar signature for the kfunc. > > I was only asking and also trying myself because it looks pretty wrong if it can be reproduced and it is something that should be fixed regardless. It is pretty normal to have forward declaration within a bpf prog itself (not from vmlinux.h). From the paste, it feels like the kfunc bpf_sock_destroy btf is generated earlier than the 'struct sock'. Which llvm version are you using? $ llvm-config --version 14.0.0
> On Feb 27, 2023, at 11:37 AM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Thu, Feb 23, 2023 at 2:05 PM Aditi Ghag <aditi.ghag@isovalent.com> wrote: >> >> The test cases for TCP and UDP iterators mirror the intended usages of the >> helper. >> >> The destroy helpers set `ECONNABORTED` error code that we can validate in the >> test code with client sockets. But UDP sockets have an overriding error code >> from the disconnect called during abort, so the error code the validation is >> only done for TCP sockets. >> >> The `struct sock` is redefined as vmlinux.h forward declares the struct, and the >> loader fails to load the program as it finds the BTF FWD type for the struct >> incompatible with the BTF STRUCT type. >> >> Here are the snippets of the verifier error, and corresponding BTF output: >> >> ``` >> verifier error: extern (func ksym) ...: func_proto ... incompatible with kernel >> >> BTF for selftest prog binary: >> >> [104] FWD 'sock' fwd_kind=struct >> [70] PTR '(anon)' type_id=104 >> [84] FUNC_PROTO '(anon)' ret_type_id=2 vlen=1 >> '(anon)' type_id=70 >> [85] FUNC 'bpf_sock_destroy' type_id=84 linkage=extern >> -- >> [96] DATASEC '.ksyms' size=0 vlen=1 >> type_id=85 offset=0 size=0 (FUNC 'bpf_sock_destroy') >> >> BTF for selftest vmlinux: >> >> [74923] FUNC 'bpf_sock_destroy' type_id=48965 linkage=static >> [48965] FUNC_PROTO '(anon)' ret_type_id=9 vlen=1 >> 'sk' type_id=1340 >> [1340] PTR '(anon)' type_id=2363 >> [2363] STRUCT 'sock' size=1280 vlen=93 >> ``` >> >> Signed-off-by: Aditi Ghag <aditi.ghag@isovalent.com> >> --- >> .../selftests/bpf/prog_tests/sock_destroy.c | 125 ++++++++++++++++++ >> .../selftests/bpf/progs/sock_destroy_prog.c | 110 +++++++++++++++ >> 2 files changed, 235 insertions(+) >> create mode 100644 tools/testing/selftests/bpf/prog_tests/sock_destroy.c >> create mode 100644 tools/testing/selftests/bpf/progs/sock_destroy_prog.c >> > > [...] > >> + n = send(clien, "t", 1, 0); >> + if (CHECK(n < 0, "client_send", "client failed to send on socket\n")) >> + goto cleanup; >> + >> + start_iter_sockets(skel->progs.iter_tcp6); >> + >> + n = send(clien, "t", 1, 0); >> + if (CHECK(n > 0, "client_send after destroy", "succeeded on destroyed socket\n")) >> + goto cleanup; >> + CHECK(errno != ECONNABORTED, "client_send", "unexpected error code on destroyed socket\n"); >> + > > please don't use CHECK() macros, prefere ASSERT_xxx() ones Yes, this'll be handled in the next revision. Thanks! > >> + >> +cleanup: >> + close(clien); >> +cleanup_serv: >> + close(serv); >> +} >> + >> + >> +void test_udp(struct sock_destroy_prog *skel) > > are these meant to be subtests? If yes, model them as such? I wasn't aware of subtests markers. I'll look into the other tests for reference. > > and in either case, make these funcs static Will do! > >> +{ >> + int serv = -1, clien = -1, n = 0; >> + >> + serv = start_server(AF_INET6, SOCK_DGRAM, NULL, 6161, 0); >> + if (CHECK(serv < 0, "start_server", "failed to start server\n")) >> + goto cleanup_serv; >> + > > [...]
diff --git a/tools/testing/selftests/bpf/prog_tests/sock_destroy.c b/tools/testing/selftests/bpf/prog_tests/sock_destroy.c new file mode 100644 index 000000000000..d9da9d3578e2 --- /dev/null +++ b/tools/testing/selftests/bpf/prog_tests/sock_destroy.c @@ -0,0 +1,125 @@ +// SPDX-License-Identifier: GPL-2.0 +#include <test_progs.h> + +#include "sock_destroy_prog.skel.h" +#include "network_helpers.h" + +#define ECONNABORTED 103 + +static int duration; + +static void start_iter_sockets(struct bpf_program *prog) +{ + struct bpf_link *link; + char buf[16] = {}; + int iter_fd, len; + + link = bpf_program__attach_iter(prog, NULL); + if (!ASSERT_OK_PTR(link, "attach_iter")) + return; + + iter_fd = bpf_iter_create(bpf_link__fd(link)); + if (!ASSERT_GE(iter_fd, 0, "create_iter")) + goto free_link; + + while ((len = read(iter_fd, buf, sizeof(buf))) > 0) + ; + CHECK(len < 0, "read", "read failed: %s\n", strerror(errno)); + + close(iter_fd); + +free_link: + bpf_link__destroy(link); +} + +void test_tcp(struct sock_destroy_prog *skel) +{ + int serv = -1, clien = -1, n = 0; + + serv = start_server(AF_INET6, SOCK_STREAM, NULL, 0, 0); + if (CHECK(serv < 0, "start_server", "failed to start server\n")) + goto cleanup_serv; + + clien = connect_to_fd(serv, 0); + if (CHECK(clien < 0, "connect_to_fd", "errno %d\n", errno)) + goto cleanup_serv; + + serv = accept(serv, NULL, NULL); + if (CHECK(serv < 0, "accept", "errno %d\n", errno)) + goto cleanup; + + n = send(clien, "t", 1, 0); + if (CHECK(n < 0, "client_send", "client failed to send on socket\n")) + goto cleanup; + + start_iter_sockets(skel->progs.iter_tcp6); + + n = send(clien, "t", 1, 0); + if (CHECK(n > 0, "client_send after destroy", "succeeded on destroyed socket\n")) + goto cleanup; + CHECK(errno != ECONNABORTED, "client_send", "unexpected error code on destroyed socket\n"); + + +cleanup: + close(clien); +cleanup_serv: + close(serv); +} + + +void test_udp(struct sock_destroy_prog *skel) +{ + int serv = -1, clien = -1, n = 0; + + serv = start_server(AF_INET6, SOCK_DGRAM, NULL, 6161, 0); + if (CHECK(serv < 0, "start_server", "failed to start server\n")) + goto cleanup_serv; + + clien = connect_to_fd(serv, 0); + if (CHECK(clien < 0, "connect_to_fd", "errno %d\n", errno)) + goto cleanup_serv; + + n = send(clien, "t", 1, 0); + if (CHECK(n < 0, "client_send", "client failed to send on socket\n")) + goto cleanup; + + start_iter_sockets(skel->progs.iter_udp6); + + n = send(clien, "t", 1, 0); + if (CHECK(n > 0, "client_send after destroy", "succeeded on destroyed socket\n")) + goto cleanup; + // UDP sockets have an overriding error code after they are disconnected. + + +cleanup: + close(clien); +cleanup_serv: + close(serv); +} + +void test_sock_destroy(void) +{ + int cgroup_fd = 0; + struct sock_destroy_prog *skel; + + skel = sock_destroy_prog__open_and_load(); + if (!ASSERT_OK_PTR(skel, "skel_open")) + return; + + cgroup_fd = test__join_cgroup("/sock_destroy"); + if (CHECK(cgroup_fd < 0, "join_cgroup", "cgroup creation failed\n")) + goto close_cgroup_fd; + + skel->links.sock_connect = bpf_program__attach_cgroup( + skel->progs.sock_connect, cgroup_fd); + if (!ASSERT_OK_PTR(skel->links.sock_connect, "prog_attach")) + goto close_cgroup_fd; + + test_tcp(skel); + test_udp(skel); + + +close_cgroup_fd: + close(cgroup_fd); + sock_destroy_prog__destroy(skel); +} diff --git a/tools/testing/selftests/bpf/progs/sock_destroy_prog.c b/tools/testing/selftests/bpf/progs/sock_destroy_prog.c new file mode 100644 index 000000000000..c6805a9b7594 --- /dev/null +++ b/tools/testing/selftests/bpf/progs/sock_destroy_prog.c @@ -0,0 +1,110 @@ +// SPDX-License-Identifier: GPL-2.0 + +#define sock sock___not_used +#include "vmlinux.h" +#undef sock + +#include <bpf/bpf_helpers.h> + +#define AF_INET6 10 + +/* Redefine the struct: vmlinux.h forward declares it, and the loader fails + * to load the program as it finds the BTF FWD type for the struct incompatible + * with the BTF STRUCT type. + */ +struct sock { + struct sock_common __sk_common; +#define sk_family __sk_common.skc_family +#define sk_cookie __sk_common.skc_cookie +}; + +int bpf_sock_destroy(struct sock_common *sk) __ksym; + +struct { + __uint(type, BPF_MAP_TYPE_ARRAY); + __uint(max_entries, 1); + __type(key, __u32); + __type(value, __u64); +} tcp_conn_sockets SEC(".maps"); + +struct { + __uint(type, BPF_MAP_TYPE_ARRAY); + __uint(max_entries, 1); + __type(key, __u32); + __type(value, __u64); +} udp_conn_sockets SEC(".maps"); + +SEC("cgroup/connect6") +int sock_connect(struct bpf_sock_addr *ctx) +{ + int key = 0; + __u64 sock_cookie = 0; + __u32 keyc = 0; + + if (ctx->family != AF_INET6 || ctx->user_family != AF_INET6) + return 1; + + sock_cookie = bpf_get_socket_cookie(ctx); + if (ctx->protocol == IPPROTO_TCP) + bpf_map_update_elem(&tcp_conn_sockets, &key, &sock_cookie, 0); + else if (ctx->protocol == IPPROTO_UDP) + bpf_map_update_elem(&udp_conn_sockets, &keyc, &sock_cookie, 0); + else + return 1; + + return 1; +} + +SEC("iter/tcp") +int iter_tcp6(struct bpf_iter__tcp *ctx) +{ + struct sock_common *sk_common = ctx->sk_common; + struct seq_file *seq = ctx->meta->seq; + __u64 sock_cookie = 0; + __u64 *val; + int key = 0; + + if (!sk_common) + return 0; + + if (sk_common->skc_family != AF_INET6) + return 0; + + sock_cookie = bpf_get_socket_cookie(sk_common); + val = bpf_map_lookup_elem(&tcp_conn_sockets, &key); + + if (!val) + return 0; + + if (sock_cookie == *val) + bpf_sock_destroy(sk_common); + + return 0; +} + +SEC("iter/udp") +int iter_udp6(struct bpf_iter__udp *ctx) +{ + struct seq_file *seq = ctx->meta->seq; + struct udp_sock *udp_sk = ctx->udp_sk; + struct sock *sk = (struct sock *) udp_sk; + __u64 sock_cookie = 0; + int key = 0; + __u64 *val; + + if (!sk) + return 0; + + sock_cookie = bpf_get_socket_cookie(sk); + val = bpf_map_lookup_elem(&udp_conn_sockets, &key); + + if (!val) + return 0; + + if (sock_cookie == *val) + bpf_sock_destroy((struct sock_common *)sk); + + return 0; +} + +char _license[] SEC("license") = "GPL";
The test cases for TCP and UDP iterators mirror the intended usages of the helper. The destroy helpers set `ECONNABORTED` error code that we can validate in the test code with client sockets. But UDP sockets have an overriding error code from the disconnect called during abort, so the error code the validation is only done for TCP sockets. The `struct sock` is redefined as vmlinux.h forward declares the struct, and the loader fails to load the program as it finds the BTF FWD type for the struct incompatible with the BTF STRUCT type. Here are the snippets of the verifier error, and corresponding BTF output: ``` verifier error: extern (func ksym) ...: func_proto ... incompatible with kernel BTF for selftest prog binary: [104] FWD 'sock' fwd_kind=struct [70] PTR '(anon)' type_id=104 [84] FUNC_PROTO '(anon)' ret_type_id=2 vlen=1 '(anon)' type_id=70 [85] FUNC 'bpf_sock_destroy' type_id=84 linkage=extern -- [96] DATASEC '.ksyms' size=0 vlen=1 type_id=85 offset=0 size=0 (FUNC 'bpf_sock_destroy') BTF for selftest vmlinux: [74923] FUNC 'bpf_sock_destroy' type_id=48965 linkage=static [48965] FUNC_PROTO '(anon)' ret_type_id=9 vlen=1 'sk' type_id=1340 [1340] PTR '(anon)' type_id=2363 [2363] STRUCT 'sock' size=1280 vlen=93 ``` Signed-off-by: Aditi Ghag <aditi.ghag@isovalent.com> --- .../selftests/bpf/prog_tests/sock_destroy.c | 125 ++++++++++++++++++ .../selftests/bpf/progs/sock_destroy_prog.c | 110 +++++++++++++++ 2 files changed, 235 insertions(+) create mode 100644 tools/testing/selftests/bpf/prog_tests/sock_destroy.c create mode 100644 tools/testing/selftests/bpf/progs/sock_destroy_prog.c