Re: [PATCH v5 net-next 09/13] gro: prevent ACE field corruption & better AccECN handling

["Ilpo Järvinen" <ij@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 4454 bytes --]

On Thu, 7 Nov 2024, Eric Dumazet wrote:

> On Tue, Nov 5, 2024 at 11:07 AM <chia-yu.chang@nokia-bell-labs.com> wrote:
> >
> > From: Ilpo Järvinen <ij@kernel.org>
> >
> > There are important differences in how the CWR field behaves
> > in RFC3168 and AccECN. With AccECN, CWR flag is part of the
> > ACE counter and its changes are important so adjust the flags
> > changed mask accordingly.
> >
> > Also, if CWR is there, set the Accurate ECN GSO flag to avoid
> > corrupting CWR flag somewhere.
> >
> > Signed-off-by: Ilpo Järvinen <ij@kernel.org>
> > Signed-off-by: Chia-Yu Chang <chia-yu.chang@nokia-bell-labs.com>
> > ---
> >  net/ipv4/tcp_offload.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/net/ipv4/tcp_offload.c b/net/ipv4/tcp_offload.c
> > index 0b05f30e9e5f..f59762d88c38 100644
> > --- a/net/ipv4/tcp_offload.c
> > +++ b/net/ipv4/tcp_offload.c
> > @@ -329,7 +329,7 @@ struct sk_buff *tcp_gro_receive(struct list_head *head, struct sk_buff *skb,
> >         th2 = tcp_hdr(p);
> >         flush = (__force int)(flags & TCP_FLAG_CWR);
> >         flush |= (__force int)((flags ^ tcp_flag_word(th2)) &
> > -                 ~(TCP_FLAG_CWR | TCP_FLAG_FIN | TCP_FLAG_PSH));
> > +                 ~(TCP_FLAG_FIN | TCP_FLAG_PSH));
> >         flush |= (__force int)(th->ack_seq ^ th2->ack_seq);
> >         for (i = sizeof(*th); i < thlen; i += 4)
> >                 flush |= *(u32 *)((u8 *)th + i) ^
> > @@ -405,7 +405,7 @@ void tcp_gro_complete(struct sk_buff *skb)
> >         shinfo->gso_segs = NAPI_GRO_CB(skb)->count;
> >
> >         if (th->cwr)
> > -               shinfo->gso_type |= SKB_GSO_TCP_ECN;
> > +               shinfo->gso_type |= SKB_GSO_TCP_ACCECN;
> >  }
> >  EXPORT_SYMBOL(tcp_gro_complete);
> >
> 
> I do not really understand this patch. How a GRO engine can know which
> ECN variant the peers are using ?

Hi Eric,

Thanks for taking a look.

GRO doesn't know. Setting SKB_GSO_TCP_ECN in case of not knowing can 
result in header change that corrupts ACE field. Thus, GRO has to assume 
the RFC3168 CWR offloading trick cannot be used anymore (unless it tracks 
the connection and knows the bits are used for RFC3168 which is something 
nobody is going to do).

The main point of SKB_GSO_TCP_ACCECN is to prevent SKB_GSO_TCP_ECN or 
NETIF_F_TSO_ECN offloading to be used for the skb as it would corrupt ACE 
field value.

SKB_GSO_TCP_ACCECN doesn't allow CWR bits change within a super-skb but 
the same CWR flag should be repeated for all segments. In a sense, it's 
simpler than RFC3168 offloading.

> SKB_GSO_TCP_ECN is also used from other points, what is your plan ?
> 
> git grep -n SKB_GSO_TCP_ECN
> drivers/net/ethernet/hisilicon/hns3/hns3_enet.c:3888:
> skb_shinfo(skb)->gso_type |= SKB_GSO_TCP_ECN;
> drivers/net/ethernet/mellanox/mlx5/core/en_rx.c:1291:
> skb_shinfo(skb)->gso_type |= SKB_GSO_TCP_ECN;
> drivers/net/ethernet/mellanox/mlx5/core/en_rx.c:1312:
> skb_shinfo(skb)->gso_type |= SKB_GSO_TCP_ECN;

These looked like they should be just changed to set SKB_GSO_TCP_ACCECN 
instead.

I don't anymore recall why I didn't change those when I made this patch 
long time ago, perhaps it was just an oversight or things have changed 
somehow since then.

> include/linux/netdevice.h:5061: BUILD_BUG_ON(SKB_GSO_TCP_ECN !=
> (NETIF_F_TSO_ECN >> NETIF_F_GSO_SHIFT));
> include/linux/skbuff.h:664:     SKB_GSO_TCP_ECN = 1 << 2,

Not relevant.

> include/linux/virtio_net.h:88:                  gso_type |= SKB_GSO_TCP_ECN;
> include/linux/virtio_net.h:161:         switch (gso_type & ~SKB_GSO_TCP_ECN) {
> include/linux/virtio_net.h:226:         if (sinfo->gso_type & SKB_GSO_TCP_ECN)

These need to be looked further what's going on as UAPI is also involved 
here.

> net/ipv4/tcp_offload.c:404:             shinfo->gso_type |= SKB_GSO_TCP_ECN;

This was changed above. :-)

> net/ipv4/tcp_output.c:389: skb_shinfo(skb)->gso_type |= SKB_GSO_TCP_ECN;

I'm pretty sure this relates to sending side which will be taken care by 
a later patch in the full series (not among these preparatory patches).


FYI, these TSO/GSO/GRO changes are what I was most unsure myself if I 
got everything right when I was working with this series a few years back 
so please keep that in mind while reviewing so my lack of knowledge 
doesn't end up breaking something. :-)

-- 
 i.