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.
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.
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.
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
