Re: [PATCH v5 bpf-next 6/6] selftest: bpf: Test bpf_sk_assign_tcp_reqsk().

[Kuniyuki Iwashima <kuniyu@amazon.com>]

From: Martin KaFai Lau <martin.lau@linux.dev>
Date: Wed, 13 Dec 2023 12:44:42 -0800
> On 12/10/23 11:36 PM, Kuniyuki Iwashima wrote:
> > This commit adds a sample selftest to demonstrate how we can use
> > bpf_sk_assign_tcp_reqsk() as the backend of SYN Proxy.
> > 
> > The test creates IPv4/IPv6 x TCP/MPTCP connections and transfer
> > messages over them on lo with BPF tc prog attached.
> > 
> > The tc prog will process SYN and returns SYN+ACK with the following
> > ISN and TS.  In a real use case, this part will be done by other
> > hosts.
> > 
> >          MSB                                   LSB
> >    ISN:  | 31 ... 8 | 7 6 |   5 |    4 | 3 2 1 0 |
> >          |   Hash_1 | MSS | ECN | SACK |  WScale |
> > 
> >    TS:   | 31 ... 8 |          7 ... 0           |
> >          |   Random |           Hash_2           |
> > 
> >    WScale in SYN is reused in SYN+ACK.
> > 
> > The client returns ACK, and tc prog will recalculate ISN and TS
> > from ACK and validate SYN Cookie.
> > 
> > If it's valid, the prog calls kfunc to allocate a reqsk for skb and
> > configure the reqsk based on the argument created from SYN Cookie.
> > 
> > Later, the reqsk will be processed in cookie_v[46]_check() to create
> > a connection.
> 
> The patch set looks good.
> 
> One thing I just noticed is about writing/reading bits into/from "struct 
> tcp_options_received". More on this below.
> 
> [ ... ]
> 
> > +void test_tcp_custom_syncookie(void)
> > +{
> > +	struct test_tcp_custom_syncookie *skel;
> > +	int i;
> > +
> > +	if (setup_netns())
> > +		return;
> > +
> > +	skel = test_tcp_custom_syncookie__open_and_load();
> > +	if (!ASSERT_OK_PTR(skel, "open_and_load"))
> > +		return;
> > +
> > +	if (setup_tc(skel))
> > +		goto destroy_skel;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(test_cases); i++) {
> > +		skel->bss->handled_syn = false;
> > +		skel->bss->handled_ack = false;
> > +
> > +		test__start_subtest(test_cases[i].name);
> 
> 
> This should be tested with:
> 
> 	if (!test__start_subtest(test_cases[i].name))
> 		continue;
> 
> to skip the create_connection(). Probably do it at the beginning of the for loop.

Thanks for catching this!
Will fix.


> 
> > +		create_connection(&test_cases[i]);
> > +
> > +		ASSERT_EQ(skel->bss->handled_syn, true, "SYN is not handled at tc.");
> > +		ASSERT_EQ(skel->bss->handled_ack, true, "ACK is not handled at tc");
> > +	}
> > +
> > +destroy_skel:
> > +	system("tc qdisc del dev lo clsact");
> > +
> > +	test_tcp_custom_syncookie__destroy(skel);
> > +}
> 
> [ ... ]
> 
> > +static int tcp_parse_option(__u32 index, struct tcp_syncookie *ctx)
> > +{
> > +	struct tcp_options_received *tcp_opt = &ctx->attr.tcp_opt;
> > +	char opcode, opsize;
> > +
> > +	if (ctx->ptr + 1 > ctx->data_end)
> > +		goto stop;
> > +
> > +	opcode = *ctx->ptr++;
> > +
> > +	if (opcode == TCPOPT_EOL)
> > +		goto stop;
> > +
> > +	if (opcode == TCPOPT_NOP)
> > +		goto next;
> > +
> > +	if (ctx->ptr + 1 > ctx->data_end)
> > +		goto stop;
> > +
> > +	opsize = *ctx->ptr++;
> > +
> > +	if (opsize < 2)
> > +		goto stop;
> > +
> > +	switch (opcode) {
> > +	case TCPOPT_MSS:
> > +		if (opsize == TCPOLEN_MSS && ctx->tcp->syn &&
> > +		    ctx->ptr + (TCPOLEN_MSS - 2) < ctx->data_end)
> > +			tcp_opt->mss_clamp = get_unaligned_be16(ctx->ptr);
> > +		break;
> > +	case TCPOPT_WINDOW:
> > +		if (opsize == TCPOLEN_WINDOW && ctx->tcp->syn &&
> > +		    ctx->ptr + (TCPOLEN_WINDOW - 2) < ctx->data_end) {
> > +			tcp_opt->wscale_ok = 1;
> > +			tcp_opt->snd_wscale = *ctx->ptr;
> 
> When writing to a bitfield of "struct tcp_options_received" which is a kernel 
> struct, it needs to use the CO-RE api. The BPF_CORE_WRITE_BITFIELD has not been 
> landed yet: 
> https://lore.kernel.org/bpf/4d3dd215a4fd57d980733886f9c11a45e1a9adf3.1702325874.git.dxu@dxuuu.xyz/
> 
> The same for reading bitfield but BPF_CORE_READ_BITFIELD() has already been 
> implemented in bpf_core_read.h
> 
> Once the BPF_CORE_WRITE_BITFIELD is landed, this test needs to be changed to use 
> the BPF_CORE_{READ,WRITE}_BITFIELD.

IIUC, the CO-RE api assumes that the offset of bitfields could be changed.

If the size of struct tcp_cookie_attributes is changed, kfunc will not work
in this test.  So, BPF_CORE_WRITE_BITFIELD() works only when the size of
tcp_cookie_attributes is unchanged but fields in tcp_options_received are
rearranged or expanded to use the unused@ bits ?

Also, do we need to use BPF_CORE_READ() for other non-bitfields in
strcut tcp_options_received (and ecn_ok in struct tcp_cookie_attributes
just in case other fields are added to tcp_cookie_attributes and ecn_ok
is rearranged) ?

Just trying to understand when to use CO-RE api.

Btw, thanks for merging BPF_CORE_WRITE_BITFIELD patches!


> 
> > +		}
> > +		break;
> > +	case TCPOPT_TIMESTAMP:
> > +		if (opsize == TCPOLEN_TIMESTAMP &&
> > +		    ctx->ptr + (TCPOLEN_TIMESTAMP - 2) < ctx->data_end) {
> > +			tcp_opt->saw_tstamp = 1;
> > +			tcp_opt->rcv_tsval = get_unaligned_be32(ctx->ptr);
> > +			tcp_opt->rcv_tsecr = get_unaligned_be32(ctx->ptr + 4);
> > +
> > +			if (ctx->tcp->syn && tcp_opt->rcv_tsecr)
> > +				tcp_opt->tstamp_ok = 0;
> > +			else
> > +				tcp_opt->tstamp_ok = 1;
> > +		}
> > +		break;
> > +	case TCPOPT_SACK_PERM:
> > +		if (opsize == TCPOLEN_SACK_PERM && ctx->tcp->syn &&
> > +		    ctx->ptr + (TCPOLEN_SACK_PERM - 2) < ctx->data_end)
> > +			tcp_opt->sack_ok = 1;
> > +		break;
> > +	}
> > +
> > +	ctx->ptr += opsize - 2;
> > +next:
> > +	return 0;
> > +stop:
> > +	return 1;
> > +}