[PATCH bpf 0/3] Fix csum unnecessary on bpf_skb_adjust_room

[PATCH bpf 0/3] Fix csum unnecessary on bpf_skb_adjust_room

[Daniel Borkmann]

This series fixes an issue originally reported by Lorenz Bauer where using
the bpf_skb_adjust_room() helper hid a checksum bug since it wasn't adjusting
CHECKSUM_UNNECESSARY's skb->csum_level after decap. The fix is two-fold:
 i) We do a safe reset in bpf_skb_adjust_room() to CHECKSUM_NONE with an opt-
    out flag BPF_F_ADJ_ROOM_NO_CSUM_RESET.
ii) We add a new bpf_csum_level() for the latter in order to allow users to
    manually inc/dec the skb->csum_level when needed.
The series is rebased against latest bpf-next tree. It can be applied there,
or to bpf after the merge win sync from net-next.

Thanks!

Daniel Borkmann (3):
  bpf: Fix up bpf_skb_adjust_room helper's skb csum setting
  bpf: add csum_level helper for fixing up csum levels
  bpf, selftests: adapt cls_redirect to call csum_level helper

 include/linux/skbuff.h                        |  8 +++
 include/uapi/linux/bpf.h                      | 51 ++++++++++++++++++-
 net/core/filter.c                             | 46 ++++++++++++++++-
 tools/include/uapi/linux/bpf.h                | 51 ++++++++++++++++++-
 .../selftests/bpf/progs/test_cls_redirect.c   |  9 ++--
 5 files changed, 158 insertions(+), 7 deletions(-)

-- 
2.21.0

[PATCH bpf 1/3] bpf: Fix up bpf_skb_adjust_room helper's skb csum setting

[Daniel Borkmann]

Lorenz recently reported:

  In our TC classifier cls_redirect [0], we use the following sequence of
  helper calls to decapsulate a GUE (basically IP + UDP + custom header)
  encapsulated packet:

    bpf_skb_adjust_room(skb, -encap_len, BPF_ADJ_ROOM_MAC, BPF_F_ADJ_ROOM_FIXED_GSO)
    bpf_redirect(skb->ifindex, BPF_F_INGRESS)

  It seems like some checksums of the inner headers are not validated in
  this case. For example, a TCP SYN packet with invalid TCP checksum is
  still accepted by the network stack and elicits a SYN ACK. [...]

  That is, we receive the following packet from the driver:

    | ETH | IP | UDP | GUE | IP | TCP |
    skb->ip_summed == CHECKSUM_UNNECESSARY

  ip_summed is CHECKSUM_UNNECESSARY because our NICs do rx checksum offloading.
  On this packet we run skb_adjust_room_mac(-encap_len), and get the following:

    | ETH | IP | TCP |
    skb->ip_summed == CHECKSUM_UNNECESSARY

  Note that ip_summed is still CHECKSUM_UNNECESSARY. After bpf_redirect()'ing
  into the ingress, we end up in tcp_v4_rcv(). There, skb_checksum_init() is
  turned into a no-op due to CHECKSUM_UNNECESSARY.

The bpf_skb_adjust_room() helper is not aware of protocol specifics. Internally,
it handles the CHECKSUM_COMPLETE case via skb_postpull_rcsum(), but that does
not cover CHECKSUM_UNNECESSARY. In this case skb->csum_level of the original
skb prior to bpf_skb_adjust_room() call was 0, that is, covering UDP. Right now
there is no way to adjust the skb->csum_level. NICs that have checksum offload
disabled (CHECKSUM_NONE) or that support CHECKSUM_COMPLETE are not affected.

Use a safe default for CHECKSUM_UNNECESSARY by resetting to CHECKSUM_NONE and
add a flag to the helper called BPF_F_ADJ_ROOM_NO_CSUM_RESET that allows users
from opting out. Opting out is useful for the case where we don't remove/add
full protocol headers, or for the case where a user wants to adjust the csum
level manually e.g. through bpf_csum_level() helper that is added in subsequent
patch.

The bpf_skb_proto_{4_to_6,6_to_4}() for NAT64/46 translation from the BPF
bpf_skb_change_proto() helper uses bpf_skb_net_hdr_{push,pop}() pair internally
as well but doesn't change layers, only transitions between v4 to v6 and vice
versa, therefore no adoption is required there.

  [0] https://lore.kernel.org/bpf/20200424185556.7358-1-lmb@cloudflare.com/

Fixes: 2be7e212d541 ("bpf: add bpf_skb_adjust_room helper")
Reported-by: Lorenz Bauer <lmb@cloudflare.com>
Reported-by: Alan Maguire <alan.maguire@oracle.com>
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
Reviewed-by: Alan Maguire <alan.maguire@oracle.com>
Link: https://lore.kernel.org/bpf/CACAyw9-uU_52esMd1JjuA80fRPHJv5vsSg8GnfW3t_qDU4aVKQ@mail.gmail.com/
---
 include/linux/skbuff.h         | 8 ++++++++
 include/uapi/linux/bpf.h       | 8 ++++++++
 net/core/filter.c              | 8 ++++++--
 tools/include/uapi/linux/bpf.h | 8 ++++++++
 4 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index a0d5c2760103..0c0377fc00c2 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -3919,6 +3919,14 @@ static inline void __skb_incr_checksum_unnecessary(struct sk_buff *skb)
 	}
 }
 
