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

On Thu, Aug 21, 2025 at 9:43 AM Paolo Abeni [off-list ref] wrote:

On 8/18/25 4:04 PM, Xin Long wrote:

quoted

+/* Check if a stream ID is valid for sending. */
+static bool quic_stream_id_send(s64 stream_id, bool is_serv)
+{
+     u8 type = (stream_id & QUIC_STREAM_TYPE_MASK);
+
+     if (is_serv) {
+             if (type == QUIC_STREAM_TYPE_CLIENT_UNI)
+                     return false;
+     } else if (type == QUIC_STREAM_TYPE_SERVER_UNI) {
+             return false;
+     }
+     return true;
+}
+
+/* Check if a stream ID is valid for receiving. */
+static bool quic_stream_id_recv(s64 stream_id, bool is_serv)
+{
+     u8 type = (stream_id & QUIC_STREAM_TYPE_MASK);
+
+     if (is_serv) {
+             if (type == QUIC_STREAM_TYPE_SERVER_UNI)
+                     return false;
+     } else if (type == QUIC_STREAM_TYPE_CLIENT_UNI) {
+             return false;
+     }
+     return true;
+}

The above two functions could be implemented using a common helper
saving some code duplication.

Not yet sure if it's worth a helper, I need to think about it.

quoted

+/* Create and register new streams for sending. */
+static struct quic_stream *quic_stream_send_create(struct quic_stream_table *streams,
+                                                s64 max_stream_id, u8 is_serv)
+{
+     struct quic_stream *stream;
+     s64 stream_id;
+
+     stream_id = streams->send.next_bidi_stream_id;
+     if (quic_stream_id_uni(max_stream_id))
+             stream_id = streams->send.next_uni_stream_id;
+
+     /* rfc9000#section-2.1: A stream ID that is used out of order results in all streams
+      * of that type with lower-numbered stream IDs also being opened.
+      */
+     while (stream_id <= max_stream_id) {

Is wrap around thererically possible?
Who provided `max_stream_id`, the user-space? or a remote pear? what if
max_stream_id - stream_id is say 1M ?

There are two values limiting this:

1. streams->send.max_uni/bidi_stream_id:
the max_uni/bidi_stream_id that peer informs to be open.

2. streams->send.max_streams_uni/bidi (max value: QUIC_MAX_STREAMS(4096)):
to limit the number of the existing streams.

[...]

quoted

+/* Check if a receive stream ID is already closed. */
+static bool quic_stream_id_recv_closed(struct quic_stream_table *streams, s64 stream_id)
+{
+     if (quic_stream_id_uni(stream_id)) {
+             if (stream_id < streams->recv.next_uni_stream_id)
+                     return true;
+     } else {
+             if (stream_id < streams->recv.next_bidi_stream_id)
+                     return true;
+     }
+     return false;
+}

I guess the above answer my previous questions, but I think that memory
accounting for stream allocation is still deserverd.

I can give it a try. sk_r/wmem_schedule() should be used for this I suppose.

quoted

+
+/* Check if a receive stream ID exceeds would exceed local's limits. */
+static bool quic_stream_id_recv_exceeds(struct quic_stream_table *streams, s64 stream_id)
+{
+     if (quic_stream_id_uni(stream_id)) {
+             if (stream_id > streams->recv.max_uni_stream_id)
+                     return true;
+     } else {
+             if (stream_id > streams->recv.max_bidi_stream_id)
+                     return true;
+     }
+     return false;
+}
+
+/* Check if a send stream ID would exceed peer's limits. */
+bool quic_stream_id_send_exceeds(struct quic_stream_table *streams, s64 stream_id)
+{
+     u64 nstreams;
+
+     if (quic_stream_id_uni(stream_id)) {
+             if (stream_id > streams->send.max_uni_stream_id)
+                     return true;
+     } else {
+             if (stream_id > streams->send.max_bidi_stream_id)
+                     return true;
+     }
+
+     if (quic_stream_id_uni(stream_id)) {
+             stream_id -= streams->send.next_uni_stream_id;
+             nstreams = quic_stream_id_to_streams(stream_id);
+             if (nstreams + streams->send.streams_uni > streams->send.max_streams_uni)
+                     return true;
+     } else {
+             stream_id -= streams->send.next_bidi_stream_id;
+             nstreams = quic_stream_id_to_streams(stream_id);
+             if (nstreams + streams->send.streams_bidi > streams->send.max_streams_bidi)
+                     return true;
+     }
+     return false;
+}
+
+/* Get or create a send stream by ID. */
+struct quic_stream *quic_stream_send_get(struct quic_stream_table *streams, s64 stream_id,
+                                      u32 flags, bool is_serv)
+{
+     struct quic_stream *stream;
+
+     if (!quic_stream_id_send(stream_id, is_serv))
+             return ERR_PTR(-EINVAL);
+
+     stream = quic_stream_find(streams, stream_id);
+     if (stream) {
+             if ((flags & MSG_STREAM_NEW) &&
+                 stream->send.state != QUIC_STREAM_SEND_STATE_READY)
+                     return ERR_PTR(-EINVAL);
+             return stream;
+     }
+
+     if (quic_stream_id_send_closed(streams, stream_id))
+             return ERR_PTR(-ENOSTR);
+
+     if (!(flags & MSG_STREAM_NEW))
+             return ERR_PTR(-EINVAL);
+
+     if (quic_stream_id_send_exceeds(streams, stream_id))
+             return ERR_PTR(-EAGAIN);
+
+     stream = quic_stream_send_create(streams, stream_id, is_serv);
+     if (!stream)
+             return ERR_PTR(-ENOSTR);
+     streams->send.active_stream_id = stream_id;
+     return stream;

There is no locking at all in lookup/add/remove. Lacking the caller of
such functions is hard to say if that is safe. You should add some info
about that in the commit message (or lock here ;)

stream_hashtable is per connection/socket, it's always protected by
sock lock, I will add information into the commit message.

Thanks.