Re: [PATCH net-next v16 06/15] quic: add stream management

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 2 · 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_stream_init() (net/quic/stream.c) allocates the per-
  socket 64-entry hlist_head table (512 bytes on 64-bit) with…
- [Medium] The per-socket stream table has a fixed 64 buckets and a
  predictable, unkeyed hash, (stream_id >> 1) & 63 in quic_stream_head().
- [Low] The uAPI header (include/uapi/linux/quic.h) puts send and receive
  stream states in one anonymous enum, so the receive states start at 6…
- [Low] The comment above
  bidi_blocked/uni_blocked/bidi_pending/uni_pending in struct…

--

quic: add stream management

This patch adds struct quic_stream_table, a per-socket hash table of
struct quic_stream objects. It also adds helpers to get, create and put
send and receive streams. It includes logic to check stream ID limits and
to decide when MAX_STREAMS frames should be sent to the peer.

> diff --git a/net/quic/stream.c b/net/quic/stream.c
> new file mode 100644
> index 0000000000000..c0bfaab0be8e2
> --- /dev/null
> +++ b/net/quic/stream.c

[ ... ]

> +#define QUIC_STREAM_HT_SIZE	64
> +
> +static struct hlist_head *quic_stream_head(struct quic_stream_table *streams,
> +					   s64 stream_id)
> +{
> +	/* Skip the SERVER initiator bit, which is constant per endpoint. */
> +	return &streams->head[(stream_id >> 1) & (QUIC_STREAM_HT_SIZE - 1)];
> +}
> +
> +struct quic_stream *quic_stream_find(struct quic_stream_table *streams,
> +				     s64 stream_id)
> +{
> +	struct hlist_head *head = quic_stream_head(streams, stream_id);
> +	struct quic_stream *stream;
> +
> +	hlist_for_each_entry(stream, head, node) {
> +		if (stream->id == stream_id)
> +			break;
> +	}
> +	return stream;
> +}

[Severity: Medium]
Can a remote peer put all of its live streams on a single chain here?

The hash in quic_stream_head() is fixed and has no key. Bit 0 of
(stream_id >> 1) is the uni bit, so bidi IDs land only in even buckets and
uni IDs only in odd ones. That gives each stream type 32 buckets, and
every 32nd stream of a type maps to the same bucket.

The peer decides which of its streams stay open. It could open streams up
to the advertised limit and then finish or reset every stream except the
ones that hash to one bucket. It then gets new credit through:

quic_stream_put()
    streams->recv.bidi_pending = 1;
quic_stream_max_streams_update()
    streams->recv.max_bidi_stream_id = max;

If the peer repeats this, every live peer stream ends up on one hlist
chain. That is up to recv.max_streams_*, which is 100 by default and up to
QUIC_MAX_STREAMS.

Once the RX path lands, quic_stream_get() will call quic_stream_find() for
each received STREAM, RESET_STREAM, STOP_SENDING and MAX_STREAM_DATA
frame. Each lookup would then walk the whole chain under the socket lock
in softirq. A single datagram can carry many small STREAM frames.

Would a keyed hash (for example, seeded per socket), or a larger or
resizable table, be worth considering? This lookup does not have an RX
caller in this series yet.

[ ... ]

> +/* Create and register new streams for sending or receiving. */
> +static struct quic_stream *quic_stream_create(struct quic_stream_table *streams,
> +					      s64 max_stream_id, bool send,
> +					      bool is_serv, gfp_t gfp)
> +{

[ ... ]

> +	while (stream_id <= max_stream_id) {
> +		stream = kzalloc_obj(*stream, gfp);
> +		if (!stream)
> +			goto free;
> +
> +		stream->id = stream_id;

[ ... ]

> +		stream->recv.window = stream->recv.max_bytes;
> +		hlist_add_head(&stream->node, &head);
> +		stream_id += QUIC_STREAM_ID_STEP;
> +	}

[Severity: Low]
Should recv.state be initialized to QUIC_STREAM_RECV_STATE_RECV here?

The uapi header puts the send and receive states in one enum:

include/uapi/linux/quic.h:
enum {
	QUIC_STREAM_SEND_STATE_READY,
	...
	QUIC_STREAM_SEND_STATE_RESET_RECVD,

	QUIC_STREAM_RECV_STATE_RECV,
	...
};

That makes QUIC_STREAM_RECV_STATE_RECV equal to 6. The stream comes from
kzalloc_obj() and recv.state is never set. Every new stream therefore
starts with recv.state == 0, which is QUIC_STREAM_SEND_STATE_READY and not
a receive state.

The only reader in this patch is quic_stream_put():

	if (stream->recv.state < QUIC_STREAM_RECV_STATE_RECVD)
		return;

This works only because 0 is less than QUIC_STREAM_RECV_STATE_RECVD. Any
equality check against QUIC_STREAM_RECV_STATE_RECV would see an invalid
receive state. So would code that reports the state to userspace through
the state field of struct quic_stream_update.

[ ... ]

> +int quic_stream_init(struct quic_stream_table *streams)
> +{
> +	struct hlist_head *head;
> +	int i;
> +
> +	head = kmalloc_objs(*head, QUIC_STREAM_HT_SIZE);

[Severity: Medium]
Should this allocation be charged to the memcg?

When no gfp argument is given, kmalloc_objs() defaults to GFP_KERNEL. The
64-entry table (512 bytes on 64-bit) is therefore not accounted.

Other per-socket memory in this patch is accounted.
quic_stream_create() adds __GFP_ACCOUNT for the stream objects, and the
sock itself comes from a SLAB_ACCOUNT proto slab.

quic_init_sock() calls this for every socket(2). An unprivileged process
in a memory-limited cgroup therefore gets 512 bytes of unaccounted slab
for each QUIC socket it creates.

Would kmalloc_objs(*head, QUIC_STREAM_HT_SIZE, GFP_KERNEL_ACCOUNT) be more
consistent here?

[ ... ]

> diff --git a/net/quic/stream.h b/net/quic/stream.h
> new file mode 100644
> index 0000000000000..cb6b35929ba51
> --- /dev/null
> +++ b/net/quic/stream.h

[ ... ]

> +	/* STREAMS_BLOCKED_BIDI/UNI sent and awaiting ACK for send limits and
> +	 * MAX_STREAMS_BIDI/UNI sent and awaiting ACK for recv limits;
> +	 * MAX_STREAMS_BIDI/UNI needs to be set for send limits.
> +	 */
> +	u8 bidi_blocked;
> +	u8 uni_blocked;
> +	u8 bidi_pending;
> +	u8 uni_pending;

[Severity: Low]
This isn't a bug, but does the last line of this comment name the right
limits?

In this patch, bidi_pending and uni_pending are only used on the recv
limits. quic_stream_put() sets streams->recv.uni_pending and
streams->recv.bidi_pending. quic_stream_max_streams_update() reads and
clears only streams->recv.*_pending when deciding whether to send
MAX_STREAMS_UNI/BIDI.

Should it say "for recv limits" instead?

[ ... ]

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