Re: [PATCH net-next v3 2/2] net: bpf: add a test for skb_segment in test_bpf module

[Eric Dumazet <eric.dumazet@gmail.com>]

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;
>  }
>  
>