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

[Menglong Dong]

On Mon, Sep 5, 2022 at 4:26 PM Paolo Abeni <pabeni@xxxxxxxxxx> wrote:
>
> Hello,
>
> On Mon, 2022-09-05 at 13:04 +0800, menglong8.dong@xxxxxxxxx wrote:
> > From: Menglong Dong <imagedong@xxxxxxxxxxx>
> >
> > 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.

> > 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.

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