| Message ID | 20201117203355.389661-2-saeedm@nvidia.com |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
On Tue, 17 Nov 2020 12:33:55 -0800 Saeed Mahameed wrote: > From: Maxim Mikityanskiy <maximmi@mellanox.com> > > All GRO flows except one call skb->destructor, however, GRO_MERGED_FREE > doesn't do it in case of NAPI_GRO_FREE_STOLEN_HEAD. For better > consistency and to add resiliency against the drivers that may pass SKBs > with a destructor, this patch changes napi_skb_free_stolen_head to use > skb_release_head_state, which should perform all the needed cleanups, > including a call to the destructor. This way the code of GRO_MERGED_FREE > becomes similar to kfree_skb_partial. > > Fixes: e44699d2c280 ("net: handle NAPI_GRO_FREE_STOLEN_HEAD case also in napi_frags_finish()") > Fixes: d7e8883cfcf4 ("net: make GRO aware of skb->head_frag") > Signed-off-by: Maxim Mikityanskiy <maximmi@mellanox.com> > Reviewed-by: Tariq Toukan <tariqt@nvidia.com> > Signed-off-by: Saeed Mahameed <saeedm@nvidia.com> CC Eric for GRO expertise. Makes sense to me, but do you still need "net/mlx5e: Fix refcount leak on kTLS RX resync" even with this applied? > diff --git a/net/core/dev.c b/net/core/dev.c > index 82dc6b48e45f..85dcc7f19902 100644 > --- a/net/core/dev.c > +++ b/net/core/dev.c > @@ -6048,8 +6048,7 @@ EXPORT_SYMBOL(gro_find_complete_by_type); > > static void napi_skb_free_stolen_head(struct sk_buff *skb) > { > - skb_dst_drop(skb); > - skb_ext_put(skb); > + skb_release_head_state(skb); > kmem_cache_free(skbuff_head_cache, skb); > } >
On Wed, 18 Nov 2020 21:02:29 +0100 Eric Dumazet wrote: > On Wed, Nov 18, 2020 at 8:22 PM Jakub Kicinski <kuba@kernel.org> wrote: > > > > On Tue, 17 Nov 2020 12:33:55 -0800 Saeed Mahameed wrote: > > > From: Maxim Mikityanskiy <maximmi@mellanox.com> > > > > > > All GRO flows except one call skb->destructor, however, GRO_MERGED_FREE > > > doesn't do it in case of NAPI_GRO_FREE_STOLEN_HEAD. For better > > > consistency and to add resiliency against the drivers that may pass SKBs > > > with a destructor, this patch changes napi_skb_free_stolen_head to use > > > skb_release_head_state, which should perform all the needed cleanups, > > > including a call to the destructor. This way the code of GRO_MERGED_FREE > > > becomes similar to kfree_skb_partial. > > > > > > Fixes: e44699d2c280 ("net: handle NAPI_GRO_FREE_STOLEN_HEAD case also in napi_frags_finish()") > > > Fixes: d7e8883cfcf4 ("net: make GRO aware of skb->head_frag") > > > Signed-off-by: Maxim Mikityanskiy <maximmi@mellanox.com> > > > Reviewed-by: Tariq Toukan <tariqt@nvidia.com> > > > Signed-off-by: Saeed Mahameed <saeedm@nvidia.com> > > > > CC Eric for GRO expertise. > > Thanks for CCing me. > > Since when drivers can pass funny skbs with destructors ??? > > Can we please stop adding more cycles to _already_ expensive GRO ? I don't think they do that today much (save for the ktls optimization in mlx5 Maxim is fixing separately). But I believe the idea of early demux in XDP had been floated in the past. If we don't want that to happen we should document it (stating the obvious).
On Wed, Nov 18, 2020 at 9:14 PM Jakub Kicinski <kuba@kernel.org> wrote: > > On Wed, 18 Nov 2020 21:02:29 +0100 Eric Dumazet wrote: > > On Wed, Nov 18, 2020 at 8:22 PM Jakub Kicinski <kuba@kernel.org> wrote: > > > > > > On Tue, 17 Nov 2020 12:33:55 -0800 Saeed Mahameed wrote: > > > > From: Maxim Mikityanskiy <maximmi@mellanox.com> > > > > > > > > All GRO flows except one call skb->destructor, however, GRO_MERGED_FREE > > > > doesn't do it in case of NAPI_GRO_FREE_STOLEN_HEAD. For better > > > > consistency and to add resiliency against the drivers that may pass SKBs > > > > with a destructor, this patch changes napi_skb_free_stolen_head to use > > > > skb_release_head_state, which should perform all the needed cleanups, > > > > including a call to the destructor. This way the code of GRO_MERGED_FREE > > > > becomes similar to kfree_skb_partial. > > > > > > > > Fixes: e44699d2c280 ("net: handle NAPI_GRO_FREE_STOLEN_HEAD case also in napi_frags_finish()") > > > > Fixes: d7e8883cfcf4 ("net: make GRO aware of skb->head_frag") > > > > Signed-off-by: Maxim Mikityanskiy <maximmi@mellanox.com> > > > > Reviewed-by: Tariq Toukan <tariqt@nvidia.com> > > > > Signed-off-by: Saeed Mahameed <saeedm@nvidia.com> > > > > > > CC Eric for GRO expertise. > > > > Thanks for CCing me. > > > > Since when drivers can pass funny skbs with destructors ??? > > > > Can we please stop adding more cycles to _already_ expensive GRO ? > > I don't think they do that today much (save for the ktls optimization > in mlx5 Maxim is fixing separately). But I believe the idea of early > demux in XDP had been floated in the past. > > If we don't want that to happen we should document it (stating the > obvious). This is a patch targeting the net tree, with Fixes: tag pretending this is an old bug. How can we possibly merge two skbs if they have destructors ? We do not make sure it is even possible. Many destructors track skb->truesize against a socket wmem_alloc or rmem_alloc, this stuff can not possibly work, unless stronger checks in GRO, since GRO changes skb->truesize without checking skb->destructor. If skb has a destructor, just bypass GRO completely, this is the only thing we can do. This would be quite unfortunate to add such a check "just because someone tries to fool us" diff --git a/net/core/dev.c b/net/core/dev.c index 4bfdcd6b20e8836e2884c51c6ce349ed54130bfa..76f0a627b6a1ee02339a724ecb6e4dbade80501b 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -5920,7 +5920,7 @@ static enum gro_result dev_gro_receive(struct napi_struct *napi, struct sk_buff int same_flow; int grow; - if (netif_elide_gro(skb->dev)) + if (netif_elide_gro(skb->dev) || skb->destructor) goto normal; gro_head = gro_list_prepare(napi, skb);
On Wed, 2020-11-18 at 21:21 +0100, Eric Dumazet wrote: > On Wed, Nov 18, 2020 at 9:14 PM Jakub Kicinski <kuba@kernel.org> > wrote: > > On Wed, 18 Nov 2020 21:02:29 +0100 Eric Dumazet wrote: > > > On Wed, Nov 18, 2020 at 8:22 PM Jakub Kicinski <kuba@kernel.org> > > > wrote: > > > > On Tue, 17 Nov 2020 12:33:55 -0800 Saeed Mahameed wrote: > > > > > From: Maxim Mikityanskiy <maximmi@mellanox.com> > > > > > > > > > > All GRO flows except one call skb->destructor, however, > > > > > GRO_MERGED_FREE > > > > > doesn't do it in case of NAPI_GRO_FREE_STOLEN_HEAD. For > > > > > better > > > > > consistency and to add resiliency against the drivers that > > > > > may pass SKBs > > > > > with a destructor, this patch changes > > > > > napi_skb_free_stolen_head to use > > > > > skb_release_head_state, which should perform all the needed > > > > > cleanups, > > > > > including a call to the destructor. This way the code of > > > > > GRO_MERGED_FREE > > > > > becomes similar to kfree_skb_partial. > > > > > > > > > > Fixes: e44699d2c280 ("net: handle NAPI_GRO_FREE_STOLEN_HEAD > > > > > case also in napi_frags_finish()") > > > > > Fixes: d7e8883cfcf4 ("net: make GRO aware of skb->head_frag") > > > > > Signed-off-by: Maxim Mikityanskiy <maximmi@mellanox.com> > > > > > Reviewed-by: Tariq Toukan <tariqt@nvidia.com> > > > > > Signed-off-by: Saeed Mahameed <saeedm@nvidia.com> > > > > > > > > CC Eric for GRO expertise. > > > > > > Thanks for CCing me. > > > > > > Since when drivers can pass funny skbs with destructors ??? > > > > > > Can we please stop adding more cycles to _already_ expensive GRO > > > ? > > > > I don't think they do that today much (save for the ktls > > optimization > > in mlx5 Maxim is fixing separately). But I believe the idea of > > early > > demux in XDP had been floated in the past. > > > > If we don't want that to happen we should document it (stating the > > obvious). > > This is a patch targeting the net tree, with Fixes: tag pretending > this is an old bug. > > How can we possibly merge two skbs if they have destructors ? > > We do not make sure it is even possible. > > Many destructors track skb->truesize against a socket wmem_alloc or > rmem_alloc, > this stuff can not possibly work, unless stronger checks in GRO, > since > GRO changes skb->truesize > without checking skb->destructor. > > If skb has a destructor, just bypass GRO completely, this is the only > thing we can do. > This would be quite unfortunate to add such a check "just because > someone tries to fool us" > Thanks Eric !! We don't actually need this patch, as the kTLS SKBs are handled locally in the drivers, I think we don't need to add any extra check in the datapath and just enforce the policy somehow with debug macros maybe WARN_ONE_ONCE() I will drop this patch, but the XDP folks who are going to implement XDP early demux should take care of this themselves. > diff --git a/net/core/dev.c b/net/core/dev.c > index > 4bfdcd6b20e8836e2884c51c6ce349ed54130bfa..76f0a627b6a1ee02339a724ecb6 > e4dbade80501b > 100644 > --- a/net/core/dev.c > +++ b/net/core/dev.c > @@ -5920,7 +5920,7 @@ static enum gro_result dev_gro_receive(struct > napi_struct *napi, struct sk_buff > int same_flow; > int grow; > > - if (netif_elide_gro(skb->dev)) > + if (netif_elide_gro(skb->dev) || skb->destructor) > goto normal; > > gro_head = gro_list_prepare(napi, skb);
diff --git a/net/core/dev.c b/net/core/dev.c index 82dc6b48e45f..85dcc7f19902 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -6048,8 +6048,7 @@ EXPORT_SYMBOL(gro_find_complete_by_type); static void napi_skb_free_stolen_head(struct sk_buff *skb) { - skb_dst_drop(skb); - skb_ext_put(skb); + skb_release_head_state(skb); kmem_cache_free(skbuff_head_cache, skb); }