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: <6f634864-2937-6e32-ba9d-7fa7f2b576cb@redhat.com>
Date:   Wed, 18 Jan 2023 22:37:47 +0100
From:   Jesper Dangaard Brouer <jbrouer@...hat.com>
To:     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

(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?

[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);
>   
> 
>