[PATCH net v4 0/3] net: mptcp: fix unreleased socket in accept queue

[PATCH net v4 0/3] net: mptcp: fix unreleased socket in accept queue

[menglong8.dong]

From: Menglong Dong <imagedong@tencent.com>

The mptcp socket and its subflow sockets in accept queue can't be
released after the process exit.

While the release of a mptcp socket in listening state, the
corresponding tcp socket will be released too. Meanwhile, the tcp
socket in the unaccept queue will be released too. However, only init
subflow is in the unaccept queue, and the joined subflow is not in the
unaccept queue, which makes the joined subflow won't be released, and
therefore the corresponding unaccepted mptcp socket will not be released
to.

In the 1th patch, we factor-out __mptcp_close() which should run under
socket lock.

In the 2th patch, we move mptcp_cancel_work() to protocol.h and make it
as an inline, as we need to use it in subflow.c.

And in the 3th patch, we fix the problem we mentioned above by closing
all of the unaccepted mptcp socket in mptcp_subflow_queue_clean()
with __mptcp_close().

Changes since v3:
- factor out __mptcp_close() and replace mptcp_close() with it in
  mptcp_subflow_queue_clean()

Menglong Dong (3):
  net: mptcp: factor out __mptcp_close() without socket lock
  net: mptcp: move mptcp_cancel_work() to protocol.h
  net: mptcp: fix unreleased socket in accept queue

 net/mptcp/protocol.c | 27 ++++++++++++++-------------
 net/mptcp/protocol.h |  9 +++++++++
 net/mptcp/subflow.c  | 37 +++++++++++--------------------------
 3 files changed, 34 insertions(+), 39 deletions(-)

-- 
2.37.2

[PATCH net v4 1/3] net: mptcp: factor out __mptcp_close() without socket lock

[menglong8.dong]

From: Menglong Dong <imagedong@tencent.com>

Factor out __mptcp_close() from mptcp_close(). The caller of
__mptcp_close() should hold the socket lock, and cancel mptcp work when
__mptcp_close() return true.

This function will be used in the next commit.

Reviewed-by: Jiang Biao <benbjiang@tencent.com>
Reviewed-by: Mengen Sun <mengensun@tencent.com>
Signed-off-by: Menglong Dong <imagedong@tencent.com>
---
 net/mptcp/protocol.c | 19 ++++++++++++++-----
 net/mptcp/protocol.h |  1 +
 2 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index d398f3810662..1cfb4f1ff4e4 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -2796,13 +2796,12 @@ static void __mptcp_destroy_sock(struct sock *sk)
 	sock_put(sk);
 }
 
-static void mptcp_close(struct sock *sk, long timeout)
+bool __mptcp_close(struct sock *sk, long timeout)
 {
 	struct mptcp_subflow_context *subflow;
 	struct mptcp_sock *msk = mptcp_sk(sk);
 	bool do_cancel_work = false;
 
-	lock_sock(sk);
 	sk->sk_shutdown = SHUTDOWN_MASK;
 
 	if ((1 << sk->sk_state) & (TCPF_LISTEN | TCPF_CLOSE)) {
@@ -2833,7 +2832,6 @@ static void mptcp_close(struct sock *sk, long timeout)
 	}
 	sock_orphan(sk);
 
-	sock_hold(sk);
 	pr_debug("msk=%p state=%d", sk, sk->sk_state);
 	if (mptcp_sk(sk)->token)
 		mptcp_event(MPTCP_EVENT_CLOSED, msk, NULL, GFP_KERNEL);
@@ -2844,10 +2842,21 @@ static void mptcp_close(struct sock *sk, long timeout)
 	} else {
 		mptcp_reset_timeout(msk, 0);
 	}
+
+	return do_cancel_work;
+}
+
+static void mptcp_close(struct sock *sk, long timeout)
+{
+	bool cancel_work;
+
+	sock_hold(sk);
+	lock_sock(sk);
+
+	cancel_work = __mptcp_close(sk, timeout);
 	release_sock(sk);
-	if (do_cancel_work)
+	if (cancel_work)
 		mptcp_cancel_work(sk);
-
 	sock_put(sk);
 }
 
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 132d50833df1..8f123d450c76 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -612,6 +612,7 @@ void mptcp_subflow_reset(struct sock *ssk);
 void mptcp_subflow_queue_clean(struct sock *ssk);
 void mptcp_sock_graft(struct sock *sk, struct socket *parent);
 struct socket *__mptcp_nmpc_socket(const struct mptcp_sock *msk);
