[MPTCP] [PATCH] mptcp: remove token_release

[MPTCP] [PATCH] mptcp: remove token_release

[Florian Westphal]

[-- Attachment #1: Type: text/plain, Size: 5673 bytes --]

This patch removes token_release and gets rid of the extra sock_put/hold
calls in token.c.

Instead its the responsibility of the subflow to make sure that the master
mptcp socket (referenced via subflow_ctx->conn) can't go away and is
released once the subflow is being destroyed.

Also, switch mptcp_shutdown to sk_common_release to plug a reference leak,
this part is taken from Paolos early patch
"mptcp: fix mptcp socket cleanup", this proposal is an alternative.

Token destruction is moved to mptcp_close -- the rationale is that
we want to remove the token as soon as we know we can't accept any more
subflows.

NB: I suspect we eventually need to extend join operations to check
that the mptcp socket obtained from looking up has following properties:
 - nonzero refcount (refcount_inc_not_zero instead of sock_hold)
 - is still in established state.

 But at this time I feel its better to wait until we support DATA_FIN
 before worrying about this.

Signed-off-by: Paolo Abeni <pabeni(a)redhat.com>
Signed-off-by: Florian Westphal <fw(a)strlen.de>
---
 net/mptcp/protocol.c |  9 ++-------
 net/mptcp/protocol.h |  1 -
 net/mptcp/subflow.c  |  6 +++---
 net/mptcp/token.c    | 30 ++----------------------------
 4 files changed, 7 insertions(+), 39 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 266a1028c9fd..d5b0c801e78f 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -633,6 +633,7 @@ static void mptcp_close(struct sock *sk, long timeout)
 	struct subflow_context *subflow, *tmp;
 	struct socket *ssk = NULL;
 
+	mptcp_token_destroy(msk->token);
 	inet_sk_state_store(sk, TCP_CLOSE);
 
 	lock_sock(sk);
@@ -652,9 +653,7 @@ static void mptcp_close(struct sock *sk, long timeout)
 		sock_release(mptcp_subflow_tcp_socket(subflow));
 	}
 	release_sock(sk);
