Re: [bpf-next PATCH v3 1/3] net: add a UID to use for ULP socket assignment

[Daniel Borkmann <daniel@iogearbox.net>]

On 01/29/2018 09:27 PM, John Fastabend wrote:
> Create a UID field and enum that can be used to assign ULPs to
> sockets. This saves a set of string comparisons if the ULP id
> is known.
> 
> For sockmap, which is added in the next patches, a ULP is used to
> hook into TCP sockets close state. In this case the ULP being added
> is done at map insert time and the ULP is known and done on the kernel
> side. In this case the named lookup is not needed. Because we don't
> want to expose psock internals to user space socket options a user
> visible flag is also added. For TLS this is set for BPF it will be
> cleared.
> 
> Alos remove pr_notice, user gets an error code back and should check
> that rather than rely on logs.
> 
> Signed-off-by: John Fastabend <john.fastabend@gmail.com>
> ---
>  include/net/tcp.h  |    6 +++++
>  net/ipv4/tcp_ulp.c |   57 +++++++++++++++++++++++++++++++++++++++++++++++-----
>  net/tls/tls_main.c |    2 ++
>  3 files changed, 60 insertions(+), 5 deletions(-)
> 
> diff --git a/include/net/tcp.h b/include/net/tcp.h
> index 093e967..ba10ca7 100644
> --- a/include/net/tcp.h
> +++ b/include/net/tcp.h
> @@ -1983,6 +1983,10 @@ static inline void tcp_listendrop(const struct sock *sk)
>  #define TCP_ULP_MAX		128
>  #define TCP_ULP_BUF_MAX		(TCP_ULP_NAME_MAX*TCP_ULP_MAX)
>  
> +enum {
> +	TCP_ULP_TLS,
> +};
> +
>  struct tcp_ulp_ops {
>  	struct list_head	list;
>  
> @@ -1991,7 +1995,9 @@ struct tcp_ulp_ops {
>  	/* cleanup ulp */
>  	void (*release)(struct sock *sk);
>  
> +	int		uid;
>  	char		name[TCP_ULP_NAME_MAX];
> +	bool		user_visible;
>  	struct module	*owner;
>  };
>  int tcp_register_ulp(struct tcp_ulp_ops *type);
> diff --git a/net/ipv4/tcp_ulp.c b/net/ipv4/tcp_ulp.c
> index 6bb9e14..6d87e7a 100644
> --- a/net/ipv4/tcp_ulp.c
> +++ b/net/ipv4/tcp_ulp.c
> @@ -29,6 +29,18 @@ static struct tcp_ulp_ops *tcp_ulp_find(const char *name)
>  	return NULL;
>  }
>  
> +static struct tcp_ulp_ops *tcp_ulp_find_id(const int ulp)
> +{
> +	struct tcp_ulp_ops *e;
> +
> +	list_for_each_entry_rcu(e, &tcp_ulp_list, list) {
> +		if (e->uid == ulp)
> +			return e;
> +	}
> +
> +	return NULL;
> +}
> +
>  static const struct tcp_ulp_ops *__tcp_ulp_find_autoload(const char *name)
>  {
>  	const struct tcp_ulp_ops *ulp = NULL;
> @@ -51,6 +63,18 @@ static const struct tcp_ulp_ops *__tcp_ulp_find_autoload(const char *name)
>  	return ulp;
>  }
>  
> +static const struct tcp_ulp_ops *__tcp_ulp_lookup(const int uid)
> +{
> +	const struct tcp_ulp_ops *ulp = NULL;

(Tiny nit: the init to NULL is not needed.)

> +	rcu_read_lock();
> +	ulp = tcp_ulp_find_id(uid);
> +	if (!ulp || !try_module_get(ulp->owner))
> +		ulp = NULL;
> +	rcu_read_unlock();
> +	return ulp;
> +}
> +
>  /* Attach new upper layer protocol to the list
>   * of available protocols.
>   */
> @@ -59,13 +83,10 @@ int tcp_register_ulp(struct tcp_ulp_ops *ulp)
>  	int ret = 0;
>  
>  	spin_lock(&tcp_ulp_list_lock);
> -	if (tcp_ulp_find(ulp->name)) {
> -		pr_notice("%s already registered or non-unique name\n",
> -			  ulp->name);
> +	if (tcp_ulp_find(ulp->name))
>  		ret = -EEXIST;
> -	} else {
> +	else
>  		list_add_tail_rcu(&ulp->list, &tcp_ulp_list);
> -	}
>  	spin_unlock(&tcp_ulp_list_lock);
>  
>  	return ret;
> @@ -124,6 +145,32 @@ int tcp_set_ulp(struct sock *sk, const char *name)
>  	if (!ulp_ops)
>  		return -ENOENT;
>  
> +	if (!ulp_ops->user_visible)

In this error path, a module_put(ulp_ops->owner) is still missing.
The prior __tcp_ulp_find_autoload() tries to get a ref on the module
as well, which we otherwise would leak (although not subject to the
two current tls/bpf users, but should still be corrected).

> +		return -ENOENT;
> +
> +	err = ulp_ops->init(sk);
> +	if (err) {
> +		module_put(ulp_ops->owner);
> +		return err;
> +	}
> +
> +	icsk->icsk_ulp_ops = ulp_ops;
> +	return 0;
> +}
> +
> +int tcp_set_ulp_id(struct sock *sk, int ulp)
> +{
> +	struct inet_connection_sock *icsk = inet_csk(sk);
> +	const struct tcp_ulp_ops *ulp_ops;
> +	int err;
> +
> +	if (icsk->icsk_ulp_ops)
> +		return -EEXIST;
> +
> +	ulp_ops = __tcp_ulp_lookup(ulp);
> +	if (!ulp_ops)
> +		return -ENOENT;
> +
>  	err = ulp_ops->init(sk);
>  	if (err) {
>  		module_put(ulp_ops->owner);
> diff --git a/net/tls/tls_main.c b/net/tls/tls_main.c
> index 736719c..b0d5fce 100644
> --- a/net/tls/tls_main.c
> +++ b/net/tls/tls_main.c
> @@ -484,6 +484,8 @@ static int tls_init(struct sock *sk)
>  
>  static struct tcp_ulp_ops tcp_tls_ulp_ops __read_mostly = {
>  	.name			= "tls",
> +	.uid			= TCP_ULP_TLS,
> +	.user_visible		= true,
>  	.owner			= THIS_MODULE,
>  	.init			= tls_init,
>  };
>