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.fastab...@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,
> };
>
