netdev - Re: [PATCH v2 net] tcp: allow again tcp_disconnect() when threads are waiting

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: <CAMaK5_ii38_Ze2uBmcyX8rnntEi35kXJ47yhxZvCb-ks0bMbxw@mail.gmail.com>
Date: Fri, 13 Oct 2023 12:42:31 +0800
From: Xin Guo <guoxin0309@...il.com>
To: Eric Dumazet <edumazet@...gle.com>
Cc: Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org, 
	Ayush Sawal <ayush.sawal@...lsio.com>, "David S. Miller" <davem@...emloft.net>, 
	Jakub Kicinski <kuba@...nel.org>, David Ahern <dsahern@...nel.org>, mptcp@...ts.linux.dev, 
	Boris Pismenny <borisp@...dia.com>, Tom Deseyn <tdeseyn@...hat.com>
Subject: Re: [PATCH v2 net] tcp: allow again tcp_disconnect() when threads are waiting

Hi,
In my view, this patch is NOT so good, and it seems that trying to fix
a problem temporarily without knowing its root cause,
because sk_wait_event function should know nothing about the other
functions were called or not,
but now this patch added a logic to let sk_wait_event know the
specific tcp_dissconnect function was called by other threads or NOT,
honestly speaking, it is NOT a good designation,
so what is root cause about the problem which [0] commit want to fix?
can we have a way to fix it directly instead of denying
tcp_disconnect() when threads are waiting?
if No better way to fix it, please add some description about the
difficulty and our compromise in the commit message, otherwise the
patch will be confused.


[0]: 4faeee0cf8a5 ("tcp: deny tcp_disconnect() when threads are waiting")

On Wed, Oct 11, 2023 at 3:36 PM Eric Dumazet <edumazet@...gle.com> wrote:
>
> On Wed, Oct 11, 2023 at 9:21 AM Paolo Abeni <pabeni@...hat.com> wrote:
> >
> > As reported by Tom, .NET and applications build on top of it rely
> > on connect(AF_UNSPEC) to async cancel pending I/O operations on TCP
> > socket.
> >
> > The blamed commit below caused a regression, as such cancellation
> > can now fail.
> >
> > As suggested by Eric, this change addresses the problem explicitly
> > causing blocking I/O operation to terminate immediately (with an error)
> > when a concurrent disconnect() is executed.
> >
> > Instead of tracking the number of threads blocked on a given socket,
> > track the number of disconnect() issued on such socket. If such counter
> > changes after a blocking operation releasing and re-acquiring the socket
> > lock, error out the current operation.
> >
> > Fixes: 4faeee0cf8a5 ("tcp: deny tcp_disconnect() when threads are waiting")
> > Reported-by: Tom Deseyn <tdeseyn@...hat.com>
> > Closes: https://bugzilla.redhat.com/show_bug.cgi?id=1886305
> > Suggested-by: Eric Dumazet <edumazet@...gle.com>
> > Signed-off-by: Paolo Abeni <pabeni@...hat.com>
>
> Reviewed-by: Eric Dumazet <edumazet@...gle.com>
>
> Thanks !
>