| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CADvbK_dJoCa5t3OA61gJtMkq1uG7C77MEMwz9XXm_HLc4FgHWA@mail.gmail.com>
Date: Sat, 23 Aug 2025 13:14:52 -0400
From: Xin Long <lucien.xin@...il.com>
To: Paolo Abeni <pabeni@...hat.com>
Cc: network dev <netdev@...r.kernel.org>, 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 Thu, Aug 21, 2025 at 9:43 AM Paolo Abeni <pabeni@...hat.com> wrote:
>
> 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.
Not yet sure if it's worth a helper, I need to think about it.
>
> > +/* 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.
>
> [...]
> > +/* 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.
> > +
> > +/* 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.
Powered by blists - more mailing lists