Re: [net-next,v9,03/15] quic: provide common utilities and data structures
From: Xin Long <lucien.xin@gmail.com>
Date: 2026-02-05 19:18:12
Also in:
linux-cifs
On Thu, Feb 5, 2026 at 7:51 AM Paolo Abeni [off-list ref] wrote:
On 2/5/26 12:54 PM, Simon Horman wrote:quoted
This is an AI-generated review of your patch. The human sending this email has considered the AI review valid, or at least plausible. For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html --- quic: provide common utilities and data structures This patch provides foundational data structures and utilities for the QUIC stack, including hash tables for socket lookup, connection ID management, and helpers for encoding/decoding transport parameters.quoted
diff --git a/net/quic/common.c b/net/quic/common.c new file mode 100644 index 000000000000..94321f3f213d --- /dev/null +++ b/net/quic/common.c[ ... ]quoted
+/* Reads a QUIC transport parameter value. */ +u8 quic_get_param(u64 *pdest, u8 **pp, u32 *plen) +{ + u64 valuelen; + + if (!quic_get_var(pp, plen, &valuelen)) + return 0; + + if (*plen < valuelen) + return 0; + + if (!quic_get_var(pp, plen, pdest)) + return 0; + + return (u8)valuelen; +}Does quic_get_param() need to verify that quic_get_var() consumed exactly valuelen bytes? The function reads valuelen at the first quic_get_var() call, then decodes the value with the second quic_get_var(). However, quic_get_var() determines how many bytes to consume based on the varint prefix in the data, not from valuelen. If a malformed packet has valuelen=2 but the varint encoding requires 4 bytes, quic_get_var() will consume 4 bytes while the function returns valuelen=2. This could desynchronize parsing of subsequent parameters since the caller expects the buffer to have advanced by valuelen bytes based on the return value.Hard to tell without any caller for this function. Possibly you could drop it? IDK if enforcing such check would be correct/safe from an interoperability PoV.
I kind of forgot why I didn't validate it, I will decide if it should be added after interop testing with those userland implementations.
quoted
quoted
diff --git a/net/quic/common.h b/net/quic/common.h new file mode 100644 index 000000000000..bfec0aaf2907 --- /dev/null +++ b/net/quic/common.h[ ... ]quoted
+#define QUIC_CONN_ID_MAX_LEN 20[ ... ]quoted
+struct quic_conn_id { + u8 data[QUIC_CONN_ID_MAX_LEN]; + u8 len; +}; + +static inline void quic_conn_id_update(struct quic_conn_id *conn_id, u8 *data, u32 len) +{ + memcpy(conn_id->data, data, len); + conn_id->len = (u8)len; +}Should quic_conn_id_update() validate that len does not exceed QUIC_CONN_ID_MAX_LEN before the memcpy? The destination buffer conn_id->data is a 20-byte array, but the len parameter is u32 with no bounds check. If a caller passes len > 20, the memcpy would overflow the buffer.It looks like the check responsibility is on the callers. I guess it would make things more robust and simpler move the check here.
Parsing the connid from incoming packets will always check its length, not good to move the check here then return an error for coding. I will only leave a comment here. like: /* Caller must ensure len does not exceed QUIC_CONN_ID_MAX_LEN. */ Thanks.