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

2018-01-26 Thread John Fastabend 
On 01/26/2018 07:52 AM, Dave Watson wrote:
> On 01/25/18 04:27 PM, John Fastabend wrote:
>> I did not however retest TLS with the small change to ULP layer.
>> Mostly because I don't have a TLS setup. I plan/hope to get around
>> to writing either a sample or preferably a selftest for this case
>> as well (assuming I didn't miss one).
> 
>> @Dave Watson can you take a quick look to verify the changes are
>> good on TLS ULP side.
> 
> Looks reasonable, and passes my test suite.  One comment below
> 
> Tested-by: Dave Watson 
> 
>> Signed-off-by: John Fastabend 
>> ---
>>  include/net/tcp.h  |6 ++
>>  net/ipv4/tcp_ulp.c |   53 
>> +++-
>>  net/tls/tls_main.c |2 ++
>>  3 files changed, 56 insertions(+), 5 deletions(-)
>>
>> diff --git a/net/ipv4/tcp_ulp.c b/net/ipv4/tcp_ulp.c
>> index 6bb9e14..8ef437d 100644
>> --- a/net/ipv4/tcp_ulp.c
>> +++ b/net/ipv4/tcp_ulp.c
>> @@ -133,3 +157,22 @@ int tcp_set_ulp(struct sock *sk, const char *name)
>>  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)
>> +icsk->icsk_ulp_ops = ulp_ops;
> 
> Does this need module_put on error, similar to tcp_set_ulp?

Not needed in current use because its only being used with an in-kernel
ULP. However, best to fix it so that we don't have a (hard to detect)
bug first time someone uses this with a module ULP.

Thanks I'll spin a v2 this morning.

> 
>> +return err;
>> +}

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

2018-01-26 Thread Dave Watson 
On 01/25/18 04:27 PM, John Fastabend wrote:
> I did not however retest TLS with the small change to ULP layer.
> Mostly because I don't have a TLS setup. I plan/hope to get around
> to writing either a sample or preferably a selftest for this case
> as well (assuming I didn't miss one).

> @Dave Watson can you take a quick look to verify the changes are
> good on TLS ULP side.

Looks reasonable, and passes my test suite.  One comment below

Tested-by: Dave Watson 

> Signed-off-by: John Fastabend 
> ---
>  include/net/tcp.h  |6 ++
>  net/ipv4/tcp_ulp.c |   53 
> +++-
>  net/tls/tls_main.c |2 ++
>  3 files changed, 56 insertions(+), 5 deletions(-)
> 
> diff --git a/net/ipv4/tcp_ulp.c b/net/ipv4/tcp_ulp.c
> index 6bb9e14..8ef437d 100644
> --- a/net/ipv4/tcp_ulp.c
> +++ b/net/ipv4/tcp_ulp.c
> @@ -133,3 +157,22 @@ int tcp_set_ulp(struct sock *sk, const char *name)
>   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)
> + icsk->icsk_ulp_ops = ulp_ops;

Does this need module_put on error, similar to tcp_set_ulp?

> + return err;
> +}

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

2018-01-25 Thread John Fastabend 
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 
---
 include/net/tcp.h  |6 ++
 net/ipv4/tcp_ulp.c |   53 +++-
 net/tls/tls_main.c |2 ++
 3 files changed, 56 insertions(+), 5 deletions(-)

diff --git a/include/net/tcp.h b/include/net/tcp.h
index 6da880d..ab9b714 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_MAX128
 #define TCP_ULP_BUF_MAX(TCP_ULP_NAME_MAX*TCP_ULP_MAX)
 
+enum {
+   TCP_ULP_TLS,
+};
+
 struct tcp_ulp_ops {
struct list_headlist;
 
@@ -1991,7 +1995,9 @@ struct tcp_ulp_ops {
/* cleanup ulp */
void (*release)(struct sock *sk);
 
+   int uid;
charname[TCP_ULP_NAME_MAX];
+   booluser_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..8ef437d 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, _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;
+
+   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(_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(>list, _ulp_list);
-   }
spin_unlock(_ulp_list_lock);
 
return ret;
@@ -124,6 +145,9 @@ int tcp_set_ulp(struct sock *sk, const char *name)
if (!ulp_ops)
return -ENOENT;
 
+   if (!ulp_ops->user_visible)
+   return -ENOENT;
+
err = ulp_ops->init(sk);
if (err) {
module_put(ulp_ops->owner);
@@ -133,3 +157,22 @@ int tcp_set_ulp(struct sock *sk, const char *name)
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)
+   icsk->icsk_ulp_ops = ulp_ops;
+   return err;
+}
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,
 };