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 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. In practice, using last_overflow as lower bound is problematic. If the synflood timestamp is concurrently updated between the time we read jiffies and the moment we store the timestamp in 'last_overflow', then 'now' becomes smaller than 'last_overflow' and tcp_synq_no_recent_overflow() returns true, potentially dropping a valid syncookie. Reading jiffies after loading the timestamp could fix the problem, but that'd require a memory barrier. Let's just accommodate for potential timestamp growth instead and extend the interval using 'last_overflow - HZ' as lower bound.
Unified · Split
| @@ -518,13 +518,23 @@ static inline bool tcp_synq_no_recent_overflow(const struct sock *sk) · 517 unmodified lines | |||
| 518 | reuse = rcu_dereference(sk->sk_reuseport_cb); | 518 | reuse = rcu_dereference(sk->sk_reuseport_cb); |
| 519 | if (likely(reuse)) { | 519 | if (likely(reuse)) { |
| 520 | last_overflow = READ_ONCE(reuse->synq_overflow_ts); | 520 | last_overflow = READ_ONCE(reuse->synq_overflow_ts); |
| 521 | return time_after32(now, last_overflow + | 521 | return !time_between32(now, last_overflow - HZ, |
| 522 | TCP_SYNCOOKIE_VALID); | 522 | last_overflow + |
| 523 | TCP_SYNCOOKIE_VALID); | ||
| 523 | } | 524 | } |
| 524 | } | 525 | } |
| 525 | 526 | ||
| 526 | last_overflow = tcp_sk(sk)->rx_opt.ts_recent_stamp; | 527 | last_overflow = tcp_sk(sk)->rx_opt.ts_recent_stamp; |
| 527 | return time_after32(now, last_overflow + TCP_SYNCOOKIE_VALID); | 528 | |
| 529 | /* If last_overflow <= jiffies <= last_overflow + TCP_SYNCOOKIE_VALID, | ||
| 530 | * then we're under synflood. However, we have to use | ||
| 531 | * 'last_overflow - HZ' as lower bound. That's because a concurrent | ||
| 532 | * tcp_synq_overflow() could update .ts_recent_stamp after we read | ||
| 533 | * jiffies but before we store .ts_recent_stamp into last_overflow, | ||
| 534 | * which could lead to rejecting a valid syncookie. | ||
| 535 | */ | ||
| 536 | return !time_between32(now, last_overflow - HZ, | ||
| 537 | last_overflow + TCP_SYNCOOKIE_VALID); | ||
| 528 | } | 538 | } |
| 529 | 539 | ||
| 530 | static inline u32 tcp_cookie_time(void) | 540 | static inline u32 tcp_cookie_time(void) |