[bpf PATCH 0/3] bpf sockmap fixes

[bpf PATCH 0/3] bpf sockmap fixes

[John Fastabend]

A set of fixes for sockmap to resolve map/prog not being cleaned
up when maps are not cleaned up from the program side.

For this we pull in the ULP infrastructure to hook into the close()
hook of the sock layer. This seemed natural because we have additional
sockmap features (to add support for TX hooks) that will also use the
ULP infrastructure. This allows us to cleanup entries in the map when
socks are closed() and avoid trying to get the sk_state_change() hook
to fire in all cases.

The second issue resolved here occurs when users don't detach
programs. The gist is a refcnt issue resolved by implementing the
release callback. See patch for details.

For testing I ran both sample/sockmap and selftests bpf/test_maps.c.
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.

---

John Fastabend (3):
      net: add a UID to use for ULP socket assignment
      bpf: sockmap, add sock close() hook to remove socks
      bpf: sockmap, fix leaking maps with attached but not detached progs


 include/net/tcp.h    |    8 ++
 kernel/bpf/sockmap.c |  164 ++++++++++++++++++++++++++++----------------------
 net/ipv4/tcp_ulp.c   |   53 +++++++++++++++-
 net/tls/tls_main.c   |    2 +
 4 files changed, 150 insertions(+), 77 deletions(-)

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

[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 <john.fastabend@gmail.com>
---
 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_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..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, &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;
+
+	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,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,
 };

[bpf PATCH 2/3] bpf: sockmap, add sock close() hook to remove socks

[John Fastabend]

The selftests test_maps program was leaving dangling BPF sockmap
programs around because not all psock elements were removed from
the map. The elements in turn hold a reference on the BPF program
they are attached to causing BPF programs to stay open even after
test_maps has completed.

The original intent was that sk_state_change() would be called
when TCP socks went through TCP_CLOSE state. However, because
socks may be in SOCK_DEAD state or the sock may be a listening
socket the event is not always triggered.

To resolve this use the ULP infrastructure and register our own
proto close() handler. This fixes the above case.

Fixes: 174a79ff9515 ("bpf: sockmap with sk redirect support")
Reported-by: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
Signed-off-by: John Fastabend <john.fastabend@gmail.com>
---
 include/net/tcp.h    |    2 +
 kernel/bpf/sockmap.c |  145 +++++++++++++++++++++++++++-----------------------
 2 files changed, 80 insertions(+), 67 deletions(-)

