netdev - Re: [PATCH net-next v2 03/15] quic: provide common utilities and data structures

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CADvbK_e4=-JjYiVFWN-Uuroog-+AxCzgDsFg5OQqHND+RPw1Ow@mail.gmail.com>
Date: Sat, 23 Aug 2025 14:15:58 -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 03/15] quic: provide common utilities and data structures

On Thu, Aug 21, 2025 at 8:58 AM Paolo Abeni <pabeni@...hat.com> wrote:
>
> On 8/18/25 4:04 PM, Xin Long wrote:
> [...]
> > +void quic_hash_tables_destroy(void)
> > +{
> > +     struct quic_hash_table *ht;
> > +     int table;
> > +
> > +     for (table = 0; table < QUIC_HT_MAX_TABLES; table++) {
> > +             ht = &quic_hash_tables[table];
> > +             ht->size = QUIC_HT_SIZE;
>
> Why?
>
> > +             kfree(ht->hash);
> > +     }
> > +}
> > +
> > +int quic_hash_tables_init(void)
> > +{
> > +     struct quic_hash_head *head;
> > +     struct quic_hash_table *ht;
> > +     int table, i;
> > +
> > +     for (table = 0; table < QUIC_HT_MAX_TABLES; table++) {
> > +             ht = &quic_hash_tables[table];
> > +             ht->size = QUIC_HT_SIZE;
>
> AFAICS the hash table size is always QUIC_HT_SIZE, which feels like too
> small for connection and possibly quick sockets.
indeed.

>
> Do yoi need to differentiate the size among the different hash types?
Yes, I will change to use alloc_large_system_hash() for these
hashtables, align with TCP:

- source connection IDs hashtable -> tcp_hashinfo.ehash
For Data receiving
- QUIC listening sockets hashtable -> tcp_hashinfo.lhash2
For New connections
- UDP tunnel sockets hashtable -> tcp_hashinfo.bhash
For binding
- QUIC sockets hashtable -> tcp_hashinfo.bhash
For some rare special cases, like receiving a stateless reset.

>
> > +             head = kmalloc_array(ht->size, sizeof(*head), GFP_KERNEL);
>
> If so, possibly you should resort to kvmalloc_array here.
>
> > +             if (!head) {
> > +                     quic_hash_tables_destroy();
> > +                     return -ENOMEM;
> > +             }
> > +             for (i = 0; i < ht->size; i++) {
> > +                     INIT_HLIST_HEAD(&head[i].head);
> > +                     if (table == QUIC_HT_UDP_SOCK) {
> > +                             mutex_init(&head[i].m_lock);
> > +                             continue;
> > +                     }
> > +                     spin_lock_init(&head[i].s_lock);
>
> Doh, I missed the union mutex/spinlock. IMHO it would be cleaner to use
> separate hash types.
Yeah, I will separate them. :D

>
> [...]
> > +/* Parse a comma-separated string into a 'quic_data' list format.
> > + *
> > + * Each comma-separated token is turned into a length-prefixed element. The
> > + * first byte of each element stores the length (minus one). Elements are
> > + * stored in 'to->data', and 'to->len' is updated.
> > + */
> > +void quic_data_from_string(struct quic_data *to, u8 *from, u32 len)
> > +{
> > +     struct quic_data d;
> > +     u8 *p = to->data;
> > +
> > +     to->len = 0;
> > +     while (len) {
> > +             d.data = p++;
> > +             d.len  = 1;
> > +             while (len && *from == ' ') {
> > +                     from++;
> > +                     len--;
> > +             }
> > +             while (len) {
> > +                     if (*from == ',') {
> > +                             from++;
> > +                             len--;
> > +                             break;
> > +                     }
> > +                     *p++ = *from++;
> > +                     len--;
> > +                     d.len++;
> > +             }
> > +             *d.data = (u8)(d.len - 1);
> > +             to->len += d.len;
> > +     }
>
> The above does not perform any bound checking vs the destination buffer,
> it feels fragile.
>
 This function needs two checks:

1. len < U8_MAX
2. to->data memory len >= len + 1.

Although the caller like quic_sock_set_alpn() ensures that, I guess they
should still be done in this function as a common helper.

Thanks.