netdev - Re: [PATCH net-next V2 2/2] net: kfree_skb_list use kmem_cache_free

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: <b0fcf845-0abc-2f70-250e-0a1da8e93d2f@huawei.com>
Date:   Thu, 19 Jan 2023 10:39:29 +0800
From:   Yunsheng Lin <linyunsheng@...wei.com>
To:     Jesper Dangaard Brouer <jbrouer@...hat.com>,
        <netdev@...r.kernel.org>
CC:     <brouer@...hat.com>, Jakub Kicinski <kuba@...nel.org>,
        "David S. Miller" <davem@...emloft.net>, <edumazet@...gle.com>,
        <pabeni@...hat.com>
Subject: Re: [PATCH net-next V2 2/2] net: kfree_skb_list use
 kmem_cache_free_bulk

On 2023/1/19 5:37, Jesper Dangaard Brouer wrote:
> (related to syzbot issue[1])
> 
> On 13/01/2023 14.52, Jesper Dangaard Brouer wrote:
>> The kfree_skb_list function walks SKB (via skb->next) and frees them
>> individually to the SLUB/SLAB allocator (kmem_cache). It is more
>> efficient to bulk free them via the kmem_cache_free_bulk API.
>>
>> This patches create a stack local array with SKBs to bulk free while
>> walking the list. Bulk array size is limited to 16 SKBs to trade off
>> stack usage and efficiency. The SLUB kmem_cache "skbuff_head_cache"
>> uses objsize 256 bytes usually in an order-1 page 8192 bytes that is
>> 32 objects per slab (can vary on archs and due to SLUB sharing). Thus,
>> for SLUB the optimal bulk free case is 32 objects belonging to same
>> slab, but runtime this isn't likely to occur.
>>
>> The expected gain from using kmem_cache bulk alloc and free API
>> have been assessed via a microbencmark kernel module[1].
>>
>> The module 'slab_bulk_test01' results at bulk 16 element:
>>   kmem-in-loop Per elem: 109 cycles(tsc) 30.532 ns (step:16)
>>   kmem-bulk    Per elem: 64 cycles(tsc) 17.905 ns (step:16)
>>
>> More detailed description of benchmarks avail in [2].
>>
>> [1] https://github.com/netoptimizer/prototype-kernel/tree/master/kernel/mm
>> [2] https://github.com/xdp-project/xdp-project/blob/master/areas/mem/kfree_skb_list01.org
>>
>> V2: rename function to kfree_skb_add_bulk.
>>
>> Reviewed-by: Saeed Mahameed <saeed@...nel.org>
>> Signed-off-by: Jesper Dangaard Brouer <brouer@...hat.com>
>> ---
>>   net/core/skbuff.c |   40 +++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 39 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
>> index 007a5fbe284b..79c9e795a964 100644
>> --- a/net/core/skbuff.c
>> +++ b/net/core/skbuff.c
>> @@ -964,16 +964,54 @@ kfree_skb_reason(struct sk_buff *skb, enum skb_drop_reason reason)
>>   }
>>   EXPORT_SYMBOL(kfree_skb_reason);
>>   +#define KFREE_SKB_BULK_SIZE    16
>> +
>> +struct skb_free_array {
>> +    unsigned int skb_count;
>> +    void *skb_array[KFREE_SKB_BULK_SIZE];
>> +};
>> +
>> +static void kfree_skb_add_bulk(struct sk_buff *skb,
>> +                   struct skb_free_array *sa,
>> +                   enum skb_drop_reason reason)
>> +{
>> +    /* if SKB is a clone, don't handle this case */
>> +    if (unlikely(skb->fclone != SKB_FCLONE_UNAVAILABLE)) {
>> +        __kfree_skb(skb);
>> +        return;
>> +    }
>> +
>> +    skb_release_all(skb, reason);
>> +    sa->skb_array[sa->skb_count++] = skb;
>> +
>> +    if (unlikely(sa->skb_count == KFREE_SKB_BULK_SIZE)) {
>> +        kmem_cache_free_bulk(skbuff_head_cache, KFREE_SKB_BULK_SIZE,
>> +                     sa->skb_array);
>> +        sa->skb_count = 0;
>> +    }
>> +}
>> +
>>   void __fix_address
>>   kfree_skb_list_reason(struct sk_buff *segs, enum skb_drop_reason reason)
>>   {
>> +    struct skb_free_array sa;
>> +
>> +    sa.skb_count = 0;
>> +
>>       while (segs) {
>>           struct sk_buff *next = segs->next;
>>   +        skb_mark_not_on_list(segs);
> 
> The syzbot[1] bug goes way if I remove this skb_mark_not_on_list().
> 
> I don't understand why I cannot clear skb->next here?

Clearing skb->next seems unrelated, it may just increase the problem
recurrence probability.

Because It seems kfree_skb_list_reason() is also used to release skb in
shinfo->frag_list, which should go through the skb_unref() checking,
and this patch seems to skip the skb_unref() checking for skb in
shinfo->frag_list.

> 
> [1] https://lore.kernel.org/all/000000000000d58eae05f28ca51f@google.com/
> 
>>           if (__kfree_skb_reason(segs, reason))
>> -            __kfree_skb(segs);
>> +            kfree_skb_add_bulk(segs, &sa, reason);
>> +
>>           segs = next;
>>       }
>> +
>> +    if (sa.skb_count)
>> +        kmem_cache_free_bulk(skbuff_head_cache, sa.skb_count,
>> +                     sa.skb_array);
>>   }
>>   EXPORT_SYMBOL(kfree_skb_list_reason);
>>  
>>
> 
> .
>