Re: [PATCH net-next v18 10/15] quic: add packet number space

[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_destroy_sock() (net/quic/socket.c) kfree()s the
  separately allocated pnspace objects at the start of teardown and…
- [Low] The commit message names a symbol that does not exist.
- [Low] After quic_pnspace_init() (net/quic/pnspace.c), base_pn is -1 but
  max_pn_seen, last_max_pn_seen and min_pn_seen stay at their kzalloc…

--

quic: add packet number space

This patch adds struct quic_pnspace, which holds per packet number space
state: a bitmap of received packet numbers for ACK generation, timestamps
for loss detection and RTT, and ECN counters. It adds helpers to check and
mark received packet numbers, build gap ack blocks and validate peer ECN
counts. quic_init_sock() allocates the pnspace objects per socket and
quic_destroy_sock() frees them.

> To support ACK frame generation, quic_pnspace includes a packet number
> acknowledgment map (pn_ack_map) that tracks received packet numbers.

[Severity: Low]
This isn't a bug, but should this say pn_map rather than pn_ack_map?
Nothing in net/quic/ is called pn_ack_map. The field in struct quic_pnspace
is:

	unsigned long *pn_map; /* Received PN bitmap for ACK generation */

> - quic_pnspace_num_gabs(): Returns the gap ACK blocks for constructing
>   ACK frames.

quic_pnspace_num_gabs() fills the caller's gabs array and returns a u16
count. When there are more than QUIC_PN_MAP_MAX_GABS gaps, it also merges
the oldest gaps into one block.

Only the code comment and the v15/v16 changelog below the --- line
describe this, and that changelog won't be kept in git history. Could the
commit message describe it?

> diff --git a/net/quic/pnspace.c b/net/quic/pnspace.c
> new file mode 100644
> index 0000000000000..fcf68f0df1848
> --- /dev/null
> +++ b/net/quic/pnspace.c
> @@ -0,0 +1,273 @@

[ ... ]

> +int quic_pnspace_init(struct quic_pnspace *space)
> +{
> +	space->pn_map = bitmap_zalloc(QUIC_PN_MAP_INITIAL, GFP_KERNEL_ACCOUNT);
> +	if (!space->pn_map)
> +		return -ENOMEM;
> +	space->pn_map_len = QUIC_PN_MAP_INITIAL;
> +
> +	space->max_time_limit = QUIC_PNSPACE_TIME_LIMIT;
> +	space->next_pn = QUIC_PNSPACE_NEXT_PN;
> +	space->max_pn_acked_seen = -1;
> +	space->base_pn = -1;
> +	return 0;
> +}

[Severity: Low]
After this runs, base_pn is -1 but max_pn_seen, last_max_pn_seen and
min_pn_seen are still 0 from kzalloc. So quic_pnspace_has_gap() reports a
gap (-1 != 0 + 1) for a space that hasn't received anything:

static inline bool quic_pnspace_has_gap(const struct quic_pnspace *space)
{
	return space->base_pn != space->max_pn_seen + 1;
}

quic_pnspace_set_base_pn() otherwise keeps max_pn_seen == base_pn - 1 as
the no-gap state.

The current callers seem fine. quic_pnspace_mark() handles base_pn == -1
before it calls has_gap(). In quic_pnspace_num_gabs(), the bounds checks in
quic_pnspace_next_gap_ack() make it return 0 (start_ = 0, end_ = 64, and
0 <= -1 + 64 - 1).

Would it be better to keep max_pn_seen and friends at base_pn - 1 here as
well? Otherwise a later caller that relies only on has_gap() before the
first packet is marked would get the wrong answer.

[ ... ]

> diff --git a/net/quic/socket.c b/net/quic/socket.c
> index 41f7de7d163a0..78a08dd998a8e 100644
> --- a/net/quic/socket.c
> +++ b/net/quic/socket.c

[ ... ]

> @@ -58,11 +61,34 @@ static int quic_init_sock(struct sock *sk)

[ ... ]

>  static void quic_destroy_sock(struct sock *sk)
>  {
> +	struct quic_pnspace *space;
> +	u8 i;
> +
> +	for (i = 0; i < QUIC_PNSPACE_MAX; i++) {
> +		space = quic_pnspace(sk, i);
> +		if (!space)
> +			continue;
> +		quic_pnspace_free(space);
> +		kfree(space);
> +	}
> +
>  	quic_path_unbind(sk, quic_paths(sk), 0);
>  	quic_path_unbind(sk, quic_paths(sk), 1);

[Severity: Medium]
The pnspaces are freed here before quic_path_unbind() and
quic_conn_id_set_free() take the socket off the UDP tunnel and out of the
RCU conn-ID hash. quic_sk(sk)->space[i] is also left pointing at freed
memory. Before v18 the pnspaces were embedded in the SOCK_RCU_FREE sock, so
they shared its lifetime.

As far as I can tell, nothing in the series reaches the freed memory today.
quic_close() sets QUIC_SS_CLOSED under lock_sock() before
sk_common_release() calls ->destroy. Later in the series,
quic_packet_process() drops packets for a closed socket before it touches
any pnspace:

	if (quic_is_closed(sk)) {
		kfree_skb(skb);
		return 0;
	}

Later patches also call quic_timer_free() and quic_packet_free() before
this free.

However, quic_pnspace() returns the raw space[] pointer without any
lifetime check. A future RX path that skips the closed-state check would
then use freed memory.

Would it be safer to free the pnspaces after the path unbind and conn-ID
teardown, and to set quic_sk(sk)->space[i] to NULL after kfree()?

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