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 commit message says the wrong thing about the order of
checks.
--
ipv6: convert do_ipv6_getsockopt() to sockopt_t
This commit moves do_ipv6_getsockopt(), ip6_mroute_getsockopt() and
ipv6_getsockopt_sticky() from sockptr_t to sockopt_t, and drops the
MCAST_MSFILTER bridge. ipv6_getsockopt() now builds the sockopt_t with
sockopt_init_user() and always writes optlen back. The BPF caller
sol_ipv6_sockopt() builds it with sockptr_to_sockopt().
> MRT6_* is still dispatched before optlen is validated, so a negative
> optlen there wins over -EOPNOTSUPP.
[Severity: Low]
Is this description accurate? After this patch, ipv6_getsockopt() checks
optlen in sockopt_init_user(). That happens before do_ipv6_getsockopt()
reaches the ip6_mroute_opt() dispatch:
net/ipv6/ipv6_sockglue.c:ipv6_getsockopt() {
...
err = sockopt_init_user(&sopt, optval, optlen);
if (err)
return err;
err = do_ipv6_getsockopt(sk, level, optname, &sopt);
...
}
sockopt_init_user() rejects a bad optlen before anything else runs:
include/linux/net.h:sockopt_init_user() {
if (get_user(len, optlen))
return -EFAULT;
if (len < 0)
return -EINVAL;
...
}
sol_ipv6_sockopt() does the same thing through sockptr_to_sockopt().
So optlen is now validated before MRT6_* is dispatched, which is the
opposite of what the message says. The stated result is still right: a
negative optlen now wins over -EOPNOTSUPP. But that is because of the new
order, not the old one.
The IPv4 sibling commit "ipv4: convert do_ip_getsockopt() to sockopt_t"
describes the same change this way: "Validating optlen now precedes the
level check and the MRT_* dispatch". Could this message use similar
wording?
Could the message also list every errno whose precedence changed? Before
this patch, ip6_mroute_getsockopt() returned these without reading optlen:
-EOPNOTSUPP (not a raw ICMPv6 socket)
-ENOENT (no mr_table)
-ENOPROTOOPT (unknown MRT6 name)
All three are now overridden by -EINVAL for a negative optlen and by
-EFAULT for an unreadable one. Under CONFIG_NETFILTER, the -ENOPROTOOPT
case used to fall through to nf_getsockopt().
Both callers now reject a negative optlen before calling in. Can the
len < 0 check in do_ipv6_getsockopt() and the olr < 0 check in
ip6_mroute_getsockopt() still be reached?
--
Sashiko AI review ยท
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260925-sockopt_expand_out_v2-v1-0-c3ef2e3bb5c0%40debian.org
