| Message ID | 20220124151340.376807-3-maximmi@nvidia.com |
|---|---|
| State | New |
| Headers | show |
| Series | New BPF helpers to accelerate synproxy | expand |
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. 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? > The only reason to build > it this way, as I see it, is either code can call it blindly without doing > 4/v6 switch. or to make it look and feel like 'tc' world, but its already > dropped the ok so its a bit different already and ifdef TC/XDP could > hanlde the different parts. > > >> + ret = __cookie_v4_check((struct iphdr *)iph, th, cookie); >> + break; >> + >> +#if IS_BUILTIN(CONFIG_IPV6) >> + case 6: >> + if (unlikely(iph_len < sizeof(struct ipv6hdr))) >> + return -EINVAL; >> + >> + ret = __cookie_v6_check((struct ipv6hdr *)iph, th, cookie); >> + break; >> +#endif /* CONFIG_IPV6 */ >> + >> + default: >> + return -EPROTONOSUPPORT; >> + } >> + >> + if (ret > 0) >> + return 0; >> + >> + return -EACCES; >> +#else >> + return -EOPNOTSUPP; >> +#endif >> +}
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? > > 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.
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.
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@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? 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. >> >>> >>> 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. 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). 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. Given these points, I'd prefer to keep it a single helper. However, if you have strong objections, I can split it. >> 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. > >
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.
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@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? > > 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. > > >> > >>> > >>> 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. > > 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. > > 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. > > 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? Sorry for diverging the thread a bit there with the two replies. Thanks, John
John Fastabend <john.fastabend@gmail.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@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? >> >> 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)? 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
On 2022-02-04 16:08, Toke Høiland-Jørgensen wrote: > John Fastabend <john.fastabend@gmail.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@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? >>> >>> 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
> -----Original Message----- > From: Kumar Kartikeya Dwivedi <memxor@gmail.com> > Sent: 21 February, 2022 17:22 > To: Maxim Mikityanskiy <maximmi@nvidia.com> > Cc: Toke Høiland-Jørgensen <toke@toke.dk>; John Fastabend > <john.fastabend@gmail.com>; bpf@vger.kernel.org; Alexei Starovoitov > <ast@kernel.org>; Daniel Borkmann <daniel@iogearbox.net>; Andrii Nakryiko > <andrii@kernel.org>; netdev@vger.kernel.org; Tariq Toukan > <tariqt@nvidia.com>; Martin KaFai Lau <kafai@fb.com>; Song Liu > <songliubraving@fb.com>; Yonghong Song <yhs@fb.com>; KP Singh > <kpsingh@kernel.org>; David S. Miller <davem@davemloft.net>; Jakub Kicinski > <kuba@kernel.org>; Petar Penkov <ppenkov@google.com>; Lorenz Bauer > <lmb@cloudflare.com>; Eric Dumazet <edumazet@google.com>; Hideaki YOSHIFUJI > <yoshfuji@linux-ipv6.org>; David Ahern <dsahern@kernel.org>; Shuah Khan > <shuah@kernel.org>; Jesper Dangaard Brouer <hawk@kernel.org>; Nathan > Chancellor <nathan@kernel.org>; Nick Desaulniers <ndesaulniers@google.com>; > Joe Stringer <joe@cilium.io>; Florent Revest <revest@chromium.org>; linux- > kselftest@vger.kernel.org; Florian Westphal <fw@strlen.de> > Subject: Re: [PATCH bpf-next v2 2/3] bpf: Add helpers to issue and check SYN > cookies in XDP > > On Mon, Feb 21, 2022 at 07:56:28PM IST, Maxim Mikityanskiy wrote: > > On 2022-02-04 16:08, Toke Høiland-Jørgensen wrote: > > > John Fastabend <john.fastabend@gmail.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@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? > > > > > > > > > > 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? > > > > I think what he meant was getting the size hint from the function prototype. > In > case of kfunc we do it by resolving type size from BTF, for the PTR_TO_MEM > case > when a size argument is missing. For helper, you can add a field to indicate > the > constant size hint in the bpf_func_proto, and then in check_func_arg > directly do > the equivalent check_helper_mem_access for arg_type_is_mem_ptr block if such > a > hint is set, instead of delaying it till check_mem_size_reg call when the > next > arg_type_is_mem_size block is executed. > > Then you can have two helpers with same argument types but different size > hint > values for the header argument, so you wouldn't need an extra mem size > parameter. > > You may also want to disallow setting both the size hint and next argument > as > ARG_CONST_SIZE. Thanks, I implemented your suggestion as a new feature of the verifier, and it works. I'm ready to respin the series, I've split my new helpers, but splitting the existing helpers won't be part of resubmission, because it is out of scope of changes I intended to push. It can be added later as an improvement, though. > > Alternatively, I can revert to ARG_PTR_TO_MEM and do size checks in > runtime > > in the helper. > > > > [...] > > -- > Kartikeya
diff --git a/include/net/tcp.h b/include/net/tcp.h index 44e442bf23f9..0aca79f7b1be 100644 --- a/include/net/tcp.h +++ b/include/net/tcp.h @@ -432,6 +432,7 @@ u16 tcp_v4_get_syncookie(struct sock *sk, struct iphdr *iph, struct tcphdr *th, u32 *cookie); u16 tcp_v6_get_syncookie(struct sock *sk, struct ipv6hdr *iph, struct tcphdr *th, u32 *cookie); +u16 tcp_parse_mss_option(const struct tcphdr *th, u16 user_mss); u16 tcp_get_syncookie_mss(struct request_sock_ops *rsk_ops, const struct tcp_request_sock_ops *af_ops, struct sock *sk, struct tcphdr *th); diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h index 4d2d4a09bf25..07864277297b 100644 --- a/include/uapi/linux/bpf.h +++ b/include/uapi/linux/bpf.h @@ -5090,6 +5090,61 @@ union bpf_attr { * associated to *xdp_md*, at *offset*. * Return * 0 on success, or a negative error in case of failure. + * + * s64 bpf_tcp_raw_gen_syncookie(void *iph, u32 iph_len, struct tcphdr *th, u32 th_len) + * Description + * Try to issue a SYN cookie for the packet with corresponding + * IP/TCP headers, *iph* and *th*, without depending on a listening + * socket. + * + * *iph* points to the start of the IPv4 or IPv6 header, while + * *iph_len* contains **sizeof**\ (**struct iphdr**) or + * **sizeof**\ (**struct ip6hdr**). + * + * *th* points to the start of the TCP header, while *th_len* + * contains the length of the TCP header (at least + * **sizeof**\ (**struct tcphdr**)). + * Return + * On success, lower 32 bits hold the generated SYN cookie in + * followed by 16 bits which hold the MSS value for that cookie, + * and the top 16 bits are unused. + * + * On failure, the returned value is one of the following: + * + * **-EINVAL** if the packet or input arguments are invalid. + * + * **-EOPNOTSUPP** if the kernel configuration does not enable SYN + * cookies (CONFIG_SYN_COOKIES is off). + * + * **-EPROTONOSUPPORT** if the IP version is not 4 or 6 (or 6, but + * CONFIG_IPV6 is disabled). + * + * int bpf_tcp_raw_check_syncookie(void *iph, u32 iph_len, struct tcphdr *th, u32 th_len) + * Description + * Check whether *iph* and *th* contain a valid SYN cookie ACK + * without depending on a listening socket. + * + * *iph* points to the start of the IPv4 or IPv6 header, while + * *iph_len* contains **sizeof**\ (**struct iphdr**) or + * **sizeof**\ (**struct ip6hdr**). + * + * *th* points to the start of the TCP header, while *th_len* + * contains the length of the TCP header (at least + * **sizeof**\ (**struct tcphdr**)). + * Return + * 0 if *iph* and *th* are a valid SYN cookie ACK. + * + * On failure, the returned value is one of the following: + * + * **-EACCES** if the SYN cookie is not valid. + * + * **-EINVAL** if the packet or input arguments are invalid. + * + * **-EOPNOTSUPP** if the kernel configuration does not enable SYN + * cookies (CONFIG_SYN_COOKIES is off). + * + * **-EPROTONOSUPPORT** if the IP version is not 4 or 6 (or 6, but + * CONFIG_IPV6 is disabled). */ #define __BPF_FUNC_MAPPER(FN) \ FN(unspec), \ @@ -5283,6 +5338,8 @@ union bpf_attr { FN(xdp_get_buff_len), \ FN(xdp_load_bytes), \ FN(xdp_store_bytes), \ + FN(tcp_raw_gen_syncookie), \ + FN(tcp_raw_check_syncookie), \ /* */ /* integer value in 'imm' field of BPF_CALL instruction selects which helper diff --git a/net/core/filter.c b/net/core/filter.c index 18559b5828a3..dc90d2649a7a 100644 --- a/net/core/filter.c +++ b/net/core/filter.c @@ -7375,6 +7375,124 @@ static const struct bpf_func_proto bpf_sock_ops_reserve_hdr_opt_proto = { .arg3_type = ARG_ANYTHING, }; +BPF_CALL_4(bpf_tcp_raw_gen_syncookie, void *, iph, u32, iph_len, + struct tcphdr *, th, u32, th_len) +{ +#ifdef CONFIG_SYN_COOKIES + u32 cookie; + u16 mss; + + if (unlikely(th_len < sizeof(*th) || th_len != th->doff * 4)) + return -EINVAL; + + if (!th->syn || th->ack || th->fin || th->rst) + return -EINVAL; + + if (unlikely(iph_len < sizeof(struct iphdr))) + return -EINVAL; + + /* 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: + mss = tcp_parse_mss_option(th, 0) ?: TCP_MSS_DEFAULT; + cookie = __cookie_v4_init_sequence(iph, th, &mss); + break; + +#if IS_BUILTIN(CONFIG_IPV6) + case 6: { + const u16 mss_clamp = IPV6_MIN_MTU - sizeof(struct tcphdr) - sizeof(struct ipv6hdr); + + if (unlikely(iph_len < sizeof(struct ipv6hdr))) + return -EINVAL; + + mss = tcp_parse_mss_option(th, 0) ?: mss_clamp; + cookie = __cookie_v6_init_sequence(iph, th, &mss); + break; + } +#endif /* CONFIG_IPV6 */ + + default: + return -EPROTONOSUPPORT; + } + + return cookie | ((u64)mss << 32); +#else + return -EOPNOTSUPP; +#endif /* CONFIG_SYN_COOKIES */ +} + +static const struct bpf_func_proto bpf_tcp_raw_gen_syncookie_proto = { + .func = bpf_tcp_raw_gen_syncookie, + .gpl_only = true, /* __cookie_v*_init_sequence() is GPL */ + .pkt_access = true, + .ret_type = RET_INTEGER, + .arg1_type = ARG_PTR_TO_MEM, + .arg2_type = ARG_CONST_SIZE, + .arg3_type = ARG_PTR_TO_MEM, + .arg4_type = ARG_CONST_SIZE, +}; + +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: + ret = __cookie_v4_check((struct iphdr *)iph, th, cookie); + break; + +#if IS_BUILTIN(CONFIG_IPV6) + case 6: + if (unlikely(iph_len < sizeof(struct ipv6hdr))) + return -EINVAL; + + ret = __cookie_v6_check((struct ipv6hdr *)iph, th, cookie); + break; +#endif /* CONFIG_IPV6 */ + + default: + return -EPROTONOSUPPORT; + } + + if (ret > 0) + return 0; + + return -EACCES; +#else + return -EOPNOTSUPP; +#endif +} + +static const struct bpf_func_proto bpf_tcp_raw_check_syncookie_proto = { + .func = bpf_tcp_raw_check_syncookie, + .gpl_only = true, /* __cookie_v*_check is GPL */ + .pkt_access = true, + .ret_type = RET_INTEGER, + .arg1_type = ARG_PTR_TO_MEM, + .arg2_type = ARG_CONST_SIZE, + .arg3_type = ARG_PTR_TO_MEM, + .arg4_type = ARG_CONST_SIZE, +}; + #endif /* CONFIG_INET */ bool bpf_helper_changes_pkt_data(void *func) @@ -7785,6 +7903,10 @@ xdp_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) return &bpf_tcp_check_syncookie_proto; case BPF_FUNC_tcp_gen_syncookie: return &bpf_tcp_gen_syncookie_proto; + case BPF_FUNC_tcp_raw_gen_syncookie: + return &bpf_tcp_raw_gen_syncookie_proto; + case BPF_FUNC_tcp_raw_check_syncookie: + return &bpf_tcp_raw_check_syncookie_proto; #endif default: return bpf_sk_base_func_proto(func_id); diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c index dc49a3d551eb..a60f6aa44b02 100644 --- a/net/ipv4/tcp_input.c +++ b/net/ipv4/tcp_input.c @@ -3961,7 +3961,7 @@ static bool smc_parse_options(const struct tcphdr *th, /* Try to parse the MSS option from the TCP header. Return 0 on failure, clamped * value on success. */ -static u16 tcp_parse_mss_option(const struct tcphdr *th, u16 user_mss) +u16 tcp_parse_mss_option(const struct tcphdr *th, u16 user_mss) { const unsigned char *ptr = (const unsigned char *)(th + 1); int length = (th->doff * 4) - sizeof(struct tcphdr); @@ -4000,6 +4000,7 @@ static u16 tcp_parse_mss_option(const struct tcphdr *th, u16 user_mss) } return mss; } +EXPORT_SYMBOL_GPL(tcp_parse_mss_option); /* Look for tcp options. Normally only called on SYN and SYNACK packets. * But, this can also be called on packets in the established flow when diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h index 4d2d4a09bf25..07864277297b 100644 --- a/tools/include/uapi/linux/bpf.h +++ b/tools/include/uapi/linux/bpf.h @@ -5090,6 +5090,61 @@ union bpf_attr { * associated to *xdp_md*, at *offset*. * Return * 0 on success, or a negative error in case of failure. + * + * s64 bpf_tcp_raw_gen_syncookie(void *iph, u32 iph_len, struct tcphdr *th, u32 th_len) + * Description + * Try to issue a SYN cookie for the packet with corresponding + * IP/TCP headers, *iph* and *th*, without depending on a listening + * socket. + * + * *iph* points to the start of the IPv4 or IPv6 header, while + * *iph_len* contains **sizeof**\ (**struct iphdr**) or + * **sizeof**\ (**struct ip6hdr**). + * + * *th* points to the start of the TCP header, while *th_len* + * contains the length of the TCP header (at least + * **sizeof**\ (**struct tcphdr**)). + * Return + * On success, lower 32 bits hold the generated SYN cookie in + * followed by 16 bits which hold the MSS value for that cookie, + * and the top 16 bits are unused. + * + * On failure, the returned value is one of the following: + * + * **-EINVAL** if the packet or input arguments are invalid. + * + * **-EOPNOTSUPP** if the kernel configuration does not enable SYN + * cookies (CONFIG_SYN_COOKIES is off). + * + * **-EPROTONOSUPPORT** if the IP version is not 4 or 6 (or 6, but + * CONFIG_IPV6 is disabled). + * + * int bpf_tcp_raw_check_syncookie(void *iph, u32 iph_len, struct tcphdr *th, u32 th_len) + * Description + * Check whether *iph* and *th* contain a valid SYN cookie ACK + * without depending on a listening socket. + * + * *iph* points to the start of the IPv4 or IPv6 header, while + * *iph_len* contains **sizeof**\ (**struct iphdr**) or + * **sizeof**\ (**struct ip6hdr**). + * + * *th* points to the start of the TCP header, while *th_len* + * contains the length of the TCP header (at least + * **sizeof**\ (**struct tcphdr**)). + * Return + * 0 if *iph* and *th* are a valid SYN cookie ACK. + * + * On failure, the returned value is one of the following: + * + * **-EACCES** if the SYN cookie is not valid. + * + * **-EINVAL** if the packet or input arguments are invalid. + * + * **-EOPNOTSUPP** if the kernel configuration does not enable SYN + * cookies (CONFIG_SYN_COOKIES is off). + * + * **-EPROTONOSUPPORT** if the IP version is not 4 or 6 (or 6, but + * CONFIG_IPV6 is disabled). */ #define __BPF_FUNC_MAPPER(FN) \ FN(unspec), \ @@ -5283,6 +5338,8 @@ union bpf_attr { FN(xdp_get_buff_len), \ FN(xdp_load_bytes), \ FN(xdp_store_bytes), \ + FN(tcp_raw_gen_syncookie), \ + FN(tcp_raw_check_syncookie), \ /* */ /* integer value in 'imm' field of BPF_CALL instruction selects which helper
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> --- include/net/tcp.h | 1 + include/uapi/linux/bpf.h | 57 +++++++++++++++ net/core/filter.c | 122 +++++++++++++++++++++++++++++++++ net/ipv4/tcp_input.c | 3 +- tools/include/uapi/linux/bpf.h | 57 +++++++++++++++ 5 files changed, 239 insertions(+), 1 deletion(-)