netdev - Re: perhaps inet_csk_reqsk_queue_is_full should also allow zero backlog

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] [day] [month] [year] [list]

Message-ID: <CAFmV8NdMqfajmq1W=zAPpeJ28tCwekfi6-7jy6wunYDZXKRVUw@mail.gmail.com>
Date: Thu, 2 Jan 2025 23:57:19 +0800
From: Zhongqiu Duan <dzq.aishenghu0@...il.com>
To: Kuniyuki Iwashima <kuniyu@...zon.com>
Cc: davem@...emloft.net, edumazet@...gle.com, kerneljasonxing@...il.com, 
	kuba@...nel.org, netdev@...r.kernel.org, pabeni@...hat.com
Subject: Re: perhaps inet_csk_reqsk_queue_is_full should also allow zero backlog

On Thu, Jan 2, 2025 at 4:03 PM Kuniyuki Iwashima <kuniyu@...zon.com> wrote:
>
> From: Zhongqiu Duan <dzq.aishenghu0@...il.com>
> Date: Wed, 1 Jan 2025 23:02:56 +0800
> > On Wed, Jan 1, 2025 at 9:53 AM Jason Xing <kerneljasonxing@...il.com> wrote:
> > >
> > > On Tue, Dec 31, 2024 at 4:24 PM Zhongqiu Duan <dzq.aishenghu0@...il.com> wrote:
> > > >
> > > > Hi all,
> > > >
> > > > We use a proprietary library in our product, it passes hardcoded zero
> > > > as the backlog of listen().
> > > > It works fine when syncookies is enabled, but when we disable syncookies
> > > > by business requirement, no connection can be made.
> > >
> > > I'm not that sure that the problem you encountered is the same as
> > > mine. I manage to reproduce it locally after noticing your report:
> > > 1) write the simplest c code with passing 0 as the backlog
> > > 2) adjust the value of net.ipv4.tcp_syncookies to see the different results
> > >
> > > When net.ipv4.tcp_syncookies is set zero only, the connection will not
> > > be established.
> > >
> >
> > Yes, that's the problem I want to describe.
> >
> > > >
> > > > After some investigation, the problem is focused on the
> > > > inet_csk_reqsk_queue_is_full().
> > > >
> > > > static inline int inet_csk_reqsk_queue_is_full(const struct sock *sk)
> > > > {
> > > >         return inet_csk_reqsk_queue_len(sk) >=
> > > > READ_ONCE(sk->sk_max_ack_backlog);
> > > > }
> > > >
> > > > I noticed that the stories happened to sk_acceptq_is_full() about this
> > > > in the past, like
> > > > the commit c609e6a (Revert "net: correct sk_acceptq_is_full()").
> > > >
> > > > Perhaps we can also avoid the problem by using ">" in the decision
> > > > condition like
> > > > `inet_csk_reqsk_queue_len(sk) > READ_ONCE(sk->sk_max_ack_backlog)`.
> > >
> > > According to the experiment I conducted, I agree the above triggers
> > > the drop in tcp_conn_request(). When that sysctl is set to zero, the
> > > return value of tcp_syn_flood_action() is false, which leads to an
> > > immediate drop.
> > >
> > > Your changes in tcp_conn_request() can solve this issue, but you're
> > > solving a not that valid issue which can be handled in a decent way as
> > > below [1]. I can't see any good reason for passing zero as a backlog
> > > value in listen() since the sk_max_ack_backlog would be zero for sure.
> > >
> > > [1]
> > > I would also suggest trying the following two steps first like other people do:
> > > 1) pass a larger backlog number when calling listen().
> > > 2) adjust the sysctl net.core.somaxconn, say, a much larger one, like 40960
> > >
> > > Thanks,
> > > Jason
> >
> > Even though only one connection is needed for this proprietary library
> > to work properly, I don't see any reason to set the backlog to zero
> > either. But it just happened. We simply bin patch the 3rd party
> > library to set a larger value for the backlog as a workaround.
>
> A common technique is to specify -1 for listen() backlog.
>
> Then you even need not know somaxconn but can use it as the max
> backlog. (see __sys_listen_socket())
>
> This is especially useful in a container env where app is not
> allowed to read sysctl knobs.
>
>

Thanks for sharing this information I do not know.

> >
> > Thanks for your suggestions, and I almost totally agree with you. I
> > just want to discuss whether it should and deserves to make some
> > changes in the kernel to keep the same behavior between
> > sk_acceptq_is_full() and inet_csk_reqsk_queue_is_full().
>
> I think you can post a patch to make it consistent with 64a146513f8f:
>
> ---8<---
> commit 64a146513f8f12ba204b7bf5cb7e9505594ead42
> Author: David S. Miller <davem@...set.davemloft.net>
> Date:   Tue Mar 6 11:21:05 2007 -0800
>
>     [NET]: Revert incorrect accept queue backlog changes.
> ...
>     A backlog value of N really does mean allow "N + 1" connections
>     to queue to a listening socket.  This allows one to specify
>     "0" as the backlog and still get 1 connection.
> ---8<---

Okay, I will post a patch later.

Best regards,
Zhongqiu