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

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] quic_data_from_string() (net/quic/common.c) splits tokens
  inconsistently.
- [Low] `struct quic_data.len` means two different things and the
  comments do not say so.

--

quic: provide common utilities and data structures

This patch adds the common QUIC data structures and helpers. These cover
packet header types, connection IDs, address handling, and hash tables
for socket and connection ID lookup. It also adds a quic_data binary type
with parsing, matching and memory helpers, plus encoders and decoders for
variable-length integers and transport parameters.

> diff --git a/net/quic/common.c b/net/quic/common.c
> new file mode 100644
> index 0000000000000..8114be4fbb1e6
> --- /dev/null
> +++ b/net/quic/common.c
> @@ -0,0 +1,565 @@

[ ... ]

> +/* 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. Elements are stored in
> + * 'to->data', and 'to->len' is updated.
> + */
> +int quic_data_from_string(struct quic_data *to, u8 *from, u32 len)
> +{
> +	u32 remlen = to->len;

[Severity: Low]
This isn't a bug, but should the comments say that to->len is an input
here as well as an output?

Everywhere else in this patch, struct quic_data.len is the number of
valid bytes. That holds for quic_data_dup(), quic_data_append(),
quic_data_free(), quic_data_cmp() and the new ticket/token/alpn members.
Here, though, to->len is read on entry as the capacity of to->data. It is
then overwritten with the produced length.

quic_data_to_string() does the same thing with *plen:

int quic_data_to_string(u8 *to, u32 *plen, struct quic_data *from)
{
	u32 remlen = *plen;
	...
}

Its comment only says "The resulting string length is written to
'*plen'". On the -EINVAL and -EOVERFLOW returns, *plen is left unchanged.

Neither helper has a caller in this patch. Suppose a later caller passes
an existing quic_data, such as quic_alpn(sk) filled by quic_data_dup().
Its payload length would then be treated as the writable buffer size.

Could the in/out convention be documented here, in quic_data_to_string(),
and on struct quic_data in common.h?

> +	struct quic_data d;
> +	u8 *p = to->data;
> +
> +	to->len = 0;
> +	while (len) {
> +		while (len && *from == ' ') {
> +			from++;
> +			len--;
> +		}
> +		if (!len)
> +			break;
> +		if (!remlen)
> +			return -EOVERFLOW;
> +		d.data = p++;
> +		d.len  = 0;
> +		remlen--;
> +		while (len) {
> +			if (*from == ',') {
> +				from++;
> +				len--;
> +				break;
> +			}
> +			if (!remlen)
> +				return -EOVERFLOW;
> +			if (d.len == U8_MAX)
> +				return -EINVAL;
> +			*p++ = *from++;
> +			len--;
> +			d.len++;
> +			remlen--;
> +		}
> +		*d.data = (u8)(d.len);
> +		to->len += d.len + 1;

[Severity: Low]
Are empty tokens and spaces handled consistently by this tokenizer?

The length byte is reserved with d.data = p++ before the code knows
whether the token has any content. The inner loop then breaks on ','
even when d.len is still 0. As a result, ",h3", "h3,,h2" and "h3, ,h2"
each store a zero-length element.

A trailing comma such as "h3," does not produce an element, because the
outer loop exits once len reaches 0.

Only leading spaces are stripped. Spaces before a comma are copied into
the token, so "h3 ,h2" stores "h3 " instead of "h3".

quic_data_has() and quic_data_match() compare elements by exact length
and memcmp():

	quic_data(&d, p, length);
	if (!quic_data_cmp(&d, d2))
		return true;

So "h3 " never matches "h3", and a zero-length element matches any
zero-length probe. quic_data_to_string() also writes the empty fields
back out, for example ",h3".

RFC 7301 does not allow empty protocol names in an ALPN list. This patch
has no caller yet, so once one is added only a user's own socket
configuration would be affected.

Should empty tokens be rejected or skipped, and spaces before a comma be
trimmed?

> +	}
> +	return 0;
> +}

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1794241654.git.lucien.xin%40gmail.com