tcp: Add TCP ROCCET congestion control module.

net/ipv4/tcp_roccet.c line 197

[Severity: Critical]
Does this initialization logic safely handle early ACKs without valid RTT samples?

roccettcp_reset() initializes the struct with zeroes, making curr_rtt = 0 and curr_min_rtt = ~0U. If an early ACK lacks a valid RTT sample (for example, a pure duplicate ACK where sample->rtt_us < 0), curr_rtt remains
0. The comparison 0 < ~0U will succeed in update_min_rtt(), locking curr_min_rtt to 1. Future valid RTTs will never be smaller than 1.

[ ... ]

net/ipv4/tcp_roccet.c line 250

[Severity: Medium]
Could this direct assignment truncate the 32-bit acked parameter?

The struct definition restricts ack_rate_cnt to only 16 bits. Without a bounding check like min_t(u32, acked, U16_MAX) here, a malicious receiver utilizing large windows or LRO/GRO aggregation could acknowledge more than 65535 packets at once. This would cause the counter to silently wrap around and spoof a low ACK rate.

[ ... ]

net/ipv4/tcp_roccet.c line 374

[Severity: High]
What happens if this min_rtt probe timer expires while the connection is mid-recovery?

Abruptly inflating the cwnd back to the pre-probe value during fast recovery would bypass the PRR pacing mechanism and inflate the window.

[ ... ]

net/ipv4/tcp_roccet.c line 463

[Severity: Critical]
Does calling param_precompute() per socket initialization create a data race on the global cube_factor variable?

Since roccettcp_init() is invoked dynamically for every newly created socket, the unprotected in-place division do_div(cube_factor, ...) mutates shared global state. If concurrent sockets are created, threads will subject the variable to multiple divisions, resulting in a miscalculation of the cwnd target in bictcp_update().

[ ... ]

net/ipv4/tcp_roccet.c line 798

[Severity: High]
Does returning the unmodified snd_ssthresh bypass TCP's required threshold reductions?

When a severe congestion event like an RTO timeout occurs, or during an ECN CE-mark response in TCP_CA_CWR, the core stack queries the .ssthresh callback to shrink the slow-start threshold. Returning the current threshold unmodified prevents PRR from pacing properly and allows the window to exponentially grow back to massive sizes.

[ ... ]

net/ipv4/tcp_roccet.c line 836

[Severity: High]
Is this fast convergence calculation using the correct window size?

The active cwnd is artificially halved during the min_rtt probe. By calculating the new last_max_cwnd using this slashed probe window before restoring the true cwnd, the historical maximum window (which anchors the CUBIC curve origin) would become permanently understated.

[ ... ]

net/ipv4/tcp_roccet.c line 848

[Severity: High]
Is this ECN handling code unreachable?

This intended ECN handling code sits inside roccettcp_recalc_ssthresh(), which appears to only be invoked by roccettcp_state() during TCP_CA_Recovery. Since ECN marks normally trigger TCP_CA_CWR (which roccettcp_state() ignores), this logic will never execute during actual ECN events, violating protocol requirements.

[ ... ]

net/ipv4/tcp_roccet.c line 891

[Severity: High]
Does instantly setting the cwnd here bypass TCP's Proportional Rate Reduction (PRR) fast recovery?

Blindly overriding the cwnd assignment upon entering TCP_CA_Recovery instantly shrinks the window and destroys the core stack's PRR pacing mechanism, which leads to retransmission bursts.

Additionally, the TCP_CA_Loss branch misses the manual ssthresh reduction applied to Recovery, cementing the RTO backoff bypass.

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

net/ipv4/tcp_roccet.c line 159

[Severity: Medium]
The comment above says precompute parameters are read-only, but beta_param is registered with mode 0644 while bic_scale_param uses 0444. Since there is no module_param_cb setter, a write to beta_param goes straight into the global with no validation and no recomputation of beta, beta_scale or cube_factor. Those are refreshed only later, when some unrelated socket runs roccettcp_init().

If the written value is out of range, param_check(true) puts the default into the hidden beta but leaves beta_param reporting the rejected value, so /sys/module/tcp_roccet/parameters/beta_param permanently disagrees with the beta actually in use. Should beta_param be 0444 like bic_scale_param, or use a setter that validates and recomputes at write time?

[ ... ]

net/ipv4/tcp_roccet.c line 170

[Severity: Medium]
The memset also zeroes ca->interval_snd_seq_start and ca->interval_una_seq_start. roccettcp_init() seeds them explicitly:

	ca->interval_snd_seq_start = tcp_sk(sk)->snd_nxt;
	ca->interval_una_seq_start = tcp_sk(sk)->snd_una;

but the TCP_CA_Loss branch of roccettcp_state() calls roccettcp_reset() without re-seeding them. Does ORBITER then compare absolute sequence numbers?

	send = tp->snd_nxt - ca->interval_snd_seq_start;
	received = tp->snd_una - ca->interval_una_seq_start;