diff --git a/include/net/tcp.h b/include/net/tcp.h
index ab9b714..6a88353 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -1985,6 +1985,7 @@ static inline void tcp_listendrop(const struct sock *sk)
 
 enum {
 	TCP_ULP_TLS,
+	TCP_ULP_BPF,
 };
 
 struct tcp_ulp_ops {
@@ -2003,6 +2004,7 @@ struct tcp_ulp_ops {
 int tcp_register_ulp(struct tcp_ulp_ops *type);
 void tcp_unregister_ulp(struct tcp_ulp_ops *type);
 int tcp_set_ulp(struct sock *sk, const char *name);
+int tcp_set_ulp_id(struct sock *sk, const int ulp);
 void tcp_get_available_ulp(char *buf, size_t len);
 void tcp_cleanup_ulp(struct sock *sk);
 
diff --git a/kernel/bpf/sockmap.c b/kernel/bpf/sockmap.c
index 1712d31..b85a79c 100644
--- a/kernel/bpf/sockmap.c
+++ b/kernel/bpf/sockmap.c
@@ -86,9 +86,10 @@ struct smap_psock {
 	struct work_struct tx_work;
 	struct work_struct gc_work;
 
+	struct proto *sk_proto;
+	void (*save_close)(struct sock *sk, long timeout);
 	void (*save_data_ready)(struct sock *sk);
 	void (*save_write_space)(struct sock *sk);
-	void (*save_state_change)(struct sock *sk);
 };
 
 static inline struct smap_psock *smap_psock_sk(const struct sock *sk)
@@ -104,12 +105,79 @@ static inline void bpf_compute_data_end_sk_skb(struct sk_buff *skb)
 	TCP_SKB_CB(skb)->bpf.data_end = skb->data + skb_headlen(skb);
 }
 
+struct proto tcp_bpf_proto;
+static int bpf_tcp_init(struct sock *sk)
+{
+	struct smap_psock *psock;
+
+	psock = smap_psock_sk(sk);
+	if (unlikely(!psock))
+		return -EINVAL;
+
+	if (unlikely(psock->sk_proto))
+		return -EBUSY;
+
+	psock->save_close = sk->sk_prot->close;
+	psock->sk_proto = sk->sk_prot;
+	sk->sk_prot = &tcp_bpf_proto;
+	return 0;
+}
+
+static void bpf_tcp_release(struct sock *sk)
+{
+	struct smap_psock *psock = smap_psock_sk(sk);
+
+	if (likely(psock)) {
+		sk->sk_prot = psock->sk_proto;
+		psock->sk_proto = NULL;
+	}
+}
+
+static void smap_release_sock(struct smap_psock *psock, struct sock *sock);
+
+void bpf_tcp_close(struct sock *sk, long timeout)
+{
+	struct smap_psock_map_entry *e, *tmp;
+	struct smap_psock *psock;
+	struct sock *osk;
+
+	psock = smap_psock_sk(sk);
+	if (unlikely(!psock))
+		return sk->sk_prot->close(sk, timeout);
+
+	write_lock_bh(&sk->sk_callback_lock);
+	list_for_each_entry_safe(e, tmp, &psock->maps, list) {
+		osk = cmpxchg(e->entry, sk, NULL);
+		if (osk == sk) {
+			list_del(&e->list);
+			smap_release_sock(psock, sk);
+		}
+	}
+	write_unlock_bh(&sk->sk_callback_lock);
+	return psock->save_close(sk, timeout);
+}
+
 enum __sk_action {
 	__SK_DROP = 0,
 	__SK_PASS,
 	__SK_REDIRECT,
 };
 
+static struct tcp_ulp_ops bpf_tcp_ulp_ops __read_mostly = {
+	.name		= "bpf_tcp",
+	.uid		= TCP_ULP_BPF,
+	.owner		= NULL,
+	.init		= bpf_tcp_init,
+	.release	= bpf_tcp_release,
+};
+
+static int bpf_tcp_ulp_register(void)
+{
+	tcp_bpf_proto = tcp_prot;
+	tcp_bpf_proto.close = bpf_tcp_close;
+	return tcp_register_ulp(&bpf_tcp_ulp_ops);
+}
+
 static int smap_verdict_func(struct smap_psock *psock, struct sk_buff *skb)
 {
 	struct bpf_prog *prog = READ_ONCE(psock->bpf_verdict);
@@ -174,68 +242,6 @@ static void smap_report_sk_error(struct smap_psock *psock, int err)
 	sk->sk_error_report(sk);
 }
 
-static void smap_release_sock(struct smap_psock *psock, struct sock *sock);
-
-/* Called with lock_sock(sk) held */
-static void smap_state_change(struct sock *sk)
-{
-	struct smap_psock_map_entry *e, *tmp;
-	struct smap_psock *psock;
-	struct socket_wq *wq;
-	struct sock *osk;
-
-	rcu_read_lock();
-
-	/* Allowing transitions into an established syn_recv states allows
-	 * for early binding sockets to a smap object before the connection
-	 * is established.
-	 */
-	switch (sk->sk_state) {
-	case TCP_SYN_SENT:
-	case TCP_SYN_RECV:
-	case TCP_ESTABLISHED:
-		break;
-	case TCP_CLOSE_WAIT:
-	case TCP_CLOSING:
-	case TCP_LAST_ACK:
-	case TCP_FIN_WAIT1:
-	case TCP_FIN_WAIT2:
-	case TCP_LISTEN:
-		break;
-	case TCP_CLOSE:
-		/* Only release if the map entry is in fact the sock in
-		 * question. There is a case where the operator deletes
-		 * the sock from the map, but the TCP sock is closed before
-		 * the psock is detached. Use cmpxchg to verify correct
-		 * sock is removed.
-		 */
-		psock = smap_psock_sk(sk);
-		if (unlikely(!psock))
-			break;
-		write_lock_bh(&sk->sk_callback_lock);
-		list_for_each_entry_safe(e, tmp, &psock->maps, list) {
-			osk = cmpxchg(e->entry, sk, NULL);
-			if (osk == sk) {
-				list_del(&e->list);
-				smap_release_sock(psock, sk);
-			}
-		}
-		write_unlock_bh(&sk->sk_callback_lock);
-		break;
-	default:
-		psock = smap_psock_sk(sk);
-		if (unlikely(!psock))
-			break;
-		smap_report_sk_error(psock, EPIPE);
-		break;
-	}
-
-	wq = rcu_dereference(sk->sk_wq);
-	if (skwq_has_sleeper(wq))
-		wake_up_interruptible_all(&wq->wait);
-	rcu_read_unlock();
-}
-
 static void smap_read_sock_strparser(struct strparser *strp,
 				     struct sk_buff *skb)
 {
@@ -330,10 +336,8 @@ static void smap_stop_sock(struct smap_psock *psock, struct sock *sk)
 		return;
 	sk->sk_data_ready = psock->save_data_ready;
 	sk->sk_write_space = psock->save_write_space;
-	sk->sk_state_change = psock->save_state_change;
 	psock->save_data_ready = NULL;
 	psock->save_write_space = NULL;
-	psock->save_state_change = NULL;
 	strp_stop(&psock->strp);
 	psock->strp_enabled = false;
 }
@@ -358,6 +362,7 @@ static void smap_release_sock(struct smap_psock *psock, struct sock *sock)
 	if (psock->refcnt)
 		return;
 
+	tcp_cleanup_ulp(sock);
 	smap_stop_sock(psock, sock);
 	clear_bit(SMAP_TX_RUNNING, &psock->state);
 	rcu_assign_sk_user_data(sock, NULL);
@@ -435,10 +440,8 @@ static void smap_start_sock(struct smap_psock *psock, struct sock *sk)
 		return;
 	psock->save_data_ready = sk->sk_data_ready;
 	psock->save_write_space = sk->sk_write_space;
-	psock->save_state_change = sk->sk_state_change;
 	sk->sk_data_ready = smap_data_ready;
 	sk->sk_write_space = smap_write_space;
-	sk->sk_state_change = smap_state_change;
 	psock->strp_enabled = true;
 }
 
