Re: [MPTCP] [PATCH] mptcp: fix mptcp socket cleanup

Re: [MPTCP] [PATCH] mptcp: fix mptcp socket cleanup

[Florian Westphal]

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

Paolo Abeni <pabeni(a)redhat.com> wrote:
> +		/* when assiging the token to the msk, we need to held
> +		 * an additional reference, will be released at msk token
> +		 * destruction time
> +		 */

Unrelated to your patch, but I think this is wrong.
We should have implicit reference for the token.

Taking a reference makes no sense because the token lifetime is limited by
the mptcp socket -- if the mptcp socket is closing, we remove the token too.

token_release() function makes no sense to me -- afaics all callers
have a pointer to the msk they want to put the reference for.

I'll have a look into cleaning that up after respinning a squashable
version of the token refactoring.

[MPTCP] [PATCH] mptcp: fix mptcp socket cleanup

[Paolo Abeni]

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

Currently the mptcp proto ->destruct() is never called.
With token accounting this causes a msk refcount leak.
Kmemleak can't detect that, due to msk <-> sk circular
pointer references.

Invoke the distructor via sk_common_release(), and reduce a
bit code duplication.

Additionally we need to acquire a msk refcount when we initialize
the msk token in mptcp_finish_connect(): not the destructor
is correctly called and the client sock will release the msk a
reference to self at close() time.

Fixes: cd2d4eeca99b ("ptcp: Add key generation and token tree")
Signed-off-by: Paolo Abeni <pabeni(a)redhat.com>
--
I'm ok with squashing this one in the fixes commit
---
 net/mptcp/protocol.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 52376b1fe64b..c862630897a6 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -653,8 +653,7 @@ static void mptcp_close(struct sock *sk, long timeout)
 	}
 	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,
@@ -830,9 +829,14 @@ void mptcp_finish_connect(struct sock *sk, int mp_capable)
 		local_bh_disable();
 		bh_lock_sock_nested(sk);
 
+		/* when assiging the token to the msk, we need to held
+		 * an additional reference, will be released at msk token
+		 * destruction time
+		 */
 		msk->remote_key = subflow->remote_key;
 		msk->local_key = subflow->local_key;
 		msk->token = subflow->token;
+		sock_hold(sk);
 		pr_debug("msk=%p, token=%u", msk, msk->token);
 		msk->dport = ntohs(inet_sk(msk->subflow->sk)->inet_dport);
 
-- 
2.21.0