John Fastabend wrote:
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)+{ +#ifdef CONFIG_SYN_COOKIES
- u32 cookie;
- int ret;
- if (unlikely(th_len < sizeof(*th)))
return -EINVAL;- if (!th->ack || th->rst || th->syn)
return -EINVAL;- if (unlikely(iph_len < sizeof(struct iphdr)))
return -EINVAL;- cookie = ntohl(th->ack_seq) - 1;
- /* Both struct iphdr and struct ipv6hdr have the version field at the
* same offset so we can cast to the shorter header (struct iphdr).*/- switch (((struct iphdr *)iph)->version) {
- case 4:
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.
I'm not sure you would need the len checks in helper, they provide some guarantees I guess, but the void * is just memory I don't see any checks on its size. It could be the last byte of a value for example?
I suspect we need to add verifier checks here anyways to ensure we don't walk off the end of a value unless something else is ensuring the iph is inside a valid memory block.
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?
The only benefit would be to shave some instructions off the program. XDP is about performance so I figure we shouldn't be adding arbitrary stuff here. OTOH you're already jumping into a helper so it might not matter at all.
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).
Just not sure you need the checks either, can you just assume the user gives good data?
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 that the XDP code I maintain has a if ipv4 {...} else ipv6{...} structure in it so could use a v4_check... and v6_check... then call the correct version directly, removing the switch from the helper.
Do you think there could be a performance reason to drop out those instructions or is it just hid by the hash itself. Also it seems a bit annoying if user is calling multiple helpers and they keep doing the same checks over and over.