On Fri, Aug 06, 2021 at 06:07:01PM +0200, Daniel Borkmann wrote:

On 8/5/21 7:01 AM, Martin KaFai Lau wrote:

quoted

This patch makes the bpf_dctcp test to fallback to cubic by
using setsockopt(TCP_CONGESTION) when the tcp flow is not
ecn ready.

It also checks setsockopt() is not available to release().

The settimeo() from the network_helpers.h is used, so the local
one is removed.

Signed-off-by: Martin KaFai Lau <redacted>

[...]

quoted

diff --git a/tools/testing/selftests/bpf/progs/bpf_dctcp.c b/tools/testing/selftests/bpf/progs/bpf_dctcp.c
index fd42247da8b4..48df7ffbefdb 100644
--- a/tools/testing/selftests/bpf/progs/bpf_dctcp.c
+++ b/tools/testing/selftests/bpf/progs/bpf_dctcp.c

@@ -17,6 +17,9 @@
  char _license[] SEC("license") = "GPL";
+volatile const char fallback[TCP_CA_NAME_MAX];
+const char bpf_dctcp[] = "bpf_dctcp";
+char cc_res[TCP_CA_NAME_MAX];
  int stg_result = 0;
  struct {

@@ -57,6 +60,23 @@ void BPF_PROG(dctcp_init, struct sock *sk)
  	struct dctcp *ca = inet_csk_ca(sk);
  	int *stg;
+	if (!(tp->ecn_flags & TCP_ECN_OK) && fallback[0]) {
+		/* Switch to fallback */
+		bpf_setsockopt(sk, SOL_TCP, TCP_CONGESTION,
+			       (void *)fallback, sizeof(fallback));
+		/* Switch back to myself which the bpf trampoline
+		 * stopped calling dctcp_init recursively.
+		 */
+		bpf_setsockopt(sk, SOL_TCP, TCP_CONGESTION,
+			       (void *)bpf_dctcp, sizeof(bpf_dctcp));
+		/* Switch back to fallback */
+		bpf_setsockopt(sk, SOL_TCP, TCP_CONGESTION,
+			       (void *)fallback, sizeof(fallback));
+		bpf_getsockopt(sk, SOL_TCP, TCP_CONGESTION,
+			       (void *)cc_res, sizeof(cc_res));
+		return;

Is there a possibility where we later on instead of return refetch ca ptr via
ca = inet_csk_ca(sk) and mangle its struct dctcp fields whereas we're actually
messing with the new ca's internal fields (potentially crashing the kernel e.g.
if there was a pointer in the private struct of the new ca that we'd be corrupting)?

Without switching to another tcp-cc,
if the bpf-tcp-cc was buggy (e.g. setting incorrect cwnd), it could also
slow down (or stall) the flow a lot by putting wrong values in its own
icsk_ca_priv.

About the potential pointer value in icsk_ca_priv,
the bpf-tcp-cc can only use the icsk_ca_priv as SCALAR, so switching
to another bpf-tcp-cc should be fine.

If a bpf-tcp-cc is switching to a kernel-tcp-cc, that kernel-tcp-cc
could potentially store a pointer in icsk_ca_priv.  The only case I
know is the tcp_cdg.c when icsk_ca_priv is not large enough and it
has to resort to kcalloc and store this pointer in icsk_ca_priv.
Other kernel-tcp-cc stores its data inline in icsk_ca_priv.
The ICSK_CA_PRIV_SIZE has been increased a few times to
store new data inline instead of doing another kmalloc, so
this should be the common case. [cc: Eric]

It could disallow switching to kernel-tcp-cc but I think
it will just end up too limiting and forcing people
to create a bpf-tcp-cc shell to mimic the kernel-tcp-cc
during fallback.  Considering only very limited kernel-tcp-cc
stores pointer in icsk_ca_priv, how about imposing a white/black
list for bpf_setsockopt(TCP_CONGESTION), e.g. disallow switching
to tcp_cdg?  In the near future,  the tagging feature that
Yonghong is working can be used to tag some specific kernel-tcp-cc's
struct that is switchable from bpf side (which most of them should
be switchable). [cc: Yonghong]

WDYT?

Thanks for the review!