Re: [PATCHv8 net-next] tcp: Add TCP ROCCET congestion control module.

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] tcp_roccet: jiffies_to_usecs wrap-around traps state machine in DRAIN
- [High] tcp_roccet: unhandled DRAIN transition leaves jiffies in usecs field
- [High] tcp_roccet: ignoring packet loss in LAUNCH phase violates RFC 5681
- [High] tcp_roccet: direct snd_cwnd assignment bypasses PRR pacing on recovery
- [Medium] tcp_roccet: idle period recovery discards first ACK burst in rate tracking
- [Medium] tcp_roccet: 32-bit integer overflow in guard space limits throughput
- [Low] tcp_roccet: struct roccettcp size exceeds ICSK_CA_PRIV_SIZE
--

commit d642a91b63f63d245583e8b6f4edc513976b1e3f
Author: Tim Fuechsel <t.fuechsel@gmx.de>

tcp: Add TCP ROCCET congestion control module.

TCP ROCCET is a new congestion control algorithm based on TCP CUBIC
that improves its overall performance in cellular networks. By its mode
of function, CUBIC causes bufferbloat while it tries to detect the
available throughput of a network path. This is particularly a problem
with large buffers in mobile networks.

Link: https://doi.org/10.1109/VTC2023-Fall60731.2023.10333357
Link: https://doi.org/10.1109/WMNC.2016.7543932
Link: https://doi.org/10.23919/WONS68803.2026.11501781
Link: http://go.lu-h.de/roccet-2026-07-21

> diff --git a/net/ipv4/tcp_roccet.c b/net/ipv4/tcp_roccet.c
> new file mode 100644
> index 0000000000000..801b575e231aa
> --- /dev/null
> +++ b/net/ipv4/tcp_roccet.c
> @@ -0,0 +1,1320 @@

[ ... ]

