On Wed, 2020-11-18 at 21:21 +0100, Eric Dumazet wrote: > On Wed, Nov 18, 2020 at 9:14 PM Jakub Kicinski <k...@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 <k...@kernel.org> > > > wrote: > > > > On Tue, 17 Nov 2020 12:33:55 -0800 Saeed Mahameed wrote: > > > > > From: Maxim Mikityanskiy <maxi...@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 <maxi...@mellanox.com> > > > > > Reviewed-by: Tariq Toukan <tar...@nvidia.com> > > > > > Signed-off-by: Saeed Mahameed <sae...@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);