From: Guillaume Nault <gnault@redhat.com>
To: David Miller <davem@davemloft.net>,
Jakub Kicinski <jakub.kicinski@netronome.com>
Cc: netdev@vger.kernel.org, Eric Dumazet <edumazet@google.com>,
Arnd Bergmann <arnd@arndb.de>
Subject: [PATCH net v2 1/2] tcp: fix rejected syncookies due to stale timestamps
Date: Thu, 5 Dec 2019 01:59:06 +0100 [thread overview]
Message-ID: <c73c4f3f6a1793775b9ed2f67752b3386cbaa61b.1575503545.git.gnault@redhat.com> (raw)
In-Reply-To: <cover.1575503545.git.gnault@redhat.com>
If no synflood happens for a long enough period of time, then the
synflood timestamp isn't refreshed and jiffies can advance so much
that time_after32() can't accurately compare them any more.
Therefore, we can end up in a situation where time_after32(now,
last_overflow + HZ) returns false, just because these two values are
too far apart. In that case, the synflood timestamp isn't updated as
it should be, which can trick tcp_synq_no_recent_overflow() into
rejecting valid syncookies.
For example, let's consider the following scenario on a system
with HZ=1000:
* The synflood timestamp is 0, either because that's the timestamp
of the last synflood or, more commonly, because we're working with
a freshly created socket.
* We receive a new SYN, which triggers synflood protection. Let's say
that this happens when jiffies == 2147484649 (that is,
'synflood timestamp' + HZ + 2^31 + 1).
* Then tcp_synq_overflow() doesn't update the synflood timestamp,
because time_after32(2147484649, 1000) returns false.
With:
- 2147484649: the value of jiffies, aka. 'now'.
- 1000: the value of 'last_overflow' + HZ.
* A bit later, we receive the ACK completing the 3WHS. But
cookie_v[46]_check() rejects it because tcp_synq_no_recent_overflow()
says that we're not under synflood. That's because
time_after32(2147484649, 120000) returns false.
With:
- 2147484649: the value of jiffies, aka. 'now'.
- 120000: the value of 'last_overflow' + TCP_SYNCOOKIE_VALID.
Of course, in reality jiffies would have increased a bit, but this
condition will last for the next 119 seconds, which is far enough
to accommodate for jiffie's growth.
Fix this by updating the overflow timestamp whenever jiffies isn't
within the [last_overflow, last_overflow + HZ] range. That shouldn't
have any performance impact: only stale timestamps would trigger the
time_before32() case, and that can only happen for the first SYN of a
flood.
Now we're guaranteed to have fresh timestamps while under synflood, so
tcp_synq_no_recent_overflow() can safely use it with time_after32() in
such situations.
Stale timestamps can still make tcp_synq_no_recent_overflow() return
the wrong verdict when not under synflood. This will be handled in the
next patch.
For 64 bits architectures, the problem was introduced with the
conversion of ->tw_ts_recent_stamp to 32 bits integer by commit
cca9bab1b72c ("tcp: use monotonic timestamps for PAWS").
The problem has always been there on 32 bits architectures.
Fixes: cca9bab1b72c ("tcp: use monotonic timestamps for PAWS")
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Guillaume Nault <gnault@redhat.com>
---
include/net/tcp.h | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/include/net/tcp.h b/include/net/tcp.h
index 36f195fb576a..f0eae83ee555 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -494,14 +494,16 @@ static inline void tcp_synq_overflow(const struct sock *sk)
reuse = rcu_dereference(sk->sk_reuseport_cb);
if (likely(reuse)) {
last_overflow = READ_ONCE(reuse->synq_overflow_ts);
- if (time_after32(now, last_overflow + HZ))
+ if (time_after32(now, last_overflow + HZ) ||
+ time_before32(now, last_overflow))
WRITE_ONCE(reuse->synq_overflow_ts, now);
return;
}
}
last_overflow = tcp_sk(sk)->rx_opt.ts_recent_stamp;
- if (time_after32(now, last_overflow + HZ))
+ if (time_after32(now, last_overflow + HZ) ||
+ time_before32(now, last_overflow))
tcp_sk(sk)->rx_opt.ts_recent_stamp = now;
}
--
2.21.0
next prev parent reply other threads:[~2019-12-05 0:59 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-12-05 0:58 [PATCH net v2 0/2] tcp: fix handling of stale syncookies timestamps Guillaume Nault
2019-12-05 0:59 ` Guillaume Nault [this message]
2019-12-05 0:59 ` [PATCH net v2 2/2] tcp: tighten acceptance of ACKs not matching a child socket Guillaume Nault
2019-12-05 3:08 ` Eric Dumazet
2019-12-05 18:00 ` Guillaume Nault
2019-12-05 18:14 ` Eric Dumazet
2019-12-05 19:22 ` Guillaume Nault
2019-12-05 19:30 ` Eric Dumazet
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=c73c4f3f6a1793775b9ed2f67752b3386cbaa61b.1575503545.git.gnault@redhat.com \
--to=gnault@redhat.com \
--cc=arnd@arndb.de \
--cc=davem@davemloft.net \
--cc=edumazet@google.com \
--cc=jakub.kicinski@netronome.com \
--cc=netdev@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).