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

[Kuniyuki Iwashima <kuniyu@amazon.com>]

From: Kuniyuki Iwashima <kuniyu@amazon.com>
Date: Thu, 14 Dec 2023 16:49:55 +0900
> From: Martin KaFai Lau <martin.lau@linux.dev>
> Date: Wed, 13 Dec 2023 22:46:11 -0800
> > On 12/13/23 7:18 PM, Kuniyuki Iwashima wrote:
> > >>> +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 ?
> > 
> > Right, CO-RE helps to figure out the offset of a member in the running kernel.
> > 
> > > 
> > > 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) ?
> > 
> > BPF_CORE_READ is a CO-RE friendly macro for using bpf_probe_read_kernel(). 
> > bpf_probe_read_kernel() is mostly for the tracing use case where the ptr is not 
> > safe to read directly.
> > 
> > It is not the case for the tcp_options_received ptr in this tc-bpf use case or 
> > other stack allocated objects. In general, no need to use BPF_CORE_READ. The 
> > relocation will be done by the libbpf for tcp_opt->mss_clamp (e.g.).
> > 
> > Going back to bitfield, it needs BPF_CORE_*_BITFIELD because the offset may not 
> > be right after __attribute__((preserve_access_index)), cc: Yonghong and Andrii 
> > who know more details than I do.
> > 
> > A verifier error has been reported: 
> > https://lore.kernel.org/bpf/391d524c496acc97a8801d8bea80976f58485810.1700676682.git.dxu@dxuuu.xyz/.
> > 
> > I also hit an error earlier in 
> > https://lore.kernel.org/all/20220817061847.4182339-1-kafai@fb.com/ when not 
> > using BPF_CORE_READ_BITFIELD. I don't exactly remember how the instruction looks 
> > like but it was reading a wrong value instead of verifier error.
> 
> Thank you so much for detailed explanation!
> 
> 
> > 
> > ================
> > 
> > Going back to this patch set here.
> > 
> > After sleeping on it longer, I am thinking it is better not to reuse 'struct 
> > tcp_options_received' (meaning no bitfield) in the bpf_sk_assign_tcp_reqsk() 
> > kfunc API.
> > 
> > There is not much benefit in reusing 'tcp_options_received'. When new tcp option 
> > was ever added to tcp_options_received, it is not like bpf_sk_assign_tcp_reqsk 
> > will support it automatically. It needs to relay this new option back to the 
> > allocated req. Unlike tcp_sock or req which may have a lot of them such that it 
> > is useful to have a compact tcp_options_received, the tc-bpf use case here is to 
> > allocate it once in the stack. Also, not all the members in tcp_options_received 
> > is useful, e.g. num_sacks, ts_recent_stamp, and user_mss are not used. Leaving 
> > it there being ignored by bpf_sk_assign_tcp_reqsk is confusing.
> > 
> > How about using a full u8 for each necessary member and directly add them to 
> > struct tcp_cookie_attributes instead of nesting them into another struct. After 
> > taking out the unnecessary members, the size may not end up to be much bigger.
> > 
> > The bpf prog can then directly access attr->tstamp_ok more naturally. The 
> > changes to patch 5 and 6 should be mostly mechanical changes.
> > 
> > I would also rename s/tcp_cookie_attributes/bpf_tcp_req_attrs/.
> > 
> > wdyt?
> 
> Totally agree.  I reused struct tcp_options_received but had a similar
> thought like unused fields, confusing fields (saw_tstamp vs tstamp_ok,
> user_mss vs clamp_mss), etc.
> 
> And I like bpf_tcp_req_attrs, tcp_cookie_attributes was bit wordy :)
> 
> So probably bpf_tcp_req_attrs would look like this ?
> 
> struct bpf_tcp_req_attrs {
> 	u32 rcv_tsval;
> 	u32 rcv_tsecr;
> 	u16 mss;
> 	u8 rcv_scale;
> 	u8 snd_scale;
> 	bool ecn_ok;
> 	bool wscale_ok;
> 	bool sack_ok;
> 	bool tstamp_ok;
> 	bool usec_ts;
> } __packed;
> 
> or you prefer u8 over bool and __packed ?

Ah, bool and __packed will require BPF_CORE_(READ|WRITE)_BITFIELD().
I'll use the following struct.

Thank you!

> 
> struct bpf_tcp_req_attrs {
> 	u32 rcv_tsval;
> 	u32 rcv_tsecr;
> 	u16 mss;
> 	u8 rcv_scale;
> 	u8 snd_scale;
> 	u8 ecn_ok;
> 	u8 wscale_ok;
> 	u8 sack_ok;
> 	u8 tstamp_ok;
> 	u8 usec_ts;
> }