[PATCH bpf-next 0/2] bpf: Update h_proto of ethhdr when the outer protocol changed

[PATCH bpf-next 0/2] bpf: Update h_proto of ethhdr when the outer protocol changed

[Ziyang Xuan]

When use bpf_skb_adjust_room() to encapsulate or decapsulate packet,
and outer protocol changed, we can update h_proto of ethhdr directly.

$./test_tc_tunnel.sh
ipip
encap 192.168.1.1 to 192.168.1.2, type ipip, mac none len 100
test basic connectivity
0
test bpf encap without decap (expect failure)
Ncat: TIMEOUT.
1
test bpf encap with tunnel device decap
0
test bpf encap with bpf decap
0
OK
ipip6
encap 192.168.1.1 to 192.168.1.2, type ipip6, mac none len 100
test basic connectivity
0
test bpf encap without decap (expect failure)
Ncat: TIMEOUT.
1
test bpf encap with tunnel device decap
0
test bpf encap with bpf decap
0
OK
ip6ip6
encap fd::1 to fd::2, type ip6tnl, mac none len 100
test basic connectivity
0
test bpf encap without decap (expect failure)
Ncat: TIMEOUT.
1
test bpf encap with tunnel device decap
0
test bpf encap with bpf decap
0
OK
sit
encap fd::1 to fd::2, type sit, mac none len 100
test basic connectivity
0
test bpf encap without decap (expect failure)
Ncat: TIMEOUT.
1
test bpf encap with tunnel device decap
0
test bpf encap with bpf decap
0
OK
...
OK. All tests passed

Ziyang Xuan (2):
  bpf: Update h_proto of ethhdr when the outer protocol changed
  selftests/bpf: Remove unnecessary codes for updating h_proto of ethhdr

 net/core/filter.c                             | 20 +++++++++++++------
 .../selftests/bpf/progs/test_tc_tunnel.c      | 18 -----------------
 2 files changed, 14 insertions(+), 24 deletions(-)

-- 
2.25.1

[PATCH bpf-next 1/2] bpf: Update h_proto of ethhdr when the outer protocol changed

[Ziyang Xuan]

When use bpf_skb_adjust_room() to encapsulate or decapsulate packet,
and outer protocol changed, we can update h_proto of ethhdr directly.

Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com>
---
 net/core/filter.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/net/core/filter.c b/net/core/filter.c
index a094694899c9..0c156550c757 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -3409,6 +3409,14 @@ static u32 bpf_skb_net_base_len(const struct sk_buff *skb)
 					  BPF_ADJ_ROOM_ENCAP_L2_MASK) | \
 					 BPF_F_ADJ_ROOM_DECAP_L3_MASK)
 
+static inline void skb_update_protocol(struct sk_buff *skb, __be16 protocol)
+{
+	struct ethhdr *hdr = eth_hdr(skb);
+
+	hdr->h_proto = protocol;
+	skb->protocol = protocol;
+}
+
 static int bpf_skb_net_grow(struct sk_buff *skb, u32 off, u32 len_diff,
 			    u64 flags)
 {
@@ -3491,13 +3499,13 @@ static int bpf_skb_net_grow(struct sk_buff *skb, u32 off, u32 len_diff,
 			skb_set_transport_header(skb, mac_len + nh_len);
 		}
 
-		/* Match skb->protocol to new outer l3 protocol */
+		/* Match skb->protocol and ethhdr->h_proto to new outer l3 protocol */
 		if (skb->protocol == htons(ETH_P_IP) &&
 		    flags & BPF_F_ADJ_ROOM_ENCAP_L3_IPV6)
-			skb->protocol = htons(ETH_P_IPV6);
+			skb_update_protocol(skb, htons(ETH_P_IPV6));
 		else if (skb->protocol == htons(ETH_P_IPV6) &&
 			 flags & BPF_F_ADJ_ROOM_ENCAP_L3_IPV4)
-			skb->protocol = htons(ETH_P_IP);
+			skb_update_protocol(skb, htons(ETH_P_IP));
 	}
 
 	if (skb_is_gso(skb)) {
@@ -3540,13 +3548,13 @@ static int bpf_skb_net_shrink(struct sk_buff *skb, u32 off, u32 len_diff,
 	if (unlikely(ret < 0))
 		return ret;
 
-	/* Match skb->protocol to new outer l3 protocol */
+	/* Match skb->protocol and ethhdr->h_proto to new outer l3 protocol */
 	if (skb->protocol == htons(ETH_P_IP) &&
 	    flags & BPF_F_ADJ_ROOM_DECAP_L3_IPV6)
-		skb->protocol = htons(ETH_P_IPV6);
+		skb_update_protocol(skb, htons(ETH_P_IPV6));
 	else if (skb->protocol == htons(ETH_P_IPV6) &&
 		 flags & BPF_F_ADJ_ROOM_DECAP_L3_IPV4)
-		skb->protocol = htons(ETH_P_IP);
+		skb_update_protocol(skb, htons(ETH_P_IP));
 
 	if (skb_is_gso(skb)) {
 		struct skb_shared_info *shinfo = skb_shinfo(skb);
-- 
2.25.1

[PATCH bpf-next 2/2] selftests/bpf: Remove unnecessary codes for updating h_proto of ethhdr

[Ziyang Xuan]

Since updating h_proto of ethhdr in kernel, remove the codes
in user bpf program.

Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com>
---
 .../selftests/bpf/progs/test_tc_tunnel.c       | 18 ------------------
 1 file changed, 18 deletions(-)

diff --git a/tools/testing/selftests/bpf/progs/test_tc_tunnel.c b/tools/testing/selftests/bpf/progs/test_tc_tunnel.c
index e6e678aa9874..a33be22a2dc4 100644
--- a/tools/testing/selftests/bpf/progs/test_tc_tunnel.c
+++ b/tools/testing/selftests/bpf/progs/test_tc_tunnel.c
@@ -236,17 +236,6 @@ static __always_inline int __encap_ipv4(struct __sk_buff *skb, __u8 encap_proto,
 				BPF_F_INVALIDATE_HASH) < 0)
 		return TC_ACT_SHOT;
 
-	/* if changing outer proto type, update eth->h_proto */
-	if (encap_proto == IPPROTO_IPV6) {
-		struct ethhdr eth;
-
-		if (bpf_skb_load_bytes(skb, 0, &eth, sizeof(eth)) < 0)
-			return TC_ACT_SHOT;
-		eth.h_proto = bpf_htons(ETH_P_IP);
-		if (bpf_skb_store_bytes(skb, 0, &eth, sizeof(eth), 0) < 0)
-			return TC_ACT_SHOT;
-	}
-
 	return TC_ACT_OK;
 }
 
@@ -412,13 +401,6 @@ static int encap_ipv6_ipip6(struct __sk_buff *skb)
 				BPF_F_INVALIDATE_HASH) < 0)
 		return TC_ACT_SHOT;
 
