From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx0b-00082601.pphosted.com ([67.231.153.30]:52400 "EHLO mx0a-00082601.pphosted.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751531AbeCUFQX (ORCPT ); Wed, 21 Mar 2018 01:16:23 -0400 Subject: Re: [PATCH net-next v3 2/2] net: bpf: add a test for skb_segment in test_bpf module To: Eric Dumazet , , , , , , CC: References: <20180320232156.3455738-1-yhs@fb.com> <20180320232156.3455738-3-yhs@fb.com> <890b597a-85b6-a3fc-3419-8cace6d0f2b7@gmail.com> From: Yonghong Song Message-ID: <75786a0f-ca4f-df89-c658-04592f467946@fb.com> Date: Tue, 20 Mar 2018 22:15:31 -0700 MIME-Version: 1.0 In-Reply-To: <890b597a-85b6-a3fc-3419-8cace6d0f2b7@gmail.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: netdev-owner@vger.kernel.org List-ID: On 3/20/18 5:44 PM, Eric Dumazet wrote: > > > On 03/20/2018 04:21 PM, Yonghong Song wrote: >> Without the previous commit, >> "modprobe test_bpf" will have the following errors: >> ... >> [ 98.149165] ------------[ cut here ]------------ >> [ 98.159362] kernel BUG at net/core/skbuff.c:3667! >> [ 98.169756] invalid opcode: 0000 [#1] SMP PTI >> [ 98.179370] Modules linked in: >> [ 98.179371] test_bpf(+) >> ... >> which triggers the bug the previous commit intends to fix. >> >> The skbs are constructed to mimic what mlx5 may generate. >> The packet size/header may not mimic real cases in production. But >> the processing flow is similar. >> >> Signed-off-by: Yonghong Song >> --- >> lib/test_bpf.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++- >> 1 file changed, 70 insertions(+), 1 deletion(-) >> >> diff --git a/lib/test_bpf.c b/lib/test_bpf.c >> index 2efb213..045d7d3 100644 >> --- a/lib/test_bpf.c >> +++ b/lib/test_bpf.c >> @@ -6574,6 +6574,72 @@ static bool exclude_test(int test_id) >> return test_id < test_range[0] || test_id > test_range[1]; >> } >> >> +static struct sk_buff *build_test_skb(void *page) >> +{ >> + u32 headroom = NET_SKB_PAD + NET_IP_ALIGN + ETH_HLEN; >> + struct sk_buff *skb[2]; >> + int i, data_size = 8; >> + >> + for (i = 0; i < 2; i++) { >> + /* this will set skb[i]->head_frag */ >> + skb[i] = build_skb(page, headroom); >> + if (!skb[i]) >> + return NULL; > > You are using the same virtual address (page) for both skb ? > > So we have 2 skbs having skb->head pointing to the same location ? Thanks, Eric. This is purely due to my 'laziness' to make it work as I know that skb_segment does not really enforce this. I will address all of your comments in the next revision. > > This is illegal. > > Please use instead : skb = dev_alloc_skb(headroom + data_size) > >> + >> + skb_reserve(skb[i], headroom); >> + skb_put(skb[i], data_size); >> + skb[i]->protocol = htons(ETH_P_IP); >> + skb_reset_network_header(skb[i]); >> + skb_set_mac_header(skb[i], -ETH_HLEN); >> + >> + skb_add_rx_frag(skb[i], > > skb_shinfo(skb[i])->nr_frags, > > 0 ? > >> + page, 0, 64, 64); > > get_page(page) ? > >> + // skb: skb_headlen(skb[i]): 8, skb[i]->head_frag = 1 >> + } >> + >> + /* setup shinfo */ >> + skb_shinfo(skb[0])->gso_size = 1448; >> + skb_shinfo(skb[0])->gso_type = SKB_GSO_TCPV4; >> + skb_shinfo(skb[0])->gso_type |= SKB_GSO_DODGY; >> + skb_shinfo(skb[0])->gso_segs = 0; >> + skb_shinfo(skb[0])->frag_list = skb[1]; >> + >> + /* adjust skb[0]'s len */ >> + skb[0]->len += skb[1]->len; >> + skb[0]->data_len += skb[1]->data_len; >> + skb[0]->truesize += skb[1]->truesize; >> + >> + return skb[0]; >> +} >> + >> +static __init int test_skb_segment(void) >> +{ >> + netdev_features_t features; >> + struct sk_buff *skb; >> + void *page; >> + int ret = -1; >> + >> + page = (void *)__get_free_page(GFP_KERNEL | __GFP_ZERO); >> + if (!page) { >> + pr_info("%s: failed to get_free_page!", __func__); >> + return ret; >> + } >> + >> + features = NETIF_F_SG | NETIF_F_GSO_PARTIAL | NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM; >> + features |= NETIF_F_RXCSUM; >> + skb = build_test_skb(page); >> + if (!skb) { >> + pr_info("%s: failed to build_test_skb", __func__); >> + } else if (skb_segment(skb, features)) { >> + ret = 0; >> + pr_info("%s: success in skb_segment!", __func__); >> + } else { >> + pr_info("%s: failed in skb_segment!", __func__); >> + } >> + free_page((unsigned long)page); > > > Where are the skbs freed ? > > >> + return ret; >> +} >> + >> static __init int test_bpf(void) >> { >> int i, err_cnt = 0, pass_cnt = 0; >> @@ -6632,8 +6698,11 @@ static int __init test_bpf_init(void) >> return ret; >> >> ret = test_bpf(); >> - >> destroy_bpf_tests(); >> + if (ret) >> + return ret; >> + >> + ret = test_skb_segment(); >> return ret; >> } >> >>