lkml.org 
[lkml]   [2022]   [Sep]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
SubjectRe: [PATCH net] net: mptcp: fix unreleased socket in accept queue
From
Date
On Mon, 2022-09-05 at 17:03 +0800, Menglong Dong wrote:
> On Mon, Sep 5, 2022 at 4:26 PM Paolo Abeni <pabeni@redhat.com> wrote:
> >
> > Hello,
> >
> > On Mon, 2022-09-05 at 13:04 +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 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.
---
>
> > > 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

>
> > > Signed-off-by: Menglong Dong <imagedong@tencent.com>
> > > ---
> > > 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

\
 
 \ /
  Last update: 2022-09-06 09:04    [W:0.095 / U:0.524 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site