Re: [PATCH bpf-next v5 4/4] selftests/bpf: Add a selftest for the tracing bpf_get_socket_cookie
On Sat, Jan 23, 2021 at 9:45 PM Yonghong Song wrote: > On 1/22/21 7:34 AM, Florent Revest wrote: > > On Wed, Jan 20, 2021 at 8:06 PM Florent Revest wrote: > >> > >> On Wed, Jan 20, 2021 at 8:04 PM Alexei Starovoitov > >> wrote: > >>> > >>> On Wed, Jan 20, 2021 at 9:08 AM KP Singh wrote: > > On Tue, Jan 19, 2021 at 5:00 PM Florent Revest > wrote: > > > > This builds up on the existing socket cookie test which checks whether > > the bpf_get_socket_cookie helpers provide the same value in > > cgroup/connect6 and sockops programs for a socket created by the > > userspace part of the test. > > > > Adding a tracing program to the existing objects requires a different > > attachment strategy and different headers. > > > > Signed-off-by: Florent Revest > > Acked-by: KP Singh > > (one minor note, doesn't really need fixing as a part of this though) > > > --- > > .../selftests/bpf/prog_tests/socket_cookie.c | 24 +++ > > .../selftests/bpf/progs/socket_cookie_prog.c | 41 --- > > 2 files changed, 52 insertions(+), 13 deletions(-) > > > > diff --git a/tools/testing/selftests/bpf/prog_tests/socket_cookie.c > > b/tools/testing/selftests/bpf/prog_tests/socket_cookie.c > > index 53d0c44e7907..e5c5e2ea1deb 100644 > > --- a/tools/testing/selftests/bpf/prog_tests/socket_cookie.c > > +++ b/tools/testing/selftests/bpf/prog_tests/socket_cookie.c > > @@ -15,8 +15,8 @@ struct socket_cookie { > > > > void test_socket_cookie(void) > > { > > + struct bpf_link *set_link, *update_sockops_link, > > *update_tracing_link; > > socklen_t addr_len = sizeof(struct sockaddr_in6); > > - struct bpf_link *set_link, *update_link; > > int server_fd, client_fd, cgroup_fd; > > struct socket_cookie_prog *skel; > > __u32 cookie_expected_value; > > @@ -39,15 +39,21 @@ void test_socket_cookie(void) > >PTR_ERR(set_link))) > > goto close_cgroup_fd; > > > > - update_link = > > bpf_program__attach_cgroup(skel->progs.update_cookie, > > -cgroup_fd); > > - if (CHECK(IS_ERR(update_link), "update-link-cg-attach", "err > > %ld\n", > > - PTR_ERR(update_link))) > > + update_sockops_link = bpf_program__attach_cgroup( > > + skel->progs.update_cookie_sockops, cgroup_fd); > > + if (CHECK(IS_ERR(update_sockops_link), > > "update-sockops-link-cg-attach", > > + "err %ld\n", PTR_ERR(update_sockops_link))) > > goto free_set_link; > > > > + update_tracing_link = bpf_program__attach( > > + skel->progs.update_cookie_tracing); > > + if (CHECK(IS_ERR(update_tracing_link), > > "update-tracing-link-attach", > > + "err %ld\n", PTR_ERR(update_tracing_link))) > > + goto free_update_sockops_link; > > + > > server_fd = start_server(AF_INET6, SOCK_STREAM, "::1", 0, 0); > > if (CHECK(server_fd < 0, "start_server", "errno %d\n", errno)) > > - goto free_update_link; > > + goto free_update_tracing_link; > > > > client_fd = connect_to_fd(server_fd, 0); > > if (CHECK(client_fd < 0, "connect_to_fd", "errno %d\n", errno)) > > @@ -71,8 +77,10 @@ void test_socket_cookie(void) > > close(client_fd); > > close_server_fd: > > close(server_fd); > > -free_update_link: > > - bpf_link__destroy(update_link); > > +free_update_tracing_link: > > + bpf_link__destroy(update_tracing_link); > > I don't think this need to block submission unless there are other > issues but the > bpf_link__destroy can just be called in a single cleanup label because > it handles null or > erroneous inputs: > > int bpf_link__destroy(struct bpf_link *link) > { > int err = 0; > > if (IS_ERR_OR_NULL(link)) > return 0; > [...] > >>> > >>> +1 to KP's point. > >>> > >>> Also Florent, how did you test it? > >>> This test fails in CI and in my manual run: > >>> ./test_progs -t cook > >>> libbpf: load bpf program failed: Permission denied > >>> libbpf: -- BEGIN DUMP LOG --- > >>> libbpf: > >>> ; int update_cookie_sockops(struct bpf_sock_ops *ctx) > >>> 0: (bf) r6 = r1 > >>> ; if (ctx->family != AF_INET6) > >>> 1: (61) r1 = *(u32 *)(r6 +20) > >>> ; if (ctx->family != AF_INET6) > >>> 2: (56) if w1 != 0xa goto pc+21 > >>> R1_w=inv10 R6_w=ctx(id=0,off=0,imm=0) R10=fp0 > >>> ; if (ctx->op != BPF_SOCK_OPS_TCP_CONNECT_CB) > >>> 3: (61) r1 = *(u32 *)(r6 +0) > >>> ; if (ctx->op != BPF_SOCK_OPS_TCP_CONNECT_CB) > >>> 4: (56) if w1 !=
Re: [PATCH bpf-next v5 4/4] selftests/bpf: Add a selftest for the tracing bpf_get_socket_cookie
On 1/22/21 7:34 AM, Florent Revest wrote: On Wed, Jan 20, 2021 at 8:06 PM Florent Revest wrote: On Wed, Jan 20, 2021 at 8:04 PM Alexei Starovoitov wrote: On Wed, Jan 20, 2021 at 9:08 AM KP Singh wrote: On Tue, Jan 19, 2021 at 5:00 PM Florent Revest wrote: This builds up on the existing socket cookie test which checks whether the bpf_get_socket_cookie helpers provide the same value in cgroup/connect6 and sockops programs for a socket created by the userspace part of the test. Adding a tracing program to the existing objects requires a different attachment strategy and different headers. Signed-off-by: Florent Revest Acked-by: KP Singh (one minor note, doesn't really need fixing as a part of this though) --- .../selftests/bpf/prog_tests/socket_cookie.c | 24 +++ .../selftests/bpf/progs/socket_cookie_prog.c | 41 --- 2 files changed, 52 insertions(+), 13 deletions(-) diff --git a/tools/testing/selftests/bpf/prog_tests/socket_cookie.c b/tools/testing/selftests/bpf/prog_tests/socket_cookie.c index 53d0c44e7907..e5c5e2ea1deb 100644 --- a/tools/testing/selftests/bpf/prog_tests/socket_cookie.c +++ b/tools/testing/selftests/bpf/prog_tests/socket_cookie.c @@ -15,8 +15,8 @@ struct socket_cookie { void test_socket_cookie(void) { + struct bpf_link *set_link, *update_sockops_link, *update_tracing_link; socklen_t addr_len = sizeof(struct sockaddr_in6); - struct bpf_link *set_link, *update_link; int server_fd, client_fd, cgroup_fd; struct socket_cookie_prog *skel; __u32 cookie_expected_value; @@ -39,15 +39,21 @@ void test_socket_cookie(void) PTR_ERR(set_link))) goto close_cgroup_fd; - update_link = bpf_program__attach_cgroup(skel->progs.update_cookie, -cgroup_fd); - if (CHECK(IS_ERR(update_link), "update-link-cg-attach", "err %ld\n", - PTR_ERR(update_link))) + update_sockops_link = bpf_program__attach_cgroup( + skel->progs.update_cookie_sockops, cgroup_fd); + if (CHECK(IS_ERR(update_sockops_link), "update-sockops-link-cg-attach", + "err %ld\n", PTR_ERR(update_sockops_link))) goto free_set_link; + update_tracing_link = bpf_program__attach( + skel->progs.update_cookie_tracing); + if (CHECK(IS_ERR(update_tracing_link), "update-tracing-link-attach", + "err %ld\n", PTR_ERR(update_tracing_link))) + goto free_update_sockops_link; + server_fd = start_server(AF_INET6, SOCK_STREAM, "::1", 0, 0); if (CHECK(server_fd < 0, "start_server", "errno %d\n", errno)) - goto free_update_link; + goto free_update_tracing_link; client_fd = connect_to_fd(server_fd, 0); if (CHECK(client_fd < 0, "connect_to_fd", "errno %d\n", errno)) @@ -71,8 +77,10 @@ void test_socket_cookie(void) close(client_fd); close_server_fd: close(server_fd); -free_update_link: - bpf_link__destroy(update_link); +free_update_tracing_link: + bpf_link__destroy(update_tracing_link); I don't think this need to block submission unless there are other issues but the bpf_link__destroy can just be called in a single cleanup label because it handles null or erroneous inputs: int bpf_link__destroy(struct bpf_link *link) { int err = 0; if (IS_ERR_OR_NULL(link)) return 0; [...] +1 to KP's point. Also Florent, how did you test it? This test fails in CI and in my manual run: ./test_progs -t cook libbpf: load bpf program failed: Permission denied libbpf: -- BEGIN DUMP LOG --- libbpf: ; int update_cookie_sockops(struct bpf_sock_ops *ctx) 0: (bf) r6 = r1 ; if (ctx->family != AF_INET6) 1: (61) r1 = *(u32 *)(r6 +20) ; if (ctx->family != AF_INET6) 2: (56) if w1 != 0xa goto pc+21 R1_w=inv10 R6_w=ctx(id=0,off=0,imm=0) R10=fp0 ; if (ctx->op != BPF_SOCK_OPS_TCP_CONNECT_CB) 3: (61) r1 = *(u32 *)(r6 +0) ; if (ctx->op != BPF_SOCK_OPS_TCP_CONNECT_CB) 4: (56) if w1 != 0x3 goto pc+19 R1_w=inv3 R6_w=ctx(id=0,off=0,imm=0) R10=fp0 ; if (!ctx->sk) 5: (79) r1 = *(u64 *)(r6 +184) ; if (!ctx->sk) 6: (15) if r1 == 0x0 goto pc+17 R1_w=sock(id=0,ref_obj_id=0,off=0,imm=0) R6_w=ctx(id=0,off=0,imm=0) R10=fp0 ; p = bpf_sk_storage_get(_cookies, ctx->sk, 0, 0); 7: (79) r2 = *(u64 *)(r6 +184) ; p = bpf_sk_storage_get(_cookies, ctx->sk, 0, 0); 8: (18) r1 = 0x888106e41400 10: (b7) r3 = 0 11: (b7) r4 = 0 12: (85) call bpf_sk_storage_get#107 R2 type=sock_or_null expected=sock_common, sock, tcp_sock, xdp_sock, ptr_ processed 12 insns (limit 100) max_states_per_insn 0 total_states 0 peak_states 0 mark_read 0 libbpf: -- END LOG -- libbpf: failed to load program 'update_cookie_sockops' libbpf: failed to load object 'socket_cookie_prog' libbpf: failed to load BPF skeleton 'socket_cookie_prog': -4007 test_socket_cookie:FAIL:socket_cookie_prog__open_and_load
Re: [PATCH bpf-next v5 4/4] selftests/bpf: Add a selftest for the tracing bpf_get_socket_cookie
On Wed, Jan 20, 2021 at 8:06 PM Florent Revest wrote: > > On Wed, Jan 20, 2021 at 8:04 PM Alexei Starovoitov > wrote: > > > > On Wed, Jan 20, 2021 at 9:08 AM KP Singh wrote: > > > > > > On Tue, Jan 19, 2021 at 5:00 PM Florent Revest > > > wrote: > > > > > > > > This builds up on the existing socket cookie test which checks whether > > > > the bpf_get_socket_cookie helpers provide the same value in > > > > cgroup/connect6 and sockops programs for a socket created by the > > > > userspace part of the test. > > > > > > > > Adding a tracing program to the existing objects requires a different > > > > attachment strategy and different headers. > > > > > > > > Signed-off-by: Florent Revest > > > > > > Acked-by: KP Singh > > > > > > (one minor note, doesn't really need fixing as a part of this though) > > > > > > > --- > > > > .../selftests/bpf/prog_tests/socket_cookie.c | 24 +++ > > > > .../selftests/bpf/progs/socket_cookie_prog.c | 41 --- > > > > 2 files changed, 52 insertions(+), 13 deletions(-) > > > > > > > > diff --git a/tools/testing/selftests/bpf/prog_tests/socket_cookie.c > > > > b/tools/testing/selftests/bpf/prog_tests/socket_cookie.c > > > > index 53d0c44e7907..e5c5e2ea1deb 100644 > > > > --- a/tools/testing/selftests/bpf/prog_tests/socket_cookie.c > > > > +++ b/tools/testing/selftests/bpf/prog_tests/socket_cookie.c > > > > @@ -15,8 +15,8 @@ struct socket_cookie { > > > > > > > > void test_socket_cookie(void) > > > > { > > > > + struct bpf_link *set_link, *update_sockops_link, > > > > *update_tracing_link; > > > > socklen_t addr_len = sizeof(struct sockaddr_in6); > > > > - struct bpf_link *set_link, *update_link; > > > > int server_fd, client_fd, cgroup_fd; > > > > struct socket_cookie_prog *skel; > > > > __u32 cookie_expected_value; > > > > @@ -39,15 +39,21 @@ void test_socket_cookie(void) > > > > PTR_ERR(set_link))) > > > > goto close_cgroup_fd; > > > > > > > > - update_link = > > > > bpf_program__attach_cgroup(skel->progs.update_cookie, > > > > -cgroup_fd); > > > > - if (CHECK(IS_ERR(update_link), "update-link-cg-attach", "err > > > > %ld\n", > > > > - PTR_ERR(update_link))) > > > > + update_sockops_link = bpf_program__attach_cgroup( > > > > + skel->progs.update_cookie_sockops, cgroup_fd); > > > > + if (CHECK(IS_ERR(update_sockops_link), > > > > "update-sockops-link-cg-attach", > > > > + "err %ld\n", PTR_ERR(update_sockops_link))) > > > > goto free_set_link; > > > > > > > > + update_tracing_link = bpf_program__attach( > > > > + skel->progs.update_cookie_tracing); > > > > + if (CHECK(IS_ERR(update_tracing_link), > > > > "update-tracing-link-attach", > > > > + "err %ld\n", PTR_ERR(update_tracing_link))) > > > > + goto free_update_sockops_link; > > > > + > > > > server_fd = start_server(AF_INET6, SOCK_STREAM, "::1", 0, 0); > > > > if (CHECK(server_fd < 0, "start_server", "errno %d\n", errno)) > > > > - goto free_update_link; > > > > + goto free_update_tracing_link; > > > > > > > > client_fd = connect_to_fd(server_fd, 0); > > > > if (CHECK(client_fd < 0, "connect_to_fd", "errno %d\n", errno)) > > > > @@ -71,8 +77,10 @@ void test_socket_cookie(void) > > > > close(client_fd); > > > > close_server_fd: > > > > close(server_fd); > > > > -free_update_link: > > > > - bpf_link__destroy(update_link); > > > > +free_update_tracing_link: > > > > + bpf_link__destroy(update_tracing_link); > > > > > > I don't think this need to block submission unless there are other > > > issues but the > > > bpf_link__destroy can just be called in a single cleanup label because > > > it handles null or > > > erroneous inputs: > > > > > > int bpf_link__destroy(struct bpf_link *link) > > > { > > > int err = 0; > > > > > > if (IS_ERR_OR_NULL(link)) > > > return 0; > > > [...] > > > > +1 to KP's point. > > > > Also Florent, how did you test it? > > This test fails in CI and in my manual run: > > ./test_progs -t cook > > libbpf: load bpf program failed: Permission denied > > libbpf: -- BEGIN DUMP LOG --- > > libbpf: > > ; int update_cookie_sockops(struct bpf_sock_ops *ctx) > > 0: (bf) r6 = r1 > > ; if (ctx->family != AF_INET6) > > 1: (61) r1 = *(u32 *)(r6 +20) > > ; if (ctx->family != AF_INET6) > > 2: (56) if w1 != 0xa goto pc+21 > > R1_w=inv10 R6_w=ctx(id=0,off=0,imm=0) R10=fp0 > > ; if (ctx->op != BPF_SOCK_OPS_TCP_CONNECT_CB) > > 3: (61) r1 = *(u32 *)(r6 +0) > > ; if (ctx->op != BPF_SOCK_OPS_TCP_CONNECT_CB) > > 4: (56) if w1 != 0x3 goto pc+19 > > R1_w=inv3 R6_w=ctx(id=0,off=0,imm=0) R10=fp0 > > ; if (!ctx->sk) > > 5: (79) r1 = *(u64 *)(r6 +184) > > ; if (!ctx->sk) > > 6: (15) if r1 == 0x0 goto pc+17 > >
Re: [PATCH bpf-next v5 4/4] selftests/bpf: Add a selftest for the tracing bpf_get_socket_cookie
On Wed, Jan 20, 2021 at 8:04 PM Alexei Starovoitov wrote: > > On Wed, Jan 20, 2021 at 9:08 AM KP Singh wrote: > > > > On Tue, Jan 19, 2021 at 5:00 PM Florent Revest wrote: > > > > > > This builds up on the existing socket cookie test which checks whether > > > the bpf_get_socket_cookie helpers provide the same value in > > > cgroup/connect6 and sockops programs for a socket created by the > > > userspace part of the test. > > > > > > Adding a tracing program to the existing objects requires a different > > > attachment strategy and different headers. > > > > > > Signed-off-by: Florent Revest > > > > Acked-by: KP Singh > > > > (one minor note, doesn't really need fixing as a part of this though) > > > > > --- > > > .../selftests/bpf/prog_tests/socket_cookie.c | 24 +++ > > > .../selftests/bpf/progs/socket_cookie_prog.c | 41 --- > > > 2 files changed, 52 insertions(+), 13 deletions(-) > > > > > > diff --git a/tools/testing/selftests/bpf/prog_tests/socket_cookie.c > > > b/tools/testing/selftests/bpf/prog_tests/socket_cookie.c > > > index 53d0c44e7907..e5c5e2ea1deb 100644 > > > --- a/tools/testing/selftests/bpf/prog_tests/socket_cookie.c > > > +++ b/tools/testing/selftests/bpf/prog_tests/socket_cookie.c > > > @@ -15,8 +15,8 @@ struct socket_cookie { > > > > > > void test_socket_cookie(void) > > > { > > > + struct bpf_link *set_link, *update_sockops_link, > > > *update_tracing_link; > > > socklen_t addr_len = sizeof(struct sockaddr_in6); > > > - struct bpf_link *set_link, *update_link; > > > int server_fd, client_fd, cgroup_fd; > > > struct socket_cookie_prog *skel; > > > __u32 cookie_expected_value; > > > @@ -39,15 +39,21 @@ void test_socket_cookie(void) > > > PTR_ERR(set_link))) > > > goto close_cgroup_fd; > > > > > > - update_link = > > > bpf_program__attach_cgroup(skel->progs.update_cookie, > > > -cgroup_fd); > > > - if (CHECK(IS_ERR(update_link), "update-link-cg-attach", "err > > > %ld\n", > > > - PTR_ERR(update_link))) > > > + update_sockops_link = bpf_program__attach_cgroup( > > > + skel->progs.update_cookie_sockops, cgroup_fd); > > > + if (CHECK(IS_ERR(update_sockops_link), > > > "update-sockops-link-cg-attach", > > > + "err %ld\n", PTR_ERR(update_sockops_link))) > > > goto free_set_link; > > > > > > + update_tracing_link = bpf_program__attach( > > > + skel->progs.update_cookie_tracing); > > > + if (CHECK(IS_ERR(update_tracing_link), > > > "update-tracing-link-attach", > > > + "err %ld\n", PTR_ERR(update_tracing_link))) > > > + goto free_update_sockops_link; > > > + > > > server_fd = start_server(AF_INET6, SOCK_STREAM, "::1", 0, 0); > > > if (CHECK(server_fd < 0, "start_server", "errno %d\n", errno)) > > > - goto free_update_link; > > > + goto free_update_tracing_link; > > > > > > client_fd = connect_to_fd(server_fd, 0); > > > if (CHECK(client_fd < 0, "connect_to_fd", "errno %d\n", errno)) > > > @@ -71,8 +77,10 @@ void test_socket_cookie(void) > > > close(client_fd); > > > close_server_fd: > > > close(server_fd); > > > -free_update_link: > > > - bpf_link__destroy(update_link); > > > +free_update_tracing_link: > > > + bpf_link__destroy(update_tracing_link); > > > > I don't think this need to block submission unless there are other > > issues but the > > bpf_link__destroy can just be called in a single cleanup label because > > it handles null or > > erroneous inputs: > > > > int bpf_link__destroy(struct bpf_link *link) > > { > > int err = 0; > > > > if (IS_ERR_OR_NULL(link)) > > return 0; > > [...] > > +1 to KP's point. > > Also Florent, how did you test it? > This test fails in CI and in my manual run: > ./test_progs -t cook > libbpf: load bpf program failed: Permission denied > libbpf: -- BEGIN DUMP LOG --- > libbpf: > ; int update_cookie_sockops(struct bpf_sock_ops *ctx) > 0: (bf) r6 = r1 > ; if (ctx->family != AF_INET6) > 1: (61) r1 = *(u32 *)(r6 +20) > ; if (ctx->family != AF_INET6) > 2: (56) if w1 != 0xa goto pc+21 > R1_w=inv10 R6_w=ctx(id=0,off=0,imm=0) R10=fp0 > ; if (ctx->op != BPF_SOCK_OPS_TCP_CONNECT_CB) > 3: (61) r1 = *(u32 *)(r6 +0) > ; if (ctx->op != BPF_SOCK_OPS_TCP_CONNECT_CB) > 4: (56) if w1 != 0x3 goto pc+19 > R1_w=inv3 R6_w=ctx(id=0,off=0,imm=0) R10=fp0 > ; if (!ctx->sk) > 5: (79) r1 = *(u64 *)(r6 +184) > ; if (!ctx->sk) > 6: (15) if r1 == 0x0 goto pc+17 > R1_w=sock(id=0,ref_obj_id=0,off=0,imm=0) R6_w=ctx(id=0,off=0,imm=0) R10=fp0 > ; p = bpf_sk_storage_get(_cookies, ctx->sk, 0, 0); > 7: (79) r2 = *(u64 *)(r6 +184) > ; p = bpf_sk_storage_get(_cookies, ctx->sk, 0, 0); > 8: (18) r1 = 0x888106e41400 > 10: (b7) r3 = 0 > 11: (b7) r4 = 0 > 12: (85) call bpf_sk_storage_get#107
Re: [PATCH bpf-next v5 4/4] selftests/bpf: Add a selftest for the tracing bpf_get_socket_cookie
On Wed, Jan 20, 2021 at 9:08 AM KP Singh wrote: > > On Tue, Jan 19, 2021 at 5:00 PM Florent Revest wrote: > > > > This builds up on the existing socket cookie test which checks whether > > the bpf_get_socket_cookie helpers provide the same value in > > cgroup/connect6 and sockops programs for a socket created by the > > userspace part of the test. > > > > Adding a tracing program to the existing objects requires a different > > attachment strategy and different headers. > > > > Signed-off-by: Florent Revest > > Acked-by: KP Singh > > (one minor note, doesn't really need fixing as a part of this though) > > > --- > > .../selftests/bpf/prog_tests/socket_cookie.c | 24 +++ > > .../selftests/bpf/progs/socket_cookie_prog.c | 41 --- > > 2 files changed, 52 insertions(+), 13 deletions(-) > > > > diff --git a/tools/testing/selftests/bpf/prog_tests/socket_cookie.c > > b/tools/testing/selftests/bpf/prog_tests/socket_cookie.c > > index 53d0c44e7907..e5c5e2ea1deb 100644 > > --- a/tools/testing/selftests/bpf/prog_tests/socket_cookie.c > > +++ b/tools/testing/selftests/bpf/prog_tests/socket_cookie.c > > @@ -15,8 +15,8 @@ struct socket_cookie { > > > > void test_socket_cookie(void) > > { > > + struct bpf_link *set_link, *update_sockops_link, > > *update_tracing_link; > > socklen_t addr_len = sizeof(struct sockaddr_in6); > > - struct bpf_link *set_link, *update_link; > > int server_fd, client_fd, cgroup_fd; > > struct socket_cookie_prog *skel; > > __u32 cookie_expected_value; > > @@ -39,15 +39,21 @@ void test_socket_cookie(void) > > PTR_ERR(set_link))) > > goto close_cgroup_fd; > > > > - update_link = bpf_program__attach_cgroup(skel->progs.update_cookie, > > -cgroup_fd); > > - if (CHECK(IS_ERR(update_link), "update-link-cg-attach", "err %ld\n", > > - PTR_ERR(update_link))) > > + update_sockops_link = bpf_program__attach_cgroup( > > + skel->progs.update_cookie_sockops, cgroup_fd); > > + if (CHECK(IS_ERR(update_sockops_link), > > "update-sockops-link-cg-attach", > > + "err %ld\n", PTR_ERR(update_sockops_link))) > > goto free_set_link; > > > > + update_tracing_link = bpf_program__attach( > > + skel->progs.update_cookie_tracing); > > + if (CHECK(IS_ERR(update_tracing_link), "update-tracing-link-attach", > > + "err %ld\n", PTR_ERR(update_tracing_link))) > > + goto free_update_sockops_link; > > + > > server_fd = start_server(AF_INET6, SOCK_STREAM, "::1", 0, 0); > > if (CHECK(server_fd < 0, "start_server", "errno %d\n", errno)) > > - goto free_update_link; > > + goto free_update_tracing_link; > > > > client_fd = connect_to_fd(server_fd, 0); > > if (CHECK(client_fd < 0, "connect_to_fd", "errno %d\n", errno)) > > @@ -71,8 +77,10 @@ void test_socket_cookie(void) > > close(client_fd); > > close_server_fd: > > close(server_fd); > > -free_update_link: > > - bpf_link__destroy(update_link); > > +free_update_tracing_link: > > + bpf_link__destroy(update_tracing_link); > > I don't think this need to block submission unless there are other > issues but the > bpf_link__destroy can just be called in a single cleanup label because > it handles null or > erroneous inputs: > > int bpf_link__destroy(struct bpf_link *link) > { > int err = 0; > > if (IS_ERR_OR_NULL(link)) > return 0; > [...] +1 to KP's point. Also Florent, how did you test it? This test fails in CI and in my manual run: ./test_progs -t cook libbpf: load bpf program failed: Permission denied libbpf: -- BEGIN DUMP LOG --- libbpf: ; int update_cookie_sockops(struct bpf_sock_ops *ctx) 0: (bf) r6 = r1 ; if (ctx->family != AF_INET6) 1: (61) r1 = *(u32 *)(r6 +20) ; if (ctx->family != AF_INET6) 2: (56) if w1 != 0xa goto pc+21 R1_w=inv10 R6_w=ctx(id=0,off=0,imm=0) R10=fp0 ; if (ctx->op != BPF_SOCK_OPS_TCP_CONNECT_CB) 3: (61) r1 = *(u32 *)(r6 +0) ; if (ctx->op != BPF_SOCK_OPS_TCP_CONNECT_CB) 4: (56) if w1 != 0x3 goto pc+19 R1_w=inv3 R6_w=ctx(id=0,off=0,imm=0) R10=fp0 ; if (!ctx->sk) 5: (79) r1 = *(u64 *)(r6 +184) ; if (!ctx->sk) 6: (15) if r1 == 0x0 goto pc+17 R1_w=sock(id=0,ref_obj_id=0,off=0,imm=0) R6_w=ctx(id=0,off=0,imm=0) R10=fp0 ; p = bpf_sk_storage_get(_cookies, ctx->sk, 0, 0); 7: (79) r2 = *(u64 *)(r6 +184) ; p = bpf_sk_storage_get(_cookies, ctx->sk, 0, 0); 8: (18) r1 = 0x888106e41400 10: (b7) r3 = 0 11: (b7) r4 = 0 12: (85) call bpf_sk_storage_get#107 R2 type=sock_or_null expected=sock_common, sock, tcp_sock, xdp_sock, ptr_ processed 12 insns (limit 100) max_states_per_insn 0 total_states 0 peak_states 0 mark_read 0 libbpf: -- END LOG -- libbpf: failed to load program 'update_cookie_sockops' libbpf: failed to load object 'socket_cookie_prog' libbpf: failed to load
Re: [PATCH bpf-next v5 4/4] selftests/bpf: Add a selftest for the tracing bpf_get_socket_cookie
On Tue, Jan 19, 2021 at 5:00 PM Florent Revest wrote: > > This builds up on the existing socket cookie test which checks whether > the bpf_get_socket_cookie helpers provide the same value in > cgroup/connect6 and sockops programs for a socket created by the > userspace part of the test. > > Adding a tracing program to the existing objects requires a different > attachment strategy and different headers. > > Signed-off-by: Florent Revest Acked-by: KP Singh (one minor note, doesn't really need fixing as a part of this though) > --- > .../selftests/bpf/prog_tests/socket_cookie.c | 24 +++ > .../selftests/bpf/progs/socket_cookie_prog.c | 41 --- > 2 files changed, 52 insertions(+), 13 deletions(-) > > diff --git a/tools/testing/selftests/bpf/prog_tests/socket_cookie.c > b/tools/testing/selftests/bpf/prog_tests/socket_cookie.c > index 53d0c44e7907..e5c5e2ea1deb 100644 > --- a/tools/testing/selftests/bpf/prog_tests/socket_cookie.c > +++ b/tools/testing/selftests/bpf/prog_tests/socket_cookie.c > @@ -15,8 +15,8 @@ struct socket_cookie { > > void test_socket_cookie(void) > { > + struct bpf_link *set_link, *update_sockops_link, *update_tracing_link; > socklen_t addr_len = sizeof(struct sockaddr_in6); > - struct bpf_link *set_link, *update_link; > int server_fd, client_fd, cgroup_fd; > struct socket_cookie_prog *skel; > __u32 cookie_expected_value; > @@ -39,15 +39,21 @@ void test_socket_cookie(void) > PTR_ERR(set_link))) > goto close_cgroup_fd; > > - update_link = bpf_program__attach_cgroup(skel->progs.update_cookie, > -cgroup_fd); > - if (CHECK(IS_ERR(update_link), "update-link-cg-attach", "err %ld\n", > - PTR_ERR(update_link))) > + update_sockops_link = bpf_program__attach_cgroup( > + skel->progs.update_cookie_sockops, cgroup_fd); > + if (CHECK(IS_ERR(update_sockops_link), > "update-sockops-link-cg-attach", > + "err %ld\n", PTR_ERR(update_sockops_link))) > goto free_set_link; > > + update_tracing_link = bpf_program__attach( > + skel->progs.update_cookie_tracing); > + if (CHECK(IS_ERR(update_tracing_link), "update-tracing-link-attach", > + "err %ld\n", PTR_ERR(update_tracing_link))) > + goto free_update_sockops_link; > + > server_fd = start_server(AF_INET6, SOCK_STREAM, "::1", 0, 0); > if (CHECK(server_fd < 0, "start_server", "errno %d\n", errno)) > - goto free_update_link; > + goto free_update_tracing_link; > > client_fd = connect_to_fd(server_fd, 0); > if (CHECK(client_fd < 0, "connect_to_fd", "errno %d\n", errno)) > @@ -71,8 +77,10 @@ void test_socket_cookie(void) > close(client_fd); > close_server_fd: > close(server_fd); > -free_update_link: > - bpf_link__destroy(update_link); > +free_update_tracing_link: > + bpf_link__destroy(update_tracing_link); I don't think this need to block submission unless there are other issues but the bpf_link__destroy can just be called in a single cleanup label because it handles null or erroneous inputs: int bpf_link__destroy(struct bpf_link *link) { int err = 0; if (IS_ERR_OR_NULL(link)) return 0; [...]
[PATCH bpf-next v5 4/4] selftests/bpf: Add a selftest for the tracing bpf_get_socket_cookie
This builds up on the existing socket cookie test which checks whether the bpf_get_socket_cookie helpers provide the same value in cgroup/connect6 and sockops programs for a socket created by the userspace part of the test. Adding a tracing program to the existing objects requires a different attachment strategy and different headers. Signed-off-by: Florent Revest --- .../selftests/bpf/prog_tests/socket_cookie.c | 24 +++ .../selftests/bpf/progs/socket_cookie_prog.c | 41 --- 2 files changed, 52 insertions(+), 13 deletions(-) diff --git a/tools/testing/selftests/bpf/prog_tests/socket_cookie.c b/tools/testing/selftests/bpf/prog_tests/socket_cookie.c index 53d0c44e7907..e5c5e2ea1deb 100644 --- a/tools/testing/selftests/bpf/prog_tests/socket_cookie.c +++ b/tools/testing/selftests/bpf/prog_tests/socket_cookie.c @@ -15,8 +15,8 @@ struct socket_cookie { void test_socket_cookie(void) { + struct bpf_link *set_link, *update_sockops_link, *update_tracing_link; socklen_t addr_len = sizeof(struct sockaddr_in6); - struct bpf_link *set_link, *update_link; int server_fd, client_fd, cgroup_fd; struct socket_cookie_prog *skel; __u32 cookie_expected_value; @@ -39,15 +39,21 @@ void test_socket_cookie(void) PTR_ERR(set_link))) goto close_cgroup_fd; - update_link = bpf_program__attach_cgroup(skel->progs.update_cookie, -cgroup_fd); - if (CHECK(IS_ERR(update_link), "update-link-cg-attach", "err %ld\n", - PTR_ERR(update_link))) + update_sockops_link = bpf_program__attach_cgroup( + skel->progs.update_cookie_sockops, cgroup_fd); + if (CHECK(IS_ERR(update_sockops_link), "update-sockops-link-cg-attach", + "err %ld\n", PTR_ERR(update_sockops_link))) goto free_set_link; + update_tracing_link = bpf_program__attach( + skel->progs.update_cookie_tracing); + if (CHECK(IS_ERR(update_tracing_link), "update-tracing-link-attach", + "err %ld\n", PTR_ERR(update_tracing_link))) + goto free_update_sockops_link; + server_fd = start_server(AF_INET6, SOCK_STREAM, "::1", 0, 0); if (CHECK(server_fd < 0, "start_server", "errno %d\n", errno)) - goto free_update_link; + goto free_update_tracing_link; client_fd = connect_to_fd(server_fd, 0); if (CHECK(client_fd < 0, "connect_to_fd", "errno %d\n", errno)) @@ -71,8 +77,10 @@ void test_socket_cookie(void) close(client_fd); close_server_fd: close(server_fd); -free_update_link: - bpf_link__destroy(update_link); +free_update_tracing_link: + bpf_link__destroy(update_tracing_link); +free_update_sockops_link: + bpf_link__destroy(update_sockops_link); free_set_link: bpf_link__destroy(set_link); close_cgroup_fd: diff --git a/tools/testing/selftests/bpf/progs/socket_cookie_prog.c b/tools/testing/selftests/bpf/progs/socket_cookie_prog.c index 81e84be6f86d..1f770b732cb1 100644 --- a/tools/testing/selftests/bpf/progs/socket_cookie_prog.c +++ b/tools/testing/selftests/bpf/progs/socket_cookie_prog.c @@ -1,11 +1,13 @@ // SPDX-License-Identifier: GPL-2.0 // Copyright (c) 2018 Facebook -#include -#include +#include "vmlinux.h" #include #include +#include + +#define AF_INET6 10 struct socket_cookie { __u64 cookie_key; @@ -19,6 +21,14 @@ struct { __type(value, struct socket_cookie); } socket_cookies SEC(".maps"); +/* + * These three programs get executed in a row on connect() syscalls. The + * userspace side of the test creates a client socket, issues a connect() on it + * and then checks that the local storage associated with this socket has: + * cookie_value == local_port << 8 | 0xFF + * The different parts of this cookie_value are appended by those hooks if they + * all agree on the output of bpf_get_socket_cookie(). + */ SEC("cgroup/connect6") int set_cookie(struct bpf_sock_addr *ctx) { @@ -32,14 +42,14 @@ int set_cookie(struct bpf_sock_addr *ctx) if (!p) return 1; - p->cookie_value = 0xFF; + p->cookie_value = 0xF; p->cookie_key = bpf_get_socket_cookie(ctx); return 1; } SEC("sockops") -int update_cookie(struct bpf_sock_ops *ctx) +int update_cookie_sockops(struct bpf_sock_ops *ctx) { struct bpf_sock *sk; struct socket_cookie *p; @@ -60,9 +70,30 @@ int update_cookie(struct bpf_sock_ops *ctx) if (p->cookie_key != bpf_get_socket_cookie(ctx)) return 1; - p->cookie_value = (ctx->local_port << 8) | p->cookie_value; + p->cookie_value |= (ctx->local_port << 8); return 1; } +SEC("fexit/inet_stream_connect") +int BPF_PROG(update_cookie_tracing, struct socket *sock, +struct sockaddr *uaddr, int addr_len, int flags) +{ + struct socket_cookie *p; +