* [PATCH bpf-next 0/2] Do not limit cb_flags when creating child sk from listen sk
@ 2020-10-02 1:34 Martin KaFai Lau
2020-10-02 1:34 ` [PATCH bpf-next 1/2] bpf: tcp: " Martin KaFai Lau
` (3 more replies)
0 siblings, 4 replies; 5+ messages in thread
From: Martin KaFai Lau @ 2020-10-02 1:34 UTC (permalink / raw)
To: bpf
Cc: Alexei Starovoitov, Daniel Borkmann, kernel-team, netdev,
Stanislav Fomichev
This set fixes an issue that the bpf_skops_init_child() unnecessarily
limited the child sk from inheriting all bpf_sock_ops_cb_flags
of the listen sk. It also adds a test to check that.
Martin KaFai Lau (2):
bpf: tcp: Do not limit cb_flags when creating child sk from listen sk
bpf: selftest: Ensure the child sk inherited all bpf_sock_ops_cb_flags
include/net/tcp.h | 33 -------------------
net/ipv4/tcp_minisocks.c | 1 -
.../bpf/prog_tests/tcp_hdr_options.c | 12 +++++++
.../bpf/progs/test_misc_tcp_hdr_options.c | 4 +--
.../bpf/progs/test_tcp_hdr_options.c | 7 ++--
.../selftests/bpf/test_tcp_hdr_options.h | 5 +--
6 files changed, 22 insertions(+), 40 deletions(-)
--
2.24.1
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH bpf-next 1/2] bpf: tcp: Do not limit cb_flags when creating child sk from listen sk
2020-10-02 1:34 [PATCH bpf-next 0/2] Do not limit cb_flags when creating child sk from listen sk Martin KaFai Lau
@ 2020-10-02 1:34 ` Martin KaFai Lau
2020-10-02 1:34 ` [PATCH bpf-next 2/2] bpf: selftest: Ensure the child sk inherited all bpf_sock_ops_cb_flags Martin KaFai Lau
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Martin KaFai Lau @ 2020-10-02 1:34 UTC (permalink / raw)
To: bpf
Cc: Alexei Starovoitov, Daniel Borkmann, kernel-team, netdev,
Stanislav Fomichev
The commit 0813a841566f ("bpf: tcp: Allow bpf prog to write and parse TCP header option")
unnecessarily introduced bpf_skops_init_child() which limited the child
sk from inheriting all bpf_sock_ops_cb_flags of the listen sk. That
breaks existing user expectation.
This patch removes the bpf_skops_init_child() and just allows
sock_copy() to do its job to copy everything from listen sk to
the child sk.
Fixes: 0813a841566f ("bpf: tcp: Allow bpf prog to write and parse TCP header option")
Reported-by: Stanislav Fomichev <sdf@google.com>
Signed-off-by: Martin KaFai Lau <kafai@fb.com>
---
include/net/tcp.h | 33 ---------------------------------
net/ipv4/tcp_minisocks.c | 1 -
2 files changed, 34 deletions(-)
diff --git a/include/net/tcp.h b/include/net/tcp.h
index 3601dea931a6..d4ef5bf94168 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -2228,34 +2228,6 @@ int __tcp_bpf_recvmsg(struct sock *sk, struct sk_psock *psock,
#endif /* CONFIG_NET_SOCK_MSG */
#ifdef CONFIG_CGROUP_BPF
-/* Copy the listen sk's HDR_OPT_CB flags to its child.
- *
- * During 3-Way-HandShake, the synack is usually sent from
- * the listen sk with the HDR_OPT_CB flags set so that
- * bpf-prog will be called to write the BPF hdr option.
- *
- * In fastopen, the child sk is used to send synack instead
- * of the listen sk. Thus, inheriting the HDR_OPT_CB flags
- * from the listen sk gives the bpf-prog a chance to write
- * BPF hdr option in the synack pkt during fastopen.
- *
- * Both fastopen and non-fastopen child will inherit the
- * HDR_OPT_CB flags to keep the bpf-prog having a consistent
- * behavior when deciding to clear this cb flags (or not)
- * during the PASSIVE_ESTABLISHED_CB.
- *
- * In the future, other cb flags could be inherited here also.
- */
-static inline void bpf_skops_init_child(const struct sock *sk,
- struct sock *child)
-{
- tcp_sk(child)->bpf_sock_ops_cb_flags =
- tcp_sk(sk)->bpf_sock_ops_cb_flags &
- (BPF_SOCK_OPS_PARSE_ALL_HDR_OPT_CB_FLAG |
- BPF_SOCK_OPS_PARSE_UNKNOWN_HDR_OPT_CB_FLAG |
- BPF_SOCK_OPS_WRITE_HDR_OPT_CB_FLAG);
-}
-
static inline void bpf_skops_init_skb(struct bpf_sock_ops_kern *skops,
struct sk_buff *skb,
unsigned int end_offset)
@@ -2264,11 +2236,6 @@ static inline void bpf_skops_init_skb(struct bpf_sock_ops_kern *skops,
skops->skb_data_end = skb->data + end_offset;
}
#else
-static inline void bpf_skops_init_child(const struct sock *sk,
- struct sock *child)
-{
-}
-
static inline void bpf_skops_init_skb(struct bpf_sock_ops_kern *skops,
struct sk_buff *skb,
unsigned int end_offset)
diff --git a/net/ipv4/tcp_minisocks.c b/net/ipv4/tcp_minisocks.c
index 56c306e3cd2f..495dda2449fe 100644
--- a/net/ipv4/tcp_minisocks.c
+++ b/net/ipv4/tcp_minisocks.c
@@ -548,7 +548,6 @@ struct sock *tcp_create_openreq_child(const struct sock *sk,
newtp->fastopen_req = NULL;
RCU_INIT_POINTER(newtp->fastopen_rsk, NULL);
- bpf_skops_init_child(sk, newsk);
tcp_bpf_clone(sk, newsk);
__TCP_INC_STATS(sock_net(sk), TCP_MIB_PASSIVEOPENS);
--
2.24.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH bpf-next 2/2] bpf: selftest: Ensure the child sk inherited all bpf_sock_ops_cb_flags
2020-10-02 1:34 [PATCH bpf-next 0/2] Do not limit cb_flags when creating child sk from listen sk Martin KaFai Lau
2020-10-02 1:34 ` [PATCH bpf-next 1/2] bpf: tcp: " Martin KaFai Lau
@ 2020-10-02 1:34 ` Martin KaFai Lau
2020-10-02 16:03 ` [PATCH bpf-next 0/2] Do not limit cb_flags when creating child sk from listen sk sdf
2020-10-02 18:50 ` patchwork-bot+bpf
3 siblings, 0 replies; 5+ messages in thread
From: Martin KaFai Lau @ 2020-10-02 1:34 UTC (permalink / raw)
To: bpf
Cc: Alexei Starovoitov, Daniel Borkmann, kernel-team, netdev,
Stanislav Fomichev
This patch adds a test to ensure the child sk inherited everything
from the bpf_sock_ops_cb_flags of the listen sk:
1. Sets one more cb_flags (BPF_SOCK_OPS_STATE_CB_FLAG) to the listen sk
in test_tcp_hdr_options.c
2. Saves the skops->bpf_sock_ops_cb_flags when handling the newly
established passive connection
3. CHECK() it is the same as the listen sk
This also covers the fastopen case as the existing test_tcp_hdr_options.c
does.
Signed-off-by: Martin KaFai Lau <kafai@fb.com>
---
.../selftests/bpf/prog_tests/tcp_hdr_options.c | 12 ++++++++++++
.../selftests/bpf/progs/test_misc_tcp_hdr_options.c | 4 ++--
.../selftests/bpf/progs/test_tcp_hdr_options.c | 7 +++++--
tools/testing/selftests/bpf/test_tcp_hdr_options.h | 5 +++--
4 files changed, 22 insertions(+), 6 deletions(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/tcp_hdr_options.c b/tools/testing/selftests/bpf/prog_tests/tcp_hdr_options.c
index 24ba0d21b641..c86e67214a9e 100644
--- a/tools/testing/selftests/bpf/prog_tests/tcp_hdr_options.c
+++ b/tools/testing/selftests/bpf/prog_tests/tcp_hdr_options.c
@@ -264,9 +264,19 @@ static int check_error_linum(const struct sk_fds *sk_fds)
static void check_hdr_and_close_fds(struct sk_fds *sk_fds)
{
+ const __u32 expected_inherit_cb_flags =
+ BPF_SOCK_OPS_PARSE_UNKNOWN_HDR_OPT_CB_FLAG |
+ BPF_SOCK_OPS_WRITE_HDR_OPT_CB_FLAG |
+ BPF_SOCK_OPS_STATE_CB_FLAG;
+
if (sk_fds_shutdown(sk_fds))
goto check_linum;
+ if (CHECK(expected_inherit_cb_flags != skel->bss->inherit_cb_flags,
+ "Unexpected inherit_cb_flags", "0x%x != 0x%x\n",
+ skel->bss->inherit_cb_flags, expected_inherit_cb_flags))
+ goto check_linum;
+
if (check_hdr_stg(&exp_passive_hdr_stg, sk_fds->passive_fd,
"passive_hdr_stg"))
goto check_linum;
@@ -321,6 +331,8 @@ static void reset_test(void)
memset(&skel->bss->active_estab_in, 0, optsize);
memset(&skel->bss->active_fin_in, 0, optsize);
+ skel->bss->inherit_cb_flags = 0;
+
skel->data->test_kind = TCPOPT_EXP;
skel->data->test_magic = 0xeB9F;
diff --git a/tools/testing/selftests/bpf/progs/test_misc_tcp_hdr_options.c b/tools/testing/selftests/bpf/progs/test_misc_tcp_hdr_options.c
index 3a216d1d0226..72ec0178f653 100644
--- a/tools/testing/selftests/bpf/progs/test_misc_tcp_hdr_options.c
+++ b/tools/testing/selftests/bpf/progs/test_misc_tcp_hdr_options.c
@@ -304,10 +304,10 @@ int misc_estab(struct bpf_sock_ops *skops)
passive_lport_n = __bpf_htons(passive_lport_h);
bpf_setsockopt(skops, SOL_TCP, TCP_SAVE_SYN,
&true_val, sizeof(true_val));
- set_hdr_cb_flags(skops);
+ set_hdr_cb_flags(skops, 0);
break;
case BPF_SOCK_OPS_TCP_CONNECT_CB:
- set_hdr_cb_flags(skops);
+ set_hdr_cb_flags(skops, 0);
break;
case BPF_SOCK_OPS_PARSE_HDR_OPT_CB:
return handle_parse_hdr(skops);
diff --git a/tools/testing/selftests/bpf/progs/test_tcp_hdr_options.c b/tools/testing/selftests/bpf/progs/test_tcp_hdr_options.c
index 9197a23df3da..678bd0fad29e 100644
--- a/tools/testing/selftests/bpf/progs/test_tcp_hdr_options.c
+++ b/tools/testing/selftests/bpf/progs/test_tcp_hdr_options.c
@@ -21,6 +21,7 @@
__u8 test_kind = TCPOPT_EXP;
__u16 test_magic = 0xeB9F;
+__u32 inherit_cb_flags = 0;
struct bpf_test_option passive_synack_out = {};
struct bpf_test_option passive_fin_out = {};
@@ -467,6 +468,8 @@ static int handle_passive_estab(struct bpf_sock_ops *skops)
struct tcphdr *th;
int err;
+ inherit_cb_flags = skops->bpf_sock_ops_cb_flags;
+
err = load_option(skops, &passive_estab_in, true);
if (err == -ENOENT) {
/* saved_syn is not found. It was in syncookie mode.
@@ -600,10 +603,10 @@ int estab(struct bpf_sock_ops *skops)
case BPF_SOCK_OPS_TCP_LISTEN_CB:
bpf_setsockopt(skops, SOL_TCP, TCP_SAVE_SYN,
&true_val, sizeof(true_val));
- set_hdr_cb_flags(skops);
+ set_hdr_cb_flags(skops, BPF_SOCK_OPS_STATE_CB_FLAG);
break;
case BPF_SOCK_OPS_TCP_CONNECT_CB:
- set_hdr_cb_flags(skops);
+ set_hdr_cb_flags(skops, 0);
break;
case BPF_SOCK_OPS_PARSE_HDR_OPT_CB:
return handle_parse_hdr(skops);
diff --git a/tools/testing/selftests/bpf/test_tcp_hdr_options.h b/tools/testing/selftests/bpf/test_tcp_hdr_options.h
index 78a8cf9eab42..6118e3ab61fc 100644
--- a/tools/testing/selftests/bpf/test_tcp_hdr_options.h
+++ b/tools/testing/selftests/bpf/test_tcp_hdr_options.h
@@ -110,12 +110,13 @@ static inline void clear_hdr_cb_flags(struct bpf_sock_ops *skops)
BPF_SOCK_OPS_WRITE_HDR_OPT_CB_FLAG));
}
-static inline void set_hdr_cb_flags(struct bpf_sock_ops *skops)
+static inline void set_hdr_cb_flags(struct bpf_sock_ops *skops, __u32 extra)
{
bpf_sock_ops_cb_flags_set(skops,
skops->bpf_sock_ops_cb_flags |
BPF_SOCK_OPS_PARSE_UNKNOWN_HDR_OPT_CB_FLAG |
- BPF_SOCK_OPS_WRITE_HDR_OPT_CB_FLAG);
+ BPF_SOCK_OPS_WRITE_HDR_OPT_CB_FLAG |
+ extra);
}
static inline void
clear_parse_all_hdr_cb_flags(struct bpf_sock_ops *skops)
--
2.24.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH bpf-next 0/2] Do not limit cb_flags when creating child sk from listen sk
2020-10-02 1:34 [PATCH bpf-next 0/2] Do not limit cb_flags when creating child sk from listen sk Martin KaFai Lau
2020-10-02 1:34 ` [PATCH bpf-next 1/2] bpf: tcp: " Martin KaFai Lau
2020-10-02 1:34 ` [PATCH bpf-next 2/2] bpf: selftest: Ensure the child sk inherited all bpf_sock_ops_cb_flags Martin KaFai Lau
@ 2020-10-02 16:03 ` sdf
2020-10-02 18:50 ` patchwork-bot+bpf
3 siblings, 0 replies; 5+ messages in thread
From: sdf @ 2020-10-02 16:03 UTC (permalink / raw)
To: Martin KaFai Lau
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, kernel-team, netdev
On 10/01, Martin KaFai Lau wrote:
> This set fixes an issue that the bpf_skops_init_child() unnecessarily
> limited the child sk from inheriting all bpf_sock_ops_cb_flags
> of the listen sk. It also adds a test to check that.
Thank you, that fixed it for me!
Tested-by: Stanislav Fomichev <sdf@google.com>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH bpf-next 0/2] Do not limit cb_flags when creating child sk from listen sk
2020-10-02 1:34 [PATCH bpf-next 0/2] Do not limit cb_flags when creating child sk from listen sk Martin KaFai Lau
` (2 preceding siblings ...)
2020-10-02 16:03 ` [PATCH bpf-next 0/2] Do not limit cb_flags when creating child sk from listen sk sdf
@ 2020-10-02 18:50 ` patchwork-bot+bpf
3 siblings, 0 replies; 5+ messages in thread
From: patchwork-bot+bpf @ 2020-10-02 18:50 UTC (permalink / raw)
To: Martin KaFai Lau; +Cc: bpf
Hello:
This series was applied to bpf/bpf-next.git (refs/heads/master):
On Thu, 1 Oct 2020 18:34:42 -0700 you wrote:
> This set fixes an issue that the bpf_skops_init_child() unnecessarily
> limited the child sk from inheriting all bpf_sock_ops_cb_flags
> of the listen sk. It also adds a test to check that.
>
> Martin KaFai Lau (2):
> bpf: tcp: Do not limit cb_flags when creating child sk from listen sk
> bpf: selftest: Ensure the child sk inherited all bpf_sock_ops_cb_flags
>
> [...]
Here is the summary with links:
- [bpf-next,1/2] bpf: tcp: Do not limit cb_flags when creating child sk from listen sk
https://git.kernel.org/bpf/bpf-next/c/82f45c6c4a70
- [bpf-next,2/2] bpf: selftest: Ensure the child sk inherited all bpf_sock_ops_cb_flags
https://git.kernel.org/bpf/bpf-next/c/96d46c508506
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] 5+ messages in thread
end of thread, other threads:[~2020-10-02 18:50 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-02 1:34 [PATCH bpf-next 0/2] Do not limit cb_flags when creating child sk from listen sk Martin KaFai Lau
2020-10-02 1:34 ` [PATCH bpf-next 1/2] bpf: tcp: " Martin KaFai Lau
2020-10-02 1:34 ` [PATCH bpf-next 2/2] bpf: selftest: Ensure the child sk inherited all bpf_sock_ops_cb_flags Martin KaFai Lau
2020-10-02 16:03 ` [PATCH bpf-next 0/2] Do not limit cb_flags when creating child sk from listen sk sdf
2020-10-02 18:50 ` patchwork-bot+bpf
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.