| Message ID | 20180320232156.3455738-3-yhs@fb.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: permit skb_segment on head_frag frag_list skb | expand |
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 <yhs@fb.com> > --- > 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 ? 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; > } > >
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 <yhs@fb.com> >> --- >> 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; >> } >> >>
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; + + 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, + page, 0, 64, 64); + // 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); + 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; }
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 <yhs@fb.com> --- lib/test_bpf.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 70 insertions(+), 1 deletion(-)