netdev - Re: [net-next PATCH 4/5] udp: Do not copy destructor if one is not present

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]

Date:   Thu, 3 May 2018 18:58:10 -0700
From:   Eric Dumazet <eric.dumazet@...il.com>
To:     Alexander Duyck <alexander.duyck@...il.com>,
        netdev@...r.kernel.org, willemb@...gle.com, davem@...emloft.net
Subject: Re: [net-next PATCH 4/5] udp: Do not copy destructor if one is not
 present



On 05/03/2018 05:33 PM, Alexander Duyck wrote:
> From: Alexander Duyck <alexander.h.duyck@...el.com>
> 
> This patch makes it so that if a destructor is not present we avoid trying
> to update the skb socket or any reference counting that would be associated
> with the NULL socket and/or descriptor. By doing this we can support
> traffic coming from another namespace without any issues.
> 
> Signed-off-by: Alexander Duyck <alexander.h.duyck@...el.com>
> ---
>  net/ipv4/udp_offload.c |   19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
> index fd94bbb369b2..52760660d674 100644
> --- a/net/ipv4/udp_offload.c
> +++ b/net/ipv4/udp_offload.c
> @@ -195,6 +195,7 @@ struct sk_buff *__udp_gso_segment(struct sk_buff *gso_skb,
>  	unsigned int sum_truesize = 0;
>  	struct udphdr *uh;
>  	unsigned int mss;
> +	bool copy_dtor;
>  	__sum16 check;
>  	__be16 newlen;
>  
> @@ -208,12 +209,14 @@ struct sk_buff *__udp_gso_segment(struct sk_buff *gso_skb,
>  	skb_pull(gso_skb, sizeof(*uh));
>  
>  	/* clear destructor to avoid skb_segment assigning it to tail */
> -	WARN_ON_ONCE(gso_skb->destructor != sock_wfree);
> +	copy_dtor = gso_skb->destructor == sock_wfree;
> +	WARN_ON_ONCE(gso_skb->destructor && !copy_dtor);

Why not simply clear gso_skb->destructor only if copy_dtor is true ?

Then we can get rid of this WARN_ON_ONCE()

>  	gso_skb->destructor = NULL;
>  
>  	segs = skb_segment(gso_skb, features);
>  	if (unlikely(IS_ERR_OR_NULL(segs))) {
> -		gso_skb->destructor = sock_wfree;
> +		if (copy_dtor)
> +			gso_skb->destructor = sock_wfree;
>  		return segs;
>  	}
>  
> @@ -234,9 +237,11 @@ struct sk_buff *__udp_gso_segment(struct sk_buff *gso_skb,
>  					    (__force u32)newlen));
>  
>  	for (;;) {
> -		seg->destructor = sock_wfree;
> -		seg->sk = sk;
> -		sum_truesize += seg->truesize;
> +		if (copy_dtor) {
> +			seg->destructor = sock_wfree;
> +			seg->sk = sk;
> +			sum_truesize += seg->truesize;
> +		}
>  
>  		if (!seg->next)
>  			break;
> @@ -268,7 +273,9 @@ struct sk_buff *__udp_gso_segment(struct sk_buff *gso_skb,
>  		uh->check = gso_make_checksum(seg, ~check);
>  
>  	/* update refcount for the packet */
> -	refcount_add(sum_truesize - gso_skb->truesize, &sk->sk_wmem_alloc);
> +	if (copy_dtor)
> +		refcount_add(sum_truesize - gso_skb->truesize,
> +			     &sk->sk_wmem_alloc);
>  out:
>  	return segs;
>  }
>