+static inline void __skb_reset_checksum_unnecessary(struct sk_buff *skb)
+{
+	if (skb->ip_summed == CHECKSUM_UNNECESSARY) {
+		skb->ip_summed = CHECKSUM_NONE;
+		skb->csum_level = 0;
+	}
+}
+
 /* Check if we need to perform checksum complete validation.
  *
  * Returns true if checksum complete is needed, false otherwise
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index b9ed9f14f2a2..3ba2bbbed80c 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -1635,6 +1635,13 @@ union bpf_attr {
  * 		Grow or shrink the room for data in the packet associated to
  * 		*skb* by *len_diff*, and according to the selected *mode*.
  *
+ * 		By default, the helper will reset any offloaded checksum
+ * 		indicator of the skb to CHECKSUM_NONE. This can be avoided
+ * 		by the following flag:
+ *
+ * 		* **BPF_F_ADJ_ROOM_NO_CSUM_RESET**: Do not reset offloaded
+ * 		  checksum data of the skb to CHECKSUM_NONE.
+ *
  *		There are two supported modes at this time:
  *
  *		* **BPF_ADJ_ROOM_MAC**: Adjust room at the mac layer
@@ -3433,6 +3440,7 @@ enum {
 	BPF_F_ADJ_ROOM_ENCAP_L3_IPV6	= (1ULL << 2),
 	BPF_F_ADJ_ROOM_ENCAP_L4_GRE	= (1ULL << 3),
 	BPF_F_ADJ_ROOM_ENCAP_L4_UDP	= (1ULL << 4),
+	BPF_F_ADJ_ROOM_NO_CSUM_RESET	= (1ULL << 5),
 };
 
 enum {
diff --git a/net/core/filter.c b/net/core/filter.c
index ae82bcb03124..278dcc0af961 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -3113,7 +3113,8 @@ static int bpf_skb_net_shrink(struct sk_buff *skb, u32 off, u32 len_diff,
 {
 	int ret;
 
-	if (flags & ~BPF_F_ADJ_ROOM_FIXED_GSO)
+	if (unlikely(flags & ~(BPF_F_ADJ_ROOM_FIXED_GSO |
+			       BPF_F_ADJ_ROOM_NO_CSUM_RESET)))
 		return -EINVAL;
 
 	if (skb_is_gso(skb) && !skb_is_gso_tcp(skb)) {
@@ -3163,7 +3164,8 @@ BPF_CALL_4(bpf_skb_adjust_room, struct sk_buff *, skb, s32, len_diff,
 	u32 off;
 	int ret;
 
-	if (unlikely(flags & ~BPF_F_ADJ_ROOM_MASK))
+	if (unlikely(flags & ~(BPF_F_ADJ_ROOM_MASK |
+			       BPF_F_ADJ_ROOM_NO_CSUM_RESET)))
 		return -EINVAL;
 	if (unlikely(len_diff_abs > 0xfffU))
 		return -EFAULT;
@@ -3191,6 +3193,8 @@ BPF_CALL_4(bpf_skb_adjust_room, struct sk_buff *, skb, s32, len_diff,
 
 	ret = shrink ? bpf_skb_net_shrink(skb, off, len_diff_abs, flags) :
 		       bpf_skb_net_grow(skb, off, len_diff_abs, flags);
+	if (!ret && !(flags & BPF_F_ADJ_ROOM_NO_CSUM_RESET))
+		__skb_reset_checksum_unnecessary(skb);
 
 	bpf_compute_data_pointers(skb);
 	return ret;
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index b9ed9f14f2a2..3ba2bbbed80c 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -1635,6 +1635,13 @@ union bpf_attr {
  * 		Grow or shrink the room for data in the packet associated to
  * 		*skb* by *len_diff*, and according to the selected *mode*.
  *
+ * 		By default, the helper will reset any offloaded checksum
+ * 		indicator of the skb to CHECKSUM_NONE. This can be avoided
+ * 		by the following flag:
+ *
+ * 		* **BPF_F_ADJ_ROOM_NO_CSUM_RESET**: Do not reset offloaded
+ * 		  checksum data of the skb to CHECKSUM_NONE.
+ *
  *		There are two supported modes at this time:
  *
  *		* **BPF_ADJ_ROOM_MAC**: Adjust room at the mac layer
@@ -3433,6 +3440,7 @@ enum {
 	BPF_F_ADJ_ROOM_ENCAP_L3_IPV6	= (1ULL << 2),
 	BPF_F_ADJ_ROOM_ENCAP_L4_GRE	= (1ULL << 3),
 	BPF_F_ADJ_ROOM_ENCAP_L4_UDP	= (1ULL << 4),
+	BPF_F_ADJ_ROOM_NO_CSUM_RESET	= (1ULL << 5),
 };
 
 enum {
-- 
2.21.0

[PATCH bpf 2/3] bpf: Add csum_level helper for fixing up csum levels

[Daniel Borkmann]

Add a bpf_csum_level() helper which BPF programs can use in combination
with bpf_skb_adjust_room() when they pass in BPF_F_ADJ_ROOM_NO_CSUM_RESET
flag to the latter to avoid falling back to CHECKSUM_NONE.

The bpf_csum_level() allows to adjust CHECKSUM_UNNECESSARY skb->csum_levels
via BPF_CSUM_LEVEL_{INC,DEC} which calls __skb_{incr,decr}_checksum_unnecessary()
on the skb. The helper also allows a BPF_CSUM_LEVEL_RESET which sets the skb's
csum to CHECKSUM_NONE as well as a BPF_CSUM_LEVEL_QUERY to just return the
current level. Without this helper, there is no way to otherwise adjust the
skb->csum_level. I did not add an extra dummy flags as there is plenty of free
bitspace in level argument itself iff ever needed in future.

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
---
 include/uapi/linux/bpf.h       | 43 +++++++++++++++++++++++++++++++++-
 net/core/filter.c              | 38 ++++++++++++++++++++++++++++++
 tools/include/uapi/linux/bpf.h | 43 +++++++++++++++++++++++++++++++++-
 3 files changed, 122 insertions(+), 2 deletions(-)

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 3ba2bbbed80c..46622901cba7 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -3220,6 +3220,38 @@ union bpf_attr {
  *		calculation.
  *	Return
  *		Requested value, or 0, if flags are not recognized.
+ *
+ * int bpf_csum_level(struct sk_buff *skb, u64 level)
+ * 	Description
+ * 		Change the skbs checksum level by one layer up or down, or
+ * 		reset it entirely to none in order to have the stack perform
+ * 		checksum validation. The level is applicable to the following
+ * 		protocols: TCP, UDP, GRE, SCTP, FCOE. For example, a decap of
+ * 		| ETH | IP | UDP | GUE | IP | TCP | into | ETH | IP | TCP |
+ * 		through **bpf_skb_adjust_room**\ () helper with passing in
+ * 		**BPF_F_ADJ_ROOM_NO_CSUM_RESET** flag would require one	call
+ * 		to **bpf_csum_level**\ () with **BPF_CSUM_LEVEL_DEC** since
+ * 		the UDP header is removed. Similarly, an encap of the latter
+ * 		into the former could be accompanied by a helper call to
+ * 		**bpf_csum_level**\ () with **BPF_CSUM_LEVEL_INC** if the
+ * 		skb is still intended to be processed in higher layers of the
+ * 		stack instead of just egressing at tc.
+ *
+ * 		There are three supported level settings at this time:
+ *
+ * 		* **BPF_CSUM_LEVEL_INC**: Increases skb->csum_level for skbs
+ * 		  with CHECKSUM_UNNECESSARY.
+ * 		* **BPF_CSUM_LEVEL_DEC**: Decreases skb->csum_level for skbs
+ * 		  with CHECKSUM_UNNECESSARY.
+ * 		* **BPF_CSUM_LEVEL_RESET**: Resets skb->csum_level to 0 and
+ * 		  sets CHECKSUM_NONE to force checksum validation by the stack.
+ * 		* **BPF_CSUM_LEVEL_QUERY**: No-op, returns the current
+ * 		  skb->csum_level.
+ * 	Return
+ * 		0 on success, or a negative error in case of failure. In the
+ * 		case of **BPF_CSUM_LEVEL_QUERY**, the current skb->csum_level
+ * 		is returned or the error code -EACCES in case the skb is not
+ * 		subject to CHECKSUM_UNNECESSARY.
  */
 #define __BPF_FUNC_MAPPER(FN)		\
 	FN(unspec),			\
