Netdev Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Daniel Borkmann <daniel@iogearbox.net>
To: Martin KaFai Lau <kafai@fb.com>, bpf@vger.kernel.org
Cc: Alexei Starovoitov <ast@kernel.org>,
	Andrii Nakryiko <andrii@kernel.org>,
	kernel-team@fb.com, netdev@vger.kernel.org
Subject: Re: [PATCH bpf-next 4/4] bpf: selftests: Add dctcp fallback test
Date: Fri, 6 Aug 2021 18:07:01 +0200	[thread overview]
Message-ID: <217393dd-9af6-7e5c-3a02-630dde4b1280@iogearbox.net> (raw)
In-Reply-To: <20210805050144.1352078-1-kafai@fb.com>

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@fb.com>
[...]
> 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",
> +};
> 


  reply	other threads:[~2021-08-06 16:07 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-05  5:01 [PATCH bpf-next 0/4] bpf: tcp: Allow bpf-tcp-cc to call bpf_(get|set)sockopt Martin KaFai Lau
2021-08-05  5:01 ` [PATCH bpf-next 1/4] " Martin KaFai Lau
2021-08-05  5:01 ` [PATCH bpf-next 2/4] bpf: selftests: Add sk_state to bpf_tcp_helpers.h Martin KaFai Lau
2021-08-05  5:01 ` [PATCH bpf-next 3/4] bpf: selftests: Add connect_to_fd_opts to network_helpers Martin KaFai Lau
2021-08-05  5:01 ` [PATCH bpf-next 4/4] bpf: selftests: Add dctcp fallback test Martin KaFai Lau
2021-08-06 16:07   ` Daniel Borkmann [this message]
2021-08-06 17:42     ` Martin KaFai Lau

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=217393dd-9af6-7e5c-3a02-630dde4b1280@iogearbox.net \
    --to=daniel@iogearbox.net \
    --cc=andrii@kernel.org \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=kafai@fb.com \
    --cc=kernel-team@fb.com \
    --cc=netdev@vger.kernel.org \
    --subject='Re: [PATCH bpf-next 4/4] bpf: selftests: Add dctcp fallback test' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).