| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID:
<PAXPR07MB7984AF8DDBDD9B646C899E07A3892@PAXPR07MB7984.eurprd07.prod.outlook.com>
Date: Tue, 6 May 2025 08:53:16 +0000
From: "Chia-Yu Chang (Nokia)" <chia-yu.chang@...ia-bell-labs.com>
To: Ilpo Järvinen <ij@...nel.org>
CC: Paolo Abeni <pabeni@...hat.com>, "horms@...nel.org" <horms@...nel.org>,
"dsahern@...nel.org" <dsahern@...nel.org>, "kuniyu@...zon.com"
<kuniyu@...zon.com>, "bpf@...r.kernel.org" <bpf@...r.kernel.org>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>, "dave.taht@...il.com"
<dave.taht@...il.com>, "jhs@...atatu.com" <jhs@...atatu.com>,
"kuba@...nel.org" <kuba@...nel.org>, "stephen@...workplumber.org"
<stephen@...workplumber.org>, "xiyou.wangcong@...il.com"
<xiyou.wangcong@...il.com>, "jiri@...nulli.us" <jiri@...nulli.us>,
"davem@...emloft.net" <davem@...emloft.net>, "edumazet@...gle.com"
<edumazet@...gle.com>, "andrew+netdev@...n.ch" <andrew+netdev@...n.ch>,
"donald.hunter@...il.com" <donald.hunter@...il.com>, "ast@...erby.net"
<ast@...erby.net>, "liuhangbin@...il.com" <liuhangbin@...il.com>,
"shuah@...nel.org" <shuah@...nel.org>, "linux-kselftest@...r.kernel.org"
<linux-kselftest@...r.kernel.org>, "ncardwell@...gle.com"
<ncardwell@...gle.com>, "Koen De Schepper (Nokia)"
<koen.de_schepper@...ia-bell-labs.com>, g.white <g.white@...lelabs.com>,
"ingemar.s.johansson@...csson.com" <ingemar.s.johansson@...csson.com>,
"mirja.kuehlewind@...csson.com" <mirja.kuehlewind@...csson.com>,
"cheshire@...le.com" <cheshire@...le.com>, "rs.ietf@....at" <rs.ietf@....at>,
"Jason_Livingood@...cast.com" <Jason_Livingood@...cast.com>, vidhi_goel
<vidhi_goel@...le.com>
Subject: RE: [PATCH v5 net-next 10/15] tcp: accecn: AccECN option send control
> -----Original Message-----
> From: Ilpo Järvinen <ij@...nel.org>
> Sent: Tuesday, May 6, 2025 1:27 AM
> To: Chia-Yu Chang (Nokia) <chia-yu.chang@...ia-bell-labs.com>
> Cc: Paolo Abeni <pabeni@...hat.com>; horms@...nel.org; dsahern@...nel.org; kuniyu@...zon.com; bpf@...r.kernel.org; netdev@...r.kernel.org; dave.taht@...il.com; jhs@...atatu.com; kuba@...nel.org; stephen@...workplumber.org; xiyou.wangcong@...il.com; jiri@...nulli.us; davem@...emloft.net; edumazet@...gle.com; andrew+netdev@...n.ch; donald.hunter@...il.com; ast@...erby.net; liuhangbin@...il.com; shuah@...nel.org; linux-kselftest@...r.kernel.org; ncardwell@...gle.com; Koen De Schepper (Nokia) <koen.de_schepper@...ia-bell-labs.com>; g.white <g.white@...lelabs.com>; ingemar.s.johansson@...csson.com; mirja.kuehlewind@...csson.com; cheshire@...le.com; rs.ietf@....at; Jason_Livingood@...cast.com; vidhi_goel <vidhi_goel@...le.com>
> Subject: RE: [PATCH v5 net-next 10/15] tcp: accecn: AccECN option send control
>
>
> 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 Mon, 5 May 2025, Chia-Yu Chang (Nokia) wrote:
>
> > > -----Original Message-----
> > > From: Paolo Abeni <pabeni@...hat.com>
> > > Sent: Tuesday, April 29, 2025 2:10 PM
> > > To: Chia-Yu Chang (Nokia) <chia-yu.chang@...ia-bell-labs.com>;
> > > horms@...nel.org; dsahern@...nel.org; kuniyu@...zon.com;
> > > bpf@...r.kernel.org; netdev@...r.kernel.org; dave.taht@...il.com;
> > > jhs@...atatu.com; kuba@...nel.org; stephen@...workplumber.org;
> > > xiyou.wangcong@...il.com; jiri@...nulli.us; davem@...emloft.net;
> > > edumazet@...gle.com; andrew+netdev@...n.ch; donald.hunter@...il.com;
> > > ast@...erby.net; liuhangbin@...il.com; shuah@...nel.org;
> > > linux-kselftest@...r.kernel.org; ij@...nel.org;
> > > ncardwell@...gle.com; Koen De Schepper (Nokia)
> > > <koen.de_schepper@...ia-bell-labs.com>; g.white
> > > <g.white@...lelabs.com>; ingemar.s.johansson@...csson.com;
> > > mirja.kuehlewind@...csson.com; cheshire@...le.com; rs.ietf@....at;
> > > Jason_Livingood@...cast.com; vidhi_goel <vidhi_goel@...le.com>
> > > Subject: Re: [PATCH v5 net-next 10/15] tcp: accecn: AccECN option
> > > send control
> > >
> > >
> > > 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 4/22/25 5:35 PM, chia-yu.chang@...ia-bell-labs.com wrote:
> > > > From: Ilpo Järvinen <ij@...nel.org>
> > > >
> > > > Instead of sending the option in every ACK, limit sending to those
> > > > ACKs where the option is necessary:
> > > > - Handshake
> > > > - "Change-triggered ACK" + the ACK following it. The
> > > > 2nd ACK is necessary to unambiguously indicate which
> > > > of the ECN byte counters in increasing. The first
> > > > ACK has two counters increasing due to the ecnfield
> > > > edge.
> > > > - ACKs with CE to allow CEP delta validations to take
> > > > advantage of the option.
> > > > - Force option to be sent every at least once per 2^22
> > > > bytes. The check is done using the bit edges of the
> > > > byte counters (avoids need for extra variables).
> > > > - AccECN option beacon to send a few times per RTT even if
> > > > nothing in the ECN state requires that. The default is 3
> > > > times per RTT, and its period can be set via
> > > > sysctl_tcp_ecn_option_beacon.
> > > >
> > > > Signed-off-by: Ilpo Järvinen <ij@...nel.org>
> > > > Co-developed-by: Chia-Yu Chang <chia-yu.chang@...ia-bell-labs.com>
> > > > Signed-off-by: Chia-Yu Chang <chia-yu.chang@...ia-bell-labs.com>
> > > > ---
> > > > include/linux/tcp.h | 3 +++
> > > > include/net/netns/ipv4.h | 1 +
> > > > include/net/tcp.h | 1 +
> > > > net/ipv4/sysctl_net_ipv4.c | 9 ++++++++
> > > > net/ipv4/tcp.c | 5 ++++-
> > > > net/ipv4/tcp_input.c | 36 +++++++++++++++++++++++++++++++-
> > > > net/ipv4/tcp_ipv4.c | 1 +
> > > > net/ipv4/tcp_minisocks.c | 2 ++
> > > > net/ipv4/tcp_output.c | 42 ++++++++++++++++++++++++++++++--------
> > > > 9 files changed, 90 insertions(+), 10 deletions(-)
>
> > > > diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c index
> > > > 3f3e285fc973..2e95dad66fe3 100644
> > > > --- a/net/ipv4/tcp_ipv4.c
> > > > +++ b/net/ipv4/tcp_ipv4.c
> > > > @@ -3451,6 +3451,7 @@ static int __net_init tcp_sk_init(struct net
> > > > *net) {
> > > > net->ipv4.sysctl_tcp_ecn = 2;
> > > > net->ipv4.sysctl_tcp_ecn_option = 2;
> > > > + net->ipv4.sysctl_tcp_ecn_option_beacon = 3;
> > > > net->ipv4.sysctl_tcp_ecn_fallback = 1;
> > >
> > > Human readable macros instead of magic numbers could help.
> >
> > OK, commments will be added here.
>
> Hi,
>
> Using named defines to replace literals would be more useful than comments (names can be grepped for, do not fall out-of-sync with code, etc.).
>
> > > > @@ -1237,13 +1253,18 @@ static unsigned int
> > > > tcp_established_options(struct sock *sk, struct sk_buff *skb
> > > >
> > > > if (tcp_ecn_mode_accecn(tp) &&
> > > > sock_net(sk)->ipv4.sysctl_tcp_ecn_option) {
> > > > - int saving = opts->num_sack_blocks > 0 ? 2 : 0;
> > > > - int remaining = MAX_TCP_OPTION_SPACE - size;
> > > > -
> > > > - opts->ecn_bytes = tp->received_ecn_bytes;
> > > > - size += tcp_options_fit_accecn(opts, tp->accecn_minlen,
> > > > - remaining,
> > > > - saving);
> > > > + if (sock_net(sk)->ipv4.sysctl_tcp_ecn_option >= 2 ||
> > > > + tp->accecn_opt_demand ||
> > > > + tcp_accecn_option_beacon_check(sk)) {
> > >
> > > Why a nested if here and just not expanding the existing one?
> >
> > Sure, will merge them.
>
> While I don't remember everything that well anymore, this might have been to reduce code churn in some later patch, so it might be worth to check it first (that patch might even fall outside of this series now that these are split into multiple chunks).
>
> --
> i.
Hi Ilpo,
Thanks for this raised point.
And I've checked that the condition is changed at the latter patches ("tcp: accecn: AccECN option failure handling"), but with the similar nested if.
So, I would try to merge it at the next version, and will check packetdrill for sure.
Chia-Yu
Powered by blists - more mailing lists