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

[Paolo Abeni <pabeni@redhat.com>]

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