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 [off-list ref] wrote:

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

quoted

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

quoted

+             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.

quoted

+             head = kmalloc_array(ht->size, sizeof(*head), GFP_KERNEL);

If so, possibly you should resort to kvmalloc_array here.

quoted

+             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

[...]

quoted

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