test_bpf: Introduce an skb_segment test for frag_list whose head_frag=true and gso_size was mangled

PATCH In review Fred Li 6 participants 23 messages Jun 26, 2024 – Jul 25, 2024

Subsystems (5)

Networking Network drivers Library code BPF +1 more subsystems

lore

Share & tools

lore

Fred Li

author

2024-06-26 06:56 UTC #0 lore raw

PATCH v2 2/2 test_bpf: Introduce an skb_segment test for frag_list whose head_frag=true and gso_size was mangled

This is a reproducer test that mimics the input skbs that lead to the mentioned BUG_ON and validates the fix submitted in patch 1.

Signed-off-by Fred Li <[email protected]>

---
 lib/test_bpf.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 64 insertions(+)

lib/test_bpf.c +64 -0

index ecde4216201e..a38d2d09ca01 100644
@@ -14706,6 +14706,63 @@ static __init struct sk_buff *build_test_skb_linear_no_head_frag(void)
 	return NULL;
 }
 
+static __init struct sk_buff *build_test_skb_head_frag(void)
+{
+	u32 headroom = 192, doffset = 66, alloc_size = 1536;
+	struct sk_buff *skb[2];
+	struct page *page[17];
+	int i, data_size = 125;
+	int j;
+
+	skb[0] = dev_alloc_skb(headroom + alloc_size);
+	if (!skb[0])
+		return NULL;
+
+	skb_reserve(skb[0], headroom + doffset);
+	skb_put(skb[0], data_size);
+	skb[0]->mac_header = 192;
+
+	skb[0]->protocol = htons(ETH_P_IP);
+	skb[0]->network_header = 206;
+
+	for (i = 0; i < 17; i++) {
+		page[i] = alloc_page(GFP_KERNEL);
+		if (!page[i])
+			goto err_page;
+
+		skb_add_rx_frag(skb[0], i, page[i], 0, data_size, data_size);
+	}
+
+	skb[1] = dev_alloc_skb(headroom + alloc_size);
+	if (!skb[1])
+		goto err_page;
+
+	skb_reserve(skb[1], headroom + doffset);
+	skb_put(skb[1], data_size);
+
+	/* setup shinfo */
+	skb_shinfo(skb[0])->gso_size = 75;
+	skb_shinfo(skb[0])->gso_type = SKB_GSO_TCPV4;
+	skb_shinfo(skb[0])->gso_type |= SKB_GSO_UDP_TUNNEL|SKB_GSO_TCP_FIXEDID|SKB_GSO_DODGY;
+	skb_shinfo(skb[0])->gso_segs = 0;
+	skb_shinfo(skb[0])->frag_list = skb[1];
+	skb_shinfo(skb[0])->hwtstamps.hwtstamp = 1000;
+
+	/* adjust skb[0]'s len */
+	skb[0]->len += skb[1]->len;
+	skb[0]->data_len += skb[1]->len;
+	skb[0]->truesize += skb[1]->truesize;
+
+	return skb[0];
+
+err_page:
+	kfree_skb(skb[0]);
+	for (j = 0; j < i; j++)
+		__free_page(page[j]);
+
+	return NULL;
+}
+
 struct skb_segment_test {
 	const char *descr;
 	struct sk_buff *(*build_skb)(void);

@@ -14727,6 +14784,13 @@ static struct skb_segment_test skb_segment_tests[] __initconst = {
 			    NETIF_F_LLTX | NETIF_F_GRO |
 			    NETIF_F_IPV6_CSUM | NETIF_F_RXCSUM |
 			    NETIF_F_HW_VLAN_STAG_TX
+	},
+	{
+		.descr = "gso_with_head_frag",
+		.build_skb = build_test_skb_head_frag,
+		.features = NETIF_F_SG | NETIF_F_HW_CSUM | NETIF_F_GSO_SHIFT |
+			    NETIF_F_TSO_ECN | NETIF_F_TSO_MANGLEID | NETIF_F_TSO6 |
+			    NETIF_F_GSO_SCTP | NETIF_F_GSO_UDP_L4 | NETIF_F_GSO_FRAGLIST
 	}
 };

Signature · 1 line

2.33.0

Fred Li

author

2024-06-26 06:56 UTC #1 lore raw

PATCH v2 1/2 net: Fix skb_segment when splitting gso_size mangled skb having linear-headed frag_list whose head_frag=true

When using calico bpf based NAT, hits a kernel BUG_ON at function skb_segment(), line 4560. Performing NAT translation when accessing a Service IP across subnets, the calico will encap vxlan and calls the bpf_skb_adjust_room to decrease the gso_size, and then call bpf_redirect send packets out.

