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 -EFAULT returns "still" leave the
caller's optlen untouched.
--
ipv6: mcast: convert ip6_mc_msfget() to sockopt_t
This converts ip6_mc_msfget() and the MCAST_MSFILTER getsockopt helpers
from sockptr_t to sockopt_t. do_ipv6_getsockopt() builds the sockopt_t.
The source list is written through iter_out after sockopt_expand_out(),
and the callers then rewind to write the fixed header. The *optlen store
moves to do_ipv6_getsockopt() and happens only on success.
> diff --git a/net/ipv6/ipv6_sockglue.c b/net/ipv6/ipv6_sockglue.c
> index 5c6a0819a2aaf..1bdb3e001e4fe 100644
> --- a/net/ipv6/ipv6_sockglue.c
> +++ b/net/ipv6/ipv6_sockglue.c
> @@ -922,48 +922,51 @@ static int ipv6_getsockopt_sticky(struct sock *sk,
> struct ipv6_txoptions *opt,
[ ... ]
> sockopt_lock_sock(sk);
> - err = ip6_mc_msfget(sk, &gsf, optval, size0);
> + err = ip6_mc_msfget(sk, &gsf, opt, size0);
> if (!err) {
> if (num > gsf.gf_numsrc)
> num = gsf.gf_numsrc;
> len = GROUP_FILTER_SIZE(num);
> - if (copy_to_sockptr(optlen, &len, sizeof(int)) ||
> - copy_to_sockptr(optval, &gsf, size0))
> + opt->optlen = len;
> +
> + /* ip6_mc_msfget() consumed the whole reply; rewind to the
> + * fixed part.
> + */
> + iov_iter_revert(&opt->iter_out, len);
> + if (copy_to_iter(&gsf, size0, &opt->iter_out) != size0)
> err = -EFAULT;
> }
> sockopt_release_sock(sk);
> return err;
> }
[ ... ]
> @@ -974,18 +977,22 @@ static int compat_ipv6_get_msfilter(struct sock *sk,
> sockptr_t optval,
[ ... ]
> len = GROUP_FILTER_SIZE(num) - (sizeof(gf)-sizeof(gf32));
> - if (copy_to_sockptr(optlen, &len, sizeof(int)) ||
> - copy_to_sockptr_offset(optval, offsetof(struct compat_group_filter,
> gf_fmode),
> - &gf.gf_fmode, sizeof(gf32.gf_fmode)) ||
> - copy_to_sockptr_offset(optval, offsetof(struct compat_group_filter,
> gf_numsrc),
> - &gf.gf_numsrc, sizeof(gf32.gf_numsrc)))
> + opt->optlen = len;
> +
> + /* Rewind to gf_fmode, which gf_numsrc follows. */
> + iov_iter_revert(&opt->iter_out,
> + len - offsetof(struct compat_group_filter, gf_fmode));
> + if (copy_to_iter(&gf.gf_fmode, sizeof(gf32.gf_fmode),
> + &opt->iter_out) != sizeof(gf32.gf_fmode) ||
> + copy_to_iter(&gf.gf_numsrc, sizeof(gf32.gf_numsrc),
> + &opt->iter_out) != sizeof(gf32.gf_numsrc))
> return -EFAULT;
> return 0;
> }
> @@ -1006,9 +1013,23 @@ int do_ipv6_getsockopt(struct sock *sk, int level, int
> optname,
> return -EINVAL;
> switch (optname) {
> case MCAST_MSFILTER:
> + {
> + struct kvec kvec;
> + sockopt_t opt;
> + int err;
> +
> + err = sockptr_to_sockopt(&opt, optval, optlen, &kvec);
> + if (err)
> + return err;
> +
> if (in_compat_syscall())
> - return compat_ipv6_get_msfilter(sk, optval, optlen,
> len);
> - return ipv6_get_msfilter(sk, optval, optlen, len);
> + err = compat_ipv6_get_msfilter(sk, &opt);
> + else
> + err = ipv6_get_msfilter(sk, &opt);
> + if (!err && copy_to_sockptr(optlen, &opt.optlen, sizeof(int)))
> + err = -EFAULT;
[Severity: Low]
The commit message says:
The *optlen store moves out to the call site, guarded by !err so the
-EINVAL, -EADDRNOTAVAIL and -EFAULT returns still leave the caller's
optlen word untouched.
Is "still" accurate for the -EFAULT that comes from writing the fixed
header?
Before this patch, ipv6_get_msfilter() stored optlen first and then wrote
the header:
if (copy_to_sockptr(optlen, &len, sizeof(int)) ||
copy_to_sockptr(optval, &gsf, size0))
compat_ipv6_get_msfilter() used the same order for gf_fmode and
gf_numsrc. So when the header write faulted, the caller got -EFAULT and
optlen had already been set to the reply length.
With this patch, the helpers write the header through iter_out, and
do_ipv6_getsockopt() only stores opt.optlen when !err. After a fault on
the header write, optlen is now left untouched.
The write order is also reversed. If optval is writable and optlen is
read-only, the header now gets written before the -EFAULT. The old code
never wrote the header in that case.
This seems to last only until a later patch in the series. In "ipv6:
convert do_ipv6_getsockopt() to sockopt_t", ipv6_getsockopt() writes
sopt.optlen back unconditionally. Both helpers set opt->optlen = len
before the header copy, so a header fault reports optlen = len again, as
the original code did.
Could the commit message describe this intermediate change instead of
saying the behavior is unchanged?
> + return err;
> + }
> case IPV6_2292PKTOPTIONS:
> {
> struct msghdr msg;
[ ... ]
--
Sashiko AI review ยท
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260925-sockopt_expand_out_v2-v1-0-c3ef2e3bb5c0%40debian.org
