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

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

[Paul Blakey]

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);
-- 
2.30.1

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

[Eric Dumazet]

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() ?

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

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 1760 bytes --]

Hi Paul,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on net/master]

url:    https://github.com/0day-ci/linux/commits/Paul-Blakey/skbuff-Release-nfct-refcount-on-napi-stolen-or-re-used-skbs/20210525-171507
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git 8c42a49738f16af0061f9ae5c2f5a955f268d9e3
config: i386-randconfig-r005-20210525 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
        # https://github.com/0day-ci/linux/commit/d4340b4d15d7b5f156b533d1ab0b3ae75ed479d3
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Paul-Blakey/skbuff-Release-nfct-refcount-on-napi-stolen-or-re-used-skbs/20210525-171507
        git checkout d4340b4d15d7b5f156b533d1ab0b3ae75ed479d3
        # save the attached .config to linux build tree
        make W=1 ARCH=i386 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   net/core/skbuff.c: In function 'napi_skb_free_stolen_head':
>> net/core/skbuff.c:942:2: error: implicit declaration of function 'nf_conntrack_put' [-Werror=implicit-function-declaration]
     942 |  nf_conntrack_put(skb_nfct(skb));
         |  ^~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors


vim +/nf_conntrack_put +942 net/core/skbuff.c

   939	
   940	void napi_skb_free_stolen_head(struct sk_buff *skb)
   941	{
 > 942		nf_conntrack_put(skb_nfct(skb));
   943		skb_dst_drop(skb);
   944		skb_ext_put(skb);
   945		napi_skb_cache_put(skb);
   946	}
   947	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 32848 bytes --]

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

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 1807 bytes --]

Hi Paul,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on net/master]

url:    https://github.com/0day-ci/linux/commits/Paul-Blakey/skbuff-Release-nfct-refcount-on-napi-stolen-or-re-used-skbs/20210525-171507
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git 8c42a49738f16af0061f9ae5c2f5a955f268d9e3
config: i386-randconfig-r005-20210525 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
        # https://github.com/0day-ci/linux/commit/d4340b4d15d7b5f156b533d1ab0b3ae75ed479d3
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Paul-Blakey/skbuff-Release-nfct-refcount-on-napi-stolen-or-re-used-skbs/20210525-171507
        git checkout d4340b4d15d7b5f156b533d1ab0b3ae75ed479d3
        # save the attached .config to linux build tree
        make W=1 ARCH=i386 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   net/core/skbuff.c: In function 'napi_skb_free_stolen_head':
>> net/core/skbuff.c:942:2: error: implicit declaration of function 'nf_conntrack_put' [-Werror=implicit-function-declaration]
     942 |  nf_conntrack_put(skb_nfct(skb));
         |  ^~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors


vim +/nf_conntrack_put +942 net/core/skbuff.c

   939	
   940	void napi_skb_free_stolen_head(struct sk_buff *skb)
   941	{
 > 942		nf_conntrack_put(skb_nfct(skb));
   943		skb_dst_drop(skb);
   944		skb_ext_put(skb);
   945		napi_skb_cache_put(skb);
   946	}
   947	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 32848 bytes --]

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

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2037 bytes --]

Hi Paul,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on net/master]

url:    https://github.com/0day-ci/linux/commits/Paul-Blakey/skbuff-Release-nfct-refcount-on-napi-stolen-or-re-used-skbs/20210525-171507
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git 8c42a49738f16af0061f9ae5c2f5a955f268d9e3
config: x86_64-randconfig-r032-20210525 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 99155e913e9bad5f7f8a247f8bb3a3ff3da74af1)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # https://github.com/0day-ci/linux/commit/d4340b4d15d7b5f156b533d1ab0b3ae75ed479d3
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Paul-Blakey/skbuff-Release-nfct-refcount-on-napi-stolen-or-re-used-skbs/20210525-171507
        git checkout d4340b4d15d7b5f156b533d1ab0b3ae75ed479d3
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> net/core/skbuff.c:942:2: error: implicit declaration of function 'nf_conntrack_put' [-Werror,-Wimplicit-function-declaration]
           nf_conntrack_put(skb_nfct(skb));
           ^
   1 error generated.


vim +/nf_conntrack_put +942 net/core/skbuff.c

   939	
   940	void napi_skb_free_stolen_head(struct sk_buff *skb)
   941	{
 > 942		nf_conntrack_put(skb_nfct(skb));
   943		skb_dst_drop(skb);
   944		skb_ext_put(skb);
   945		napi_skb_cache_put(skb);
   946	}
   947	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 34135 bytes --]

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

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2087 bytes --]

Hi Paul,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on net/master]

url:    https://github.com/0day-ci/linux/commits/Paul-Blakey/skbuff-Release-nfct-refcount-on-napi-stolen-or-re-used-skbs/20210525-171507
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git 8c42a49738f16af0061f9ae5c2f5a955f268d9e3
config: x86_64-randconfig-r032-20210525 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 99155e913e9bad5f7f8a247f8bb3a3ff3da74af1)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # https://github.com/0day-ci/linux/commit/d4340b4d15d7b5f156b533d1ab0b3ae75ed479d3
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Paul-Blakey/skbuff-Release-nfct-refcount-on-napi-stolen-or-re-used-skbs/20210525-171507
        git checkout d4340b4d15d7b5f156b533d1ab0b3ae75ed479d3
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> net/core/skbuff.c:942:2: error: implicit declaration of function 'nf_conntrack_put' [-Werror,-Wimplicit-function-declaration]
           nf_conntrack_put(skb_nfct(skb));
           ^
   1 error generated.


vim +/nf_conntrack_put +942 net/core/skbuff.c

   939	
   940	void napi_skb_free_stolen_head(struct sk_buff *skb)
   941	{
 > 942		nf_conntrack_put(skb_nfct(skb));
   943		skb_dst_drop(skb);
   944		skb_ext_put(skb);
   945		napi_skb_cache_put(skb);
   946	}
   947	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 34135 bytes --]

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

[Paul Blakey]

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.