Re: [net-next,v9,02/15] net: build socket infrastructure for QUIC protocol

[Simon Horman <horms@kernel.org>]

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
---
net: build socket infrastructure for QUIC protocol

This patch lays the groundwork for QUIC socket support in the kernel.
It defines the core structures and protocol hooks needed to create
QUIC sockets, without implementing any protocol behavior at this stage.
Basic integration is included to allow building the module via
CONFIG_IP_QUIC=m. This provides the scaffolding necessary for adding
actual QUIC socket behavior in follow-up patches.

> diff --git a/net/quic/protocol.c b/net/quic/protocol.c
> new file mode 100644
> index 000000000000..6ab81f1c50cc
> --- /dev/null
> +++ b/net/quic/protocol.c

[ ... ]

> @@ -0,0 +1,369 @@

[ ... ]

> +static int quic_protosw_init(void)
> +{
> +	int err;
> +
> +	err = proto_register(&quic_prot, 1);
> +	if (err)
> +		return err;
> +
> +	err = proto_register(&quicv6_prot, 1);
> +	if (err) {
> +		proto_unregister(&quic_prot);
> +		return err;
> +	}
> +
> +	inet_register_protosw(&quic_stream_protosw);
> +	inet_register_protosw(&quic_dgram_protosw);
> +	inet6_register_protosw(&quicv6_stream_protosw);
> +	inet6_register_protosw(&quicv6_dgram_protosw);
> +
> +	return 0;
> +}

Can the return values from inet6_register_protosw() be checked?

Looking at the implementation, inet6_register_protosw() returns int and
can fail with -EINVAL (if p->type >= SOCK_MAX) or -EPERM (if trying to
override a permanent protocol). The calls at lines 234-235 don't check
these return values, so if registration fails, quic_protosw_init() still
returns 0 (success).

More critically, quic_protosw_exit() unconditionally calls
inet6_unregister_protosw() on both structures:

static void quic_protosw_exit(void)
{
	inet_unregister_protosw(&quic_dgram_protosw);
	inet_unregister_protosw(&quic_stream_protosw);
	proto_unregister(&quic_prot);

	inet6_unregister_protosw(&quicv6_dgram_protosw);
	inet6_unregister_protosw(&quicv6_stream_protosw);
	proto_unregister(&quicv6_prot);
}

If a protosw was never successfully registered, its list field remains
zero-initialized (NULL pointers). inet6_unregister_protosw() calls
list_del_rcu(&p->list), which with NULL pointers will dereference NULL
in __list_del_entry() -> __list_del(NULL, NULL), causing a kernel crash.

Note that inet_register_protosw() returns void (cannot fail from caller's
perspective), but inet6_register_protosw() returns int (can fail). The
code correctly checks errors from proto_register() but not from
inet6_register_protosw().

Should the return values from inet6_register_protosw() be checked, and
should the cleanup path track which registrations succeeded so only those
are unregistered?