diff mbox series

[net,v2] net: mptcp: fix unreleased socket in accept queue

Message ID 20220907083304.605526-1-imagedong@tencent.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series [net,v2] net: mptcp: fix unreleased socket in accept queue | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net
netdev/fixes_present success Fixes tag present in non-next series
netdev/subject_prefix success Link
netdev/cover_letter success Single patches do not need cover letters
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit fail Errors and warnings before: 3 this patch: 9
netdev/cc_maintainers success CCed 10 of 10 maintainers
netdev/build_clang fail Errors and warnings before: 5 this patch: 8
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn fail Errors and warnings before: 3 this patch: 9
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 95 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Menglong Dong Sept. 7, 2022, 8:33 a.m. UTC 
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_nolock().

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.

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>
---
 net/mptcp/protocol.c | 13 +++++++++----
 net/mptcp/subflow.c  | 33 ++++++++-------------------------
 2 files changed, 17 insertions(+), 29 deletions(-)

Comments

Matthieu Baerts Sept. 7, 2022, 8:51 a.m. UTC | #1 
Hi Menglong,

On 07/09/2022 10:33, 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.

Thank you for the patch!

(...)

> ---
>  net/mptcp/protocol.c | 13 +++++++++----
>  net/mptcp/subflow.c  | 33 ++++++++-------------------------
>  2 files changed, 17 insertions(+), 29 deletions(-)
> 
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index d398f3810662..fe6b7fbb145c 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)
> +void mptcp_close_nolock(struct sock *sk, long timeout)

I didn't look at it into details but like the previous previous, I don't
think this one compiles without errors: you define this (non static)
function here in protocol.c but you don't "expose" it in protocol.h ...
(see below)

> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index c7d49fb6e7bd..cebabf2bb222 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c

(...)

> @@ -1765,11 +1740,19 @@ void mptcp_subflow_queue_clean(struct sock *listener_ssk)
>  		struct sock *sk = (struct sock *)msk;
>  		bool slow;
>  
> +		sock_hold(sk);
>  		slow = lock_sock_fast_nested(sk);
>  		next = msk->dl_next;
>  		msk->first = NULL;
>  		msk->dl_next = NULL;
> +
> +		/* mptcp_close_nolock() will put a extra reference on sk,
> +		 * so we hold one here.
> +		 */
> +		sock_hold(sk);
> +		mptcp_close_nolock(sk, 0);

... I guess the compiler will complain if you try to use it here from
subflow.c, no?

Also, did you have the opportunity to run the different MPTCP selftests
with this patch?

Cheers,
Matt
Menglong Dong Sept. 7, 2022, 8:59 a.m. UTC | #2 
On Wed, Sep 7, 2022 at 4:51 PM Matthieu Baerts
<matthieu.baerts@tessares.net> wrote:
>
> Hi Menglong,
>
> On 07/09/2022 10:33, 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.
>
> Thank you for the patch!
>
> (...)
>
> > ---
> >  net/mptcp/protocol.c | 13 +++++++++----
> >  net/mptcp/subflow.c  | 33 ++++++++-------------------------
> >  2 files changed, 17 insertions(+), 29 deletions(-)
> >
> > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> > index d398f3810662..fe6b7fbb145c 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)
> > +void mptcp_close_nolock(struct sock *sk, long timeout)
>
> I didn't look at it into details but like the previous previous, I don't
> think this one compiles without errors: you define this (non static)
> function here in protocol.c but you don't "expose" it in protocol.h ...
> (see below)
>

Oops...I forgot to commit the protocol.h :)

> > diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> > index c7d49fb6e7bd..cebabf2bb222 100644
> > --- a/net/mptcp/subflow.c
> > +++ b/net/mptcp/subflow.c
>
> (...)
>
> > @@ -1765,11 +1740,19 @@ void mptcp_subflow_queue_clean(struct sock *listener_ssk)
> >               struct sock *sk = (struct sock *)msk;
> >               bool slow;
> >
> > +             sock_hold(sk);
> >               slow = lock_sock_fast_nested(sk);
> >               next = msk->dl_next;
> >               msk->first = NULL;
> >               msk->dl_next = NULL;
> > +
> > +             /* mptcp_close_nolock() will put a extra reference on sk,
> > +              * so we hold one here.
> > +              */
> > +             sock_hold(sk);
> > +             mptcp_close_nolock(sk, 0);
>
> ... I guess the compiler will complain if you try to use it here from
> subflow.c, no?
>

Hmm...The compiler didn't, as I modified protocol.h locally. That
's why I didn't find this mistake :)

> Also, did you have the opportunity to run the different MPTCP selftests
> with this patch?
>

Not yet. I'll do it before the next version.

Thanks!
Menglong Dong