-
-	sock_orphan(sk);
-	sock_put(sk);
+	sk_common_release(sk);
 }
 
 static struct sock *mptcp_accept(struct sock *sk, int flags, int *err,
@@ -729,11 +728,7 @@ static struct sock *mptcp_accept(struct sock *sk, int flags, int *err,
 
 static void mptcp_destroy(struct sock *sk)
 {
-	struct mptcp_sock *msk = mptcp_sk(sk);
 
-	pr_debug("msk=%p", sk);
-
-	mptcp_token_destroy(msk->token);
 }
 
 static int mptcp_setsockopt(struct sock *sk, int level, int optname,
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 49ac7c842a3c..38d3f5bdf926 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -235,7 +235,6 @@ int mptcp_token_new_connect(struct sock *sk);
 int mptcp_token_new_accept(u32 token);
 void mptcp_token_update_accept(struct sock *sk, struct sock *conn);
 struct mptcp_sock *mptcp_token_get_sock(u32 token);
-void token_release(u32 token);
 void mptcp_token_destroy(u32 token);
 
 void crypto_key_sha1(u64 key, u32 *token, u64 *idsn);
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 468f9b2aeb30..a49cc5ed1b27 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -390,6 +390,7 @@ int subflow_create_socket(struct sock *sk, struct socket **new_sock)
 	pr_debug("subflow=%p", subflow);
 
 	*new_sock = sf;
+	sock_hold(sk);
 	subflow->conn = sk;
 
 	return 0;
@@ -444,9 +445,8 @@ static void subflow_ulp_release(struct sock *sk)
 {
 	struct subflow_context *ctx = subflow_ctx(sk);
 
-	pr_debug("subflow=%p", ctx);
-
-	token_release(ctx->token);
+	if (ctx->conn)
+		sock_put(ctx->conn);
 
 	kfree(ctx);
 }
diff --git a/net/mptcp/token.c b/net/mptcp/token.c
index 5bd3d0c78d04..f493ebb4c2d8 100644
--- a/net/mptcp/token.c
+++ b/net/mptcp/token.c
@@ -38,11 +38,6 @@ static RADIX_TREE(token_req_tree, GFP_ATOMIC);
 static DEFINE_SPINLOCK(token_tree_lock);
 static int token_used __read_mostly;
 
-static struct sock *__token_lookup(u32 token)
-{
-	return radix_tree_lookup(&token_tree, token);
-}
-
 /**
  * mptcp_token_new_request - create new key/idsn/token for subflow_request
  * @req - the request socket
@@ -126,8 +121,6 @@ int mptcp_token_new_connect(struct sock *sk)
 		spin_unlock_bh(&token_tree_lock);
 	}
 	err = radix_tree_insert(&token_tree, subflow->token, mptcp_sock);
-	if (err == 0)
-		sock_hold(mptcp_sock);
 	spin_unlock_bh(&token_tree_lock);
 
 	return err;
@@ -174,7 +167,6 @@ void mptcp_token_update_accept(struct sock *sk, struct sock *conn)
 	if (slot) {
 		WARN_ON_ONCE(*slot != &token_used);
 		*slot = conn;
-		sock_hold(conn);
 	}
 	spin_unlock_bh(&token_tree_lock);
 }
@@ -193,7 +185,7 @@ struct mptcp_sock *mptcp_token_get_sock(u32 token)
 	struct sock *conn;
 
 	spin_lock_bh(&token_tree_lock);
-	conn = __token_lookup(token);
+	conn = radix_tree_lookup(&token_tree, token);
 	if (conn)
 		sock_hold(conn);
 	spin_unlock_bh(&token_tree_lock);
@@ -215,33 +207,15 @@ void mptcp_token_destroy_request(u32 token)
 	spin_unlock_bh(&token_tree_lock);
 }
 
-void token_release(u32 token)
-{
-	struct sock *conn;
-
-	pr_debug("token=%u", token);
-	spin_lock_bh(&token_tree_lock);
-	conn = __token_lookup(token);
-	if (conn)
-		sock_put(conn);
-	spin_unlock_bh(&token_tree_lock);
-}
-
 /**
  * mptcp_token_destroy - remove mptcp connection/token
  * @token - token of mptcp connection to remove
  *
  * Remove the connection identified by @token.
- * If the connection isn't found, no action is taken.
  */
 void mptcp_token_destroy(u32 token)
 {
-	struct sock *conn;
-
 	spin_lock_bh(&token_tree_lock);
-	conn = radix_tree_delete(&token_tree, token);
+	radix_tree_delete(&token_tree, token);
 	spin_unlock_bh(&token_tree_lock);
-
-	if (conn && (int *)conn != &token_used)
-		sock_put(conn);
 }
-- 
2.21.0

Re: [MPTCP] [PATCH] mptcp: remove token_release

[Matthieu Baerts]

[-- Attachment #1: Type: text/plain, Size: 1613 bytes --]

Hi Florian,

On 13/09/2019 17:38, Florian Westphal wrote:
> Matthieu Baerts <matthieu.baerts(a)tessares.net> wrote:
>> Do you want me to add it at the end of squash in another commit?
>> I was going to add it at the end but the commit message seems to show
>> that it should be squashed in another commit.
>>
>> Paolo proposed cd2d4eeca99b ("mptcp: Add key generation and token tree")
>> in his initial commit. Should I do that too?
> 
> I would be fine with squashing but I'd wager it adds some extra work due
> to the refactoring.
> 
> So, handle it as you deem best.

Indeed, the refactoring at the end is not helpful if we want to continue
squashing thing. I also saw that the commit introducing the Token API
("mptcp: Add key generation and token tree") is calling functions that
are not available. So I decided to include all changes related to how
the Token API is used in the original commit and the rest in the
refactoring. I hope it is fine like that!

- 5ee88b657e3f: "squashed", part 1 in "mptcp: Add key generation and
token tree"
- 8e2060c69d83: conflict in t/mptcp-token-add-mptcp-prefixes-to-functions
- 41af4de887d8: "squashed", part 2 in "mptcp: Add handling of incoming
MP_JOIN requests"
- 27740d83cacb: "squashed", part 3 in "mptcp: token: add mptcp prefixes
to functions"

Tests are still OK!

Next time I will apply patches quicker! I hope I didn't block you!

Cheers,
Matt
-- 
Matthieu Baerts | R&D Engineer
matthieu.baerts(a)tessares.net
Tessares SA | Hybrid Access Solutions
www.tessares.net
1 Avenue Jean Monnet, 1348 Louvain-la-Neuve, Belgium

Re: [MPTCP] [PATCH] mptcp: remove token_release

[Florian Westphal]

[-- Attachment #1: Type: text/plain, Size: 507 bytes --]

Matthieu Baerts <matthieu.baerts(a)tessares.net> wrote:
> Do you want me to add it at the end of squash in another commit?
> I was going to add it at the end but the commit message seems to show
> that it should be squashed in another commit.
> 
> Paolo proposed cd2d4eeca99b ("mptcp: Add key generation and token tree")
> in his initial commit. Should I do that too?

I would be fine with squashing but I'd wager it adds some extra work due
to the refactoring.

So, handle it as you deem best.

Re: [MPTCP] [PATCH] mptcp: remove token_release

[Matthieu Baerts]

[-- Attachment #1: Type: text/plain, Size: 1724 bytes --]

Hi Florian,

Sorry for the delay.

On 10/09/2019 17:53, Florian Westphal wrote:
> This patch removes token_release and gets rid of the extra sock_put/hold
> calls in token.c.
> 
> Instead its the responsibility of the subflow to make sure that the master
> mptcp socket (referenced via subflow_ctx->conn) can't go away and is
> released once the subflow is being destroyed.
> 
> Also, switch mptcp_shutdown to sk_common_release to plug a reference leak,
> this part is taken from Paolos early patch
> "mptcp: fix mptcp socket cleanup", this proposal is an alternative.
> 
> Token destruction is moved to mptcp_close -- the rationale is that
> we want to remove the token as soon as we know we can't accept any more
> subflows.
> 
> NB: I suspect we eventually need to extend join operations to check
> that the mptcp socket obtained from looking up has following properties:
>  - nonzero refcount (refcount_inc_not_zero instead of sock_hold)
>  - is still in established state.
> 
>  But at this time I feel its better to wait until we support DATA_FIN
>  before worrying about this.
> 
> Signed-off-by: Paolo Abeni <pabeni(a)redhat.com>
> Signed-off-by: Florian Westphal <fw(a)strlen.de>

Do you want me to add it at the end of squash in another commit?
I was going to add it at the end but the commit message seems to show
that it should be squashed in another commit.

Paolo proposed cd2d4eeca99b ("mptcp: Add key generation and token tree")
in his initial commit. Should I do that too?

Cheers,
Matt
-- 
Matthieu Baerts | R&D Engineer
matthieu.baerts(a)tessares.net
Tessares SA | Hybrid Access Solutions
www.tessares.net
1 Avenue Jean Monnet, 1348 Louvain-la-Neuve, Belgium

Re: [MPTCP] [PATCH] mptcp: remove token_release

[Paolo Abeni]

[-- Attachment #1: Type: text/plain, Size: 6162 bytes --]

On Tue, 2019-09-10 at 17:53 +0200, Florian Westphal wrote:
> This patch removes token_release and gets rid of the extra sock_put/hold
> calls in token.c.
> 
> Instead its the responsibility of the subflow to make sure that the master
> mptcp socket (referenced via subflow_ctx->conn) can't go away and is
> released once the subflow is being destroyed.
> 
> Also, switch mptcp_shutdown to sk_common_release to plug a reference leak,
> this part is taken from Paolos early patch
> "mptcp: fix mptcp socket cleanup", this proposal is an alternative.
> 
> Token destruction is moved to mptcp_close -- the rationale is that
> we want to remove the token as soon as we know we can't accept any more
> subflows.
> 
> NB: I suspect we eventually need to extend join operations to check
> that the mptcp socket obtained from looking up has following properties:
>  - nonzero refcount (refcount_inc_not_zero instead of sock_hold)
>  - is still in established state.
> 
>  But at this time I feel its better to wait until we support DATA_FIN
>  before worrying about this.
> 
> Signed-off-by: Paolo Abeni <pabeni(a)redhat.com>
> Signed-off-by: Florian Westphal <fw(a)strlen.de>
> ---
>  net/mptcp/protocol.c |  9 ++-------
>  net/mptcp/protocol.h |  1 -
>  net/mptcp/subflow.c  |  6 +++---
>  net/mptcp/token.c    | 30 ++----------------------------
>  4 files changed, 7 insertions(+), 39 deletions(-)
> 
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 266a1028c9fd..d5b0c801e78f 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -633,6 +633,7 @@ static void mptcp_close(struct sock *sk, long timeout)
>  	struct subflow_context *subflow, *tmp;
>  	struct socket *ssk = NULL;
>  
> +	mptcp_token_destroy(msk->token);
>  	inet_sk_state_store(sk, TCP_CLOSE);
>  
>  	lock_sock(sk);
> @@ -652,9 +653,7 @@ static void mptcp_close(struct sock *sk, long timeout)
>  		sock_release(mptcp_subflow_tcp_socket(subflow));
>  	}
>  	release_sock(sk);
> -
> -	sock_orphan(sk);
> -	sock_put(sk);
> +	sk_common_release(sk);
>  }
>  
>  static struct sock *mptcp_accept(struct sock *sk, int flags, int *err,
> @@ -729,11 +728,7 @@ static struct sock *mptcp_accept(struct sock *sk, int flags, int *err,
>  
>  static void mptcp_destroy(struct sock *sk)
>  {
> -	struct mptcp_sock *msk = mptcp_sk(sk);
>  
> -	pr_debug("msk=%p", sk);
> -
> -	mptcp_token_destroy(msk->token);
>  }
>  
>  static int mptcp_setsockopt(struct sock *sk, int level, int optname,
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index 49ac7c842a3c..38d3f5bdf926 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -235,7 +235,6 @@ int mptcp_token_new_connect(struct sock *sk);
>  int mptcp_token_new_accept(u32 token);
>  void mptcp_token_update_accept(struct sock *sk, struct sock *conn);
>  struct mptcp_sock *mptcp_token_get_sock(u32 token);
> -void token_release(u32 token);
>  void mptcp_token_destroy(u32 token);
>  
>  void crypto_key_sha1(u64 key, u32 *token, u64 *idsn);
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 468f9b2aeb30..a49cc5ed1b27 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -390,6 +390,7 @@ int subflow_create_socket(struct sock *sk, struct socket **new_sock)
>  	pr_debug("subflow=%p", subflow);
>  
>  	*new_sock = sf;
> +	sock_hold(sk);
>  	subflow->conn = sk;
>  
>  	return 0;
> @@ -444,9 +445,8 @@ static void subflow_ulp_release(struct sock *sk)
>  {
>  	struct subflow_context *ctx = subflow_ctx(sk);
>  
> -	pr_debug("subflow=%p", ctx);
> -
> -	token_release(ctx->token);
> +	if (ctx->conn)
> +		sock_put(ctx->conn);
>  
>  	kfree(ctx);
>  }
> diff --git a/net/mptcp/token.c b/net/mptcp/token.c
> index 5bd3d0c78d04..f493ebb4c2d8 100644
> --- a/net/mptcp/token.c
> +++ b/net/mptcp/token.c
> @@ -38,11 +38,6 @@ static RADIX_TREE(token_req_tree, GFP_ATOMIC);
>  static DEFINE_SPINLOCK(token_tree_lock);
>  static int token_used __read_mostly;
>  
> -static struct sock *__token_lookup(u32 token)
> -{
> -	return radix_tree_lookup(&token_tree, token);
> -}
> -
>  /**
>   * mptcp_token_new_request - create new key/idsn/token for subflow_request
>   * @req - the request socket
> @@ -126,8 +121,6 @@ int mptcp_token_new_connect(struct sock *sk)
>  		spin_unlock_bh(&token_tree_lock);
>  	}
>  	err = radix_tree_insert(&token_tree, subflow->token, mptcp_sock);
> -	if (err == 0)
> -		sock_hold(mptcp_sock);
>  	spin_unlock_bh(&token_tree_lock);
>  
>  	return err;
> @@ -174,7 +167,6 @@ void mptcp_token_update_accept(struct sock *sk, struct sock *conn)
>  	if (slot) {
>  		WARN_ON_ONCE(*slot != &token_used);
>  		*slot = conn;
> -		sock_hold(conn);
>  	}
>  	spin_unlock_bh(&token_tree_lock);
>  }
> @@ -193,7 +185,7 @@ struct mptcp_sock *mptcp_token_get_sock(u32 token)
>  	struct sock *conn;
>  
>  	spin_lock_bh(&token_tree_lock);
> -	conn = __token_lookup(token);
> +	conn = radix_tree_lookup(&token_tree, token);
>  	if (conn)
>  		sock_hold(conn);
>  	spin_unlock_bh(&token_tree_lock);
> @@ -215,33 +207,15 @@ void mptcp_token_destroy_request(u32 token)
>  	spin_unlock_bh(&token_tree_lock);
>  }
>  
> -void token_release(u32 token)
> -{
> -	struct sock *conn;
> -
> -	pr_debug("token=%u", token);
> -	spin_lock_bh(&token_tree_lock);
> -	conn = __token_lookup(token);
> -	if (conn)
> -		sock_put(conn);
> -	spin_unlock_bh(&token_tree_lock);
> -}
> -
>  /**
>   * mptcp_token_destroy - remove mptcp connection/token
>   * @token - token of mptcp connection to remove
>   *
>   * Remove the connection identified by @token.
> - * If the connection isn't found, no action is taken.
>   */
>  void mptcp_token_destroy(u32 token)
>  {
> -	struct sock *conn;
> -
>  	spin_lock_bh(&token_tree_lock);
> -	conn = radix_tree_delete(&token_tree, token);
> +	radix_tree_delete(&token_tree, token);
>  	spin_unlock_bh(&token_tree_lock);
> -
> -	if (conn && (int *)conn != &token_used)
> -		sock_put(conn);
>  }

LGTM!

Also tested successfully with retransmit infrastructure on top.

/P