Message-ID: <a79bfa8b-657f-4358-99f3-2774eb65d49f@redhat.com>
Date: Thu, 21 Aug 2025 15:43:47 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: Xin Long <lucien.xin@...il.com>, network dev <netdev@...r.kernel.org>
Cc: davem@...emloft.net, kuba@...nel.org, Eric Dumazet <edumazet@...gle.com>,
 Simon Horman <horms@...nel.org>, Stefan Metzmacher <metze@...ba.org>,
 Moritz Buhl <mbuhl@...nbsd.org>, Tyler Fanelli <tfanelli@...hat.com>,
 Pengtao He <hepengtao@...omi.com>, linux-cifs@...r.kernel.org,
 Steve French <smfrench@...il.com>, Namjae Jeon <linkinjeon@...nel.org>,
 Paulo Alcantara <pc@...guebit.com>, Tom Talpey <tom@...pey.com>,
 kernel-tls-handshake@...ts.linux.dev, Chuck Lever <chuck.lever@...cle.com>,
 Jeff Layton <jlayton@...nel.org>, Benjamin Coddington <bcodding@...hat.com>,
 Steve Dickson <steved@...hat.com>, Hannes Reinecke <hare@...e.de>,
 Alexander Aring <aahringo@...hat.com>, David Howells <dhowells@...hat.com>,
 Cong Wang <xiyou.wangcong@...il.com>, "D . Wythe"
 <alibuda@...ux.alibaba.com>, Jason Baron <jbaron@...mai.com>,
 illiliti <illiliti@...tonmail.com>, Sabrina Dubroca <sd@...asysnail.net>,
 Marcelo Ricardo Leitner <marcelo.leitner@...il.com>,
 Daniel Stenberg <daniel@...x.se>,
 Andy Gospodarek <andrew.gospodarek@...adcom.com>
Subject: Re: [PATCH net-next v2 06/15] quic: add stream management

On 8/18/25 4:04 PM, Xin Long wrote:
> +/* 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.

> +/* 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 ?

[...]
> +/* 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.

> +
> +/* 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 ;)

/P

Powered by blists - more mailing lists