New BPF helpers to accelerate synproxy

PATCH Superseded Maxim Mikityanskiy 4 participants 17 messages Jan 24, 2022 – Feb 24, 2022

Subsystems (8)

Networking Network drivers Kselftest BPF +4 more subsystems

lore

Share & tools

lore

Maxim Mikityanskiy

author

2022-01-24 15:13 UTC #0 lore raw

v2 cover New BPF helpers to accelerate synproxy

The first patch of this series is an improvement to the existing syncookie BPF helper.

The two other patches add new functionality that allows XDP to accelerate iptables synproxy.

v1 of this series [1] used to include a patch that exposed conntrack lookup to BPF using stable helpers. It was superseded by series [2] by Kumar Kartikeya Dwivedi, which implements this functionality using unstable helpers.

The second patch adds new helpers to issue and check SYN cookies without binding to a socket, which is useful in the synproxy scenario.

The third patch adds a selftest, which consists of a script, an XDP program and a userspace control application. The XDP program uses socketless SYN cookie helpers and queries conntrack status instead of socket status. The userspace control application allows to tune parameters of the XDP program. This program also serves as a minimal example of usage of the new functionality.

The draft of the new functionality was presented on Netdev 0x15 [3].

v2 changes:

Split into two series, submitted bugfixes to bpf, dropped the conntrack patches, implemented the timestamp cookie in BPF using bpf_loop, dropped the timestamp cookie patch.

[1]: https://lore.kernel.org/bpf/[email protected]/t/
[2]: https://lore.kernel.org/bpf/[email protected]/
[3]: https://netdevconf.info/0x15/session.html?Accelerating-synproxy-with-XDP

Maxim Mikityanskiy (3):
bpf: Make errors of bpf_tcp_check_syncookie distinguishable
bpf: Add helpers to issue and check SYN cookies in XDP
bpf: Add selftests for raw syncookie helpers

 include/net/tcp.h                             |   1 +
 include/uapi/linux/bpf.h                      |  75 +-
 net/core/filter.c                             | 128 ++-
 net/ipv4/tcp_input.c                          |   3 +-
 tools/include/uapi/linux/bpf.h                |  75 +-
 tools/testing/selftests/bpf/.gitignore        |   1 +
 tools/testing/selftests/bpf/Makefile          |   5 +-
 .../selftests/bpf/progs/xdp_synproxy_kern.c   | 743 ++++++++++++++++++
 .../selftests/bpf/test_xdp_synproxy.sh        |  71 ++
 tools/testing/selftests/bpf/xdp_synproxy.c    | 418 ++++++++++
 10 files changed, 1510 insertions(+), 10 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/progs/xdp_synproxy_kern.c
 create mode 100755 tools/testing/selftests/bpf/test_xdp_synproxy.sh
 create mode 100644 tools/testing/selftests/bpf/xdp_synproxy.c

Signature · 1 line

2.30.2

Maxim Mikityanskiy

author

2022-01-24 15:14 UTC #1 lore raw

PATCH v2 1/3 bpf: Make errors of bpf_tcp_check_syncookie distinguishable

bpf_tcp_check_syncookie returns ambiguous error codes in some cases. The list below shows various error conditions and matching error codes:

1. NULL socket: -EINVAL.

2. Invalid packet: -EINVAL, -ENOENT.

3. Bad cookie: -ENOENT.

4. Cookies are not in use: -EINVAL, -ENOENT.

5. Good cookie: 0.

As we see, the same error code may correspond to multiple error conditions, making them undistinguishable, and at the same time one error condition may return different codes, although it's typically handled in the same way.

This patch reassigns error codes of bpf_tcp_check_syncookie and documents them:

1. Invalid packet or NULL socket: -EINVAL;

2. Bad cookie: -EACCES.

3. Cookies are not in use: -ENOENT.

4. Good cookie: 0.

This change allows XDP programs to make smarter decisions based on error code, because different error conditions are now easily distinguishable.

Backward compatibility shouldn't suffer because of these reasons:

1. The specific error codes weren't documented. The behavior that used to be documented (0 is good cookie, negative values are errors) still holds. Anyone who relied on implementation details should have understood the risks.

2. Two known usecases (classification of ACKs with cookies that initial new connections, SYN flood protection) take decisions which don't depend on specific error codes:

Traffic classification:
ACK packet is new, error == 0: classify as NEW. ACK packet is new, error < 0: classify as INVALID.

SYN flood protection:
ACK packet is new, error == 0: good cookie, XDP_PASS. ACK packet is new, error < 0: bad cookie, XDP_DROP.

As Lorenz Bauer confirms, their implementation of traffic classifier won't break, as well as the kernel selftests.

3. It's hard to imagine that old error codes could be used for any useful decisions.

Signed-off-by Maxim Mikityanskiy <[email protected]>

Reviewed-by Tariq Toukan <[email protected]>

---
 include/uapi/linux/bpf.h       | 18 ++++++++++++++++--
 net/core/filter.c              |  6 +++---
 tools/include/uapi/linux/bpf.h | 18 ++++++++++++++++--
 3 files changed, 35 insertions(+), 7 deletions(-)

include/uapi/linux/bpf.h +16 -2

