| Message ID | 1621934002-16711-1-git-send-email-paulb@nvidia.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] skbuff: Release nfct refcount on napi stolen or re-used skbs | expand |
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() ?
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
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
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.
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);
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(+)