Re: [PATCH bpf-next v2 2/3] bpf: Add helpers to issue and check SYN cookies in XDP

[John Fastabend <john.fastabend@gmail.com>]

Maxim Mikityanskiy wrote:
> On 2022-01-25 09:54, John Fastabend wrote:
> > Maxim Mikityanskiy wrote:
> >> The new helpers bpf_tcp_raw_{gen,check}_syncookie allow an XDP program
> >> to generate SYN cookies in response to TCP SYN packets and to check
> >> those cookies upon receiving the first ACK packet (the final packet of
> >> the TCP handshake).
> >>
> >> Unlike bpf_tcp_{gen,check}_syncookie these new helpers don't need a
> >> listening socket on the local machine, which allows to use them together
> >> with synproxy to accelerate SYN cookie generation.
> >>
> >> Signed-off-by: Maxim Mikityanskiy <maximmi@nvidia.com>
> >> Reviewed-by: Tariq Toukan <tariqt@nvidia.com>
> >> ---
> > 
> > [...]
> > 
> >> +
> >> +BPF_CALL_4(bpf_tcp_raw_check_syncookie, void *, iph, u32, iph_len,
> >> +	   struct tcphdr *, th, u32, th_len)
> >> +{

[...]

>> > 
> > Did you consider just exposing __cookie_v4_check() and __cookie_v6_check()?
> 
> No, I didn't, I just implemented it consistently with 
> bpf_tcp_check_syncookie, but let's consider it.
> 
> I can't just pass a pointer from BPF without passing the size, so I 
> would need some wrappers around __cookie_v{4,6}_check anyway. The checks 
> for th_len and iph_len would have to stay in the helpers. The check for 
> TCP flags (ACK, !RST, !SYN) could be either in the helper or in BPF. The 
> switch would obviously be gone.

For consideration... those duplicate checks in the runtime that we
already could know from verifier side are bothering me.

We could have some new mem types, PTR_TO_IPV4, PTR_TO_IPv6, and PTR_TO_TCP.
Then we simplify the helper signatures to just,

  bpf_tcp_raw_check_syncookie_v4(iph, tcph);
  bpf_tcp_raw_check_syncookie_v6(iph, tcph);

And the verifier "knows" what a v4/v6 header is and does the mem
check at verification time instead of run time.

Then the code becomes very straightforward,

 BPF_CALL_2(bpf_tcp_raw_check_syncookie_v4, void *, iph, void *, th)
{
   u16 mss = tcp_parse_mss_option(th, 0) ?: TCP_MSS_DEFAULT;   
   return  __cookie_v4_init_sequence(iph, th, &mss);
}

We don't need length checks because we are guaranteed by conmpiler
to have valid lengths, assume code is smart enough to understand
syn, ack, rst because any real program likely already knows this.
And v4/v6 is likely also known by real program already.

If we push a bit more on this mss with PTR_TO_TCP and PTR_TO_IP
we can simply mark tcp_parse_mss_option and __cookie_v4_init_sequence
and let BPF side call them.

Curious what others think here.

> 
> The bottom line is that it would be the same code, but without the 
> switch, and repeated twice. What benefit do you see in this approach? 
>  From my side, I only see the ability to drop one branch at the expense 
> of duplicating the code above the switch (th_len and iph_len checks).
> 
> > My code at least has already run the code above before it would ever call
> > this helper so all the other bits are duplicate.
> 
> Sorry, I didn't quite understand this part. What "your code" are you 
> referring to?

Just the XDP parsers we have already switch early on based on v4/v6
and I imagine that most progs also know this. So yes we are arguing
about a simple switch, but instruction here and instruction there
add up over time. Also passing the size through the helper bothers
me slightly given the verifier should know the size already.