[PATCH] tcp: Fix tcp_max_syn_backlog limit on connection requests

[PATCH] tcp: Fix tcp_max_syn_backlog limit on connection requests

[Ttttabcd]

In the original logic of tcp_conn_request, the backlog parameter of the
listen system call and net.ipv4.tcp_max_syn_backlog are independent of
each other, which causes some confusion in the processing.

The backlog determines the maximum length of request_sock_queue, hereafter
referred to as backlog.

In the original design, if syn_cookies is not turned on, a quarter of
tcp_max_syn_backlog will be reserved for clients that have proven to
exist, mitigating syn attacks.

Suppose now that tcp_max_syn_backlog is 1000, but the backlog is only 200,
then 1000 >> 2 = 250, the backlog is used up by the syn attack, and the
above mechanism will not work.

Is tcp_max_syn_backlog used to limit the
maximum length of request_sock_queue?

Now suppose sycookie is enabled, backlog is 1000, and tcp_max_syn_backlog
is only 200. In this case tcp_max_syn_backlog will be useless.

Because syn_cookies is enabled, the tcp_max_syn_backlog logic will
be ignored, and the length of request_sock_queue will exceed
tcp_max_syn_backlog until the backlog.

I modified the original logic and set the minimum value in backlog and
tcp_max_syn_backlog as the maximum length limit of request_sock_queue.

Now there is only a unified limit.

The maximum length limit variable is "max_syn_backlog".

Use syn_cookies whenever max_syn_backlog is exceeded.

If syn_cookies is not enabled, a quarter of the max_syn_backlog queue is
reserved for hosts that have proven to exist.

In any case, request_sock_queue will not exceed max_syn_backlog.
When syn_cookies is not turned on, a quarter of the queue retention
will not be preempted.