+bool __mptcp_close(struct sock *sk, long timeout);
 
 bool mptcp_addresses_equal(const struct mptcp_addr_info *a,
 			   const struct mptcp_addr_info *b, bool use_port);
-- 
2.37.2

[PATCH net v4 2/3] net: mptcp: move mptcp_cancel_work() to protocol.h

[menglong8.dong]

From: Menglong Dong <imagedong@tencent.com>

mptcp_cancel_work() is simple enough and we can make it as inline and
move it to protocol.h. mptcp_cancel_work() will be used in subflow.c
in the next commit.

Reviewed-by: Jiang Biao <benbjiang@tencent.com>
Reviewed-by: Mengen Sun <mengensun@tencent.com>
Signed-off-by: Menglong Dong <imagedong@tencent.com>
---
 net/mptcp/protocol.c | 8 --------
 net/mptcp/protocol.h | 8 ++++++++
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 1cfb4f1ff4e4..d6eb033e93b9 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -2656,14 +2656,6 @@ static void __mptcp_clear_xmit(struct sock *sk)
 		dfrag_clear(sk, dfrag);
 }
 
-static void mptcp_cancel_work(struct sock *sk)
-{
-	struct mptcp_sock *msk = mptcp_sk(sk);
-
-	if (cancel_work_sync(&msk->work))
-		__sock_put(sk);
-}
-
 void mptcp_subflow_shutdown(struct sock *sk, struct sock *ssk, int how)
 {
 	lock_sock(ssk);
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 8f123d450c76..9de891373959 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -378,6 +378,14 @@ static inline struct mptcp_data_frag *mptcp_rtx_head(const struct sock *sk)
 	return list_first_entry_or_null(&msk->rtx_queue, struct mptcp_data_frag, list);
 }
 
