Re: [PATCH net] sctp: preserve stream scheduler across outq teardown

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The patch changes how outqueue teardown behaves in the common
  case, and the commit message does not say so.

--

sctp: preserve stream scheduler across outq teardown

This patch keeps q->sched in sctp_outq_init() across the memset, so that
sctp_sched_set_sched() can free the old scheduler's per-stream state when
sctp_outq_teardown() reinitializes a live outqueue. Without that,
installing a different default scheduler could read stale data through
the shared union in struct sctp_stream_out_ext.

> diff --git a/net/sctp/outqueue.c b/net/sctp/outqueue.c
> index e92fb9da4647a..6cb4b4339d8b7 100644
> --- a/net/sctp/outqueue.c
> +++ b/net/sctp/outqueue.c
> @@ -190,9 +190,13 @@ static inline int sctp_cacc_skip(struct sctp_transport *primary,
>   */
>  void sctp_outq_init(struct sctp_association *asoc, struct sctp_outq *q)
>  {
> +	const struct sctp_sched_ops *sched = q->sched;
> +
>  	memset(q, 0, sizeof(struct sctp_outq));
>
>  	q->asoc = asoc;
> +	/* Retain the scheduler to allow freeing its per-stream state. */
> +	q->sched = sched;

[Severity: Medium]
The commit message only talks about the case where the installed scheduler
differs from sctp_sk(sk)->default_ss. Does this also change behaviour in
the more common case, where the two are the same?

Before this patch, q->sched was NULL after the memset. That meant
sctp_sched_set_sched() never took its early return, and every outqueue
purge ran n->init() and n->init_sid() on each stream:

net/sctp/stream_sched.c:sctp_sched_set_sched() {
    ...
	n = sctp_sched_ops[sched];
	if (old == n)
		return ret;
    ...
	n->init(&asoc->stream);
	for (i = 0; i < asoc->stream.outcnt; i++) {
		...
		ret = n->init_sid(&asoc->stream, i, GFP_ATOMIC);
    ...
}

With q->sched kept, old == n whenever default_ss matches the installed
scheduler, so the per-stream reset is skipped. The init_sid callbacks that
no longer run on teardown are:

net/sctp/stream_sched_prio.c:sctp_sched_prio_init_sid() {
	...
	return sctp_sched_prio_set(stream, sid, 0, gfp);
}

net/sctp/stream_sched_fc.c:sctp_sched_fc_init_sid() {
	...
	soute->fc_length = 0;
	soute->fc_weight = 1;
	...
}

As a result, PRIO priorities and WFQ weights set through
SCTP_STREAM_SCHEDULER_VALUE now survive a peer restart and a stale-cookie
retry. Previously both paths reset them to 0 and 1:

sctp_sf_do_dupcook_a() / sctp_sf_do_5_2_6_stale()
  -> SCTP_CMD_PURGE_OUTQUEUE
    -> sctp_outq_teardown()
      -> sctp_outq_init()
        -> sctp_sched_set_sched()   /* returns early, old == n */

For FC and WFQ there is a second effect. __sctp_outq_teardown() calls
sctp_sched_dequeue_done() on every leftover data chunk it discards:

net/sctp/outqueue.c:__sctp_outq_teardown() {
    ...
	while ((chunk = sctp_outq_dequeue_data(q)) != NULL) {
		sctp_sched_dequeue_done(q, chunk);

		/* Mark as send failure. */
		sctp_chunk_fail(chunk, q->error);
    ...
}

sctp_sched_fc_dequeue_done() then adds ch->skb->len to soute->fc_length.
That used to be cleared by init_sid. Now it isn't, so a stream with a
large unsent backlog at restart begins the restarted association with
fc_length inflated by bytes that were never sent.

Keeping the user-configured values may well be the preferred behaviour.
Since this is going to stable, could the commit message describe it? If
it isn't intended, should teardown reset the per-stream scheduler state
explicitly, or at least the fc_length accumulated from discarded chunks?

>  	INIT_LIST_HEAD(&q->out_chunk_list);
>  	INIT_LIST_HEAD(&q->control_chunk_list);
>  	INIT_LIST_HEAD(&q->retransmit);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261104105507.62743-1-juny24602%40gmail.com