>From: Ilpo Järvinen <ij@xxxxxxxxxx> >Sent: Thursday, November 7, 2024 8:28 PM >To: Eric Dumazet <edumazet@xxxxxxxxxx> >Cc: Chia-Yu Chang (Nokia) <chia-yu.chang@xxxxxxxxxxxxxxxxxxx>; netdev@xxxxxxxxxxxxxxx; dsahern@xxxxxxxxx; davem@xxxxxxxxxxxxx; dsahern@xxxxxxxxxx; pabeni@xxxxxxxxxx; joel.granados@xxxxxxxxxx; kuba@xxxxxxxxxx; andrew+netdev@xxxxxxx; horms@xxxxxxxxxx; pablo@xxxxxxxxxxxxx; kadlec@xxxxxxxxxxxxx; netfilter-devel@xxxxxxxxxxxxxxx; coreteam@xxxxxxxxxxxxx; ncardwell@xxxxxxxxxx; Koen De Schepper (Nokia) <koen.de_schepper@xxxxxxxxxxxxxxxxxxx>; g.white@xxxxxxxxxxxxx; ingemar.s.johansson@xxxxxxxxxxxx; mirja.kuehlewind@xxxxxxxxxxxx; cheshire@xxxxxxxxx; rs.ietf@xxxxxx; Jason_Livingood@xxxxxxxxxxx; vidhi_goel@xxxxxxxxx >Subject: Re: [PATCH v5 net-next 09/13] gro: prevent ACE field corruption & better AccECN handling > > >CAUTION: This is an external email. Please be very careful when clicking links or opening attachments. See the URL nok.it/ext for additional information. > > > >On Thu, 7 Nov 2024, Eric Dumazet wrote: > >>On Tue, Nov 5, 2024 at 11:07 AM <chia-yu.chang@xxxxxxxxxxxxxxxxxxx> wrote: >>> >>> From: Ilpo Järvinen <ij@xxxxxxxxxx> >>> >>> 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@xxxxxxxxxx> >>> Signed-off-by: Chia-Yu Chang <chia-yu.chang@xxxxxxxxxxxxxxxxxxx> >>> --- >>> 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. Hi Eric and Ilpo,
