netdev - Re: [RFC] skb: avoid unnecessary reallocations in __skb

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: <1338294848.2840.15.camel@edumazet-glaptop>
Date:	Tue, 29 May 2012 14:34:08 +0200
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	Felix Fietkau <nbd@...nwrt.org>
Cc:	netdev@...r.kernel.org
Subject: Re: [RFC] skb: avoid unnecessary reallocations in __skb_cow

On Sun, 2012-05-27 at 17:26 +0200, Felix Fietkau wrote:
> At the beginning of __skb_cow, headroom gets set to a minimum of
> NET_SKB_PAD. This causes unnecessary reallocations if the buffer was not
> cloned and the headroom is just below NET_SKB_PAD, but still more than the
> amount requested by the caller.
> This was showing up frequently in my tests on VLAN tx, where
> vlan_insert_tag calls skb_cow_head(skb, VLAN_HLEN).
> 
> Fix this by only setting the headroom delta if either there is less
> headroom than specified by the caller, or if reallocation has to be done
> anyway because the skb was cloned.
> 
> Signed-off-by: Felix Fietkau <nbd@...nwrt.org>
> ---
>  include/linux/skbuff.h |    9 ++++++---
>  1 files changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index 0e50171..1898471 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -1894,12 +1894,15 @@ static inline int skb_clone_writable(const struct sk_buff *skb, unsigned int len
>  static inline int __skb_cow(struct sk_buff *skb, unsigned int headroom,
>  			    int cloned)
>  {
> +	unsigned int alloc_headroom = headroom;
>  	int delta = 0;
>  
>  	if (headroom < NET_SKB_PAD)
> -		headroom = NET_SKB_PAD;
> -	if (headroom > skb_headroom(skb))
> -		delta = headroom - skb_headroom(skb);
> +		alloc_headroom = NET_SKB_PAD;
> +	if (headroom > skb_headroom(skb) ||
> +	    (cloned && alloc_headroom > skb_headroom(skb))) {
> +		delta = alloc_headroom - skb_headroom(skb);
> +	}
>  
>  	if (delta || cloned)
>  		return pskb_expand_head(skb, ALIGN(delta, NET_SKB_PAD), 0,

Nice catch.

Scratching my head on this one. Why not the obvious fix ?

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 0e50171..b534a1b 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -1896,8 +1896,6 @@ static inline int __skb_cow(struct sk_buff *skb, unsigned int headroom,
 {
 	int delta = 0;
 
-	if (headroom < NET_SKB_PAD)
-		headroom = NET_SKB_PAD;
 	if (headroom > skb_headroom(skb))
 		delta = headroom - skb_headroom(skb);
 


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html