From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <netdev-owner@vger.kernel.org>
Received: from mx0a-00082601.pphosted.com ([67.231.145.42]:60084 "EHLO
        mx0a-00082601.pphosted.com" rhost-flags-OK-OK-OK-OK)
        by vger.kernel.org with ESMTP id S1752954AbeCUUPo (ORCPT
        <rfc822;netdev@vger.kernel.org>); Wed, 21 Mar 2018 16:15:44 -0400
Subject: Re: [PATCH net-next v4 2/2] net: bpf: add a test for skb_segment in
 test_bpf module
To: Eric Dumazet <eric.dumazet@gmail.com>, <edumazet@google.com>,
        <ast@fb.com>, <daniel@iogearbox.net>, <diptanu@fb.com>,
        <netdev@vger.kernel.org>
CC: <kernel-team@fb.com>
References: <20180321064722.1411857-1-yhs@fb.com>
 <20180321064722.1411857-3-yhs@fb.com>
 <435477a4-bf2d-f710-a1a0-f1957eb01f7a@gmail.com>
From: Yonghong Song <yhs@fb.com>
Message-ID: <03322dd3-05c1-2361-6b4d-96752029168c@fb.com>
Date: Wed, 21 Mar 2018 13:15:13 -0700
MIME-Version: 1.0
In-Reply-To: <435477a4-bf2d-f710-a1a0-f1957eb01f7a@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: <netdev.vger.kernel.org>



On 3/21/18 8:26 AM, Eric Dumazet wrote:
> 
> 
> On 03/20/2018 11:47 PM, Yonghong Song wrote:
>> +static __init int test_skb_segment(void)
>> +{
>> +	netdev_features_t features;
>> +	struct sk_buff *skb;
>> +	int ret = -1;
>> +
>> +	features = NETIF_F_SG | NETIF_F_GSO_PARTIAL | NETIF_F_IP_CSUM |
>> +		   NETIF_F_IPV6_CSUM;
>> +	features |= NETIF_F_RXCSUM;
>> +	skb = build_test_skb();
>> +	if (!skb) {
>> +		pr_info("%s: failed to build_test_skb", __func__);
>> +		goto done;
>> +	}
>> +
>> +	if (skb_segment(skb, features)) {
>> +		ret = 0;
>> +		pr_info("%s: success in skb_segment!", __func__);
>> +	} else {
>> +		pr_info("%s: failed in skb_segment!", __func__);
>> +	}
>> +	kfree_skb(skb);
> 
> If skb_segmen() was successful (original) skb was already freed.
> 
> kfree_skb(old_skb) should thus panic the box, if you run this code
> on a kernel having some debugging features like KASAN

I tried with KASAN. It does not panic.
Looking at the code in net/core/dev.c: validate_xmit_skb:

static struct sk_buff *validate_xmit_skb(struct sk_buff *skb, struct 
net_device *dev, bool *again)
...

         if (netif_needs_gso(skb, features)) {
                 struct sk_buff *segs;

                 segs = skb_gso_segment(skb, features);
                 if (IS_ERR(segs)) {
                         goto out_kfree_skb;
                 } else if (segs) {
                         consume_skb(skb);
                         skb = segs;
                 }
...
out_kfree_skb:
         kfree_skb(skb);

which also indicates kfree_skb/consume_skb probably is the right way
to free skb after skb_gso_segment/skb_segment.

This probably explains why my above kfree_skb(skb) does not crash.

> 
> So you must store in a variable the return of skb_segment(),
> to be able to free skb(s), using kfree_skb_list()

Totally agree. Will make the change. Thanks!

> 
> 
>> +done:
>> +	return ret;
>> +}
>> +