From: Martin KaFai Lau <martin.lau@xxxxxxxxx>
Date: Tue, 5 Dec 2023 19:11:17 -0800
> On 12/5/23 5:29 PM, Kuniyuki Iwashima wrote:
> > From: Martin KaFai Lau <martin.lau@xxxxxxxxx>
> > Date: Tue, 5 Dec 2023 16:19:20 -0800
> >> On 12/4/23 5:34 PM, Kuniyuki Iwashima wrote:
> >>> diff --git a/net/ipv4/syncookies.c b/net/ipv4/syncookies.c
> >>> index 61f1c96cfe63..0f9c3aed2014 100644
> >>> --- a/net/ipv4/syncookies.c
> >>> +++ b/net/ipv4/syncookies.c
> >>> @@ -304,6 +304,59 @@ static int cookie_tcp_reqsk_init(struct sock *sk, struct sk_buff *skb,
> >>> return 0;
> >>> }
> >>>
> >>> +#if IS_ENABLED(CONFIG_BPF)
> >>> +struct request_sock *cookie_bpf_check(struct net *net, struct sock *sk,
> >>> + struct sk_buff *skb)
> >>> +{
> >>> + struct request_sock *req = inet_reqsk(skb->sk);
> >>> + struct inet_request_sock *ireq = inet_rsk(req);
> >>> + struct tcp_request_sock *treq = tcp_rsk(req);
> >>> + struct tcp_options_received tcp_opt;
> >>> + int ret;
> >>> +
> >>> + skb->sk = NULL;
> >>> + skb->destructor = NULL;
> >>> + req->rsk_listener = NULL;
> >>> +
> >>> + memset(&tcp_opt, 0, sizeof(tcp_opt));
> >>> + tcp_parse_options(net, skb, &tcp_opt, 0, NULL);
> >>
> >> In patch 2, the bpf prog is passing the tcp_opt to the kfunc. The selftest in
> >> patch 3 is also parsing the tcp-options.
> >>
> >> The kernel parses the tcp-option here again to do some checking and req's member
> >> initialization. Can these checking and initialization be done in the
> >> bpf_sk_assign_tcp_reqsk() kfunc instead to avoid the double tcp-option parsing?
> >
> > If TS is not used as a cookie storage, bpf prog need not parse it.
> > OTOH, if a value is encoded into TS, bpf prog need to parse it.
> > In that case, we cannot avoid parsing options in bpf prog.
>
> If I read patch 2 correctly, the ireq->tstamp_ok is set by the kfunc, so I
> assume that the bpf prog has to parse the tcp-option.
>
> Like the "if (ireq->tstamp_ok ^ tcp_opt.saw_tstamp)" test below, ireq->tstamp_ok
> will always be 0 if the bpf prog did not parse the tcp-option.
Ah sorry, I assumed TS bit was encoded in SYN as disabled.
TCP option parsing is needed at least once for SYN, but we
need not do so for SYN+ACK if TS bit is in ISN.
>
> >
> > The parsing here comes from my paranoia, so.. probably we can drop it
> > and the first test below, and rely on bpf prog's tcp_opt, especially
> > tstamp_ok, rcv_tsval, and rcv_tsecr ?
>
> My preference is that it is clearer to allow the bpf prog to initialize all
> tcp_opt instead of only taking the tcp_opt.tstamp_ok from bpf_prog but ignore
> the tcp_opt.rcv_tsval/tsecr. The kfunc will then use the tcp_opt to initialize
> the req.
I'll drop the option parsing in kernel and allow bpf prog to fully
initialise tcp_opt.
>
> It is also better to detect the following error cases as much as possible in the
> kfunc instead of failing later in the tcp stack. e.g. checking the sysctl should
> be doable in the kfunc.
Ok, I'll move the sysctl tests and ts_off init to kfunc.
>
> >
> > I placed other tests here to align with the normal cookie flow, but
> > they can be moved to kfunc. However, initialisation assuems skb
> > points to TCP header, so here would be better place, I think.
> >
> >
> >>
> >>> +
> >>> + if (ireq->tstamp_ok ^ tcp_opt.saw_tstamp) {
> >>> + __NET_INC_STATS(net, LINUX_MIB_SYNCOOKIESFAILED);
> >>> + goto reset;
> >>> + }
> >>> +
> >>> + __NET_INC_STATS(net, LINUX_MIB_SYNCOOKIESRECV);
> >>> +
> >>> + if (ireq->tstamp_ok) {
> >>> + if (!READ_ONCE(net->ipv4.sysctl_tcp_timestamps))
> >>> + goto reset;
> >>> +
> >>> + req->ts_recent = tcp_opt.rcv_tsval;
> >>> + treq->ts_off = tcp_opt.rcv_tsecr - tcp_ns_to_ts(false, tcp_clock_ns());
> >>> + }
> >>> +
> >>> + if (ireq->sack_ok && !READ_ONCE(net->ipv4.sysctl_tcp_sack))
> >>> + goto reset;
> >>> +
> >>> + if (ireq->wscale_ok && !READ_ONCE(net->ipv4.sysctl_tcp_window_scaling))
> >>> + goto reset;
> >>> +
> >>> + ret = cookie_tcp_reqsk_init(sk, skb, req);
> >>> + if (ret) {
> >>> + reqsk_free(req);
> >>> + req = NULL;
> >>> + }
> >>> +
> >>> + return req;
> >>> +
> >>> +reset:
> >>> + reqsk_free(req);
> >>> + return ERR_PTR(-EINVAL);
> >>> +}
> >>> +EXPORT_SYMBOL_GPL(cookie_bpf_check);
> >>> +#endif
