tcp: fix handling of stale syncookies timestamps
Subsystems (3)
v2 cover tcp: fix handling of stale syncookies timestamps
The synflood timestamps (->ts_recent_stamp and ->synq_overflow_ts) are only refreshed when the syncookie protection triggers. Therefore, their value can become very far apart from jiffies if no synflood happens for a long time.
If jiffies grows too much and wraps while the synflood timestamp isn't refreshed, then time_after32() might consider the later to be in the future. This can trick tcp_synq_no_recent_overflow() into returning erroneous values and rejecting valid ACKs.
Patch 1 handles the case of ACKs using legitimate syncookies. Patch 2 handles the case of stray ACKs.
Changes from v1:
- Initialising timestamps at socket creation time is not enough because jiffies wraps in 24 days with HZ=1000 (Eric Dumazet). Handle stale timestamps in tcp_synq_overflow() and tcp_synq_no_recent_overflow() instead.
- Rework commit description.
- Add a second patch to handle the case of stray ACKs.
Guillaume Nault (2):
tcp: fix rejected syncookies due to stale timestamps
tcp: tighten acceptance of ACKs not matching a child socket
include/net/tcp.h | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)
Signature · 4 lines
@DaveM, I'm sending both patches in one series as they logically fit together, although patch 2 is arguably a performance optimisation. I can drop it from the series and repost it when net-next reopens if you prefer. Although that'd make the link between the two less obvious.
PATCH v2 1/2 tcp: fix rejected syncookies due to stale timestamps
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.
--- include/net/tcp.h | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
include/net/tcp.h +4 -2
@@ -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; }
Signature · 1 line
2.21.0
PATCH v2 2/2 tcp: tighten acceptance of ACKs not matching a child socket
In reply to Guillaume NaultWhen no synflood occurs, the synflood timestamp isn't updated. Therefore it can be so old that time_after32() can consider it to be in the future.
That's a problem for tcp_synq_no_recent_overflow() as it may report that a recent overflow occurred while, in fact, it's just that jiffies has grown past 'last_overflow' + TCP_SYNCOOKIE_VALID + 2^31.
Spurious detection of recent overflows lead to extra syncookie verification in cookie_v[46]_check(). At that point, the verification should fail and the packet dropped. But we should have dropped the packet earlier as we didn't even send a syncookie.
Let's refine tcp_synq_no_recent_overflow() to report a recent overflow only if jiffies is within the
[last_overflow, last_overflow + TCP_SYNCOOKIE_VALID] interval. This way, no spurious recent overflow is reported when jiffies wraps and 'last_overflow' becomes in the future from the point of view of time_after32().
However, if jiffies wraps and enters the
[last_overflow, last_overflow + TCP_SYNCOOKIE_VALID] interval (with 'last_overflow' being a stale synflood timestamp), then tcp_synq_no_recent_overflow() still erroneously reports an overflow. In such cases, we have to rely on syncookie verification to drop the packet. We unfortunately have no way to differentiate between a fresh and a stale syncookie timestamp.
--- include/net/tcp.h | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
include/net/tcp.h +4 -2
@@ -520,12 +520,14 @@ static inline bool tcp_synq_no_recent_overflow(const struct sock *sk)if (likely (reuse)) { last_overflow =READ_ONCE (reuse->synq_overflow_ts);return time_after32 (now, last_overflow + -TCP_SYNCOOKIE_VALID ); +TCP_SYNCOOKIE_VALID ) || +time_before32 (now, last_overflow); } } last_overflow =tcp_sk (sk)->rx_opt.ts_recent_stamp; -return time_after32 (now, last_overflow +TCP_SYNCOOKIE_VALID ); +return time_after32 (now, last_overflow +TCP_SYNCOOKIE_VALID ) || +time_before32 (now, last_overflow); }static inline u32 tcp_cookie_time (void )
Signature · 1 line
2.21.0
Re: [PATCH net v2 2/2] tcp: tighten acceptance of ACKs not matching a child socket
Quoted 42 lines · Guillaume Nault
When no synflood occurs, the synflood timestamp isn't updated. Therefore it can be so old that
time_after32()can consider it to be in the future.That's a problem for
tcp_synq_no_recent_overflow()as it may report that a recent overflow occurred while, in fact, it's just that jiffies has grown past 'last_overflow' +TCP_SYNCOOKIE_VALID+ 2^31.Spurious detection of recent overflows lead to extra syncookie verification in cookie_v[46]_check(). At that point, the verification should fail and the packet dropped. But we should have dropped the packet earlier as we didn't even send a syncookie.
Let's refine
tcp_synq_no_recent_overflow()to report a recent overflow only if jiffies is within the
[last_overflow,last_overflow+TCP_SYNCOOKIE_VALID] interval. This way, no spurious recent overflow is reported when jiffies wraps and 'last_overflow' becomes in the future from the point of view oftime_after32().However, if jiffies wraps and enters the
[last_overflow,last_overflow+TCP_SYNCOOKIE_VALID] interval (with 'last_overflow' being a stale synflood timestamp), thentcp_synq_no_recent_overflow()still erroneously reports an overflow. In such cases, we have to rely on syncookie verification to drop the packet. We unfortunately have no way to differentiate between a fresh and a stale syncookie timestamp.Signed-off-by Guillaume Nault <[email protected]>--- include/net/tcp.h | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)include/net/tcp.h +0
-1@@ -520,12 +520,14 @@ static inline bool tcp_synq_no_recent_overflow(const struct sock *sk)if (likely (reuse)) { last_overflow =READ_ONCE (reuse->synq_overflow_ts); return time_after32(now, last_overflow + -TCP_SYNCOOKIE_VALID );
include/net/tcp.h static inline bool tcp_synq_no_recent_overflow(const struct sock *sk) · line 531
Quoted 10 lines
include/net/tcp.h +4
-1+ TCP_SYNCOOKIE_VALID) || +time_before32 (now, last_overflow); } } last_overflow =tcp_sk (sk)->rx_opt.ts_recent_stamp; -return time_after32 (now, last_overflow +TCP_SYNCOOKIE_VALID ); +return time_after32 (now, last_overflow +TCP_SYNCOOKIE_VALID ) || +time_before32 (now, last_overflow); }
There is a race I believe here.
CPU1 CPU2
now = jiffies.
...
jiffies++
...
SYN received, last_overflow is updated to the new jiffies.CPU1
timer_before32(now, last_overflow) is true, because last_overflow was set to now+1
I suggest some cushion here.
Also we TCP uses between() macro, we might add a time_between32(a, b, c) macro to ease code review.
->
return !time_between32(last_overflow - HZ, now, last_overflow + TCP_SYNCOOKIE_VALID);
Re: [PATCH net v2 2/2] tcp: tighten acceptance of ACKs not matching a child socket
Quoted 68 lines · Eric Dumazet
Quoted 42 lines · Guillaume Nault
When no synflood occurs, the synflood timestamp isn't updated. Therefore it can be so old that
time_after32()can consider it to be in the future.That's a problem for
tcp_synq_no_recent_overflow()as it may report that a recent overflow occurred while, in fact, it's just that jiffies has grown past 'last_overflow' +TCP_SYNCOOKIE_VALID+ 2^31.Spurious detection of recent overflows lead to extra syncookie verification in cookie_v[46]_check(). At that point, the verification should fail and the packet dropped. But we should have dropped the packet earlier as we didn't even send a syncookie.
Let's refine
tcp_synq_no_recent_overflow()to report a recent overflow only if jiffies is within the
[last_overflow,last_overflow+TCP_SYNCOOKIE_VALID] interval. This way, no spurious recent overflow is reported when jiffies wraps and 'last_overflow' becomes in the future from the point of view oftime_after32().However, if jiffies wraps and enters the
[last_overflow,last_overflow+TCP_SYNCOOKIE_VALID] interval (with 'last_overflow' being a stale synflood timestamp), thentcp_synq_no_recent_overflow()still erroneously reports an overflow. In such cases, we have to rely on syncookie verification to drop the packet. We unfortunately have no way to differentiate between a fresh and a stale syncookie timestamp.Signed-off-by Guillaume Nault <[email protected]>--- include/net/tcp.h | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)include/net/tcp.h +0
-1@@ -520,12 +520,14 @@ static inline bool tcp_synq_no_recent_overflow(const struct sock *sk)if (likely (reuse)) { last_overflow =READ_ONCE (reuse->synq_overflow_ts); return time_after32(now, last_overflow + -TCP_SYNCOOKIE_VALID );include/net/tcp.h static inline bool tcp_synq_no_recent_overflow(const struct sock *sk) · line 531
Quoted 10 lines
include/net/tcp.h +4
-1+ TCP_SYNCOOKIE_VALID) || +time_before32 (now, last_overflow); } } last_overflow =tcp_sk (sk)->rx_opt.ts_recent_stamp; -return time_after32 (now, last_overflow +TCP_SYNCOOKIE_VALID ); +return time_after32 (now, last_overflow +TCP_SYNCOOKIE_VALID ) || +time_before32 (now, last_overflow); }There is a race I believe here.
CPU1 CPU2
now = jiffies.
... jiffies++ ... SYN received, last_overflow is updated to the new jiffies.
Quoted 6 lines
CPU1
timer_before32(now, last_overflow)is true, becauselast_overflowwas set to now+1I suggest some cushion here.
Yes, we should wrap access to ->rx_opt.ts_recent_stamp into READ_ONCE(), to ensure that last_overflow won't be reloaded between the time_after32() and the time_before32() calls. Is that what you had in mind?
- last_overflow = tcp_sk(sk)->rx_opt.ts_recent_stamp;
+ last_overflow = READ_ONCE(tcp_sk(sk)->rx_opt.ts_recent_stamp);
Patch 1 would need the same fix BTW.
Quoted 3 lines
Also we TCP uses
between()macro, we might add atime_between32(a, b, c)macro to ease code review.
I didn't realise that. I'll define it in v3.
Quoted 3 lines
->
return !time_between32(last_overflow - HZ, now,last_overflow+TCP_SYNCOOKIE_VALID);
'last_overflow - HZ'? I don't get why we'd take HZ into account here.
Re: [PATCH net v2 2/2] tcp: tighten acceptance of ACKs not matching a child socket
Quoted 89 lines · Guillaume Nault
Quoted 68 lines · Eric Dumazet
Quoted 42 lines · Guillaume Nault
When no synflood occurs, the synflood timestamp isn't updated. Therefore it can be so old that
time_after32()can consider it to be in the future.That's a problem for
tcp_synq_no_recent_overflow()as it may report that a recent overflow occurred while, in fact, it's just that jiffies has grown past 'last_overflow' +TCP_SYNCOOKIE_VALID+ 2^31.Spurious detection of recent overflows lead to extra syncookie verification in cookie_v[46]_check(). At that point, the verification should fail and the packet dropped. But we should have dropped the packet earlier as we didn't even send a syncookie.
Let's refine
tcp_synq_no_recent_overflow()to report a recent overflow only if jiffies is within the
[last_overflow,last_overflow+TCP_SYNCOOKIE_VALID] interval. This way, no spurious recent overflow is reported when jiffies wraps and 'last_overflow' becomes in the future from the point of view oftime_after32().However, if jiffies wraps and enters the
[last_overflow,last_overflow+TCP_SYNCOOKIE_VALID] interval (with 'last_overflow' being a stale synflood timestamp), thentcp_synq_no_recent_overflow()still erroneously reports an overflow. In such cases, we have to rely on syncookie verification to drop the packet. We unfortunately have no way to differentiate between a fresh and a stale syncookie timestamp.Signed-off-by Guillaume Nault <[email protected]>--- include/net/tcp.h | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)include/net/tcp.h +0
-1@@ -520,12 +520,14 @@ static inline bool tcp_synq_no_recent_overflow(const struct sock *sk)if (likely (reuse)) { last_overflow =READ_ONCE (reuse->synq_overflow_ts); return time_after32(now, last_overflow + -TCP_SYNCOOKIE_VALID );include/net/tcp.h static inline bool tcp_synq_no_recent_overflow(const struct sock *sk) · line 531
Quoted 10 lines
include/net/tcp.h +4
-1+ TCP_SYNCOOKIE_VALID) || +time_before32 (now, last_overflow); } } last_overflow =tcp_sk (sk)->rx_opt.ts_recent_stamp; -return time_after32 (now, last_overflow +TCP_SYNCOOKIE_VALID ); +return time_after32 (now, last_overflow +TCP_SYNCOOKIE_VALID ) || +time_before32 (now, last_overflow); }There is a race I believe here.
CPU1 CPU2
now = jiffies.
... jiffies++ ... SYN received, last_overflow is updated to the new jiffies.Quoted 6 lines
CPU1
timer_before32(now, last_overflow)is true, becauselast_overflowwas set to now+1I suggest some cushion here.
Yes, we should wrap access to ->rx_opt.ts_recent_stamp into
READ_ONCE(), to ensure thatlast_overflowwon't be reloaded between thetime_after32()and thetime_before32()calls. Is that what you had in mind?(quoted diff) +1
-1- last_overflow = tcp_sk(sk)->rx_opt.ts_recent_stamp; + last_overflow = READ_ONCE(tcp_sk(sk)->rx_opt.ts_recent_stamp);Patch 1 would need the same fix BTW.
Quoted 3 lines
Also we TCP uses
between()macro, we might add atime_between32(a, b, c)macro to ease code review.
Quoted 7 lines
I didn't realise that. I'll define it in v3.
Quoted 3 lines
->
return !time_between32(last_overflow - HZ, now,last_overflow+TCP_SYNCOOKIE_VALID);'
last_overflow- HZ'? I don't get why we'd take HZ into account here.
Please read carefuly my prior feedback.
Even with READ_ONCE(), you still have a race.
CPU1 CPU2
now = jiffies.
<some long interrupt>
jiffies++ (or jiffies += 3 or 4
if CPU1 has been interrupted by a long interrupt)
...
SYN received, last_overflow is
updated to the new jiffies.
CPU1
@now still has a stale values (an old jiffies value) timer_before32(now, last_overflow) is true, because last_overflow was
set to now+1 (or now + 2 or now + 3)
Re: [PATCH net v2 2/2] tcp: tighten acceptance of ACKs not matching a child socket
Quoted 114 lines · Eric Dumazet
Quoted 89 lines · Guillaume Nault
Quoted 68 lines · Eric Dumazet
Quoted 42 lines · Guillaume Nault
When no synflood occurs, the synflood timestamp isn't updated. Therefore it can be so old that
time_after32()can consider it to be in the future.That's a problem for
tcp_synq_no_recent_overflow()as it may report that a recent overflow occurred while, in fact, it's just that jiffies has grown past 'last_overflow' +TCP_SYNCOOKIE_VALID+ 2^31.Spurious detection of recent overflows lead to extra syncookie verification in cookie_v[46]_check(). At that point, the verification should fail and the packet dropped. But we should have dropped the packet earlier as we didn't even send a syncookie.
Let's refine
tcp_synq_no_recent_overflow()to report a recent overflow only if jiffies is within the
[last_overflow,last_overflow+TCP_SYNCOOKIE_VALID] interval. This way, no spurious recent overflow is reported when jiffies wraps and 'last_overflow' becomes in the future from the point of view oftime_after32().However, if jiffies wraps and enters the
[last_overflow,last_overflow+TCP_SYNCOOKIE_VALID] interval (with 'last_overflow' being a stale synflood timestamp), thentcp_synq_no_recent_overflow()still erroneously reports an overflow. In such cases, we have to rely on syncookie verification to drop the packet. We unfortunately have no way to differentiate between a fresh and a stale syncookie timestamp.Signed-off-by Guillaume Nault <[email protected]>--- include/net/tcp.h | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)include/net/tcp.h +0
-1@@ -520,12 +520,14 @@ static inline bool tcp_synq_no_recent_overflow(const struct sock *sk)if (likely (reuse)) { last_overflow =READ_ONCE (reuse->synq_overflow_ts); return time_after32(now, last_overflow + -TCP_SYNCOOKIE_VALID );include/net/tcp.h static inline bool tcp_synq_no_recent_overflow(const struct sock *sk) · line 531
Quoted 10 lines
include/net/tcp.h +4
-1+ TCP_SYNCOOKIE_VALID) || +time_before32 (now, last_overflow); } } last_overflow =tcp_sk (sk)->rx_opt.ts_recent_stamp; -return time_after32 (now, last_overflow +TCP_SYNCOOKIE_VALID ); +return time_after32 (now, last_overflow +TCP_SYNCOOKIE_VALID ) || +time_before32 (now, last_overflow); }There is a race I believe here.
CPU1 CPU2
now = jiffies.
... jiffies++ ... SYN received, last_overflow is updated to the new jiffies.Quoted 6 lines
CPU1
timer_before32(now, last_overflow)is true, becauselast_overflowwas set to now+1I suggest some cushion here.
Yes, we should wrap access to ->rx_opt.ts_recent_stamp into
READ_ONCE(), to ensure thatlast_overflowwon't be reloaded between thetime_after32()and thetime_before32()calls. Is that what you had in mind?(quoted diff) +1
-1- last_overflow = tcp_sk(sk)->rx_opt.ts_recent_stamp; + last_overflow = READ_ONCE(tcp_sk(sk)->rx_opt.ts_recent_stamp);Patch 1 would need the same fix BTW.
Quoted 3 lines
Also we TCP uses
between()macro, we might add atime_between32(a, b, c)macro to ease code review.Quoted 7 lines
I didn't realise that. I'll define it in v3.
Quoted 3 lines
->
return !time_between32(last_overflow - HZ, now,last_overflow+TCP_SYNCOOKIE_VALID);'
last_overflow- HZ'? I don't get why we'd take HZ into account here.Please read carefuly my prior feedback.
Even with
READ_ONCE(), you still have a race.CPU1 CPU2
now = jiffies.
<some long interrupt>jiffies++ (or jiffies += 3 or 4if CPU1 has been interrupted by a long interrupt)
...
SYN received,last_overflowis
updated to the new jiffies.
Quoted 6 lines
CPU1
@now still has a stale values (an old jiffies value)
timer_before32(now, last_overflow)is true, becauselast_overflowwas
set to now+1 (or now + 2 or now + 3)
Ok, I get it now. Thanks!
Will send v3 using 'last_overflow - HZ' as lower bound.
I think READ_ONCE()/WRITE_ONCE() are still necessary to prevent reloading and imaginary write of last_overflow. At least that's my understanding after reading memory-barriers.txt again.
Re: [PATCH net v2 2/2] tcp: tighten acceptance of ACKs not matching a child socket
Quoted 8 lines · Guillaume Nault
Ok, I get it now. Thanks!
Will send v3 using 'last_overflow- HZ' as lower bound.I think READ_ONCE()/WRITE_ONCE() are still necessary to prevent reloading and imaginary write of
last_overflow. At least that's my understanding after reading memory-barriers.txt again.
Sure, it can not hurt, but please make this as a separate patch, thanks !