[PATCH net 0/3] Few BPF helper related checksum fixes

The set contains three fixes with regards to CHECKSUM_COMPLETE
and BPF helper functions. For details please see individual
patches.

Thanks!

Daniel Borkmann (3):
  bpf: also call skb_postpush_rcsum on xmit occasions
  bpf: fix checksum fixups on bpf_skb_store_bytes
  bpf: fix checksum for vlan push/pop helper

 include/linux/skbuff.h | 38 ++++++++++++++++++++++----------------
 net/core/filter.c      | 29 ++++++++++++++++++++++++-----
 2 files changed, 46 insertions(+), 21 deletions(-)

-- 
1.9.3

Follow-up to commit f8ffad69c9f8 ("bpf: add skb_postpush_rcsum and fix
dev_forward_skb occasions") to fix an issue for dev_queue_xmit() redirect
locations which need CHECKSUM_COMPLETE fixups on ingress.

For the same reasons as described in f8ffad69c9f8 already, we of course
also need this here, since dev_queue_xmit() on a veth device will let us
end up in the dev_forward_skb() helper again to cross namespaces.

Latter then calls into skb_postpull_rcsum() to pull out L2 header, so
that netif_rx_internal() sees CHECKSUM_COMPLETE as it is expected. That
is, CHECKSUM_COMPLETE on ingress covering L2 _payload_, not L2 headers.

Also here we have to address bpf_redirect() and bpf_clone_redirect().

Fixes: 3896d655f4d4 ("bpf: introduce bpf_clone_redirect() helper")
Fixes: 27b29f63058d ("bpf: add bpf_redirect() helper")
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Acked-by: Alexei Starovoitov <ast@kernel.org>
---
 net/core/filter.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/net/core/filter.c b/net/core/filter.c
index 5708999..c46244f 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c

@@ -1365,6 +1365,12 @@ static inline int bpf_try_make_writable(struct sk_buff *skb,
 	return err;
 }
 
+static inline void bpf_push_mac_rcsum(struct sk_buff *skb)
+{
+	if (skb_at_tc_ingress(skb))
+		skb_postpush_rcsum(skb, skb_mac_header(skb), skb->mac_len);
+}
+
 static u64 bpf_skb_store_bytes(u64 r1, u64 r2, u64 r3, u64 r4, u64 flags)
 {
 	struct bpf_scratchpad *sp = this_cpu_ptr(&bpf_sp);

@@ -1607,9 +1613,6 @@ static const struct bpf_func_proto bpf_csum_diff_proto = {
 
 static inline int __bpf_rx_skb(struct net_device *dev, struct sk_buff *skb)
 {
-	if (skb_at_tc_ingress(skb))
-		skb_postpush_rcsum(skb, skb_mac_header(skb), skb->mac_len);
-
 	return dev_forward_skb(dev, skb);
 }
 

@@ -1648,6 +1651,8 @@ static u64 bpf_clone_redirect(u64 r1, u64 ifindex, u64 flags, u64 r4, u64 r5)
 	if (unlikely(!skb))
 		return -ENOMEM;
 
+	bpf_push_mac_rcsum(skb);
+
 	return flags & BPF_F_INGRESS ?
 	       __bpf_rx_skb(dev, skb) : __bpf_tx_skb(dev, skb);
 }

@@ -1693,6 +1698,8 @@ int skb_do_redirect(struct sk_buff *skb)
 		return -EINVAL;
 	}
 
+	bpf_push_mac_rcsum(skb);
+
 	return ri->flags & BPF_F_INGRESS ?
 	       __bpf_rx_skb(dev, skb) : __bpf_tx_skb(dev, skb);
 }

-- 
1.9.3

bpf_skb_store_bytes() invocations above L2 header need BPF_F_RECOMPUTE_CSUM
flag for updates, so that CHECKSUM_COMPLETE will be fixed up along the way.
Only other exception besides L2 header where BPF_F_RECOMPUTE_CSUM is not
passed to bpf_skb_store_bytes() are packet changes affecting pseudo headers
where we use bpf_l4_csum_replace() with BPF_F_PSEUDO_HDR flag that will
implicitly take care of CHECKSUM_COMPLETE.

Where we ran into an issue with bpf_skb_store_bytes() is when we did a
single-byte update on the IPv6 hoplimit despite using BPF_F_RECOMPUTE_CSUM
flag; simple ping via ICMPv6 triggered a hw csum failure as a result. The
underlying issue has been tracked down to a buffer alignment issue.

