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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 7 Sep 2022 16:59:29 +0800
From:   Menglong Dong <menglong8.dong@...il.com>
To:     Matthieu Baerts <matthieu.baerts@...sares.net>
Cc:     pabeni@...hat.com, mathew.j.martineau@...ux.intel.com,
        davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
        fw@...len.de, peter.krystad@...ux.intel.com,
        netdev@...r.kernel.org, mptcp@...ts.linux.dev,
        linux-kernel@...r.kernel.org,
        Menglong Dong <imagedong@...cent.com>,
        Jiang Biao <benbjiang@...cent.com>,
        Mengen Sun <mengensun@...cent.com>
Subject: Re: [PATCH net v2] net: mptcp: fix unreleased socket in accept queue

On Wed, Sep 7, 2022 at 4:51 PM Matthieu Baerts
<matthieu.baerts@...sares.net> wrote:
>
> Hi Menglong,
>
> On 07/09/2022 10:33, menglong8.dong@...il.com wrote:
> > From: Menglong Dong <imagedong@...cent.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.
>
> 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