index 16a7574292a5..4d2d4a09bf25 100644
@@ -3575,8 +3575,22 @@ union bpf_attr {
  * 		*th* points to the start of the TCP header, while *th_len*
  * 		contains **sizeof**\ (**struct tcphdr**).
  * 	Return
- * 		0 if *iph* and *th* are a valid SYN cookie ACK, or a negative
- * 		error otherwise.
+ *		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.
+ *
+ *		**-ENOENT** if SYN cookies are not issued (no SYN flood, or SYN
+ *		cookies are disabled in sysctl).
+ *
+ *		**-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).
  *
  * long bpf_sysctl_get_name(struct bpf_sysctl *ctx, char *buf, size_t buf_len, u64 flags)
  *	Description

net/core/filter.c +3 -3

index a06931c27eeb..18559b5828a3 100644
@@ -6998,10 +6998,10 @@ BPF_CALL_5(bpf_tcp_check_syncookie, struct sock *, sk, void *, iph, u32, iph_len
 		return -EINVAL;
 
 	if (!sock_net(sk)->ipv4.sysctl_tcp_syncookies)
-		return -EINVAL;
+		return -ENOENT;
 
 	if (!th->ack || th->rst || th->syn)
-		return -ENOENT;
+		return -EINVAL;
 
 	if (tcp_synq_no_recent_overflow(sk))
 		return -ENOENT;

@@ -7032,7 +7032,7 @@ BPF_CALL_5(bpf_tcp_check_syncookie, struct sock *, sk, void *, iph, u32, iph_len
 	if (ret > 0)
 		return 0;
 
-	return -ENOENT;
+	return -EACCES;
 #else
 	return -ENOTSUPP;
 #endif

tools/include/uapi/linux/bpf.h +16 -2

index 16a7574292a5..4d2d4a09bf25 100644
@@ -3575,8 +3575,22 @@ union bpf_attr {
  * 		*th* points to the start of the TCP header, while *th_len*
  * 		contains **sizeof**\ (**struct tcphdr**).
  * 	Return
- * 		0 if *iph* and *th* are a valid SYN cookie ACK, or a negative
- * 		error otherwise.
+ *		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.
+ *
+ *		**-ENOENT** if SYN cookies are not issued (no SYN flood, or SYN
+ *		cookies are disabled in sysctl).
+ *
+ *		**-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).
  *
  * long bpf_sysctl_get_name(struct bpf_sysctl *ctx, char *buf, size_t buf_len, u64 flags)
  *	Description

Signature · 1 line

2.30.2

John Fastabend

maintainer

2022-01-25 08:57 UTC #2 lore raw

RE: [PATCH bpf-next v2 1/3] bpf: Make errors of bpf_tcp_check_syncookie distinguishable

Quoted 23 lines · Maxim Mikityanskiy

bpf_tcp_check_syncookie returns ambiguous error codes in some cases. The list below shows various error conditions and matching error codes:
1. NULL socket: -EINVAL.
2. Invalid packet: -EINVAL, -ENOENT.
3. Bad cookie: -ENOENT.
4. Cookies are not in use: -EINVAL, -ENOENT.
5. Good cookie: 0.
As we see, the same error code may correspond to multiple error conditions, making them undistinguishable, and at the same time one error condition may return different codes, although it's typically handled in the same way.
This patch reassigns error codes of bpf_tcp_check_syncookie and documents them:
1. Invalid packet or NULL socket: -EINVAL;

Quoted 8 lines

2. Bad cookie: -EACCES.
3. Cookies are not in use: -ENOENT.
4. Good cookie: 0.
This change allows XDP programs to make smarter decisions based on error code, because different error conditions are now easily distinguishable.

I'm missing the point here it still looks a bit like shuffling around of code. What do you gain, whats the real bug? Are you trying to differentiate between an overflow condition and a valid syncookie? But I don't think you said this anywhere.

At the moment EINVAL tells me somethings wrong with the input or configuration, although really any app that cares checked the sysctl flag right?

ENOENT tells me either recent overflow or cookie is invalid. If there is no '!ack || rst || syn' then I can either learn that directly from the program (why would a real program through these at the helper), but it also falls into the incorrect cookie in some sense.

Quoted 7 lines

Backward compatibility shouldn't suffer because of these reasons:
1. The specific error codes weren't documented. The behavior that used to be documented (0 is good cookie, negative values are errors) still holds. Anyone who relied on implementation details should have understood the risks.

I'll disagree, just because a user facing API doesn't document its behavior very well doesn't mean users should some how understand the risks. Ideally we would have done better with error codes up front, but thats old history. If a user complains that this breaks a real application it would likely be reason to revert it.

At least I would remove this from the commit.

Quoted 61 lines

2. Two known usecases (classification of ACKs with cookies that initial new connections, SYN flood protection) take decisions which don't depend on specific error codes:
Traffic classification:
ACK packet is new, error == 0: classify as NEW. ACK packet is new, error < 0: classify as INVALID.
SYN flood protection:
ACK packet is new, error == 0: good cookie, XDP_PASS. ACK packet is new, error < 0: bad cookie, XDP_DROP.
As Lorenz Bauer confirms, their implementation of traffic classifier won't break, as well as the kernel selftests.
3. It's hard to imagine that old error codes could be used for any useful decisions.
Signed-off-by Maxim Mikityanskiy <[email protected]>
Reviewed-by Tariq Toukan <[email protected]>
---
 include/uapi/linux/bpf.h       | 18 ++++++++++++++++--
 net/core/filter.c              |  6 +++---
 tools/include/uapi/linux/bpf.h | 18 ++++++++++++++++--
 3 files changed, 35 insertions(+), 7 deletions(-)
include/uapi/linux/bpf.h +16 -2
index 16a7574292a5..4d2d4a09bf25 100644
@@ -3575,8 +3575,22 @@ union bpf_attr {
  * 		*th* points to the start of the TCP header, while *th_len*
  * 		contains **sizeof**\ (**struct tcphdr**).
  * 	Return
- * 		0 if *iph* and *th* are a valid SYN cookie ACK, or a negative
- * 		error otherwise.
+ *		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.
+ *
+ *		**-ENOENT** if SYN cookies are not issued (no SYN flood, or SYN
+ *		cookies are disabled in sysctl).
+ *
+ *		**-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).
  *
  * long bpf_sysctl_get_name(struct bpf_sysctl *ctx, char *buf, size_t buf_len, u64 flags)
  *	Description
net/core/filter.c +0 -0
index a06931c27eeb..18559b5828a3 100644

net/core/filter.c BPF_CALL_5(bpf_tcp_check_syncookie, struct sock *, sk, void *, iph, u32, iph_len · line 7001

Quoted 5 lines

net/core/filter.c +1 -1

@@ -6998,10 +6998,10 @@ BPF_CALL_5(bpf_tcp_check_syncookie, struct sock *, sk, void *, iph, u32, iph_len
 		return -EINVAL;
 
 	if (!sock_net(sk)->ipv4.sysctl_tcp_syncookies)
-		return -EINVAL;
+		return -ENOENT;

I wouldn't change this.

net/core/filter.c BPF_CALL_5(bpf_tcp_check_syncookie, struct sock *, sk, void *, iph, u32, iph_len · line 7004

Quoted 4 lines

net/core/filter.c +1 -1

 	if (!th->ack || th->rst || th->syn)
-		return -ENOENT;
+		return -EINVAL;

not sure if this is useful change and it is bpf program visible.

net/core/filter.c BPF_CALL_5(bpf_tcp_check_syncookie, struct sock *, sk, void *, iph, u32, iph_len · line 7035

Quoted 9 lines

net/core/filter.c +1 -1

 	if (tcp_synq_no_recent_overflow(sk))
 		return -ENOENT;

@@ -7032,7 +7032,7 @@ BPF_CALL_5(bpf_tcp_check_syncookie, struct sock *, sk, void *, iph, u32, iph_len
 	if (ret > 0)
 		return 0;
 
-	return -ENOENT;
+	return -EACCES;

This one might have a valid argument to differentiate between an overflow condition and an invalid cookie. But, curious what do you do with this info?

Thanks,
John

Maxim Mikityanskiy

author

2022-01-31 13:39 UTC #3 lore raw

Re: [PATCH bpf-next v2 1/3] bpf: Make errors of bpf_tcp_check_syncookie distinguishable

Quoted 28 lines · John Fastabend

Quoted 27 lines · Maxim Mikityanskiy
bpf_tcp_check_syncookie returns ambiguous error codes in some cases. The list below shows various error conditions and matching error codes:
1. NULL socket: -EINVAL.
2. Invalid packet: -EINVAL, -ENOENT.
3. Bad cookie: -ENOENT.
4. Cookies are not in use: -EINVAL, -ENOENT.
5. Good cookie: 0.
As we see, the same error code may correspond to multiple error conditions, making them undistinguishable, and at the same time one error condition may return different codes, although it's typically handled in the same way.
This patch reassigns error codes of bpf_tcp_check_syncookie and documents them:
1. Invalid packet or NULL socket: -EINVAL;
2. Bad cookie: -EACCES.
3. Cookies are not in use: -ENOENT.

Quoted 7 lines

Quoted 4 lines
4. Good cookie: 0.
This change allows XDP programs to make smarter decisions based on error code, because different error conditions are now easily distinguishable.
I'm missing the point here it still looks a bit like shuffling around of code. What do you gain, whats the real bug?

Current error codes are useless. If the caller gets a negative value, all it knows is that some error happened. I'm making different error conditions distinguishable (invalid input, bad cookie, not expecting a cookie).

Use cases could be: statistic counters, debugging, logging. For example, the kernel counts LINUX_MIB_SYNCOOKIESFAILED, which only includes the "bad cookie" case.

Another use case is replying with RST when not expecting a cookie (a new ACK packet could just mean that the server was rebooted, and it's good to tell the client that the connection is broken), but dropping packets under the flood on receiving bad cookies (to avoid wasting resources on sending replies).

Quoted 3 lines

Are you
trying to differentiate between an overflow condition and a valid syncookie? But I don't think you said this anywhere.

I'm not sure what you mean by the "overflow condition", but a valid syncookie is easily distinguishable from other cases, it's 0.

Quoted 3 lines

At the moment EINVAL tells me somethings wrong with the input or configuration, although really any app that cares checked the sysctl flag right?

There is no way to check sysctl from XDP. A single program could be useful both with and without syncookies, it could determine its behavior in runtime, not to say that the sysctl option could change in runtime. The workaround you suggested will work in some cases, but other cases just won't work (there are no notification events on sysctl changes that a userspace application could monitor and pass to the XDP program; it would be not immediate anyway).

Quoted 1 line

ENOENT tells me either recent overflow or cookie is invalid.

And these cases should be distinguished, as I said above.

Quoted 114 lines

If
there is no '!ack || rst || syn' then I can either learn that directly from the program (why would a real program through these at the helper), but it also falls into the incorrect cookie in some sense.
Quoted 7 lines
Backward compatibility shouldn't suffer because of these reasons:
1. The specific error codes weren't documented. The behavior that used to be documented (0 is good cookie, negative values are errors) still holds. Anyone who relied on implementation details should have understood the risks.
I'll disagree, just because a user facing API doesn't document its behavior very well doesn't mean users should some how understand the risks. Ideally we would have done better with error codes up front, but thats old history. If a user complains that this breaks a real application it would likely be reason to revert it.
At least I would remove this from the commit.
Quoted 61 lines
2. Two known usecases (classification of ACKs with cookies that initial new connections, SYN flood protection) take decisions which don't depend on specific error codes:
Traffic classification:
ACK packet is new, error == 0: classify as NEW. ACK packet is new, error < 0: classify as INVALID.
SYN flood protection:
ACK packet is new, error == 0: good cookie, XDP_PASS. ACK packet is new, error < 0: bad cookie, XDP_DROP.
As Lorenz Bauer confirms, their implementation of traffic classifier won't break, as well as the kernel selftests.
3. It's hard to imagine that old error codes could be used for any useful decisions.
Signed-off-by Maxim Mikityanskiy <[email protected]>
Reviewed-by Tariq Toukan <[email protected]>
---
  include/uapi/linux/bpf.h       | 18 ++++++++++++++++--
  net/core/filter.c              |  6 +++---
  tools/include/uapi/linux/bpf.h | 18 ++++++++++++++++--
  3 files changed, 35 insertions(+), 7 deletions(-)
include/uapi/linux/bpf.h +16 -2
index 16a7574292a5..4d2d4a09bf25 100644
@@ -3575,8 +3575,22 @@ union bpf_attr {
   * 		*th* points to the start of the TCP header, while *th_len*
   * 		contains **sizeof**\ (**struct tcphdr**).
   * 	Return
- * 		0 if *iph* and *th* are a valid SYN cookie ACK, or a negative
- * 		error otherwise.
+ *		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.
+ *
+ *		**-ENOENT** if SYN cookies are not issued (no SYN flood, or SYN
+ *		cookies are disabled in sysctl).
+ *
+ *		**-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).
   *
   * long bpf_sysctl_get_name(struct bpf_sysctl *ctx, char *buf, size_t buf_len, u64 flags)
   *	Description
net/core/filter.c +0 -0
index a06931c27eeb..18559b5828a3 100644
net/core/filter.c BPF_CALL_5(bpf_tcp_check_syncookie, struct sock *, sk, void *, iph, u32, iph_len · line 7001
Quoted 5 lines
net/core/filter.c +1 -1
@@ -6998,10 +6998,10 @@ BPF_CALL_5(bpf_tcp_check_syncookie, struct sock *, sk, void *, iph, u32, iph_len
  		return -EINVAL;
 
  	if (!sock_net(sk)->ipv4.sysctl_tcp_syncookies)
-		return -EINVAL;
+		return -ENOENT;
I wouldn't change this.
net/core/filter.c BPF_CALL_5(bpf_tcp_check_syncookie, struct sock *, sk, void *, iph, u32, iph_len · line 7004
Quoted 4 lines
net/core/filter.c +1 -1
  	if (!th->ack || th->rst || th->syn)
-		return -ENOENT;
+		return -EINVAL;
not sure if this is useful change and it is bpf program visible.
net/core/filter.c BPF_CALL_5(bpf_tcp_check_syncookie, struct sock *, sk, void *, iph, u32, iph_len · line 7035
Quoted 9 lines
net/core/filter.c +1 -1
  	if (tcp_synq_no_recent_overflow(sk))
  		return -ENOENT;
@@ -7032,7 +7032,7 @@ BPF_CALL_5(bpf_tcp_check_syncookie, struct sock *, sk, void *, iph, u32, iph_len
  	if (ret > 0)
  		return 0;
 
-	return -ENOENT;
+	return -EACCES;
This one might have a valid argument to differentiate between an overflow condition and an invalid cookie. But, curious what do you do with this info?
Thanks,
John

Maxim Mikityanskiy

author

2022-01-24 15:14 UTC #4 lore raw

PATCH v2 2/3 bpf: Add helpers to issue and check SYN cookies in XDP

In reply to Maxim Mikityanskiy

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 <[email protected]>

Reviewed-by Tariq Toukan <[email protected]>

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

include/net/tcp.h +1 -0

index 44e442bf23f9..0aca79f7b1be 100644
@@ -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);

include/uapi/linux/bpf.h +57 -0

69 lines not on this page · raw message

net/core/filter.c +122 -0

134 lines not on this page · raw message

net/ipv4/tcp_input.c +2 -1

index dc49a3d551eb..a60f6aa44b02 100644
@@ -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

tools/include/uapi/linux/bpf.h +57 -0

69 lines not on this page · raw message

Signature · 1 line

2.30.2

John Fastabend

maintainer

2022-01-25 08:59 UTC #5 lore raw

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

Quoted 12 lines · Maxim Mikityanskiy

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 <[email protected]>
Reviewed-by Tariq Toukan <[email protected]>
---

[...]

Quoted 14 lines

net/core/filter.c +14 -0

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

net/core/filter.c static const struct bpf_func_proto bpf_sock_ops_reserve_hdr_opt_proto = { · line 7459

Quoted 10 lines

net/core/filter.c +10 -0

+	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()? My code at least has already run the code above before it would ever call this helper so all the other bits are duplicate. 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.

Quoted 24 lines

net/core/filter.c +24 -0

+		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

author

2022-01-31 13:38 UTC #6 lore raw

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

Quoted 34 lines · John Fastabend

Quoted 12 lines · Maxim Mikityanskiy
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 <[email protected]>
Reviewed-by Tariq Toukan <[email protected]>
---
[...]
Quoted 18 lines
net/core/filter.c +18 -0
+
+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;

Quoted 8 lines

net/core/filter.c static const struct bpf_func_proto bpf_sock_ops_reserve_hdr_opt_proto = { · line 7398
Quoted 6 lines
net/core/filter.c +6 -0
+
+	/* 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).

Quoted 2 lines

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?

Quoted 31 lines

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.
Quoted 24 lines
net/core/filter.c +24 -0
+		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
+}

John Fastabend

maintainer

2022-01-31 21:12 UTC #7 lore raw

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

Quoted 44 lines · Maxim Mikityanskiy

Quoted 34 lines · John Fastabend
Quoted 12 lines · Maxim Mikityanskiy
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 <[email protected]>
Reviewed-by Tariq Toukan <[email protected]>
---
[...]
Quoted 18 lines
net/core/filter.c +18 -0
+
+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;
Quoted 8 lines
net/core/filter.c static const struct bpf_func_proto bpf_sock_ops_reserve_hdr_opt_proto = { · line 7398
Quoted 6 lines
net/core/filter.c +6 -0
+
+	/* 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()?

Quoted 8 lines

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?

Quoted 3 lines

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.

Quoted 2 lines

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?

Quoted 6 lines

Quoted 3 lines
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

maintainer

2022-01-31 21:19 UTC #8 lore raw

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

Quoted 54 lines · John Fastabend

Quoted 44 lines · Maxim Mikityanskiy
Quoted 34 lines · John Fastabend
Quoted 12 lines · Maxim Mikityanskiy
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 <[email protected]>
Reviewed-by Tariq Toukan <[email protected]>
---
[...]
Quoted 18 lines
net/core/filter.c +18 -0
+
+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;
Quoted 8 lines
net/core/filter.c static const struct bpf_func_proto bpf_sock_ops_reserve_hdr_opt_proto = { · line 7398
Quoted 6 lines
net/core/filter.c +6 -0
+
+	/* 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()?
Quoted 8 lines
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.

Quoted 4 lines

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.

Quoted 31 lines

Quoted 4 lines
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.
Quoted 2 lines
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?
Quoted 6 lines
Quoted 3 lines
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.

Maxim Mikityanskiy

author

2022-02-02 11:10 UTC #9 lore raw

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

Quoted 55 lines · John Fastabend

Quoted 54 lines · John Fastabend
Quoted 52 lines · Maxim Mikityanskiy
Quoted 34 lines · John Fastabend
Quoted 12 lines · Maxim Mikityanskiy
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 <[email protected]>
Reviewed-by Tariq Toukan <[email protected]>
---
[...]
Quoted 18 lines
net/core/filter.c +18 -0
+
+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;
Quoted 8 lines
net/core/filter.c static const struct bpf_func_proto bpf_sock_ops_reserve_hdr_opt_proto = { · line 7398
Quoted 6 lines
net/core/filter.c +6 -0
+
+	/* 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.

Quoted 4 lines

Quoted 4 lines
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.

Quoted 3 lines

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.

Quoted 10 lines

Quoted 10 lines
Quoted 4 lines
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.

Quoted 5 lines

Quoted 5 lines
Quoted 2 lines
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).

Quoted 9 lines

Quoted 9 lines
Quoted 6 lines
Quoted 3 lines
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.

Quoted 9 lines

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

maintainer

2022-02-04 02:50 UTC #10 lore raw

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

Quoted 70 lines · Maxim Mikityanskiy

Quoted 55 lines · John Fastabend
Quoted 54 lines · John Fastabend
Quoted 52 lines · Maxim Mikityanskiy
Quoted 34 lines · John Fastabend
Quoted 12 lines · Maxim Mikityanskiy
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 <[email protected]>
Reviewed-by Tariq Toukan <[email protected]>
---
[...]
Quoted 18 lines
net/core/filter.c +18 -0
+
+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;
Quoted 8 lines
net/core/filter.c static const struct bpf_func_proto bpf_sock_ops_reserve_hdr_opt_proto = { · line 7398
Quoted 6 lines
net/core/filter.c +6 -0
+
+	/* 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.
Quoted 4 lines
Quoted 4 lines
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.

Quoted 8 lines

Quoted 3 lines
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.

Quoted 30 lines

Quoted 11 lines
Quoted 11 lines
Quoted 5 lines
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.
Quoted 5 lines
Quoted 5 lines
Quoted 2 lines
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).
Quoted 9 lines
Quoted 9 lines
Quoted 6 lines
Quoted 3 lines
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

Quoted 6 lines

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.

Quoted 3 lines

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.

Quoted 6 lines

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.

Quoted 3 lines

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

Toke Høiland-Jørgensen

2022-02-04 14:08 UTC #11 lore raw

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

Quoted 75 lines · John Fastabend

Quoted 74 lines · Maxim Mikityanskiy
Quoted 55 lines · John Fastabend
Quoted 54 lines · John Fastabend
Quoted 52 lines · Maxim Mikityanskiy
Quoted 34 lines · John Fastabend
Quoted 12 lines · Maxim Mikityanskiy
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 <[email protected]>
Reviewed-by Tariq Toukan <[email protected]>
---
[...]
Quoted 18 lines
(quoted diff) +18 -0
+
+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;
Quoted 8 lines
Review
Quoted 6 lines
(quoted diff) +6 -0
+
+	/* 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.
Quoted 4 lines
Quoted 4 lines
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.
Quoted 3 lines
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.

Quoted 8 lines

Quoted 4 lines
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 :)

Quoted 28 lines

Quoted 28 lines
Quoted 9 lines
Quoted 9 lines
Quoted 3 lines
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.
Quoted 5 lines
Quoted 5 lines
Quoted 2 lines
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).
Quoted 9 lines
Quoted 9 lines
Quoted 6 lines
Quoted 3 lines
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

Quoted 8 lines

Quoted 6 lines
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):

Quoted 8 lines

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

Quoted 4 lines

Quoted 2 lines
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).

Quoted 10 lines

Quoted 5 lines
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...

Quoted 7 lines

Quoted 2 lines
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

Maxim Mikityanskiy

author

2022-02-21 14:26 UTC #12 lore raw

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

Quoted 133 lines · Toke Høiland-Jørgensen

Quoted 75 lines · John Fastabend
Quoted 74 lines · Maxim Mikityanskiy
Quoted 55 lines · John Fastabend
Quoted 54 lines · John Fastabend
Quoted 52 lines · Maxim Mikityanskiy
Quoted 34 lines · John Fastabend
Quoted 12 lines · Maxim Mikityanskiy
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 <[email protected]>
Reviewed-by Tariq Toukan <[email protected]>
---
[...]
Quoted 18 lines
(quoted diff) +18 -0
+
+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;
Quoted 8 lines
Review
Quoted 6 lines
(quoted diff) +6 -0
+
+	/* 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.
Quoted 4 lines
Quoted 4 lines
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.
Quoted 3 lines
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.
Quoted 8 lines
Quoted 4 lines
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 :)
Quoted 28 lines
Quoted 28 lines
Quoted 9 lines
Quoted 9 lines
Quoted 3 lines
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.
Quoted 5 lines
Quoted 5 lines
Quoted 2 lines
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).
Quoted 9 lines
Quoted 9 lines
Quoted 6 lines
Quoted 3 lines
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
Quoted 8 lines
Quoted 6 lines
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):
Quoted 6 lines
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);

Quoted 6 lines

Quoted 2 lines
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.

Quoted 46 lines

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...
Quoted 4 lines
Quoted 2 lines
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).
Quoted 10 lines
Quoted 5 lines
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...
Quoted 7 lines
Quoted 2 lines
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

Kumar Kartikeya Dwivedi

maintainer

2022-02-21 15:21 UTC #13 lore raw

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

Quoted 158 lines · Maxim Mikityanskiy

Quoted 133 lines · Toke Høiland-Jørgensen
Quoted 75 lines · John Fastabend
Quoted 74 lines · Maxim Mikityanskiy
Quoted 55 lines · John Fastabend
Quoted 54 lines · John Fastabend
Quoted 52 lines · Maxim Mikityanskiy
Quoted 34 lines · John Fastabend
Quoted 12 lines · Maxim Mikityanskiy
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 <[email protected]>
Reviewed-by Tariq Toukan <[email protected]>
---
[...]
Quoted 18 lines
(quoted diff) +18 -0
+
+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;
Quoted 8 lines
Review
Quoted 6 lines
(quoted diff) +6 -0
+
+	/* 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.
Quoted 4 lines
Quoted 4 lines
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.
Quoted 3 lines
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.
Quoted 8 lines
Quoted 4 lines
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 :)
Quoted 28 lines
Quoted 28 lines
Quoted 9 lines
Quoted 9 lines
Quoted 3 lines
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.
Quoted 5 lines
Quoted 5 lines
Quoted 2 lines
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).
Quoted 9 lines
Quoted 9 lines
Quoted 6 lines
Quoted 3 lines
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
Quoted 8 lines
Quoted 6 lines
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):
Quoted 6 lines
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);
Quoted 6 lines
Quoted 2 lines
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 --

Quoted 4 lines

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.

Quoted 4 lines

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

--
Kartikeya

Maxim Mikityanskiy

author

2022-02-24 14:29 UTC #14 lore raw

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

Quoted 240 lines

-----Original Message-----
From: Kumar Kartikeya Dwivedi <[email protected]>
Sent: 21 February, 2022 17:22
To: Maxim Mikityanskiy <[email protected]>
Cc: Toke Høiland-Jørgensen <[email protected]>; John Fastabend <[email protected]>; [email protected]; Alexei Starovoitov <[email protected]>; Daniel Borkmann <[email protected]>; Andrii Nakryiko <[email protected]>; [email protected]; Tariq Toukan <[email protected]>; Martin KaFai Lau <[email protected]>; Song Liu <[email protected]>; Yonghong Song <[email protected]>; KP Singh <[email protected]>; David S. Miller <[email protected]>; Jakub Kicinski <[email protected]>; Petar Penkov <[email protected]>; Lorenz Bauer <[email protected]>; Eric Dumazet <[email protected]>; Hideaki YOSHIFUJI <[email protected]>; David Ahern <[email protected]>; Shuah Khan <[email protected]>; Jesper Dangaard Brouer <[email protected]>; Nathan Chancellor <[email protected]>; Nick Desaulniers <[email protected]>; Joe Stringer <[email protected]>; Florent Revest <[email protected]>; linux- [email protected]; Florian Westphal <[email protected]>
Subject: Re: [PATCH bpf-next v2 2/3] bpf: Add helpers to issue and check SYN cookies in XDP
Quoted 10 lines · Maxim Mikityanskiy
Quoted 9 lines · Toke Høiland-Jørgensen
Quoted 7 lines · John Fastabend
Quoted 6 lines · Maxim Mikityanskiy
Quoted 5 lines · John Fastabend
Quoted 4 lines · John Fastabend
Quoted 3 lines · Maxim Mikityanskiy
Quoted 2 lines · John Fastabend
Quoted 1 line · Maxim Mikityanskiy
The new helpers bpf_tcp_raw_{gen,check}_syncookie allow an
XDP program
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
to generate SYN cookies in response to TCP SYN packets and
to check
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
those cookies upon receiving the first ACK packet (the
final packet of
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
the TCP handshake).
Unlike bpf_tcp_{gen,check}_syncookie these new helpers
don't need a
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
listening socket on the local machine, which allows to use
them together
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 5 lines
with synproxy to accelerate SYN cookie generation.
Signed-off-by Maxim Mikityanskiy <[email protected]>
Reviewed-by Tariq Toukan <[email protected]>
---
[...]
Review
Quoted 2 lines
(quoted diff) +2 -0
+
+BPF_CALL_4(bpf_tcp_raw_check_syncookie, void *, iph, u32,
iph_len,
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
(quoted diff) +10 -0
+	   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)
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Review
Quoted 8 lines
(quoted diff) +8 -0
+		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
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Review
Quoted 1 line
(quoted diff) +1 -0
+	 * same offset so we can cast to the shorter header
(struct iphdr).
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Review
Quoted 3 lines
(quoted diff) +3 -0
+	 */
+	switch (((struct iphdr *)iph)->version) {
+	case 4:
Did you consider just exposing __cookie_v4_check() and
__cookie_v6_check()?
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
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
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
would need some wrappers around __cookie_v{4,6}_check anyway.
The checks
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
for th_len and iph_len would have to stay in the helpers. The
check for
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
TCP flags (ACK, !RST, !SYN) could be either in the helper or
in BPF. The
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
switch would obviously be gone.
I'm not sure you would need the len checks in helper, they
provide
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
some guarantees I guess, but the void * is just memory I don't
see
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 2 lines
Quoted 2 lines
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
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
sure that the memory region defined by these two parameters is
valid,
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
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.
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
I suspect we need to add verifier checks here anyways to ensure we
don't
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
walk off the end of a value unless something else is ensuring the
iph
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
is inside a valid memory block.
The verifier ensures that the [iph; iph+iph_len) is valid memory,
but
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
the helper still has to check that struct iphdr fits into this
region.
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Otherwise iph_len could be too small, and the helper would access
memory
Quoted 10 lines
Quoted 10 lines
Quoted 5 lines
Quoted 1 line
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 :)
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
The bottom line is that it would be the same code, but without
the
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
switch, and repeated twice. What benefit do you see in this
approach?
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
The only benefit would be to shave some instructions off the
program.
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
XDP is about performance so I figure we shouldn't be adding
arbitrary
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
stuff here. OTOH you're already jumping into a helper so it
might
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
not matter at all.
Quoted 1 line
From my side, I only see the ability to drop one branch at
the expense
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
of duplicating the code above the switch (th_len and iph_len
checks).
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Just not sure you need the checks either, can you just assume
the user
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
Quoted 1 line
gives good data?
No, since the BPF program would be able to trick the kernel into
reading
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
from an invalid location (see the explanation above).
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
My code at least has already run the code above before it
would ever call
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
this helper so all the other bits are duplicate.
Sorry, I didn't quite understand this part. What "your code"
are you
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
referring to?
Just that the XDP code I maintain has a if ipv4 {...} else
ipv6{...}
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 1 line
Quoted 1 line
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
Quoted 9 lines
Quoted 9 lines
Quoted 5 lines
Quoted 3 lines
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):
Quoted 1 line
We could have some new mem types, PTR_TO_IPV4, PTR_TO_IPv6, and
PTR_TO_TCP.
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Then we simplify the helper signatures to just,
   bpf_tcp_raw_check_syncookie_v4(iph, tcph);
   bpf_tcp_raw_check_syncookie_v6(iph, tcph);
Quoted 8 lines
Quoted 6 lines
Quoted 2 lines
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
Quoted 16 lines
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 --
Quoted 2 lines
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
Quoted 2 lines
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.

Quoted 8 lines

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.

Quoted 8 lines

Quoted 1 line
Alternatively, I can revert to ARG_PTR_TO_MEM and do size checks in
runtime
Quoted 3 lines
in the helper.
[...]
--
Kartikeya

John Fastabend

maintainer

2022-02-04 02:30 UTC #15 lore raw

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

In reply to Maxim Mikityanskiy

Quoted 21 lines · Maxim Mikityanskiy

Quoted 20 lines · John Fastabend
Quoted 12 lines · Maxim Mikityanskiy
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 <[email protected]>
Reviewed-by Tariq Toukan <[email protected]>
---
[...]
Quoted 4 lines
net/core/filter.c +4 -0
+
+BPF_CALL_4(bpf_tcp_raw_check_syncookie, void *, iph, u32, iph_len,
+	   struct tcphdr *, th, u32, th_len)
+{

[...]

Quoted 11 lines

Quoted 2 lines
Quoted 1 line
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.

Quoted 11 lines

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).
Quoted 2 lines
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

author

2022-01-24 15:14 UTC #16 lore raw

PATCH v2 3/3 bpf: Add selftests for raw syncookie helpers

In reply to Maxim Mikityanskiy

This commit adds selftests for the new BPF helpers:
bpf_tcp_raw_gen_syncookie and bpf_tcp_raw_check_syncookie.

xdp_synproxy_kern.c is a BPF program that generates SYN cookies on allowed TCP ports and sends SYNACKs to clients, accelerating synproxy iptables module.

xdp_synproxy.c is a userspace control application that allows to configure the following options in runtime: list of allowed ports, MSS, window scale, TTL.

test_xdp_synproxy.sh is a script that demonstrates the setup of synproxy with XDP acceleration and serves as a selftest for the new feature.

Signed-off-by Maxim Mikityanskiy <[email protected]>

Reviewed-by Tariq Toukan <[email protected]>

---
 tools/testing/selftests/bpf/.gitignore        |   1 +
 tools/testing/selftests/bpf/Makefile          |   5 +-
 .../selftests/bpf/progs/xdp_synproxy_kern.c   | 743 ++++++++++++++++++
 .../selftests/bpf/test_xdp_synproxy.sh        |  71 ++
 tools/testing/selftests/bpf/xdp_synproxy.c    | 418 ++++++++++
 5 files changed, 1236 insertions(+), 2 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/progs/xdp_synproxy_kern.c
 create mode 100755 tools/testing/selftests/bpf/test_xdp_synproxy.sh
 create mode 100644 tools/testing/selftests/bpf/xdp_synproxy.c

tools/testing/selftests/bpf/.gitignore +1 -0

index 1dad8d617da8..1bcffd73632e 100644
@@ -41,3 +41,4 @@ test_cpp
 *.tmp
 xdpxceiver
 xdp_redirect_multi
+xdp_synproxy

tools/testing/selftests/bpf/Makefile +3 -2

index 945f92d71db3..8b9bcb2226da 100644
@@ -72,7 +72,8 @@ TEST_PROGS := test_kmod.sh \
 	test_bpftool.sh \
 	test_bpftool_metadata.sh \
 	test_doc_build.sh \
-	test_xsk.sh
+	test_xsk.sh \
+	test_xdp_synproxy.sh
 
 TEST_PROGS_EXTENDED := with_addr.sh \
 	with_tunnels.sh \

@@ -82,7 +83,7 @@ TEST_PROGS_EXTENDED := with_addr.sh \
 TEST_GEN_PROGS_EXTENDED = test_sock_addr test_skb_cgroup_id_user \
 	flow_dissector_load test_flow_dissector test_tcp_check_syncookie_user \
 	test_lirc_mode2_user xdping test_cpp runqslower bench bpf_testmod.ko \
-	xdpxceiver xdp_redirect_multi
+	xdpxceiver xdp_redirect_multi xdp_synproxy
 
 TEST_CUSTOM_PROGS = $(OUTPUT)/urandom_read

tools/testing/selftests/bpf/progs/xdp_synproxy_kern.c new file +743 -0

743 lines not on this page · raw message

tools/testing/selftests/bpf/test_xdp_synproxy.sh new file +71 -0

71 lines not on this page · raw message

tools/testing/selftests/bpf/xdp_synproxy.c new file +418 -0

418 lines not on this page · raw message

Signature · 1 line

2.30.2