@@ -3356,7 +3388,8 @@ union bpf_attr {
 	FN(ringbuf_reserve),		\
 	FN(ringbuf_submit),		\
 	FN(ringbuf_discard),		\
-	FN(ringbuf_query),
+	FN(ringbuf_query),		\
+	FN(csum_level),
 
 /* integer value in 'imm' field of BPF_CALL instruction selects which helper
  * function eBPF program intends to call
@@ -3433,6 +3466,14 @@ enum {
 	BPF_F_CURRENT_NETNS		= (-1L),
 };
 
+/* BPF_FUNC_csum_level level values. */
+enum {
+	BPF_CSUM_LEVEL_QUERY,
+	BPF_CSUM_LEVEL_INC,
+	BPF_CSUM_LEVEL_DEC,
+	BPF_CSUM_LEVEL_RESET,
+};
+
 /* BPF_FUNC_skb_adjust_room flags. */
 enum {
 	BPF_F_ADJ_ROOM_FIXED_GSO	= (1ULL << 0),
diff --git a/net/core/filter.c b/net/core/filter.c
index 278dcc0af961..d01a244b5087 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -2015,6 +2015,40 @@ static const struct bpf_func_proto bpf_csum_update_proto = {
 	.arg2_type	= ARG_ANYTHING,
 };
 
+BPF_CALL_2(bpf_csum_level, struct sk_buff *, skb, u64, level)
+{
+	/* The interface is to be used in combination with bpf_skb_adjust_room()
+	 * for encap/decap of packet headers when BPF_F_ADJ_ROOM_NO_CSUM_RESET
+	 * is passed as flags, for example.
+	 */
+	switch (level) {
+	case BPF_CSUM_LEVEL_INC:
+		__skb_incr_checksum_unnecessary(skb);
+		break;
+	case BPF_CSUM_LEVEL_DEC:
+		__skb_decr_checksum_unnecessary(skb);
+		break;
+	case BPF_CSUM_LEVEL_RESET:
+		__skb_reset_checksum_unnecessary(skb);
+		break;
+	case BPF_CSUM_LEVEL_QUERY:
+		return skb->ip_summed == CHECKSUM_UNNECESSARY ?
+		       skb->csum_level : -EACCES;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static const struct bpf_func_proto bpf_csum_level_proto = {
+	.func		= bpf_csum_level,
+	.gpl_only	= false,
+	.ret_type	= RET_INTEGER,
+	.arg1_type	= ARG_PTR_TO_CTX,
+	.arg2_type	= ARG_ANYTHING,
+};
+
 static inline int __bpf_rx_skb(struct net_device *dev, struct sk_buff *skb)
 {
 	return dev_forward_skb(dev, skb);
@@ -6280,6 +6314,8 @@ tc_cls_act_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 		return &bpf_csum_diff_proto;
 	case BPF_FUNC_csum_update:
 		return &bpf_csum_update_proto;
+	case BPF_FUNC_csum_level:
+		return &bpf_csum_level_proto;
 	case BPF_FUNC_l3_csum_replace:
 		return &bpf_l3_csum_replace_proto;
 	case BPF_FUNC_l4_csum_replace:
@@ -6613,6 +6649,8 @@ lwt_xmit_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 		return &bpf_skb_store_bytes_proto;
 	case BPF_FUNC_csum_update:
 		return &bpf_csum_update_proto;
+	case BPF_FUNC_csum_level:
+		return &bpf_csum_level_proto;
 	case BPF_FUNC_l3_csum_replace:
 		return &bpf_l3_csum_replace_proto;
 	case BPF_FUNC_l4_csum_replace:
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 3ba2bbbed80c..46622901cba7 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -3220,6 +3220,38 @@ union bpf_attr {
  *		calculation.
  *	Return
  *		Requested value, or 0, if flags are not recognized.
+ *
+ * int bpf_csum_level(struct sk_buff *skb, u64 level)
+ * 	Description
+ * 		Change the skbs checksum level by one layer up or down, or
+ * 		reset it entirely to none in order to have the stack perform
+ * 		checksum validation. The level is applicable to the following
+ * 		protocols: TCP, UDP, GRE, SCTP, FCOE. For example, a decap of
+ * 		| ETH | IP | UDP | GUE | IP | TCP | into | ETH | IP | TCP |
+ * 		through **bpf_skb_adjust_room**\ () helper with passing in
+ * 		**BPF_F_ADJ_ROOM_NO_CSUM_RESET** flag would require one	call
+ * 		to **bpf_csum_level**\ () with **BPF_CSUM_LEVEL_DEC** since
+ * 		the UDP header is removed. Similarly, an encap of the latter
+ * 		into the former could be accompanied by a helper call to
+ * 		**bpf_csum_level**\ () with **BPF_CSUM_LEVEL_INC** if the
+ * 		skb is still intended to be processed in higher layers of the
+ * 		stack instead of just egressing at tc.
+ *
+ * 		There are three supported level settings at this time:
+ *
+ * 		* **BPF_CSUM_LEVEL_INC**: Increases skb->csum_level for skbs
+ * 		  with CHECKSUM_UNNECESSARY.
+ * 		* **BPF_CSUM_LEVEL_DEC**: Decreases skb->csum_level for skbs
+ * 		  with CHECKSUM_UNNECESSARY.
+ * 		* **BPF_CSUM_LEVEL_RESET**: Resets skb->csum_level to 0 and
+ * 		  sets CHECKSUM_NONE to force checksum validation by the stack.
+ * 		* **BPF_CSUM_LEVEL_QUERY**: No-op, returns the current
+ * 		  skb->csum_level.
+ * 	Return
+ * 		0 on success, or a negative error in case of failure. In the
+ * 		case of **BPF_CSUM_LEVEL_QUERY**, the current skb->csum_level
+ * 		is returned or the error code -EACCES in case the skb is not
+ * 		subject to CHECKSUM_UNNECESSARY.
  */
 #define __BPF_FUNC_MAPPER(FN)		\
 	FN(unspec),			\
@@ -3356,7 +3388,8 @@ union bpf_attr {
 	FN(ringbuf_reserve),		\
 	FN(ringbuf_submit),		\
 	FN(ringbuf_discard),		\
-	FN(ringbuf_query),
+	FN(ringbuf_query),		\
+	FN(csum_level),
 
 /* integer value in 'imm' field of BPF_CALL instruction selects which helper
  * function eBPF program intends to call
@@ -3433,6 +3466,14 @@ enum {
 	BPF_F_CURRENT_NETNS		= (-1L),
 };
 
+/* BPF_FUNC_csum_level level values. */
+enum {
+	BPF_CSUM_LEVEL_QUERY,
+	BPF_CSUM_LEVEL_INC,
+	BPF_CSUM_LEVEL_DEC,
+	BPF_CSUM_LEVEL_RESET,
+};
+
 /* BPF_FUNC_skb_adjust_room flags. */
 enum {
 	BPF_F_ADJ_ROOM_FIXED_GSO	= (1ULL << 0),
-- 
2.21.0

[PATCH bpf 3/3] bpf, selftests: Adapt cls_redirect to call csum_level helper

[Daniel Borkmann]

Adapt bpf_skb_adjust_room() to pass in BPF_F_ADJ_ROOM_NO_CSUM_RESET flag and
use the new bpf_csum_level() helper to inc/dec the checksum level by one after
the encap/decap.

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
---
 tools/testing/selftests/bpf/progs/test_cls_redirect.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/bpf/progs/test_cls_redirect.c b/tools/testing/selftests/bpf/progs/test_cls_redirect.c
index 1668b993eb86..f0b72e86bee5 100644
--- a/tools/testing/selftests/bpf/progs/test_cls_redirect.c
+++ b/tools/testing/selftests/bpf/progs/test_cls_redirect.c
@@ -380,9 +380,10 @@ static ret_t accept_locally(struct __sk_buff *skb, encap_headers_t *encap)
 	}
 
 	if (bpf_skb_adjust_room(skb, -encap_overhead, BPF_ADJ_ROOM_MAC,
-				BPF_F_ADJ_ROOM_FIXED_GSO)) {
+				BPF_F_ADJ_ROOM_FIXED_GSO |
+				BPF_F_ADJ_ROOM_NO_CSUM_RESET) ||
+	    bpf_csum_level(skb, BPF_CSUM_LEVEL_DEC))
 		return TC_ACT_SHOT;
-	}
 
 	return bpf_redirect(skb->ifindex, BPF_F_INGRESS);
 }
@@ -472,7 +473,9 @@ static ret_t forward_with_gre(struct __sk_buff *skb, encap_headers_t *encap,
 	}
 
 	if (bpf_skb_adjust_room(skb, delta, BPF_ADJ_ROOM_NET,
-				BPF_F_ADJ_ROOM_FIXED_GSO)) {
+				BPF_F_ADJ_ROOM_FIXED_GSO |
+				BPF_F_ADJ_ROOM_NO_CSUM_RESET) ||
+	    bpf_csum_level(skb, BPF_CSUM_LEVEL_INC)) {
 		metrics->errors_total_encap_adjust_failed++;
 		return TC_ACT_SHOT;
 	}
-- 
2.21.0

Re: [PATCH bpf 3/3] bpf, selftests: Adapt cls_redirect to call csum_level helper

[Lorenz Bauer]

On Tue, 2 Jun 2020 at 15:58, Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> Adapt bpf_skb_adjust_room() to pass in BPF_F_ADJ_ROOM_NO_CSUM_RESET flag and
> use the new bpf_csum_level() helper to inc/dec the checksum level by one after
> the encap/decap.

Just to be on the safe side: we go from
    | ETH | IP | UDP | GUE | IP | TCP |
to
    | ETH | IP | TCP |
by cutting | IP | UDP | GUE | after the Ethernet header.

Since IP is never included in csum_level and because GUE is not eligible for
CHECKSUM_UNNECESSARY we only need to do csum_level-- once, not twice.

If that is correct:
Reviewed-by: Lorenz Bauer <lmb@cloudflare.com>

>
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> ---
>  tools/testing/selftests/bpf/progs/test_cls_redirect.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/progs/test_cls_redirect.c b/tools/testing/selftests/bpf/progs/test_cls_redirect.c
> index 1668b993eb86..f0b72e86bee5 100644
> --- a/tools/testing/selftests/bpf/progs/test_cls_redirect.c
> +++ b/tools/testing/selftests/bpf/progs/test_cls_redirect.c
> @@ -380,9 +380,10 @@ static ret_t accept_locally(struct __sk_buff *skb, encap_headers_t *encap)
>         }
>
>         if (bpf_skb_adjust_room(skb, -encap_overhead, BPF_ADJ_ROOM_MAC,
> -                               BPF_F_ADJ_ROOM_FIXED_GSO)) {
> +                               BPF_F_ADJ_ROOM_FIXED_GSO |
> +                               BPF_F_ADJ_ROOM_NO_CSUM_RESET) ||
> +           bpf_csum_level(skb, BPF_CSUM_LEVEL_DEC))
>                 return TC_ACT_SHOT;
> -       }
>
>         return bpf_redirect(skb->ifindex, BPF_F_INGRESS);
>  }
> @@ -472,7 +473,9 @@ static ret_t forward_with_gre(struct __sk_buff *skb, encap_headers_t *encap,
>         }
>
>         if (bpf_skb_adjust_room(skb, delta, BPF_ADJ_ROOM_NET,
> -                               BPF_F_ADJ_ROOM_FIXED_GSO)) {
> +                               BPF_F_ADJ_ROOM_FIXED_GSO |
> +                               BPF_F_ADJ_ROOM_NO_CSUM_RESET) ||
> +           bpf_csum_level(skb, BPF_CSUM_LEVEL_INC)) {
>                 metrics->errors_total_encap_adjust_failed++;
>                 return TC_ACT_SHOT;
>         }
> --
> 2.21.0
>


