Message-ID: <6b6ce8a2-e409-0297-cb29-fe9493c9d637@nvidia.com>
Date:   Mon, 21 Feb 2022 16:26:28 +0200
From:   Maxim Mikityanskiy <maximmi@...dia.com>
To:     Toke Høiland-Jørgensen <toke@...e.dk>,
        "John Fastabend" <john.fastabend@...il.com>
CC:     <bpf@...r.kernel.org>, Alexei Starovoitov <ast@...nel.org>,
        "Daniel Borkmann" <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>, <netdev@...r.kernel.org>,
        Tariq Toukan <tariqt@...dia.com>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        KP Singh <kpsingh@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        "Jakub Kicinski" <kuba@...nel.org>,
        Petar Penkov <ppenkov@...gle.com>,
        Lorenz Bauer <lmb@...udflare.com>,
        Eric Dumazet <edumazet@...gle.com>,
        Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>,
        David Ahern <dsahern@...nel.org>,
        Shuah Khan <shuah@...nel.org>,
        Jesper Dangaard Brouer <hawk@...nel.org>,
        "Nathan Chancellor" <nathan@...nel.org>,
        Nick Desaulniers <ndesaulniers@...gle.com>,
        Joe Stringer <joe@...ium.io>,
        Florent Revest <revest@...omium.org>,
        <linux-kselftest@...r.kernel.org>,
        Kumar Kartikeya Dwivedi <memxor@...il.com>,
        Florian Westphal <fw@...len.de>
Subject: Re: [PATCH bpf-next v2 2/3] bpf: Add helpers to issue and check SYN
 cookies in XDP

On 2022-02-04 16:08, Toke Høiland-Jørgensen wrote:
> John Fastabend <john.fastabend@...il.com> writes:
> 
>> Maxim Mikityanskiy wrote:
>>> On 2022-01-31 23:19, John Fastabend wrote:
>>>> 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@...dia.com>
>>>>>>>> Reviewed-by: Tariq Toukan <tariqt@...dia.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?
>>>
>>> The verifier makes sure that the packet pointer and the size come
>>> together in function parameters (see check_arg_pair_ok). It also makes
>>> sure that the memory region defined by these two parameters is valid,
>>> i.e. in our case it belongs to packet data.
>>>
>>> Now that the helper got a valid memory region, its length is still
>>> arbitrary. The helper has to check it's big enough to contain a TCP
>>> header, before trying to access its fields. Hence the checks in the helper.
>>>
>>>> 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 verifier ensures that the [iph; iph+iph_len) is valid memory, but
>>> the helper still has to check that struct iphdr fits into this region.
>>> Otherwise iph_len could be too small, and the helper would access memory
>>> outside of the valid region.
>>
>> Thanks for the details this all makes sense. See response to
>> other mail about adding new types. Replied to the wrong email
>> but I think the context is not lost.
> 
> Keeping my reply here in an attempt to de-fork :)
> 
>>>>>>
>>>>>> 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?
>>>
>>> No, since the BPF program would be able to trick the kernel into reading
>>> from an invalid location (see the explanation above).
>>>
>>>>>>
>>>>>>> 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
>>>
>>> Same for my code (see the last patch in the series).
>>>
>>> Splitting into two helpers would allow to drop the extra switch in the
>>> helper, however:
>>>
>>> 1. The code will be duplicated for the checks.
>>
>> See response wrt PTR_TO_IP, PTR_TO_TCP types.
> 
> So about that (quoting some context from your other email):
> 
>> 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.
> 
> I think this could probably be achieved with PTR_TO_BTF arguments to the
> helper (if we define appropriate struct types that the program can use
> for each header type)?

I get the following error when I try to pass the headers from packet 
data to a helper that accepts ARG_PTR_TO_BTF_ID:

; value = bpf_tcp_raw_gen_syncookie_ipv4(hdr->ipv4, hdr->tcp,
297: (79) r1 = *(u64 *)(r10 -80)      ; R1_w=pkt(id=0,off=14,r=74,imm=0) 
R10=fp0
298: (79) r2 = *(u64 *)(r10 -72)      ; 
R2_w=pkt(id=5,off=14,r=74,umax_value=60,var_off=(0x0; 0x3c)) R10=fp0
299: (bc) w3 = w9                     ; 
R3_w=invP(id=0,umin_value=20,umax_value=60,var_off=(0x0; 0x3c)) 
R9=invP(id=0,umin_value=20,umax_value=60,var_off=(0x0; 0x3c))
300: (85) call bpf_tcp_raw_gen_syncookie_ipv4#192
R1 type=pkt expected=ptr_
processed 317 insns (limit 1000000) max_states_per_insn 0 total_states 
23 peak_states 23 mark_read 12
-- END PROG LOAD LOG --

It looks like the verifier doesn't currently support such type 
conversion. Could you give any clue what is needed to add this support? 
Is it enough to extend compatible_reg_types, or should more checks be 
added anywhere?

Alternatively, I can revert to ARG_PTR_TO_MEM and do size checks in 
runtime in the helper.

> It doesn't really guard against pointing into the wrong bit of the
> packet (or somewhere else entirely), so the header can still contain
> garbage, but at least the len check should be handled automatically with
> PTR_TO_BTF, and we avoid the need to define a whole bunch of new
> PTR_TO_* types...
> 
>>> 2. It won't be consistent with bpf_tcp_check_syncookie (and all other
>>> existing helpers - as far as I see, there is no split for IPv4/IPv6).
>>
>> This does seem useful to me.
> 
> If it's consistency we're after we could split the others as well? I
> guess the main drawback here is code bloat (can't inline the functions
> as they need to be available for BPF_CALL, so we either get duplicates
> or an additional function call overhead for the old helper if it just
> calls the new ones).
> 
>>> 3. It's easier to misuse, e.g., pass an IPv6 header to the IPv4 helper.
>>> This point is controversial, since it shouldn't pose any additional
>>> security threat, but in my opinion, it's better to be foolproof. That
>>> means, I'd add the IP version check even to the separate helpers, which
>>> defeats the purpose of separating them.
>>
>> Not really convinced that we need to guard against misuse. This is
>> down in XDP space its not a place we should be adding extra insns
>> to stop developers from hurting themselves, just as a general
>> rule.
> 
> Yeah, I think in general for XDP, if you pass garbage data to the
> helpers to get to keep the pieces when it breaks. We need to make sure
> the *kernel* doesn't misbehave (i.e., no crashing, and no invalid state
> being created inside the kernel), but it's up to the XDP program author
> to use the API correctly...
> 
>>> Given these points, I'd prefer to keep it a single helper. However, if
>>> you have strong objections, I can split it.
>>
>> I think (2) is the strongest argument combined with the call is
>> heavy operation and saving some cycles maybe isn't terribly
>> important, but its XDP land again and insns matter IMO. I'm on
>> the fence maybe others have opinions?
> 
> It's not a very strong opinion, but I think in general, optimising for
> performance in XDP is the right thing to do. That's kinda what it's for :)
> 
> -Toke

Powered by blists - more mailing lists