Re: [PATCH net-next v18 07/15] quic: add connection id management

[netdev-bot+sashiko@kernel.org]

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

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

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] quic_conn_id_retire() in net/quic/connid.c starts with `if
  (WARN_ON_ONCE(id_set->use_count <= 1)) return;`.
- [Low] The commit message says quic_conn_id_token_exists() checks 'if a
  stateless reset token exists in any dest Connection ID'.
- [Low] The connection ID patch also rewrites the comments on
  bidi_blocked/uni_blocked in struct quic_stream_limits…

--

quic: add connection id management

This patch adds struct quic_conn_id_set to manage source and destination
connection IDs. It also adds helpers to add, retire, find and look up CIDs.
Source CIDs are inserted into the global hash table, and destination CIDs
can be checked for stateless reset tokens.

> - quic_conn_id_token_exists(): Check if a stateless reset token exists
>   in any dest Connection ID (used during stateless reset processing).

[Severity: Low]
This isn't a bug, but is "any dest Connection ID" accurate here?

quic_conn_id_token_exists() stops at the furthest used CID, which is the
larger of active and alt by sequence number. It also skips all-zero tokens,
so unused destination CIDs are never compared.

That matches RFC 9000 Section 10.3 and the code comment ("in the used
connection IDs"). The only explanation of the narrower check is in the v17
changelog below the "---", and that part will not be kept in git history.

Could the commit message describe the used-CID-only check instead?

> diff --git a/net/quic/connid.c b/net/quic/connid.c
> new file mode 100644
> index 0000000000000..63bf39e71b15f
> --- /dev/null
> +++ b/net/quic/connid.c
> @@ -0,0 +1,311 @@

[ ... ]

> +/* Retire a connection ID by marking it dead, then remove consecutive dead
> + * entries from the head of the list. The caller must ensure the number fall
> + * within the consecutive range [first_number, last_number].
> + */
> +void quic_conn_id_retire(struct quic_conn_id_set *id_set, u32 number)
> +{
> +	struct quic_common_conn_id *common, *tmp, *retired = NULL;
> +	struct list_head *list;
> +
> +	if (WARN_ON_ONCE(id_set->use_count <= 1))
> +		return;
> +
> +	list = &id_set->head;
> +	list_for_each_entry(common, list, list) {
> +		if (common->number > number)
> +			break;
> +		if (common->number == number) {
> +			if (common->dead)
> +				return;

[Severity: Medium]
Can retiring the same number twice trigger the WARN_ON_ONCE() in
quic_conn_id_retire()?

The dead check here suggests a duplicate retirement is meant to be a
harmless no-op. The use_count check runs before the lookup gets this far,
though.

Take a source set with CIDs 0 and 1 (count == 2, use_count == 2):

quic_conn_id_retire(set, 1)
    entry 1 is marked dead, use_count becomes 1
    entry 1 is not at the head, so it stays on the list

quic_conn_id_retire(set, 1)    (e.g. a retransmitted RETIRE_CONNECTION_ID)
    WARN_ON_ONCE(id_set->use_count <= 1) fires
    the "if (common->dead) return;" path is never reached

The comment above the function only asks the caller to keep number within
[first_number, last_number], and 1 is inside that range here.

RETIRE_CONNECTION_ID frames, and NEW_CONNECTION_ID frames with Retire Prior
To, come from the peer. Once frame processing calls this helper, could this
become a WARN that the peer can trigger, or a panic with panic_on_warn=1?
Nothing in the series calls quic_conn_id_retire() yet, so this depends on
how future callers use it.

The function also returns void. How would a caller tell a refused
retirement from a successful one, for example to raise a
PROTOCOL_VIOLATION?

Would it make sense to run the lookup and dead check before the use_count
check, or to return an error instead of warning?

[ ... ]

> diff --git a/net/quic/stream.h b/net/quic/stream.h
> index 48b27aa339679..c96d1516ecbec 100644
> --- a/net/quic/stream.h
> +++ b/net/quic/stream.h
> @@ -84,8 +84,8 @@ struct quic_stream_limits {
>  	s64 max_uni_stream_id;   /* Highest allowed uni stream ID */
>  	s64 active_stream_id;    /* Most recently opened stream ID */
>  
> -	u8 bidi_blocked;  /* STREAMS_BLOCKED_BIDI sent, awaiting ACK */
> -	u8 uni_blocked;   /* STREAMS_BLOCKED_UNI sent, awaiting ACK */
> +	u8 bidi_blocked;  /* STREAMS_BLOCKED/MAX_STREAMS_BIDI sent */
> +	u8 uni_blocked;   /* STREAMS_BLOCKED/MAX_STREAMS_UNI sent */

[Severity: Low]
This isn't a bug, but this comment change in struct quic_stream_limits looks
unrelated to connection ID management. The commit message doesn't mention
it either.

Nothing in net/quic reads or writes bidi_blocked or uni_blocked yet, in
this commit or at the end of the series. So the new MAX_STREAMS wording
describes behaviour that is not in the tree yet. The only explanation is
the v18 changelog note, which is dropped when the patch is applied.

Would this change fit better in a separate patch, or in the patch that adds
the MAX_STREAMS tracking?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1794241654.git.lucien.xin%40gmail.com