Re: [PATCH net-next v2 2/4] net: call getsockopt_iter if available

[Stanislav Fomichev]

On 04/02, Breno Leitao wrote:
> Hello Stanislav,
> 
> On Wed, Apr 01, 2026 at 11:10:22AM -0700, Stanislav Fomichev wrote:
> > So maybe something like this is better to communicate your long term intent?
> > 
> > 	} else if (ops->getsockopt_iter) {
> > 		optval = sockptr_to_iter(optval)
> > 		optlen = sockptr_to_iter(optlen)
> > 		do_sock_getsockopt_iter(...) /* does not know what sockpt_t is */
> > 	}
> > 
> > ?
> > 
> > Then your new do_sock_getsockopt_iter is sockptr-free from the beginning
> > and at some point we'll just drop/move those sockptr_to_iter calls?
> 
> Sure, that would work as well. It would look like the following, from my
> current implemention:
> 
> +static int sockptr_to_sockopt(sockopt_t *opt, sockptr_t optval,
> +                             sockptr_t optlen, struct kvec *kvec)
> +{
> +       int koptlen;
> +
> +       if (copy_from_sockptr(&koptlen, optlen, sizeof(int)))
> +               return -EFAULT;
> +
> +       if (optval.is_kernel) {
> +               kvec->iov_base = optval.kernel;
> +               kvec->iov_len = koptlen;
> +               iov_iter_kvec(&opt->iter_out, ITER_DEST, kvec, 1, koptlen);
> +               iov_iter_kvec(&opt->iter_in, ITER_SOURCE, kvec, 1, koptlen);
> +       } else {
> +               iov_iter_ubuf(&opt->iter_out, ITER_DEST, optval.user, koptlen);
> +               iov_iter_ubuf(&opt->iter_in, ITER_SOURCE, optval.user,
> +                             koptlen);
> +       }
> +       opt->optlen = koptlen;
> +
> +       return 0;
> +}
> +
>  int do_sock_getsockopt(struct socket *sock, bool compat, int level,
>                        int optname, sockptr_t optval, sockptr_t optlen)
>  {
> @@ -2366,15 +2390,31 @@ int do_sock_getsockopt(struct socket *sock, bool compat, int level,
> 
> +       } else if (ops->getsockopt_iter) {
> +               struct kvec kvec;
> +               sockopt_t opt;
> +
> +               err = sockptr_to_sockopt(&opt, optval, optlen, &kvec);
> +               if (err)
> +                       return err;
> +
> +               err = ops->getsockopt_iter(sock, level, optname, &opt);
> +
> +               /* Always write back optlen, even on failure. Some protocols
> +                * (e.g. CAN raw) return -ERANGE and set optlen to the
> +                * required buffer size so userspace can discover it.
> +                */
> +               if (copy_to_sockptr(optlen, &opt.optlen, sizeof(int)))
> +                       return -EFAULT;
> +       } else if (ops->getsockopt) {
> ....
> 
> > I hope this way it will be easier to review protocol handler changes.
> > 
> > For example, looking at your AF_PACKET patch, you won't have to care
> > about flipping the source and doing the revert. Most/all of the changes will
> > be simple:
> > - s/get_user(len, optlen)/len = opt->optlen/
> > - s/put_user(len, optlen)/opt->optlen = len/
> > - s/copy_from_user(xxx, optval, len)/copy_from_iter(xxx, len, &opt->iter_in)/
> > - s/copy_to_user(optval, xxx, len)/copy_to_iter(xxx, len, &opt->iter_out)/
> 
> That is, in fact, a great proposal. It will make the protocol changes review
> way easier.
> 
> This is what I have right now.
> 
> 	typedef struct sockopt {
> 		struct iov_iter iter_out;
> 		struct iov_iter iter_in;
> 		int optlen;
> 	} sockopt_t;
> 
> 
> And then, the drivers change would be as simple as:
> 
>  static int packet_getsockopt(struct socket *sock, int level, int optname,
> -                            char __user *optval, int __user *optlen)
> +                            sockopt_t *opt)
>  {
>         int len;
>         int val, lv = sizeof(val);
> @@ -4065,8 +4066,7 @@ static int packet_getsockopt(struct socket *sock, int level, int optname,
>         if (level != SOL_PACKET)
>                 return -ENOPROTOOPT;
> 
> -       if (get_user(len, optlen))
> -               return -EFAULT;
> +       len = opt->optlen;
> 
>         if (len < 0)
>                 return -EINVAL;
> @@ -4115,7 +4115,7 @@ static int packet_getsockopt(struct socket *sock, int level, int optname,
>                         len = sizeof(int);
>                 if (len < sizeof(int))
>                         return -EINVAL;
> -               if (copy_from_user(&val, optval, len))
> +               if (copy_from_iter(&val, len, &opt->iter_in) != len)
>                         return -EFAULT;
>                 switch (val) {
>                 case TPACKET_V1:
> @@ -4171,9 +4171,8 @@ static int packet_getsockopt(struct socket *sock, int level, int optname,
> 
>         if (len > lv)
>                 len = lv;
> -       if (put_user(len, optlen))
> -               return -EFAULT;
> -       if (copy_to_user(optval, data, len))
> +       opt->optlen = len;
> +       if (copy_to_iter(data, len, &opt->iter_out) != len)
>                 return -EFAULT;
>         return 0;
> 
> This is not fully tested yet, but, in case you want to see how this looks like
> so far, I have it in https://github.com/leitao/linux/tree/b4/getsockopt_v3.
> 
> I will submit a newer version after I am done with the testing.
> 
> Thanks for the insights,
> --breno

LGTM, thanks!