From: Kuniyuki Iwashima <kuniyu@xxxxxxxxxx>
Date: Thu, 14 Dec 2023 16:49:55 +0900
> From: Martin KaFai Lau <martin.lau@xxxxxxxxx>
> 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@xxxxxxxxx/
> > >>
> > >> 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@xxxxxxxxx/.
> >
> > I also hit an error earlier in
> > https://lore.kernel.org/all/20220817061847.4182339-1-kafai@xxxxxx/ 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;
> }