With both baselines at 0 this reduces to "bytes in flight > 1% of cwnd * mss", which looks true for as long as the flow is sending, so the later

	if (!tcp_is_cwnd_limited(sk) || send_more_than_acked)
		return;

blocks all window growth until the first srRTT evaluation re-seeds the baselines, and that same evaluation can authorize a congestion event.

net/ipv4/tcp_roccet.c line 197

[Severity: High]
Can this latch a 1us minimum RTT when no RTT sample has been taken yet?

After roccettcp_reset() or roccettcp_init(), ca->curr_rtt is 0 and ca->curr_min_rtt is ~0U. roccettcp_acked() returns early for invalid samples:

	/* Some calls are for duplicates without timestamps */
	if (sample->rtt_us < 0)
		return;

and tcp_clean_rtx_queue() calls pkts_acked unconditionally with sample.rtt_us taken from rate->rtt_us, which is -1 when no RTT could be measured. roccet_control() then still runs update_min_rtt() and update_srrtt() with curr_rtt == 0, so 0 < ~0U is true and curr_min_rtt becomes max(0, 1) == 1.

Reachable cases would be a SACK-only or duplicate first ACK when the first packet of the initial window is lost, and the first ACK after an RTO on a connection without TCP timestamps.

Once curr_min_rtt is 1, update_srrtt() computes
rrtt = 100 * (curr_rtt - 1) / 1, i.e. millions, so curr_srrtt stays far above sr_rtt_upper_bound (default 100) and the bufferbloat detector fires continuously until the next minimum RTT probe five seconds later. Would it help to distinguish "no sample yet" from a real 1us RTT here?

net/ipv4/tcp_roccet.c line 257

[Severity: Low]
The new-interval branch assigns the u32 argument acked straight into the u16 ca->ack_rate_cnt with no clamp, while the sibling branch a few lines below saturates with min_t(u32, ..., U16_MAX). Should the first assignment saturate too? An ACK covering more than 65535 segments at an interval boundary truncates (70000 becomes 4464) and feeds a wrong value into get_ack_rate_diff().

[ ... ]

net/ipv4/tcp_roccet.c line 286

[Severity: Low]
There is no curr_min_rtt_timed member in struct roccettcp; the field is the flat u32 curr_min_rtt. Same kind of thing in update_min_rtt(), where the comment says the next probe happens "in ROCCET_NEXT_MIN_RTT_PROBE seconds" while the macro is documented as milliseconds and is multiplied by USEC_PER_MSEC, i.e. five seconds and not 5000 seconds.

net/ipv4/tcp_roccet.c line 297

[Severity: Medium]
The div_u64() numerator is widened to 64 bits, but the result is stored in a u32 and the EWMA below is evaluated entirely in 32-bit arithmetic. Does 80 * ca->curr_srrtt wrap once curr_srrtt exceeds roughly 53.6M?

With curr_min_rtt floored to 1us (see the update_min_rtt() question above), a 540ms RTT sample already gives rrtt above 53.6M, and curr_srrtt converges toward it. curr_srrtt is the only bufferbloat signal gating roccet_congestion_event() and the LAUNCH exit, so a wrap there either defeats the reaction or fires it spuriously.

The v2/v3 changelog says "Fix potential integer overflow in rRTT calculation" and "Fix potential overflow in rRTT calculation by using 64-bit arithmetic", but only the numerator was widened.

[ ... ]

net/ipv4/tcp_roccet.c line 380

[Severity: Medium]
The cwnd restore depends on ca->state still being RTT_PROBE or RTT_PROBE_REFILL and on an ACK landing inside the refill window. Can other code paths clear the state and leave the reduced window in place?

roccettcp_recalc_ssthresh() starts with:

	if (ca->state == RTT_PROBE_REFILL)
		ca->state = ORBITER;

and roccet_control() overwrites ca->state in the two higher priority branches:

	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;

In all of these, probe_min_rtt_until and refill_until stay non-zero, so the restore branch is never taken. The sender then keeps the 2x-3x reduced cwnd with snd_ssthresh pinned to it, and when the deadline finally passes the terminal else only clears probe_min_rtt_until, making that probe round a no-op. The same skip happens if no ACK arrives inside the refill window.

net/ipv4/tcp_roccet.c line 406

[Severity: Medium]
param_check(true) is called from roccettcp_init(), i.e. once per socket. Can this flood the log? The out-of-range path never rewrites beta_param itself, only the hidden beta, so once an invalid value is stored every subsequent connection that selects roccet prints two to four lines, indefinitely. Would pr_*_once() or ratelimited variants, or moving the diagnostics to module init and a param setter, avoid that?

[ ... ]

