Re: [PATCH net-next v16 06/15] quic: add stream management
From: netdev-bot+sashiko@kernel.org
Date: 2026-10-07 01:05:10
Also in:
linux-cifs
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.
quoted hunk
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
[ ... ]
quoted hunk
+#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.
[ ... ]
quoted hunk
+/* 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) +{
[ ... ]
quoted hunk
+ while (stream_id <= max_stream_id) { + stream = kzalloc_obj(*stream, gfp); + if (!stream) + goto free; + + stream->id = stream_id;
[ ... ]
quoted hunk
+ 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.
[ ... ]
quoted hunk
+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? [ ... ]
quoted hunk
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
[ ... ]
quoted hunk
+ /* 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