Signed-off-by: AK Deng <ttttabcd@protonmail.com>
---
 net/ipv4/tcp_input.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 88b987ca9ebb..8f7e12844ae7 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -6562,14 +6562,18 @@ int tcp_conn_request(struct request_sock_ops *rsk_ops,
 	struct request_sock *req;
 	bool want_cookie = false;
 	struct dst_entry *dst;
+	u32 max_syn_backlog;
 	struct flowi fl;

+	max_syn_backlog = min_t(u32, net->ipv4.sysctl_max_syn_backlog,
+			      sk->sk_max_ack_backlog);
+
 	/* TW buckets are converted to open requests without
 	 * limitations, they conserve resources and peer is
 	 * evidently real one.
 	 */
 	if ((net->ipv4.sysctl_tcp_syncookies == 2 ||
-	     inet_csk_reqsk_queue_is_full(sk)) && !isn) {
+	     inet_csk_reqsk_queue_len(sk) >= max_syn_backlog) && !isn) {
 		want_cookie = tcp_syn_flood_action(sk, rsk_ops->slab_name);
 		if (!want_cookie)
 			goto drop;
@@ -6621,8 +6625,8 @@ int tcp_conn_request(struct request_sock_ops *rsk_ops,
 	if (!want_cookie && !isn) {
 		/* Kill the following clause, if you dislike this way. */
 		if (!net->ipv4.sysctl_tcp_syncookies &&
-		    (net->ipv4.sysctl_max_syn_backlog - inet_csk_reqsk_queue_len(sk) <
-		     (net->ipv4.sysctl_max_syn_backlog >> 2)) &&
+		    (max_syn_backlog - inet_csk_reqsk_queue_len(sk) <
+		     (max_syn_backlog >> 2)) &&
 		    !tcp_peer_is_proven(req, dst)) {
 			/* Without syncookies last quarter of
 			 * backlog is filled with destinations,
--
2.24.0

Re: [PATCH] tcp: Fix tcp_max_syn_backlog limit on connection requests

[David Miller]

From: Ttttabcd <ttttabcd@protonmail.com>
Date: Tue, 31 Dec 2019 01:21:47 +0000

> In the original logic of tcp_conn_request, the backlog parameter of the
> listen system call and net.ipv4.tcp_max_syn_backlog are independent of
> each other, which causes some confusion in the processing.
> 
> The backlog determines the maximum length of request_sock_queue, hereafter
> referred to as backlog.
> 
> In the original design, if syn_cookies is not turned on, a quarter of
> tcp_max_syn_backlog will be reserved for clients that have proven to
> exist, mitigating syn attacks.
> 
> Suppose now that tcp_max_syn_backlog is 1000, but the backlog is only 200,
> then 1000 >> 2 = 250, the backlog is used up by the syn attack, and the
> above mechanism will not work.
> 
> Is tcp_max_syn_backlog used to limit the
> maximum length of request_sock_queue?
> 
> Now suppose sycookie is enabled, backlog is 1000, and tcp_max_syn_backlog
> is only 200. In this case tcp_max_syn_backlog will be useless.
> 
> Because syn_cookies is enabled, the tcp_max_syn_backlog logic will
> be ignored, and the length of request_sock_queue will exceed
> tcp_max_syn_backlog until the backlog.
> 
> I modified the original logic and set the minimum value in backlog and
> tcp_max_syn_backlog as the maximum length limit of request_sock_queue.
> 
> Now there is only a unified limit.
> 
> The maximum length limit variable is "max_syn_backlog".
> 
> Use syn_cookies whenever max_syn_backlog is exceeded.
> 
> If syn_cookies is not enabled, a quarter of the max_syn_backlog queue is
> reserved for hosts that have proven to exist.
> 
> In any case, request_sock_queue will not exceed max_syn_backlog.
> When syn_cookies is not turned on, a quarter of the queue retention
> will not be preempted.
> 
> Signed-off-by: AK Deng <ttttabcd@protonmail.com>

On the surface this looks fine to me, but I'll give Eric a chance to
review and give feedback.

Thank you.

Re: [PATCH] tcp: Fix tcp_max_syn_backlog limit on connection requests

[Ttttabcd]

> On the surface this looks fine to me, but I'll give Eric a chance to
> review and give feedback.
>
> Thank you.

okay, thank you

Re: [PATCH] tcp: Fix tcp_max_syn_backlog limit on connection requests

[Eric Dumazet]

On Mon, Dec 30, 2019 at 5:21 PM Ttttabcd <ttttabcd@protonmail.com> wrote:
>
> In the original logic of tcp_conn_request, the backlog parameter of the
> listen system call and net.ipv4.tcp_max_syn_backlog are independent of
> each other, which causes some confusion in the processing.
>
> The backlog determines the maximum length of request_sock_queue, hereafter
> referred to as backlog.
>
> In the original design, if syn_cookies is not turned on, a quarter of
> tcp_max_syn_backlog will be reserved for clients that have proven to
> exist, mitigating syn attacks.
>
> Suppose now that tcp_max_syn_backlog is 1000, but the backlog is only 200,
> then 1000 >> 2 = 250, the backlog is used up by the syn attack, and the
> above mechanism will not work.
>
> Is tcp_max_syn_backlog used to limit the
> maximum length of request_sock_queue?
>
> Now suppose sycookie is enabled, backlog is 1000, and tcp_max_syn_backlog
> is only 200. In this case tcp_max_syn_backlog will be useless.
>
> Because syn_cookies is enabled, the tcp_max_syn_backlog logic will
> be ignored, and the length of request_sock_queue will exceed
> tcp_max_syn_backlog until the backlog.
>
> I modified the original logic and set the minimum value in backlog and
> tcp_max_syn_backlog as the maximum length limit of request_sock_queue.
>
> Now there is only a unified limit.
>
> The maximum length limit variable is "max_syn_backlog".
>
> Use syn_cookies whenever max_syn_backlog is exceeded.
>
> If syn_cookies is not enabled, a quarter of the max_syn_backlog queue is
> reserved for hosts that have proven to exist.
>
> In any case, request_sock_queue will not exceed max_syn_backlog.
> When syn_cookies is not turned on, a quarter of the queue retention
> will not be preempted.
>
> Signed-off-by: AK Deng <ttttabcd@protonmail.com>
> ---
>  net/ipv4/tcp_input.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index 88b987ca9ebb..8f7e12844ae7 100644
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -6562,14 +6562,18 @@ int tcp_conn_request(struct request_sock_ops *rsk_ops,
>         struct request_sock *req;
>         bool want_cookie = false;
>         struct dst_entry *dst;
> +       u32 max_syn_backlog;
>         struct flowi fl;
>
> +       max_syn_backlog = min_t(u32, net->ipv4.sysctl_max_syn_backlog,
> +                             sk->sk_max_ack_backlog);
> +
>         /* TW buckets are converted to open requests without
>          * limitations, they conserve resources and peer is
>          * evidently real one.
>          */
>         if ((net->ipv4.sysctl_tcp_syncookies == 2 ||
> -            inet_csk_reqsk_queue_is_full(sk)) && !isn) {
> +            inet_csk_reqsk_queue_len(sk) >= max_syn_backlog) && !isn) {
>                 want_cookie = tcp_syn_flood_action(sk, rsk_ops->slab_name);
>                 if (!want_cookie)
>                         goto drop;
> @@ -6621,8 +6625,8 @@ int tcp_conn_request(struct request_sock_ops *rsk_ops,
>         if (!want_cookie && !isn) {
>                 /* Kill the following clause, if you dislike this way. */
>                 if (!net->ipv4.sysctl_tcp_syncookies &&
> -                   (net->ipv4.sysctl_max_syn_backlog - inet_csk_reqsk_queue_len(sk) <
> -                    (net->ipv4.sysctl_max_syn_backlog >> 2)) &&

Note that the prior condition is using signed arithmetic.


> +                   (max_syn_backlog - inet_csk_reqsk_queue_len(sk) <
> +                    (max_syn_backlog >> 2)) &&
>                     !tcp_peer_is_proven(req, dst)) {
>                         /* Without syncookies last quarter of
>                          * backlog is filled with destinations,
> --
> 2.24.0

I would prefer not changing this code, unless you prove there is a real problem.

(sysctl_max_syn_backlog defauts to 4096, and syncookies are enabled by
default, for a good reason)

Basically, sysctl_max_syn_backlog is not used today (because
syncookies are enabled...)

Your change might break users that suddenly will get behavior changes
if their sysctl_max_syn_backlog was set to a small value.
Unfortunately  some sysctl values are often copied/pasted from various
web pages claiming how to get best TCP performance.

It would be quite silly to change the kernel to adapt a change
(sysctl_max_syn_backlog set to 200 ... ) done by one of these admins.

Thanks.

Re: [PATCH] tcp: Fix tcp_max_syn_backlog limit on connection requests

[Ttttabcd]

> I would prefer not changing this code, unless you prove there is a real problem.
>
> (sysctl_max_syn_backlog defauts to 4096, and syncookies are enabled by
> default, for a good reason)
>
> Basically, sysctl_max_syn_backlog is not used today (because
> syncookies are enabled...)
>
> Your change might break users that suddenly will get behavior changes
> if their sysctl_max_syn_backlog was set to a small value.
> Unfortunately some sysctl values are often copied/pasted from various
> web pages claiming how to get best TCP performance.
>
> It would be quite silly to change the kernel to adapt a change
> (sysctl_max_syn_backlog set to 200 ... ) done by one of these admins.
>
> Thanks.

Of course, the sysctl_max_syn_backlog is set to 200 just for the sake of example, not the actual configuration.

I found this bug when summarizing how the kernel handles syn attacks. I'm reading the kernel source code and not really encountering errors.

I also thought of another scenario where the above BUG might cause problems.

Imagine a machine with low performance and small memory. Set sysctl_max_syn_backlog to a small value to save memory (304 bytes for a connection request), and enable syn cookies to handle excessive requests.

Because the sysctl_max_syn_backlog is invalid after syn cookies are enabled, the entire backlog is consumed and too much memory is consumed.

Of course, the above scenario is rarely encountered in general.

I fixed this bug mainly because I thought its logic was indeed wrong, not because it caused some serious problems, I was a bit obsessive about the correctness of the code.

So if, as you said, it can cause backward compatibility issues, just leave it as it is.

Re: [PATCH] tcp: Fix tcp_max_syn_backlog limit on connection requests

[Ttttabcd]

However, I think that backward compatibility should not be too serious because sysctl_max_syn_backlog is only enabled when syn_cookies is turned off.

If sysctl_max_syn_backlog is set small, there is no difference between the original code and the new code.

Only in the BUG scenarios I mentioned in the patch, the system behavior will change, but these are corrections that have no impact on users.

It's just that the part of the request retention queue will not be mistakenly occupied, and earlier use of syn cookies instead of filling up the backlog.

Re: [PATCH] tcp: Fix tcp_max_syn_backlog limit on connection requests

[Eric Dumazet]

On Fri, Jan 3, 2020 at 5:19 AM Ttttabcd <ttttabcd@protonmail.com> wrote:
>
> However, I think that backward compatibility should not be too serious because sysctl_max_syn_backlog is only enabled when syn_cookies is turned off.
>

Yes, but not your new code :

       max_syn_backlog = min_t(u32, net->ipv4.sysctl_max_syn_backlog,
                             sk->sk_max_ack_backlog);


> If sysctl_max_syn_backlog is set small, there is no difference between the original code and the new code.

There is a difference though....

Set sysctl_max_syn_backlog to 1, and start a reasonable test (not a synflood)

As soon as one SYN_RECV request socket is inserted in the hash, other
SYN packets will generate a syncookie, even if the backlog of the
listener is 100,000

I think the intent of the code is to allow a heavy duty server to use
a huge backlog (say 1,000,000) to avoid syncookies if it has enough
RAM.

In this mode, people never had to tweak sysctl_max_syn_backlog.

>
> Only in the BUG scenarios I mentioned in the patch, the system behavior will change, but these are corrections that have no impact on users.
>
> It's just that the part of the request retention queue will not be mistakenly occupied, and earlier use of syn cookies instead of filling up the backlog.

Re: [PATCH] tcp: Fix tcp_max_syn_backlog limit on connection requests

[Ttttabcd]

> There is a difference though....
>
> Set sysctl_max_syn_backlog to 1, and start a reasonable test (not a synflood)
>
> As soon as one SYN_RECV request socket is inserted in the hash, other
> SYN packets will generate a syncookie, even if the backlog of the
> listener is 100,000
>
> I think the intent of the code is to allow a heavy duty server to use
> a huge backlog (say 1,000,000) to avoid syncookies if it has enough
> RAM.
>
> In this mode, people never had to tweak sysctl_max_syn_backlog.

Your scenario is the same as what I said in the previous email, big backlog, small sysctl_max_syn_backlog.

The question now is whether sysctl_max_syn_backlog should still work after syn cookies are turned on. Does the backlog want to be exhausted or kept unused? Whether to use as much memory as possible or keep it as low as possible.

This is also where I was most confused at first. This is totally inconsistent with ip-sysctl.txt. In some scenarios, sysctl_max_syn_backlog is invalid.

Unless we can find the person who wrote this code, we may not find answers to these questions.

This is, uh ... a problem left over by history, it seems that it can only remain the same.

Re: [PATCH] tcp: Fix tcp_max_syn_backlog limit on connection requests

[Ttttabcd]

If we don't modify the kernel code, I think we should modify ip-sysctl.txt to add that sysctl_max_syn_backlog is invalid after syn cookies are enabled.

The behavior in the kernel is completely inconsistent with ip-sysctl.txt, which is completely misleading.


somaxconn - INTEGER
	Limit of socket listen() backlog, known in userspace as SOMAXCONN.
	Defaults to 4096. (Was 128 before linux-5.4)
	See also tcp_max_syn_backlog for additional tuning for TCP sockets.

tcp_max_syn_backlog - INTEGER
	Maximal number of remembered connection requests (SYN_RECV),
	which have not received an acknowledgment from connecting client.
	This is a per-listener limit.
	The minimal value is 128 for low memory machines, and it will
	increase in proportion to the memory of machine.
	If server suffers from overload, try increasing this number.
	Remember to also check /proc/sys/net/core/somaxconn
	A SYN_RECV request socket consumes about 304 bytes of memory.

tcp_syncookies - BOOLEAN
	Only valid when the kernel was compiled with CONFIG_SYN_COOKIES
	Send out syncookies when the syn backlog queue of a socket
	overflows. This is to prevent against the common 'SYN flood attack'
	Default: 1

	Note, that syncookies is fallback facility.
	It MUST NOT be used to help highly loaded servers to stand
	against legal connection rate. If you see SYN flood warnings
	in your logs, but investigation	shows that they occur
	because of overload with legal connections, you should tune
	another parameters until this warning disappear.
	See: tcp_max_syn_backlog, tcp_synack_retries, tcp_abort_on_overflow.

	syncookies seriously violate TCP protocol, do not allow
	to use TCP extensions, can result in serious degradation
	of some services (f.e. SMTP relaying), visible not by you,
	but your clients and relays, contacting you. While you see
	SYN flood warnings in logs not being really flooded, your server
	is seriously misconfigured.

	If you want to test which effects syncookies have to your
	network connections you can set this knob to 2 to enable
	unconditionally generation of syncookies.

Re: [PATCH] tcp: Fix tcp_max_syn_backlog limit on connection requests

[Ttttabcd]

So now is it sure not to modify the kernel code?

If so, I plan to modify ip-sysctl.txt and submit a new patch.

Re: [PATCH] tcp: Fix tcp_max_syn_backlog limit on connection requests

[Ttttabcd]

> You have to post your updated patch as a new list posting, not as a reply
> to an existing posting.
>
> Thank you.

okay, thank you

Re: [PATCH] tcp: Fix tcp_max_syn_backlog limit on connection requests

[David Miller]

From: Ttttabcd <ttttabcd@protonmail.com>
Date: Tue, 31 Dec 2019 00:22:44 +0000

> I have fixed this issue in the previous email, using min_t

You have to post your updated patch as a new list posting, not as a reply
to an existing posting.

Thank you.

Re: [PATCH] tcp: Fix tcp_max_syn_backlog limit on connection requests

[Ttttabcd]

I have fixed this issue in the previous email, using min_t

Re: [PATCH] tcp: Fix tcp_max_syn_backlog limit on connection requests

[kbuild test robot]

Hi Ttttabcd,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on net/master]
[also build test WARNING on net-next/master ipvs/master v5.5-rc4 next-20191220]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Ttttabcd/tcp-Fix-tcp_max_syn_backlog-limit-on-connection-requests/20191230-164004
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git bb3d0b8bf5be61ab1d6f472c43cbf34de17e796b
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.1-129-g341daf20-dirty
        make ARCH=x86_64 allmodconfig
        make C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)

>> net/ipv4/tcp_input.c:6568:27: sparse: sparse: incompatible types in comparison expression (different signedness):
>> net/ipv4/tcp_input.c:6568:27: sparse:    int *
>> net/ipv4/tcp_input.c:6568:27: sparse:    unsigned int *
   net/ipv4/tcp_input.c:6673:17: sparse: sparse: context imbalance in 'tcp_conn_request' - unexpected unlock

vim +6568 net/ipv4/tcp_input.c

  6551	
  6552	int tcp_conn_request(struct request_sock_ops *rsk_ops,
  6553			     const struct tcp_request_sock_ops *af_ops,
  6554			     struct sock *sk, struct sk_buff *skb)
  6555	{
  6556		struct tcp_fastopen_cookie foc = { .len = -1 };
  6557		__u32 isn = TCP_SKB_CB(skb)->tcp_tw_isn;
  6558		struct tcp_options_received tmp_opt;
  6559		struct tcp_sock *tp = tcp_sk(sk);
  6560		struct net *net = sock_net(sk);
  6561		struct sock *fastopen_sk = NULL;
  6562		struct request_sock *req;
  6563		bool want_cookie = false;
  6564		struct dst_entry *dst;
  6565		int max_syn_backlog;
  6566		struct flowi fl;
  6567	
> 6568		max_syn_backlog = min(net->ipv4.sysctl_max_syn_backlog,

---
0-DAY kernel test infrastructure                 Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation

RE: [PATCH] tcp: Fix tcp_max_syn_backlog limit on connection requests

[Ttttabcd]

In the original logic of tcp_conn_request, the backlog parameter of the
listen system call and net.ipv4.tcp_max_syn_backlog are independent of
each other, which causes some confusion in the processing.

The backlog determines the maximum length of request_sock_queue, hereafter
referred to as backlog.

In the original design, if syn_cookies is not turned on, a quarter of
tcp_max_syn_backlog will be reserved for clients that have proven to
exist, mitigating syn attacks.

Suppose now that tcp_max_syn_backlog is 1000, but the backlog is only 200,
then 1000 >> 2 = 250, the backlog is used up by the syn attack, and the
above mechanism will not work.

Is tcp_max_syn_backlog used to limit the
maximum length of request_sock_queue?

Now suppose sycookie is enabled, backlog is 1000, and tcp_max_syn_backlog
is only 200. In this case tcp_max_syn_backlog will be useless.

Because syn_cookies is enabled, the tcp_max_syn_backlog logic will
be ignored, and the length of request_sock_queue will exceed
tcp_max_syn_backlog until the backlog.

I modified the original logic and set the minimum value in backlog and
tcp_max_syn_backlog as the maximum length limit of request_sock_queue.

Now there is only a unified limit.

The maximum length limit variable is "max_syn_backlog".

Use syn_cookies whenever max_syn_backlog is exceeded.

If syn_cookies is not enabled, a quarter of the max_syn_backlog queue is
reserved for hosts that have proven to exist.

In any case, request_sock_queue will not exceed max_syn_backlog.
When syn_cookies is not turned on, a quarter of the queue retention
will not be preempted.

Signed-off-by: ak <ak@localhost.com>
---
 net/ipv4/tcp_input.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 88b987ca9ebb..8f7e12844ae7 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -6562,14 +6562,18 @@ int tcp_conn_request(struct request_sock_ops *rsk_ops,
 	struct request_sock *req;
 	bool want_cookie = false;
 	struct dst_entry *dst;
+	u32 max_syn_backlog;
 	struct flowi fl;

+	max_syn_backlog = min_t(u32, net->ipv4.sysctl_max_syn_backlog,
+			      sk->sk_max_ack_backlog);
+
 	/* TW buckets are converted to open requests without
 	 * limitations, they conserve resources and peer is
 	 * evidently real one.
 	 */
 	if ((net->ipv4.sysctl_tcp_syncookies == 2 ||
-	     inet_csk_reqsk_queue_is_full(sk)) && !isn) {
+	     inet_csk_reqsk_queue_len(sk) >= max_syn_backlog) && !isn) {
 		want_cookie = tcp_syn_flood_action(sk, rsk_ops->slab_name);
 		if (!want_cookie)
 			goto drop;
@@ -6621,8 +6625,8 @@ int tcp_conn_request(struct request_sock_ops *rsk_ops,
 	if (!want_cookie && !isn) {
 		/* Kill the following clause, if you dislike this way. */
 		if (!net->ipv4.sysctl_tcp_syncookies &&
-		    (net->ipv4.sysctl_max_syn_backlog - inet_csk_reqsk_queue_len(sk) <
-		     (net->ipv4.sysctl_max_syn_backlog >> 2)) &&
+		    (max_syn_backlog - inet_csk_reqsk_queue_len(sk) <
+		     (max_syn_backlog >> 2)) &&
 		    !tcp_peer_is_proven(req, dst)) {
 			/* Without syncookies last quarter of
 			 * backlog is filled with destinations,
--
2.24.0

RE: [PATCH] tcp: Fix tcp_max_syn_backlog limit on connection requests

[Jose Abreu]

From: Ttttabcd <ttttabcd@protonmail.com>
Date: Dec/30/2019, 12:03:51 (UTC+00:00)

> Uh ... I can not solve this warning, it's none of my business, but the kernel is also used elsewhere min (), and there is no problem.

You *can* and *must* solve the warning. You can use min_t for this.

---
Thanks,
Jose Miguel Abreu

Re: [PATCH] tcp: Fix tcp_max_syn_backlog limit on connection requests

[Ttttabcd]

> Hi Ttttabcd,
>
> Thank you for the patch! Perhaps something to improve:
>

> If you fix the issue, kindly add following tag
> Reported-by: kbuild test robot lkp@intel.com
>
> All warnings (new ones prefixed by >>):
>
> In file included from include/asm-generic/bug.h:19:0,
> from ./arch/um/include/generated/asm/bug.h:1,
> from include/linux/bug.h:5,
> from include/linux/mmdebug.h:5,
> from include/linux/mm.h:9,
> from net/ipv4/tcp_input.c:67:
> net/ipv4/tcp_input.c: In function 'tcp_conn_request':
> include/linux/kernel.h:844:29: warning: comparison of distinct pointer types lacks a cast
> (!!(sizeof((typeof(x) *)1 == (typeof(y) *)1)))
> ^
> include/linux/kernel.h:858:4: note: in expansion of macro '__typecheck'
> (__typecheck(x, y) && __no_side_effects(x, y))
> ^~~~~~~~~~~
> include/linux/kernel.h:868:24: note: in expansion of macro '__safe_cmp'
> __builtin_choose_expr(__safe_cmp(x, y), \
> ^~~~~~~~~~
> include/linux/kernel.h:877:19: note: in expansion of macro '__careful_cmp'
> #define min(x, y) __careful_cmp(x, y, <)
> ^~~~~~~~~~~~~
>
> > > net/ipv4/tcp_input.c:6568:20: note: in expansion of macro 'min'
>
>      max_syn_backlog = min(net->ipv4.sysctl_max_syn_backlog,
>
>                        ^~~

Uh ... I can not solve this warning, it's none of my business, but the kernel is also used elsewhere min (), and there is no problem.

Re: [PATCH] tcp: Fix tcp_max_syn_backlog limit on connection requests

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3066 bytes --]

Hi Ttttabcd,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on net/master]
[also build test WARNING on net-next/master ipvs/master v5.5-rc4 next-20191220]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Ttttabcd/tcp-Fix-tcp_max_syn_backlog-limit-on-connection-requests/20191230-164004
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git bb3d0b8bf5be61ab1d6f472c43cbf34de17e796b
config: um-x86_64_defconfig (attached as .config)
compiler: gcc-7 (Debian 7.5.0-3) 7.5.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=um SUBARCH=x86_64

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In file included from include/asm-generic/bug.h:19:0,
                    from ./arch/um/include/generated/asm/bug.h:1,
                    from include/linux/bug.h:5,
                    from include/linux/mmdebug.h:5,
                    from include/linux/mm.h:9,
                    from net/ipv4/tcp_input.c:67:
   net/ipv4/tcp_input.c: In function 'tcp_conn_request':
   include/linux/kernel.h:844:29: warning: comparison of distinct pointer types lacks a cast
      (!!(sizeof((typeof(x) *)1 == (typeof(y) *)1)))
                                ^
   include/linux/kernel.h:858:4: note: in expansion of macro '__typecheck'
      (__typecheck(x, y) && __no_side_effects(x, y))
       ^~~~~~~~~~~
   include/linux/kernel.h:868:24: note: in expansion of macro '__safe_cmp'
     __builtin_choose_expr(__safe_cmp(x, y), \
                           ^~~~~~~~~~
   include/linux/kernel.h:877:19: note: in expansion of macro '__careful_cmp'
    #define min(x, y) __careful_cmp(x, y, <)
                      ^~~~~~~~~~~~~
>> net/ipv4/tcp_input.c:6568:20: note: in expansion of macro 'min'
     max_syn_backlog = min(net->ipv4.sysctl_max_syn_backlog,
                       ^~~

vim +/min +6568 net/ipv4/tcp_input.c

  6551	
  6552	int tcp_conn_request(struct request_sock_ops *rsk_ops,
  6553			     const struct tcp_request_sock_ops *af_ops,
  6554			     struct sock *sk, struct sk_buff *skb)
  6555	{
  6556		struct tcp_fastopen_cookie foc = { .len = -1 };
  6557		__u32 isn = TCP_SKB_CB(skb)->tcp_tw_isn;
  6558		struct tcp_options_received tmp_opt;
  6559		struct tcp_sock *tp = tcp_sk(sk);
  6560		struct net *net = sock_net(sk);
  6561		struct sock *fastopen_sk = NULL;
  6562		struct request_sock *req;
  6563		bool want_cookie = false;
  6564		struct dst_entry *dst;
  6565		int max_syn_backlog;
  6566		struct flowi fl;
  6567	
> 6568		max_syn_backlog = min(net->ipv4.sysctl_max_syn_backlog,

---
0-DAY kernel test infrastructure                 Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 8440 bytes --]

[PATCH] tcp: Fix tcp_max_syn_backlog limit on connection requests

[Ttttabcd]

In the original logic of tcp_conn_request, the backlog parameter of the
listen system call and net.ipv4.tcp_max_syn_backlog are independent of
each other, which causes some confusion in the processing.

The backlog determines the maximum length of request_sock_queue, hereafter
referred to as backlog.

In the original design, if syn_cookies is not turned on, a quarter of
tcp_max_syn_backlog will be reserved for clients that have proven to
exist, mitigating syn attacks.

Suppose now that tcp_max_syn_backlog is 1000, but the backlog is only 200,
then 1000 >> 2 = 250, the backlog is used up by the syn attack, and the
above mechanism will not work.

Is tcp_max_syn_backlog used to limit the
maximum length of request_sock_queue?

Now suppose sycookie is enabled, backlog is 1000, and tcp_max_syn_backlog
is only 200. In this case tcp_max_syn_backlog will be useless.

Because syn_cookies is enabled, the tcp_max_syn_backlog logic will
be ignored, and the length of request_sock_queue will exceed
tcp_max_syn_backlog until the backlog.

I modified the original logic and set the minimum value in backlog and
tcp_max_syn_backlog as the maximum length limit of request_sock_queue.

Now there is only a unified limit.

The maximum length limit variable is "max_syn_backlog".

Use syn_cookies whenever max_syn_backlog is exceeded.

If syn_cookies is not enabled, a quarter of the max_syn_backlog queue is
reserved for hosts that have proven to exist.

In any case, request_sock_queue will not exceed max_syn_backlog.
When syn_cookies is not turned on, a quarter of the queue retention
will not be preempted.

Signed-off-by: AK Deng <ttttabcd@protonmail.com>
---
 net/ipv4/tcp_input.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 88b987ca9ebb..190edaaa9dce 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -6562,14 +6562,18 @@ int tcp_conn_request(struct request_sock_ops *rsk_ops,
 	struct request_sock *req;
 	bool want_cookie = false;
 	struct dst_entry *dst;
+	int max_syn_backlog;
 	struct flowi fl;

+	max_syn_backlog = min(net->ipv4.sysctl_max_syn_backlog,
+			      sk->sk_max_ack_backlog);
+
 	/* TW buckets are converted to open requests without
 	 * limitations, they conserve resources and peer is
 	 * evidently real one.
 	 */
 	if ((net->ipv4.sysctl_tcp_syncookies == 2 ||
-	     inet_csk_reqsk_queue_is_full(sk)) && !isn) {
+	     inet_csk_reqsk_queue_len(sk) >= max_syn_backlog) && !isn) {
 		want_cookie = tcp_syn_flood_action(sk, rsk_ops->slab_name);
 		if (!want_cookie)
 			goto drop;
@@ -6621,8 +6625,8 @@ int tcp_conn_request(struct request_sock_ops *rsk_ops,
 	if (!want_cookie && !isn) {
 		/* Kill the following clause, if you dislike this way. */
 		if (!net->ipv4.sysctl_tcp_syncookies &&
-		    (net->ipv4.sysctl_max_syn_backlog - inet_csk_reqsk_queue_len(sk) <
-		     (net->ipv4.sysctl_max_syn_backlog >> 2)) &&
+		    (max_syn_backlog - inet_csk_reqsk_queue_len(sk) <
+		     (max_syn_backlog >> 2)) &&
 		    !tcp_peer_is_proven(req, dst)) {
 			/* Without syncookies last quarter of
 			 * backlog is filled with destinations,
--
2.24.0