Re: [PATCH net-next 2/2] udp: convert udp_lib_getsockopt to sockopt_t

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

Breno Leitao wrote:
> In preparation for converting the proto-layer getsockopt callbacks to the
> sockopt_t interface, switch udp_lib_getsockopt() to take a sockopt_t.

What is the benefit of this conversion?

It does add a lot more complexity and makes the code less obvious.
 
> The thin udp_getsockopt()/udpv6_getsockopt() wrappers keep their __user
> signature for now: they build a user-backed sockopt_t with
> sockopt_init_user(), call the helper, and write the returned length back
> to optlen. The helper uses copy_to_iter() instead of copy_to_user().
> No functional change.
> 
> Signed-off-by: Breno Leitao <leitao@debian.org>
> ---
>  include/net/udp.h |  2 +-
>  net/ipv4/udp.c    | 40 ++++++++++++++++++++++++++--------------
>  net/ipv6/udp.c    | 17 ++++++++++++++---
>  3 files changed, 41 insertions(+), 18 deletions(-)
> 
> diff --git a/include/net/udp.h b/include/net/udp.h
> index 8262e2b215b4e..1fee17274745f 100644
> --- a/include/net/udp.h
> +++ b/include/net/udp.h
> @@ -430,7 +430,7 @@ struct sk_buff *skb_udp_tunnel_segment(struct sk_buff *skb,
>  				       netdev_features_t features,
>  				       bool is_ipv6);
>  int udp_lib_getsockopt(struct sock *sk, int level, int optname,
> -		       char __user *optval, int __user *optlen);
> +		       sockopt_t *opt);
>  int udp_lib_setsockopt(struct sock *sk, int level, int optname,
>  		       sockptr_t optval, unsigned int optlen,
>  		       int (*push_pending_frames)(struct sock *));
> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> index 70f6cbd4ef73b..0691f74db2c11 100644
> --- a/net/ipv4/udp.c
> +++ b/net/ipv4/udp.c
> @@ -76,6 +76,7 @@
>  
>  #include <linux/bpf-cgroup.h>
>  #include <linux/uaccess.h>
> +#include <linux/uio.h>
>  #include <asm/ioctls.h>
>  #include <linux/memblock.h>
>  #include <linux/highmem.h>
> @@ -2995,18 +2996,12 @@ static int udp_setsockopt(struct sock *sk, int level, int optname, sockptr_t opt
>  }
>  
>  int udp_lib_getsockopt(struct sock *sk, int level, int optname,
> -		       char __user *optval, int __user *optlen)
> +		       sockopt_t *opt)
>  {
>  	struct udp_sock *up = udp_sk(sk);
>  	int val, len;
>  
> -	if (get_user(len, optlen))
> -		return -EFAULT;
> -
> -	if (len < 0)
> -		return -EINVAL;
> -
> -	len = min_t(unsigned int, len, sizeof(int));
> +	len = min_t(unsigned int, opt->optlen, sizeof(int));
>  
>  	switch (optname) {
>  	case UDP_CORK:
> @@ -3037,9 +3032,8 @@ int udp_lib_getsockopt(struct sock *sk, int level, int optname,
>  		return -ENOPROTOOPT;
>  	}
>  
> -	if (put_user(len, optlen))
> -		return -EFAULT;
> -	if (copy_to_user(optval, &val, len))
> +	opt->optlen = len;
> +	if (copy_to_iter(&val, len, &opt->iter_out) != len)
>  		return -EFAULT;
>  	return 0;
>  }
> @@ -3047,9 +3041,27 @@ int udp_lib_getsockopt(struct sock *sk, int level, int optname,
>  static int udp_getsockopt(struct sock *sk, int level, int optname,
>  			  char __user *optval, int __user *optlen)
>  {
> -	if (level == SOL_UDP)
> -		return udp_lib_getsockopt(sk, level, optname, optval, optlen);
> -	return ip_getsockopt(sk, level, optname, optval, optlen);
> +	sockopt_t opt;
> +	int err;
> +
> +	/*
> +	 * keep the old __user pointers, until ip_getsockopt() moves
> +	 * to sockopt_t
> +	 */
> +	if (level != SOL_UDP)
> +		return ip_getsockopt(sk, level, optname, optval, optlen);
> +
> +	err = sockopt_init_user(&opt, optval, optlen);
> +	if (err)
> +		return err;
> +
> +	err = udp_lib_getsockopt(sk, level, optname, &opt);
> +	/* indepedendent of the err, return optlen */

typo

> +	if (put_user(opt.optlen, optlen))
> +		return -EFAULT;
> +	/* Optval was updated in copy_to_iter in udp_lib_getsockopt() */
> +
> +	return err;
>  }
>  
>  /**
> diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
> index 15e032194eccc..731122deed8a5 100644
> --- a/net/ipv6/udp.c
> +++ b/net/ipv6/udp.c
> @@ -1826,9 +1826,20 @@ static int udpv6_setsockopt(struct sock *sk, int level, int optname,
>  static int udpv6_getsockopt(struct sock *sk, int level, int optname,
>  			    char __user *optval, int __user *optlen)
>  {
> -	if (level == SOL_UDP)
> -		return udp_lib_getsockopt(sk, level, optname, optval, optlen);
> -	return ipv6_getsockopt(sk, level, optname, optval, optlen);
> +	sockopt_t opt;
> +	int err;
> +
> +	if (level != SOL_UDP)
> +		return ipv6_getsockopt(sk, level, optname, optval, optlen);
> +
> +	err = sockopt_init_user(&opt, optval, optlen);
> +	if (err)
> +		return err;
> +
> +	err = udp_lib_getsockopt(sk, level, optname, &opt);
> +	if (put_user(opt.optlen, optlen))
> +		return -EFAULT;
> +	return err;
>  }
>  
>  
> 
> -- 
> 2.53.0-Meta
>