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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 21 potential
issue(s) to consider:

Critical: 0 · High: 5 · Medium: 8 · Low: 8

- [High] ROCCET performs no congestion response to ECN.
- [High] ROCCET removes the standard congestion response to loss/RTO, and
  the commit message does not disclose it.
- [High] `update_min_rtt()` (net/ipv4/tcp_roccet.c:191-206) treats an
  absent RTT sample (`ca->curr_rtt == 0`) as a valid measurement and…
- [High] `param_check()` only rejects `beta_param` outside (0, 1024).
- [High] `roccettcp_init()` (net/ipv4/tcp_roccet.c:457-463) — called for
  every new TCP socket that selects ROCCET, i.e. by any unprivileged…
- [Medium] `param_check(true)` is invoked from `roccettcp_init()`
  (net/ipv4/tcp_roccet.c:462) for every new socket, and it emits…
- [Medium] The writable `beta_param` interface
  (net/ipv4/tcp_roccet.c:144, `module_param(beta_param, int, 0644)` at…
- [Medium] `roccettcp_reset()` (net/ipv4/tcp_roccet.c:168) memsets the
  whole per-socket CA state, zeroing…
- [Medium] `initial_round_completed` is documented as a full-round guard
  ('Set to true after the initial roccet-control round has completed',…
- [Medium] The LAUNCH exit path in `roccettcp_cong_avoid()` computes
  `tcp_sk(sk)->snd_ssthresh = tcp_snd_cwnd(tp) / 2`…
- [Medium] `roccet_min_rtt_probe()` reduces cwnd and relies on a later
  call, while `ca->state` is still RTT_PROBE/RTT_PROBE_REFILL and while…
- [Medium] In `roccettcp_recalc_ssthresh()`, the RTT_PROBE branch updates
  `ca->last_max_cwnd` from the local `cwnd`, which at that point still…
- [Medium] `update_srrtt()` truncates the 64-bit rRTT result into a u32
  and then evaluates the EWMA entirely in 32-bit arithmetic: `u32 rrtt =…
- [Low] Unfinished-submission artifacts in the new file: leftover
  commented-out code `//WRITE_ONCE(sk->sk_pacing_rate, 0);` in…
- [Low] The main commit description omits ROCCET's periodic forced
  congestion-window reduction for minimum-RTT probing.
- [Low] Two comments describe artifacts that do not exist or state the
  wrong unit: `update_srrtt()` explains its invariant in terms of…
- [Low] `param_check()`'s comment (net/ipv4/tcp_roccet.c:382-385) claims
  it exists 'to check the roccet params in advance in order to avoid…
- [Low] In `update_ack_rate()`, the 'new interval' branch assigns the u32
  parameter `acked` directly into the u16 field `ca->ack_rate_cnt`…
- [Low] In `roccettcp_cong_avoid()`, the LAUNCH branch does `acked =
  tcp_slow_start(tp, acked); if (!acked) return;`…
- [Low] The ORBITER send/received monitoring interval cannot account for
  more than 4 GiB of data: `send = tp->snd_nxt -…
- [Low] `sr_rtt_upper_bound` and `ack_rate_diff_ss` are
  `module_param(..., 0644)` and are read from the packet-processing hot…

--

tcp: Add TCP ROCCET congestion control module.

This adds a new loadable congestion control module, ROCCET, which builds on
the CUBIC window growth function and adds an srRTT metric, an ACK arrival
rate metric and a send/receive byte comparison to detect bufferbloat.  It
replaces HyStart with its own slow start (LAUNCH) and drives everything from
a .cong_control callback.

A few questions about the new file follow.

The commit body describes the congestion avoidance phase as purely passive:

  "For the congestion avoidance phase, ROCCET relies on the srRTT and
   monitoring the send and received Bytes to detect the filling of the
   bottleneck buffer."

[Severity: Low]
Could the commit message also describe the active minimum RTT probing?
roccet_min_rtt_probe() cuts cwnd to max(cwnd/2, TCP_INIT_CWND), or
max(cwnd/3, TCP_INIT_CWND) when tcp_is_cwnd_limited() is false, pins
snd_ssthresh to that value for at least 200ms or one RTT, resets
curr_min_rtt and later restores the previous window.  It is armed every time
the five second deadline in ca->next_min_rtt_probe expires.  Only the
revision history mentions it ("Add minimum RTT probing"), while the body
reads as if no forced window reduction happens.

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

[ ... ]

> +/* min RTT probe period in ms */
> +#define ROCCET_NEXT_MIN_RTT_PROBE 5000

[ ... ]

> +/* Parameters that are specific to the ROCCET-Algorithm */
> +
> +// = 717/1024 (BICTCP_BETA_SCALE)
> +#define BETA_PARAM_DEFAULT 717
> +#define BIC_SCALE_PARAM_DEFAULT 41
> +
> +static uint sr_rtt_upper_bound __read_mostly = 100;
> +static int ack_rate_diff_ss __read_mostly = 10;
> +
> +module_param(sr_rtt_upper_bound, uint, 0644);
> +MODULE_PARM_DESC(sr_rtt_upper_bound, "ROCCET's upper bound for srRTT.");
> +module_param(ack_rate_diff_ss, int, 0644);
> +MODULE_PARM_DESC(ack_rate_diff_ss,
> +		 "ROCCET's threshold to exit slow start if ACK-rate defer by given amount of segments.");

[ ... ]

> +/* 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_param, int, 0644);
> +MODULE_PARM_DESC(beta_param, "beta for multiplicative increase");

[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?

[ ... ]

> +static void roccettcp_reset(struct roccettcp *ca)
> +{
> +	memset(ca, 0, sizeof(struct roccettcp));

[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.

> +static void update_min_rtt(struct sock *sk)
> +{
> +	struct roccettcp *ca = inet_csk_ca(sk);
> +
> +	/* Check if new lower min RTT was found. If so, set it directly */
> +	if (ca->curr_rtt < ca->curr_min_rtt) {
> +		ca->curr_min_rtt = max(ca->curr_rtt, 1);

[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?

> +		/* Probe for the min RTT in ROCCET_NEXT_MIN_RTT_PROBE seconds
> +		 * if no other update occurs.
> +		 */

[ ... ]

> +			ca->ack_rate_curr_rate = ca->ack_rate_cnt;
> +			ca->ack_rate_cnt =
> +				acked; // start counting for the new interval
> +		}
> +
> +		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: 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().

[ ... ]

> +	/* Calculate the new rRTT (Scaled by 100).
> +	 * 100 * ((sRTT - sRTT_min) / sRTT_min).
> +	 *
> +	 * curr_min_rtt_timed.rtt is always <= than curr_rtt,
> +	 * since this is the minimum of the rtt.

[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.

> +	 *
> +	 * 0 is a valid value for rrtt.
> +	 */
> +	u32 rrtt = div_u64(100 * (u64)(ca->curr_rtt - ca->curr_min_rtt),
> +			   ca->curr_min_rtt);
> +
> +	// (1 - alpha) * srRTT + alpha * rRTT
> +	ca->curr_srrtt = ((100 - ROCCET_ALPHA_TIMES_100) * ca->curr_srrtt +
> +			  ROCCET_ALPHA_TIMES_100 * rrtt) /
> +			 100;
> +}

[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.

[ ... ]

> +		probe_cwnd = max(tcp_snd_cwnd(tp) / 2, TCP_INIT_CWND);
> +		if (!tcp_is_cwnd_limited(sk))
> +			probe_cwnd = max(tcp_snd_cwnd(tp) / 3, TCP_INIT_CWND);

[ ... ]

> +		ca->refill_until = ca->probe_min_rtt_until + interval;
> +	} else if (before(now, ca->refill_until)) {
> +		/* Reset cwnd and refill the pipe. */
> +		if (ca->state != RTT_PROBE_REFILL) {
> +			tcp_snd_cwnd_set(tp, ca->cwnd_before_min_rtt_probe);
> +			tcp_sk(sk)->snd_ssthresh = tcp_snd_cwnd(tp);
> +			ca->state = RTT_PROBE_REFILL;
> +		}
> +	} else {
> +		/* End min RTT probing phase. */
> +		ca->probe_min_rtt_until = 0;
> +		ca->state = ORBITER;
> +	}
> +}

[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.

> +/* Used to check the roccet params in advance in order to avoid
> + * invalid-param-attacks. This validates the provided params and
> + * then saves valid copies of the params.
> + */

[Severity: Low]
This describes all of the parameters, but only beta_param and
bic_scale_param are validated and copied.  sr_rtt_upper_bound,
ack_rate_diff_ss, initial_ssthresh, fast_convergence and tcp_friendliness
are all 0644 and are read live with no bounds check and no validated copy,
including initial_ssthresh which is assigned directly to tp->snd_ssthresh in
roccettcp_init().  Could the comment be narrowed to what is actually
checked?

> +static int param_check(bool use_defaults)
> +{
> +	int ret = 0;
> +
> +	/*
> +	 * Validate parameters to avoid division by zero errors.
> +	 */
> +	if (beta_param <= 0 || beta_param >= BICTCP_BETA_SCALE) {
> +		pr_err("roccet: beta must be between 0 and %d\n",
> +		       BICTCP_BETA_SCALE);
> +
> +		if (use_defaults) {
> +			pr_info("Using default value of %d for beta.\n",
> +				BETA_PARAM_DEFAULT);
> +			beta = BETA_PARAM_DEFAULT;
> +		} else {
> +			ret = -EINVAL;
> +		}
> +	} else {
> +		beta = beta_param;
> +	}

[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?

[ ... ]

> +static void param_precompute(void)
> +{
> +	/* Precompute a bunch of the scaling factors that are used per-packet
> +	 * based on SRTT of 100ms
> +	 */
> +	beta_scale =
> +		8 * (BICTCP_BETA_SCALE + beta) / 3 / (BICTCP_BETA_SCALE - beta);
> +
> +	cube_rtt_scale = (bic_scale * 10); /* 1024*c/rtt */

[ ... ]

> +	/* 1/c * 2^2*bictcp_HZ * srtt */
> +	cube_factor = 1ull << (10 + 3 * BICTCP_HZ); /* 2^40 */
> +
> +	/* divide by bic_scale and by constant Srtt (100ms) */
> +	do_div(cube_factor, bic_scale * 10);
> +}
> +
> +static void roccettcp_init(struct sock *sk)
> +{
> +	/* Check & precompute on `init` in order to use the newest
> +	 * available params.
> +	 */
> +	param_check(true);
> +	param_precompute();

[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?

> +
> +	struct roccettcp *ca = inet_csk_ca(sk);
> +
> +	roccettcp_reset(ca);

[ ... ]

> +	cmpxchg(&sk->sk_pacing_status, SK_PACING_NONE, SK_PACING_NEEDED);
> +	//WRITE_ONCE(sk->sk_pacing_rate, 0);
> +}

[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;

[ ... ]

> +tcp_friendliness:
> +	/* TCP Friendly */
> +	if (tcp_friendliness) {
> +		u32 scale = beta_scale;
> +
> +		delta = (cwnd * scale) >> 3;
> +		while (ca->ack_cnt > delta) { /* update tcp cwnd */
> +			ca->ack_cnt -= delta;
> +			ca->tcp_cwnd++;
> +		}

[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?

[ ... ]

> +		if ((ca->curr_srrtt > sr_rtt_upper_bound &&
> +		     get_ack_rate_diff(ca) <= ack_rate_diff_ss) ||
> +		     (!tcp_is_cwnd_limited(sk) &&
> +		      ca->initial_round_completed)) {
> +			ca->epoch_start = 0;
> +
> +			/* Handle initial slow start.
> +			 * Most bufferbloat occurs here
> +			 */
> +			if (tp->snd_ssthresh == TCP_INFINITE_SSTHRESH) {
> +				tcp_sk(sk)->snd_ssthresh = tcp_snd_cwnd(tp)
> +								/ 2;
> +				/* since this is the initial slow start,
> +				 * the min cwnd won't be 1, so the window
> +				 * can't be set to 0 by accident.
> +				 * Halfing the cwnd will undo the previous step
> +				 * of slow start. Which is fine since the pipe
> +				 * is already full.
> +				 */
> +				tcp_snd_cwnd_set(tp, max(tcp_snd_cwnd(tp) / 2,
> +							 TCP_INIT_CWND));

[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?

> +			} else {

[ ... ]

> +		acked = tcp_slow_start(tp, acked);
> +		if (!acked)
> +			return;
> +
> +	} else if (ca->state == ORBITER) {

[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?

[ ... ]

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

[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?

[ ... ]

> +		roccet_xj = div_u64((u64)jitter * 100, ca->curr_min_rtt) +
> +			    sr_rtt_upper_bound;
> +		if (roccet_xj < sr_rtt_upper_bound)
> +			roccet_xj = sr_rtt_upper_bound;

[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.

[ ... ]

> +static u32 roccettcp_ssthresh(struct sock *sk)
> +{
> +	return tcp_sk(sk)->snd_ssthresh;
> +}

[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?

> +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 (ca->state == RTT_PROBE) {

[ ... ]

> +		/* 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;

[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.

[ ... ]

> +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]
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".

[ ... ]

> +	/* current rate is (cwnd * mss) / srtt
> +	 * In Slow Start [1], set sk_pacing_rate to 200 % the current rate.
> +	 * In Congestion Avoidance phase, set it to 120 % the current rate.

[ ... ]

> +	ca->initial_round_completed = true;
> +}

[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