Re: [PATCH v4 bpf-next 1/3] bpf: tcp: Handle BPF SYN Cookie in cookie_v[46]_check().

[Kuniyuki Iwashima <kuniyu@amazon.com>]

From: Martin KaFai Lau <martin.lau@linux.dev>
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@linux.dev>
> > 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