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

[Martin KaFai Lau <martin.lau@linux.dev>]

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.

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