+static inline void mptcp_cancel_work(struct sock *sk)
+{
+	struct mptcp_sock *msk = mptcp_sk(sk);
+
+	if (cancel_work_sync(&msk->work))
+		__sock_put(sk);
+}
+
 struct csum_pseudo_header {
 	__be64 data_seq;
 	__be32 subflow_seq;
-- 
2.37.2

[PATCH net v4 3/3] net: mptcp: fix unreleased socket in accept queue

[menglong8.dong]

From: Menglong Dong <imagedong@tencent.com>

The mptcp socket and its subflow sockets in accept queue can't be
released after the process exit.

While the release of a mptcp socket in listening state, the
corresponding tcp socket will be released too. Meanwhile, the tcp
socket in the unaccept queue will be released too. However, only init
subflow is in the unaccept queue, and the joined subflow is not in the
unaccept queue, which makes the joined subflow won't be released, and
therefore the corresponding unaccepted mptcp socket will not be released
to.

This can be reproduced easily with following steps:

1. create 2 namespace and veth:
   $ ip netns add mptcp-client
   $ ip netns add mptcp-server
   $ sysctl -w net.ipv4.conf.all.rp_filter=0
   $ ip netns exec mptcp-client sysctl -w net.mptcp.enabled=1
   $ ip netns exec mptcp-server sysctl -w net.mptcp.enabled=1
   $ ip link add red-client netns mptcp-client type veth peer red-server \
     netns mptcp-server
   $ ip -n mptcp-server address add 10.0.0.1/24 dev red-server
   $ ip -n mptcp-server address add 192.168.0.1/24 dev red-server
   $ ip -n mptcp-client address add 10.0.0.2/24 dev red-client
   $ ip -n mptcp-client address add 192.168.0.2/24 dev red-client
   $ ip -n mptcp-server link set red-server up
   $ ip -n mptcp-client link set red-client up

2. configure the endpoint and limit for client and server:
   $ ip -n mptcp-server mptcp endpoint flush
   $ ip -n mptcp-server mptcp limits set subflow 2 add_addr_accepted 2
   $ ip -n mptcp-client mptcp endpoint flush
   $ ip -n mptcp-client mptcp limits set subflow 2 add_addr_accepted 2
   $ ip -n mptcp-client mptcp endpoint add 192.168.0.2 dev red-client id \
     1 subflow

3. listen and accept on a port, such as 9999. The nc command we used
   here is modified, which makes it use mptcp protocol by default.
   $ ip netns exec mptcp-server nc -l -k -p 9999

4. open another *two* terminal and use each of them to connect to the
   server with the following command:
   $ ip netns exec mptcp-client nc 10.0.0.1 9999
   Input something after connect to triger the connection of the second
   subflow. So that there are two established mptcp connections, with the
   second one still unaccepted.

5. exit all the nc command, and check the tcp socket in server namespace.
   And you will find that there is one tcp socket in CLOSE_WAIT state
   and can't release forever.

Fix this by closing all of the unaccepted mptcp socket in
mptcp_subflow_queue_clean() with __mptcp_close().

Now, we can ensure that all unaccepted mptcp sockets will be cleaned by
__mptcp_close() before they are released, so mptcp_sock_destruct(), which
is used to clean the unaccepted mptcp socket, is not needed anymore.

The selftests for mptcp is ran for this commit, and no new failures.

Fixes: f296234c98a8 ("mptcp: Add handling of incoming MP_JOIN requests")
Reviewed-by: Jiang Biao <benbjiang@tencent.com>
Reviewed-by: Mengen Sun <mengensun@tencent.com>
Signed-off-by: Menglong Dong <imagedong@tencent.com>
---
v4:
- factor-out __mptcp_close() and use it in this commit.

v3:
- remove mptcp_close_nolock() and call mptcp_close() directly in
  mptcp_subflow_queue_clean(), as mptcp_close_nolock() will cause
  dead lock.

v2:
- remove mptcp_sock_destruct()
- introduce mptcp_close_nolock() and replace mptcp_close() with it in
  mptcp_subflow_queue_clean()
---
 net/mptcp/subflow.c | 37 +++++++++++--------------------------
 1 file changed, 11 insertions(+), 26 deletions(-)

diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index c7d49fb6e7bd..7c437debc368 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -602,30 +602,6 @@ static bool subflow_hmac_valid(const struct request_sock *req,
 	return !crypto_memneq(hmac, mp_opt->hmac, MPTCPOPT_HMAC_LEN);
 }
 
-static void mptcp_sock_destruct(struct sock *sk)
-{
-	/* if new mptcp socket isn't accepted, it is free'd
-	 * from the tcp listener sockets request queue, linked
-	 * from req->sk.  The tcp socket is released.
-	 * This calls the ULP release function which will
-	 * also remove the mptcp socket, via
-	 * sock_put(ctx->conn).
-	 *
-	 * Problem is that the mptcp socket will be in
-	 * ESTABLISHED state and will not have the SOCK_DEAD flag.
-	 * Both result in warnings from inet_sock_destruct.
-	 */
-	if ((1 << sk->sk_state) & (TCPF_ESTABLISHED | TCPF_CLOSE_WAIT)) {
-		sk->sk_state = TCP_CLOSE;
-		WARN_ON_ONCE(sk->sk_socket);
-		sock_orphan(sk);
-	}
-
-	/* We don't need to clear msk->subflow, as it's still NULL at this point */
-	mptcp_destroy_common(mptcp_sk(sk), 0);
-	inet_sock_destruct(sk);
-}
-
 static void mptcp_force_close(struct sock *sk)
 {
 	/* the msk is not yet exposed to user-space */
@@ -768,7 +744,6 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
 			/* new mpc subflow takes ownership of the newly
 			 * created mptcp socket
 			 */
-			new_msk->sk_destruct = mptcp_sock_destruct;
 			mptcp_sk(new_msk)->setsockopt_seq = ctx->setsockopt_seq;
 			mptcp_pm_new_connection(mptcp_sk(new_msk), child, 1);
 			mptcp_token_accept(subflow_req, mptcp_sk(new_msk));
@@ -1763,13 +1738,23 @@ void mptcp_subflow_queue_clean(struct sock *listener_ssk)
 
 	for (msk = head; msk; msk = next) {
 		struct sock *sk = (struct sock *)msk;
-		bool slow;
+		bool slow, cancel_work;
 
+		sock_hold(sk);
 		slow = lock_sock_fast_nested(sk);
 		next = msk->dl_next;
 		msk->first = NULL;
 		msk->dl_next = NULL;
+
+		/* __mptcp_close() will put a extra reference on sk,
+		 * so we hold one here.
+		 */
+		sock_hold(sk);
+		cancel_work = __mptcp_close(sk, 0);
 		unlock_sock_fast(sk, slow);
+		if (cancel_work)
+			mptcp_cancel_work(sk);
+		sock_put(sk);
 	}
 
 	/* we are still under the listener msk socket lock */
-- 
2.37.2

Re: [PATCH net v4 1/3] net: mptcp: factor out __mptcp_close() without socket lock

[Paolo Abeni]

Hello,

overall the series LGTM, but there are a few small cosmetic changes I
I'd  like to suggest, see below and the other patches for the details.

On Fri, 2022-09-09 at 13:49 +0800, menglong8.dong@gmail.com wrote:
> From: Menglong Dong <imagedong@tencent.com>
> 
> Factor out __mptcp_close() from mptcp_close(). The caller of
> __mptcp_close() should hold the socket lock, and cancel mptcp work when
> __mptcp_close() return true.
> 
> This function will be used in the next commit.
> 
> Reviewed-by: Jiang Biao <benbjiang@tencent.com>
> Reviewed-by: Mengen Sun <mengensun@tencent.com>
> Signed-off-by: Menglong Dong <imagedong@tencent.com>
> ---
>  net/mptcp/protocol.c | 19 ++++++++++++++-----
>  net/mptcp/protocol.h |  1 +
>  2 files changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index d398f3810662..1cfb4f1ff4e4 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -2796,13 +2796,12 @@ static void __mptcp_destroy_sock(struct sock *sk)
>  	sock_put(sk);
>  }
>  
> -static void mptcp_close(struct sock *sk, long timeout)
> +bool __mptcp_close(struct sock *sk, long timeout)
>  {
>  	struct mptcp_subflow_context *subflow;
>  	struct mptcp_sock *msk = mptcp_sk(sk);
>  	bool do_cancel_work = false;
>  
> -	lock_sock(sk);
>  	sk->sk_shutdown = SHUTDOWN_MASK;
>  
>  	if ((1 << sk->sk_state) & (TCPF_LISTEN | TCPF_CLOSE)) {
> @@ -2833,7 +2832,6 @@ static void mptcp_close(struct sock *sk, long timeout)
>  	}
>  	sock_orphan(sk);
>  
> -	sock_hold(sk);

If you don't remove this line you can avoid adding a sock_hold(sk) in
mptcp_close() and an additional one in patch 3. The diff will be
smaller and the patch clearer.

>  	pr_debug("msk=%p state=%d", sk, sk->sk_state);
>  	if (mptcp_sk(sk)->token)
>  		mptcp_event(MPTCP_EVENT_CLOSED, msk, NULL, GFP_KERNEL);
> @@ -2844,10 +2842,21 @@ static void mptcp_close(struct sock *sk, long timeout)
>  	} else {
>  		mptcp_reset_timeout(msk, 0);
>  	}
> +
> +	return do_cancel_work;
> +}
> +
> +static void mptcp_close(struct sock *sk, long timeout)
> +{
> +	bool cancel_work;

I think this variable name is a bit confusing, as we already have a
global function with the same name. I think 'do_cancel_work' will be
better (and again will produce a smaller diff and a clearer patch).


Thanks,

Paolo

Re: [PATCH net v4 2/3] net: mptcp: move mptcp_cancel_work() to protocol.h

[Paolo Abeni]

On Fri, 2022-09-09 at 13:49 +0800, menglong8.dong@gmail.com wrote:
> From: Menglong Dong <imagedong@tencent.com>
> 
> mptcp_cancel_work() is simple enough and we can make it as inline and
> move it to protocol.h. mptcp_cancel_work() will be used in subflow.c
> in the next commit.
> 
> Reviewed-by: Jiang Biao <benbjiang@tencent.com>
> Reviewed-by: Mengen Sun <mengensun@tencent.com>
> Signed-off-by: Menglong Dong <imagedong@tencent.com>
> ---
>  net/mptcp/protocol.c | 8 --------
>  net/mptcp/protocol.h | 8 ++++++++
>  2 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 1cfb4f1ff4e4..d6eb033e93b9 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -2656,14 +2656,6 @@ static void __mptcp_clear_xmit(struct sock *sk)
>  		dfrag_clear(sk, dfrag);
>  }
>  
> -static void mptcp_cancel_work(struct sock *sk)

