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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 11 potential issue(s) to consider:
- [High] tcp_roccet: ignores packet loss during LAUNCH slow-start phase
- [High] tcp_roccet: timer wraparound to exactly 0 causes permanent DRAIN state lockup
- [High] tcp_roccet: double cwnd reduction and last_max_cwnd corruption during TCP_CA_CWR
- [High] tcp_roccet: ACK rate metric 16-bit saturation falsely aborts slow start
- [High] tcp_roccet: forces TCP_INIT_CWND on RTO, injecting severe bursts
- [Medium] tcp_roccet: ack_carry_over from slow start exit is lost to tcp_cong_avoid_ai
- [Medium] tcp_roccet: 32-bit usecs timestamp wraps after 35 minutes, freezing idle connections
- [Medium] tcp_roccet: zero-microsecond RTT samples are discarded
- [Medium] tcp_roccet: 32-bit sequence tracking wraps in 5 RTT measurement window
- [Medium] tcp_roccet: writable sysfs parameters lack runtime bounds validation
- [Low] tcp_roccet: struct roccettcp exceeds ICSK_CA_PRIV_SIZE limit
--

commit e3cfdf8398561ab8f09dd45039b191e26c9801e8
Author: Tim Fuechsel <t.fuechsel@gmx.de>
Subject: tcp: Add TCP ROCCET congestion control module.