net/ipv4/tcp_roccet.c line 463

[Severity: High]
Is it safe to write the module-wide globals from a per-socket callback? roccettcp_init() runs for every new socket that selects roccet, and param_check()/param_precompute() write beta, bic_scale, beta_scale, cube_rtt_scale and cube_factor with no lock, RCU or atomic annotation, while every other live flow reads them locklessly from the ACK path (bictcp_update(), roccet_congestion_event(), roccettcp_recalc_ssthresh()).

cube_factor is also published in an intermediate state:

	cube_factor = 1ull << (10 + 3 * BICTCP_HZ); /* 2^40 */
	do_div(cube_factor, bic_scale * 10);

so a concurrent bictcp_update() can multiply the undivided 2^40, and two initializers racing here can apply do_div() twice to the same variable, shrinking cube_factor by another factor of ~410 for the rest of the module's life.

param_check() also reads beta_param twice, once for the range test and once for "beta = beta_param;". If the second read picks up a concurrently written 1024, param_precompute() divides by (BICTCP_BETA_SCALE - beta) == 0.

Upstream CUBIC does this precomputation once in __init cubictcp_register() and keeps bic_scale read-only. Could ROCCET do the same?

net/ipv4/tcp_roccet.c line 477

[Severity: Low]
This isn't a bug, but the commented-out WRITE_ONCE() looks like leftover debug code and should probably be dropped. There are also several C99 // comments in the file (next to BETA_PARAM_DEFAULT, in update_ack_rate() and in update_srrtt()) which checkpatch flags as errors.

And in roccet_control() the pacing comment contradicts both the code and the comment right below it:

	 * In Congestion Avoidance phase, set it to 120 % the current rate.
	...
		/* Pacing rate of 100%
		 * (instead of ipv4.sysctl_tcp_pacing_ca_ratio)
		 */
		rate *= 100;

[ ... ]

net/ipv4/tcp_roccet.c line 641

[Severity: High]
Can this loop spin forever when delta is 0? ca->ack_cnt is non-zero here because "ca->ack_cnt += acked;" runs at the top of bictcp_update(), and subtracting 0 never changes it, so there is no exit condition.

param_check() only rejects beta_param outside (0, 1024), and the surviving values truncate beta_scale down far enough that (cwnd * scale) >> 3 is 0 for small cwnd:

	beta_param = 300 -> beta_scale 4 -> delta 0 at cwnd 1
	beta_param = 200 -> beta_scale 3 -> delta 0 at cwnd <= 2
	beta_param = 1   -> beta_scale 2 -> delta 0 at cwnd <= 3

cwnd of 2 is reachable because roccet_congestion_event() clamps to a floor of 2. This runs in softirq context on the ACK path, so the CPU would stop processing softirqs.

Unlike upstream CUBIC, which computes beta_scale once in __init cubictcp_register(), this module recomputes it from beta_param on every new socket, so a runtime write to the 0644 parameter reaches live traffic. Should the accepted range be tightened, or delta == 0 guarded?

[ ... ]

net/ipv4/tcp_roccet.c line 701

[Severity: Medium]
Is the assumption in that comment always true? cwnd == 1 together with snd_ssthresh == TCP_INFINITE_SSTHRESH looks reachable:

tcp_enter_loss()

    WRITE_ONCE(tp->snd_ssthresh, icsk->icsk_ca_ops->ssthresh(sk));
        -> roccettcp_ssthresh() returns the unchanged TCP_INFINITE_SSTHRESH
    tcp_snd_cwnd_set(tp, tcp_packets_in_flight(tp) + 1);  /* == 1 */
tcp_set_ca_state(sk, TCP_CA_Loss)
    roccettcp_state() -> roccettcp_reset() -> state = LAUNCH

Taking this branch with cwnd == 1 stores snd_ssthresh = 0 while cwnd is raised to TCP_INIT_CWND. After that tcp_in_slow_start() can never be true again (cwnd is never < 0), so roccet_control() never selects LAUNCH, tcp_slow_start() is unreachable for the rest of the connection, and the pacing branch "tcp_snd_cwnd(tp) < tp->snd_ssthresh / 2" is dead. cwnd of 2 or 3 gives snd_ssthresh 1. Should there be a floor here?

net/ipv4/tcp_roccet.c line 717

[Severity: Low]
The non-zero leftover returned by tcp_slow_start() means cwnd just reached snd_ssthresh and the remaining credit should be applied in congestion avoidance, which is what stock CUBIC does with bictcp_update() plus tcp_cong_avoid_ai(). Here the LAUNCH branch just falls out of the if/else chain, and ca->state only becomes ORBITER on the next roccet_control() call, so the credit is dropped. Was that intended?

[ ... ]

net/ipv4/tcp_roccet.c line 740

