[PATCH] net: Fix the gso BUG_ON that treat the skb which head_frag is true as non head_frag

[PATCH] net: Fix the gso BUG_ON that treat the skb which head_frag is true as non head_frag

[dracoding]

From: Fred Li <dracodingfly@gmail.com>

The crashed kernel version is 5.16.20, and I have not test this patch
because I dont find a way to reproduce it, and the mailine may be
has the same problem.

When using bpf based NAT, hits a kernel BUG_ON at function skb_segment(),
BUG_ON(skb_headlen(list_skb) > len). The bpf calls the bpf_skb_adjust_room
to decrease the gso_size, and then call bpf_redirect send packet out.

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
#10 [ffffa3f2cce08800] tcp_gso_segment at ffffffffb98d0320
#11 [ffffa3f2cce08860] tcp4_gso_segment at ffffffffb98d07a3
#12 [ffffa3f2cce08880] inet_gso_segment at ffffffffb98e6de0
#13 [ffffa3f2cce088e0] skb_mac_gso_segment at ffffffffb97f3741
#14 [ffffa3f2cce08918] skb_udp_tunnel_segment at ffffffffb98daa59
#15 [ffffa3f2cce08980] udp4_ufo_fragment at ffffffffb98db471
#16 [ffffa3f2cce089b0] inet_gso_segment at ffffffffb98e6de0
#17 [ffffa3f2cce08a10] skb_mac_gso_segment at ffffffffb97f3741
#18 [ffffa3f2cce08a48] __skb_gso_segment at ffffffffb97f388e
#19 [ffffa3f2cce08a78] validate_xmit_skb at ffffffffb97f3d6e
#20 [ffffa3f2cce08ab8] __dev_queue_xmit at ffffffffb97f4614
#21 [ffffa3f2cce08b50] dev_queue_xmit at ffffffffb97f5030
#22 [ffffa3f2cce08b60] __bpf_redirect at ffffffffb98199a8
#23 [ffffa3f2cce08b88] skb_do_redirect at ffffffffb98205cd
...

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

