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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <75786a0f-ca4f-df89-c658-04592f467946@fb.com>
Date:   Tue, 20 Mar 2018 22:15:31 -0700
From:   Yonghong Song <yhs@...com>
To:     Eric Dumazet <eric.dumazet@...il.com>, <edumazet@...gle.com>,
        <ast@...com>, <daniel@...earbox.net>, <diptanu@...com>,
        <netdev@...r.kernel.org>, <alexander.duyck@...il.com>
CC:     <kernel-team@...com>
Subject: Re: [PATCH net-next v3 2/2] net: bpf: add a test for skb_segment in
 test_bpf module



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