On Wed, Jan 1, 2025 at 9:53 AM Jason Xing [off-list ref] wrote:

On Tue, Dec 31, 2024 at 4:24 PM Zhongqiu Duan [off-list ref] wrote:

quoted

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.

quoted

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.

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().

Regards,
Zhongqiu