Re: [PATCH net-next 6/6] selftests: net: getsockopt_iter: cover ip and ipv6

[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 ipv6 fixture does not test the SOL_IPV6 length handling that
  the commit message describes.
- [Low] FIXTURE_SETUP(ipv6) sets IPV6_HOPOPTS and SKIPs the whole fixture
  if that fails.
- [Low] ip.mroute_wrong_type and ipv6.mroute_wrong_type read ENOPROTOOPT
  as "CONFIG_IP_MROUTE / CONFIG_IPV6_MROUTE disabled" and SKIP.
- [Low] The "roundtrip" and clamp tests check only return codes and
  lengths, not the data that comes back.

--

selftests: net: getsockopt_iter: cover ip and ipv6

This adds ip and ipv6 fixtures to the getsockopt_iter selftest. They
check the returned length and errno across buffer sizes, the branches
that answer empty, an unknown optname, and the MRT options dispatched
before the main switch.

> diff --git a/tools/testing/selftests/net/getsockopt_iter.c b/tools/testing/selftests/net/getsockopt_iter.c
> index 6c2408df46123..32b6e0a50f1f9 100644
> --- a/tools/testing/selftests/net/getsockopt_iter.c
> +++ b/tools/testing/selftests/net/getsockopt_iter.c

[ ... ]

> @@ -492,6 +499,301 @@ TEST_F(rawv6, bad_optname)

[ ... ]

> +/* The MRT_* options are dispatched ahead of the rest of the switch and
> + * want a raw IGMP socket. Without CONFIG_IP_MROUTE they are not
> + * dispatched at all and the switch answers ENOPROTOOPT instead.
> + */
> +TEST_F(ip, mroute_wrong_type)
> +{
> +	socklen_t optlen = sizeof(int);
> +	int val;
> +
> +	ASSERT_EQ(-1, getsockopt(self->fd, SOL_IP, MRT_VERSION, &val,
> +				 &optlen));
> +	if (errno == ENOPROTOOPT)
> +		SKIP(return, "CONFIG_IP_MROUTE disabled");
> +	ASSERT_EQ(EOPNOTSUPP, errno);
> +}

[Severity: Low]
Can this SKIP hide a regression in the early MRT dispatch? A kernel with
CONFIG_IP_MROUTE=n returns ENOPROTOOPT here. A kernel where a conversion
dropped or reordered this dispatch returns the same errno:

net/ipv4/ip_sockglue.c:do_ip_getsockopt() {
    ...
	if (ip_mroute_opt(optname))
		return ip_mroute_getsockopt(sk, optname, sopt);
    ...
}

Without that dispatch, MRT_VERSION falls through to the default case and
gets -ENOPROTOOPT. ip_getsockopt() then skips the nf_getsockopt() fallback,
because !ip_mroute_opt(optname) is false. The test sees ENOPROTOOPT and
skips.

ipv6.mroute_wrong_type does the same for the ip6_mroute_opt() dispatch in
do_ipv6_getsockopt(). There, nf_getsockopt() also answers ENOPROTOOPT for an
unknown optname.

tools/testing/selftests/net/config sets CONFIG_IPV6_MROUTE=y. So on a CI
kernel, the IPv6 skip would hide a real regression.

Could the tests tell these two cases apart, rather than treating every
ENOPROTOOPT as a config skip?

[ ... ]

> +FIXTURE_SETUP(ipv6)
> +{
> +	/* an 8 byte hop-by-hop header, so the sticky options answer */
> +	static const unsigned char hopopt[8] = { 0, 0, 1, 4, 0, 0, 0, 0 };
> +	int hops = 42;
> +
> +	self->fd = socket(AF_INET6, SOCK_DGRAM, 0);
> +	if (self->fd < 0)
> +		SKIP(return, "AF_INET6 dgram socket: %s", strerror(errno));
> +
> +	if (setsockopt(self->fd, SOL_IPV6, IPV6_UNICAST_HOPS, &hops,
> +		       sizeof(hops)) < 0)
> +		SKIP(return, "set IPV6_UNICAST_HOPS: %s", strerror(errno));
> +
> +	if (setsockopt(self->fd, SOL_IPV6, IPV6_HOPOPTS, hopopt,
> +		       sizeof(hopopt)) < 0)
> +		SKIP(return, "set IPV6_HOPOPTS: %s", strerror(errno));
> +}

[Severity: Low]
Does this make the whole ipv6 fixture skip without CAP_NET_RAW? The
IPV6_HOPOPTS setsockopt is privileged:

net/ipv6/ipv6_sockglue.c:ipv6_set_opt_hdr() {
    ...
	/* hop-by-hop / destination options are privileged option */
	if (optname != IPV6_RTHDR && !sockopt_ns_capable(net->user_ns, CAP_NET_RAW))
		return -EPERM;
    ...
}

When that returns EPERM, all nine ipv6 tests skip. Six of them never use
the sticky header: hops_exact, hops_oversize_clamped, pathmtu_undersize,
pktoptions_wrong_type, mroute_wrong_type and bad_optname. hopopts_absent
opens its own socket, so it doesn't use the header either.

The ip fixture sets a router alert IP_OPTIONS, which needs no capability.
So the two fixtures behave differently under the same privileges.

Could the IPV6_HOPOPTS setup move into the hopopts tests that need it?

[ ... ]

> +TEST_F(ipv6, hops_exact)
> +{
> +	socklen_t optlen = sizeof(int);
> +	int val = 0;
> +
> +	ASSERT_EQ(0, getsockopt(self->fd, SOL_IPV6, IPV6_UNICAST_HOPS, &val,
> +				&optlen));
> +	ASSERT_EQ(sizeof(int), optlen);
> +	ASSERT_EQ(42, val);
> +}
> +
> +TEST_F(ipv6, hops_oversize_clamped)
> +{
> +	socklen_t optlen = 64;
> +	char buf[64] = {};
> +
> +	ASSERT_EQ(0, getsockopt(self->fd, SOL_IPV6, IPV6_UNICAST_HOPS, buf,
> +				&optlen));
> +	ASSERT_EQ(sizeof(int), optlen);
> +}

[Severity: Low]
The commit message says:

    SOL_IP answers a sub-int buffer with one byte where SOL_IPV6 clamps the
    int.

ip.ttl_single_byte covers the SOL_IP half. Is there an ipv6 test for the
other half? The ipv6 fixture only has hops_exact and hops_oversize_clamped.
Nothing matches ip.ttl_single_byte, ip.ttl_zero_len or ip.negative_optlen.

So no test runs the clamping tail of do_ipv6_getsockopt() with a sub-int
buffer:

net/ipv6/ipv6_sockglue.c:do_ipv6_getsockopt() {
    ...
	len = min_t(unsigned int, sizeof(int), len);
	sopt->optlen = len;
    ...
}

An earlier patch in this series, "ipv6: reject a negative optlen in
do_ipv6_getsockopt()", added a negative optlen rejection. It now lives in
sockopt_init_user(), called from ipv6_getsockopt(), and nothing here tests
it.

rawv6.hdrincl_undersize_clamped doesn't reach this path either.
rawv6_getsockopt() sends IPV6_HDRINCL to do_rawv6_getsockopt() instead.

Would it make sense to add sub-int, zero-length and negative optlen cases
for IPV6_UNICAST_HOPS?

> +
> +TEST_F(ipv6, hopopts_roundtrip)
> +{
> +	unsigned char buf[64] = {};
> +	socklen_t optlen = sizeof(buf);
> +
> +	ASSERT_EQ(0, getsockopt(self->fd, SOL_IPV6, IPV6_HOPOPTS, buf,
> +				&optlen));
> +	ASSERT_EQ(8, optlen);
> +}
> +
> +TEST_F(ipv6, hopopts_undersize_clamped)
> +{
> +	unsigned char buf[4] = {};
> +	socklen_t optlen = sizeof(buf);
> +
> +	ASSERT_EQ(0, getsockopt(self->fd, SOL_IPV6, IPV6_HOPOPTS, buf,
> +				&optlen));
> +	ASSERT_EQ(sizeof(buf), optlen);
> +}

[Severity: Low]
Should these tests compare buf against hopopt[]? Both only check optlen.
ipv6_getsockopt_sticky() copies the data separately from returning the
length:

	len = min_t(unsigned int, len, ipv6_optlen(hdr));
	if (copy_to_iter(hdr, len, &sopt->iter_out) != len)
		return -EFAULT;
	return len;

A conversion that copied the wrong bytes, or skipped the copy entirely,
would still pass as long as the length was right.

The IPv4 tests have a similar gap. ip.options_roundtrip only checks
buf[0] == 0x94, and ignores the length byte and the value bytes of
ipopts[]. ip.options_undersize_clamped only checks the length. In both
cases the data comes from the copy_to_iter() of opt->__data in
do_ip_getsockopt().

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260925-sockopt_expand_out_v2-v1-0-c3ef2e3bb5c0%40debian.org