[Severity: Low]
These are 32-bit sequence deltas, and the interval is five RTTs (ca->next_srrtt_check = now + 5 * ca->curr_rtt). If a flow advances more than 4GiB within that interval the modulo totals invert the send_more_than_acked comparison. That needs roughly 34Gbit/s at a 200ms RTT, so it is an extreme case, but should the totals be accumulated in u64?

[ ... ]

net/ipv4/tcp_roccet.c line 768

[Severity: Low]
sr_rtt_upper_bound is a 0644 module parameter read here twice as a plain load, and the second read is the wrap guard for the first. Since sysfs writes only take the module param mutex, which this softirq-context reader does not hold, the compiler is free to reload it and defeat the guard. Would a single READ_ONCE() snapshot into a local be better? The LAUNCH exit test reads sr_rtt_upper_bound and ack_rate_diff_ss the same way.

[ ... ]

net/ipv4/tcp_roccet.c line 798

[Severity: High]
With .ssthresh pointing at this getter, is there any multiplicative decrease left on loss or RTO?

tcp_enter_loss() does:

	WRITE_ONCE(tp->snd_ssthresh, icsk->icsk_ca_ops->ssthresh(sk));

which stores the same value back, so snd_ssthresh is never reduced on RTO. roccettcp_state() then calls roccettcp_reset() for TCP_CA_Loss, which memsets the state, wiping ca->last_max_cwnd (CUBIC's W_max) and returning the flow to LAUNCH. roccettcp_recalc_ssthresh() returns cwnd unchanged while tcp_in_slow_start(), and because .cong_control is provided tcp_cong_control() returns before tcp_cwnd_reduction(), so PRR does not run either.

After an RTO the flow slow-starts straight back to the same unreduced ssthresh. On a lossy or shallow-buffered path, where RTT never inflates and the srRTT heuristic never fires, that leaves no back-off on loss at all. Only the file header mentions that LAUNCH ignores loss; could the commit message describe the RTO and ORBITER behaviour as well?

net/ipv4/tcp_roccet.c line 836

[Severity: Medium]
At this point cwnd still holds tcp_snd_cwnd(tp), which during a probe is the deliberately deflated probe window; the assignment from ca->cwnd_before_min_rtt_probe happens only afterwards and is used just to recompute cwnd_before_min_rtt_probe. Doesn't that contradict the comment above ("we use the cwnd before the probing interval to calculate the cwnd reduction ... it is very likely that congestion was caused by the cwnd value before min RTT probing")?

With last_max_cwnd taken from the small probe window, once probing ends and cwnd is restored to the larger pre-probe value, bictcp_update() sees ca->last_max_cwnd <= cwnd and takes the

			ca->bic_K = 0;
			ca->bic_origin_point = cwnd;

path, skipping the concave region and growing convexly right after a congestion event.

[ ... ]

net/ipv4/tcp_roccet.c line 930

[Severity: High]
Is roccettcp_recalc_ssthresh() actually reachable from the ECE path? It is only called from the TCP_CA_Recovery branch of roccettcp_state(), while a pure ECN mark goes through:

tcp_try_to_open()

    tcp_enter_cwr()
        tcp_init_cwnd_reduction()
            WRITE_ONCE(tp->snd_ssthresh, ca_ops->ssthresh(sk));
        tcp_set_ca_state(sk, TCP_CA_CWR);

ca_ops->ssthresh is roccettcp_ssthresh(), which returns snd_ssthresh unchanged, and roccettcp_state() has no TCP_CA_CWR branch. Since .cong_control is set, tcp_cong_control() also returns before tcp_cwnd_reduction(), so PRR does not reduce cwnd either.

On an ECN-marked path without loss, does anything reduce ssthresh or cwnd? The latched ece_received then appears to be applied much later as a stale reduction at an unrelated Recovery event. The v3 changelog says "Always react to ECE bits and reset flag".

[ ... ]

net/ipv4/tcp_roccet.c line 1011

[Severity: Medium]
This is set unconditionally at the end of the first roccet_control() call that has rs->acked_sacked != 0, but the field is documented as "Set to true after the initial roccet-control round has completed" and roccettcp_cong_avoid() relies on that meaning:

* This condition is checked only after the initial round of
* LAUNCH has completed. This is done in order to avoid false
* positives, which can occur when the tracked outstanding
* packets have not yet caught up to the initial cwnd.

Since there is no RTT or sequence-space round boundary test, the "!tcp_is_cwnd_limited(sk) && ca->initial_round_completed" term can fire from the second ACK onwards. With cwnd 10 and only three packets in flight, tcp_is_cwnd_limited() is false in slow start (10 < 2 * 3), so the exit runs and leaves the flow permanently out of slow start at cwnd 10, growing only via the cubic path afterwards. Should the flag be set on a round boundary instead?

Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/apWz9rPcGMgYCKOR%40volt-roccet-vm