> Cheers,
> Matt
> --
> Tessares | Belgium | Hybrid Access Solutions
> www.tessares.net
kernel test robot Sept. 7, 2022, 9:36 a.m. UTC | #3 
Hi,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on net/master]

url:    https://github.com/intel-lab-lkp/linux/commits/menglong8-dong-gmail-com/net-mptcp-fix-unreleased-socket-in-accept-queue/20220907-163559
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git e1091e226a2bab4ded1fe26efba2aee1aab06450
config: parisc-allyesconfig (https://download.01.org/0day-ci/archive/20220907/202209071742.Cv2hLTkj-lkp@intel.com/config)
compiler: hppa-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/d238db12dcac26bfb2197e0aae24fadf6bbfdcb6
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review menglong8-dong-gmail-com/net-mptcp-fix-unreleased-socket-in-accept-queue/20220907-163559
        git checkout d238db12dcac26bfb2197e0aae24fadf6bbfdcb6
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=parisc SHELL=/bin/bash net/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> net/mptcp/protocol.c:2799:6: warning: no previous prototype for 'mptcp_close_nolock' [-Wmissing-prototypes]
    2799 | void mptcp_close_nolock(struct sock *sk, long timeout)
         |      ^~~~~~~~~~~~~~~~~~


vim +/mptcp_close_nolock +2799 net/mptcp/protocol.c

  2798	
> 2799	void mptcp_close_nolock(struct sock *sk, long timeout)
  2800	{
  2801		struct mptcp_subflow_context *subflow;
  2802		struct mptcp_sock *msk = mptcp_sk(sk);
  2803		bool do_cancel_work = false;
  2804	
  2805		sk->sk_shutdown = SHUTDOWN_MASK;
  2806	
  2807		if ((1 << sk->sk_state) & (TCPF_LISTEN | TCPF_CLOSE)) {
  2808			inet_sk_state_store(sk, TCP_CLOSE);
  2809			goto cleanup;
  2810		}
  2811	
  2812		if (mptcp_close_state(sk))
  2813			__mptcp_wr_shutdown(sk);
  2814	
  2815		sk_stream_wait_close(sk, timeout);
  2816	
  2817	cleanup:
  2818		/* orphan all the subflows */
  2819		inet_csk(sk)->icsk_mtup.probe_timestamp = tcp_jiffies32;
  2820		mptcp_for_each_subflow(msk, subflow) {
  2821			struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
  2822			bool slow = lock_sock_fast_nested(ssk);
  2823	
  2824			/* since the close timeout takes precedence on the fail one,
  2825			 * cancel the latter
  2826			 */
  2827			if (ssk == msk->first)
  2828				subflow->fail_tout = 0;
  2829	
  2830			sock_orphan(ssk);
  2831			unlock_sock_fast(ssk, slow);
  2832		}
  2833		sock_orphan(sk);
  2834	
  2835		pr_debug("msk=%p state=%d", sk, sk->sk_state);
  2836		if (mptcp_sk(sk)->token)
  2837			mptcp_event(MPTCP_EVENT_CLOSED, msk, NULL, GFP_KERNEL);
  2838	
  2839		if (sk->sk_state == TCP_CLOSE) {
  2840			__mptcp_destroy_sock(sk);
  2841			do_cancel_work = true;
  2842		} else {
  2843			mptcp_reset_timeout(msk, 0);
  2844		}
  2845	
  2846		if (do_cancel_work)
  2847			mptcp_cancel_work(sk);
  2848	}
  2849
diff mbox series

Patch

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index d398f3810662..fe6b7fbb145c 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)
+void mptcp_close_nolock(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,17 @@  static void mptcp_close(struct sock *sk, long timeout)
 	} else {
 		mptcp_reset_timeout(msk, 0);
 	}
-	release_sock(sk);
+
 	if (do_cancel_work)
 		mptcp_cancel_work(sk);
+}
 
+static void mptcp_close(struct sock *sk, long timeout)
+{
+	sock_hold(sk);
+	lock_sock(sk);
+	mptcp_close_nolock(sk, timeout);
+	release_sock(sk);
 	sock_put(sk);
 }
 
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index c7d49fb6e7bd..cebabf2bb222 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));
@@ -1765,11 +1740,19 @@  void mptcp_subflow_queue_clean(struct sock *listener_ssk)
 		struct sock *sk = (struct sock *)msk;
 		bool slow;
 
+		sock_hold(sk);
 		slow = lock_sock_fast_nested(sk);
 		next = msk->dl_next;
 		msk->first = NULL;
 		msk->dl_next = NULL;
+
+		/* mptcp_close_nolock() will put a extra reference on sk,
+		 * so we hold one here.
+		 */
+		sock_hold(sk);
+		mptcp_close_nolock(sk, 0);
 		unlock_sock_fast(sk, slow);
+		sock_put(sk);
 	}
 
 	/* we are still under the listener msk socket lock */