* [PATCH bpf-next v7 2/5] bpf: Expose bpf_get_socket_cookie to tracing programs
2021-02-10 11:14 [PATCH bpf-next v7 1/5] bpf: Be less specific about socket cookies guarantees Florent Revest
@ 2021-02-10 11:14 ` Florent Revest
2021-02-10 19:51 ` Andrii Nakryiko
2021-02-12 2:28 ` Alexei Starovoitov
2021-02-10 11:14 ` [PATCH bpf-next v7 3/5] selftests/bpf: Integrate the socket_cookie test to test_progs Florent Revest
` (3 subsequent siblings)
4 siblings, 2 replies; 9+ messages in thread
From: Florent Revest @ 2021-02-10 11:14 UTC (permalink / raw)
To: bpf
Cc: ast, daniel, andrii, kpsingh, revest, jackmanb, linux-kernel,
Florent Revest, KP Singh
This needs a new helper that:
- can work in a sleepable context (using sock_gen_cookie)
- takes a struct sock pointer and checks that it's not NULL
Signed-off-by: Florent Revest <revest@chromium.org>
Acked-by: KP Singh <kpsingh@kernel.org>
---
include/linux/bpf.h | 1 +
include/uapi/linux/bpf.h | 8 ++++++++
kernel/trace/bpf_trace.c | 2 ++
net/core/filter.c | 12 ++++++++++++
tools/include/uapi/linux/bpf.h | 8 ++++++++
5 files changed, 31 insertions(+)
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 321966fc35db..d212ae7d9731 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1888,6 +1888,7 @@ extern const struct bpf_func_proto bpf_per_cpu_ptr_proto;
extern const struct bpf_func_proto bpf_this_cpu_ptr_proto;
extern const struct bpf_func_proto bpf_ktime_get_coarse_ns_proto;
extern const struct bpf_func_proto bpf_sock_from_file_proto;
+extern const struct bpf_func_proto bpf_get_socket_ptr_cookie_proto;
const struct bpf_func_proto *bpf_tracing_func_proto(
enum bpf_func_id func_id, const struct bpf_prog *prog);
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 0b735c2729b2..a8d9ad543300 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -1673,6 +1673,14 @@ union bpf_attr {
* Return
* A 8-byte long unique number.
*
+ * u64 bpf_get_socket_cookie(struct sock *sk)
+ * Description
+ * Equivalent to **bpf_get_socket_cookie**\ () helper that accepts
+ * *sk*, but gets socket from a BTF **struct sock**. This helper
+ * also works for sleepable programs.
+ * Return
+ * A 8-byte long unique number or 0 if *sk* is NULL.
+ *
* u32 bpf_get_socket_uid(struct sk_buff *skb)
* Return
* The owner UID of the socket associated to *skb*. If the socket
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 6c0018abe68a..845b2168e006 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -1760,6 +1760,8 @@ tracing_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
return &bpf_sk_storage_delete_tracing_proto;
case BPF_FUNC_sock_from_file:
return &bpf_sock_from_file_proto;
+ case BPF_FUNC_get_socket_cookie:
+ return &bpf_get_socket_ptr_cookie_proto;
#endif
case BPF_FUNC_seq_printf:
return prog->expected_attach_type == BPF_TRACE_ITER ?
diff --git a/net/core/filter.c b/net/core/filter.c
index e15d4741719a..57aaed478362 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -4631,6 +4631,18 @@ static const struct bpf_func_proto bpf_get_socket_cookie_sock_proto = {
.arg1_type = ARG_PTR_TO_CTX,
};
+BPF_CALL_1(bpf_get_socket_ptr_cookie, struct sock *, sk)
+{
+ return sk ? sock_gen_cookie(sk) : 0;
+}
+
+const struct bpf_func_proto bpf_get_socket_ptr_cookie_proto = {
+ .func = bpf_get_socket_ptr_cookie,
+ .gpl_only = false,
+ .ret_type = RET_INTEGER,
+ .arg1_type = ARG_PTR_TO_BTF_ID_SOCK_COMMON,
+};
+
BPF_CALL_1(bpf_get_socket_cookie_sock_ops, struct bpf_sock_ops_kern *, ctx)
{
return __sock_gen_cookie(ctx->sk);
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 0b735c2729b2..a8d9ad543300 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -1673,6 +1673,14 @@ union bpf_attr {
* Return
* A 8-byte long unique number.
*
+ * u64 bpf_get_socket_cookie(struct sock *sk)
+ * Description
+ * Equivalent to **bpf_get_socket_cookie**\ () helper that accepts
+ * *sk*, but gets socket from a BTF **struct sock**. This helper
+ * also works for sleepable programs.
+ * Return
+ * A 8-byte long unique number or 0 if *sk* is NULL.
+ *
* u32 bpf_get_socket_uid(struct sk_buff *skb)
* Return
* The owner UID of the socket associated to *skb*. If the socket
--
2.30.0.478.g8a0d178c01-goog
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH bpf-next v7 2/5] bpf: Expose bpf_get_socket_cookie to tracing programs
2021-02-10 11:14 ` [PATCH bpf-next v7 2/5] bpf: Expose bpf_get_socket_cookie to tracing programs Florent Revest
@ 2021-02-10 19:51 ` Andrii Nakryiko
2021-02-10 19:55 ` Florent Revest
2021-02-12 2:28 ` Alexei Starovoitov
1 sibling, 1 reply; 9+ messages in thread
From: Andrii Nakryiko @ 2021-02-10 19:51 UTC (permalink / raw)
To: Florent Revest
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
KP Singh, Florent Revest, Brendan Jackman, open list, KP Singh
On Wed, Feb 10, 2021 at 3:14 AM Florent Revest <revest@chromium.org> wrote:
>
> This needs a new helper that:
> - can work in a sleepable context (using sock_gen_cookie)
> - takes a struct sock pointer and checks that it's not NULL
>
> Signed-off-by: Florent Revest <revest@chromium.org>
> Acked-by: KP Singh <kpsingh@kernel.org>
> ---
It's customary to send cover letter with patch sets of 2 or more
related patches. It's a good place to explain the motivation of a
patch set. And a good place to ack all patches in one go ;)
Acked-by: Andrii Nakryiko <andrii@kernel.org>
> include/linux/bpf.h | 1 +
> include/uapi/linux/bpf.h | 8 ++++++++
> kernel/trace/bpf_trace.c | 2 ++
> net/core/filter.c | 12 ++++++++++++
> tools/include/uapi/linux/bpf.h | 8 ++++++++
> 5 files changed, 31 insertions(+)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 321966fc35db..d212ae7d9731 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -1888,6 +1888,7 @@ extern const struct bpf_func_proto bpf_per_cpu_ptr_proto;
> extern const struct bpf_func_proto bpf_this_cpu_ptr_proto;
> extern const struct bpf_func_proto bpf_ktime_get_coarse_ns_proto;
> extern const struct bpf_func_proto bpf_sock_from_file_proto;
> +extern const struct bpf_func_proto bpf_get_socket_ptr_cookie_proto;
>
> const struct bpf_func_proto *bpf_tracing_func_proto(
> enum bpf_func_id func_id, const struct bpf_prog *prog);
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 0b735c2729b2..a8d9ad543300 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -1673,6 +1673,14 @@ union bpf_attr {
> * Return
> * A 8-byte long unique number.
> *
> + * u64 bpf_get_socket_cookie(struct sock *sk)
> + * Description
> + * Equivalent to **bpf_get_socket_cookie**\ () helper that accepts
> + * *sk*, but gets socket from a BTF **struct sock**. This helper
> + * also works for sleepable programs.
> + * Return
> + * A 8-byte long unique number or 0 if *sk* is NULL.
> + *
> * u32 bpf_get_socket_uid(struct sk_buff *skb)
> * Return
> * The owner UID of the socket associated to *skb*. If the socket
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index 6c0018abe68a..845b2168e006 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -1760,6 +1760,8 @@ tracing_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
> return &bpf_sk_storage_delete_tracing_proto;
> case BPF_FUNC_sock_from_file:
> return &bpf_sock_from_file_proto;
> + case BPF_FUNC_get_socket_cookie:
> + return &bpf_get_socket_ptr_cookie_proto;
> #endif
> case BPF_FUNC_seq_printf:
> return prog->expected_attach_type == BPF_TRACE_ITER ?
> diff --git a/net/core/filter.c b/net/core/filter.c
> index e15d4741719a..57aaed478362 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -4631,6 +4631,18 @@ static const struct bpf_func_proto bpf_get_socket_cookie_sock_proto = {
> .arg1_type = ARG_PTR_TO_CTX,
> };
>
> +BPF_CALL_1(bpf_get_socket_ptr_cookie, struct sock *, sk)
> +{
> + return sk ? sock_gen_cookie(sk) : 0;
> +}
> +
> +const struct bpf_func_proto bpf_get_socket_ptr_cookie_proto = {
> + .func = bpf_get_socket_ptr_cookie,
> + .gpl_only = false,
> + .ret_type = RET_INTEGER,
> + .arg1_type = ARG_PTR_TO_BTF_ID_SOCK_COMMON,
> +};
> +
> BPF_CALL_1(bpf_get_socket_cookie_sock_ops, struct bpf_sock_ops_kern *, ctx)
> {
> return __sock_gen_cookie(ctx->sk);
> diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
> index 0b735c2729b2..a8d9ad543300 100644
> --- a/tools/include/uapi/linux/bpf.h
> +++ b/tools/include/uapi/linux/bpf.h
> @@ -1673,6 +1673,14 @@ union bpf_attr {
> * Return
> * A 8-byte long unique number.
> *
> + * u64 bpf_get_socket_cookie(struct sock *sk)
> + * Description
> + * Equivalent to **bpf_get_socket_cookie**\ () helper that accepts
> + * *sk*, but gets socket from a BTF **struct sock**. This helper
> + * also works for sleepable programs.
> + * Return
> + * A 8-byte long unique number or 0 if *sk* is NULL.
> + *
> * u32 bpf_get_socket_uid(struct sk_buff *skb)
> * Return
> * The owner UID of the socket associated to *skb*. If the socket
> --
> 2.30.0.478.g8a0d178c01-goog
>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH bpf-next v7 2/5] bpf: Expose bpf_get_socket_cookie to tracing programs
2021-02-10 19:51 ` Andrii Nakryiko
@ 2021-02-10 19:55 ` Florent Revest
0 siblings, 0 replies; 9+ messages in thread
From: Florent Revest @ 2021-02-10 19:55 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
KP Singh, Florent Revest, Brendan Jackman, open list, KP Singh
On Wed, Feb 10, 2021 at 8:52 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Wed, Feb 10, 2021 at 3:14 AM Florent Revest <revest@chromium.org> wrote:
> >
> > This needs a new helper that:
> > - can work in a sleepable context (using sock_gen_cookie)
> > - takes a struct sock pointer and checks that it's not NULL
> >
> > Signed-off-by: Florent Revest <revest@chromium.org>
> > Acked-by: KP Singh <kpsingh@kernel.org>
> > ---
>
> It's customary to send cover letter with patch sets of 2 or more
> related patches. It's a good place to explain the motivation of a
> patch set. And a good place to ack all patches in one go ;)
You're right :) I first (naively!) thought it would be a short series
but it grew bigger than I originally thought. I will make sure I do in
the future. ;)
> Acked-by: Andrii Nakryiko <andrii@kernel.org>
>
>
> > include/linux/bpf.h | 1 +
> > include/uapi/linux/bpf.h | 8 ++++++++
> > kernel/trace/bpf_trace.c | 2 ++
> > net/core/filter.c | 12 ++++++++++++
> > tools/include/uapi/linux/bpf.h | 8 ++++++++
> > 5 files changed, 31 insertions(+)
> >
> > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > index 321966fc35db..d212ae7d9731 100644
> > --- a/include/linux/bpf.h
> > +++ b/include/linux/bpf.h
> > @@ -1888,6 +1888,7 @@ extern const struct bpf_func_proto bpf_per_cpu_ptr_proto;
> > extern const struct bpf_func_proto bpf_this_cpu_ptr_proto;
> > extern const struct bpf_func_proto bpf_ktime_get_coarse_ns_proto;
> > extern const struct bpf_func_proto bpf_sock_from_file_proto;
> > +extern const struct bpf_func_proto bpf_get_socket_ptr_cookie_proto;
> >
> > const struct bpf_func_proto *bpf_tracing_func_proto(
> > enum bpf_func_id func_id, const struct bpf_prog *prog);
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index 0b735c2729b2..a8d9ad543300 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -1673,6 +1673,14 @@ union bpf_attr {
> > * Return
> > * A 8-byte long unique number.
> > *
> > + * u64 bpf_get_socket_cookie(struct sock *sk)
> > + * Description
> > + * Equivalent to **bpf_get_socket_cookie**\ () helper that accepts
> > + * *sk*, but gets socket from a BTF **struct sock**. This helper
> > + * also works for sleepable programs.
> > + * Return
> > + * A 8-byte long unique number or 0 if *sk* is NULL.
> > + *
> > * u32 bpf_get_socket_uid(struct sk_buff *skb)
> > * Return
> > * The owner UID of the socket associated to *skb*. If the socket
> > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> > index 6c0018abe68a..845b2168e006 100644
> > --- a/kernel/trace/bpf_trace.c
> > +++ b/kernel/trace/bpf_trace.c
> > @@ -1760,6 +1760,8 @@ tracing_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
> > return &bpf_sk_storage_delete_tracing_proto;
> > case BPF_FUNC_sock_from_file:
> > return &bpf_sock_from_file_proto;
> > + case BPF_FUNC_get_socket_cookie:
> > + return &bpf_get_socket_ptr_cookie_proto;
> > #endif
> > case BPF_FUNC_seq_printf:
> > return prog->expected_attach_type == BPF_TRACE_ITER ?
> > diff --git a/net/core/filter.c b/net/core/filter.c
> > index e15d4741719a..57aaed478362 100644
> > --- a/net/core/filter.c
> > +++ b/net/core/filter.c
> > @@ -4631,6 +4631,18 @@ static const struct bpf_func_proto bpf_get_socket_cookie_sock_proto = {
> > .arg1_type = ARG_PTR_TO_CTX,
> > };
> >
> > +BPF_CALL_1(bpf_get_socket_ptr_cookie, struct sock *, sk)
> > +{
> > + return sk ? sock_gen_cookie(sk) : 0;
> > +}
> > +
> > +const struct bpf_func_proto bpf_get_socket_ptr_cookie_proto = {
> > + .func = bpf_get_socket_ptr_cookie,
> > + .gpl_only = false,
> > + .ret_type = RET_INTEGER,
> > + .arg1_type = ARG_PTR_TO_BTF_ID_SOCK_COMMON,
> > +};
> > +
> > BPF_CALL_1(bpf_get_socket_cookie_sock_ops, struct bpf_sock_ops_kern *, ctx)
> > {
> > return __sock_gen_cookie(ctx->sk);
> > diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
> > index 0b735c2729b2..a8d9ad543300 100644
> > --- a/tools/include/uapi/linux/bpf.h
> > +++ b/tools/include/uapi/linux/bpf.h
> > @@ -1673,6 +1673,14 @@ union bpf_attr {
> > * Return
> > * A 8-byte long unique number.
> > *
> > + * u64 bpf_get_socket_cookie(struct sock *sk)
> > + * Description
> > + * Equivalent to **bpf_get_socket_cookie**\ () helper that accepts
> > + * *sk*, but gets socket from a BTF **struct sock**. This helper
> > + * also works for sleepable programs.
> > + * Return
> > + * A 8-byte long unique number or 0 if *sk* is NULL.
> > + *
> > * u32 bpf_get_socket_uid(struct sk_buff *skb)
> > * Return
> > * The owner UID of the socket associated to *skb*. If the socket
> > --
> > 2.30.0.478.g8a0d178c01-goog
> >
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH bpf-next v7 2/5] bpf: Expose bpf_get_socket_cookie to tracing programs
2021-02-10 11:14 ` [PATCH bpf-next v7 2/5] bpf: Expose bpf_get_socket_cookie to tracing programs Florent Revest
2021-02-10 19:51 ` Andrii Nakryiko
@ 2021-02-12 2:28 ` Alexei Starovoitov
1 sibling, 0 replies; 9+ messages in thread
From: Alexei Starovoitov @ 2021-02-12 2:28 UTC (permalink / raw)
To: Florent Revest
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
KP Singh, Florent Revest, Brendan Jackman, LKML, KP Singh
On Wed, Feb 10, 2021 at 3:14 AM Florent Revest <revest@chromium.org> wrote:
>
> +BPF_CALL_1(bpf_get_socket_ptr_cookie, struct sock *, sk)
> +{
> + return sk ? sock_gen_cookie(sk) : 0;
> +}
> +
> +const struct bpf_func_proto bpf_get_socket_ptr_cookie_proto = {
> + .func = bpf_get_socket_ptr_cookie,
> + .gpl_only = false,
> + .ret_type = RET_INTEGER,
> + .arg1_type = ARG_PTR_TO_BTF_ID_SOCK_COMMON,
> +};
As Daniel pointed out there is an sk_destruct issue here, but I don't
think it's fair
to penalize this set and future similar patches. They don't make things worse.
The issue has been there for some time due to sk_storage in tracing and
other helpers. We need to come up with a holistic approach to solve it.
I suspect allow/deny lists will certainly make it better, but won't
really address it,
and will be fragile over long term.
I think tracing would need to be integrated with bpf_lsm and start relying
on security_*_free callbacks to cover this last 1%.
I think that would be a great topic for the next bpf office hours on Feb 25.
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH bpf-next v7 3/5] selftests/bpf: Integrate the socket_cookie test to test_progs
2021-02-10 11:14 [PATCH bpf-next v7 1/5] bpf: Be less specific about socket cookies guarantees Florent Revest
2021-02-10 11:14 ` [PATCH bpf-next v7 2/5] bpf: Expose bpf_get_socket_cookie to tracing programs Florent Revest
@ 2021-02-10 11:14 ` Florent Revest
2021-02-10 11:14 ` [PATCH bpf-next v7 4/5] selftests/bpf: Use vmlinux.h in socket_cookie_prog.c Florent Revest
` (2 subsequent siblings)
4 siblings, 0 replies; 9+ messages in thread
From: Florent Revest @ 2021-02-10 11:14 UTC (permalink / raw)
To: bpf
Cc: ast, daniel, andrii, kpsingh, revest, jackmanb, linux-kernel,
Florent Revest, KP Singh
Currently, the selftest for the BPF socket_cookie helpers is built and
run independently from test_progs. It's easy to forget and hard to
maintain.
This patch moves the socket cookies test into prog_tests/ and vastly
simplifies its logic by:
- rewriting the loading code with BPF skeletons
- rewriting the server/client code with network helpers
- rewriting the cgroup code with test__join_cgroup
- rewriting the error handling code with CHECKs
Signed-off-by: Florent Revest <revest@chromium.org>
Acked-by: KP Singh <kpsingh@kernel.org>
Acked-by: Andrii Nakryiko <andrii@kernel.org>
---
tools/testing/selftests/bpf/.gitignore | 1 -
tools/testing/selftests/bpf/Makefile | 3 +-
.../selftests/bpf/prog_tests/socket_cookie.c | 71 ++++++
.../selftests/bpf/progs/socket_cookie_prog.c | 2 -
.../selftests/bpf/test_socket_cookie.c | 208 ------------------
5 files changed, 72 insertions(+), 213 deletions(-)
create mode 100644 tools/testing/selftests/bpf/prog_tests/socket_cookie.c
delete mode 100644 tools/testing/selftests/bpf/test_socket_cookie.c
diff --git a/tools/testing/selftests/bpf/.gitignore b/tools/testing/selftests/bpf/.gitignore
index 9abca0616ec0..c0c48fdb9ac1 100644
--- a/tools/testing/selftests/bpf/.gitignore
+++ b/tools/testing/selftests/bpf/.gitignore
@@ -17,7 +17,6 @@ test_sockmap
test_lirc_mode2_user
get_cgroup_id_user
test_skb_cgroup_id_user
-test_socket_cookie
test_cgroup_storage
test_flow_dissector
flow_dissector_load
diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index f0674d406f40..044bfdcf5b74 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -31,7 +31,7 @@ LDLIBS += -lcap -lelf -lz -lrt -lpthread
# Order correspond to 'make run_tests' order
TEST_GEN_PROGS = test_verifier test_tag test_maps test_lru_map test_lpm_map test_progs \
test_verifier_log test_dev_cgroup \
- test_sock test_sockmap get_cgroup_id_user test_socket_cookie \
+ test_sock test_sockmap get_cgroup_id_user \
test_cgroup_storage \
test_netcnt test_tcpnotify_user test_sysctl \
test_progs-no_alu32
@@ -185,7 +185,6 @@ $(OUTPUT)/test_dev_cgroup: cgroup_helpers.c
$(OUTPUT)/test_skb_cgroup_id_user: cgroup_helpers.c
$(OUTPUT)/test_sock: cgroup_helpers.c
$(OUTPUT)/test_sock_addr: cgroup_helpers.c
-$(OUTPUT)/test_socket_cookie: cgroup_helpers.c
$(OUTPUT)/test_sockmap: cgroup_helpers.c
$(OUTPUT)/test_tcpnotify_user: cgroup_helpers.c trace_helpers.c
$(OUTPUT)/get_cgroup_id_user: cgroup_helpers.c
diff --git a/tools/testing/selftests/bpf/prog_tests/socket_cookie.c b/tools/testing/selftests/bpf/prog_tests/socket_cookie.c
new file mode 100644
index 000000000000..e12a31d3752c
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/socket_cookie.c
@@ -0,0 +1,71 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2020 Google LLC.
+// Copyright (c) 2018 Facebook
+
+#include <test_progs.h>
+#include "socket_cookie_prog.skel.h"
+#include "network_helpers.h"
+
+static int duration;
+
+struct socket_cookie {
+ __u64 cookie_key;
+ __u32 cookie_value;
+};
+
+void test_socket_cookie(void)
+{
+ int server_fd = 0, client_fd = 0, cgroup_fd = 0, err = 0;
+ socklen_t addr_len = sizeof(struct sockaddr_in6);
+ struct socket_cookie_prog *skel;
+ __u32 cookie_expected_value;
+ struct sockaddr_in6 addr;
+ struct socket_cookie val;
+
+ skel = socket_cookie_prog__open_and_load();
+ if (!ASSERT_OK_PTR(skel, "skel_open"))
+ return;
+
+ cgroup_fd = test__join_cgroup("/socket_cookie");
+ if (CHECK(cgroup_fd < 0, "join_cgroup", "cgroup creation failed\n"))
+ goto out;
+
+ skel->links.set_cookie = bpf_program__attach_cgroup(
+ skel->progs.set_cookie, cgroup_fd);
+ if (!ASSERT_OK_PTR(skel->links.set_cookie, "prog_attach"))
+ goto close_cgroup_fd;
+
+ skel->links.update_cookie = bpf_program__attach_cgroup(
+ skel->progs.update_cookie, cgroup_fd);
+ if (!ASSERT_OK_PTR(skel->links.update_cookie, "prog_attach"))
+ goto close_cgroup_fd;
+
+ server_fd = start_server(AF_INET6, SOCK_STREAM, "::1", 0, 0);
+ if (CHECK(server_fd < 0, "start_server", "errno %d\n", errno))
+ goto close_cgroup_fd;
+
+ client_fd = connect_to_fd(server_fd, 0);
+ if (CHECK(client_fd < 0, "connect_to_fd", "errno %d\n", errno))
+ goto close_server_fd;
+
+ err = bpf_map_lookup_elem(bpf_map__fd(skel->maps.socket_cookies),
+ &client_fd, &val);
+ if (!ASSERT_OK(err, "map_lookup(socket_cookies)"))
+ goto close_client_fd;
+
+ err = getsockname(client_fd, (struct sockaddr *)&addr, &addr_len);
+ if (!ASSERT_OK(err, "getsockname"))
+ goto close_client_fd;
+
+ cookie_expected_value = (ntohs(addr.sin6_port) << 8) | 0xFF;
+ ASSERT_EQ(val.cookie_value, cookie_expected_value, "cookie_value");
+
+close_client_fd:
+ close(client_fd);
+close_server_fd:
+ close(server_fd);
+close_cgroup_fd:
+ close(cgroup_fd);
+out:
+ socket_cookie_prog__destroy(skel);
+}
diff --git a/tools/testing/selftests/bpf/progs/socket_cookie_prog.c b/tools/testing/selftests/bpf/progs/socket_cookie_prog.c
index 0cb5656a22b0..81e84be6f86d 100644
--- a/tools/testing/selftests/bpf/progs/socket_cookie_prog.c
+++ b/tools/testing/selftests/bpf/progs/socket_cookie_prog.c
@@ -65,6 +65,4 @@ int update_cookie(struct bpf_sock_ops *ctx)
return 1;
}
-int _version SEC("version") = 1;
-
char _license[] SEC("license") = "GPL";
diff --git a/tools/testing/selftests/bpf/test_socket_cookie.c b/tools/testing/selftests/bpf/test_socket_cookie.c
deleted file mode 100644
index ca7ca87e91aa..000000000000
--- a/tools/testing/selftests/bpf/test_socket_cookie.c
+++ /dev/null
@@ -1,208 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-// Copyright (c) 2018 Facebook
-
-#include <string.h>
-#include <unistd.h>
-
-#include <arpa/inet.h>
-#include <netinet/in.h>
-#include <sys/types.h>
-#include <sys/socket.h>
-
-#include <bpf/bpf.h>
-#include <bpf/libbpf.h>
-
-#include "bpf_rlimit.h"
-#include "cgroup_helpers.h"
-
-#define CG_PATH "/foo"
-#define SOCKET_COOKIE_PROG "./socket_cookie_prog.o"
-
-struct socket_cookie {
- __u64 cookie_key;
- __u32 cookie_value;
-};
-
-static int start_server(void)
-{
- struct sockaddr_in6 addr;
- int fd;
-
- fd = socket(AF_INET6, SOCK_STREAM, 0);
- if (fd == -1) {
- log_err("Failed to create server socket");
- goto out;
- }
-
- memset(&addr, 0, sizeof(addr));
- addr.sin6_family = AF_INET6;
- addr.sin6_addr = in6addr_loopback;
- addr.sin6_port = 0;
-
- if (bind(fd, (const struct sockaddr *)&addr, sizeof(addr)) == -1) {
- log_err("Failed to bind server socket");
- goto close_out;
- }
-
- if (listen(fd, 128) == -1) {
- log_err("Failed to listen on server socket");
- goto close_out;
- }
-
- goto out;
-
-close_out:
- close(fd);
- fd = -1;
-out:
- return fd;
-}
-
-static int connect_to_server(int server_fd)
-{
- struct sockaddr_storage addr;
- socklen_t len = sizeof(addr);
- int fd;
-
- fd = socket(AF_INET6, SOCK_STREAM, 0);
- if (fd == -1) {
- log_err("Failed to create client socket");
- goto out;
- }
-
- if (getsockname(server_fd, (struct sockaddr *)&addr, &len)) {
- log_err("Failed to get server addr");
- goto close_out;
- }
-
- if (connect(fd, (const struct sockaddr *)&addr, len) == -1) {
- log_err("Fail to connect to server");
- goto close_out;
- }
-
- goto out;
-
-close_out:
- close(fd);
- fd = -1;
-out:
- return fd;
-}
-
-static int validate_map(struct bpf_map *map, int client_fd)
-{
- __u32 cookie_expected_value;
- struct sockaddr_in6 addr;
- socklen_t len = sizeof(addr);
- struct socket_cookie val;
- int err = 0;
- int map_fd;
-
- if (!map) {
- log_err("Map not found in BPF object");
- goto err;
- }
-
- map_fd = bpf_map__fd(map);
-
- err = bpf_map_lookup_elem(map_fd, &client_fd, &val);
-
- err = getsockname(client_fd, (struct sockaddr *)&addr, &len);
- if (err) {
- log_err("Can't get client local addr");
- goto out;
- }
-
- cookie_expected_value = (ntohs(addr.sin6_port) << 8) | 0xFF;
- if (val.cookie_value != cookie_expected_value) {
- log_err("Unexpected value in map: %x != %x", val.cookie_value,
- cookie_expected_value);
- goto err;
- }
-
- goto out;
-err:
- err = -1;
-out:
- return err;
-}
-
-static int run_test(int cgfd)
-{
- enum bpf_attach_type attach_type;
- struct bpf_prog_load_attr attr;
- struct bpf_program *prog;
- struct bpf_object *pobj;
- const char *prog_name;
- int server_fd = -1;
- int client_fd = -1;
- int prog_fd = -1;
- int err = 0;
-
- memset(&attr, 0, sizeof(attr));
- attr.file = SOCKET_COOKIE_PROG;
- attr.prog_type = BPF_PROG_TYPE_UNSPEC;
- attr.prog_flags = BPF_F_TEST_RND_HI32;
-
- err = bpf_prog_load_xattr(&attr, &pobj, &prog_fd);
- if (err) {
- log_err("Failed to load %s", attr.file);
- goto out;
- }
-
- bpf_object__for_each_program(prog, pobj) {
- prog_name = bpf_program__section_name(prog);
-
- if (libbpf_attach_type_by_name(prog_name, &attach_type))
- goto err;
-
- err = bpf_prog_attach(bpf_program__fd(prog), cgfd, attach_type,
- BPF_F_ALLOW_OVERRIDE);
- if (err) {
- log_err("Failed to attach prog %s", prog_name);
- goto out;
- }
- }
-
- server_fd = start_server();
- if (server_fd == -1)
- goto err;
-
- client_fd = connect_to_server(server_fd);
- if (client_fd == -1)
- goto err;
-
- if (validate_map(bpf_map__next(NULL, pobj), client_fd))
- goto err;
-
- goto out;
-err:
- err = -1;
-out:
- close(client_fd);
- close(server_fd);
- bpf_object__close(pobj);
- printf("%s\n", err ? "FAILED" : "PASSED");
- return err;
-}
-
-int main(int argc, char **argv)
-{
- int cgfd = -1;
- int err = 0;
-
- cgfd = cgroup_setup_and_join(CG_PATH);
- if (cgfd < 0)
- goto err;
-
- if (run_test(cgfd))
- goto err;
-
- goto out;
-err:
- err = -1;
-out:
- close(cgfd);
- cleanup_cgroup_environment();
- return err;
-}
--
2.30.0.478.g8a0d178c01-goog
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH bpf-next v7 4/5] selftests/bpf: Use vmlinux.h in socket_cookie_prog.c
2021-02-10 11:14 [PATCH bpf-next v7 1/5] bpf: Be less specific about socket cookies guarantees Florent Revest
2021-02-10 11:14 ` [PATCH bpf-next v7 2/5] bpf: Expose bpf_get_socket_cookie to tracing programs Florent Revest
2021-02-10 11:14 ` [PATCH bpf-next v7 3/5] selftests/bpf: Integrate the socket_cookie test to test_progs Florent Revest
@ 2021-02-10 11:14 ` Florent Revest
2021-02-10 11:14 ` [PATCH bpf-next v7 5/5] selftests/bpf: Add a selftest for the tracing bpf_get_socket_cookie Florent Revest
2021-02-12 2:20 ` [PATCH bpf-next v7 1/5] bpf: Be less specific about socket cookies guarantees patchwork-bot+netdevbpf
4 siblings, 0 replies; 9+ messages in thread
From: Florent Revest @ 2021-02-10 11:14 UTC (permalink / raw)
To: bpf
Cc: ast, daniel, andrii, kpsingh, revest, jackmanb, linux-kernel,
Florent Revest, KP Singh
When migrating from the bpf.h's to the vmlinux.h's definition of struct
bps_sock, an interesting LLVM behavior happened. LLVM started producing
two fetches of ctx->sk in the sockops program this means that the
verifier could not keep track of the NULL-check on ctx->sk. Therefore,
we need to extract ctx->sk in a variable before checking and
dereferencing it.
Acked-by: KP Singh <kpsingh@kernel.org>
Acked-by: Andrii Nakryiko <andrii@kernel.org>
Signed-off-by: Florent Revest <revest@chromium.org>
---
.../testing/selftests/bpf/progs/socket_cookie_prog.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/tools/testing/selftests/bpf/progs/socket_cookie_prog.c b/tools/testing/selftests/bpf/progs/socket_cookie_prog.c
index 81e84be6f86d..fbd5eaf39720 100644
--- a/tools/testing/selftests/bpf/progs/socket_cookie_prog.c
+++ b/tools/testing/selftests/bpf/progs/socket_cookie_prog.c
@@ -1,12 +1,13 @@
// SPDX-License-Identifier: GPL-2.0
// Copyright (c) 2018 Facebook
-#include <linux/bpf.h>
-#include <sys/socket.h>
+#include "vmlinux.h"
#include <bpf/bpf_helpers.h>
#include <bpf/bpf_endian.h>
+#define AF_INET6 10
+
struct socket_cookie {
__u64 cookie_key;
__u32 cookie_value;
@@ -41,7 +42,7 @@ int set_cookie(struct bpf_sock_addr *ctx)
SEC("sockops")
int update_cookie(struct bpf_sock_ops *ctx)
{
- struct bpf_sock *sk;
+ struct bpf_sock *sk = ctx->sk;
struct socket_cookie *p;
if (ctx->family != AF_INET6)
@@ -50,10 +51,10 @@ int update_cookie(struct bpf_sock_ops *ctx)
if (ctx->op != BPF_SOCK_OPS_TCP_CONNECT_CB)
return 1;
- if (!ctx->sk)
+ if (!sk)
return 1;
- p = bpf_sk_storage_get(&socket_cookies, ctx->sk, 0, 0);
+ p = bpf_sk_storage_get(&socket_cookies, sk, 0, 0);
if (!p)
return 1;
--
2.30.0.478.g8a0d178c01-goog
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH bpf-next v7 5/5] selftests/bpf: Add a selftest for the tracing bpf_get_socket_cookie
2021-02-10 11:14 [PATCH bpf-next v7 1/5] bpf: Be less specific about socket cookies guarantees Florent Revest
` (2 preceding siblings ...)
2021-02-10 11:14 ` [PATCH bpf-next v7 4/5] selftests/bpf: Use vmlinux.h in socket_cookie_prog.c Florent Revest
@ 2021-02-10 11:14 ` Florent Revest
2021-02-12 2:20 ` [PATCH bpf-next v7 1/5] bpf: Be less specific about socket cookies guarantees patchwork-bot+netdevbpf
4 siblings, 0 replies; 9+ messages in thread
From: Florent Revest @ 2021-02-10 11:14 UTC (permalink / raw)
To: bpf
Cc: ast, daniel, andrii, kpsingh, revest, jackmanb, linux-kernel,
Florent Revest, KP Singh
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.
Instead of having an update_cookie sockops program tag a socket local
storage with 0xFF, this uses both an update_cookie_sockops program and
an update_cookie_tracing program which succesively tag the socket with
0x0F and then 0xF0.
Signed-off-by: Florent Revest <revest@chromium.org>
Acked-by: KP Singh <kpsingh@kernel.org>
---
.../selftests/bpf/prog_tests/socket_cookie.c | 11 ++++--
.../selftests/bpf/progs/socket_cookie_prog.c | 36 +++++++++++++++++--
2 files changed, 41 insertions(+), 6 deletions(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/socket_cookie.c b/tools/testing/selftests/bpf/prog_tests/socket_cookie.c
index e12a31d3752c..232db28dde18 100644
--- a/tools/testing/selftests/bpf/prog_tests/socket_cookie.c
+++ b/tools/testing/selftests/bpf/prog_tests/socket_cookie.c
@@ -35,9 +35,14 @@ void test_socket_cookie(void)
if (!ASSERT_OK_PTR(skel->links.set_cookie, "prog_attach"))
goto close_cgroup_fd;
- skel->links.update_cookie = bpf_program__attach_cgroup(
- skel->progs.update_cookie, cgroup_fd);
- if (!ASSERT_OK_PTR(skel->links.update_cookie, "prog_attach"))
+ skel->links.update_cookie_sockops = bpf_program__attach_cgroup(
+ skel->progs.update_cookie_sockops, cgroup_fd);
+ if (!ASSERT_OK_PTR(skel->links.update_cookie_sockops, "prog_attach"))
+ goto close_cgroup_fd;
+
+ skel->links.update_cookie_tracing = bpf_program__attach(
+ skel->progs.update_cookie_tracing);
+ if (!ASSERT_OK_PTR(skel->links.update_cookie_tracing, "prog_attach"))
goto close_cgroup_fd;
server_fd = start_server(AF_INET6, SOCK_STREAM, "::1", 0, 0);
diff --git a/tools/testing/selftests/bpf/progs/socket_cookie_prog.c b/tools/testing/selftests/bpf/progs/socket_cookie_prog.c
index fbd5eaf39720..35630a5aaf5f 100644
--- a/tools/testing/selftests/bpf/progs/socket_cookie_prog.c
+++ b/tools/testing/selftests/bpf/progs/socket_cookie_prog.c
@@ -5,6 +5,7 @@
#include <bpf/bpf_helpers.h>
#include <bpf/bpf_endian.h>
+#include <bpf/bpf_tracing.h>
#define AF_INET6 10
@@ -20,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)
{
@@ -33,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 = ctx->sk;
struct socket_cookie *p;
@@ -61,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;
+
+ if (uaddr->sa_family != AF_INET6)
+ return 0;
+
+ p = bpf_sk_storage_get(&socket_cookies, sock->sk, 0, 0);
+ if (!p)
+ return 0;
+
+ if (p->cookie_key != bpf_get_socket_cookie(sock->sk))
+ return 0;
+
+ p->cookie_value |= 0xF0;
+
+ return 0;
+}
+
char _license[] SEC("license") = "GPL";
--
2.30.0.478.g8a0d178c01-goog
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH bpf-next v7 1/5] bpf: Be less specific about socket cookies guarantees
2021-02-10 11:14 [PATCH bpf-next v7 1/5] bpf: Be less specific about socket cookies guarantees Florent Revest
` (3 preceding siblings ...)
2021-02-10 11:14 ` [PATCH bpf-next v7 5/5] selftests/bpf: Add a selftest for the tracing bpf_get_socket_cookie Florent Revest
@ 2021-02-12 2:20 ` patchwork-bot+netdevbpf
4 siblings, 0 replies; 9+ messages in thread
From: patchwork-bot+netdevbpf @ 2021-02-12 2:20 UTC (permalink / raw)
To: Florent Revest
Cc: bpf, ast, daniel, andrii, kpsingh, revest, jackmanb,
linux-kernel, kpsingh
Hello:
This series was applied to bpf/bpf-next.git (refs/heads/master):
On Wed, 10 Feb 2021 12:14:02 +0100 you wrote:
> Since "92acdc58ab11 bpf, net: Rework cookie generator as per-cpu one"
> socket cookies are not guaranteed to be non-decreasing. The
> bpf_get_socket_cookie helper descriptions are currently specifying that
> cookies are non-decreasing but we don't want users to rely on that.
>
> Reported-by: Daniel Borkmann <daniel@iogearbox.net>
> Signed-off-by: Florent Revest <revest@chromium.org>
> Acked-by: KP Singh <kpsingh@kernel.org>
>
> [...]
Here is the summary with links:
- [bpf-next,v7,1/5] bpf: Be less specific about socket cookies guarantees
https://git.kernel.org/bpf/bpf-next/c/07881ccbf40c
- [bpf-next,v7,2/5] bpf: Expose bpf_get_socket_cookie to tracing programs
https://git.kernel.org/bpf/bpf-next/c/c5dbb89fc2ac
- [bpf-next,v7,3/5] selftests/bpf: Integrate the socket_cookie test to test_progs
https://git.kernel.org/bpf/bpf-next/c/61f8c9c8f3c8
- [bpf-next,v7,4/5] selftests/bpf: Use vmlinux.h in socket_cookie_prog.c
https://git.kernel.org/bpf/bpf-next/c/6cd4dcc3fb81
- [bpf-next,v7,5/5] selftests/bpf: Add a selftest for the tracing bpf_get_socket_cookie
https://git.kernel.org/bpf/bpf-next/c/6fdd671baaf5
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] 9+ messages in thread