From: Roland Fischer Date: Thu, 28 May 2020 05:58:00 +0000 (-0400) Subject: pcap: 32bit counters can wrap-around X-Git-Tag: suricata-6.0.0-beta1~137 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=9f1efa3c10291acf5fead882d7ca0568c3d66019;p=thirdparty%2Fsuricata.git pcap: 32bit counters can wrap-around Fixes issue 2845. pcap_stats is based on 32bit counters and given a big enough throughput will overflow them. This was reported by people using Myricom cards which should only be a happenstance. The problem exists for all pcap-based interfaces. Let's use internal 64bit counters that drag along the pcap_stats and handle pcap_stats wrap-around as we update the 64bit stats "often enough" before the pcap_stats can wrap around twice. --- diff --git a/src/source-pcap.c b/src/source-pcap.c index 06b4205794..7ee7be502b 100644 --- a/src/source-pcap.c +++ b/src/source-pcap.c @@ -47,6 +47,22 @@ #define PCAP_RECONNECT_TIMEOUT 500000 +/** + * \brief 64bit pcap stats counters. + * + * libpcap only supports 32bit counters. They will eventually wrap around. + * + * Keep track of libpcap counters as 64bit counters to keep on counting even + * if libpcap's 32bit counters wrap around. + * Requires pcap_stats() to be called before 32bit stats wrap around twice, + * which we do. + */ +typedef struct PcapStats64_ { + uint64_t ps_recv; + uint64_t ps_drop; + uint64_t ps_ifdrop; +} PcapStats64; + /** * \brief Structure to hold thread specific variables. */ @@ -67,9 +83,8 @@ typedef struct PcapThreadVars_ int datalink; /* counters */ - uint32_t pkts; + uint64_t pkts; uint64_t bytes; - uint32_t errs; uint16_t capture_kernel_packets; uint16_t capture_kernel_drops; @@ -88,6 +103,8 @@ typedef struct PcapThreadVars_ ChecksumValidationMode checksum_mode; LiveDevice *livedev; + + PcapStats64 last_stats64; } PcapThreadVars; static TmEcode ReceivePcapThreadInit(ThreadVars *, const void *, void **); @@ -99,12 +116,14 @@ static TmEcode DecodePcapThreadInit(ThreadVars *, const void *, void **); static TmEcode DecodePcapThreadDeinit(ThreadVars *tv, void *data); static TmEcode DecodePcap(ThreadVars *, Packet *, void *); +void SourcePcapRegisterTests(void); + /** protect pcap_compile and pcap_setfilter, as they are not thread safe: * http://seclists.org/tcpdump/2009/q1/62 */ static SCMutex pcap_bpf_compile_lock = SCMUTEX_INITIALIZER; /** - * \brief Registration Function for RecievePcap. + * \brief Registration Function for ReceivePcap. */ void TmModuleReceivePcapRegister (void) { @@ -115,6 +134,7 @@ void TmModuleReceivePcapRegister (void) tmm_modules[TMM_RECEIVEPCAP].ThreadExitPrintStats = ReceivePcapThreadExitStats; tmm_modules[TMM_RECEIVEPCAP].cap_flags = SC_CAP_NET_RAW; tmm_modules[TMM_RECEIVEPCAP].flags = TM_FLAG_RECEIVE_TM; + tmm_modules[TMM_RECEIVEPCAP].RegisterTests = SourcePcapRegisterTests; } /** @@ -129,14 +149,52 @@ void TmModuleDecodePcapRegister (void) tmm_modules[TMM_DECODEPCAP].flags = TM_FLAG_DECODE_TM; } +/** + * \brief Update 64 bit |last| value from |current32| value taking one + * wrap-around into account. + */ +static inline void UpdatePcapStatsValue64(uint64_t *last, uint32_t current32) +{ + /* uint64_t -> uint32_t is defined behaviour. It slices lower 32bits. */ + uint32_t last32 = *last; + + /* Branchless code as wrap-around is defined for unsigned */ + *last += (uint32_t)(current32 - last32); + + /* Same calculation as: + if (likely(current32 >= last32)) { + *last += current32 - last32; + } else { + *last += (1ull << 32) + current32 - last32; + } + */ +} + +/** + * \brief Update 64 bit |last| stat values with values from |current| + * 32 bit pcap_stat. + */ +static inline void UpdatePcapStats64( + PcapStats64 *last, const struct pcap_stat *current) +{ + UpdatePcapStatsValue64(&last->ps_recv, current->ps_recv); + UpdatePcapStatsValue64(&last->ps_drop, current->ps_drop); + UpdatePcapStatsValue64(&last->ps_ifdrop, current->ps_ifdrop); +} + static inline void PcapDumpCounters(PcapThreadVars *ptv) { struct pcap_stat pcap_s; if (likely((pcap_stats(ptv->pcap_handle, &pcap_s) >= 0))) { - StatsSetUI64(ptv->tv, ptv->capture_kernel_packets, pcap_s.ps_recv); - StatsSetUI64(ptv->tv, ptv->capture_kernel_drops, pcap_s.ps_drop); - (void) SC_ATOMIC_SET(ptv->livedev->drop, pcap_s.ps_drop); - StatsSetUI64(ptv->tv, ptv->capture_kernel_ifdrops, pcap_s.ps_ifdrop); + UpdatePcapStats64(&ptv->last_stats64, &pcap_s); + + StatsSetUI64(ptv->tv, ptv->capture_kernel_packets, + ptv->last_stats64.ps_recv); + StatsSetUI64( + ptv->tv, ptv->capture_kernel_drops, ptv->last_stats64.ps_drop); + (void)SC_ATOMIC_SET(ptv->livedev->drop, ptv->last_stats64.ps_drop); + StatsSetUI64(ptv->tv, ptv->capture_kernel_ifdrops, + ptv->last_stats64.ps_ifdrop); } } @@ -506,11 +564,11 @@ static void ReceivePcapThreadExitStats(ThreadVars *tv, void *data) if (pcap_stats(ptv->pcap_handle, &pcap_s) < 0) { SCLogError(SC_ERR_STAT,"(%s) Failed to get pcap_stats: %s", tv->name, pcap_geterr(ptv->pcap_handle)); - SCLogInfo("(%s) Packets %" PRIu32 ", bytes %" PRIu64 "", - tv->name, ptv->pkts, ptv->bytes); + SCLogInfo("(%s) Packets %" PRIu64 ", bytes %" PRIu64 "", tv->name, + ptv->pkts, ptv->bytes); } else { - SCLogInfo("(%s) Packets %" PRIu32 ", bytes %" PRIu64 "", - tv->name, ptv->pkts, ptv->bytes); + SCLogInfo("(%s) Packets %" PRIu64 ", bytes %" PRIu64 "", tv->name, + ptv->pkts, ptv->bytes); /* these numbers are not entirely accurate as ps_recv contains packets * that are still waiting to be processed at exit. ps_drop only contains @@ -520,11 +578,18 @@ static void ReceivePcapThreadExitStats(ThreadVars *tv, void *data) * Note: ps_recv includes dropped packets and should be considered total. * Unless we start to look at ps_ifdrop which isn't supported everywhere. */ - SCLogInfo("(%s) Pcap Total:%" PRIu64 " Recv:%" PRIu64 " Drop:%" PRIu64 " (%02.1f%%).", - tv->name, (uint64_t)pcap_s.ps_recv, - (uint64_t)pcap_s.ps_recv - (uint64_t)pcap_s.ps_drop, - (uint64_t)pcap_s.ps_drop, - (((float)(uint64_t)pcap_s.ps_drop)/(float)(uint64_t)pcap_s.ps_recv)*100); + UpdatePcapStats64(&ptv->last_stats64, &pcap_s); + float drop_percent = + likely(ptv->last_stats64.ps_recv > 0) + ? (((float)ptv->last_stats64.ps_drop) / + (float)ptv->last_stats64.ps_recv) * + 100 + : 0; + SCLogInfo("(%s) Pcap Total:%" PRIu64 " Recv:%" PRIu64 " Drop:%" PRIu64 + " (%02.1f%%).", + tv->name, ptv->last_stats64.ps_recv, + ptv->last_stats64.ps_recv - ptv->last_stats64.ps_drop, + ptv->last_stats64.ps_drop, drop_percent); } } @@ -639,3 +704,21 @@ void PcapTranslateIPToDevice(char *pcap_dev, size_t len) pcap_freealldevs(alldevsp); } + +/* + * unittests + */ + +#ifdef UNITTESTS +#include "tests/source-pcap.c" +#endif /* UNITTESTS */ + +/** + * \brief Register the Unit tests for pcap source + */ +void SourcePcapRegisterTests(void) +{ +#ifdef UNITTESTS + SourcePcapRegisterStatsTests(); +#endif /* UNITTESTS */ +} diff --git a/src/tests/source-pcap.c b/src/tests/source-pcap.c new file mode 100644 index 0000000000..fc1b275e3f --- /dev/null +++ b/src/tests/source-pcap.c @@ -0,0 +1,232 @@ +/* Copyright (C) 2020 Open Information Security Foundation + * + * You can copy, redistribute or modify this Program under the terms of + * the GNU General Public License version 2 as published by the Free + * Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * version 2 along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA + * 02110-1301, USA. + */ + +#include "../suricata-common.h" +#include "../util-unittest.h" + +static uint32_t Upper32(uint64_t value) +{ + /* uint64_t -> uint32_t is defined behaviour. It slices lower 32bits. */ + return value >> 32; +} +static uint32_t Lower32(uint64_t value) +{ + /* uint64_t -> uint32_t is defined behaviour. It slices lower 32bits. */ + return value; +} + +/* Structured test data to make it easier on my eyes */ +typedef struct TestData_ { + uint64_t last; /* internal 64bit counter to drag along */ + u_int current; /* 32bit pcap stat */ +} TestData; + +static int UpdatePcapStatsValue64NoChange01(void) +{ + /* + * No change in counter values. + * Last count is within first 32bit range, i.e. same as pcap_stat range. + */ + TestData data[] = {{.last = 0, .current = 0}, + {.last = 12345, .current = 12345}, + {.last = (uint64_t)UINT32_MAX, .current = UINT_MAX}}; + + for (size_t i = 0; i < ARRAY_SIZE(data); ++i) { + FAIL_IF_NOT(data[i].last == data[i].current); + + UpdatePcapStatsValue64(&data[i].last, data[i].current); + FAIL_IF_NOT(data[i].last == data[i].current); + } + + PASS; +} + +static int UpdatePcapStatsValue64NoChange02(void) +{ + /* + * No change in counter values. + * Last count is outside 32bits range. + */ + TestData data[] = {{.last = (2ull << 32) + 0, .current = 0}, + {.last = (3ull << 32) + 12345, .current = 12345}, + {.last = (3ull << 32) + (uint64_t)UINT32_MAX, .current = UINT_MAX}, + {.last = UINT64_MAX, .current = UINT_MAX}}; + + for (size_t i = 0; i < ARRAY_SIZE(data); ++i) { + uint32_t upper = Upper32(data[i].last); + FAIL_IF_NOT(Lower32(data[i].last) == data[i].current); + + UpdatePcapStatsValue64(&data[i].last, data[i].current); + FAIL_IF_NOT(Lower32(data[i].last) == data[i].current); + FAIL_IF_NOT(Upper32(data[i].last) == upper); + } + + PASS; +} + +static int UpdatePcapStatsValue64NoOverflow01(void) +{ + /* + * Non-overflowing counter value is simply taken over in lower 32bits. + * Last count is within first 32bit range, i.e. same as pcap_stat range. + * Also test edges and simple +1. + */ + TestData data[] = {{.last = 0, .current = 1}, + {.last = 12345, .current = 34567}, + {.last = (uint64_t)UINT32_MAX - 1, .current = UINT_MAX}}; + + for (size_t i = 0; i < ARRAY_SIZE(data); ++i) { + FAIL_IF_NOT(data[i].last < data[i].current); + + UpdatePcapStatsValue64(&data[i].last, data[i].current); + FAIL_IF_NOT(data[i].last == data[i].current); + } + + PASS; +} + +static int UpdatePcapStatsValue64NoOverflow02(void) +{ + /* + * Non-overflowing counter value is simply taken over in lower 32bits. + * Last count is outside 32bits range. + */ + TestData data[] = {{.last = (2ull << 32) + 0, .current = 1}, + {.last = (3ull << 32) + 12345, .current = 34567}, + {.last = UINT64_MAX - 1, .current = UINT_MAX}}; + + for (size_t i = 0; i < ARRAY_SIZE(data); ++i) { + uint32_t upper = Upper32(data[i].last); + FAIL_IF_NOT(Lower32(data[i].last) < data[i].current); + + UpdatePcapStatsValue64(&data[i].last, data[i].current); + FAIL_IF_NOT(Lower32(data[i].last) == data[i].current); + FAIL_IF_NOT(Upper32(data[i].last) == upper); + } + + PASS; +} + +static int UpdatePcapStatsValue64Overflow01(void) +{ + /* + * Overflowing counter value is simply taken over in lower 32bits. + * Last count is within first 32bit range, i.e. same as pcap_stat range. + */ + TestData data[] = {{.last = 1, .current = 0}, + {.last = 12345, .current = 22}, {.last = 12345, .current = 12344}, + {.last = (uint64_t)UINT32_MAX, .current = UINT_MAX - 1}}; + + for (size_t i = 0; i < ARRAY_SIZE(data); ++i) { + FAIL_IF_NOT(data[i].last > data[i].current); + + UpdatePcapStatsValue64(&data[i].last, data[i].current); + FAIL_IF_NOT(Lower32(data[i].last) == data[i].current); + FAIL_IF_NOT(Upper32(data[i].last) == 1); /* wrap around */ + } + + PASS; +} + +static int UpdatePcapStatsValue64Overflow02(void) +{ + /* + * Overflowing counter value is simply taken over in lower 32bits. + * Last count is outside 32bits range. + */ + TestData data[] = {{.last = (2ull << 32) + 1, .current = 0}, + {.last = (3ull << 32) + 12345, .current = 22}, + {.last = (3ull << 32) + 12345, .current = 12344}, + {.last = UINT64_MAX, .current = UINT_MAX - 1}}; + + for (size_t i = 0; i < ARRAY_SIZE(data); ++i) { + uint32_t upper = Upper32(data[i].last); + FAIL_IF_NOT(Lower32(data[i].last) > data[i].current); + + UpdatePcapStatsValue64(&data[i].last, data[i].current); + FAIL_IF_NOT(Lower32(data[i].last) == data[i].current); + FAIL_IF_NOT(Upper32(data[i].last) == upper + 1); /* wrap around */ + } + + PASS; +} + +static int UpdatePcapStatsValue64Overflow03(void) +{ + /* + * Overflowing counter value is simply taken over in lower 32bits. + * Edge cases where upper32 bit wrap around to 0. + */ + TestData data[] = {{.last = UINT64_MAX, .current = 0}, + {.last = UINT64_MAX, .current = 3333}}; + + for (size_t i = 0; i < ARRAY_SIZE(data); ++i) { + FAIL_IF_NOT(Lower32(data[i].last) > data[i].current); + + UpdatePcapStatsValue64(&data[i].last, data[i].current); + FAIL_IF_NOT(Lower32(data[i].last) == data[i].current); + FAIL_IF_NOT(Upper32(data[i].last) == 0); /* wrap around */ + } + + PASS; +} + +static int UpdatePcapStats64Assorted01(void) +{ + /* + * Test that all fields of the struct are correctly updated. + * + * Full testing of value behaviour is done in UpdatePcapStatsValue64...() + * tests. + */ + PcapStats64 last = {.ps_recv = 0, .ps_drop = 1234, .ps_ifdrop = 8765}; + struct pcap_stat current = { + .ps_recv = 12, .ps_drop = 2345, .ps_ifdrop = 9876}; + + // test setup sanity check + FAIL_IF_NOT(last.ps_recv < current.ps_recv); + FAIL_IF_NOT(last.ps_drop < current.ps_drop); + FAIL_IF_NOT(last.ps_ifdrop < current.ps_ifdrop); + + UpdatePcapStats64(&last, ¤t); + + FAIL_IF_NOT(last.ps_recv == current.ps_recv); + FAIL_IF_NOT(last.ps_drop == current.ps_drop); + FAIL_IF_NOT(last.ps_ifdrop == current.ps_ifdrop); + + PASS; +} + +static void SourcePcapRegisterStatsTests(void) +{ + UtRegisterTest("UpdatePcapStatsValue64NoChange01", + UpdatePcapStatsValue64NoChange01); + UtRegisterTest("UpdatePcapStatsValue64NoChange02", + UpdatePcapStatsValue64NoChange02); + UtRegisterTest("UpdatePcapStatsValue64NoOverflow01", + UpdatePcapStatsValue64NoOverflow01); + UtRegisterTest("UpdatePcapStatsValue64NoOverflow02", + UpdatePcapStatsValue64NoOverflow02); + UtRegisterTest("UpdatePcapStatsValue64Overflow01", + UpdatePcapStatsValue64Overflow01); + UtRegisterTest("UpdatePcapStatsValue64Overflow02", + UpdatePcapStatsValue64Overflow02); + UtRegisterTest("UpdatePcapStatsValue64Overflow03", + UpdatePcapStatsValue64Overflow03); + + UtRegisterTest("UpdatePcapStats64Assorted01", UpdatePcapStats64Assorted01); +}