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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 12 potential issue(s) to consider:
- [Critical] `.cong_control` bypasses core PRR logic and actively increases CWND during loss recovery.
- [High] Incorrect sequence number wrap-around handling using `before()` causes spurious massive delta values and off-by-one errors.
- [High] Incorrect state machine hooking in `TCP_CA_Recovery` causes double CWND reduction and bypasses Proportional Rate Reduction (PRR).
- [High] ROCCET severely penalizes application-limited flows by actively halving their congestion window during slow start.
- [High] Stale `ece_received` flag causes subsequent unrelated packet losses to be incorrectly treated as ECN events.
- [High] Packet loss during the `RTT_PROBE` phase causes the congestion window to restore to its unreduced pre-probe size.
- [High] Deliberately ignores multiplicative decrease on packet loss during Slow Start.
- [Medium] Using 32-bit integers for microsecond timestamps breaks connection tracking on idle connections due to wrap-around.
- [Medium] Writable module parameters bypass bounds checks at runtime, desynchronizing scale factors and enabling overflows.
- [Medium] The `u16` limit on `ack_rate_cnt` saturates at high speeds (>7.8 Gbps), falsely triggering slow-start exits.
- [Medium] Immediate CWND drop on ECN bypasses TCP's graceful CWR drain.
- [Low] Using `0` as a sentinel for uninitialized sequence numbers breaks when the TCP sequence legitimately wraps to zero.
--

--- Patch [1]: [PATCHv4 net-next] tcp: Add TCP ROCCET congestion control module. ---
commit 02eaad1795bcc0571841fb03cecea583c8694e2f
Author: Tim Fuechsel <t.fuechsel@gmx.de>
tcp: Add TCP ROCCET congestion control module.

TCP ROCCET is an 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.

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..c611e263136b3
> --- /dev/null
> +++ b/net/ipv4/tcp_roccet.c
> @@ -0,0 +1,974 @@
[ ... ]
> +static int fast_convergence __read_mostly = 1;
> +static int beta __read_mostly = 717; /* = 717/1024 (BICTCP_BETA_SCALE) */
> +static int initial_ssthresh __read_mostly;
> +static int bic_scale __read_mostly = 41;
> +static int tcp_friendliness __read_mostly = 1;
> +
> +static u32 cube_rtt_scale __read_mostly;
> +static u32 beta_scale __read_mostly;
> +static u64 cube_factor __read_mostly;
> +
> +/* Note parameters that are used for precomputing scale factors are read-only */
> +module_param(fast_convergence, int, 0644);
> +MODULE_PARM_DESC(fast_convergence, "turn on/off fast convergence");
> +module_param(beta, int, 0644);

[Severity: Medium]
The beta module parameter is registered as writable (0644). Since validation
and generation of dependent statics (like beta_scale) only occur during
initialization, could a privileged user alter beta to a negative value at
runtime to bypass the beta <= 0 check?

Would this trigger an unsigned multiplication overflow in
(tcp_snd_cwnd(tp) * beta) inside roccet_congestion_event(), resulting in a
huge CWND while beta_scale remains stale?

