Eric Dumazet wrote: > On Thu, May 9, 2024 at 8:58 PM Richard Gobert <richardbgobert@xxxxxxxxx> wrote: >> > >> >> Interesting, I think that is indeed a bug, that exists also in the current >> implementation. >> NAPI_GRO_CB(p)->ip_fixedid (is_atomic before we renamed it in this commit) >> is cleared as being part of NAPI_GRO_CB(skb)->zeroed in dev_gro_receive. > > And the code there seems wrong. > > A compiler can absolutely reorder things, I have seen this many times. > > I would play safe here, to make sure NAPI_GRO_CB(skb)->is_atomic = 1; > can not be lost. > > diff --git a/net/core/gro.c b/net/core/gro.c > index c7901253a1a8fc1e9425add77014e15b363a1623..6e4203ea4d54b8955a504e42633f7667740b796e > 100644 > --- a/net/core/gro.c > +++ b/net/core/gro.c > @@ -470,6 +470,7 @@ static enum gro_result dev_gro_receive(struct > napi_struct *napi, struct sk_buff > BUILD_BUG_ON(!IS_ALIGNED(offsetof(struct napi_gro_cb, zeroed), > sizeof(u32))); /* Avoid slow > unaligned acc */ > *(u32 *)&NAPI_GRO_CB(skb)->zeroed = 0; > + barrier(); > NAPI_GRO_CB(skb)->flush = skb_has_frag_list(skb); > NAPI_GRO_CB(skb)->is_atomic = 1; > NAPI_GRO_CB(skb)->count = 1; I removed NAPI_GRO_CB(skb)->is_atomic = 1 from dev_gro_receive in this series since it is not needed anymore, so going forward this issue is fixed. I understand your concern about previous versions, I could send a patch to net to settle this issue. WDYT?