I think it's better to just drop the 'static' scope and the function
declaration to the header.

This is not critical/fast path, so it's better to keep the generated
code smaller vs. possibly avoiding a function call due to inlining.

Thanks,

Paolo

Re: [PATCH net v4 3/3] net: mptcp: fix unreleased socket in accept queue

[Paolo Abeni]

On Fri, 2022-09-09 at 13:49 +0800, menglong8.dong@gmail.com wrote:
> From: Menglong Dong <imagedong@tencent.com>
> 
> The mptcp socket and its subflow sockets in accept queue can't be
> released after the process exit.
> 
> While the release of a mptcp socket in listening state, the
> corresponding tcp socket will be released too. Meanwhile, the tcp
> socket in the unaccept queue will be released too. However, only init
> subflow is in the unaccept queue, and the joined subflow is not in the
> unaccept queue, which makes the joined subflow won't be released, and
> therefore the corresponding unaccepted mptcp socket will not be released
> to.
> 
> This can be reproduced easily with following steps:
> 
> 1. create 2 namespace and veth:
>    $ ip netns add mptcp-client
>    $ ip netns add mptcp-server
>    $ sysctl -w net.ipv4.conf.all.rp_filter=0
>    $ ip netns exec mptcp-client sysctl -w net.mptcp.enabled=1
>    $ ip netns exec mptcp-server sysctl -w net.mptcp.enabled=1
>    $ ip link add red-client netns mptcp-client type veth peer red-server \
>      netns mptcp-server
>    $ ip -n mptcp-server address add 10.0.0.1/24 dev red-server
>    $ ip -n mptcp-server address add 192.168.0.1/24 dev red-server
>    $ ip -n mptcp-client address add 10.0.0.2/24 dev red-client
>    $ ip -n mptcp-client address add 192.168.0.2/24 dev red-client
>    $ ip -n mptcp-server link set red-server up
>    $ ip -n mptcp-client link set red-client up
> 
> 2. configure the endpoint and limit for client and server:
>    $ ip -n mptcp-server mptcp endpoint flush
>    $ ip -n mptcp-server mptcp limits set subflow 2 add_addr_accepted 2
>    $ ip -n mptcp-client mptcp endpoint flush
>    $ ip -n mptcp-client mptcp limits set subflow 2 add_addr_accepted 2
>    $ ip -n mptcp-client mptcp endpoint add 192.168.0.2 dev red-client id \
>      1 subflow
> 
> 3. listen and accept on a port, such as 9999. The nc command we used
>    here is modified, which makes it use mptcp protocol by default.
>    $ ip netns exec mptcp-server nc -l -k -p 9999
> 
> 4. open another *two* terminal and use each of them to connect to the
>    server with the following command:
>    $ ip netns exec mptcp-client nc 10.0.0.1 9999
>    Input something after connect to triger the connection of the second
>    subflow. So that there are two established mptcp connections, with the
>    second one still unaccepted.
> 
> 5. exit all the nc command, and check the tcp socket in server namespace.
>    And you will find that there is one tcp socket in CLOSE_WAIT state
>    and can't release forever.
> 
> Fix this by closing all of the unaccepted mptcp socket in
> mptcp_subflow_queue_clean() with __mptcp_close().
> 
> Now, we can ensure that all unaccepted mptcp sockets will be cleaned by
> __mptcp_close() before they are released, so mptcp_sock_destruct(), which
> is used to clean the unaccepted mptcp socket, is not needed anymore.
> 
> The selftests for mptcp is ran for this commit, and no new failures.
> 
> Fixes: f296234c98a8 ("mptcp: Add handling of incoming MP_JOIN requests")
> Reviewed-by: Jiang Biao <benbjiang@tencent.com>
> Reviewed-by: Mengen Sun <mengensun@tencent.com>
> Signed-off-by: Menglong Dong <imagedong@tencent.com>
> ---
> v4:
> - factor-out __mptcp_close() and use it in this commit.
> 
> v3:
> - remove mptcp_close_nolock() and call mptcp_close() directly in
>   mptcp_subflow_queue_clean(), as mptcp_close_nolock() will cause
>   dead lock.
> 
> v2:
> - remove mptcp_sock_destruct()
> - introduce mptcp_close_nolock() and replace mptcp_close() with it in
>   mptcp_subflow_queue_clean()
> ---
>  net/mptcp/subflow.c | 37 +++++++++++--------------------------
>  1 file changed, 11 insertions(+), 26 deletions(-)
> 
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index c7d49fb6e7bd..7c437debc368 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -602,30 +602,6 @@ static bool subflow_hmac_valid(const struct request_sock *req,
>  	return !crypto_memneq(hmac, mp_opt->hmac, MPTCPOPT_HMAC_LEN);
>  }
>  
> -static void mptcp_sock_destruct(struct sock *sk)
> -{
> -	/* if new mptcp socket isn't accepted, it is free'd
> -	 * from the tcp listener sockets request queue, linked
> -	 * from req->sk.  The tcp socket is released.
> -	 * This calls the ULP release function which will
> -	 * also remove the mptcp socket, via
> -	 * sock_put(ctx->conn).
> -	 *
> -	 * Problem is that the mptcp socket will be in
> -	 * ESTABLISHED state and will not have the SOCK_DEAD flag.
> -	 * Both result in warnings from inet_sock_destruct.
> -	 */
> -	if ((1 << sk->sk_state) & (TCPF_ESTABLISHED | TCPF_CLOSE_WAIT)) {
> -		sk->sk_state = TCP_CLOSE;
> -		WARN_ON_ONCE(sk->sk_socket);
> -		sock_orphan(sk);
> -	}
> -
> -	/* We don't need to clear msk->subflow, as it's still NULL at this point */
> -	mptcp_destroy_common(mptcp_sk(sk), 0);
> -	inet_sock_destruct(sk);
> -}
> -
>  static void mptcp_force_close(struct sock *sk)
>  {
>  	/* the msk is not yet exposed to user-space */
> @@ -768,7 +744,6 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
>  			/* new mpc subflow takes ownership of the newly
>  			 * created mptcp socket
>  			 */
> -			new_msk->sk_destruct = mptcp_sock_destruct;
>  			mptcp_sk(new_msk)->setsockopt_seq = ctx->setsockopt_seq;
>  			mptcp_pm_new_connection(mptcp_sk(new_msk), child, 1);
>  			mptcp_token_accept(subflow_req, mptcp_sk(new_msk));
> @@ -1763,13 +1738,23 @@ void mptcp_subflow_queue_clean(struct sock *listener_ssk)
>  
>  	for (msk = head; msk; msk = next) {
>  		struct sock *sk = (struct sock *)msk;
> -		bool slow;
> +		bool slow, cancel_work;

I think is better to rename 'cancel_work' to 'do_cancel_work'

>  
> +		sock_hold(sk);
>  		slow = lock_sock_fast_nested(sk);
>  		next = msk->dl_next;
>  		msk->first = NULL;
>  		msk->dl_next = NULL;
> +
> +		/* __mptcp_close() will put a extra reference on sk,
> +		 * so we hold one here.
> +		 */
> +		sock_hold(sk);
With the suggested changes to patch 1 one of the above 2 sock_hold() 
will not be needed anymore.

Thanks,

Paolo

Re: [PATCH net v4 1/3] net: mptcp: factor out __mptcp_close() without socket lock

[Menglong Dong]

On Fri, Sep 9, 2022 at 3:32 PM Paolo Abeni <pabeni@redhat.com> wrote:
>
> Hello,
>
> overall the series LGTM, but there are a few small cosmetic changes I
> I'd  like to suggest, see below and the other patches for the details.
>
> On Fri, 2022-09-09 at 13:49 +0800, menglong8.dong@gmail.com wrote:
> > From: Menglong Dong <imagedong@tencent.com>
> >
> > Factor out __mptcp_close() from mptcp_close(). The caller of
> > __mptcp_close() should hold the socket lock, and cancel mptcp work when
> > __mptcp_close() return true.
> >
> > This function will be used in the next commit.
> >
> > Reviewed-by: Jiang Biao <benbjiang@tencent.com>
> > Reviewed-by: Mengen Sun <mengensun@tencent.com>
> > Signed-off-by: Menglong Dong <imagedong@tencent.com>
> > ---
> >  net/mptcp/protocol.c | 19 ++++++++++++++-----
> >  net/mptcp/protocol.h |  1 +
> >  2 files changed, 15 insertions(+), 5 deletions(-)
> >
> > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> > index d398f3810662..1cfb4f1ff4e4 100644
> > --- a/net/mptcp/protocol.c
> > +++ b/net/mptcp/protocol.c
> > @@ -2796,13 +2796,12 @@ static void __mptcp_destroy_sock(struct sock *sk)
> >       sock_put(sk);
> >  }
> >
> > -static void mptcp_close(struct sock *sk, long timeout)
> > +bool __mptcp_close(struct sock *sk, long timeout)
> >  {
> >       struct mptcp_subflow_context *subflow;
> >       struct mptcp_sock *msk = mptcp_sk(sk);
> >       bool do_cancel_work = false;
> >
> > -     lock_sock(sk);
> >       sk->sk_shutdown = SHUTDOWN_MASK;
> >
> >       if ((1 << sk->sk_state) & (TCPF_LISTEN | TCPF_CLOSE)) {
> > @@ -2833,7 +2832,6 @@ static void mptcp_close(struct sock *sk, long timeout)
> >       }
> >       sock_orphan(sk);
> >
> > -     sock_hold(sk);
>
> If you don't remove this line you can avoid adding a sock_hold(sk) in
> mptcp_close() and an additional one in patch 3. The diff will be
> smaller and the patch clearer.

I thought about it before, and I was a little afraid that
it would make the function hard to understand.

>
> >       pr_debug("msk=%p state=%d", sk, sk->sk_state);
> >       if (mptcp_sk(sk)->token)
> >               mptcp_event(MPTCP_EVENT_CLOSED, msk, NULL, GFP_KERNEL);
> > @@ -2844,10 +2842,21 @@ static void mptcp_close(struct sock *sk, long timeout)
> >       } else {
> >               mptcp_reset_timeout(msk, 0);
> >       }
> > +
> > +     return do_cancel_work;
> > +}
> > +
> > +static void mptcp_close(struct sock *sk, long timeout)
> > +{
> > +     bool cancel_work;
>
> I think this variable name is a bit confusing, as we already have a
> global function with the same name. I think 'do_cancel_work' will be
> better (and again will produce a smaller diff and a clearer patch).
>

Sounds nice!

(BTW, Happy Mid-Autumn Festival~ )

Thanks!
Menglong Dong

>
> Thanks,
>
> Paolo
>

Re: [PATCH net v4 2/3] net: mptcp: move mptcp_cancel_work() to protocol.h

[Menglong Dong]

On Fri, Sep 9, 2022 at 3:33 PM Paolo Abeni <pabeni@redhat.com> wrote:
>
> On Fri, 2022-09-09 at 13:49 +0800, menglong8.dong@gmail.com wrote:
> > From: Menglong Dong <imagedong@tencent.com>
> >
> > mptcp_cancel_work() is simple enough and we can make it as inline and
> > move it to protocol.h. mptcp_cancel_work() will be used in subflow.c
> > in the next commit.
> >
> > Reviewed-by: Jiang Biao <benbjiang@tencent.com>
> > Reviewed-by: Mengen Sun <mengensun@tencent.com>
> > Signed-off-by: Menglong Dong <imagedong@tencent.com>
> > ---
> >  net/mptcp/protocol.c | 8 --------
> >  net/mptcp/protocol.h | 8 ++++++++
> >  2 files changed, 8 insertions(+), 8 deletions(-)
> >
> > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> > index 1cfb4f1ff4e4..d6eb033e93b9 100644
> > --- a/net/mptcp/protocol.c
> > +++ b/net/mptcp/protocol.c
> > @@ -2656,14 +2656,6 @@ static void __mptcp_clear_xmit(struct sock *sk)
> >               dfrag_clear(sk, dfrag);
> >  }
> >
> > -static void mptcp_cancel_work(struct sock *sk)
>
> I think it's better to just drop the 'static' scope and the function
> declaration to the header.
>
> This is not critical/fast path, so it's better to keep the generated
> code smaller vs. possibly avoiding a function call due to inlining.
>

Okay!

> Thanks,
>
> Paolo
>