@@ -517,6 +520,10 @@ static struct bpf_map *sock_map_alloc(union bpf_attr *attr)
 	if (attr->value_size > KMALLOC_MAX_SIZE)
 		return ERR_PTR(-E2BIG);
 
+	err = bpf_tcp_ulp_register();
+	if (err && err != -EEXIST)
+		return ERR_PTR(err);
+
 	stab = kzalloc(sizeof(*stab), GFP_USER);
 	if (!stab)
 		return ERR_PTR(-ENOMEM);
@@ -768,6 +775,10 @@ static int sock_map_ctx_update_elem(struct bpf_sock_ops_kern *skops,
 			goto out_progs;
 		}
 
+		err = tcp_set_ulp_id(sock, TCP_ULP_BPF);
+		if (err)
+			goto out_progs;
+
 		set_bit(SMAP_TX_RUNNING, &psock->state);
 	}
 

[bpf PATCH 3/3] bpf: sockmap, fix leaking maps with attached but not detached progs

[John Fastabend]

When a program is attached to a map we increment the program refcnt
to ensure that the program is not removed while it is potentially
being referenced from sockmap side. However, if this same program
also references the map (this is a reasonably common pattern in
my programs) then the verifier will also increment the maps refcnt
from the verifier. This is to ensure the map doesn't get garbage
collected while the program has a reference to it.

So we are left in a state where the map holds the refcnt on the
program stopping it from being removed and releasing the map refcnt.
And vice versa the program holds a refcnt on the map stopping it
from releasing the refcnt on the prog.

All this is fine as long as users detach the program while the
map fd is still around. But, if the user omits this detach command
we are left with a dangling map we can no longer release.

To resolve this when the map fd is released decrement the program
references and remove any reference from the map to the program.
This fixes the issue with possibly dangling map and creates a
user side API constraint. That is, the map fd must be held open
for programs to be attached to a map.