-	/* update eth->h_proto */
-	if (bpf_skb_load_bytes(skb, 0, &eth, sizeof(eth)) < 0)
-		return TC_ACT_SHOT;
-	eth.h_proto = bpf_htons(ETH_P_IPV6);
-	if (bpf_skb_store_bytes(skb, 0, &eth, sizeof(eth), 0) < 0)
-		return TC_ACT_SHOT;
-
 	return TC_ACT_OK;
 }
 
-- 
2.25.1

Re: [PATCH bpf-next 0/2] bpf: Update h_proto of ethhdr when the outer protocol changed

[Yonghong Song]

On 8/9/23 11:25 PM, Ziyang Xuan wrote:
> When use bpf_skb_adjust_room() to encapsulate or decapsulate packet,
> and outer protocol changed, we can update h_proto of ethhdr directly.

My mailbox somehow lost patch 1/2.

Looks like current bpf_skb_adjust_room() only changes skb meta data and
tries not to modify the packet. Probably there is a reason for this.

> 
> $./test_tc_tunnel.sh
> ipip
> encap 192.168.1.1 to 192.168.1.2, type ipip, mac none len 100
> test basic connectivity
> 0
> test bpf encap without decap (expect failure)
> Ncat: TIMEOUT.
> 1
> test bpf encap with tunnel device decap
> 0
> test bpf encap with bpf decap
> 0
> OK
> ipip6
[...]

Re: [PATCH bpf-next 1/2] bpf: Update h_proto of ethhdr when the outer protocol changed

[Martin KaFai Lau]

On 8/9/23 11:25 PM, Ziyang Xuan wrote:
> When use bpf_skb_adjust_room() to encapsulate or decapsulate packet,
> and outer protocol changed, we can update h_proto of ethhdr directly.

This could break some existing bpf programs. e.g what if the existing prog is 
testing the h_proto after bpf_skb_adjust_room() and expect it hasn't changed yet?

Re: [PATCH bpf-next 0/2] bpf: Update h_proto of ethhdr when the outer protocol changed

[Ziyang Xuan (William)]

> On 8/9/23 11:25 PM, Ziyang Xuan wrote:
>> When use bpf_skb_adjust_room() to encapsulate or decapsulate packet,
>> and outer protocol changed, we can update h_proto of ethhdr directly.
> 
> My mailbox somehow lost patch 1/2.
> 
> Looks like current bpf_skb_adjust_room() only changes skb meta data and
> tries not to modify the packet. Probably there is a reason for this.
> 
I found that this could reduce updating h_proto of ethhdr by bpf_skb_store_bytes()
in bpf program when I used bpf_skb_adjust_room(). I thought it should be a small
optimization. So I did it. No other problems.

Thank you!
William Xuan
>>
>> $./test_tc_tunnel.sh
>> ipip
>> encap 192.168.1.1 to 192.168.1.2, type ipip, mac none len 100
>> test basic connectivity
>> 0
>> test bpf encap without decap (expect failure)
>> Ncat: TIMEOUT.
>> 1
>> test bpf encap with tunnel device decap
>> 0
>> test bpf encap with bpf decap
>> 0
>> OK
>> ipip6
> [...]
> 
> .

Re: [PATCH bpf-next 1/2] bpf: Update h_proto of ethhdr when the outer protocol changed

[Ziyang Xuan (William)]

> On 8/9/23 11:25 PM, Ziyang Xuan wrote:
>> When use bpf_skb_adjust_room() to encapsulate or decapsulate packet,
>> and outer protocol changed, we can update h_proto of ethhdr directly.
> 
> This could break some existing bpf programs. e.g what if the existing prog is testing the h_proto after bpf_skb_adjust_room() and expect it hasn't changed yet?
> 
I think some new modifications break some existing things are not unacceptable.
Maybe my modification is inappropriate because its benefits are small and
some kind of principle is broken, such as Yonghong Song pointed:
"bpf_skb_adjust_room() only changes skb meta data and tries not to modify the packet."
If it is, the modification should be rejected.

Thank you!
William Xuan
> 
> .