3962 struct sk_buff *skb_segment(struct sk_buff *head_skb,
3963                             netdev_features_t features)
3964 {
3965         struct sk_buff *segs = NULL;
3966         struct sk_buff *tail = NULL;
...
4181                 while (pos < offset + len) {
4182                         if (i >= nfrags) {
4183                                 i = 0;
4184                                 nfrags = skb_shinfo(list_skb)->nr_frags;
4185                                 frag = skb_shinfo(list_skb)->frags;
4186                                 frag_skb = list_skb;

After segment the head_skb's last frag, the (pos == offset+len), so break the
while at line 4181, run into this BUG_ON(), not segment the head_frag frag_list
skb.

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

In commit 3dcbdb134f32 (net: gso: Fix skb_segment splat when splitting gso_size
mangled skb having linear-headed frag_list), it is cleared the NETIF_F_SG if it
has non head_frag skb. It is not cleared the NETIF_F_SG only with one head_frag
frag_list skb.

Signed-off-by: Fred Li <dracodingfly@gmail.com>
---
 net/core/skbuff.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 001152c..d805a47 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -4070,7 +4070,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) &&
+		if (hsize <= 0 && i >= nfrags && !list_skb->head_frag && skb_headlen(list_skb) &&
 		    (skb_headlen(list_skb) == len || sg)) {
 			BUG_ON(skb_headlen(list_skb) > len);
 
-- 
1.8.3.1

Re: [PATCH] net: Fix the gso BUG_ON that treat the skb which head_frag is true as non head_frag

[Willem de Bruijn]

dracoding wrote:
> From: Fred Li <dracodingfly@gmail.com>
> 
> The crashed kernel version is 5.16.20, and I have not test this patch
> because I dont find a way to reproduce it, and the mailine may be
> has the same problem.

That is a pretty old kernel.

There has been work in this space in the meantime. Such as commit
3dcbdb134f32 ("net: gso: Fix skb_segment splat when splitting gso_size
mangled skb having linear-headed frag_list") or commit 9e4b7a99a03a
("net: gso: fix panic on frag_list with mixed head alloc types").

Re: [PATCH] net: Fix the gso BUG_ON that treat the skb which head_frag is true as non head_frag

[Fred Li]

From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>

> dracoding wrote:
> > From: Fred Li <dracodingfly@gmail.com>
> > 
> > The crashed kernel version is 5.16.20, and I have not test this patch
> > because I dont find a way to reproduce it, and the mailine may be
> > has the same problem.
> 
> That is a pretty old kernel.
> 
> There has been work in this space in the meantime. Such as commit
> 3dcbdb134f32 ("net: gso: Fix skb_segment splat when splitting gso_size
> mangled skb having linear-headed frag_list") or commit 9e4b7a99a03a
> ("net: gso: fix panic on frag_list with mixed head alloc types").

The mainline kernel is using the commit 9e4b7a99a03a("net: gso: fix panic
on frag_list with mixed head alloc types") version, but it not work for me.
It disable NETIF_F_SG only if it has non head_frag skb.I will send a test
case which will cause the system crash, also in the kernel 6.6.8.
 

Re: [PATCH] net: Fix the gso BUG_ON that treat the skb which head_frag is true as non head_frag

[Fred Li]

From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>

> dracoding wrote:
> > From: Fred Li <dracodingfly@gmail.com>
> > 
> > The crashed kernel version is 5.16.20, and I have not test this patch
> > because I dont find a way to reproduce it, and the mailine may be
> > has the same problem.
> 
> That is a pretty old kernel.
> 
> There has been work in this space in the meantime. Such as commit
> 3dcbdb134f32 ("net: gso: Fix skb_segment splat when splitting gso_size
> mangled skb having linear-headed frag_list") or commit 9e4b7a99a03a
> ("net: gso: fix panic on frag_list with mixed head alloc types").

The mainline kernel is using the commit 9e4b7a99a03a("net: gso: fix panic
on frag_list with mixed head alloc types") version, but it not work for me.
It disable NETIF_F_SG only if it has non head_frag skb. This case is it has
only one head_frag frag_list skb. I will send a test case which will cause 
the system crash, also in a newest kernel 6.6.8.
 

[PATCH] test_bpf: Add an skb_segment test for a non linear frag_list whose head_frag=1 and gso_size was mangled

[Fred Li]

The patch was based on kernel 6.6.8, the skb properties as
mentioned in [1]. This test will cause system crash without
the patch described in [1].

[1] https://lore.kernel.org/netdev/20240515144313.61680-1-dracodingfly@gmail.com/

Signed-off-by: Fred Li <dracodingfly@gmail.com>
---
 lib/test_bpf.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 64 insertions(+)

diff --git a/lib/test_bpf.c b/lib/test_bpf.c
index ecde42162..a38d2d09c 100644
--- a/lib/test_bpf.c
+++ b/lib/test_bpf.c
@@ -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
 	}
 };
 
-- 
2.33.0

Re: [PATCH] test_bpf: Add an skb_segment test for a non linear frag_list whose head_frag=1 and gso_size was mangled

[Fred Li]

For kernel 6.6.8, when sg is true and skb_headlen(list_skb) != len, it also has 
chance run into this BUG_ON() line 4548.
'''
4544                 hsize = skb_headlen(head_skb) - offset;
4545 
4546                 if (hsize <= 0 && i >= nfrags && skb_headlen(list_skb) &&
4547                     (skb_headlen(list_skb) == len || sg)) {
4548                         BUG_ON(skb_headlen(list_skb) > len);
4549 
4550                         nskb = skb_clone(list_skb, GFP_ATOMIC);
'''

As commit 9e4b7a99a03a("net: gso: fix panic on frag_list with mixed head alloc types")
said. It walk the frag_list in skb_segment and clear NETIF_F_SG when there is non head_frag 
skb. 

But for frag_list only with one head_frag, NETIF_F_SG was not cleared, if skb_headlen(list_skb) != len,
in this case, maybe we can fix it with run into segment as commit 13acc94eff122(net: permit skb_segment on 
head_frag frag_list skb). 

Any suggestions for resolving this issue.

Thanks

Fred Li