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

From: Alexander Duyck <alexander.h.duyck@intel.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@intel.com>
---
 net/ipv4/udp_offload.c |   19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

On 05/03/2018 05:33 PM, Alexander Duyck wrote:
> From: Alexander Duyck <alexander.h.duyck@intel.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@intel.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;
>  }
>

On Thu, May 3, 2018 at 6:58 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
>
> On 05/03/2018 05:33 PM, Alexander Duyck wrote:
>> From: Alexander Duyck <alexander.h.duyck@intel.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@intel.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()

Sounds good. I think I had code similar to that in an earlier version
of this patch, but when I started breaking things up I got rid of this
code until I realized I needed it due to a kernel panic triggered by
running the offload over a VXLAN from another namespace.

Thanks.

- Alex