Meaning, that csum_partial() computations via skb_postpull_rcsum() and
skb_postpush_rcsum() pair invoked (due to passing BPF_F_PSEUDO_HDR flag)
had a wrong result since they operated on an odd address for the hoplimit,
while other computations were done on an even address. This mix doesn't
work as-is with skb_postpull_rcsum(), skb_postpush_rcsum() pair as it always
expects at least half-word alignment of input buffers, which is normally
the case. Thus, instead of these helpers using csum_sub() and (implicitly)
csum_add(), we need to use csum_block_sub(), csum_block_add(), respectively.
For unaligned offsets, they rotate the sum to align it to a half-word
boundary again, otherwise they do the same as csum_sub() and csum_add().

Adding __skb_postpull_rcsum(), __skb_postpush_rcsum() variants that take
the offset as an input and adapting bpf_skb_store_bytes() to them fixes
the hw csum failures again. The skb_postpull_rcsum(), skb_postpush_rcsum()
helpers use a 0 constant for offset so that the compiler optimizes the
offset & 1 test away and generates the same code as with csum_sub()/_add().

Fixes: 608cd71a9c7c ("tc: bpf: generalize pedit action")
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Acked-by: Alexei Starovoitov <ast@kernel.org>
---
 include/linux/skbuff.h | 38 ++++++++++++++++++++++----------------
 net/core/filter.c      |  4 ++--
 2 files changed, 24 insertions(+), 18 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 6f0b3e0..5f8c2fa 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h

@@ -2858,35 +2858,41 @@ static inline int skb_linearize_cow(struct sk_buff *skb)
  *	CHECKSUM_NONE so that it can be recomputed from scratch.
  */
 
-static inline void skb_postpull_rcsum(struct sk_buff *skb,
-				      const void *start, unsigned int len)
+static __always_inline void
+__skb_postpull_rcsum(struct sk_buff *skb, const void *start, unsigned int len,
+		     unsigned int off)
 {
 	if (skb->ip_summed == CHECKSUM_COMPLETE)
-		skb->csum = csum_sub(skb->csum, csum_partial(start, len, 0));
+		skb->csum = csum_block_sub(skb->csum,
+					   csum_partial(start, len, 0), off);
 	else if (skb->ip_summed == CHECKSUM_PARTIAL &&
 		 skb_checksum_start_offset(skb) < 0)
 		skb->ip_summed = CHECKSUM_NONE;
 }
 
-unsigned char *skb_pull_rcsum(struct sk_buff *skb, unsigned int len);
+static inline void skb_postpull_rcsum(struct sk_buff *skb,
+				      const void *start, unsigned int len)
+{
+	__skb_postpull_rcsum(skb, start, len, 0);
+}
+
+static __always_inline void
+__skb_postpush_rcsum(struct sk_buff *skb, const void *start, unsigned int len,
+		     unsigned int off)
+{
+	if (skb->ip_summed == CHECKSUM_COMPLETE)
+		skb->csum = csum_block_add(skb->csum,
+					   csum_partial(start, len, 0), off);
+}
 
 static inline void skb_postpush_rcsum(struct sk_buff *skb,
 				      const void *start, unsigned int len)
 {
-	/* For performing the reverse operation to skb_postpull_rcsum(),
-	 * we can instead of ...
-	 *
-	 *   skb->csum = csum_add(skb->csum, csum_partial(start, len, 0));
-	 *
-	 * ... just use this equivalent version here to save a few
-	 * instructions. Feeding csum of 0 in csum_partial() and later
-	 * on adding skb->csum is equivalent to feed skb->csum in the
-	 * first place.
-	 */
-	if (skb->ip_summed == CHECKSUM_COMPLETE)
-		skb->csum = csum_partial(start, len, skb->csum);
+	__skb_postpush_rcsum(skb, start, len, 0);
 }
 
