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

[Paul Blakey <paulb@nvidia.com>]

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@nvidia.com>
> > Signed-off-by: Paul Blakey <paulb@nvidia.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.