Re: [PATCH net-next 1/3] net/tcp-ao: convert the getsockopt helpers to sockopt_t

[Stanislav Fomichev <sdf.kernel@gmail.com>]

On 11/12, Breno Leitao wrote:
> TCP_AO_GET_KEYS, TCP_AO_INFO and TCP_AO_REPAIR are read through
> tcp_ao_get_mkts(), tcp_ao_get_sock_info() and tcp_ao_get_repair(),
> reached from do_tcp_getsockopt() and from nowhere else. Convert them to
> sockopt_t. While do_tcp_getsockopt() still carries a sockptr_t pair, it
> calls them through tcp_ao_get_sockptr(), which builds the sockopt_t and
> writes optlen back.
> 
> TCP_AO_GET_KEYS and TCP_AO_INFO read a versioned struct from optval.
> Only a user buffer gets there, as BPF's sol_tcp_sockopt() takes no
> TCP-AO option, so they keep reading it with copy_struct_from_user(),
> now on iter_in.ubuf. A kernel buffer gets -EINVAL, as in
> sockopt_expand_out().
> 
> TCP_AO_GET_KEYS is sized by a field in optval rather than by optlen:
> optlen is the size of one entry, and nkeys says how many entries the
> buffer holds. The match count then goes into the first entry's nkeys,
> past optlen when the caller's struct is shorter than that field.
> 
> Userspace relies on this, so sockopt_expand_out() grows optval for the
> reply, as for MCAST_MSFILTER. The growth is bounded by the keys on the
> socket, not by nkeys alone: sockopt_expand_out() refuses a reply above
> MAX_RW_COUNT, and a large nkeys would otherwise get -EINVAL.
> 
> iter_out only moves forward, so the entries are written in turn and the
> bytes past each one are skipped, as before. The match count is written
> last, through a copy of iter_out saved at the head of optval.
> 
> A reply that still needs more than MAX_RW_COUNT bytes now fails with
> -EINVAL. With two keys on the socket, that takes entries of about 1 GiB.
> 
> TCP_AO_INFO and TCP_AO_REPAIR now write optlen back on success, with
> the value they were given. TCP_AO_GET_KEYS stores optlen after nkeys
> instead of before it, which only shows when one of the two writes
> faults.
> 
> Signed-off-by: Breno Leitao <leitao@debian.org>
> ---
>  include/net/tcp_ao.h | 13 ++++-----
>  net/ipv4/tcp.c       | 28 +++++++++++++++++--
>  net/ipv4/tcp_ao.c    | 77 ++++++++++++++++++++++++++++++----------------------
>  3 files changed, 76 insertions(+), 42 deletions(-)
> 
> diff --git a/include/net/tcp_ao.h b/include/net/tcp_ao.h
> index 9a2333e62e9940..0e1edd777a600a 100644
> --- a/include/net/tcp_ao.h
> +++ b/include/net/tcp_ao.h
> @@ -210,9 +210,9 @@ void tcp_ao_calc_traffic_key(const struct tcp_ao_key *mkt, u8 *traffic_key,
>  void tcp_ao_destroy_sock(struct sock *sk, bool twsk);
>  void tcp_ao_time_wait(struct tcp_timewait_sock *tcptw, struct tcp_sock *tp);
>  bool tcp_ao_ignore_icmp(const struct sock *sk, int family, int type, int code);
> -int tcp_ao_get_mkts(struct sock *sk, sockptr_t optval, sockptr_t optlen);
> -int tcp_ao_get_sock_info(struct sock *sk, sockptr_t optval, sockptr_t optlen);
> -int tcp_ao_get_repair(struct sock *sk, sockptr_t optval, sockptr_t optlen);
> +int tcp_ao_get_mkts(struct sock *sk, sockopt_t *sopt);
> +int tcp_ao_get_sock_info(struct sock *sk, sockopt_t *sopt);
> +int tcp_ao_get_repair(struct sock *sk, sockopt_t *sopt);
>  int tcp_ao_set_repair(struct sock *sk, sockptr_t optval, unsigned int optlen);
>  enum skb_drop_reason tcp_inbound_ao_hash(struct sock *sk,
>  			const struct sk_buff *skb, unsigned short int family,
> @@ -334,18 +334,17 @@ static inline void tcp_ao_connect_init(struct sock *sk)
>  {
>  }
>  
> -static inline int tcp_ao_get_mkts(struct sock *sk, sockptr_t optval, sockptr_t optlen)
> +static inline int tcp_ao_get_mkts(struct sock *sk, sockopt_t *sopt)
>  {
>  	return -ENOPROTOOPT;
>  }
>  
> -static inline int tcp_ao_get_sock_info(struct sock *sk, sockptr_t optval, sockptr_t optlen)
> +static inline int tcp_ao_get_sock_info(struct sock *sk, sockopt_t *sopt)
>  {
>  	return -ENOPROTOOPT;
>  }
>  
> -static inline int tcp_ao_get_repair(struct sock *sk,
> -				    sockptr_t optval, sockptr_t optlen)
> +static inline int tcp_ao_get_repair(struct sock *sk, sockopt_t *sopt)
>  {
>  	return -ENOPROTOOPT;
>  }
> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index a1f89c1fa2ae51..552e8fe22e7e20 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -4492,6 +4492,25 @@ struct sk_buff *tcp_get_timestamping_opt_stats(const struct sock *sk,
>  	return stats;
>  }
>  
> +/* Bridge the sockptr_t pair of do_tcp_getsockopt() to a TCP-AO getter. */
> +static int tcp_ao_get_sockptr(struct sock *sk,
> +			      int (*get)(struct sock *sk, sockopt_t *sopt),
> +			      sockptr_t optval, sockptr_t optlen)
> +{
> +	struct kvec kvec;
> +	sockopt_t sopt;
> +	int err;
> +
> +	err = sockptr_to_sockopt(&sopt, optval, optlen, &kvec);
> +	if (err)
> +		return err;
> +
> +	err = get(sk, &sopt);
> +	if (!err && copy_to_sockptr(optlen, &sopt.optlen, sizeof(int)))
> +		err = -EFAULT;
> +	return err;
> +}
> +
>  int do_tcp_getsockopt(struct sock *sk, int level,
>  		      int optname, sockptr_t optval, sockptr_t optlen)
>  {
> @@ -4830,16 +4849,19 @@ int do_tcp_getsockopt(struct sock *sk, int level,
>  	case TCP_AO_REPAIR:
>  		if (!tcp_can_repair_sock(sk))
>  			return -EPERM;
> -		return tcp_ao_get_repair(sk, optval, optlen);
> +		return tcp_ao_get_sockptr(sk, tcp_ao_get_repair,
> +					  optval, optlen);
>  	case TCP_AO_GET_KEYS:
>  	case TCP_AO_INFO: {
>  		int err;
>  
>  		sockopt_lock_sock(sk);
>  		if (optname == TCP_AO_GET_KEYS)
> -			err = tcp_ao_get_mkts(sk, optval, optlen);
> +			err = tcp_ao_get_sockptr(sk, tcp_ao_get_mkts,
> +						 optval, optlen);
>  		else
> -			err = tcp_ao_get_sock_info(sk, optval, optlen);
> +			err = tcp_ao_get_sockptr(sk, tcp_ao_get_sock_info,
> +						 optval, optlen);
>  		sockopt_release_sock(sk);
>  
>  		return err;
> diff --git a/net/ipv4/tcp_ao.c b/net/ipv4/tcp_ao.c
> index bb7bbc20ba3fad..af33042478f9a8 100644
> --- a/net/ipv4/tcp_ao.c
> +++ b/net/ipv4/tcp_ao.c
> @@ -2054,14 +2054,11 @@ int tcp_v4_parse_ao(struct sock *sk, int cmd, sockptr_t optval, int optlen)
>  	return tcp_parse_ao(sk, cmd, AF_INET, optval, optlen);
>  }
>  
> -/* tcp_ao_copy_mkts_to_user(ao_info, optval, optlen)
> +/* tcp_ao_copy_mkts_to_user(ao_info, sopt)
>   *
>   * @ao_info:	struct tcp_ao_info on the socket that
>   *		socket getsockopt(TCP_AO_GET_KEYS) is executed on
> - * @optval:	pointer to array of tcp_ao_getsockopt structures in user space.
> - *		Must be != NULL.
> - * @optlen:	pointer to size of tcp_ao_getsockopt structure.
> - *		Must be != NULL.
> + * @sopt:	optval and optlen of that getsockopt(), described below.
>   *
>   * Return value: 0 on success, a negative error number otherwise.
>   *
> @@ -2091,29 +2088,31 @@ int tcp_v4_parse_ao(struct sock *sk, int cmd, sockptr_t optval, int optlen)
>   */
>  static int tcp_ao_copy_mkts_to_user(const struct sock *sk,
>  				    struct tcp_ao_info *ao_info,
> -				    sockptr_t optval, sockptr_t optlen)
> +				    sockopt_t *sopt)
>  {
>  	struct tcp_ao_getsockopt opt_in, opt_out;
>  	struct tcp_ao_key *key, *current_key;
>  	bool do_address_matching = true;
>  	union tcp_ao_addr *addr = NULL;
>  	int err, l3index, user_len;
> +	unsigned int nr_keys = 0;
>  	unsigned int max_keys;	/* maximum number of keys to copy to user */
> -	size_t out_offset = 0;
>  	size_t bytes_to_write;	/* number of bytes to write to user level */
> +	struct iov_iter head;
>  	u32 matched_keys;	/* keys from ao_info matched so far */
> -	int optlen_out;
> +	size_t reply_len;
>  	__be16 port = 0;
>  
> -	if (copy_from_sockptr(&user_len, optlen, sizeof(int)))
> -		return -EFAULT;
> +	user_len = sopt->optlen;
>  
>  	if (user_len <= 0)
>  		return -EINVAL;
>  
>  	memset(&opt_in, 0, sizeof(struct tcp_ao_getsockopt));
> -	err = copy_struct_from_sockptr(&opt_in, sizeof(opt_in),
> -				       optval, user_len);
> +	if (WARN_ON_ONCE(!iter_is_ubuf(&sopt->iter_in)))
> +		return -EINVAL;
> +	err = copy_struct_from_user(&opt_in, sizeof(opt_in), sopt->iter_in.ubuf,
> +				    user_len);
>  	if (err < 0)
>  		return err;
>  
> @@ -2214,6 +2213,21 @@ static int tcp_ao_copy_mkts_to_user(const struct sock *sk,
>  	}
>  
>  	bytes_to_write = min_t(int, user_len, sizeof(struct tcp_ao_getsockopt));
> +

[..]

> +	/* The reply is up to nkeys entries, user_len apart, with the match
> +	 * count in the first entry's nkeys, so it can outgrow optlen. Size it
> +	 * by the keys on the socket rather than by the caller's nkeys.
> +	 */
> +	hlist_for_each_entry_rcu(key, &ao_info->head, node,
> +				 lockdep_sock_is_held(sk))
> +		nr_keys++;
> +	reply_len = max(array_size(min(max_keys, nr_keys), user_len),
> +			offsetofend(struct tcp_ao_getsockopt, nkeys));
> +	err = sockopt_expand_out(sopt, reply_len);
> +	if (err)
> +		return err;
> +	head = sopt->iter_out;

Is that faster / cleaner / more preferable than calling sockopt_expand_out
on every iteration hlist_for_each_entry_rcu below?