Re: Changing packet fields before redirect


Daniel Borkmann
 

On 01/27/2016 02:56 PM, Ashhad Sheikh via iovisor-dev wrote:
I've tried using bpf_skb_store_bytes() but no luck so far. Actually I'm
trying to implement a router function but when the first packet passes
through a linux bridge it broadcasts an ARP request(since there isn't any
path that exists). I guess that's stopping me to change fields since the
ARP request packet is cloned.
Do you get -EFAULT error from bpf_skb_store_bytes()? What buffer size
do you pass into the function (R4)?

Would something like the below help? Just compile-tested for now, still
needs an audit whether we are always safe with regards to shared skbs
where skb is cloned. (Looks like pskb_expand_head() is called from
__skb_vlan_pop() as well, though.)

Cheers,
Daniel

From 0928ab81f893caf582c20d8efbd0d983f99f7ea5 Mon Sep 17 00:00:00 2001
Message-Id: <0928ab81f893caf582c20d8efbd0d983f99f7ea5.1453906418.git.daniel@...>
From: Daniel Borkmann <daniel@...>
Date: Wed, 27 Jan 2016 11:55:42 +0100
Subject: [PATCH] bpf: try harder on clones when writing into skb

When we're dealing with clones and the area is not writeable, try harder
and get a copy via pskb_expand_head().

Signed-off-by: Daniel Borkmann <daniel@...>
---
include/linux/skbuff.h | 7 +++++++
net/core/filter.c | 21 +++++++++++----------
2 files changed, 18 insertions(+), 10 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 11f935c..0eb5dd7 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -2621,6 +2621,13 @@ static inline int skb_clone_writable(const struct sk_buff *skb, unsigned int len
skb_headroom(skb) + len <= skb->hdr_len;
}

+static inline int skb_try_make_writable(struct sk_buff *skb, int offset,
+ int len)
+{
+ return skb_cloned(skb) && !skb_clone_writable(skb, offset + len) &&
+ pskb_expand_head(skb, 0, 0, GFP_ATOMIC);
+}
+
static inline int __skb_cow(struct sk_buff *skb, unsigned int headroom,
int cloned)
{
diff --git a/net/core/filter.c b/net/core/filter.c
index 6e6bbac..2f5ce50 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -1361,9 +1361,7 @@ static u64 bpf_skb_store_bytes(u64 r1, u64 r2, u64 r3, u64 r4, u64 flags)
*/
if (unlikely((u32) offset > 0xffff || len > sizeof(sp->buff)))
return -EFAULT;
-
- if (unlikely(skb_cloned(skb) &&
- !skb_clone_writable(skb, offset + len)))
+ if (unlikely(skb_try_make_writable(skb, offset, len)))
return -EFAULT;

ptr = skb_header_pointer(skb, offset, len, sp->buff);
@@ -1436,10 +1434,8 @@ static u64 bpf_l3_csum_replace(u64 r1, u64 r2, u64 from, u64 to, u64 flags)
return -EINVAL;
if (unlikely((u32) offset > 0xffff))
return -EFAULT;
-
- if (unlikely(skb_cloned(skb) &&
- !skb_clone_writable(skb, offset + sizeof(sum))))
- return -EFAULT;
+ if (unlikely(skb_try_make_writable(skb, offset, sizeof(sum))))
+ return -EFAULT;

ptr = skb_header_pointer(skb, offset, sizeof(sum), &sum);
if (unlikely(!ptr))
@@ -1485,9 +1481,7 @@ static u64 bpf_l4_csum_replace(u64 r1, u64 r2, u64 from, u64 to, u64 flags)
return -EINVAL;
if (unlikely((u32) offset > 0xffff))
return -EFAULT;
-
- if (unlikely(skb_cloned(skb) &&
- !skb_clone_writable(skb, offset + sizeof(sum))))
+ if (unlikely(skb_try_make_writable(skb, offset, sizeof(sum))))
return -EFAULT;

ptr = skb_header_pointer(skb, offset, sizeof(sum), &sum);
@@ -1737,6 +1731,13 @@ bool bpf_helper_changes_skb_data(void *func)
return true;
if (func == bpf_skb_vlan_pop)
return true;
+ if (func == bpf_skb_store_bytes)
+ return true;
+ if (func == bpf_l3_csum_replace)
+ return true;
+ if (func == bpf_l4_csum_replace)
+ return true;
+
return false;
}

--
1.9.3

Join iovisor-dev@lists.iovisor.org to automatically receive all group messages.