Re: [PATCH bpf-next 4/4] bpf: selftests: Add dctcp fallback test

[Daniel Borkmann <daniel@xxxxxxxxxxxxx>]

On 8/5/21 7:01 AM, Martin KaFai Lau wrote:
> 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 <kafai@xxxxxx>
[...]
> 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)?

> +	}
> +
>   	ca->prior_rcv_nxt = tp->rcv_nxt;
>   	ca->dctcp_alpha = min(dctcp_alpha_on_init, DCTCP_MAX_ALPHA);
>   	ca->loss_cwnd = 0;
> diff --git a/tools/testing/selftests/bpf/progs/bpf_dctcp_release.c b/tools/testing/selftests/bpf/progs/bpf_dctcp_release.c
> new file mode 100644
> index 000000000000..d836f7c372f0
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/bpf_dctcp_release.c
> @@ -0,0 +1,26 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2021 Facebook */
> +
> +#include <stddef.h>
> +#include <linux/bpf.h>
> +#include <linux/types.h>
> +#include <linux/stddef.h>
> +#include <linux/tcp.h>
> +#include <bpf/bpf_helpers.h>
> +#include <bpf/bpf_tracing.h>
> +#include "bpf_tcp_helpers.h"
> +
> +char _license[] SEC("license") = "GPL";
> +const char cubic[] = "cubic";
> +
> +void BPF_STRUCT_OPS(dctcp_nouse_release, struct sock *sk)
> +{
> +	bpf_setsockopt(sk, SOL_TCP, TCP_CONGESTION,
> +		       (void *)cubic, sizeof(cubic));
> +}
> +
> +SEC(".struct_ops")
> +struct tcp_congestion_ops dctcp_rel = {
> +	.release	= (void *)dctcp_nouse_release,
> +	.name		= "bpf_dctcp_rel",
> +};