From b020591a18897c4fd8043e872606d9705558c8f7 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sun, 16 Jun 2024 00:52:37 +0000 Subject: [PATCH] Correct three bugs in the rate limiter. * We should be dividing by the packet size, not multiplying, as we should be crediting fewer packets for larger packets, not more. * We should handle the packet count underflowing as it may have been some time since the last packet. * The bucket limit needs to be `<= 0xd00`, not `< 0xf00`, as `0xf00` + `0x100` overflows our counter and resets the bucket. --- xdp.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/xdp.c b/xdp.c index 122a9eb..860fe25 100644 --- a/xdp.c +++ b/xdp.c @@ -246,14 +246,18 @@ if (rate) { \ /* than packets, we can keep counting packets and simply adjust the ratelimit by the*/ \ /* size of the packet we're looking at. */ \ /* Thus, here, we simply reduce our packet counter by the */ \ - /* time difference / our ns/packet limit times the size of the current packet. */ \ - int64_t pkts_since_last = (time_diff << RATE_BUCKET_BITS) * ((uint64_t)amt_in_pkt) / ((uint64_t)limit_ns_per_pkt); \ - bucket_pkts -= pkts_since_last; \ + /* time difference / (our ns/packet limit * the size of the current packet). */ \ + /* We shift by RATE_BUCKET_DECIMAL_BITS first since we're calculating whole packets. */ \ + int64_t pkts_allowed_since_last_update = \ + (time_diff << RATE_BUCKET_BITS) / (((uint64_t)amt_in_pkt) * ((uint64_t)limit_ns_per_pkt)); \ + bucket_pkts -= pkts_allowed_since_last_update; \ } \ /* Accept as long as we can add one to our bucket without overflow */ \ - if (bucket_pkts < (((1 << RATE_BUCKET_INTEGER_BITS) - 1) << RATE_BUCKET_DECIMAL_BITS)) { \ + const int64_t MAX_PACKETS = (1 << RATE_BUCKET_INTEGER_BITS) - 2; \ + if (bucket_pkts <= (MAX_PACKETS << RATE_BUCKET_DECIMAL_BITS)) { \ if (unlikely(bucket_pkts < 0)) bucket_pkts = 0; \ - uint64_t new_packet_count = bucket_pkts + (1 << RATE_BUCKET_DECIMAL_BITS); \ + int64_t new_packet_count = bucket_pkts + (1 << RATE_BUCKET_DECIMAL_BITS); \ + if (new_packet_count < 0) { new_packet_count = 0; } \ rate->pkts_and_time = time_masked | (new_packet_count << (64 - RATE_BUCKET_BITS)); \ matchbool = 0; \ } else { \ -- 2.39.5