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

Subsystems: networking [general], networking [mptcp], the rest

STALE1465d

6 messages, 3 authors, 2022-09-07 · open the first message on its own page

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

From: <hidden>
Date: 2022-09-05 05:04:30

From: Menglong Dong <redacted>

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 uses mptcp protocol by default.
   And the default backlog is 1:
   ip netns exec mptcp-server nc -l -k -p 9999

4. open another *two* terminal and 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

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.

There are some solutions that I thought:

1. release all unaccepted mptcp socket with mptcp_close() while the
   listening tcp socket release in mptcp_subflow_queue_clean(). This is
   what we do in this commit.
2. release the mptcp socket with mptcp_close() in subflow_ulp_release().
3. etc

Signed-off-by: Menglong Dong <redacted>
---
 net/mptcp/subflow.c | 4 ++++
 1 file changed, 4 insertions(+)
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index c7d49fb6e7bd..e39dff5d5d84 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -1770,6 +1770,10 @@ void mptcp_subflow_queue_clean(struct sock *listener_ssk)
 		msk->first = NULL;
 		msk->dl_next = NULL;
 		unlock_sock_fast(sk, slow);
+
+		/*  */
+		sock_hold(sk);
+		sk->sk_prot->close(sk);
 	}
 
 	/* we are still under the listener msk socket lock */
-- 
2.37.2

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

From: kernel test robot <hidden>
Date: 2022-09-05 06:48:49

Hi,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on net/master]

url:    https://github.com/intel-lab-lkp/linux/commits/menglong8-dong-gmail-com/net-mptcp-fix-unreleased-socket-in-accept-queue/20220905-130457
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git c90714017cb3f197e71c7ff1317335b96d4d19e8
config: s390-randconfig-r015-20220905
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project c55b41d5199d2394dd6cdb8f52180d8b81d809d4)
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
        # install s390 cross compiling tool for clang build
        # apt-get install binutils-s390x-linux-gnu
        # https://github.com/intel-lab-lkp/linux/commit/496c680afa6c8a180858e88ba2b5a6aa6d262bed
        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/20220905-130457
        git checkout 496c680afa6c8a180858e88ba2b5a6aa6d262bed
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=s390 SHELL=/bin/bash net/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <redacted>

