| Message ID | 1625482491-17536-1-git-send-email-paulb@nvidia.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [net,v3] skbuff: Release nfct refcount on napi stolen or re-used skbs | expand |
Hello: This patch was applied to netdev/net.git (refs/heads/master): On Mon, 5 Jul 2021 13:54:51 +0300 you 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, and > in gro_list_prepare, check conntrack metadata diff. > > [...] Here is the summary with links: - [net,v3] skbuff: Release nfct refcount on napi stolen or re-used skbs https://git.kernel.org/netdev/net/c/8550ff8d8c75 You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html
On 7/5/2021 3:54 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, and > in gro_list_prepare, check conntrack metadata diff. > > 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> FWIW this change broke builds with CONFIG_SKB_EXTENSIONS disabled and also does not guard against CONFIG_NET_TC_SKB_EXT being disabled as well: CC net/core/dev.o net/core/dev.c: In function 'gro_list_prepare': net/core/dev.c:6015:33: error: implicit declaration of function 'skb_ext_find'; did you mean 'skb_ext_copy'? [-Werror=implicit-function-declaration] struct tc_skb_ext *skb_ext = skb_ext_find(skb, TC_SKB_EXT); ^~~~~~~~~~~~ skb_ext_copy net/core/dev.c:6015:51: error: 'TC_SKB_EXT' undeclared (first use in this function); did you mean 'TC_U32_EAT'? struct tc_skb_ext *skb_ext = skb_ext_find(skb, TC_SKB_EXT); ^~~~~~~~~~ TC_U32_EAT net/core/dev.c:6015:51: note: each undeclared identifier is reported only once for each function it appears in net/core/dev.c:6020:19: error: dereferencing pointer to incomplete type 'struct tc_skb_ext' diffs |= p_ext->chain ^ skb_ext->chain; ^~ cc1: some warnings being treated as errors make[2]: *** [scripts/Makefile.build:273: net/core/dev.o] Error 1 make[1]: *** [scripts/Makefile.build:516: net/core] Error 2 make: *** [Makefile:1847: net] Error 2 Fix: https://lore.kernel.org/netdev/20210708041051.17851-1-f.fainelli@gmail.com/ > --- > Changelog: > v2->v1: > in napi_skb_free_stolen_head() use nf_reset_ct(skb) instead, so we also zero nfct ptr. > v1->v2: > Check for different flows based on CT and chain metadata in gro_list_prepare > > net/core/dev.c | 13 +++++++++++++ > net/core/skbuff.c | 1 + > 2 files changed, 14 insertions(+) > > diff --git a/net/core/dev.c b/net/core/dev.c > index 439faadab0c2..bf62cb2ec6da 100644 > --- a/net/core/dev.c > +++ b/net/core/dev.c > @@ -5981,6 +5981,18 @@ static void gro_list_prepare(const struct list_head *head, > diffs = memcmp(skb_mac_header(p), > skb_mac_header(skb), > maclen); > + > + diffs |= skb_get_nfct(p) ^ skb_get_nfct(skb); > + > + if (!diffs) { > + struct tc_skb_ext *skb_ext = skb_ext_find(skb, TC_SKB_EXT); > + struct tc_skb_ext *p_ext = skb_ext_find(p, TC_SKB_EXT); > + > + diffs |= (!!p_ext) ^ (!!skb_ext); > + if (!diffs && unlikely(skb_ext)) > + diffs |= p_ext->chain ^ skb_ext->chain; > + } > + > NAPI_GRO_CB(p)->same_flow = !diffs; > } > } > @@ -6243,6 +6255,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 bbc3b4b62032..30ca61d91b69 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_reset_ct(skb); > skb_dst_drop(skb); > skb_ext_put(skb); > napi_skb_cache_put(skb); > -- Florian
diff --git a/net/core/dev.c b/net/core/dev.c index 439faadab0c2..bf62cb2ec6da 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -5981,6 +5981,18 @@ static void gro_list_prepare(const struct list_head *head, diffs = memcmp(skb_mac_header(p), skb_mac_header(skb), maclen); + + diffs |= skb_get_nfct(p) ^ skb_get_nfct(skb); + + if (!diffs) { + struct tc_skb_ext *skb_ext = skb_ext_find(skb, TC_SKB_EXT); + struct tc_skb_ext *p_ext = skb_ext_find(p, TC_SKB_EXT); + + diffs |= (!!p_ext) ^ (!!skb_ext); + if (!diffs && unlikely(skb_ext)) + diffs |= p_ext->chain ^ skb_ext->chain; + } + NAPI_GRO_CB(p)->same_flow = !diffs; } } @@ -6243,6 +6255,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 bbc3b4b62032..30ca61d91b69 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_reset_ct(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, and in gro_list_prepare, check conntrack metadata diff. 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> --- Changelog: v2->v1: in napi_skb_free_stolen_head() use nf_reset_ct(skb) instead, so we also zero nfct ptr. v1->v2: Check for different flows based on CT and chain metadata in gro_list_prepare net/core/dev.c | 13 +++++++++++++ net/core/skbuff.c | 1 + 2 files changed, 14 insertions(+)