> +static void update_ack_rate(struct roccettcp *ca, u32 acked, u32 now)
> +{
> +	const s32 idle_threshold = USEC_PER_SEC * 2;
> +	const s32 interval = USEC_PER_MSEC * 100;
> +
> +	/* Check if the last interval has elapsed.
> +	 *
> +	 * Check via time_between32, so that idle connections
> +	 * (for more than U32_MAX/2 usecs), also trigger a new update
> +	 * and don't stall the ack-counting.
> +	 */
> +	bool interval_elapsed = !time_between32(now, ca->last_ack_rate_time
> +						  - interval,
> +						  ca->last_ack_rate_time
> +						  + interval);
> +
> +	if (interval_elapsed) {
> +		/* Check if the connection was idle for *idle_threshold*
> +		 * seconds (e.g. no ACK for X seconds). If so, reset the ack
> +		 * counting as if a new connection was created.
> +		 */
> +		if (!time_between32(now, ca->last_ack_rate_time
> +					- idle_threshold,
> +					ca->last_ack_rate_time
> +					+ idle_threshold)) {
> +			ca->ack_rate_last_rate = 0;
> +			ca->last_ack_rate_time =
> +				jiffies_to_usecs(tcp_jiffies32);
> +			ca->curr_ack_rate = 0;
> +			ca->ack_rate_cnt = 0;

[Severity: Medium]
Are the newly received ACKs lost during idle period recovery?

When recovering from an idle period, the ack_rate_cnt is zeroed, but the acked
parameter is completely ignored. This appears to incorrectly calculate the ACK
rate as zero for the first 100ms interval by discarding the first burst of
ACKs.

> +		} else {
> +			/* start counting for the new interval */
> +			ca->last_ack_rate_time = now;
> +			ca->ack_rate_last_rate = ca->curr_ack_rate;

[ ... ]

> +static void roccet_enter_min_rtt_probe(struct sock *sk, u32 now)
> +{
> +	struct tcp_sock *tp = tcp_sk(sk);
> +	struct roccettcp *ca = inet_csk_ca(sk);
> +	u32 interval, probe_cwnd;
> +
> +	/* If probing is already set, there was a mix up.
> +	 * Continue anyway so we can recover.
> +	 */
> +	if (ca->rtt_probe_timers_set)
> +		pr_warn_once("TCP ROCCET: Probing time should not be set when entering RTT Probe\n");
> +
> +	/* Safeguard for an "infinite" probe. This should logically never
> +	 * happen but still safeguard it.
> +	 */
> +	if (ca->curr_rtt == U32_MAX) {
> +		pr_warn_once("TCP ROCCET: curr_rtt is U32_MAX, cannot enter RTT_PROBE\n");
> +		ca->state = ORBITER;
> +		return;
> +	}
> +
> +	/* Use field to store when we entered min RTT probing. Here we use
> +	 * jiffies to stay consistent with the epoch_start format.
> +	 */
> +	ca->next_min_rtt_probe = tcp_jiffies32;

[Severity: High]
Could an interrupted state transition leave a jiffies timestamp permanently
stranded in the next_min_rtt_probe field?

In roccet_enter_min_rtt_probe(), next_min_rtt_probe is overloaded with a
tcp_jiffies32 value. If a packet loss or ECN triggers roccet_congestion_event()
while in RTT_PROBE_REFILL, the state shifts directly to DRAIN.

When transitioning from DRAIN to ORBITER, roccet_handle_state_transitions()
fails to rewrite this timestamp (since prev_state is DRAIN, not a probe
state). Could this trap the connection in an infinite RTT_PROBE_ENTER loop
when ORBITER compares a large microsecond now against this small jiffies
value?

> +
> +	/* Start min RTT probing */

[ ... ]

> +static void roccet_orbiter_update(struct sock *sk, u32 acked)
> +{
> +	/* ORBITER: Increase the cwnd by using the CUBIC cwnd growth function,
> +	 * if no roccet congestion event is detected.
> +	 */
> +
> +	struct tcp_sock *tp = tcp_sk(sk);
> +	struct roccettcp *ca = inet_csk_ca(sk);
> +
> +	u32 now = jiffies_to_usecs(tcp_jiffies32);
> +	bool evaluate_srrtt = false;
> +	bool sent_more_than_acked = false;
> +	u32 roccet_xj, jitter, sent, received;
> +
> +	/* Enter DRAIN when roccet was recently triggered */
> +	if (ca->last_event_time_set &&
> +	    time_before32(now, ca->roccet_last_event_time_us +
> +			  100 * USEC_PER_MSEC)) {
> +		ca->state = DRAIN;
> +		return;
> +	}

[Severity: High]
Can jiffies_to_usecs() overflow and trap the connection in DRAIN?

At HZ=1000, jiffies_to_usecs(tcp_jiffies32) wraps around 32 bits every ~71.58
minutes. If a connection lives for more than 35.79 minutes (2^31 microseconds)
without a congestion event, the time delta flips sign, causing time_before32()
to erroneously evaluate to true on every subsequent packet.

> +
> +	/* Enter RTT_PROBE when the "timer" has expired.

[ ... ]

> +	/* Calculate if more bytes were sent than received
> +	 * in the time interval.
> +	 *
> +	 * Handle wrap arounds by relying on unsigned subtraction.
> +	 * e.g. if snd_nxt wraps to 10 and seq_start is U32_MAX - 10,
> +	 * the subtraction will result in the value of 21.
> +	 */
> +	sent = tp->snd_nxt - ca->interval_snd_seq_start;
> +	received = tp->snd_una - ca->interval_una_seq_start;
> +
> +	/* Check sent and received bytes from the previous interval.
> +	 * Here we use a guard space of 1% of the current cwnd.
> +	 * We do this to avoid a false positive evaluation due
> +	 * to delays caused by jitter or scheduling.
> +	 */
> +	sent_more_than_acked =
> +		sent >
> +		received + ((tcp_snd_cwnd(tp) * tp->mss_cache) / 100);

[Severity: Medium]
Can the multiplication of tcp_snd_cwnd(tp) and tp->mss_cache overflow here?

Since both are 32-bit unsigned integers, connections with high
bandwidth-delay products or large MTUs (e.g., Loopback with 64KB MSS) can
easily exceed 4.29 billion before the division by 100 occurs. This overflow
could falsely trigger a ROCCET congestion event and needlessly limit
throughput.

> +
> +	/* Check if it's time to evaluate the srRTT */

[ ... ]

> +static u32 roccet_recalc_ssthresh(struct sock *sk)
> +{
> +	const struct tcp_sock *tp = tcp_sk(sk);
> +	struct roccettcp *ca = inet_csk_ca(sk);
> +	u32 cwnd = tcp_snd_cwnd(tp);
> +
> +	/* In LAUNCH, we want no reduction on loss/ECN.
> +	 * On ECN this is set later on in roccet_state()
> +	 */
> +	if (ca->state == LAUNCH)
> +		return cwnd;

[Severity: High]
By returning cwnd unchanged instead of scaling it down during fast recovery,
does this ignore packet drops in the initial slow start phase? Refusing to
back off when router buffers overflow appears to violate RFC 5681 and could
risk severe congestion collapse.

> +
> +	/* In min RTT probe, use the cwnd before the probe to not undershoot */

[ ... ]

> +static void roccet_state(struct sock *sk, u8 new_state)
> +{
> +	struct roccettcp *ca = inet_csk_ca(sk);
> +	struct tcp_sock *tp = tcp_sk(sk);
> +	u32 cwnd;
> +	u32 now = jiffies_to_usecs(tcp_jiffies32);
> +	enum roccet_state prev_state = ca->state;
> +
> +	if (new_state == TCP_CA_Loss) {
> +		roccet_reset(sk, ca);
> +		tcp_snd_cwnd_set(tp, TCP_INIT_CWND);
> +	} else if (new_state == TCP_CA_CWR) {
> +		/* Handle CWR as ROCCET congestion event,
> +		 * however afterwards always set Wmax to the current cwnd
> +		 * except during min RTT probing.
> +		 */
> +		cwnd = tcp_snd_cwnd(tp);
> +		roccet_congestion_event(sk, now);
> +		if (ca->state != RTT_PROBE_ENTER && ca->state != RTT_PROBE &&
> +		    ca->state != RTT_PROBE_REFILL)
> +			ca->last_max_cwnd = cwnd;
> +	} else if (new_state == TCP_CA_Recovery) {
> +		/* Directly reduce cwnd and rely on pacing */
> +		cwnd = roccet_handle_recovery(sk);
> +		tcp_snd_cwnd_set(tp, cwnd);
> +	}

[Severity: High]
Does instantly halving the congestion window here bypass the internal
Proportional Rate Reduction (PRR) pacing mechanisms? By directly mutating
snd_cwnd inside the state machine callback, it appears the sender could be
artificially blocked from transmitting until a full half-window of ACKs
drains, causing large transmission stalls.

> +
> +	roccet_handle_state_transitions(sk, prev_state, now);

[ ... ]

> +static int __init roccet_register(void)
> +{
> +	int param_err;
> +
> +	BUILD_BUG_ON(sizeof(struct roccettcp) > ICSK_CA_PRIV_SIZE);

[Severity: Low]
Will this compile-time assertion fail deterministically?

The struct roccettcp requires 108 bytes of storage (24 u32s = 96 bytes,
3 u16s = 6 bytes, 2 bytes padding to align the 4-byte enum bitfield, making
108 bytes). The BUILD_BUG_ON() enforces a 104-byte limit for
ICSK_CA_PRIV_SIZE, which would cause a build failure on all platforms.

> +
> +	/* Check for valid parameter ranges and then precompute factors */

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/arK46Rr0cB7gJ7JS@volt-roccet-vm?part=1