All errors (new ones prefixed by >>):

   In file included from net/mptcp/subflow.c:11:
   In file included from include/linux/netdevice.h:38:
   In file included from include/net/net_namespace.h:43:
   In file included from include/linux/skbuff.h:31:
   In file included from include/linux/dma-mapping.h:10:
   In file included from include/linux/scatterlist.h:9:
   In file included from arch/s390/include/asm/io.h:75:
   include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __raw_readb(PCI_IOBASE + addr);
                             ~~~~~~~~~~ ^
   include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/big_endian.h:37:59: note: expanded from macro '__le16_to_cpu'
   #define __le16_to_cpu(x) __swab16((__force __u16)(__le16)(x))
                                                             ^
   include/uapi/linux/swab.h:102:54: note: expanded from macro '__swab16'
   #define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
                                                        ^
   In file included from net/mptcp/subflow.c:11:
   In file included from include/linux/netdevice.h:38:
   In file included from include/net/net_namespace.h:43:
   In file included from include/linux/skbuff.h:31:
   In file included from include/linux/dma-mapping.h:10:
   In file included from include/linux/scatterlist.h:9:
   In file included from arch/s390/include/asm/io.h:75:
   include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/big_endian.h:35:59: note: expanded from macro '__le32_to_cpu'
   #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
                                                             ^
   include/uapi/linux/swab.h:115:54: note: expanded from macro '__swab32'
   #define __swab32(x) (__u32)__builtin_bswap32((__u32)(x))
                                                        ^
   In file included from net/mptcp/subflow.c:11:
   In file included from include/linux/netdevice.h:38:
   In file included from include/net/net_namespace.h:43:
   In file included from include/linux/skbuff.h:31:
   In file included from include/linux/dma-mapping.h:10:
   In file included from include/linux/scatterlist.h:9:
   In file included from arch/s390/include/asm/io.h:75:
   include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writeb(value, PCI_IOBASE + addr);
                               ~~~~~~~~~~ ^
   include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
   include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
   include/asm-generic/io.h:692:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsb(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:700:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsw(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:708:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsl(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:717:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesb(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
   include/asm-generic/io.h:726:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesw(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
   include/asm-generic/io.h:735:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesl(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
quoted
net/mptcp/subflow.c:1776:24: error: too few arguments to function call, expected 2, have 1
                   sk->sk_prot->close(sk);
                   ~~~~~~~~~~~~~~~~~~   ^
   12 warnings and 1 error generated.


vim +1776 net/mptcp/subflow.c

  1726	
  1727	void mptcp_subflow_queue_clean(struct sock *listener_ssk)
  1728	{
  1729		struct request_sock_queue *queue = &inet_csk(listener_ssk)->icsk_accept_queue;
  1730		struct mptcp_sock *msk, *next, *head = NULL;
  1731		struct request_sock *req;
  1732	
  1733		/* build a list of all unaccepted mptcp sockets */
  1734		spin_lock_bh(&queue->rskq_lock);
  1735		for (req = queue->rskq_accept_head; req; req = req->dl_next) {
  1736			struct mptcp_subflow_context *subflow;
  1737			struct sock *ssk = req->sk;
  1738			struct mptcp_sock *msk;
  1739	
  1740			if (!sk_is_mptcp(ssk))
  1741				continue;
  1742	
  1743			subflow = mptcp_subflow_ctx(ssk);
  1744			if (!subflow || !subflow->conn)
  1745				continue;
  1746	
  1747			/* skip if already in list */
  1748			msk = mptcp_sk(subflow->conn);
  1749			if (msk->dl_next || msk == head)
  1750				continue;
  1751	
  1752			msk->dl_next = head;
  1753			head = msk;
  1754		}
  1755		spin_unlock_bh(&queue->rskq_lock);
  1756		if (!head)
  1757			return;
  1758	
  1759		/* can't acquire the msk socket lock under the subflow one,
  1760		 * or will cause ABBA deadlock
  1761		 */
  1762		release_sock(listener_ssk);
  1763	
  1764		for (msk = head; msk; msk = next) {
  1765			struct sock *sk = (struct sock *)msk;
  1766			bool slow;
  1767	
  1768			slow = lock_sock_fast_nested(sk);
  1769			next = msk->dl_next;
  1770			msk->first = NULL;
  1771			msk->dl_next = NULL;
  1772			unlock_sock_fast(sk, slow);
  1773	
  1774			/*  */
  1775			sock_hold(sk);
1776			sk->sk_prot->close(sk);
  1777		}
  1778	
  1779		/* we are still under the listener msk socket lock */
  1780		lock_sock_nested(listener_ssk, SINGLE_DEPTH_NESTING);
  1781	}
  1782	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

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

From: Paolo Abeni <pabeni@redhat.com>
Date: 2022-09-05 08:27:03

Hello,

On Mon, 2022-09-05 at 13:04 +0800, menglong8.dong@gmail.com wrote:
From: Menglong Dong <redacted>

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 uses mptcp protocol by default.
   And the default backlog is 1:
   ip netns exec mptcp-server nc -l -k -p 9999

4. open another *two* terminal and 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

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.
Thank you for the report! 

I have a doubt WRT the above scenario: AFAICS 'nc' will accept the
incoming sockets ASAP, so the unaccepted queue should be empty at
shutdown, but that does not fit with your description?!?
There are some solutions that I thought:

1. release all unaccepted mptcp socket with mptcp_close() while the
   listening tcp socket release in mptcp_subflow_queue_clean(). This is
   what we do in this commit.
2. release the mptcp socket with mptcp_close() in subflow_ulp_release().
3. etc
Can you please point to a commit introducing the issue?
quoted hunk
Signed-off-by: Menglong Dong <redacted>
---
 net/mptcp/subflow.c | 4 ++++
 1 file changed, 4 insertions(+)
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index c7d49fb6e7bd..e39dff5d5d84 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -1770,6 +1770,10 @@ void mptcp_subflow_queue_clean(struct sock *listener_ssk)
 		msk->first = NULL;
 		msk->dl_next = NULL;
 		unlock_sock_fast(sk, slow);
+
+		/*  */
+		sock_hold(sk);
+		sk->sk_prot->close(sk);
You can call mptcp_close() directly here.

Perhaps we could as well drop the mptcp_sock_destruct() hack?

Perhpas even providing a __mptcp_close() variant not acquiring the
socket lock and move such close call inside the existing sk socket lock
above?

Thanks,

Paolo

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

From: Menglong Dong <hidden>
Date: 2022-09-05 09:04:09

On Mon, Sep 5, 2022 at 4:26 PM Paolo Abeni [off-list ref] wrote:
Hello,

On Mon, 2022-09-05 at 13:04 +0800, menglong8.dong@gmail.com wrote:
quoted
From: Menglong Dong <redacted>

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 uses mptcp protocol by default.
   And the default backlog is 1:
   ip netns exec mptcp-server nc -l -k -p 9999

4. open another *two* terminal and 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

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.
Thank you for the report!

I have a doubt WRT the above scenario: AFAICS 'nc' will accept the
incoming sockets ASAP, so the unaccepted queue should be empty at
shutdown, but that does not fit with your description?!?
By default, as far as in my case, nc won't accept the new connection
until the first connection closes with the '-k' set. Therefor, the second
connection will stay in the unaccepted queue.
quoted
There are some solutions that I thought:

1. release all unaccepted mptcp socket with mptcp_close() while the
   listening tcp socket release in mptcp_subflow_queue_clean(). This is
   what we do in this commit.
2. release the mptcp socket with mptcp_close() in subflow_ulp_release().
3. etc
Can you please point to a commit introducing the issue?
In fact, I'm not sure. In my case, I found this issue in kernel 5.10.
And I wanted to find the solution in the upstream, but find that
upstream has this issue too.

Hmm...I am curious if this issue exists in the beginning? I
can't find the opportunity that the joined subflow which are
unaccepted can be released.
quoted
Signed-off-by: Menglong Dong <redacted>
---
 net/mptcp/subflow.c | 4 ++++
 1 file changed, 4 insertions(+)
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index c7d49fb6e7bd..e39dff5d5d84 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -1770,6 +1770,10 @@ void mptcp_subflow_queue_clean(struct sock *listener_ssk)
              msk->first = NULL;
              msk->dl_next = NULL;
              unlock_sock_fast(sk, slow);
+
+             /*  */
+             sock_hold(sk);
+             sk->sk_prot->close(sk);
You can call mptcp_close() directly here.

Perhaps we could as well drop the mptcp_sock_destruct() hack?
Do you mean to call mptcp_sock_destruct() directly here?
Perhpas even providing a __mptcp_close() variant not acquiring the
socket lock and move such close call inside the existing sk socket lock
above?
Yeah, sounds nice.

Thanks!
Menglong Dong
Thanks,

Paolo

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

From: Paolo Abeni <pabeni@redhat.com>
Date: 2022-09-06 07:03:03

On Mon, 2022-09-05 at 17:03 +0800, Menglong Dong wrote:
On Mon, Sep 5, 2022 at 4:26 PM Paolo Abeni [off-list ref] wrote:
quoted
Hello,

On Mon, 2022-09-05 at 13:04 +0800, menglong8.dong@gmail.com wrote:
quoted
From: Menglong Dong <redacted>

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 uses mptcp protocol by default.
   And the default backlog is 1:
   ip netns exec mptcp-server nc -l -k -p 9999

4. open another *two* terminal and 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

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.
Thank you for the report!

I have a doubt WRT the above scenario: AFAICS 'nc' will accept the
incoming sockets ASAP, so the unaccepted queue should be empty at
shutdown, but that does not fit with your description?!?
By default, as far as in my case, nc won't accept the new connection
until the first connection closes with the '-k' set. Therefor, the second
connection will stay in the unaccepted queue.
I missed the fact you opened 2 connections. I guess that is point 4
above. Please rephrase that sentence with something alike:

---
4. open another *two* terminal and use each of them to connect to the
server with the following command:
...
So that there are two established mptcp connections, with the second
one still unaccepted.
---
quoted
quoted
There are some solutions that I thought:

1. release all unaccepted mptcp socket with mptcp_close() while the
   listening tcp socket release in mptcp_subflow_queue_clean(). This is
   what we do in this commit.
2. release the mptcp socket with mptcp_close() in subflow_ulp_release().
3. etc
Can you please point to a commit introducing the issue?
In fact, I'm not sure. In my case, I found this issue in kernel 5.10.
And I wanted to find the solution in the upstream, but find that
upstream has this issue too.

Hmm...I am curious if this issue exists in the beginning? I
can't find the opportunity that the joined subflow which are
unaccepted can be released.
It looks like the problem is there since MPJ support, commit
f296234c98a8fcec94eec80304a873f635d350ea
quoted
quoted
Signed-off-by: Menglong Dong <redacted>
---
 net/mptcp/subflow.c | 4 ++++
 1 file changed, 4 insertions(+)
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index c7d49fb6e7bd..e39dff5d5d84 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -1770,6 +1770,10 @@ void mptcp_subflow_queue_clean(struct sock *listener_ssk)
              msk->first = NULL;
              msk->dl_next = NULL;
              unlock_sock_fast(sk, slow);
+
+             /*  */
+             sock_hold(sk);
+             sk->sk_prot->close(sk);
You can call mptcp_close() directly here.

Perhaps we could as well drop the mptcp_sock_destruct() hack?
Do you mean to call mptcp_sock_destruct() directly here?
I suspect that with this change setting msk->sk_destruct to
mptcp_sock_destruct in subflow_syn_recv_sock() is not needed anymore,
and the relevant intialization (and callback definition) could be
removed.
Cheers,

Paolo

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

From: Menglong Dong <hidden>
Date: 2022-09-07 07:13:00

On Tue, Sep 6, 2022 at 3:02 PM Paolo Abeni [off-list ref] wrote:
On Mon, 2022-09-05 at 17:03 +0800, Menglong Dong wrote:
quoted
On Mon, Sep 5, 2022 at 4:26 PM Paolo Abeni [off-list ref] wrote:
quoted
Hello,

On Mon, 2022-09-05 at 13:04 +0800, menglong8.dong@gmail.com wrote:
quoted
From: Menglong Dong <redacted>

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 uses mptcp protocol by default.
   And the default backlog is 1:
   ip netns exec mptcp-server nc -l -k -p 9999

4. open another *two* terminal and 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

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.
Thank you for the report!

I have a doubt WRT the above scenario: AFAICS 'nc' will accept the
incoming sockets ASAP, so the unaccepted queue should be empty at
shutdown, but that does not fit with your description?!?
By default, as far as in my case, nc won't accept the new connection
until the first connection closes with the '-k' set. Therefor, the second
connection will stay in the unaccepted queue.
I missed the fact you opened 2 connections. I guess that is point 4
above. Please rephrase that sentence with something alike:

---
4. open another *two* terminal and use each of them to connect to the
server with the following command:
...
So that there are two established mptcp connections, with the second
one still unaccepted.
---
Sounds nice! Thanks~
quoted
quoted
quoted
There are some solutions that I thought:

1. release all unaccepted mptcp socket with mptcp_close() while the
   listening tcp socket release in mptcp_subflow_queue_clean(). This is
   what we do in this commit.
2. release the mptcp socket with mptcp_close() in subflow_ulp_release().
3. etc
Can you please point to a commit introducing the issue?
In fact, I'm not sure. In my case, I found this issue in kernel 5.10.
And I wanted to find the solution in the upstream, but find that
upstream has this issue too.

Hmm...I am curious if this issue exists in the beginning? I
can't find the opportunity that the joined subflow which are
unaccepted can be released.
It looks like the problem is there since MPJ support, commit
f296234c98a8fcec94eec80304a873f635d350ea
Yeah, I'll add a Fixes tag for this commit.
quoted
quoted
quoted
Signed-off-by: Menglong Dong <redacted>
---
 net/mptcp/subflow.c | 4 ++++
 1 file changed, 4 insertions(+)
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index c7d49fb6e7bd..e39dff5d5d84 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -1770,6 +1770,10 @@ void mptcp_subflow_queue_clean(struct sock *listener_ssk)
              msk->first = NULL;
              msk->dl_next = NULL;
              unlock_sock_fast(sk, slow);
+
+             /*  */
+             sock_hold(sk);
+             sk->sk_prot->close(sk);
You can call mptcp_close() directly here.

Perhaps we could as well drop the mptcp_sock_destruct() hack?
Do you mean to call mptcp_sock_destruct() directly here?
I suspect that with this change setting msk->sk_destruct to
mptcp_sock_destruct in subflow_syn_recv_sock() is not needed anymore,
and the relevant intialization (and callback definition) could be
removed.
Your suspect should be right. The mptcp_subflow_queue_clean()
should always be called before unaccepted tcp socket and the
corresponding mptcp socket release.
Therefore this change can ensure that the mptcp socket will be
CLOSE state when it is released. I'll remove
mptcp_sock_destruct(), BTW.

Thanks!
Menglong Dong
quoted
Cheers,

Paolo
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help