Netdev Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Martin KaFai Lau <kafai@fb.com>
To: Daniel Borkmann <daniel@iogearbox.net>
Cc: <bpf@vger.kernel.org>, Alexei Starovoitov <ast@kernel.org>,
	Andrii Nakryiko <andrii@kernel.org>,
	Eric Dumazet <eric.dumazet@gmail.com>, Yonghong Song <yhs@fb.com>,
	<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 10:42:08 -0700	[thread overview]
Message-ID: <20210806174208.dhxf2fo7755kdvz7@kafai-mbp> (raw)
In-Reply-To: <217393dd-9af6-7e5c-3a02-630dde4b1280@iogearbox.net>

On Fri, Aug 06, 2021 at 06:07:01PM +0200, Daniel Borkmann wrote:
> 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)?
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!

      reply	other threads:[~2021-08-06 17:42 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
2021-08-06 17:42     ` Martin KaFai Lau [this message]

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=20210806174208.dhxf2fo7755kdvz7@kafai-mbp \
    --to=kafai@fb.com \
    --cc=andrii@kernel.org \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=daniel@iogearbox.net \
    --cc=eric.dumazet@gmail.com \
    --cc=kernel-team@fb.com \
    --cc=netdev@vger.kernel.org \
    --cc=yhs@fb.com \
    --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).