Re: [v13,net-next,09/15] tcp: accecn: retransmit SYN/ACK without AccECN option or non-AccECN SYN/ACK

[Paolo Abeni <pabeni@redhat.com>]

On 2/3/26 2:56 PM, Chia-Yu Chang (Nokia) wrote:
>> -----Original Message-----
>> From: Paolo Abeni <pabeni@redhat.com>
>> Sent: Tuesday, February 3, 2026 1:36 PM
>> To: Chia-Yu Chang (Nokia) <chia-yu.chang@nokia-bell-labs.com>
>> Subject: Re: [v13,net-next,09/15] tcp: accecn: retransmit SYN/ACK without AccECN option or non-AccECN SYN/ACK
>>
>>
>> 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.
>>
>>
>>
>> This is an AI-generated review of your patch. The human sending this email has considered the AI review valid, or at least plausible.
>>
>> For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
>> ---
>> tcp: accecn: retransmit SYN/ACK without AccECN option or non-AccECN SYN/ACK
>>
>> For Accurate ECN, the first SYN/ACK sent by the TCP server shall set the ACE flag (Table 1 of RFC9768) and the AccECN option to complete the capability negotiation. However, if the TCP server needs to retransmit such a SYN/ACK (for example, because it did not receive an ACK acknowledging its SYN/ACK, or received a second SYN requesting AccECN support), the TCP server retransmits the SYN/ACK without the AccECN option. This is because the SYN/ACK may be lost due to congestion, or a middlebox may block the AccECN option. Furthermore, if this retransmission also times out, to expedite connection establishment, the TCP server should retransmit the SYN/ACK with (AE,CWR,ECE) = (0,0,0) and without the AccECN option, while maintaining AccECN feedback mode.
>>
>> This complies with Section 3.2.3.2.2 of the AccECN spec RFC9768.
>>
>> This commit fixes a bug but lacks a Fixes: tag. Consider adding:
>>
>> Fixes: b40671b5ee58 ("tcp: accecn: AccECN option failure handling")
>>
>>> diff --git a/include/net/tcp_ecn.h b/include/net/tcp_ecn.h index
>>> a709fb1756eb..796c613b5ef3 100644
>>> --- a/include/net/tcp_ecn.h
>>> +++ b/include/net/tcp_ecn.h
>>> @@ -649,12 +649,22 @@ static inline void tcp_ecn_clear_syn(struct sock
>>> *sk, struct sk_buff *skb)  }
>>>
>>>  static inline void
>>> -tcp_ecn_make_synack(const struct request_sock *req, struct tcphdr
>>> *th)
>>> +tcp_ecn_make_synack(const struct request_sock *req, struct tcphdr *th,
>>> +                 enum tcp_synack_type synack_type)
>>>  {
>>> -     if (tcp_rsk(req)->accecn_ok)
>>> -             tcp_accecn_echo_syn_ect(th, tcp_rsk(req)->syn_ect_rcv);
>>> -     else if (inet_rsk(req)->ecn_ok)
>>> -             th->ece = 1;
>>> +     /* Accurate ECN shall retransmit SYN/ACK with ACE=0 if the
>>> +      * previously retransmitted SYN/ACK also times out.
>>> +      */
>>> +     if (!req->num_timeout || synack_type != TCP_SYNACK_RETRANS) {
>>                  ^^^^^^^^^^^^^^^^
>>
>> Does this condition match the intended behavior described in the commit message and code comment?
>>
>> The commit message states the TCP server should first "retransmit the SYN/ACK without the AccECN option" and then "if this retransmission also times out, to expedite connection establishment, the TCP server should retransmit the SYN/ACK with (AE,CWR,ECE) = (0,0,0)."
>>
>> The code comment states "retransmit SYN/ACK with ACE=0 if the previously retransmitted SYN/ACK also times out."
>>
>> Both describe a two-step process:
>> 1. First retransmission (num_timeout=1): without AccECN option, but WITH
>>    ACE flags
>> 2. Second retransmission (num_timeout=2): without AccECN option AND with
>>    ACE=(0,0,0)
>>
>> However, the condition `!req->num_timeout` evaluates to false when num_timeout >= 1, causing ACE flags to be cleared on the FIRST retransmission, not the second:
>>
>> - First retransmission (num_timeout=1): !1 is false, so we skip the if
>>   block and clear ACE flags to (0,0,0) in the else if
>> - Second retransmission (num_timeout=2): !2 is false, same behavior
>>
>> Should the condition be `req->num_timeout < 2` instead to match RFC9768 Section 3.2.3.2.2's requirement that ACE flags are only cleared after the previously retransmitted SYN/ACK times out?
>>
> Hi Paolo,
> 
> This is correct because the increment of num_timeout is done after this block.
> 
> There are 2 places doing the retransmissnios:
> 1. In tcp_timer.c, you can see that the retrnamission is done via "tcp_rtx_synack(sk, req)" and the counter increase via "req->num_timeout++".
> 
> 2. In inet_connection_sock.c, the retrnamission is done via "tcp_rtx_synack(sk_listener, req)", and the counter increases via "if (req->num_timeout++ == 0)"

Ok, I suspected I got lost (and the AI with me). No need to resubmit, I
think we are better of merging as-is.

Thanks,

Paolo