+unsigned char *skb_pull_rcsum(struct sk_buff *skb, unsigned int len);
+
 /**
  *	skb_push_rcsum - push skb and update receive checksum
  *	@skb: buffer to update

diff --git a/net/core/filter.c b/net/core/filter.c
index c46244f..5ecd5c9 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c

@@ -1401,7 +1401,7 @@ static u64 bpf_skb_store_bytes(u64 r1, u64 r2, u64 r3, u64 r4, u64 flags)
 		return -EFAULT;
 
 	if (flags & BPF_F_RECOMPUTE_CSUM)
-		skb_postpull_rcsum(skb, ptr, len);
+		__skb_postpull_rcsum(skb, ptr, len, offset);
 
 	memcpy(ptr, from, len);
 

@@ -1410,7 +1410,7 @@ static u64 bpf_skb_store_bytes(u64 r1, u64 r2, u64 r3, u64 r4, u64 flags)
 		skb_store_bits(skb, offset, ptr, len);
 
 	if (flags & BPF_F_RECOMPUTE_CSUM)
-		skb_postpush_rcsum(skb, ptr, len);
+		__skb_postpush_rcsum(skb, ptr, len, offset);
 	if (flags & BPF_F_INVALIDATE_HASH)
 		skb_clear_hash(skb);
 

-- 
1.9.3

When having skbs on ingress with CHECKSUM_COMPLETE, tc BPF programs don't
push rcsum of mac header back in and after BPF run back pull out again as
opposed to some other subsystems (ovs, for example).

For cases like q-in-q, meaning when a vlan tag for offloading is already
present and we're about to push another one, then skb_vlan_push() pushes the
inner one into the skb, increasing mac header and skb_postpush_rcsum()'ing
the 4 bytes vlan header diff. Likewise, for the reverse operation in
skb_vlan_pop() for the case where vlan header needs to be pulled out of the
skb, we're decreasing the mac header and skb_postpull_rcsum()'ing the 4 bytes
rcsum of the vlan header that was removed.

However mangling the rcsum here will lead to hw csum failure for BPF case,
since we're pulling or pushing data that was not part of the current rcsum.
Changing tc BPF programs in general to push/pull rcsum around BPF_PROG_RUN()
is also not really an option since current behaviour is ABI by now, but apart
from that would also mean to do quite a bit of useless work in the sense that
usually 12 bytes need to be rcsum pushed/pulled also when we don't need to
touch this vlan related corner case. One way to fix it would be to push the
necessary rcsum fixup down into vlan helpers that are (mostly) slow-path
anyway.

Fixes: 4e10df9a60d9 ("bpf: introduce bpf_skb_vlan_push/pop() helpers")
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Acked-by: Alexei Starovoitov <ast@kernel.org>
---
 net/core/filter.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/net/core/filter.c b/net/core/filter.c
index 5ecd5c9..b5add4e 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c

@@ -1371,6 +1371,12 @@ static inline void bpf_push_mac_rcsum(struct sk_buff *skb)
 		skb_postpush_rcsum(skb, skb_mac_header(skb), skb->mac_len);
 }
 
+static inline void bpf_pull_mac_rcsum(struct sk_buff *skb)
+{
+	if (skb_at_tc_ingress(skb))
+		skb_postpull_rcsum(skb, skb_mac_header(skb), skb->mac_len);
+}
+
 static u64 bpf_skb_store_bytes(u64 r1, u64 r2, u64 r3, u64 r4, u64 flags)
 {
 	struct bpf_scratchpad *sp = this_cpu_ptr(&bpf_sp);

@@ -1763,7 +1769,10 @@ static u64 bpf_skb_vlan_push(u64 r1, u64 r2, u64 vlan_tci, u64 r4, u64 r5)
 		     vlan_proto != htons(ETH_P_8021AD)))
 		vlan_proto = htons(ETH_P_8021Q);
 
+	bpf_push_mac_rcsum(skb);
 	ret = skb_vlan_push(skb, vlan_proto, vlan_tci);
+	bpf_pull_mac_rcsum(skb);
+
 	bpf_compute_data_end(skb);
 	return ret;
 }

@@ -1783,7 +1792,10 @@ static u64 bpf_skb_vlan_pop(u64 r1, u64 r2, u64 r3, u64 r4, u64 r5)
 	struct sk_buff *skb = (struct sk_buff *) (long) r1;
 	int ret;
 
+	bpf_push_mac_rcsum(skb);
 	ret = skb_vlan_pop(skb);
+	bpf_pull_mac_rcsum(skb);
+
 	bpf_compute_data_end(skb);
 	return ret;
 }

-- 
1.9.3

Hi Daniel,

[auto build test WARNING on net/master]

url:    https://github.com/0day-ci/linux/commits/Daniel-Borkmann/Few-BPF-helper-related-checksum-fixes/20160804-085300
reproduce: make htmldocs

All warnings (new ones prefixed by >>):

   include/linux/skbuff.h:940: warning: No description found for parameter 'sk'

quoted

include/linux/skbuff.h:2864: warning: No description found for parameter 'off'

   include/net/sock.h:449: warning: No description found for parameter 'sk_padding'
   include/net/sock.h:449: warning: No description found for parameter 'sk_rcu'
   include/linux/netdevice.h:1889: warning: No description found for parameter 'prio_tc_map[TC_BITMASK + 1]'

vim +/off +2864 include/linux/skbuff.h

^1da177e Linus Torvalds  2005-04-16  2848  }
^1da177e Linus Torvalds  2005-04-16  2849  
^1da177e Linus Torvalds  2005-04-16  2850  /**
^1da177e Linus Torvalds  2005-04-16  2851   *	skb_postpull_rcsum - update checksum for received skb after pull
^1da177e Linus Torvalds  2005-04-16  2852   *	@skb: buffer to update
^1da177e Linus Torvalds  2005-04-16  2853   *	@start: start of data before pull
^1da177e Linus Torvalds  2005-04-16  2854   *	@len: length of data pulled
^1da177e Linus Torvalds  2005-04-16  2855   *
^1da177e Linus Torvalds  2005-04-16  2856   *	After doing a pull on a received packet, you need to call this to
84fa7933 Patrick McHardy 2006-08-29  2857   *	update the CHECKSUM_COMPLETE checksum, or set ip_summed to
84fa7933 Patrick McHardy 2006-08-29  2858   *	CHECKSUM_NONE so that it can be recomputed from scratch.
^1da177e Linus Torvalds  2005-04-16  2859   */
^1da177e Linus Torvalds  2005-04-16  2860  
1716d701 Daniel Borkmann 2016-08-04  2861  static __always_inline void
1716d701 Daniel Borkmann 2016-08-04  2862  __skb_postpull_rcsum(struct sk_buff *skb, const void *start, unsigned int len,
1716d701 Daniel Borkmann 2016-08-04  2863  		     unsigned int off)
^1da177e Linus Torvalds  2005-04-16 @2864  {
84fa7933 Patrick McHardy 2006-08-29  2865  	if (skb->ip_summed == CHECKSUM_COMPLETE)
1716d701 Daniel Borkmann 2016-08-04  2866  		skb->csum = csum_block_sub(skb->csum,
1716d701 Daniel Borkmann 2016-08-04  2867  					   csum_partial(start, len, 0), off);
6ae459bd Pravin B Shelar 2015-09-22  2868  	else if (skb->ip_summed == CHECKSUM_PARTIAL &&
31b33dfb Pravin B Shelar 2015-09-28  2869  		 skb_checksum_start_offset(skb) < 0)
6ae459bd Pravin B Shelar 2015-09-22  2870  		skb->ip_summed = CHECKSUM_NONE;
^1da177e Linus Torvalds  2005-04-16  2871  }
^1da177e Linus Torvalds  2005-04-16  2872  

