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 ?
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;
}