Fixes: 174a79ff9515 ("bpf: sockmap with sk redirect support")
Signed-off-by: John Fastabend <john.fastabend@gmail.com>
---
 kernel/bpf/sockmap.c |   19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/kernel/bpf/sockmap.c b/kernel/bpf/sockmap.c
index b85a79c..a8cd1b7 100644
--- a/kernel/bpf/sockmap.c
+++ b/kernel/bpf/sockmap.c
@@ -611,11 +611,6 @@ static void sock_map_free(struct bpf_map *map)
 	}
 	rcu_read_unlock();
 
-	if (stab->bpf_verdict)
-		bpf_prog_put(stab->bpf_verdict);
-	if (stab->bpf_parse)
-		bpf_prog_put(stab->bpf_parse);
-
 	sock_map_remove_complete(stab);
 }
 
@@ -891,6 +886,19 @@ static int sock_map_update_elem(struct bpf_map *map,
 	return err;
 }
 
+static void sock_map_release(struct bpf_map *map, struct file *map_file)
+{
+	struct bpf_stab *stab = container_of(map, struct bpf_stab, map);
+	struct bpf_prog *orig;
+
+	orig = xchg(&stab->bpf_parse, NULL);
+	if (orig)
+		bpf_prog_put(orig);
+	orig = xchg(&stab->bpf_verdict, NULL);
+	if (orig)
+		bpf_prog_put(orig);
+}
+
 const struct bpf_map_ops sock_map_ops = {
 	.map_alloc = sock_map_alloc,
 	.map_free = sock_map_free,
@@ -898,6 +906,7 @@ static int sock_map_update_elem(struct bpf_map *map,
 	.map_get_next_key = sock_map_get_next_key,
 	.map_update_elem = sock_map_update_elem,
 	.map_delete_elem = sock_map_delete_elem,
+	.map_release = sock_map_release,
 };
 
 BPF_CALL_4(bpf_sock_map_update, struct bpf_sock_ops_kern *, bpf_sock,

Re: [bpf PATCH 0/3] bpf sockmap fixes

[John Fastabend]

On 01/25/2018 04:26 PM, John Fastabend wrote:
> A set of fixes for sockmap to resolve map/prog not being cleaned
> up when maps are not cleaned up from the program side.
> 

Well that first sentence is a bit confusing now that I read it again.
Here is a better version,

"
A set of fixes for sockmap to resolve programs referencing sockmaps
and closing without deleting all entries in the map and/or not detaching
BPF programs attached to the map. Both leaving entries in the map and
not detaching programs may result in the map failing to be removed by
BPF infrastructure due to reference counts never reaching zero.
"


> For this we pull in the ULP infrastructure to hook into the close()
> hook of the sock layer. This seemed natural because we have additional
> sockmap features (to add support for TX hooks) that will also use the
> ULP infrastructure. This allows us to cleanup entries in the map when
> socks are closed() and avoid trying to get the sk_state_change() hook
> to fire in all cases.
> 
> The second issue resolved here occurs when users don't detach
> programs. The gist is a refcnt issue resolved by implementing the
> release callback. See patch for details.
> 
> For testing I ran both sample/sockmap and selftests bpf/test_maps.c.
> 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.
> 
> ---
> 
> John Fastabend (3):
>       net: add a UID to use for ULP socket assignment
>       bpf: sockmap, add sock close() hook to remove socks
>       bpf: sockmap, fix leaking maps with attached but not detached progs
> 
> 
>  include/net/tcp.h    |    8 ++
>  kernel/bpf/sockmap.c |  164 ++++++++++++++++++++++++++++----------------------
>  net/ipv4/tcp_ulp.c   |   53 +++++++++++++++-
>  net/tls/tls_main.c   |    2 +
>  4 files changed, 150 insertions(+), 77 deletions(-)
> 
> --
> Signature
> 

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

[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 <davejwatson@fb.com>

> Signed-off-by: John Fastabend <john.fastabend@gmail.com>
> ---
>  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;
> +}

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

[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 <davejwatson@fb.com>
> 
>> Signed-off-by: John Fastabend <john.fastabend@gmail.com>
>> ---
>>  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;
>> +}