[ ... ]
> +/* Update ack rate sampled by 100ms.
> + */
> +static __always_inline void update_ack_rate(struct sock *sk, u32 acked, u32 now)
> +{
> +	struct roccettcp *ca = inet_csk_ca(sk);
> +	s32 interval = USEC_PER_MSEC * 100;
> +
> +	s32 time_delta = (s32)(ca->ack_rate_last_rate_time - now);

[Severity: Medium]
A 32-bit microsecond counter wraps roughly every 71.5 minutes. If a connection
is idle for over 35.7 minutes, does the subtraction
ack_rate_last_rate_time - now overflow and evaluate to a massive positive
value?

Could this freeze ACK rate tracking, and cause the 
(s32)now - ca->roccet_last_event_time_us <= 100 * USEC_PER_MSEC
check in roccet_control() to remain perpetually true, locking the connection
in the DRAIN state indefinitely?

> +	const s32 idle_threshold = USEC_PER_SEC * 2;
[ ... ]
> +	} else {
> +		// Cap the ack count to avoid overflow
> +		ca->ack_rate_cnt = min_t(u32, ca->ack_rate_cnt + acked,
> +					 U16_MAX);

[Severity: Medium]
ack_rate_cnt is clamped to U16_MAX (65,535). In a 100ms interval with a
1500-byte MTU, does this limit tracking to approximately 7.8 Gbps?

If a flow runs faster than this, does the count saturate, causing
get_ack_rate_diff() to return 0 and forcing a premature exit from slow start
due to an interpreted bandwidth plateau?

> +	}
> +}
[ ... ]
> +static void roccettcp_cong_avoid(struct sock *sk, u32 ack, u32 acked)
> +{
> +	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 send_more_than_acked = false;
> +	u32 roccet_xj;
> +	u32 jitter;
> +	u32 send, received;
> +
> +	if (ca->state == LAUNCH) {
> +		/* LAUNCH: Detect an exit point for tcp slow start
> +		 * in networks with large buffers of multiple BDP
> +		 * Like in cellular networks (5G, ...).
> +		 * Or exit LAUNCH if cwnd is too large for application layer
> +		 * data rate (tcp cwnd validation).
> +		 */
> +		if ((ca->curr_srrtt > sr_rtt_upper_bound &&
> +		     get_ack_rate_diff(ca) <= ack_rate_diff_ss) ||
> +		    !tcp_is_cwnd_limited(sk)) {

[Severity: High]
In the LAUNCH phase, if the flow evaluates to !tcp_is_cwnd_limited(sk) because
the application temporarily paused sending data, does ROCCET explicitly force
an exit from slow start and halve both ssthresh and snd_cwnd?

Could this drastically reduce throughput for bursty or interactive workloads by
aggressively punishing applications for not having bulk data queued instantly?

> +			ca->epoch_start = 0;
[ ... ]
> +		/* Calculate if more bytes was send than received
> +		 * in the time interval.
> +		 */
> +		if (before(tp->snd_nxt, ca->interval_snd_seq_start)) {
> +			/* We had a wrap around in seq no counter */
> +			send = (~0U - ca->interval_snd_seq_start + tp->snd_nxt);
> +		} else {

[Severity: High]
When snd_nxt logically regresses (such as during retransmission or sequence
rewinds), the before() check evaluates to true. Does the mitigation
~0U - start + nxt evaluate to a massive positive number (near 4GB) in this
case, ensuring send_more_than_acked is true and falsely triggering
roccet_congestion_event()?

> +			send = (tp->snd_nxt - ca->interval_snd_seq_start);
> +		}
[ ... ]
> +static u32 roccettcp_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);
> +
> +	/* If a loss/ECN occurs in the refill phase of min RTT probing
> +	 * we reduce the cwnd and abort the refill.
> +	 */
> +	if (ca->state == RTT_PROBE_REFILL)
> +		ca->state = ORBITER;
> +
> +	/* If ROCCET is in min RTT probing and a loss/ECN occurs,
> +	 * we use the cwnd before the probing interval to
> +	 * calculate the cwnd reduction and continue probing.
> +	 * After min RTT probing the cwnd is set to the reduced
> +	 * value. During min RTT probing it is very likely that
> +	 * congestion was caused by the cwnd value before min
> +	 * RTT probing.
> +	 */
> +	if (ca->state == RTT_PROBE) {
> +		/* Handle ECN as cubic congestion event in min
> +		 * RTT probe.
> +		 */
> +		ca->ece_received = false;
> +
> +		ca->epoch_start = 0; /* end of epoch */
> +
> +		/* Wmax and fast convergence */
> +		if (cwnd < ca->last_max_cwnd && fast_convergence)
> +			ca->last_max_cwnd =
> +				(cwnd * (BICTCP_BETA_SCALE + beta)) /
> +				(2 * BICTCP_BETA_SCALE);
> +		else
> +			ca->last_max_cwnd = cwnd;
> +
> +		cwnd = ca->cwnd_before_min_rtt_probe;
> +		ca->cwnd_before_min_rtt_probe =
> +			max((cwnd * beta) / BICTCP_BETA_SCALE, 2U);
> +
> +		return cwnd;

[Severity: High]
If packet loss occurs while in RTT_PROBE, this function reduces
ca->cwnd_before_min_rtt_probe but returns the original, unreduced cwnd as the
new ssthresh. Because cwnd was halved for probing, does this create a state
where cwnd is much lower than ssthresh?

Would PRR then inflate the connection window to try and reach this erroneously
high ssthresh, disabling loss recovery and risking congestion collapse?

> +	}
> +
> +	/* Handle ECN as ROCCET congestion event. */
> +	if (ca->ece_received) {
> +		ca->ece_received = false;
> +		roccet_congestion_event(sk, jiffies_to_usecs(tcp_jiffies32));
> +		return tcp_snd_cwnd(tp);

[Severity: Medium]
When processing an ECN event, roccet_congestion_event() explicitly sets
tp->snd_cwnd to the reduced value via tcp_snd_cwnd_set. RFC 3168 designates
the .ssthresh callback to simply return the new threshold, allowing the CWR
state machine to gracefully drain the window by 1 segment per ACK. By forcing
it down directly, does ROCCET break this pacing?

> +	}
> +
> +	/* On loss in slow start enter congestion avoidance
> +	 * without a cwnd reduction. Additional slow start
> +	 * exit conditions with a cwnd reduction are handled
> +	 * in roccettcp_cong_avoid.
> +	 */
> +	if (tcp_in_slow_start(tp))
> +		return tcp_snd_cwnd(tp);

[Severity: High]
If tcp_in_slow_start(tp) is true, the callback returns the current, unreduced
cwnd as the new ssthresh. Does this fundamentally prevent cwnd from changing
despite packet loss, violating core TCP safety invariants that require a
multiplicative decrease upon packet loss?

> +
> +	/* CUBIC congestion event */
[ ... ]
> +static void roccettcp_state(struct sock *sk, u8 new_state)
> +{
> +	struct roccettcp *ca = inet_csk_ca(sk);
> +	struct tcp_sock *tp = tcp_sk(sk);
> +
> +	if (new_state == TCP_CA_Loss) {
> +		roccettcp_reset(ca);
> +	} else if (new_state == TCP_CA_Recovery) {
> +		tcp_sk(sk)->snd_ssthresh = roccettcp_recalc_ssthresh(sk);
> +		tcp_snd_cwnd_set(tp, tcp_sk(sk)->snd_ssthresh);

[Severity: High]
tcp_enter_recovery in the TCP core invokes the .ssthresh callback immediately
after set_state. By manually calling roccettcp_recalc_ssthresh(sk) and
explicitly overwriting snd_cwnd here, is roccettcp_recalc_ssthresh invoked
twice consecutively on the already-reduced CWND?

Also, does directly mutating snd_cwnd inside set_state defeat the core PRR
algorithm by instantaneously dropping the window instead of gracefully
pacing it?

> +	}
> +}
[ ... ]
> +static void roccet_in_ack_event(struct sock *sk, u32 flags)
> +{
> +	struct roccettcp *ca = inet_csk_ca(sk);
> +
> +	/* Handle ECE bit.
> +	 * Processing of ECE events is done in roccettcp_recalc_ssthresh()
> +	 */
> +	if (flags & CA_ACK_ECE)
> +		ca->ece_received = true;

[Severity: High]
The ca->ece_received flag is set in the in_ack_event hook, which executes at
the end of tcp_ack(). Since the core stack calls the .ssthresh callback much
earlier, is the initial ECN marking treated as standard loss because the flag
is false at that time?

When a subsequent unrelated packet loss occurs, does .ssthresh see the stale
true flag, clear it, and wrongly delegate to ECN handling, failing to
correctly reduce the window?

> +}
> +
> +static void roccet_control(struct sock *sk, u32 ack, int flag,
> +			   const struct rate_sample *rs)
> +{
> +	struct tcp_sock *tp = tcp_sk(sk);
> +	struct roccettcp *ca = inet_csk_ca(sk);
> +
> +	u32 now = jiffies_to_usecs(tcp_jiffies32);
> +	u64 rate;
> +
> +	/* Update roccet parameters */
> +	update_ack_rate(sk, rs->acked_sacked, now);
> +	update_min_rtt(sk);
> +	update_srrtt(sk);
> +
> +	/* Set values for send and receive rate */
> +	if (ca->interval_snd_seq_start == 0) {
> +		ca->interval_snd_seq_start = tp->snd_nxt;
> +		ca->interval_una_seq_start = tp->snd_una;
> +	}

[Severity: Low]
The initialization check treats 0 as a sentinel. If the TCP sequence number
legitimately wraps exactly to 0, does the logic assume the tracking interval
is uninitialized and reset the interval start values unexpectedly?

> +
> +	/* Update roccet state */
> +	if (tcp_in_slow_start(tp)) {
> +		ca->state = LAUNCH;
> +	} else if ((s32)now - ca->roccet_last_event_time_us <=
> +		   100 * USEC_PER_MSEC) {
> +		ca->state = DRAIN;
> +	} else if (after(now, ca->next_min_rtt_probe) ||
> +		   ca->state == RTT_PROBE || ca->state == RTT_PROBE_REFILL) {
> +		if (ca->state != RTT_PROBE_REFILL)
> +			ca->state = RTT_PROBE;
> +		roccet_min_rtt_probe(sk, now);
> +	} else {
> +		ca->state = ORBITER;
> +	}
> +
> +	/* If nothing was fully acked do not increase the cwnd */
> +	if (!rs->acked_sacked)
> +		return;
> +
> +	/* Increase the cwnd.
> +	 * Loss recovery is handled in roccettcp_state()
> +	 */
> +	roccettcp_cong_avoid(sk, ack, rs->acked_sacked);

[Severity: Critical]
By implementing the .cong_control hook, ROCCET overrides the TCP core's
tcp_cong_control() and the kernel stops managing PRR. However,
roccet_control() unconditionally calls roccettcp_cong_avoid() without
verifying if the socket is in TCP_CA_Recovery.

During severe packet loss, does every ACK cause ROCCET to continuously
increase its window via the CUBIC growth function, exacerbating the loss?

> +
> +	/* Adjust pacing rate. The code here is similar to the

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