4438 struct sk_buff *skb_segment(struct sk_buff *head_skb,
4439 netdev_features_t features)
4440 {
4441 struct sk_buff *segs = NULL;
4442 struct sk_buff *tail = NULL;
...
4558 if (hsize <= 0 && i >= nfrags && skb_headlen(list_skb) &&
4559 (skb_headlen(list_skb) == len || sg)) {
4560 BUG_ON(skb_headlen(list_skb) > len);
4561
4562 nskb = skb_clone(list_skb, GFP_ATOMIC);
4563 if (unlikely(!nskb))
4564 goto err;

call stack:
...
[exception RIP: skb_segment+3016]

    RIP: ffffffffb97df2a8  RSP: ffffa3f2cce08728  RFLAGS: 00010293
    RAX: 000000000000007d  RBX: 00000000fffff7b3  RCX: 0000000000000011
    RDX: 0000000000000000  RSI: ffff895ea32c76c0  RDI: 00000000000008c1
    RBP: ffffa3f2cce087f8   R8: 000000000000088f   R9: 0000000000000011
    R10: 000000000000090c  R11: ffff895e47e68000  R12: ffff895eb2022f00
    R13: 000000000000004b  R14: ffff895ecdaf2000  R15: ffff895eb2023f00
    ORIG_RAX: ffffffffffffffff  CS: 0010  SS: 0018
 #9 [ffffa3f2cce08720] skb_segment at ffffffffb97ded63

...

The skb has the following properties:

    doffset = 66
    list_skb = skb_shinfo(skb)->frag_list
    list_skb->head_frag = true
    skb->len = 2441 && skb->data_len = 2250
    skb_shinfo(skb)->nr_frags = 17
    skb_shinfo(skb)->gso_size = 75
    skb_shinfo(skb)->frags[0...16].bv_len = 125
    list_skb->len = 125
    list_skb->data_len = 0

When slicing the frag_list skb, there three cases:
1. Only *non* head_frag
sg will be false, only when skb_headlen(list_skb)==len is satisfied, it will enter the branch at line 4560, and there will be no crash.
2. Mixed head_frag
sg will be false, Only when skb_headlen(list_skb)==len is satisfied, it will enter the branch at line 4560, and there will be no crash.
3. Only frag_list with head_frag=true
sg is true, three cases below:
(1) skb_headlen(list_skb)==len is satisfied, it will enter the branch at line 4560, and there will be no crash.
(2) skb_headlen(list_skb)<len is satisfied, it will enter the branch at line 4560, and there will be no crash.
(3) skb_headlen(list_skb)>len is satisfied, it will be crash.

Applying this patch, three cases will be:
1. Only *non* head_frag
sg will be false. No difference with before.
2. Mixed head_frag
sg will be false. No difference with before.
3. Only frag_list with head_frag=true
sg is true, there also three cases:
(1) skb_headlen(list_skb)==len is satisfied, no difference with before.
(2) skb_headlen(list_skb)<len is satisfied, will be revert to copying in this case.
(3) skb_headlen(list_skb)>len is satisfied, will be revert to copying in this case.

Since commit 13acc94eff122("net: permit skb_segment on head_frag frag_list skb"), it is allowed to segment the head_frag frag_list skb.

Signed-off-by Fred Li <[email protected]>

---
 net/core/skbuff.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

net/core/skbuff.c +1 -1

index f0a9ef1aeaa2..b1dab1b071fc 100644
@@ -4556,7 +4556,7 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb,
 		hsize = skb_headlen(head_skb) - offset;
 
 		if (hsize <= 0 && i >= nfrags && skb_headlen(list_skb) &&
-		    (skb_headlen(list_skb) == len || sg)) {
+		    (skb_headlen(list_skb) == len)) {
 			BUG_ON(skb_headlen(list_skb) > len);
 
 			nskb = skb_clone(list_skb, GFP_ATOMIC);

Signature · 1 line

2.33.0

Paolo Abeni

maintainer

2024-07-02 09:23 UTC #2 lore raw

Re: [PATCH v2 1/2] net: Fix skb_segment when splitting gso_size mangled skb having linear-headed frag_list whose head_frag=true

Quoted 85 lines · Fred Li

When using calico bpf based NAT, hits a kernel BUG_ON at function skb_segment(), line 4560. Performing NAT translation when accessing a Service IP across subnets, the calico will encap vxlan and calls the bpf_skb_adjust_room to decrease the gso_size, and then call bpf_redirect send packets out.
4438 struct sk_buff *skb_segment(struct sk_buff *head_skb,
4439 netdev_features_t features)
4440 {
4441 struct sk_buff *segs = NULL;
4442 struct sk_buff *tail = NULL;
...
4558 if (hsize <= 0 && i >= nfrags && skb_headlen(list_skb) &&
4559 (skb_headlen(list_skb) == len || sg)) {
4560 BUG_ON(skb_headlen(list_skb) > len);
4561
4562 nskb = skb_clone(list_skb, GFP_ATOMIC);
4563 if (unlikely(!nskb))
4564 goto err;
call stack:
...
[exception RIP: skb_segment+3016]
    RIP: ffffffffb97df2a8  RSP: ffffa3f2cce08728  RFLAGS: 00010293
    RAX: 000000000000007d  RBX: 00000000fffff7b3  RCX: 0000000000000011
    RDX: 0000000000000000  RSI: ffff895ea32c76c0  RDI: 00000000000008c1
    RBP: ffffa3f2cce087f8   R8: 000000000000088f   R9: 0000000000000011
    R10: 000000000000090c  R11: ffff895e47e68000  R12: ffff895eb2022f00
    R13: 000000000000004b  R14: ffff895ecdaf2000  R15: ffff895eb2023f00
    ORIG_RAX: ffffffffffffffff  CS: 0010  SS: 0018
 #9 [ffffa3f2cce08720] skb_segment at ffffffffb97ded63
...
The skb has the following properties:
    doffset = 66
    list_skb = skb_shinfo(skb)->frag_list
    list_skb->head_frag = true
    skb->len = 2441 && skb->data_len = 2250
    skb_shinfo(skb)->nr_frags = 17
    skb_shinfo(skb)->gso_size = 75
    skb_shinfo(skb)->frags[0...16].bv_len = 125
    list_skb->len = 125
    list_skb->data_len = 0
When slicing the frag_list skb, there three cases:
1. Only *non* head_frag
sg will be false, only when skb_headlen(list_skb)==len is satisfied, it will enter the branch at line 4560, and there will be no crash.
2. Mixed head_frag
sg will be false, Only when skb_headlen(list_skb)==len is satisfied, it will enter the branch at line 4560, and there will be no crash.
3. Only frag_list with head_frag=true
sg is true, three cases below:
(1) skb_headlen(list_skb)==len is satisfied, it will enter the branch at line 4560, and there will be no crash.
(2) skb_headlen(list_skb)<len is satisfied, it will enter the branch at line 4560, and there will be no crash.
(3) skb_headlen(list_skb)>len is satisfied, it will be crash.
Applying this patch, three cases will be:
1. Only *non* head_frag
sg will be false. No difference with before.
2. Mixed head_frag
sg will be false. No difference with before.
3. Only frag_list with head_frag=true
sg is true, there also three cases:
(1) skb_headlen(list_skb)==len is satisfied, no difference with before.
(2) skb_headlen(list_skb)<len is satisfied, will be revert to copying in this case.
(3) skb_headlen(list_skb)>len is satisfied, will be revert to copying in this case.
Since commit 13acc94eff122("net: permit skb_segment on head_frag frag_list skb"), it is allowed to segment the head_frag frag_list skb.
Signed-off-by Fred Li <[email protected]>
---
 net/core/skbuff.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
net/core/skbuff.c +0 -0
index f0a9ef1aeaa2..b1dab1b071fc 100644

net/core/skbuff.c struct sk_buff *skb_segment(struct sk_buff *head_skb, · line 4562

Quoted 8 lines

net/core/skbuff.c +1 -1

@@ -4556,7 +4556,7 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb,
 		hsize = skb_headlen(head_skb) - offset;
 
 		if (hsize <= 0 && i >= nfrags && skb_headlen(list_skb) &&
-		    (skb_headlen(list_skb) == len || sg)) {
+		    (skb_headlen(list_skb) == len)) {
 			BUG_ON(skb_headlen(list_skb) > len);
 
 			nskb = skb_clone(list_skb, GFP_ATOMIC);

I must admit I more than a bit lost in the many turns of skb_segment(), but the above does not look like the correct solution, as it will make the later BUG_ON() unreachable/meaningless.

Do I read correctly that when the BUG_ON() triggers:

list_skb->len is 125
len is 75
list_skb->frag_head is true

It looks like skb_segment() is becoming even and ever more complex to cope with unexpected skb layouts, only possibly when skb_segment() is called by some BPF helpers.

I think it should be up to the helper itself to adjust the skb and/or the device features to respect skb_segment() constraints, instead of making the common code increasingly not maintainable.

Thanks,

Paolo

Fred Li

author

2024-07-03 12:22 UTC #3 lore raw

Re: [PATCH v2 1/2] net: Fix skb_segment when splitting gso_size mangled skb having linear-headed frag_list whose head_frag=true

Quoted 3 lines

I must admit I more than a bit lost in the many turns of skb_segment(), but the above does not look like the correct solution, as it will make the later BUG_ON() unreachable/meaningless.

Sorry, the subsequent BUG_ON maybe should be removed in this patch, because for skb_headlen(list_skb) > len, it will continue splitting as commit 13acc94eff122 (net: permit skb_segment on head_frag frag_list skb) does.

Quoted 7 lines

Do I read correctly that when the BUG_ON() triggers:
list_skb->len is 125
len is 75
list_skb->frag_head is true

yes, it's correct.
list_skb->len is 125
gso_size is 75, also the len in the BUG_ON conditon list_skb->head_frag is true

Quoted 7 lines

It looks like skb_segment() is becoming even and ever more complex to cope with unexpected skb layouts, only possibly when skb_segment() is called by some BPF helpers.
Thanks,
Paolo

I'll wait for more suggestions from others.

Thanks

Fred Li

Willem de Bruijn

2024-07-06 14:26 UTC #4 lore raw

Re: [PATCH v2 1/2] net: Fix skb_segment when splitting gso_size mangled skb having linear-headed frag_list whose head_frag=true

Quoted 25 lines · Fred Li

Quoted 3 lines
I must admit I more than a bit lost in the many turns of skb_segment(), but the above does not look like the correct solution, as it will make the later BUG_ON() unreachable/meaningless.
Sorry, the subsequent BUG_ON maybe should be removed in this patch, because for skb_headlen(list_skb) > len, it will continue splitting as commit 13acc94eff122 (net: permit skb_segment on head_frag frag_list skb) does.
Quoted 7 lines
Do I read correctly that when the BUG_ON() triggers:
list_skb->len is 125
len is 75
list_skb->frag_head is true
yes, it's correct.
list_skb->len is 125
gso_size is 75, also the len in the BUG_ON conditon list_skb->head_frag is true
Quoted 4 lines
It looks like skb_segment() is becoming even and ever more complex to cope with unexpected skb layouts, only possibly when skb_segment() is called by some BPF helpers.

Quoted 5 lines

Quoted 3 lines
Thanks,
Paolo
I'll wait for more suggestions from others.

In general, agreed with Paolo. Segmentation is getting ever more complex and the code hard to follow.

Maybe at some point we'll have to bite the bullet and seriously refactor it. Or at least parts, such as the frag_list handling case. But that kills any odds of backporting fixes, so not if we can avoid it.

In particular, changing gso_size on skbs with frag_list is fragile.

Instead of adding another special case, how about just linearizing sbks after BPF calls adjust_room (at least if called without BPF_F_ADJ_ROOM_FIXED_GSO) if they have frag_list.

Fred Li

author

2024-07-08 14:31 UTC #5 lore raw

PATCH 1/1 net: linearizing skb when downgrade gso_size

Here is a patch that linearizing skb when downgrade gso_size and sg should disabled, If there are no issues, I will submit a formal patch shortly.

Signed-off-by Fred Li <[email protected]>

---
 include/linux/skbuff.h | 22 ++++++++++++++++++++++
 net/core/filter.c      | 16 ++++++++++++----
 net/core/skbuff.c      | 19 ++-----------------
 3 files changed, 36 insertions(+), 21 deletions(-)

include/linux/skbuff.h +22 -0

index 5f11f9873341..99b7fc1e826a 100644
@@ -2400,6 +2400,28 @@ static inline unsigned int skb_headlen(const struct sk_buff *skb)
 	return skb->len - skb->data_len;
 }
 
+static inline bool skb_is_nonsg(const struct sk_buff *skb)
+{
+	struct sk_buff *list_skb = skb_shinfo(skb)->frag_list;
+	struct sk_buff *check_skb;
+	for (check_skb = list_skb; check_skb; check_skb = check_skb->next) {
+		if (skb_headlen(check_skb) && !check_skb->head_frag) {
+			/* gso_size is untrusted, and we have a frag_list with
+                         * a linear non head_frag item.
+                         *
+                         * If head_skb's headlen does not fit requested gso_size,
+                         * it means that the frag_list members do NOT terminate
+                         * on exact gso_size boundaries. Hence we cannot perform
+                         * skb_frag_t page sharing. Therefore we must fallback to
+                         * copying the frag_list skbs; we do so by disabling SG.
+                         */
+			return true;
+		}
+	}
+
+	return false;
+}
+
 static inline unsigned int __skb_pagelen(const struct sk_buff *skb)
 {
 	unsigned int i, len = 0;

net/core/filter.c +12 -4

index df4578219e82..c0e6e7f28635 100644
@@ -3525,13 +3525,21 @@ static int bpf_skb_net_grow(struct sk_buff *skb, u32 off, u32 len_diff,
 	if (skb_is_gso(skb)) {
 		struct skb_shared_info *shinfo = skb_shinfo(skb);
 
-		/* Due to header grow, MSS needs to be downgraded. */
-		if (!(flags & BPF_F_ADJ_ROOM_FIXED_GSO))
-			skb_decrease_gso_size(shinfo, len_diff);
-
 		/* Header must be checked, and gso_segs recomputed. */
 		shinfo->gso_type |= gso_type;
 		shinfo->gso_segs = 0;
+
+		/* Due to header grow, MSS needs to be downgraded.
+		 * There is BUG_ON When segment the frag_list with
+		 * head_frag true so linearize skb after downgrade
+		 * the MSS.
+		 */
+		if (!(flags & BPF_F_ADJ_ROOM_FIXED_GSO)) {
+			skb_decrease_gso_size(shinfo, len_diff);
+			if (skb_is_nonsg(skb))
+				return skb_linearize(skb) ? : 0;
+		}
+
 	}
 
 	return 0;

net/core/skbuff.c +2 ~~-17~~

index b1dab1b071fc..81e018185527 100644
@@ -4458,23 +4458,8 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb,
 
 	if ((skb_shinfo(head_skb)->gso_type & SKB_GSO_DODGY) &&
 	    mss != GSO_BY_FRAGS && mss != skb_headlen(head_skb)) {
-		struct sk_buff *check_skb;
-
-		for (check_skb = list_skb; check_skb; check_skb = check_skb->next) {
-			if (skb_headlen(check_skb) && !check_skb->head_frag) {
-				/* gso_size is untrusted, and we have a frag_list with
-				 * a linear non head_frag item.
-				 *
-				 * If head_skb's headlen does not fit requested gso_size,
-				 * it means that the frag_list members do NOT terminate
-				 * on exact gso_size boundaries. Hence we cannot perform
-				 * skb_frag_t page sharing. Therefore we must fallback to
-				 * copying the frag_list skbs; we do so by disabling SG.
-				 */
-				features &= ~NETIF_F_SG;
-				break;
-			}
-		}
+		if (skb_is_nonsg(head_skb))
+			features &= ~NETIF_F_SG;
 	}
 
 	__skb_push(head_skb, doffset);

Signature · 1 line

2.33.0

Willem de Bruijn

2024-07-09 15:53 UTC #6 lore raw

Re: [PATCH] net: linearizing skb when downgrade gso_size

Quoted 3 lines · Fred Li

Here is a patch that linearizing skb when downgrade gso_size and sg should disabled, If there are no issues, I will submit a formal patch shortly.

Target bpf.

Probably does not need quite as many direct CCs.

Quoted 12 lines

Signed-off-by Fred Li <[email protected]>

---
 include/linux/skbuff.h | 22 ++++++++++++++++++++++
 net/core/filter.c      | 16 ++++++++++++----
 net/core/skbuff.c      | 19 ++-----------------
 3 files changed, 36 insertions(+), 21 deletions(-)

include/linux/skbuff.h +0 -0

index 5f11f9873341..99b7fc1e826a 100644

include/linux/skbuff.h static inline unsigned int skb_headlen(const struct sk_buff *skb) · line 2404

Quoted 5 lines

include/linux/skbuff.h +2 -0

@@ -2400,6 +2400,28 @@ static inline unsigned int skb_headlen(const struct sk_buff *skb)
 	return skb->len - skb->data_len;
 }
 
+static inline bool skb_is_nonsg(const struct sk_buff *skb)
+{

is_nonsg does not cover the functionality, which is fairly subtle. But maybe we don't need this function at all, see below..

include/linux/skbuff.h static inline unsigned int skb_headlen(const struct sk_buff *skb) · line 2406

Quoted 2 lines

include/linux/skbuff.h +2 -0

+	struct sk_buff *list_skb = skb_shinfo(skb)->frag_list;
+	struct sk_buff *check_skb;

No need for separate check_skb

Quoted 38 lines

include/linux/skbuff.h +18 -0

+	for (check_skb = list_skb; check_skb; check_skb = check_skb->next) {
+		if (skb_headlen(check_skb) && !check_skb->head_frag) {
+			/* gso_size is untrusted, and we have a frag_list with
+                         * a linear non head_frag item.
+                         *
+                         * If head_skb's headlen does not fit requested gso_size,
+                         * it means that the frag_list members do NOT terminate
+                         * on exact gso_size boundaries. Hence we cannot perform
+                         * skb_frag_t page sharing. Therefore we must fallback to
+                         * copying the frag_list skbs; we do so by disabling SG.
+                         */
+			return true;
+		}
+	}
+
+	return false;
+}
+
 static inline unsigned int __skb_pagelen(const struct sk_buff *skb)
 {
 	unsigned int i, len = 0;

net/core/filter.c +2 -4

index df4578219e82..c0e6e7f28635 100644
@@ -3525,13 +3525,21 @@ static int bpf_skb_net_grow(struct sk_buff *skb, u32 off, u32 len_diff,
 	if (skb_is_gso(skb)) {
 		struct skb_shared_info *shinfo = skb_shinfo(skb);
 
-		/* Due to header grow, MSS needs to be downgraded. */
-		if (!(flags & BPF_F_ADJ_ROOM_FIXED_GSO))
-			skb_decrease_gso_size(shinfo, len_diff);
-
 		/* Header must be checked, and gso_segs recomputed. */
 		shinfo->gso_type |= gso_type;
 		shinfo->gso_segs = 0;
+
+		/* Due to header grow, MSS needs to be downgraded.

net/core/filter.c static int bpf_skb_net_grow(struct sk_buff *skb, u32 off, u32 len_diff, · line 3542

Quoted 10 lines

net/core/filter.c +10 -0

+		 * There is BUG_ON When segment the frag_list with
+		 * head_frag true so linearize skb after downgrade
+		 * the MSS.
+		 */
+		if (!(flags & BPF_F_ADJ_ROOM_FIXED_GSO)) {
+			skb_decrease_gso_size(shinfo, len_diff);
+			if (skb_is_nonsg(skb))
+				return skb_linearize(skb) ? : 0;
+		}
+

No need for ternary statement.

Instead of the complex test in skb_is_nonsg, can we just assume that alignment will be off if having frag_list and changing gso_size.

The same will apply to bpf_skb_net_shrink too.

Not sure that it is okay to linearize inside a BPF helper function. Hopefully bpf experts can chime in on that.

Quoted 36 lines

net/core/filter.c +0 -0

 	}
 
 	return 0;

net/core/skbuff.c +2 ~~-17~~

index b1dab1b071fc..81e018185527 100644
@@ -4458,23 +4458,8 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb,
 
 	if ((skb_shinfo(head_skb)->gso_type & SKB_GSO_DODGY) &&
 	    mss != GSO_BY_FRAGS && mss != skb_headlen(head_skb)) {
-		struct sk_buff *check_skb;
-
-		for (check_skb = list_skb; check_skb; check_skb = check_skb->next) {
-			if (skb_headlen(check_skb) && !check_skb->head_frag) {
-				/* gso_size is untrusted, and we have a frag_list with
-				 * a linear non head_frag item.
-				 *
-				 * If head_skb's headlen does not fit requested gso_size,
-				 * it means that the frag_list members do NOT terminate
-				 * on exact gso_size boundaries. Hence we cannot perform
-				 * skb_frag_t page sharing. Therefore we must fallback to
-				 * copying the frag_list skbs; we do so by disabling SG.
-				 */
-				features &= ~NETIF_F_SG;
-				break;
-			}
-		}
+		if (skb_is_nonsg(head_skb))
+			features &= ~NETIF_F_SG;
 	}
 
 	__skb_push(head_skb, doffset);

Signature · 1 line

2.33.0

Herbert Xu

2024-07-09 20:17 UTC #7 lore raw

Re: [PATCH] net: linearizing skb when downgrade gso_size

Quoted 6 lines · Willem de Bruijn

net/core/filter.c static int bpf_skb_net_grow(struct sk_buff *skb, u32 off, u32 len_diff, · line 3536
Quoted 6 lines
net/core/filter.c +5 -0
+		/* Due to header grow, MSS needs to be downgraded.
+		 * There is BUG_ON When segment the frag_list with
+		 * head_frag true so linearize skb after downgrade
+		 * the MSS.
+		 */

This sounds completely wrong. You should never grow the TCP header by changing gso_size. What is the usage-scenario for this?

Think about it, if a router forwards a TCP packet, and ends up growing its TCP header and then splits the packet into two, then this router is brain-dead.

Cheers,

Signature · 3 lines

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Willem de Bruijn

2024-07-09 21:30 UTC #8 lore raw

Re: [PATCH] net: linearizing skb when downgrade gso_size

Quoted 11 lines · Herbert Xu

Quoted 6 lines · Willem de Bruijn
net/core/filter.c static int bpf_skb_net_grow(struct sk_buff *skb, u32 off, u32 len_diff, · line 3536
Quoted 6 lines
net/core/filter.c +5 -0
+		/* Due to header grow, MSS needs to be downgraded.
+		 * There is BUG_ON When segment the frag_list with
+		 * head_frag true so linearize skb after downgrade
+		 * the MSS.
+		 */
This sounds completely wrong. You should never grow the TCP header by changing gso_size. What is the usage-scenario for this?

Quoted 3 lines

Think about it, if a router forwards a TCP packet, and ends up growing its TCP header and then splits the packet into two, then this router is brain-dead.

This is an unfortunate feature, but already exists.

It decreases gso_size to account for tunnel headers.

For USO, we added BPF_F_ADJ_ROOM_FIXED_GSO to avoid this in better, newer users.

Herbert Xu

2024-07-10 23:07 UTC #9 lore raw

Re: [PATCH] net: linearizing skb when downgrade gso_size

Quoted 4 lines · Willem de Bruijn

This is an unfortunate feature, but already exists.
It decreases gso_size to account for tunnel headers.

Growing the tunnel header is totally fine. But you should not decrease gso_size because of that. Instead the correct course of action is to drop the packet and generate an ICMP if it no longer fits the MTU.

A router that resegments a TCP packet at the TCP-level (not IP) is brain-dead.

Cheers,

Signature · 3 lines

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Fred Li

author

2024-07-12 08:17 UTC #10 lore raw

Re: [PATCH] net: linearizing skb when downgrade gso_size

In reply to Willem de Bruijn

Quoted 6 lines

No need for ternary statement.
Instead of the complex test in skb_is_nonsg, can we just assume that alignment will be off if having frag_list and changing gso_size.
The same will apply to bpf_skb_net_shrink too.

increase gso_size may be no problem and we can use BPF_F_ADJ_ROOM_FIXED_GSO to avoid update gso_size when shrink.

Quoted 3 lines

Not sure that it is okay to linearize inside a BPF helper function. Hopefully bpf experts can chime in on that.

Thanks

Fred Li

author

2024-07-17 05:35 UTC #11 lore raw

PATCH v3 1/1 net: linearizing skb when downgrade gso_size

In reply to Willem de Bruijn

Linearizing skb when downgrade gso_size because it may trigger the BUG_ON when segment skb as described in [1].

v3 changes:
linearize skb if having frag_list as Willem de Bruijn suggested[2].

[1] https://lore.kernel.org/all/[email protected]/
[2] https://lore.kernel.org/all/[email protected]/

Signed-off-by Fred Li <[email protected]>

---
 net/core/filter.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

net/core/filter.c +12 -4

index df4578219e82..70919b532d68 100644
@@ -3525,13 +3525,21 @@ static int bpf_skb_net_grow(struct sk_buff *skb, u32 off, u32 len_diff,
 	if (skb_is_gso(skb)) {
 		struct skb_shared_info *shinfo = skb_shinfo(skb);
 
-		/* Due to header grow, MSS needs to be downgraded. */
-		if (!(flags & BPF_F_ADJ_ROOM_FIXED_GSO))
-			skb_decrease_gso_size(shinfo, len_diff);
-
 		/* Header must be checked, and gso_segs recomputed. */
 		shinfo->gso_type |= gso_type;
 		shinfo->gso_segs = 0;
+
+		/* Due to header grow, MSS needs to be downgraded.
+		 * There is BUG_ON When segment the frag_list with
+		 * head_frag true so linearize skb after downgrade
+		 * the MSS.
+		 */
+		if (!(flags & BPF_F_ADJ_ROOM_FIXED_GSO)) {
+			skb_decrease_gso_size(shinfo, len_diff);
+			if (shinfo->frag_list)
+				return skb_linearize(skb);
+		}
+
 	}
 
 	return 0;

Signature · 1 line

2.33.0

Willem de Bruijn

2024-07-17 16:32 UTC #12 lore raw

Re: [PATCH v3] net: linearizing skb when downgrade gso_size

Quoted 10 lines · Fred Li

Linearizing skb when downgrade gso_size because it may trigger the BUG_ON when segment skb as described in [1].
v3 changes:
linearize skb if having frag_list as Willem de Bruijn suggested[2].
[1] https://lore.kernel.org/all/[email protected]/
[2] https://lore.kernel.org/all/[email protected]/
Signed-off-by Fred Li <[email protected]>

A fix needs a Fixed tag.

This might be the original commit that introduced gso_size adjustment, commit 6578171a7ff0c ("bpf: add bpf_skb_change_proto helper")

Unless support for frag_list came later.

Quoted 15 lines

---
 net/core/filter.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

net/core/filter.c +0 -3

index df4578219e82..70919b532d68 100644
@@ -3525,13 +3525,21 @@ static int bpf_skb_net_grow(struct sk_buff *skb, u32 off, u32 len_diff,
 	if (skb_is_gso(skb)) {
 		struct skb_shared_info *shinfo = skb_shinfo(skb);
 
-		/* Due to header grow, MSS needs to be downgraded. */
-		if (!(flags & BPF_F_ADJ_ROOM_FIXED_GSO))
-			skb_decrease_gso_size(shinfo, len_diff);

net/core/filter.c static int bpf_skb_net_grow(struct sk_buff *skb, u32 off, u32 len_diff, · line 3536

Quoted 10 lines

net/core/filter.c +6 -1

-
 		/* Header must be checked, and gso_segs recomputed. */
 		shinfo->gso_type |= gso_type;
 		shinfo->gso_segs = 0;
+
+		/* Due to header grow, MSS needs to be downgraded.
+		 * There is BUG_ON When segment the frag_list with
+		 * head_frag true so linearize skb after downgrade
+		 * the MSS.
+		 */

Super tiny nit: no capitalization of When in the middle of a sentence.

net/core/filter.c static int bpf_skb_net_grow(struct sk_buff *skb, u32 off, u32 len_diff, · line 3540

Quoted 4 lines

net/core/filter.c +4 -0

+		if (!(flags & BPF_F_ADJ_ROOM_FIXED_GSO)) {
+			skb_decrease_gso_size(shinfo, len_diff);
+			if (shinfo->frag_list)
+				return skb_linearize(skb);

I previously asked whether it was safe to call pskb_expand_head from within a BPF external function. There are actually plenty of existing examples of this, so this is fine.

Quoted 8 lines

net/core/filter.c +2 -0
+		}
+
 	}
 
 	return 0;
Signature · 1 line
2.33.0

Fred Li

author

2024-07-18 07:43 UTC #13 lore raw

Re: [PATCH v3] net: linearizing skb when downgrade gso_size

Quoted 10 lines

Quoted 10 lines
Linearizing skb when downgrade gso_size because it may trigger the BUG_ON when segment skb as described in [1].
v3 changes:
linearize skb if having frag_list as Willem de Bruijn suggested[2].
[1] https://lore.kernel.org/all/[email protected]/
[2] https://lore.kernel.org/all/[email protected]/

Quoted 7 lines

Quoted 1 line
Signed-off-by Fred Li <[email protected]>
A fix needs a Fixed tag.
This might be the original commit that introduced gso_size adjustment, commit 6578171a7ff0c ("bpf: add bpf_skb_change_proto helper")

Yes, this is the original commit, but it's already fixed by commit 364745fbe981a (bpf: Do not change gso_size during bpf_skb_change_proto())

Another commit 2be7e212d5419 (bpf: add bpf_skb_adjust_room helper) introduced gso_size too.

Quoted 14 lines

Unless support for frag_list came later.

Quoted 12 lines

---
 net/core/filter.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

net/core/filter.c +0 -0

index df4578219e82..70919b532d68 100644
@@ -3525,13 +3525,21 @@ static int bpf_skb_net_grow(struct sk_buff *skb, u32 off, u32 len_diff,
 	if (skb_is_gso(skb)) {
 		struct skb_shared_info *shinfo = skb_shinfo(skb);

Quoted 15 lines

net/core/filter.c static int bpf_skb_net_grow(struct sk_buff *skb, u32 off, u32 len_diff, · line 3536

Quoted 13 lines

net/core/filter.c +6 -4

-		/* Due to header grow, MSS needs to be downgraded. */
-		if (!(flags & BPF_F_ADJ_ROOM_FIXED_GSO))
-			skb_decrease_gso_size(shinfo, len_diff);
-
 		/* Header must be checked, and gso_segs recomputed. */
 		shinfo->gso_type |= gso_type;
 		shinfo->gso_segs = 0;
+
+		/* Due to header grow, MSS needs to be downgraded.
+		 * There is BUG_ON When segment the frag_list with
+		 * head_frag true so linearize skb after downgrade
+		 * the MSS.
+		 */

Super tiny nit: no capitalization of When in the middle of a sentence.

Thanks, i'will fix.

Quoted 18 lines

net/core/filter.c static int bpf_skb_net_grow(struct sk_buff *skb, u32 off, u32 len_diff, · line 3540
Quoted 5 lines
net/core/filter.c +4 -0
+		if (!(flags & BPF_F_ADJ_ROOM_FIXED_GSO)) {
+			skb_decrease_gso_size(shinfo, len_diff);
+			if (shinfo->frag_list)
+				return skb_linearize(skb);
I previously asked whether it was safe to call pskb_expand_head from within a BPF external function. There are actually plenty of existing examples of this, so this is fine.
Quoted 8 lines
net/core/filter.c +2 -0
+		}
+
 	}
 
 	return 0;
Signature · 1 line
2.33.0

Fred Li

author

2024-07-19 02:47 UTC #14 lore raw

PATCH v4 1/1 bpf: Fixed a segment issue when downgrade gso_size

In reply to Willem de Bruijn

Linearizing skb when downgrade gso_size because it may trigger the BUG_ON when segment skb as described in [1].

v4 changes:
add fixed tag.

v3 changes:
linearize skb if having frag_list as Willem de Bruijn suggested[2].

[1] https://lore.kernel.org/all/[email protected]/
[2] https://lore.kernel.org/all/[email protected]/

Fixes 2be7e212d5419 ("bpf: add bpf_skb_adjust_room helper")

Signed-off-by Fred Li <[email protected]>

---
 net/core/filter.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

net/core/filter.c +12 -4

index df4578219e82..71396ecfc574 100644
@@ -3525,13 +3525,21 @@ static int bpf_skb_net_grow(struct sk_buff *skb, u32 off, u32 len_diff,
 	if (skb_is_gso(skb)) {
 		struct skb_shared_info *shinfo = skb_shinfo(skb);
 
-		/* Due to header grow, MSS needs to be downgraded. */
-		if (!(flags & BPF_F_ADJ_ROOM_FIXED_GSO))
-			skb_decrease_gso_size(shinfo, len_diff);
-
 		/* Header must be checked, and gso_segs recomputed. */
 		shinfo->gso_type |= gso_type;
 		shinfo->gso_segs = 0;
+
+		/* Due to header grow, MSS needs to be downgraded.
+		 * There is BUG_ON when segment the frag_list with
+		 * head_frag true so linearize skb after downgrade
+		 * the MSS.
+		 */
+		if (!(flags & BPF_F_ADJ_ROOM_FIXED_GSO)) {
+			skb_decrease_gso_size(shinfo, len_diff);
+			if (shinfo->frag_list)
+				return skb_linearize(skb);
+		}
+
 	}
 
 	return 0;

Signature · 1 line

2.33.0

Willem de Bruijn

Reviewed-by on v4 1/1

2024-07-19 18:23 UTC #15 lore raw

Re: [PATCH v4] bpf: Fixed a segment issue when downgrade gso_size

Quoted 7 lines · Fred Li

Linearizing skb when downgrade gso_size because it may trigger the BUG_ON when segment skb as described in [1].
v4 changes:
add fixed tag.

Quoted 8 lines

v3 changes:
linearize skb if having frag_list as Willem de Bruijn suggested[2].
[1] https://lore.kernel.org/all/[email protected]/
[2] https://lore.kernel.org/all/[email protected]/
Fixes 2be7e212d5419 ("bpf: add bpf_skb_adjust_room helper")
Signed-off-by Fred Li <[email protected]>

Reviewed-by Willem de Bruijn <[email protected]>

For next time:
- add target: [PATCH bpf]
- use imperative mood in commit messages: "Linearize ..."

https://www.kernel.org/doc/Documentation/bpf/bpf_devel_QA.rst
https://www.kernel.org/doc/Documentation/process/submitting-patches.rst

Quoted 37 lines

---
 net/core/filter.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

net/core/filter.c +12 -5

index df4578219e82..71396ecfc574 100644
@@ -3525,13 +3525,21 @@ static int bpf_skb_net_grow(struct sk_buff *skb, u32 off, u32 len_diff,
        if (skb_is_gso(skb)) {
                struct skb_shared_info *shinfo = skb_shinfo(skb);
 
-               /* Due to header grow, MSS needs to be downgraded. */
-               if (!(flags & BPF_F_ADJ_ROOM_FIXED_GSO))
-                       skb_decrease_gso_size(shinfo, len_diff);
-
                /* Header must be checked, and gso_segs recomputed. */
                shinfo->gso_type |= gso_type;
                shinfo->gso_segs = 0;
+
+               /* Due to header grow, MSS needs to be downgraded.
+                * There is BUG_ON when segment the frag_list with
+                * head_frag true so linearize skb after downgrade
+                * the MSS.
+                */
+               if (!(flags & BPF_F_ADJ_ROOM_FIXED_GSO)) {
+                       skb_decrease_gso_size(shinfo, len_diff);
+                       if (shinfo->frag_list)
+                               return skb_linearize(skb);
+               }
+
        }
 
        return 0;
--

2.33.0

[email protected]

2024-07-23 01:12 UTC #16 lore raw

Re: [PATCH v4] bpf: Fixed a segment issue when downgrade gso_size

In reply to Fred Li

CI has tested the following submission:

Dear patch submitter,

CI has tested the following submission:

Status:     FAILURE
Name:       [v4] bpf: Fixed a segment issue when downgrade gso_size
Patchwork:  https://patchwork.kernel.org/project/netdevbpf/list/?series=872394&state=*
Matrix:     https://github.com/kernel-patches/bpf/actions/runs/10050697643

Failed jobs:
test_progs_no_alu32-aarch64-gcc: https://github.com/kernel-patches/bpf/actions/runs/10050697643/job/27779246113
test_progs_no_alu32-s390x-gcc: https://github.com/kernel-patches/bpf/actions/runs/10050697643/job/27779299595
test_progs_no_alu32-x86_64-gcc: https://github.com/kernel-patches/bpf/actions/runs/10050697643/job/27779290704
test_progs_no_alu32-x86_64-llvm-17: https://github.com/kernel-patches/bpf/actions/runs/10050697643/job/27779296439
test_progs_no_alu32-x86_64-llvm-18: https://github.com/kernel-patches/bpf/actions/runs/10050697643/job/27779303677

First test_progs failure (test_progs_no_alu32-aarch64-gcc):
#134 libbpf_get_fd_by_id_opts
libbpf: prog 'check_access': BPF program load failed: Invalid argument
libbpf: prog 'check_access': -- BEGIN PROG LOAD LOG --
0: R1=ctx() R10=fp0
; int BPF_PROG(check_access, struct bpf_map *map, fmode_t fmode) @ test_libbpf_get_fd_by_id_opts.c:27
0: (b7) r0 = 0 ; R0_w=0
1: (79) r2 = *(u64 *)(r1 +0)
func 'bpf_lsm_bpf_map' arg0 has btf_id 2072 type STRUCT 'bpf_map'
2: R1=ctx() R2_w=trusted_ptr_bpf_map()
; if (map != (struct bpf_map *)&data_input) @ test_libbpf_get_fd_by_id_opts.c:29
2: (18) r3 = 0xffff0000c3462600 ; R3_w=map_ptr(map=data_input,ks=4,vs=4)
4: (5d) if r2 != r3 goto pc+4 ; R2_w=trusted_ptr_bpf_map() R3_w=map_ptr(map=data_input,ks=4,vs=4)
; int BPF_PROG(check_access, struct bpf_map *map, fmode_t fmode) @ test_libbpf_get_fd_by_id_opts.c:27
5: (79) r0 = *(u64 *)(r1 +8) ; R0_w=scalar() R1=ctx()
; if (fmode & FMODE_WRITE) @ test_libbpf_get_fd_by_id_opts.c:32
6: (67) r0 <<= 62 ; R0_w=scalar(smax=0x4000000000000000,umax=0xc000000000000000,smin32=0,smax32=umax32=0,var_off=(0x0; 0xc000000000000000))
7: (c7) r0 s>>= 63 ; R0_w=scalar(smin=smin32=-1,smax=smax32=0)
; @ test_libbpf_get_fd_by_id_opts.c:0
8: (57) r0 &= -13 ; R0_w=scalar(smax=0x7ffffffffffffff3,umax=0xfffffffffffffff3,smax32=0x7ffffff3,umax32=0xfffffff3,var_off=(0x0; 0xfffffffffffffff3))
; int BPF_PROG(check_access, struct bpf_map *map, fmode_t fmode) @ test_libbpf_get_fd_by_id_opts.c:27 9: (95) exit
At program exit the register R0 has smax=9223372036854775795 should have been in [-4095, 0] processed 9 insns (limit 1000000) max_states_per_insn 0 total_states 0 peak_states 0 mark_read 0 -- END PROG LOAD LOG --
libbpf: prog 'check_access': failed to load: -22
libbpf: failed to load object 'test_libbpf_get_fd_by_id_opts'
libbpf: failed to load BPF skeleton 'test_libbpf_get_fd_by_id_opts': -22 test_libbpf_get_fd_by_id_opts:FAIL:test_libbpf_get_fd_by_id_opts__open_and_load unexpected error: -22

Please note: this email is coming from an unmonitored mailbox. If you have questions or feedback, please reach out to the Meta Kernel CI team at [email protected].

Fred Li

author

2024-07-22 03:08 UTC #17 lore raw

PATCH v5 1/1 bpf: Fixed segment issue when downgrade gso_size

In reply to Willem de Bruijn

Linearize skb when downgrad gso_size to prevent triggering the BUG_ON during segment skb as described in [1].

v5 changes:
- add bpf subject prefix.
- adjust message to imperative mood.

v4 changes:
- add fixed tag.

v3 changes:
- linearize skb if having frag_list as Willem de Bruijn suggested [2].

[1] https://lore.kernel.org/all/[email protected]/
[2] https://lore.kernel.org/all/[email protected]/

Fixes 2be7e212d541 ("bpf: add bpf_skb_adjust_room helper")

Signed-off-by Fred Li <[email protected]>

---
 net/core/filter.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

net/core/filter.c +12 -4

index df4578219e82..71396ecfc574 100644
@@ -3525,13 +3525,21 @@ static int bpf_skb_net_grow(struct sk_buff *skb, u32 off, u32 len_diff,
 	if (skb_is_gso(skb)) {
 		struct skb_shared_info *shinfo = skb_shinfo(skb);
 
-		/* Due to header grow, MSS needs to be downgraded. */
-		if (!(flags & BPF_F_ADJ_ROOM_FIXED_GSO))
-			skb_decrease_gso_size(shinfo, len_diff);
-
 		/* Header must be checked, and gso_segs recomputed. */
 		shinfo->gso_type |= gso_type;
 		shinfo->gso_segs = 0;
+
+		/* Due to header grow, MSS needs to be downgraded.
+		 * There is BUG_ON when segment the frag_list with
+		 * head_frag true so linearize skb after downgrade
+		 * the MSS.
+		 */
+		if (!(flags & BPF_F_ADJ_ROOM_FIXED_GSO)) {
+			skb_decrease_gso_size(shinfo, len_diff);
+			if (shinfo->frag_list)
+				return skb_linearize(skb);
+		}
+
 	}
 
 	return 0;

Signature · 1 line

2.33.0

[email protected]

2024-07-22 03:34 UTC #18 lore raw

Re: [PATCH bpf v5] bpf: Fixed segment issue when downgrade gso_size

CI has tested the following submission:

Dear patch submitter,

CI has tested the following submission:

Status:     SUCCESS
Name:       [bpf,v5] bpf: Fixed segment issue when downgrade gso_size
Patchwork:  https://patchwork.kernel.org/project/netdevbpf/list/?series=872810&state=*
Matrix:     https://github.com/kernel-patches/bpf/actions/runs/10034064977

No further action is necessary on your part.

Please note: this email is coming from an unmonitored mailbox. If you have questions or feedback, please reach out to the Meta Kernel CI team at [email protected].

Willem de Bruijn

Reviewed-by on v5 1/1

2024-07-22 16:29 UTC #19 lore raw

Re: [PATCH bpf v5] bpf: Fixed segment issue when downgrade gso_size

In reply to Fred Li

Quoted 11 lines · Fred Li

Linearize skb when downgrad gso_size to prevent triggering the BUG_ON during segment skb as described in [1].
v5 changes:
- add bpf subject prefix.
- adjust message to imperative mood.
v4 changes:
- add fixed tag.

Quoted 8 lines

v3 changes:
- linearize skb if having frag_list as Willem de Bruijn suggested [2].
[1] https://lore.kernel.org/all/[email protected]/
[2] https://lore.kernel.org/all/[email protected]/
Fixes 2be7e212d541 ("bpf: add bpf_skb_adjust_room helper")
Signed-off-by Fred Li <[email protected]>

Reviewed-by Willem de Bruijn <[email protected]>

My comments were informational, for a next patch if any, really. v4 was fine. v5 is too.

Fred Li

author

2024-07-24 13:37 UTC #20 lore raw

Re: [PATCH bpf v5] bpf: Fixed segment issue when downgrade gso_size

Quoted 17 lines

Quoted 17 lines
Linearize skb when downgrad gso_size to prevent triggering the BUG_ON during segment skb as described in [1].
v5 changes:
- add bpf subject prefix.
- adjust message to imperative mood.
v4 changes:
- add fixed tag.
v3 changes:
- linearize skb if having frag_list as Willem de Bruijn suggested [2].
[1] https://lore.kernel.org/all/[email protected]/
[2] https://lore.kernel.org/all/[email protected]/

Quoted 7 lines

Quoted 2 lines
Fixes 2be7e212d541 ("bpf: add bpf_skb_adjust_room helper")
Signed-off-by Fred Li <[email protected]>
Reviewed-by Willem de Bruijn <[email protected]>
My comments were informational, for a next patch if any, really. v4 was fine. v5 is too.

Thanks for your advise.

Fred Li

Daniel Borkmann

maintainer

2024-07-25 10:09 UTC #21 lore raw

Re: [PATCH bpf v5] bpf: Fixed segment issue when downgrade gso_size

Quoted 20 lines · Fred Li

Quoted 20 lines
Quoted 19 lines
Linearize skb when downgrad gso_size to prevent triggering the BUG_ON during segment skb as described in [1].
v5 changes:
- add bpf subject prefix.
- adjust message to imperative mood.
v4 changes:
- add fixed tag.
v3 changes:
- linearize skb if having frag_list as Willem de Bruijn suggested [2].
[1] https://lore.kernel.org/all/[email protected]/
[2] https://lore.kernel.org/all/[email protected]/
Fixes 2be7e212d541 ("bpf: add bpf_skb_adjust_room helper")
Signed-off-by Fred Li <[email protected]>

Quoted 8 lines

Quoted 4 lines
Reviewed-by Willem de Bruijn <[email protected]>
My comments were informational, for a next patch if any, really. v4 was fine. v5 is too.
Thanks for your advise.
Fred Li

lgtm, I slightly improved wording & applied, thanks!

Fred Li

author

2024-06-26 06:56 UTC #22 lore raw

v2 cover net: Fix BUG_ON for segment frag_list with mangled gso_size and head_frag is true

In reply to Fred Li

This serices fix the BUG_ON when segment frag_list with mangled gso_size and head_frag is true. This patch was tested on kernel 6.6.35.

v2 changes:
Updated with remove the sg condition may be more make sense.

Fred Li (2):
net: Fix skb_segment when splitting gso_size mangled skb having
linear-headed frag_list whose head_frag=true
test_bpf: Introduce an skb_segment test for frag_list whose
head_frag=true and gso_size was mangled

 lib/test_bpf.c    | 64 +++++++++++++++++++++++++++++++++++++++++++++++
 net/core/skbuff.c |  2 +-
 2 files changed, 65 insertions(+), 1 deletion(-)

Signature · 1 line

2.33.0