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

[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] The patch installs the native family ioctl handlers in
  quic_proto_ops (`.ioctl = inet_ioctl`, net/quic/protocol.c:138) and…

--

net: build socket infrastructure for QUIC protocol

This patch adds the first socket scaffolding for an in-kernel QUIC
implementation. It defines the QUIC proto and proto_ops tables for IPv4
and IPv6, per-netns SNMP statistics and sysctls. It also adds the
Kconfig/Makefile glue needed to build the code as CONFIG_IP_QUIC=m.

> diff --git a/net/quic/protocol.c b/net/quic/protocol.c
> new file mode 100644
> index 0000000000000..91db48e79c2c6
> --- /dev/null
> +++ b/net/quic/protocol.c
> @@ -0,0 +1,378 @@

[ ... ]

> +static const struct proto_ops quic_proto_ops = {
> +	.family		   = PF_INET,

[ ... ]

> +	.poll		   = quic_inet_poll,
> +	.ioctl		   = inet_ioctl,
> +	.gettstamp	   = sock_gettstamp,

[ ... ]

> +static const struct proto_ops quicv6_proto_ops = {
> +	.family		   = PF_INET6,

[ ... ]

> +	.poll		   = quic_inet_poll,
> +	.ioctl		   = inet6_ioctl,
> +	.gettstamp	   = sock_gettstamp,

[Severity: Low]
Should quic_proto_ops and quicv6_proto_ops also set .compat_ioctl, to
inet_compat_ioctl and inet6_compat_ioctl respectively?

Both tables use the native inet_ioctl() and inet6_ioctl() handlers but
leave .compat_ioctl NULL. On a CONFIG_COMPAT kernel, a 32-bit task that
issues SIOCADDRT or SIOCDELRT on a QUIC socket ends up here:

net/socket.c:compat_sock_ioctl() {
    ...
	if (ops->compat_ioctl)
		ret = ops->compat_ioctl(sock, cmd, arg);
    ...
	if (ret == -ENOIOCTLCMD)
		ret = compat_sock_ioctl_trans(file, sock, cmd, arg);
    ...
}

compat_sock_ioctl_trans() has no case for SIOCADDRT or SIOCDELRT. The
32-bit task therefore gets -ENOTTY. A native 64-bit task on the same
socket reaches the routing ioctl.

Only inet_compat_ioctl() and inet6_compat_ioctl() translate the compat
struct rtentry and struct in6_rtmsg:

net/ipv4/af_inet.c:inet_compat_ioctl() {
    ...
	case SIOCADDRT:
	case SIOCDELRT:
		return inet_compat_routing_ioctl(sk, cmd, argp);
    ...
}

The other INET and INET6 proto_ops tables that use inet_ioctl() or
inet6_ioctl() all set .compat_ioctl. These include inet_stream_ops,
inet_dgram_ops, inet_sockraw_ops, inet6_stream_ops, inet6_dgram_ops, and
the SCTP, MPTCP and raw IPv6 ops.

In the final revision of the series, the only ioctl entries under
net/quic/ are these two .ioctl lines. So no later patch seems to add
.compat_ioctl for QUIC either.

[ ... ]

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