netdev - Re: [PATCH net] skbuff: Release nfct refcount on napi stolen or re-used skbs

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] [day] [month] [year] [list]

Date:   Thu, 27 May 2021 16:30:57 +0300
From:   Paul Blakey <paulb@...dia.com>
To:     Eric Dumazet <eric.dumazet@...il.com>
CC:     <netdev@...r.kernel.org>, "David S. Miller" <davem@...emloft.net>,
        "Jakub Kicinski" <kuba@...nel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        "Daniel Borkmann" <daniel@...earbox.net>,
        John Fastabend <john.fastabend@...il.com>,
        Saeed Mahameed <saeedm@...dia.com>,
        Oz Shlomo <ozsh@...dia.com>, Roi Dayan <roid@...dia.com>,
        Vlad Buslov <vladbu@...dia.com>
Subject: Re: [PATCH net] skbuff: Release nfct refcount on napi stolen or
 re-used skbs




On Tue, 25 May 2021, Eric Dumazet wrote:

> 
> 
> On 5/25/21 11:13 AM, Paul Blakey wrote:
> > When multiple SKBs are merged to a new skb under napi GRO,
> > or SKB is re-used by napi, if nfct was set for them in the
> > driver, it will not be released while freeing their stolen
> > head state or on re-use.
> > 
> > Release nfct on napi's stolen or re-used SKBs.
> > 
> > Fixes: 5c6b94604744 ("net/mlx5e: CT: Handle misses after executing CT action")
> > Reviewed-by: Roi Dayan <roid@...dia.com>
> > Signed-off-by: Paul Blakey <paulb@...dia.com>
> > ---
> >  net/core/dev.c    | 1 +
> >  net/core/skbuff.c | 1 +
> >  2 files changed, 2 insertions(+)
> > 
> > diff --git a/net/core/dev.c b/net/core/dev.c
> > index ef8cf7619baf..a5324ca7dc65 100644
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> > @@ -6243,6 +6243,7 @@ static void napi_reuse_skb(struct napi_struct *napi, struct sk_buff *skb)
> >  	skb_shinfo(skb)->gso_type = 0;
> >  	skb->truesize = SKB_TRUESIZE(skb_end_offset(skb));
> >  	skb_ext_reset(skb);
> > +	nf_reset_ct(skb);
> >  
> >  	napi->skb = skb;
> >  }
> > diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> > index 3ad22870298c..6127bab2fe2f 100644
> > --- a/net/core/skbuff.c
> > +++ b/net/core/skbuff.c
> > @@ -939,6 +939,7 @@ void __kfree_skb_defer(struct sk_buff *skb)
> >  
> >  void napi_skb_free_stolen_head(struct sk_buff *skb)
> >  {
> > +	nf_conntrack_put(skb_nfct(skb));
> >  	skb_dst_drop(skb);
> >  	skb_ext_put(skb);
> >  	napi_skb_cache_put(skb);
> > 
> 
> Sadly we are very consistently making GRO slow as hell.
> 
> Why merging SKB with different ct would be allowed ?
> 
> If we accept this patch, then you will likely add another check in gro_list_prepare() ?
> 
> 

Yes, good catch, I'll check if that's even possible in our case.
If so I'm going to add checking diff conntracks on the gro list,
and diff tc chains, both are metadata on the skb.