-- 
Lorenz Bauer  |  Systems Engineer
6th Floor, County Hall/The Riverside Building, SE1 7PB, UK

www.cloudflare.com

Re: [PATCH bpf 2/3] bpf: Add csum_level helper for fixing up csum levels

[Lorenz Bauer]

On Tue, 2 Jun 2020 at 15:58, Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> Add a bpf_csum_level() helper which BPF programs can use in combination
> with bpf_skb_adjust_room() when they pass in BPF_F_ADJ_ROOM_NO_CSUM_RESET
> flag to the latter to avoid falling back to CHECKSUM_NONE.
>
> The bpf_csum_level() allows to adjust CHECKSUM_UNNECESSARY skb->csum_levels
> via BPF_CSUM_LEVEL_{INC,DEC} which calls __skb_{incr,decr}_checksum_unnecessary()
> on the skb. The helper also allows a BPF_CSUM_LEVEL_RESET which sets the skb's
> csum to CHECKSUM_NONE as well as a BPF_CSUM_LEVEL_QUERY to just return the
> current level. Without this helper, there is no way to otherwise adjust the
> skb->csum_level. I did not add an extra dummy flags as there is plenty of free
> bitspace in level argument itself iff ever needed in future.
>
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> ---
>  include/uapi/linux/bpf.h       | 43 +++++++++++++++++++++++++++++++++-
>  net/core/filter.c              | 38 ++++++++++++++++++++++++++++++
>  tools/include/uapi/linux/bpf.h | 43 +++++++++++++++++++++++++++++++++-
>  3 files changed, 122 insertions(+), 2 deletions(-)
>
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 3ba2bbbed80c..46622901cba7 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -3220,6 +3220,38 @@ union bpf_attr {
>   *             calculation.
>   *     Return
>   *             Requested value, or 0, if flags are not recognized.
> + *
> + * int bpf_csum_level(struct sk_buff *skb, u64 level)

u64 flags? We can also stuff things into level I guess.

> + *     Description
> + *             Change the skbs checksum level by one layer up or down, or
> + *             reset it entirely to none in order to have the stack perform
> + *             checksum validation. The level is applicable to the following
> + *             protocols: TCP, UDP, GRE, SCTP, FCOE. For example, a decap of
> + *             | ETH | IP | UDP | GUE | IP | TCP | into | ETH | IP | TCP |
> + *             through **bpf_skb_adjust_room**\ () helper with passing in
> + *             **BPF_F_ADJ_ROOM_NO_CSUM_RESET** flag would require one call
> + *             to **bpf_csum_level**\ () with **BPF_CSUM_LEVEL_DEC** since
> + *             the UDP header is removed. Similarly, an encap of the latter
> + *             into the former could be accompanied by a helper call to
> + *             **bpf_csum_level**\ () with **BPF_CSUM_LEVEL_INC** if the
> + *             skb is still intended to be processed in higher layers of the
> + *             stack instead of just egressing at tc.
> + *
> + *             There are three supported level settings at this time:
> + *
> + *             * **BPF_CSUM_LEVEL_INC**: Increases skb->csum_level for skbs
> + *               with CHECKSUM_UNNECESSARY.
> + *             * **BPF_CSUM_LEVEL_DEC**: Decreases skb->csum_level for skbs
> + *               with CHECKSUM_UNNECESSARY.
> + *             * **BPF_CSUM_LEVEL_RESET**: Resets skb->csum_level to 0 and
> + *               sets CHECKSUM_NONE to force checksum validation by the stack.
> + *             * **BPF_CSUM_LEVEL_QUERY**: No-op, returns the current
> + *               skb->csum_level.
> + *     Return
> + *             0 on success, or a negative error in case of failure. In the
> + *             case of **BPF_CSUM_LEVEL_QUERY**, the current skb->csum_level
> + *             is returned or the error code -EACCES in case the skb is not
> + *             subject to CHECKSUM_UNNECESSARY.
>   */
>  #define __BPF_FUNC_MAPPER(FN)          \
>         FN(unspec),                     \
> @@ -3356,7 +3388,8 @@ union bpf_attr {
>         FN(ringbuf_reserve),            \
>         FN(ringbuf_submit),             \
>         FN(ringbuf_discard),            \
> -       FN(ringbuf_query),
> +       FN(ringbuf_query),              \
> +       FN(csum_level),
>
>  /* integer value in 'imm' field of BPF_CALL instruction selects which helper
>   * function eBPF program intends to call
> @@ -3433,6 +3466,14 @@ enum {
>         BPF_F_CURRENT_NETNS             = (-1L),
>  };
>
> +/* BPF_FUNC_csum_level level values. */
> +enum {
> +       BPF_CSUM_LEVEL_QUERY,
> +       BPF_CSUM_LEVEL_INC,
> +       BPF_CSUM_LEVEL_DEC,
> +       BPF_CSUM_LEVEL_RESET,
> +};
> +
>  /* BPF_FUNC_skb_adjust_room flags. */
>  enum {
>         BPF_F_ADJ_ROOM_FIXED_GSO        = (1ULL << 0),
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 278dcc0af961..d01a244b5087 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -2015,6 +2015,40 @@ static const struct bpf_func_proto bpf_csum_update_proto = {
>         .arg2_type      = ARG_ANYTHING,
>  };
>
> +BPF_CALL_2(bpf_csum_level, struct sk_buff *, skb, u64, level)
> +{
> +       /* The interface is to be used in combination with bpf_skb_adjust_room()
> +        * for encap/decap of packet headers when BPF_F_ADJ_ROOM_NO_CSUM_RESET
> +        * is passed as flags, for example.
> +        */
> +       switch (level) {
> +       case BPF_CSUM_LEVEL_INC:
> +               __skb_incr_checksum_unnecessary(skb);
> +               break;
> +       case BPF_CSUM_LEVEL_DEC:
> +               __skb_decr_checksum_unnecessary(skb);
> +               break;
> +       case BPF_CSUM_LEVEL_RESET:
> +               __skb_reset_checksum_unnecessary(skb);
> +               break;
> +       case BPF_CSUM_LEVEL_QUERY:
> +               return skb->ip_summed == CHECKSUM_UNNECESSARY ?
> +                      skb->csum_level : -EACCES;
> +       default:
> +               return -EINVAL;
> +       }
> +
> +       return 0;
> +}
> +
> +static const struct bpf_func_proto bpf_csum_level_proto = {
> +       .func           = bpf_csum_level,
> +       .gpl_only       = false,
> +       .ret_type       = RET_INTEGER,
> +       .arg1_type      = ARG_PTR_TO_CTX,
> +       .arg2_type      = ARG_ANYTHING,
> +};
> +
>  static inline int __bpf_rx_skb(struct net_device *dev, struct sk_buff *skb)
>  {
>         return dev_forward_skb(dev, skb);
> @@ -6280,6 +6314,8 @@ tc_cls_act_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
>                 return &bpf_csum_diff_proto;
>         case BPF_FUNC_csum_update:
>                 return &bpf_csum_update_proto;
> +       case BPF_FUNC_csum_level:
> +               return &bpf_csum_level_proto;
>         case BPF_FUNC_l3_csum_replace:
>                 return &bpf_l3_csum_replace_proto;
>         case BPF_FUNC_l4_csum_replace:
> @@ -6613,6 +6649,8 @@ lwt_xmit_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
>                 return &bpf_skb_store_bytes_proto;
>         case BPF_FUNC_csum_update:
>                 return &bpf_csum_update_proto;
> +       case BPF_FUNC_csum_level:
> +               return &bpf_csum_level_proto;
>         case BPF_FUNC_l3_csum_replace:
>                 return &bpf_l3_csum_replace_proto;
>         case BPF_FUNC_l4_csum_replace:
> diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
> index 3ba2bbbed80c..46622901cba7 100644
> --- a/tools/include/uapi/linux/bpf.h
> +++ b/tools/include/uapi/linux/bpf.h
> @@ -3220,6 +3220,38 @@ union bpf_attr {
>   *             calculation.
>   *     Return
>   *             Requested value, or 0, if flags are not recognized.
> + *
> + * int bpf_csum_level(struct sk_buff *skb, u64 level)
> + *     Description
> + *             Change the skbs checksum level by one layer up or down, or
> + *             reset it entirely to none in order to have the stack perform
> + *             checksum validation. The level is applicable to the following
> + *             protocols: TCP, UDP, GRE, SCTP, FCOE. For example, a decap of
> + *             | ETH | IP | UDP | GUE | IP | TCP | into | ETH | IP | TCP |
> + *             through **bpf_skb_adjust_room**\ () helper with passing in
> + *             **BPF_F_ADJ_ROOM_NO_CSUM_RESET** flag would require one call
> + *             to **bpf_csum_level**\ () with **BPF_CSUM_LEVEL_DEC** since
> + *             the UDP header is removed. Similarly, an encap of the latter
> + *             into the former could be accompanied by a helper call to
> + *             **bpf_csum_level**\ () with **BPF_CSUM_LEVEL_INC** if the
> + *             skb is still intended to be processed in higher layers of the
> + *             stack instead of just egressing at tc.
> + *
> + *             There are three supported level settings at this time:
> + *
> + *             * **BPF_CSUM_LEVEL_INC**: Increases skb->csum_level for skbs
> + *               with CHECKSUM_UNNECESSARY.
> + *             * **BPF_CSUM_LEVEL_DEC**: Decreases skb->csum_level for skbs
> + *               with CHECKSUM_UNNECESSARY.
> + *             * **BPF_CSUM_LEVEL_RESET**: Resets skb->csum_level to 0 and
> + *               sets CHECKSUM_NONE to force checksum validation by the stack.
> + *             * **BPF_CSUM_LEVEL_QUERY**: No-op, returns the current
> + *               skb->csum_level.
> + *     Return
> + *             0 on success, or a negative error in case of failure. In the
> + *             case of **BPF_CSUM_LEVEL_QUERY**, the current skb->csum_level
> + *             is returned or the error code -EACCES in case the skb is not
> + *             subject to CHECKSUM_UNNECESSARY.
>   */
>  #define __BPF_FUNC_MAPPER(FN)          \
>         FN(unspec),                     \
> @@ -3356,7 +3388,8 @@ union bpf_attr {
>         FN(ringbuf_reserve),            \
>         FN(ringbuf_submit),             \
>         FN(ringbuf_discard),            \
> -       FN(ringbuf_query),
> +       FN(ringbuf_query),              \
> +       FN(csum_level),
>
>  /* integer value in 'imm' field of BPF_CALL instruction selects which helper
>   * function eBPF program intends to call
> @@ -3433,6 +3466,14 @@ enum {
>         BPF_F_CURRENT_NETNS             = (-1L),
>  };
>
> +/* BPF_FUNC_csum_level level values. */
> +enum {
> +       BPF_CSUM_LEVEL_QUERY,
> +       BPF_CSUM_LEVEL_INC,
> +       BPF_CSUM_LEVEL_DEC,
> +       BPF_CSUM_LEVEL_RESET,
> +};
> +
>  /* BPF_FUNC_skb_adjust_room flags. */
>  enum {
>         BPF_F_ADJ_ROOM_FIXED_GSO        = (1ULL << 0),
> --
> 2.21.0
>

Acked-by: Lorenz Bauer <lmb@cloudflare.com>

-- 
Lorenz Bauer  |  Systems Engineer
6th Floor, County Hall/The Riverside Building, SE1 7PB, UK

www.cloudflare.com

Re: [PATCH bpf 0/3] Fix csum unnecessary on bpf_skb_adjust_room

[Lorenz Bauer]

On Tue, 2 Jun 2020 at 15:58, Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> This series fixes an issue originally reported by Lorenz Bauer where using
> the bpf_skb_adjust_room() helper hid a checksum bug since it wasn't adjusting
> CHECKSUM_UNNECESSARY's skb->csum_level after decap. The fix is two-fold:
>  i) We do a safe reset in bpf_skb_adjust_room() to CHECKSUM_NONE with an opt-
>     out flag BPF_F_ADJ_ROOM_NO_CSUM_RESET.
> ii) We add a new bpf_csum_level() for the latter in order to allow users to
>     manually inc/dec the skb->csum_level when needed.
> The series is rebased against latest bpf-next tree. It can be applied there,
> or to bpf after the merge win sync from net-next.
>
> Thanks!
>
> Daniel Borkmann (3):
>   bpf: Fix up bpf_skb_adjust_room helper's skb csum setting
>   bpf: add csum_level helper for fixing up csum levels
>   bpf, selftests: adapt cls_redirect to call csum_level helper
>
>  include/linux/skbuff.h                        |  8 +++
>  include/uapi/linux/bpf.h                      | 51 ++++++++++++++++++-
>  net/core/filter.c                             | 46 ++++++++++++++++-
>  tools/include/uapi/linux/bpf.h                | 51 ++++++++++++++++++-
>  .../selftests/bpf/progs/test_cls_redirect.c   |  9 ++--
>  5 files changed, 158 insertions(+), 7 deletions(-)
>
> --
> 2.21.0
>

Thanks for pushing this out, Daniel!

-- 
Lorenz Bauer  |  Systems Engineer
6th Floor, County Hall/The Riverside Building, SE1 7PB, UK

www.cloudflare.com

Re: [PATCH bpf 2/3] bpf: Add csum_level helper for fixing up csum levels

[Daniel Borkmann]

On 6/2/20 5:19 PM, Lorenz Bauer wrote:
> On Tue, 2 Jun 2020 at 15:58, Daniel Borkmann <daniel@iogearbox.net> wrote:
>>
>> Add a bpf_csum_level() helper which BPF programs can use in combination
>> with bpf_skb_adjust_room() when they pass in BPF_F_ADJ_ROOM_NO_CSUM_RESET
>> flag to the latter to avoid falling back to CHECKSUM_NONE.
>>
>> The bpf_csum_level() allows to adjust CHECKSUM_UNNECESSARY skb->csum_levels
>> via BPF_CSUM_LEVEL_{INC,DEC} which calls __skb_{incr,decr}_checksum_unnecessary()
>> on the skb. The helper also allows a BPF_CSUM_LEVEL_RESET which sets the skb's
>> csum to CHECKSUM_NONE as well as a BPF_CSUM_LEVEL_QUERY to just return the
>> current level. Without this helper, there is no way to otherwise adjust the
>> skb->csum_level. I did not add an extra dummy flags as there is plenty of free
>> bitspace in level argument itself iff ever needed in future.
>>
>> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
>> ---
>>   include/uapi/linux/bpf.h       | 43 +++++++++++++++++++++++++++++++++-
>>   net/core/filter.c              | 38 ++++++++++++++++++++++++++++++
>>   tools/include/uapi/linux/bpf.h | 43 +++++++++++++++++++++++++++++++++-
>>   3 files changed, 122 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
>> index 3ba2bbbed80c..46622901cba7 100644
>> --- a/include/uapi/linux/bpf.h
>> +++ b/include/uapi/linux/bpf.h
>> @@ -3220,6 +3220,38 @@ union bpf_attr {
>>    *             calculation.
>>    *     Return
>>    *             Requested value, or 0, if flags are not recognized.
>> + *
>> + * int bpf_csum_level(struct sk_buff *skb, u64 level)
> 
> u64 flags? We can also stuff things into level I guess.

Yeah, I did mention it in the commit log. There is plenty of bit space to extend
with flags in there iff ever needed. Originally, helper was called bpf_csum_adjust()
but then renamed into bpf_csum_level() to be more 'topic specific' (aka do one thing
and do it well...) and avoid future api overloading, so if necessary level can be
used since I don't think the enum will be extended much further from what we have
here anyway.

[...]
> 
> Acked-by: Lorenz Bauer <lmb@cloudflare.com>

Thanks!

Re: [PATCH bpf 3/3] bpf, selftests: Adapt cls_redirect to call csum_level helper

[Daniel Borkmann]

On 6/2/20 5:13 PM, Lorenz Bauer wrote:
> On Tue, 2 Jun 2020 at 15:58, Daniel Borkmann <daniel@iogearbox.net> wrote:
>>
>> Adapt bpf_skb_adjust_room() to pass in BPF_F_ADJ_ROOM_NO_CSUM_RESET flag and
>> use the new bpf_csum_level() helper to inc/dec the checksum level by one after
>> the encap/decap.
> 
> Just to be on the safe side: we go from
>      | ETH | IP | UDP | GUE | IP | TCP |
> to
>      | ETH | IP | TCP |
> by cutting | IP | UDP | GUE | after the Ethernet header.
> 
> Since IP is never included in csum_level and because GUE is not eligible for
> CHECKSUM_UNNECESSARY we only need to do csum_level-- once, not twice.

Yes, that is correct.

Re: [PATCH bpf 2/3] bpf: Add csum_level helper for fixing up csum levels

[Alan Maguire]

On Tue, 2 Jun 2020, Daniel Borkmann wrote:

> On 6/2/20 5:19 PM, Lorenz Bauer wrote:
> > On Tue, 2 Jun 2020 at 15:58, Daniel Borkmann <daniel@iogearbox.net> wrote:
> >>
> >> Add a bpf_csum_level() helper which BPF programs can use in combination
> >> with bpf_skb_adjust_room() when they pass in BPF_F_ADJ_ROOM_NO_CSUM_RESET
> >> flag to the latter to avoid falling back to CHECKSUM_NONE.
> >>
> >> The bpf_csum_level() allows to adjust CHECKSUM_UNNECESSARY skb->csum_levels
> >> via BPF_CSUM_LEVEL_{INC,DEC} which calls
> >> __skb_{incr,decr}_checksum_unnecessary()
> >> on the skb. The helper also allows a BPF_CSUM_LEVEL_RESET which sets the
> >> skb's
> >> csum to CHECKSUM_NONE as well as a BPF_CSUM_LEVEL_QUERY to just return the
> >> current level. Without this helper, there is no way to otherwise adjust the
> >> skb->csum_level. I did not add an extra dummy flags as there is plenty of
> >> free
> >> bitspace in level argument itself iff ever needed in future.
> >>
> >> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> >> ---
> >>   include/uapi/linux/bpf.h       | 43 +++++++++++++++++++++++++++++++++-
> >>   net/core/filter.c              | 38 ++++++++++++++++++++++++++++++
> >>   tools/include/uapi/linux/bpf.h | 43 +++++++++++++++++++++++++++++++++-
> >>   3 files changed, 122 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> >> index 3ba2bbbed80c..46622901cba7 100644
> >> --- a/include/uapi/linux/bpf.h
> >> +++ b/include/uapi/linux/bpf.h
> >> @@ -3220,6 +3220,38 @@ union bpf_attr {
> >>    *             calculation.
> >>    *     Return
> >>    *             Requested value, or 0, if flags are not recognized.
> >> + *
> >> + * int bpf_csum_level(struct sk_buff *skb, u64 level)
> > 
> > u64 flags? We can also stuff things into level I guess.
> 
> Yeah, I did mention it in the commit log. There is plenty of bit space to
> extend
> with flags in there iff ever needed. Originally, helper was called
> bpf_csum_adjust()
> but then renamed into bpf_csum_level() to be more 'topic specific' (aka do one
> thing
> and do it well...) and avoid future api overloading, so if necessary level can
> be
> used since I don't think the enum will be extended much further from what we
> have
> here anyway.
> 
> [...]
> > 
> > Acked-by: Lorenz Bauer <lmb@cloudflare.com>
>

Looks great! The only thing that gave me pause was
the -EACCES return value for the case where we query
and the skb is not subject to CHECKSUM_UNNECESSESARY ;
-ENOENT ("no such level") feels slightly closer to the
situation to me but either is a reasonable choice I think.

Reviewed-by: Alan Maguire <alan.maguire@oracle.com>

Re: [PATCH bpf 2/3] bpf: Add csum_level helper for fixing up csum levels

[Daniel Borkmann]

On 6/2/20 6:41 PM, Alan Maguire wrote:
> On Tue, 2 Jun 2020, Daniel Borkmann wrote:
>> On 6/2/20 5:19 PM, Lorenz Bauer wrote:
>>> On Tue, 2 Jun 2020 at 15:58, Daniel Borkmann <daniel@iogearbox.net> wrote:
>>>>
>>>> Add a bpf_csum_level() helper which BPF programs can use in combination
>>>> with bpf_skb_adjust_room() when they pass in BPF_F_ADJ_ROOM_NO_CSUM_RESET
>>>> flag to the latter to avoid falling back to CHECKSUM_NONE.
>>>>
>>>> The bpf_csum_level() allows to adjust CHECKSUM_UNNECESSARY skb->csum_levels
>>>> via BPF_CSUM_LEVEL_{INC,DEC} which calls
>>>> __skb_{incr,decr}_checksum_unnecessary()
>>>> on the skb. The helper also allows a BPF_CSUM_LEVEL_RESET which sets the
>>>> skb's
>>>> csum to CHECKSUM_NONE as well as a BPF_CSUM_LEVEL_QUERY to just return the
>>>> current level. Without this helper, there is no way to otherwise adjust the
>>>> skb->csum_level. I did not add an extra dummy flags as there is plenty of
>>>> free
>>>> bitspace in level argument itself iff ever needed in future.
>>>>
>>>> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
>>>> ---
>>>>    include/uapi/linux/bpf.h       | 43 +++++++++++++++++++++++++++++++++-
>>>>    net/core/filter.c              | 38 ++++++++++++++++++++++++++++++
>>>>    tools/include/uapi/linux/bpf.h | 43 +++++++++++++++++++++++++++++++++-
>>>>    3 files changed, 122 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
>>>> index 3ba2bbbed80c..46622901cba7 100644
>>>> --- a/include/uapi/linux/bpf.h
>>>> +++ b/include/uapi/linux/bpf.h
>>>> @@ -3220,6 +3220,38 @@ union bpf_attr {
>>>>     *             calculation.
>>>>     *     Return
>>>>     *             Requested value, or 0, if flags are not recognized.
>>>> + *
>>>> + * int bpf_csum_level(struct sk_buff *skb, u64 level)
>>>
>>> u64 flags? We can also stuff things into level I guess.
>>
>> Yeah, I did mention it in the commit log. There is plenty of bit space to
>> extend
>> with flags in there iff ever needed. Originally, helper was called
>> bpf_csum_adjust()
>> but then renamed into bpf_csum_level() to be more 'topic specific' (aka do one
>> thing
>> and do it well...) and avoid future api overloading, so if necessary level can
>> be
>> used since I don't think the enum will be extended much further from what we
>> have
>> here anyway.
>>
>> [...]
>>>
>>> Acked-by: Lorenz Bauer <lmb@cloudflare.com>
> 
> Looks great! The only thing that gave me pause was
> the -EACCES return value for the case where we query
> and the skb is not subject to CHECKSUM_UNNECESSESARY ;
> -ENOENT ("no such level") feels slightly closer to the
> situation to me but either is a reasonable choice I think.

My thinking was in the line of 'error since we cannot access skb->csum_level
for the given skb->ip_summed'. I don't feel strong about which code it is either
way though; important thing is that it is documented & distinguishable from
other errors, so that the program has a way to make sense of the data returned
by BPF_CSUM_LEVEL_QUERY.

> Reviewed-by: Alan Maguire <alan.maguire@oracle.com>

Thanks!
Daniel

Re: [PATCH bpf 0/3] Fix csum unnecessary on bpf_skb_adjust_room

[Alexei Starovoitov]

On Tue, Jun 2, 2020 at 7:58 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> This series fixes an issue originally reported by Lorenz Bauer where using
> the bpf_skb_adjust_room() helper hid a checksum bug since it wasn't adjusting
> CHECKSUM_UNNECESSARY's skb->csum_level after decap. The fix is two-fold:
>  i) We do a safe reset in bpf_skb_adjust_room() to CHECKSUM_NONE with an opt-
>     out flag BPF_F_ADJ_ROOM_NO_CSUM_RESET.
> ii) We add a new bpf_csum_level() for the latter in order to allow users to
>     manually inc/dec the skb->csum_level when needed.
> The series is rebased against latest bpf-next tree. It can be applied there,
> or to bpf after the merge win sync from net-next.

Applied. Thanks