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

[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: 0 · Low: 1

- [Low] quic_conn_id_lookup() (net/quic/connid.c) takes a hard socket
  reference via refcount_inc_not_zero(&s_conn_id->sk->sk_refcnt) but…

--

quic: add connection id management

This patch adds struct quic_conn_id_set plus helpers to add, remove, and
look up source and destination Connection IDs, including insertion of
source CIDs into the global connection ID hash table.  It also wires the
per-socket source/dest sets into quic_init_sock() and quic_destroy_sock().

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

[ ... ]

> +/* Lookup a source connection ID (scid) in the global source connection ID hash
> + * table.
> + */
> +struct quic_conn_id *quic_conn_id_lookup(struct net *net, u8 *scid, u32 len)
> +{
> +	struct quic_shash_head *head = quic_source_conn_id_head(net, scid, len);
> +	struct quic_source_conn_id *s_conn_id;
> +	struct quic_conn_id *conn_id = NULL;
> +	struct hlist_nulls_node *node;
> +
> +	WARN_ON_ONCE(!rcu_read_lock_held());
> +
> +	hlist_nulls_for_each_entry_rcu(s_conn_id, node, &head->head, node) {
> +		if (net != sock_net(s_conn_id->sk))
> +			continue;
> +		if (s_conn_id->common.id.len != len ||
> +		    memcmp(scid, &s_conn_id->common.id.data, len))
> +			continue;
> +		if (likely(refcount_inc_not_zero(&s_conn_id->sk->sk_refcnt)))
> +			conn_id = &s_conn_id->common.id;
> +		break;
> +	}

[Severity: Low]
This isn't a bug, but could the reference semantics here be spelled out?

quic_conn_id_lookup() takes a hard socket reference with
refcount_inc_not_zero(&s_conn_id->sk->sk_refcnt), yet it returns a
struct quic_conn_id pointer.  Neither the kerneldoc-style comment above the
function ("Lookup a source connection ID (scid) in the global source
connection ID hash table.") nor the prototype in connid.h mentions that the
caller now owns an sk reference that must be dropped with sock_put().

Recovering the socket from the return value also requires the caller to know
about the type-punning accessor:

net/quic/connid.h:
	static inline struct sock *quic_conn_id_sk(struct quic_conn_id *conn_id)
	{
		return ((struct quic_source_conn_id *)conn_id)->sk;
	}

Within this patch there is no matching sock_put() anywhere in the tree; the
balance only appears later in the series in "quic: add packet parser base",
where quic_packet_get_sock() converts the result via
return quic_conn_id_sk(conn_id); and quic_packet_rcv() drops the reference
with sock_put(sk) on every path after the lookup.  So this is not a leak as
the series stands, only an implicit handoff that is easy to miss.

Would a short comment on quic_conn_id_lookup() stating that it returns with
an sk reference held, and that the caller must sock_put(quic_conn_id_sk())
it, make the contract clearer?

> +	/* No need to check get_nulls_value(node) != hash for !conn_id, as
> +	 * hashtable size is fixed and a conn_id can not rehashed.
> +	 */
> +	return conn_id;
> +}

[ ... ]

> diff --git a/net/quic/connid.h b/net/quic/connid.h
> new file mode 100644
> index 0000000000000..abad396a6ad6a
> --- /dev/null
> +++ b/net/quic/connid.h
> @@ -0,0 +1,183 @@

[ ... ]

> +struct quic_conn_id *quic_conn_id_find(struct quic_conn_id_set *id_set,
> +				       u32 number);
> +struct quic_conn_id *quic_conn_id_lookup(struct net *net, u8 *scid, u32 len);

[Severity: Low]
These two declarations sit next to each other, return the same type, but
have opposite reference-counting rules: quic_conn_id_find() returns a
borrowed pointer while quic_conn_id_lookup() returns one with an sk
reference attached.  Would it be worth documenting that difference here, or
returning the struct sock * (or an output sk parameter) from
quic_conn_id_lookup() so the acquired reference is visible in the signature?

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