| 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: <ed5283a7-674b-4c5a-aade-c4f220485ce8@redhat.com>
Date: Tue, 24 Jun 2025 08:39:36 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: "Chia-Yu Chang (Nokia)" <chia-yu.chang@...ia-bell-labs.com>,
"edumazet@...gle.com" <edumazet@...gle.com>,
"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
"corbet@....net" <corbet@....net>, "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>,
"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>,
"ij@...nel.org" <ij@...nel.org>, "ncardwell@...gle.com"
<ncardwell@...gle.com>,
"Koen De Schepper (Nokia)" <koen.de_schepper@...ia-bell-labs.com>,
"g.white@...lelabs.com" <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@...le.com" <vidhi_goel@...le.com>
Subject: Re: [PATCH v8 net-next 09/15] tcp: accecn: AccECN option
Hi,
I'm sorry for the late reply.
On 6/21/25 1:22 AM, Chia-Yu Chang (Nokia) wrote:
> From: Paolo Abeni <pabeni@...hat.com Sent: Tuesday, June 17, 2025 11:27 AM
>> 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 6/10/25 2:53 PM, chia-yu.chang@...ia-bell-labs.com wrote:
>>> @@ -294,6 +295,9 @@ struct tcp_sock {
>>> rate_app_limited:1; /* rate_{delivered,interval_us} limited? */
>>> u8 received_ce_pending:4, /* Not yet transmit cnt of received_ce */
>>> unused2:4;
>>> + u8 accecn_minlen:2,/* Minimum length of AccECN option sent */
>>> + est_ecnfield:2,/* ECN field for AccECN delivered
>>> + estimates */
>>
>> It's unclear to me why you didn't use the 4 bits avail in 'unused2', instead of adding more fragmented bitfields.
>>
> Hi Paolo,
>
> This is becuase some bits of unused2 will be used in latter patches.
I see. Still it would be more clear to use the avail unused space first.
The final effect/layout would be the same. Or add an explicit note in
the commit message.
>>> @@ -4236,6 +4375,7 @@ static int tcp_ack(struct sock *sk, const struct sk_buff *skb, int flag)
>>> if (tcp_ecn_mode_accecn(tp))
>>> ecn_count = tcp_accecn_process(sk, skb,
>>> tp->delivered -
>>> delivered,
>>> +
>>> + sack_state.delivered_bytes,
>>> &flag);
>>>
>>> tcp_in_ack_event(sk, flag);
>>> @@ -4275,6 +4415,7 @@ static int tcp_ack(struct sock *sk, const struct sk_buff *skb, int flag)
>>> if (tcp_ecn_mode_accecn(tp))
>>> ecn_count = tcp_accecn_process(sk, skb,
>>> tp->delivered -
>>> delivered,
>>> +
>>> + sack_state.delivered_bytes,
>>> &flag);
>>
>> The two above chunks suggest you could move more code into
>> tcp_accecn_process()
>
> I do not get your point here.
>
> These two chunks reflect a new argument is added to tcp_accecn_process().
>
> But the value of this argument is computed by other fnuctions already, so not sure how to move code into tcp_accecn_process().
My point is that the 2 above chunks are identical, so you could possibly
move more (idenical) code into the helper and reduce the code duplication.
>>> static void tcp_options_write(struct tcphdr *th, struct tcp_sock *tp,
>>> tp->duplicate_sack : tp->selective_acks;
>>> int this_sack;
>>>
>>> - *ptr++ = htonl((TCPOPT_NOP << 24) |
>>> - (TCPOPT_NOP << 16) |
>>> + *ptr++ = htonl((leftover_bytes << 16) |
>>> (TCPOPT_SACK << 8) |
>>> (TCPOLEN_SACK_BASE + (opts->num_sack_blocks *
>>>
>>> TCPOLEN_SACK_PERBLOCK)));
>>
>> Here leftover_size/bytes are consumed and not updated, which should be safe as they will not be used later in this function, but looks inconsistent.
>>
>> The whole options handling looks very fragile to me. I really would prefer something simpler (i.e. just use the avail space if any) if that would work.
>
> I would still use leftover_size/bytes, but make it more consistent.
>
> As this part of code already pass AccECN packetdrill tests.
>
>>
>>> @@ -957,6 +1068,17 @@ static unsigned int tcp_syn_options(struct sock *sk, struct sk_buff *skb,
>>> }
>>> }
>>>
>>> + /* Simultaneous open SYN/ACK needs AccECN option but not SYN */
>>> + if (unlikely((TCP_SKB_CB(skb)->tcp_flags & TCPHDR_ACK) &&
>>> + tcp_ecn_mode_accecn(tp) &&
>>> + sock_net(sk)->ipv4.sysctl_tcp_ecn_option &&
>>> + remaining >= TCPOLEN_ACCECN_BASE)) {
>>> + u32 saving = tcp_synack_options_combine_saving(opts);
>>> +
>>> + opts->ecn_bytes = synack_ecn_bytes;
>>> + remaining -= tcp_options_fit_accecn(opts, 0, remaining, saving);
>>> + }
>>> +
>>> bpf_skops_hdr_opt_len(sk, skb, NULL, NULL, 0, opts, &remaining);
>>>
>>> return MAX_TCP_OPTION_SPACE - remaining;
>>
>> [...]
>>> @@ -1036,6 +1159,14 @@ static unsigned int tcp_synack_options(const
>>> struct sock *sk,
>>>
>>> smc_set_option_cond(tcp_sk(sk), ireq, opts, &remaining);
>>>
>>> + if (treq->accecn_ok && sock_net(sk)->ipv4.sysctl_tcp_ecn_option &&
>>> + remaining >= TCPOLEN_ACCECN_BASE) {
>>> + u32 saving = tcp_synack_options_combine_saving(opts);
>>> +
>>> + opts->ecn_bytes = synack_ecn_bytes;
>>> + remaining -= tcp_options_fit_accecn(opts, 0, remaining, saving);
>>> + }
>>> +
>>> bpf_skops_hdr_opt_len((struct sock *)sk, skb, req, syn_skb,
>>> synack_type, opts, &remaining);
>>>
>>
>> The similarities of the above 2 chuncks hints you could move more code into tcp_options_fit_accecn().
>>
>> /P
>
> I also do not get it, because tcp_options_fit_accecn() will be called with different argument values.
>
> So, I would prefer to keep as it is.
AFAICS the 3 lines inside the if branch are identical. You could create
an helper for that.
Side note: I'm spending quite a bit of time trimming the irrelevant part
of the reply to make it as straightforward as possible. Please do the
same: having to navigate hundred of lines of unrelated quoted text to
find a single line of contents maximize the chances of missing it.
Thanks,
Paolo
/P
Powered by blists - more mailing lists