netdev - Re: (2) FW: [Resource Leak] Suggesting patch for tcp

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]

Message-ID: <CAKD1Yr14r6rGL8O9oofP9yQRN-i14HwG7nR2TNc4ua4eAfPrVg@mail.gmail.com>
Date:   Tue, 27 Nov 2018 23:36:06 -0800
From:   Lorenzo Colitti <lorenzo@...gle.com>
To:     배석진 <soukjin.bae@...sung.com>
Cc:     Eric Dumazet <eric.dumazet@...il.com>, netdev@...r.kernel.org,
        유금환 <geumhwan.yu@...sung.com>,
        Maciej Żenczykowski <zenczykowski@...il.com>
Subject: Re: (2) FW: [Resource Leak] Suggesting patch for tcp_close

On Tue, Nov 27, 2018 at 10:17 PM 배석진 <soukjin.bae@...sung.com> wrote:
> >> we saw hundreds of not closed tcp session with FIN_WAIT1 and LAST_ACK.
> >
> > These sessions should have a timer, and eventually disappear.
>
> FIN_WAIT2 and TIME_WAIT have a timer.
> but FIN_WAIT1 and LAST_ACK are have too?

What harm is caused by these stale sessions? I thought that was the
intended behaviour.

If you look at the original design discussions that led to the
SOCK_DESTROY and tcp_abort patch, the goal of SOCK_DESTROY was not
primarily to clear TCP state, but primarily to unblock applications
that were stuck in blocking socket operations such as read(), write()
and connect. That is the reason why it only calls tcp_done if the
SOCK_DEAD flag is not set. See in particular
https://www.spinics.net/lists/netdev/msg352716.html , where opposition
was voiced to being able to close sockets in TIME_WAIT_STATE. That
said, I don't have a strong opinion on this: whatever works for Eric
works for me.

> > Do you have a test to demonstrate the issue ?
> >
> > I know Lorenzo wrote tests, so presumably new tests are needed.

There are definitely tests that check that SOCK_DESTROY does nothing
for dead sockets (i.e., the current behaviour without this patch).
Those tests are part of the Android conformance test suites (VTS), so
I think taking this patch will cause the device to fail the Android
conformance here:

https://cs.corp.google.com/android/kernel/tests/net/test/sock_diag_test.py?l=663

Soukjin, you will need to modify that test if this patch is applied.