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

[Martin KaFai Lau <kafai@fb.com>]

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!