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

[Menglong Dong]

On Wed, Sep 7, 2022 at 4:51 PM Matthieu Baerts
<matthieu.baerts@xxxxxxxxxxxx> wrote:
>
> Hi Menglong,
>
> On 07/09/2022 10:33, 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.
>
> 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