* [PATCH net 1/3] bpf: also call skb_postpush_rcsum on xmit occasions
2016-08-04 0:50 [PATCH net 0/3] Few BPF helper related checksum fixes Daniel Borkmann
@ 2016-08-04 0:50 ` Daniel Borkmann
2016-08-04 0:50 ` [PATCH net 2/3] bpf: fix checksum fixups on bpf_skb_store_bytes Daniel Borkmann
2016-08-04 0:50 ` [PATCH net 3/3] bpf: fix checksum for vlan push/pop helper Daniel Borkmann
2 siblings, 0 replies; 6+ messages in thread
From: Daniel Borkmann @ 2016-08-04 0:50 UTC (permalink / raw)
To: davem; +Cc: alexei.starovoitov, tgraf, netdev, Daniel Borkmann
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH net 2/3] bpf: fix checksum fixups on bpf_skb_store_bytes
2016-08-04 0:50 [PATCH net 0/3] Few BPF helper related checksum fixes Daniel Borkmann
2016-08-04 0:50 ` [PATCH net 1/3] bpf: also call skb_postpush_rcsum on xmit occasions Daniel Borkmann
@ 2016-08-04 0:50 ` Daniel Borkmann
2016-08-04 3:10 ` kbuild test robot
2016-08-04 0:50 ` [PATCH net 3/3] bpf: fix checksum for vlan push/pop helper Daniel Borkmann
2 siblings, 1 reply; 6+ messages in thread
From: Daniel Borkmann @ 2016-08-04 0:50 UTC (permalink / raw)
To: davem; +Cc: alexei.starovoitov, tgraf, netdev, Daniel Borkmann
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH net 3/3] bpf: fix checksum for vlan push/pop helper
2016-08-04 0:50 [PATCH net 0/3] Few BPF helper related checksum fixes Daniel Borkmann
2016-08-04 0:50 ` [PATCH net 1/3] bpf: also call skb_postpush_rcsum on xmit occasions Daniel Borkmann
2016-08-04 0:50 ` [PATCH net 2/3] bpf: fix checksum fixups on bpf_skb_store_bytes Daniel Borkmann
@ 2016-08-04 0:50 ` Daniel Borkmann
2 siblings, 0 replies; 6+ messages in thread
From: Daniel Borkmann @ 2016-08-04 0:50 UTC (permalink / raw)
To: davem; +Cc: alexei.starovoitov, tgraf, netdev, Daniel Borkmann
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
^ permalink raw reply related [flat|nested] 6+ messages in thread