From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eric Dumazet Subject: Re: [net-next PATCH 4/5] udp: Do not copy destructor if one is not present Date: Thu, 3 May 2018 18:58:10 -0700 Message-ID: References: <20180504002817.4496.64616.stgit@localhost.localdomain> <20180504003339.4496.26712.stgit@localhost.localdomain> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit To: Alexander Duyck , netdev@vger.kernel.org, willemb@google.com, davem@davemloft.net Return-path: Received: from mail-pg0-f67.google.com ([74.125.83.67]:34505 "EHLO mail-pg0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751229AbeEDB6L (ORCPT ); Thu, 3 May 2018 21:58:11 -0400 Received: by mail-pg0-f67.google.com with SMTP id g20-v6so7722726pgv.1 for ; Thu, 03 May 2018 18:58:11 -0700 (PDT) In-Reply-To: <20180504003339.4496.26712.stgit@localhost.localdomain> Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: On 05/03/2018 05:33 PM, Alexander Duyck wrote: > From: Alexander Duyck > > 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 > --- > 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; > } >