:::::: The code at line 2864 was first introduced by commit
:::::: 1da177e4c3f41524e886b7f1b8a0c1fc7321cac2 Linux-2.6.12-rc2

:::::: TO: Linus Torvalds [off-list ref]
:::::: CC: Linus Torvalds [off-list ref]

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

On 08/04/2016 05:10 AM, kbuild test robot wrote:

Hi Daniel,

[auto build test WARNING on net/master]

url:    https://github.com/0day-ci/linux/commits/Daniel-Borkmann/Few-BPF-helper-related-checksum-fixes/20160804-085300
reproduce: make htmldocs

All warnings (new ones prefixed by >>):

    include/linux/skbuff.h:940: warning: No description found for parameter 'sk'

quoted

quoted

include/linux/skbuff.h:2864: warning: No description found for parameter 'off'

    include/net/sock.h:449: warning: No description found for parameter 'sk_padding'
    include/net/sock.h:449: warning: No description found for parameter 'sk_rcu'
    include/linux/netdevice.h:1889: warning: No description found for parameter 'prio_tc_map[TC_BITMASK + 1]'

vim +/off +2864 include/linux/skbuff.h

^1da177e Linus Torvalds  2005-04-16  2848  }
^1da177e Linus Torvalds  2005-04-16  2849
^1da177e Linus Torvalds  2005-04-16  2850  /**
^1da177e Linus Torvalds  2005-04-16  2851   *	skb_postpull_rcsum - update checksum for received skb after pull
^1da177e Linus Torvalds  2005-04-16  2852   *	@skb: buffer to update
^1da177e Linus Torvalds  2005-04-16  2853   *	@start: start of data before pull
^1da177e Linus Torvalds  2005-04-16  2854   *	@len: length of data pulled
^1da177e Linus Torvalds  2005-04-16  2855   *
^1da177e Linus Torvalds  2005-04-16  2856   *	After doing a pull on a received packet, you need to call this to
84fa7933 Patrick McHardy 2006-08-29  2857   *	update the CHECKSUM_COMPLETE checksum, or set ip_summed to
84fa7933 Patrick McHardy 2006-08-29  2858   *	CHECKSUM_NONE so that it can be recomputed from scratch.
^1da177e Linus Torvalds  2005-04-16  2859   */

Alright, I move the kdoc under skb_postpull_rcsum() and also add one for
skb_postpush_rcsum() in a v2 of this set. I didn't touch it because of -net
tree, but since someone might be using 'make htmldocs' it shouldn't generate
additional warnings. So, will respin with v2!