TCP ROCCET is a new congestion control algorithm based on TCP CUBIC that
improves performance in cellular networks by mitigating bufferbloat. It
adds a modified slow start and monitors RTT and acknowledgment arrival
rates to adjust the send rate.

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..10ad4748d994d
> --- /dev/null
> +++ b/net/ipv4/tcp_roccet.c
[ ... ]
> +struct roccettcp {
> +	u32 cnt;	/* increase cwnd by 1 after ACKs */
[ ... ]
> +	u16 ack_rate_cnt;	/* Used for counting acks */
> +
> +	enum roccet_state state : 3; /* Current operating state of roccet */

[Severity: Low]
Does the transition from u16 to an enum bitfield force a 4-byte alignment
here?

It appears this introduces 2 bytes of padding, expanding the struct beyond
104 bytes and causing the BUILD_BUG_ON(sizeof(struct roccettcp) >
ICSK_CA_PRIV_SIZE) assertion in roccet_register() to fail at compile time.

[ ... ]
> +static int beta __read_mostly = 717; /* = 717/1024 (BICTCP_BETA_SCALE) */
[ ... ]
> +module_param(beta, int, 0644);

[Severity: Medium]
Since beta is writable by root at runtime (0644), shouldn't there be a
runtime bounds validation check?

The current param_check() is only executed during initialization. If beta
is written to a negative value at runtime, it could cause an unsigned
integer overflow in roccet_congestion_event() during congestion window
scaling calculations.

[ ... ]
> +static void update_ack_rate(struct roccettcp *ca, u32 acked, u32 now)
> +{
[ ... ]
> +		} else {
> +			/* start counting for the new interval */
> +			ca->ack_rate_last_rate_time = now;
> +			ca->ack_rate_last_rate = ca->ack_rate_curr_rate;
> +			ca->ack_rate_curr_rate = ca->ack_rate_cnt;
> +			ca->ack_rate_cnt = min_t(u32, acked, U16_MAX);
> +		}
> +
> +		ca->was_idle = false;
> +	} else {
> +		/* Cap the ack count to avoid overflow */
> +		ca->ack_rate_cnt = min_t(u32, ca->ack_rate_cnt + acked,
> +					 U16_MAX);

[Severity: High]
Does artificially capping this measurement to U16_MAX flatten the tracked
rate on high-speed networks?

On networks exceeding ~7.8 Gbps, the connection will receive more than
65535 ACKs per 100ms interval. This saturation would constantly yield a
rate difference of 0, falsely signaling that throughput has plateaued and
causing roccet_launch_update() to prematurely abort slow start.

[ ... ]
> +static void roccet_congestion_event(struct sock *sk, u32 now)
> +{
[ ... ]
> +	ca->epoch_start = 0;
> +	ca->roccet_last_event_time_us = now;

[Severity: High]
Can this assign a wrapping microsecond timestamp to a field that later
treats 0 as a special uninitialized sentinel?

The 32-bit microsecond timestamp generated in roccet_control() will wrap
to exactly 0 every ~6.2 days. If a congestion event occurs at that precise
millisecond, roccet_last_event_time_us becomes 0.

This would cause roccet_drain_update() to bypass its interval check
permanently, trapping the connection in the DRAIN state.

[ ... ]
> +static void roccet_min_rtt_probe(struct sock *sk, u32 now)
> +{
[ ... ]
> +	if (ca->probe_min_rtt_until == 0)
> +		pr_warn_once("ROCCET: Probing time should be set");
> +	else if (time_before32(now, ca->probe_min_rtt_until))

[Severity: High]
Could this sentinel check falsely trigger if the calculated deadline wrapped
to exactly 0?

Similar to the issue above, because it treats 0 as uninitialized, a probe
duration that naturally lands on 0 during wraparound will mysteriously
abort the min RTT probing phase.

[ ... ]
> +static void roccet_launch_update(struct sock *sk, u32 acked)
> +{
[ ... ]
> +	/* If cwnd hits ssthresh, go to ORBITER and if any ACKs are
> +	 * leftover save them for ORBITER.
> +	 * Check via tcp_in_slow_start() in case no ACKs are left.
> +	 */
> +	if (!tcp_in_slow_start(tp)) {
> +		ca->state = ORBITER;
> +		ca->epoch_start = 0;
> +		ca->ack_carry_over = acked;
> +	}
> +}

[Severity: Medium]
When leftover ACKs are saved in ack_carry_over, are they ever applied to
the actual congestion window growth?

Looking at roccet_orbiter_update(), it calls bictcp_update() which adds
the carryover to ack_cnt, but that is only used for TCP friendliness
fallback calculations. The subsequent call to tcp_cong_avoid_ai() only
takes the current round's acked value, meaning the carried-over ACKs are
permanently discarded.

[ ... ]
> +static void roccet_orbiter_update(struct sock *sk, u32 acked)
> +{
[ ... ]
> +	/* 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;

[Severity: Medium]
Will this calculation be subject to catastrophic 32-bit sequence wrap-around
on networks operating at 100+ Gbps?

Since this measures bytes transferred over a 5 RTT measurement window,
transferring more than 4 Gigabytes in that time will wrap the 32-bit
sequence subtraction and break the sent_more_than_acked evaluation.

[ ... ]
> +static u32 roccet_recalc_ssthresh(struct sock *sk)
> +{
[ ... ]
> +	/* 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]
Does forcing ssthresh to remain at the full congestion window size despite
an active congestion event bypass standard congestion control safeguards?

Ignoring packet loss during the LAUNCH slow-start phase entirely prevents
the sender from backing off during severe congestion, which can lead to
persistent buffer overflow.

[ ... ]
> +static u32 roccet_handle_recovery(struct sock *sk)
> +{
[ ... ]
> +	/* On loss in LAUNCH, enter ORBITER without a cwnd reduction. */
> +	if (ca->state == LAUNCH) {
> +		ca->state = ORBITER;
> +		return cwnd;
> +	}

[Severity: High]
Does returning the unreduced cwnd intentionally circumvent standard cwnd
reduction upon Fast Recovery during the initial growth phase?

As above, this completely ignores network congestion signals and violates
TCP congestion control safety invariants.

[ ... ]
> +static void roccet_state(struct sock *sk, u8 new_state)
> +{
[ ... ]
> +	if (new_state == TCP_CA_Loss) {
> +		roccet_reset(sk, ca);
> +		tcp_snd_cwnd_set(tp, TCP_INIT_CWND);

[Severity: High]
Does explicitly overriding the TCP core's safe cwnd reduction of 1 with
TCP_INIT_CWND cause severe congestion bursts?

Injecting up to 10 packets immediately following a full Retransmission
Timeout violently floods an already deeply congested network.

> +	} else if (new_state == TCP_CA_CWR) {
> +		/* Handle CWR as ROCCET congestion event,
> +		 * however afterwards always set Wmax to the current cwnd.
> +		 */
> +		cwnd = tcp_snd_cwnd(tp);
> +		roccet_congestion_event(sk, now);
> +		ca->last_max_cwnd = cwnd;
> +	} else if (new_state == TCP_CA_Recovery) {

[Severity: High]
Is the congestion window getting penalized twice for TCP_CA_CWR?

The TCP core already halves cwnd prior to calling this state callback.
Passing the already-halved cwnd into roccet_congestion_event() triggers a
redundant congestion event.

Additionally, blindly assigning this doubly-penalized value to
last_max_cwnd permanently corrupts CUBIC's target growth metric.

[ ... ]
> +static void roccet_acked(struct sock *sk, const struct ack_sample *sample)
> +{
[ ... ]
> +	/* Get valid sample for roccet */
> +	if (sample->rtt_us > 0) {
> +		ca->last_rtt = ca->curr_rtt;
> +		ca->curr_rtt = sample->rtt_us;
> +	}
> +}

[Severity: Medium]
Does discarding valid 0-microsecond RTT samples completely disable ROCCET
on ultra-low latency links?

On loopback or ultra-low latency interfaces, sub-microsecond RTTs are
rounded down to 0. By ignoring them entirely instead of treating them as
1us, curr_rtt is prevented from ever initializing past U32_MAX, permanently
breaking the congestion avoidance logic.

[ ... ]
> +static void roccet_drain_update(struct sock *sk, u32 now)
> +{
> +	struct tcp_sock *tp = tcp_sk(sk);
> +	struct roccettcp *ca = inet_csk_ca(sk);
> +
> +	if (ca->roccet_last_event_time_us &&
> +	    time_after32(now, ca->roccet_last_event_time_us +
> +			 100 * USEC_PER_MSEC)) {

[Severity: Medium]
Can this condition incorrectly fail if the connection idles for more than
35.7 minutes?

Because now is a 32-bit microsecond timestamp, the subtraction in
time_after32() will exceed its signed boundary if the time elapsed is
greater than 2^31 microseconds. If an application idles and then resumes,
it could be incorrectly trapped in the DRAIN state, freezing window growth
until the timer wraps again.

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