From 076df9aaa5a1df1a05382f156bbbab741046d64b Mon Sep 17 00:00:00 2001 From: Xichen96 Date: Sat, 1 Aug 2026 03:54:09 +1000 Subject: [PATCH 1/6] [dhcpmon]: Apply small packet and C++ hardening fixes Bound each raw-socket callback to 64 packets and reset the recvfrom address length for every receive attempt. Rename three file-scope helpers that begin with underscores because C++ reserves those identifiers in the global namespace; static linkage already marks them as internal. Replace four nullptr comparisons with NULL to match the daemon's established style. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 39f979be-d826-4d5c-949a-f20abb58bb83 Signed-off-by: Xichen96 --- src/dhcp_mon.cpp | 8 ++++---- src/packet_handler.cpp | 22 ++++++++++++++-------- src/sock_mgr.cpp | 6 +++--- src/util.cpp | 8 ++++---- 4 files changed, 25 insertions(+), 19 deletions(-) diff --git a/src/dhcp_mon.cpp b/src/dhcp_mon.cpp index ef4f6623d..e5a5fca3c 100644 --- a/src/dhcp_mon.cpp +++ b/src/dhcp_mon.cpp @@ -125,21 +125,21 @@ static bool db_counters_initialized(const std::string &ifname) table_name = construct_counter_db_table_key(ifname, false); auto rx_v4 = mCountersDbPtr->hget(table_name, "RX"); - if (rx_v4 == nullptr || rx_v4->empty()) { + if (rx_v4 == NULL || rx_v4->empty()) { return false; } auto tx_v4 = mCountersDbPtr->hget(table_name, "TX"); - if (tx_v4 == nullptr || tx_v4->empty()) { + if (tx_v4 == NULL || tx_v4->empty()) { return false; } table_name = construct_counter_db_table_key(ifname, true); auto rx_v6 = mCountersDbPtr->hget(table_name, "RX"); - if (rx_v6 == nullptr || rx_v6->empty()) { + if (rx_v6 == NULL || rx_v6->empty()) { return false; } auto tx_v6 = mCountersDbPtr->hget(table_name, "TX"); - if (tx_v6 == nullptr || tx_v6->empty()) { + if (tx_v6 == NULL || tx_v6->empty()) { return false; } diff --git a/src/packet_handler.cpp b/src/packet_handler.cpp index 7ec5d07a3..09f1ae752 100644 --- a/src/packet_handler.cpp +++ b/src/packet_handler.cpp @@ -16,8 +16,11 @@ #include "dhcp_check_profile.h" /** to get dhcp/v6 check profile */ #include "util.h" +// Bound one callback to keep health/DB events responsive during packet bursts. +static constexpr int MAX_PACKETS_PER_CALLBACK = 64; + /** - * @code _increase_cache_counter(ifname, sock, type); + * @code increase_single_cache_counter(ifname, sock, type); * @brief helper function to increase cache counter. Simple increase of counter, no complications. In the event of * extremely unexpected nonexistent ifname, just fail * @param ifname interface name @@ -25,9 +28,9 @@ * @param type message type * @return none */ -static inline void _increase_cache_counter(const std::string &ifname, int sock, uint8_t type) +static inline void increase_single_cache_counter(const std::string &ifname, int sock, uint8_t type) { - syslog_debug(LOG_INFO, "_increase_cache_counter: increasing cache counter for ifname %s, sock %d, type %d", + syslog_debug(LOG_INFO, "increase_single_cache_counter: increasing cache counter for ifname %s, sock %d, type %d", ifname.c_str(), sock, type); sock_mgr_get_sock_info(sock).all_counters.at(ifname)[type]++; } @@ -45,7 +48,7 @@ static inline void _increase_cache_counter(const std::string &ifname, int sock, */ static void increase_cache_counter(const std::string &ifname, const dhcp_device_context_t *context, int sock, uint8_t type, bool dup_to_context=false) { - _increase_cache_counter(ifname, sock, type); + increase_single_cache_counter(ifname, sock, type); // we seperate mgmt interface from others and do not increase agg counter if (mgmt_ifname != "" && mgmt_ifname.compare(context->intf) == 0) { @@ -54,7 +57,7 @@ static void increase_cache_counter(const std::string &ifname, const dhcp_device_ // when ifname belongs to another context ifname, increase the aggregate counter for that context, // else when ifname is the context, we increase agg counter for all. - _increase_cache_counter(get_agg_counter_ifname(ifname, context->intf), sock, type); + increase_single_cache_counter(get_agg_counter_ifname(ifname, context->intf), sock, type); // optionally duplicate to context ifname, it will only be true when this is standby physical interface under a vlan on a dual tor if (dup_to_context) { @@ -861,11 +864,14 @@ void callback_common(int fd, short event, void *arg) { ssize_t buffer_sz; struct sockaddr_ll sll; - socklen_t slen = sizeof(sll); sock_info_t &sock_info = sock_mgr_get_sock_info(fd); - while ((buffer_sz = recvfrom(fd, sock_info.buffer, sock_info.snaplen, MSG_DONTWAIT, (struct sockaddr *)&sll, &slen)) > 0) - { + for (int packet_count = 0; packet_count < MAX_PACKETS_PER_CALLBACK; packet_count++) { + socklen_t slen = sizeof(sll); + if ((buffer_sz = recvfrom(fd, sock_info.buffer, sock_info.snaplen, MSG_DONTWAIT, + (struct sockaddr *)&sll, &slen)) <= 0) { + break; + } char ifname_buf[IF_NAMESIZE]; if (if_indextoname(sll.sll_ifindex, ifname_buf) == NULL) { syslog_debug(LOG_WARNING, "if_indextoname: invalid input interface index %d %s", sll.sll_ifindex, strerror(errno)); diff --git a/src/sock_mgr.cpp b/src/sock_mgr.cpp index 8d3e48d81..f01c07a74 100644 --- a/src/sock_mgr.cpp +++ b/src/sock_mgr.cpp @@ -153,11 +153,11 @@ static void print_bpf_prog(const struct bpf_program *bp) { } /** - * @code _compile_bpf_prog(handle, bp, filter, fprog) + * @code compile_bpf_prog(handle, bp, filter, fprog) * * @brief helper function to compile filter to bpf byte code and store in fprog */ -static int _compile_bpf_prog(pcap_t *handle, struct bpf_program *bp, const char *filter, struct sock_fprog *fprog){ +static int compile_bpf_prog(pcap_t *handle, struct bpf_program *bp, const char *filter, struct sock_fprog *fprog){ syslog(LOG_INFO, "Compiling filter %s to bpf prog", filter); if (pcap_compile(handle, bp, filter, 1, PCAP_NETMASK_UNKNOWN) < 0) { @@ -213,7 +213,7 @@ static int sock_mgr_compile_all_bpf_prog() struct bpf_program bp; for (auto &[sock, info] : sock_map) { - if (_compile_bpf_prog(handle, &bp, info.filter, &info.bpf_prog) < 0) { + if (compile_bpf_prog(handle, &bp, info.filter, &info.bpf_prog) < 0) { syslog(LOG_ALERT, "Failed to compile %s into bpf prog", info.filter); sock_mgr_free_all_bpf_prog(); pcap_close(handle); diff --git a/src/util.cpp b/src/util.cpp index aab7d83bd..8abe17787 100644 --- a/src/util.cpp +++ b/src/util.cpp @@ -32,14 +32,14 @@ struct udp6_pseudo_header { }; /** - * @code _addr_is_primary(ifname, addr, addr_len); + * @code addr_is_primary_impl(ifname, addr, addr_len); * @brief Check if the given address is primary on the interface by querying ConfigDB. * @param ifname interface name * @param addr pointer to the address (in_addr for IPv4 or in6_addr for IPv6) * @param addr_len length of the address (4 for IPv4, 16 for IPv6) * @return true if the address is primary, false if secondary or not found */ -static bool _addr_is_primary(const std::string &ifname, const uint8_t *addr, size_t addr_len) +static bool addr_is_primary_impl(const std::string &ifname, const uint8_t *addr, size_t addr_len) { auto match_pattern = std::string("*INTERFACE|" + ifname + "|*"); auto keys = mConfigDbPtr->keys(match_pattern); @@ -73,12 +73,12 @@ static bool _addr_is_primary(const std::string &ifname, const uint8_t *addr, siz bool addr_is_primary(const std::string &ifname, const in_addr *addr) { - return _addr_is_primary(ifname, (const uint8_t *)addr, sizeof(struct in_addr)); + return addr_is_primary_impl(ifname, (const uint8_t *)addr, sizeof(struct in_addr)); } bool addr6_is_primary(const std::string &ifname, const in6_addr *addr) { - return _addr_is_primary(ifname, (const uint8_t *)addr, sizeof(struct in6_addr)); + return addr_is_primary_impl(ifname, (const uint8_t *)addr, sizeof(struct in6_addr)); } bool intf_is_standby(const std::string &ifname) From 1331563d931232adc59801eaab1d92fd95ff761c Mon Sep 17 00:00:00 2001 From: Xichen96 Date: Sat, 1 Aug 2026 03:54:09 +1000 Subject: [PATCH 2/6] [dhcpmon]: Track physical members beneath VLAN PortChannels Preserve VLAN and PortChannel membership so a physical member resolves through its immediate PortChannel parent to the monitored VLAN context. Keep direct VLAN-member precedence, bound hierarchy traversal, ignore self-references, and centralize parent, context, and aggregate-name lookup. Aggregate only to the immediate parent so pre-VLAN observations are not rolled into the root VLAN counter and misreported as relay loss. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 39f979be-d826-4d5c-949a-f20abb58bb83 Signed-off-by: Xichen96 --- src/dhcp_devman.cpp | 78 ++++++++++++++++++++++++++++-------------- src/dhcp_devman.h | 26 ++++++++++++-- src/dhcp_mon.cpp | 10 ++++-- src/packet_handler.cpp | 6 ++-- src/util.h | 11 +++--- 5 files changed, 91 insertions(+), 40 deletions(-) diff --git a/src/dhcp_devman.cpp b/src/dhcp_devman.cpp index 9fa951e3c..7d8fc09f4 100644 --- a/src/dhcp_devman.cpp +++ b/src/dhcp_devman.cpp @@ -180,22 +180,9 @@ int dhcp_devman_setup_dual_tor_mode(const char *name) bool dhcp_devman_is_tracked_interface(const std::string &ifname) { - auto itr = intfs.find(ifname); - if (itr != intfs.end()) { - return true; - } - auto vlan_itr = vlan_map.find(ifname); - if (vlan_itr != vlan_map.end()) { - return true; - } - auto portchan_itr = portchan_map.find(ifname); - if (portchan_itr != portchan_map.end()) { - return true; - } - if (ifname == mgmt_ifname) { - return true; - } - return false; + return intfs.find(ifname) != intfs.end() || + !dhcp_devman_get_parent_ifname(ifname).empty() || + ifname == mgmt_ifname; } /** @@ -248,7 +235,10 @@ static void update_portchannel_mapping() auto second = key.find_last_of('|'); auto portchannel = key.substr(first + 1, second - first - 1); auto ifname = key.substr(second + 1); - if (intfs.find(portchannel) == intfs.end()) { + bool portchannel_is_context = intfs.find(portchannel) != intfs.end(); + bool portchannel_is_vlan_member = vlan_map.find(portchannel) != vlan_map.end(); + // Dual-ToR downlink counters require MUX attribution that is unavailable on a nested PortChannel. + if (!portchannel_is_context && (!portchannel_is_vlan_member || dual_tor_mode)) { all_skipped_ifname += "<" + ifname + ", " + portchannel + ">, "; continue; } @@ -296,7 +286,7 @@ int dhcp_devman_init() agg_dev_all = "Agg-" + downstream_ifname; agg_dev_prefix = agg_dev_all + "-"; - // vlan and its members, portchannel and its members are initialized regardless of whether they are in cmdline + // PortChannel members depend on VLAN mappings to recognize a PortChannel under a monitored VLAN. update_vlan_mapping(); update_portchannel_mapping(); @@ -315,21 +305,59 @@ void dhcp_devman_free() intfs.clear(); } -const dhcp_device_context_t *dhcp_devman_get_device_context(const std::string &ifname) +static constexpr unsigned int MAX_CONTEXT_DEPTH = 3; + +std::string dhcp_devman_get_parent_ifname(const std::string &ifname) { - const auto iter = intfs.find(ifname); - if (iter != intfs.end()) { - return iter->second; + if (intfs.find(ifname) != intfs.end()) { + return ""; } const auto vlan = vlan_map.find(ifname); if (vlan != vlan_map.end() && ifname != vlan->second) { - return dhcp_devman_get_device_context(vlan->second); + return vlan->second; } const auto port_channel = portchan_map.find(ifname); if (port_channel != portchan_map.end() && ifname != port_channel->second) { - return dhcp_devman_get_device_context(port_channel->second); + return port_channel->second; } - return NULL; + return ""; +} + +/** + * @code get_device_context(ifname, depth); + * + * @brief Follow parent mappings until reaching a tracked input interface + * + * @param ifname Interface name to resolve + * @param depth Current parent traversal depth + * + * @return Tracked interface context, or NULL when no context is found + */ +static const dhcp_device_context_t *get_device_context( + const std::string &ifname, unsigned int depth) +{ + if (depth > MAX_CONTEXT_DEPTH) { + syslog_debug(LOG_WARNING, "Exceeded interface membership depth at %s", ifname.c_str()); + return NULL; + } + const auto iter = intfs.find(ifname); + if (iter != intfs.end()) { + return iter->second; + } + const std::string parent_ifname = dhcp_devman_get_parent_ifname(ifname); + return parent_ifname.empty() ? NULL : get_device_context(parent_ifname, depth + 1); +} + +const dhcp_device_context_t *dhcp_devman_get_device_context(const std::string &ifname) +{ + return get_device_context(ifname, 0); +} + +std::string dhcp_devman_get_agg_counter_ifname(const std::string &ifname) +{ + const std::string parent_ifname = dhcp_devman_get_parent_ifname(ifname); + return parent_ifname.empty() ? agg_dev_all : + get_agg_counter_ifname(parent_ifname); } void dhcp_devman_print_all_status(dhcp_counters_type_t type) diff --git a/src/dhcp_devman.h b/src/dhcp_devman.h index 73c1cd6f2..f0cdde3be 100644 --- a/src/dhcp_devman.h +++ b/src/dhcp_devman.h @@ -127,17 +127,39 @@ int dhcp_devman_init(); */ void dhcp_devman_free(); +/** + * @code dhcp_devman_get_parent_ifname(ifname); + * + * @brief find the immediate parent interface of a tracked interface. + * + * @param ifname interface name + * + * @return Immediate parent interface name, or an empty string when the interface is a tracked root or is unmapped + */ +std::string dhcp_devman_get_parent_ifname(const std::string &ifname); + /** * @code dhcp_devman_get_device_context(ifname); * - * @brief find device context, if its physical interface, will query vlan_map and portchannel_map first + * @brief find the tracked input interface that owns an interface. * * @param ifname interface name * - * @return pointer to device (interface) context if found, NULL otherwise + * @return The interface's tracked context; a tracked input interface returns its own context */ const dhcp_device_context_t* dhcp_devman_get_device_context(const std::string &ifname); +/** + * @code dhcp_devman_get_agg_counter_ifname(ifname); + * + * @brief find the aggregate counter updated by an interface observation. + * + * @param ifname interface name + * + * @return immediate-parent aggregate, or the root aggregate when no parent exists + */ +std::string dhcp_devman_get_agg_counter_ifname(const std::string &ifname); + /** * @code dhcp_devman_print_all_status(type); * diff --git a/src/dhcp_mon.cpp b/src/dhcp_mon.cpp index e5a5fca3c..654c3c1b1 100644 --- a/src/dhcp_mon.cpp +++ b/src/dhcp_mon.cpp @@ -81,7 +81,7 @@ static void recalculate_agg_counter(all_counters_t &all_counters) if (mgmt_ifname == context->intf) { continue; } - counter_t &agg_counter = all_counters.at(get_agg_counter_ifname(ifname, context->intf)); + counter_t &agg_counter = all_counters.at(dhcp_devman_get_agg_counter_ifname(ifname)); for (const auto &[msg_type, count] : counter) { agg_counter[msg_type] += count; } @@ -466,7 +466,9 @@ static void initialize_all_intf_counters() initialize_all_counters(ifname); } initialize_all_counters(vlan); - sock_mgr_init_cache_counters(agg_dev_prefix + vlan, DHCP_MESSAGE_TYPE_COUNT, DHCPV6_MESSAGE_TYPE_COUNT); + sock_mgr_init_cache_counters( + get_agg_counter_ifname(vlan), + DHCP_MESSAGE_TYPE_COUNT, DHCPV6_MESSAGE_TYPE_COUNT); } for (const auto &[portchan, intfs] : rev_portchan_map) { @@ -474,7 +476,9 @@ static void initialize_all_intf_counters() initialize_all_counters(ifname); } initialize_all_counters(portchan); - sock_mgr_init_cache_counters(agg_dev_prefix + portchan, DHCP_MESSAGE_TYPE_COUNT, DHCPV6_MESSAGE_TYPE_COUNT); + sock_mgr_init_cache_counters( + get_agg_counter_ifname(portchan), + DHCP_MESSAGE_TYPE_COUNT, DHCPV6_MESSAGE_TYPE_COUNT); } // Now all vlan and portchannel related interfaces have entries in counters, now do the rest (uplink) diff --git a/src/packet_handler.cpp b/src/packet_handler.cpp index 09f1ae752..b9532833e 100644 --- a/src/packet_handler.cpp +++ b/src/packet_handler.cpp @@ -50,14 +50,12 @@ static void increase_cache_counter(const std::string &ifname, const dhcp_device_ { increase_single_cache_counter(ifname, sock, type); - // we seperate mgmt interface from others and do not increase agg counter + // we separate mgmt interface from others and do not increase agg counter if (mgmt_ifname != "" && mgmt_ifname.compare(context->intf) == 0) { return; } - // when ifname belongs to another context ifname, increase the aggregate counter for that context, - // else when ifname is the context, we increase agg counter for all. - increase_single_cache_counter(get_agg_counter_ifname(ifname, context->intf), sock, type); + increase_single_cache_counter(dhcp_devman_get_agg_counter_ifname(ifname), sock, type); // optionally duplicate to context ifname, it will only be true when this is standby physical interface under a vlan on a dual tor if (dup_to_context) { diff --git a/src/util.h b/src/util.h index f4fbd24c3..293688822 100644 --- a/src/util.h +++ b/src/util.h @@ -217,15 +217,14 @@ inline bool is_agg_counter(const std::string &ifname) } /** - * @code get_agg_counter_ifname(ifname, context); - * @brief Get aggregate counter name for given ifname and device context - * @param ifname Interface name - * @param context Pointer to device context + * @code get_agg_counter_ifname(parent_ifname); + * @brief Get aggregate counter name for a parent's immediate children + * @param parent_ifname Parent interface name * @return Aggregate counter name */ -inline std::string get_agg_counter_ifname(const std::string &ifname, const std::string &context_ifname) +inline std::string get_agg_counter_ifname(const std::string &parent_ifname) { - return ifname != context_ifname ? agg_dev_prefix + context_ifname : agg_dev_all; + return agg_dev_prefix + parent_ifname; } /** From 831de60c785293224ba50f9fbadfc9400e9c510e Mon Sep 17 00:00:00 2001 From: Xichen96 Date: Sat, 1 Aug 2026 03:54:25 +1000 Subject: [PATCH 3/6] [dhcpmon]: Track parent and aggregate counters per interface Populate one state per discovered VLAN or PortChannel check, derive aggregate ratios from topology, and format aggregate disparity errors with the interface and duration. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 39f979be-d826-4d5c-949a-f20abb58bb83 Signed-off-by: Xichen96 --- src/dhcp_device.cpp | 135 ++++++++++++--------------- src/dhcp_device.h | 12 +-- src/dhcp_mon.cpp | 1 + src/health_check.cpp | 213 +++++++++++-------------------------------- src/health_check.h | 23 ++++- 5 files changed, 135 insertions(+), 249 deletions(-) diff --git a/src/dhcp_device.cpp b/src/dhcp_device.cpp index 8b1ed4f6c..e7e22ea2a 100644 --- a/src/dhcp_device.cpp +++ b/src/dhcp_device.cpp @@ -20,11 +20,7 @@ extern bool debug_on; -extern std::string agg_dev_all; -extern std::string agg_dev_prefix; - extern std::unordered_map> rev_vlan_map; -extern std::unordered_map> rev_portchan_map; const std::string db_counter_name[DHCP_MESSAGE_TYPE_COUNT] = { "Unknown", "Discover", "Offer", "Request", "Decline", "Ack", "Nak", "Release", "Inform", "Bootp", "Malformed", "Ignored" @@ -204,7 +200,7 @@ static dhcp_mon_status_t dhcp_device_check_negative_health_v6(const std::string * @return true if deltas are equal with given ratio, false otherwise */ static bool check_counters_delta_expected(const std::string &ifname, const std::string &other_ifname, int sock, - uint8_t ratio, const int *monitored_msgs, size_t monitored_msg_cnt) + size_t ratio, const int *monitored_msgs, size_t monitored_msg_cnt) { const sock_info_t &sock_info = sock_mgr_get_sock_info(sock); const counter_t &counters = sock_info.all_counters.at(ifname); @@ -223,63 +219,49 @@ static bool check_counters_delta_expected(const std::string &ifname, const std:: return true; } -static dhcp_mon_status_t dhcp_device_check_agg_equal_rx(const std::string &ifname) -{ - std::string agg_ifname = agg_dev_prefix + ifname; - return check_counters_delta_expected(ifname, agg_ifname, rx_sock, 1, (const int *)monitored_msgs, monitored_msg_sz) ? - DHCP_MON_STATUS_HEALTHY : DHCP_MON_STATUS_UNHEALTHY; -} - -static dhcp_mon_status_t dhcp_device_check_agg_equal_tx(const std::string &ifname) -{ - std::string agg_ifname = agg_dev_prefix + ifname; - return check_counters_delta_expected(ifname, agg_ifname, tx_sock, 1, (const int *)monitored_msgs, monitored_msg_sz) ? - DHCP_MON_STATUS_HEALTHY : DHCP_MON_STATUS_UNHEALTHY; -} - -static dhcp_mon_status_t dhcp_device_check_agg_equal_rx_v6(const std::string &ifname) -{ - std::string agg_ifname = agg_dev_prefix + ifname; - return check_counters_delta_expected(ifname, agg_ifname, rx_sock_v6, 1, (const int *)monitored_v6_msgs, monitored_v6_msg_sz) ? - DHCP_MON_STATUS_HEALTHY : DHCP_MON_STATUS_UNHEALTHY; -} - -static dhcp_mon_status_t dhcp_device_check_agg_equal_tx_v6(const std::string &ifname) -{ - std::string agg_ifname = agg_dev_prefix + ifname; - return check_counters_delta_expected(ifname, agg_ifname, tx_sock_v6, 1, (const int *)monitored_v6_msgs, monitored_v6_msg_sz) ? - DHCP_MON_STATUS_HEALTHY : DHCP_MON_STATUS_UNHEALTHY; -} - -static dhcp_mon_status_t dhcp_device_check_agg_multiple_rx(const std::string &ifname) -{ - std::string agg_ifname = agg_dev_prefix + ifname; - return check_counters_delta_expected(ifname, agg_ifname, rx_sock, readonly_access(rev_vlan_map, ifname).size() + readonly_access(rev_portchan_map, ifname).size(), - (const int *)monitored_msgs, monitored_msg_sz) ? - DHCP_MON_STATUS_HEALTHY : DHCP_MON_STATUS_UNHEALTHY; -} - -static dhcp_mon_status_t dhcp_device_check_agg_multiple_tx(const std::string &ifname) -{ - std::string agg_ifname = agg_dev_prefix + ifname; - return check_counters_delta_expected(ifname, agg_ifname, tx_sock, readonly_access(rev_vlan_map, ifname).size() + readonly_access(rev_portchan_map, ifname).size(), - (const int *)monitored_msgs, monitored_msg_sz) ? - DHCP_MON_STATUS_HEALTHY : DHCP_MON_STATUS_UNHEALTHY; -} - -static dhcp_mon_status_t dhcp_device_check_agg_multiple_rx_v6(const std::string &ifname) +/** + * @code get_aggregate_ratio(ifname, sock); + * + * @brief Get the expected aggregate ratio for an interface and packet direction + * + * @param ifname Parent interface name + * @param sock Socket identifying the protocol family and direction + * + * @return One for RX and PortChannel TX; direct VLAN member count for VLAN TX + */ +static size_t get_aggregate_ratio(const std::string &ifname, int sock) { - std::string agg_ifname = agg_dev_prefix + ifname; - return check_counters_delta_expected(ifname, agg_ifname, rx_sock_v6, readonly_access(rev_vlan_map, ifname).size() + readonly_access(rev_portchan_map, ifname).size(), - (const int *)monitored_v6_msgs, monitored_v6_msg_sz) ? - DHCP_MON_STATUS_HEALTHY : DHCP_MON_STATUS_UNHEALTHY; + // RX crosses one edge. VLAN TX is observed on every direct member; PortChannel TX is observed on one member. + if (sock_mgr_get_sock_info(sock).is_rx) { + return 1; + } + const auto vlan = rev_vlan_map.find(ifname); + return vlan == rev_vlan_map.end() ? 1 : vlan->second.size(); } -static dhcp_mon_status_t dhcp_device_check_agg_multiple_tx_v6(const std::string &ifname) +/** + * @code check_aggregate_health(ifname, sock, monitored_msgs, monitored_msg_cnt); + * + * @brief Compare a parent interface counter with the aggregate of its direct member-interface counters + * + * @param ifname Parent interface name + * @param sock Socket identifying the protocol family and direction + * @param monitored_msgs Message types to compare + * @param monitored_msg_cnt Number of message types to compare + * + * @return HEALTHY when idle or matching, UNHEALTHY when the expected ratio does not match + */ +static dhcp_mon_status_t check_aggregate_health(const std::string &ifname, int sock, const int *monitored_msgs, + size_t monitored_msg_cnt) { - std::string agg_ifname = agg_dev_prefix + ifname; - return check_counters_delta_expected(ifname, agg_ifname, tx_sock_v6, readonly_access(rev_vlan_map, ifname).size() + readonly_access(rev_portchan_map, ifname).size(), - (const int *)monitored_v6_msgs, monitored_v6_msg_sz) ? + const std::string agg_ifname = get_agg_counter_ifname(ifname); + if (!check_counter_increased(ifname, sock, monitored_msgs, monitored_msg_cnt) && + !check_counter_increased(agg_ifname, sock, monitored_msgs, monitored_msg_cnt)) { + return DHCP_MON_STATUS_HEALTHY; + } + return check_counters_delta_expected( + ifname, agg_ifname, sock, get_aggregate_ratio(ifname, sock), + monitored_msgs, monitored_msg_cnt) ? DHCP_MON_STATUS_HEALTHY : DHCP_MON_STATUS_UNHEALTHY; } @@ -382,7 +364,14 @@ void dhcp_device_print_status_debug(const std::string &ifname, dhcp_counters_typ dhcp_mon_status_t dhcp_device_get_status(const std::string &ifname, dhcp_device_check_t check_type) { - if (sock_mgr_counters_unchanged(ifname, (const int *)monitored_msgs, monitored_msg_sz, (const int *)monitored_v6_msgs, monitored_v6_msg_sz)) { + bool aggregate_check = + check_type == DHCP_DEVICE_CHECK_AGG_RX || + check_type == DHCP_DEVICE_CHECK_AGG_TX || + check_type == DHCP_DEVICE_CHECK_AGG_RX_V6 || + check_type == DHCP_DEVICE_CHECK_AGG_TX_V6; + if (!aggregate_check && + sock_mgr_counters_unchanged(ifname, (const int *)monitored_msgs, monitored_msg_sz, + (const int *)monitored_v6_msgs, monitored_v6_msg_sz)) { return DHCP_MON_STATUS_INDETERMINATE; } @@ -395,27 +384,17 @@ dhcp_mon_status_t dhcp_device_get_status(const std::string &ifname, dhcp_device_ return dhcp_device_check_positive_health_v6(ifname); case DHCP_DEVICE_CHECK_NEGATIVE_V6: return dhcp_device_check_negative_health_v6(ifname); - case DHCP_DEVICE_CHECK_AGG_EQUAL_RX: - return dhcp_device_check_agg_equal_rx(ifname); - case DHCP_DEVICE_CHECK_AGG_EQUAL_TX: - return dhcp_device_check_agg_equal_tx(ifname); - case DHCP_DEVICE_CHECK_AGG_EQUAL_RX_V6: - return dhcp_device_check_agg_equal_rx_v6(ifname); - case DHCP_DEVICE_CHECK_AGG_EQUAL_TX_V6: - return dhcp_device_check_agg_equal_tx_v6(ifname); - case DHCP_DEVICE_CHECK_AGG_MULTIPLE_RX: - return dhcp_device_check_agg_multiple_rx(ifname); - case DHCP_DEVICE_CHECK_AGG_MULTIPLE_TX: - return dhcp_device_check_agg_multiple_tx(ifname); - case DHCP_DEVICE_CHECK_AGG_MULTIPLE_RX_V6: - return dhcp_device_check_agg_multiple_rx_v6(ifname); - case DHCP_DEVICE_CHECK_AGG_MULTIPLE_TX_V6: - return dhcp_device_check_agg_multiple_tx_v6(ifname); + case DHCP_DEVICE_CHECK_AGG_RX: + return check_aggregate_health(ifname, rx_sock, (const int *)monitored_msgs, monitored_msg_sz); + case DHCP_DEVICE_CHECK_AGG_TX: + return check_aggregate_health(ifname, tx_sock, (const int *)monitored_msgs, monitored_msg_sz); + case DHCP_DEVICE_CHECK_AGG_RX_V6: + return check_aggregate_health(ifname, rx_sock_v6, (const int *)monitored_v6_msgs, monitored_v6_msg_sz); + case DHCP_DEVICE_CHECK_AGG_TX_V6: + return check_aggregate_health(ifname, tx_sock_v6, (const int *)monitored_v6_msgs, monitored_v6_msg_sz); default: - break; + return DHCP_MON_STATUS_UNHEALTHY; } - - return DHCP_MON_STATUS_UNHEALTHY; } int initialize_intf_mac_and_ip_addr(dhcp_device_context_t *context) diff --git a/src/dhcp_device.h b/src/dhcp_device.h index 5d4b00721..7ca677850 100644 --- a/src/dhcp_device.h +++ b/src/dhcp_device.h @@ -136,14 +136,10 @@ typedef enum DHCP_DEVICE_CHECK_POSITIVE, /** Validate that received DORA packets are relayed */ DHCP_DEVICE_CHECK_NEGATIVE_V6, /** Presence of relayed DHCPv6 packets activity is flagged as unhealthy state */ DHCP_DEVICE_CHECK_POSITIVE_V6, /** Validate that received SARR packets are relayed */ - DHCP_DEVICE_CHECK_AGG_EQUAL_RX, /** Validate that aggregate device rx counters equal sum of member interfaces rx counters */ - DHCP_DEVICE_CHECK_AGG_EQUAL_TX, /** Validate that aggregate device tx counters equal sum of member interfaces tx counters */ - DHCP_DEVICE_CHECK_AGG_EQUAL_RX_V6, /** Validate that aggregate device rx counters equal sum of member interfaces rx counters for IPv6 */ - DHCP_DEVICE_CHECK_AGG_EQUAL_TX_V6, /** Validate that aggregate device tx counters equal sum of member interfaces tx counters for IPv6 */ - DHCP_DEVICE_CHECK_AGG_MULTIPLE_RX, /** Validate that aggregate device rx counters are multiple of member interfaces rx counters */ - DHCP_DEVICE_CHECK_AGG_MULTIPLE_TX, /** Validate that aggregate device tx counters are multiple of member interfaces tx counters */ - DHCP_DEVICE_CHECK_AGG_MULTIPLE_RX_V6, /** Validate that aggregate device rx counters are multiple of member interfaces rx counters for IPv6 */ - DHCP_DEVICE_CHECK_AGG_MULTIPLE_TX_V6 /** Validate that aggregate device tx counters are multiple of member interfaces tx counters for IPv6 */ + DHCP_DEVICE_CHECK_AGG_RX, /** Compare IPv4 RX on a parent interface with its member aggregate */ + DHCP_DEVICE_CHECK_AGG_TX, /** Compare IPv4 TX on a parent interface with its member aggregate */ + DHCP_DEVICE_CHECK_AGG_RX_V6, /** Compare IPv6 RX on a parent interface with its member aggregate */ + DHCP_DEVICE_CHECK_AGG_TX_V6 /** Compare IPv6 TX on a parent interface with its member aggregate */ } dhcp_device_check_t; /** Monitored DHCP message type */ diff --git a/src/dhcp_mon.cpp b/src/dhcp_mon.cpp index 654c3c1b1..8798c0182 100644 --- a/src/dhcp_mon.cpp +++ b/src/dhcp_mon.cpp @@ -524,6 +524,7 @@ int dhcp_mon_init(size_t snaplen, int window_sec, int max_count, int db_update_i // deinitialization of counters is not our responsibility // cache counter will be cleanup by sock_mgr_free and the initialized db we intend to keep initialize_all_intf_counters(); + initialize_dhcp_relay_health(); syslog(LOG_INFO, "Initialized all counters for tracked interfaces"); window_interval_sec = window_sec; diff --git a/src/health_check.cpp b/src/health_check.cpp index e9949d18c..8ea39d3ff 100644 --- a/src/health_check.cpp +++ b/src/health_check.cpp @@ -7,6 +7,7 @@ #include #include #include +#include #include "health_check.h" @@ -22,202 +23,98 @@ int dhcp_unhealthy_max_count = 10; extern std::string mgmt_ifname; extern std::string agg_dev_all; -extern std::string agg_dev_prefix; extern std::unordered_map> rev_vlan_map; extern std::unordered_map> rev_portchan_map; -static dhcp_mon_status_t check_agg_health() -{ - return dhcp_device_get_status(agg_dev_all, DHCP_DEVICE_CHECK_POSITIVE); -} - -static dhcp_mon_status_t check_mgmt_health() -{ - if (mgmt_ifname.size() > 0) { - return dhcp_device_get_status(mgmt_ifname, DHCP_DEVICE_CHECK_NEGATIVE); - } - return DHCP_MON_STATUS_HEALTHY; -} +static const char relay_disparity_error[] = + "dhcpmon detected DHCPv4/v6 packets received but none transmitted for intf: %s. Duration: %d (sec)"; +static const char mgmt_error[] = + "dhcpmon detected DHCP packets traveling through mgmt interface (please check BGP routes.)" + " Intf: %s. Duration: %d (sec)"; +static const char agg_rx_disparity_error[] = + "dhcpmon detected an IPv4 RX disparity between interface %s and the aggregate of its member interface counters." + " Duration: %d (sec)"; +static const char agg_tx_disparity_error[] = + "dhcpmon detected an IPv4 TX disparity between interface %s and the aggregate of its member interface counters." + " Duration: %d (sec)"; +static const char agg_rx_v6_disparity_error[] = + "dhcpmon detected an IPv6 RX disparity between interface %s and the aggregate of its member interface counters." + " Duration: %d (sec)"; +static const char agg_tx_v6_disparity_error[] = + "dhcpmon detected an IPv6 TX disparity between interface %s and the aggregate of its member interface counters." + " Duration: %d (sec)"; +/** + * @code alert_dhcp_relay_disparity(duration); + * + * @brief Publish the existing DHCP relay disparity event + * + * @param duration Unhealthy duration in seconds + * + * @return None + */ static void alert_dhcp_relay_disparity(int duration) { event_params_t params = {{ "vlan", agg_dev_all}, { "duration", std::to_string(duration)}}; event_publish(g_events_handle, "dhcp-relay-disparity", ¶ms); } -static void log_agg_error(int duration) -{ - syslog(LOG_ALERT, "dhcpmon detected DHCPv4/v6 packets received but none transmitted. Duration: %d (sec) for intf: %s", - duration, agg_dev_all.c_str()); -} +static std::vector state_data; -static void log_mgmt_error(int duration) +void initialize_dhcp_relay_health() { - syslog(LOG_ALERT, "dhcpmon detected DHCP packets traveling through mgmt interface (please check BGP routes.)" - " Duration: %d (sec) for intf: %s", - duration, mgmt_ifname.c_str()); -} + state_data.clear(); -static dhcp_mon_status_t check_agg_health_v6() -{ - return dhcp_device_get_status(agg_dev_all, DHCP_DEVICE_CHECK_POSITIVE_V6); -} - -static dhcp_mon_status_t check_mgmt_health_v6() -{ + state_data.push_back({agg_dev_all, DHCP_DEVICE_CHECK_POSITIVE, alert_dhcp_relay_disparity, + relay_disparity_error, 0}); if (mgmt_ifname.size() > 0) { - return dhcp_device_get_status(mgmt_ifname, DHCP_DEVICE_CHECK_NEGATIVE_V6); - } - return DHCP_MON_STATUS_HEALTHY; -} - -static dhcp_mon_status_t check_per_interface_rx_health() -{ - for (const auto &[vlan, _] : rev_vlan_map) { - if (dhcp_device_get_status(vlan, DHCP_DEVICE_CHECK_AGG_EQUAL_RX) == DHCP_MON_STATUS_UNHEALTHY) { - return DHCP_MON_STATUS_UNHEALTHY; - } - } - for (const auto &[portchan, _] : rev_portchan_map) { - if (dhcp_device_get_status(portchan, DHCP_DEVICE_CHECK_AGG_EQUAL_RX) == DHCP_MON_STATUS_UNHEALTHY) { - return DHCP_MON_STATUS_UNHEALTHY; - } - } - return DHCP_MON_STATUS_HEALTHY; -} - -static void log_agg_per_interface_rx_error(int duration) -{ - syslog(LOG_ALERT, "sum of rx per interface counter does not equal corresponding vlan/portchan counter." - " Duration: %d (sec)", duration); -} - -static dhcp_mon_status_t check_per_interface_tx_health() -{ - for (const auto &[vlan, _] : rev_vlan_map) { - if (dhcp_device_get_status(vlan, DHCP_DEVICE_CHECK_AGG_MULTIPLE_TX) == DHCP_MON_STATUS_UNHEALTHY) { - return DHCP_MON_STATUS_UNHEALTHY; - } + state_data.push_back({mgmt_ifname, DHCP_DEVICE_CHECK_NEGATIVE, NULL, mgmt_error, 0}); } - for (const auto &[portchan, _] : rev_portchan_map) { - if (dhcp_device_get_status(portchan, DHCP_DEVICE_CHECK_AGG_EQUAL_TX) == DHCP_MON_STATUS_UNHEALTHY) { - return DHCP_MON_STATUS_UNHEALTHY; - } + state_data.push_back({agg_dev_all, DHCP_DEVICE_CHECK_POSITIVE_V6, alert_dhcp_relay_disparity, + relay_disparity_error, 0}); + if (mgmt_ifname.size() > 0) { + state_data.push_back({mgmt_ifname, DHCP_DEVICE_CHECK_NEGATIVE_V6, NULL, mgmt_error, 0}); } - return DHCP_MON_STATUS_HEALTHY; -} - -static void log_agg_per_interface_tx_error(int duration) -{ - syslog(LOG_ALERT, "each tx per interface counter does not equal corresponding vlan counter," - " or sum of tx per interface counter does not equal corresponding portchan counter." - " Duration: %d (sec)", duration); -} -static dhcp_mon_status_t check_per_interface_rx_health_v6() -{ for (const auto &[vlan, _] : rev_vlan_map) { - if (dhcp_device_get_status(vlan, DHCP_DEVICE_CHECK_AGG_EQUAL_RX_V6) == DHCP_MON_STATUS_UNHEALTHY) { - return DHCP_MON_STATUS_UNHEALTHY; - } + state_data.push_back({vlan, DHCP_DEVICE_CHECK_AGG_RX, NULL, agg_rx_disparity_error, 0}); + state_data.push_back({vlan, DHCP_DEVICE_CHECK_AGG_TX, NULL, agg_tx_disparity_error, 0}); + state_data.push_back({vlan, DHCP_DEVICE_CHECK_AGG_RX_V6, NULL, agg_rx_v6_disparity_error, 0}); + state_data.push_back({vlan, DHCP_DEVICE_CHECK_AGG_TX_V6, NULL, agg_tx_v6_disparity_error, 0}); } - for (const auto &[portchan, _] : rev_portchan_map) { - if (dhcp_device_get_status(portchan, DHCP_DEVICE_CHECK_AGG_EQUAL_RX_V6) == DHCP_MON_STATUS_UNHEALTHY) { - return DHCP_MON_STATUS_UNHEALTHY; - } - } - return DHCP_MON_STATUS_HEALTHY; -} -static dhcp_mon_status_t check_per_interface_tx_health_v6() -{ - for (const auto &[vlan, _] : rev_vlan_map) { - if (dhcp_device_get_status(vlan, DHCP_DEVICE_CHECK_AGG_MULTIPLE_TX_V6) == DHCP_MON_STATUS_UNHEALTHY) { - return DHCP_MON_STATUS_UNHEALTHY; - } - } for (const auto &[portchan, _] : rev_portchan_map) { - if (dhcp_device_get_status(portchan, DHCP_DEVICE_CHECK_AGG_EQUAL_TX_V6) == DHCP_MON_STATUS_UNHEALTHY) { - return DHCP_MON_STATUS_UNHEALTHY; - } + state_data.push_back({portchan, DHCP_DEVICE_CHECK_AGG_RX, NULL, agg_rx_disparity_error, 0}); + state_data.push_back({portchan, DHCP_DEVICE_CHECK_AGG_TX, NULL, agg_tx_disparity_error, 0}); + state_data.push_back({portchan, DHCP_DEVICE_CHECK_AGG_RX_V6, NULL, agg_rx_v6_disparity_error, 0}); + state_data.push_back({portchan, DHCP_DEVICE_CHECK_AGG_TX_V6, NULL, agg_tx_v6_disparity_error, 0}); } - return DHCP_MON_STATUS_HEALTHY; } -/** DHCP monitor state data for aggregate device for mgmt device */ -static dhcp_mon_state_t state_data[] = { - [0] = { - .check_health = check_agg_health, - .alert = alert_dhcp_relay_disparity, - .log = log_agg_error, - .count = 0, - }, - [1] = { - .check_health = check_mgmt_health, - .log = log_mgmt_error, - .count = 0, - }, - [2] = { - .check_health = check_agg_health_v6, - .alert = alert_dhcp_relay_disparity, - .log = log_agg_error, - .count = 0, - }, - [3] = { - .check_health = check_mgmt_health_v6, - .log = log_mgmt_error, - .count = 0, - }, - [4] = { - .check_health = check_per_interface_rx_health, - .log = log_agg_per_interface_rx_error, - .count = 0, - }, - [5] = { - .check_health = check_per_interface_tx_health, - .log = log_agg_per_interface_tx_error, - .count = 0, - }, - [6] = { - .check_health = check_per_interface_rx_health_v6, - .log = log_agg_per_interface_rx_error, - .count = 0, - }, - [7] = { - .check_health = check_per_interface_tx_health_v6, - .log = log_agg_per_interface_tx_error, - .count = 0, - }, -}; - -static size_t state_data_sz = sizeof(state_data) / sizeof(*state_data); - void check_dhcp_relay_health() { syslog_debug(LOG_INFO, "Checking DHCP relay health"); - for (uint8_t i = 0; i < state_data_sz; i++) { - dhcp_mon_status_t dhcp_mon_status = state_data[i].check_health(); + for (auto &state : state_data) { + dhcp_mon_status_t dhcp_mon_status = dhcp_device_get_status(state.ifname, state.check_type); switch (dhcp_mon_status) { case DHCP_MON_STATUS_UNHEALTHY: - if (++state_data[i].count > dhcp_unhealthy_max_count) { - int duration = state_data[i].count * window_interval_sec; + if (++state.count > dhcp_unhealthy_max_count) { + int duration = state.count * window_interval_sec; - if (state_data[i].alert) { - state_data[i].alert(duration); - } - if (state_data[i].log) { - state_data[i].log(duration); + if (state.alert) { + state.alert(duration); } + syslog(LOG_ALERT, state.error_format, state.ifname.c_str(), duration); } break; case DHCP_MON_STATUS_HEALTHY: - state_data[i].count = 0; + state.count = 0; break; case DHCP_MON_STATUS_INDETERMINATE: - if (state_data[i].count) { - state_data[i].count++; + if (state.count) { + state.count++; } break; default: diff --git a/src/health_check.h b/src/health_check.h index 552dd528c..16243438a 100644 --- a/src/health_check.h +++ b/src/health_check.h @@ -9,14 +9,16 @@ #include "dhcp_device.h" #include +#include /** DHCP device/interface state */ typedef struct { - dhcp_mon_status_t (*check_health)(); /** check function */ - void (*alert)(int duration); /** alert function when check failed */ - void (*log)(int duration); /** log function when check passed */ - int count; /** count in the number of unhealthy checks */ + std::string ifname; /** interface to check */ + dhcp_device_check_t check_type; /** check to apply */ + void (*alert)(int duration); /** alert function */ + const char *error_format; /** threshold error format */ + int count; /** consecutive unhealthy checks */ } dhcp_mon_state_t; extern event_handle_t g_events_handle; @@ -26,7 +28,18 @@ extern int window_interval_sec; extern int dhcp_unhealthy_max_count; /** - * @code check_dhcp_relay_health(state_data); + * @code initialize_dhcp_relay_health(); + * + * @brief Populate health states from discovered VLAN and PortChannel members + * + * @param none + * + * @return none + */ +void initialize_dhcp_relay_health(); + +/** + * @code check_dhcp_relay_health(); * * @brief check DHCP relay overall health * From 95df31bd988079949ed970f56d6363a42a3a0e90 Mon Sep 17 00:00:00 2001 From: Xichen96 Date: Sat, 1 Aug 2026 03:54:43 +1000 Subject: [PATCH 4/6] [dhcpmon]: Validate DHCPv6 relay transformations Replace invalid same-message-type DHCPv6 disparity checks with relay-aware activity checks. Require RX Solicit, Request, or Relay-Forward activity to produce TX Relay-Forward activity, and RX Relay-Reply activity to produce TX Advertise, Reply, or Relay-Reply activity. Keep direct lookups for the single Relay-Forward and Relay-Reply counters and compare activity rather than magnitude so multiple configured servers remain valid. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 39f979be-d826-4d5c-949a-f20abb58bb83 Signed-off-by: Xichen96 --- src/dhcp_device.cpp | 87 ++++++++++++++++++++++++++++++--------------- src/dhcp_device.h | 2 +- 2 files changed, 60 insertions(+), 29 deletions(-) diff --git a/src/dhcp_device.cpp b/src/dhcp_device.cpp index e7e22ea2a..386af194b 100644 --- a/src/dhcp_device.cpp +++ b/src/dhcp_device.cpp @@ -51,6 +51,18 @@ const dhcpv6_message_type_t monitored_v6_msgs[] = { uint8_t monitored_v6_msg_sz = sizeof(monitored_v6_msgs) / sizeof(*monitored_v6_msgs); +static const dhcpv6_message_type_t monitored_v6_forward_rx_msgs[] = { + DHCPV6_MESSAGE_TYPE_SOLICIT, + DHCPV6_MESSAGE_TYPE_REQUEST, + DHCPV6_MESSAGE_TYPE_RELAY_FORW +}; + +static const dhcpv6_message_type_t monitored_v6_reply_tx_msgs[] = { + DHCPV6_MESSAGE_TYPE_ADVERTISE, + DHCPV6_MESSAGE_TYPE_REPLY, + DHCPV6_MESSAGE_TYPE_RELAY_REPL +}; + const char *intf_type_name[DHCP_DEVICE_INTF_TYPE_COUNT] = { [DHCP_DEVICE_INTF_TYPE_UPLINK] = "uplink (north)", [DHCP_DEVICE_INTF_TYPE_DOWNLINK] = "downlink (south)", @@ -99,31 +111,18 @@ static bool check_counter_not_transmitted(const std::string &ifname, int rx_sock } /** - * @code dhcp_device_check_positive_health(ifname); - * @brief Check that DHCP relayed messages are being transmitted out of this interface/dev - * using its counters. The interface is positively healthy if there are DHCP message - * travelling through it. - * @param ifname interface name - * @return DHCP_MON_STATUS_HEALTHY, DHCP_MON_STATUS_UNHEALTHY, or DHCP_MON_STATUS_INDETERMINATE - */ -static dhcp_mon_status_t dhcp_device_check_positive_health(const std::string &ifname) -{ - return check_counter_not_transmitted(ifname, rx_sock, tx_sock, (const int *)monitored_msgs, monitored_msg_sz) ? - DHCP_MON_STATUS_UNHEALTHY : DHCP_MON_STATUS_HEALTHY; -} - -/** - * @code dhcp_device_check_positive_health_v6(ifname); - * @brief Check that DHCPv6 relayed messages are being transmitted out of this interface/dev - * using its counters. The interface is positively healthy if there are DHCPv6 message - * travelling through it. - * @param ifname interface name - * @return DHCP_MON_STATUS_HEALTHY, DHCP_MON_STATUS_UNHEALTHY, or DHCP_MON_STATUS_INDETERMINATE + * @code get_counter_delta(ifname, sock, msg_type); + * @brief Get the increase in one message-type counter since the last snapshot. + * @param ifname interface name + * @param sock socket containing the counter + * @param msg_type message type + * @return counter increase since the last snapshot */ -static dhcp_mon_status_t dhcp_device_check_positive_health_v6(const std::string &ifname) +static uint64_t get_counter_delta(const std::string &ifname, int sock, int msg_type) { - return check_counter_not_transmitted(ifname, rx_sock_v6, tx_sock_v6, (const int *)monitored_v6_msgs, monitored_v6_msg_sz) ? - DHCP_MON_STATUS_UNHEALTHY : DHCP_MON_STATUS_HEALTHY; + const sock_info_t &sock_info = sock_mgr_get_sock_info(sock); + return sock_info.all_counters.at(ifname).at(msg_type) - + sock_info.all_counters_snapshot.at(ifname).at(msg_type); } /** @@ -140,19 +139,51 @@ static dhcp_mon_status_t dhcp_device_check_positive_health_v6(const std::string */ static bool check_counter_increased(const std::string &ifname, int sock, const int *monitored_msgs, size_t monitored_msg_cnt) { - const sock_info_t &sock_info = sock_mgr_get_sock_info(sock); - const counter_t &counters = sock_info.all_counters.at(ifname); - const counter_t &counters_snapshot = sock_info.all_counters_snapshot.at(ifname); - // true if any counter has increased for (size_t i = 0; i < monitored_msg_cnt; i++) { - if (counters.at(monitored_msgs[i]) > counters_snapshot.at(monitored_msgs[i])) { + if (get_counter_delta(ifname, sock, monitored_msgs[i]) > 0) { return true; } } return false; } +/** + * @code dhcp_device_check_positive_health(ifname); + * @brief Check that RX Discover, Offer, Request, and Ack activity has matching same-type TX activity. + * @param ifname interface name + * @return DHCP_MON_STATUS_HEALTHY, DHCP_MON_STATUS_UNHEALTHY, or DHCP_MON_STATUS_INDETERMINATE + */ +static dhcp_mon_status_t dhcp_device_check_positive_health(const std::string &ifname) +{ + return check_counter_not_transmitted(ifname, rx_sock, tx_sock, (const int *)monitored_msgs, monitored_msg_sz) ? + DHCP_MON_STATUS_UNHEALTHY : DHCP_MON_STATUS_HEALTHY; +} + +/** + * @code dhcp_device_check_positive_health_v6(ifname); + * @brief Check that RX Solicit, Request, or Relay-Forward activity has TX Relay-Forward activity, + * and RX Relay-Reply activity has TX Advertise, Reply, or Relay-Reply activity. + * @param ifname interface name + * @return DHCP_MON_STATUS_HEALTHY, DHCP_MON_STATUS_UNHEALTHY, or DHCP_MON_STATUS_INDETERMINATE + */ +static dhcp_mon_status_t dhcp_device_check_positive_health_v6(const std::string &ifname) +{ + const bool forward_rx = check_counter_increased( + ifname, rx_sock_v6, (const int *)monitored_v6_forward_rx_msgs, + sizeof(monitored_v6_forward_rx_msgs) / sizeof(*monitored_v6_forward_rx_msgs)); + const bool forward_tx = get_counter_delta(ifname, tx_sock_v6, DHCPV6_MESSAGE_TYPE_RELAY_FORW) > 0; + const bool reply_rx = get_counter_delta(ifname, rx_sock_v6, DHCPV6_MESSAGE_TYPE_RELAY_REPL) > 0; + const bool reply_tx = check_counter_increased( + ifname, tx_sock_v6, (const int *)monitored_v6_reply_tx_msgs, + sizeof(monitored_v6_reply_tx_msgs) / sizeof(*monitored_v6_reply_tx_msgs)); + + if ((forward_rx && !forward_tx) || (reply_rx && !reply_tx)) { + return DHCP_MON_STATUS_UNHEALTHY; + } + return forward_rx || reply_rx ? DHCP_MON_STATUS_HEALTHY : DHCP_MON_STATUS_INDETERMINATE; +} + /** * @code dhcp_device_check_negative_health(ifname); * diff --git a/src/dhcp_device.h b/src/dhcp_device.h index 7ca677850..e8f4411f6 100644 --- a/src/dhcp_device.h +++ b/src/dhcp_device.h @@ -135,7 +135,7 @@ typedef enum DHCP_DEVICE_CHECK_NEGATIVE, /** Presence of relayed DHCP packets activity is flagged as unhealthy state */ DHCP_DEVICE_CHECK_POSITIVE, /** Validate that received DORA packets are relayed */ DHCP_DEVICE_CHECK_NEGATIVE_V6, /** Presence of relayed DHCPv6 packets activity is flagged as unhealthy state */ - DHCP_DEVICE_CHECK_POSITIVE_V6, /** Validate that received SARR packets are relayed */ + DHCP_DEVICE_CHECK_POSITIVE_V6, /** Validate SARR and DHCPv6 relay-wrapper transformations */ DHCP_DEVICE_CHECK_AGG_RX, /** Compare IPv4 RX on a parent interface with its member aggregate */ DHCP_DEVICE_CHECK_AGG_TX, /** Compare IPv4 TX on a parent interface with its member aggregate */ DHCP_DEVICE_CHECK_AGG_RX_V6, /** Compare IPv6 RX on a parent interface with its member aggregate */ From 6913601db9f1762bfc7832ee2f0767088e42b193 Mon Sep 17 00:00:00 2001 From: Xichen96 Date: Sat, 1 Aug 2026 03:55:02 +1000 Subject: [PATCH 5/6] [dhcpmon]: Validate downstream reply fan-out Allow first-hop DHCPOFFER, DHCPACK, and DHCPNAK profile validation without a destination-IP assumption. Require each IPv4 VLAN TX packet to appear on either one direct member for unicast or every direct member for broadcast; keep all other parent/member comparisons exact. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 39f979be-d826-4d5c-949a-f20abb58bb83 Signed-off-by: Xichen96 --- src/dhcp_check_profile_relay.cpp | 4 +-- src/dhcp_device.cpp | 55 ++++++++++++++------------------ 2 files changed, 26 insertions(+), 33 deletions(-) diff --git a/src/dhcp_check_profile_relay.cpp b/src/dhcp_check_profile_relay.cpp index a5635066f..41088555e 100644 --- a/src/dhcp_check_profile_relay.cpp +++ b/src/dhcp_check_profile_relay.cpp @@ -62,14 +62,14 @@ dhcp_check_profile_t dhcp_check_profile_first_relay_rx = { }; // DHCP messages sent to client -// Relay sends reply packets to client with broadcast ip, and giaddr remains the first relay identifier. +// Relay replies may use broadcast or unicast IP based on the client's broadcast flag, not a relay decision. +// SONiC relay supports broadcast only while ISC supports both; giaddr remains the first relay identifier. // In single-ToR, giaddr_ip and vlan_ip are the same downstream VLAN SVI address. In dualtor, giaddr_ip is // Loopback0 for the server-facing relay identity, but client-facing replies are still sent from the downstream // VLAN SVI. Therefore src ip should be vlan_ip, while giaddr should remain giaddr_ip. static dhcp_msg_check_profile_t tx_first_relay_reply = { {DHCP_CHECK_INTF_TYPE, (const void *)(new std::vector{DHCP_DEVICE_INTF_TYPE_DOWNLINK, DHCP_DEVICE_INTF_TYPE_MGMT})}, {DHCP_CHECK_SRC_IP, (const void *)(new std::vector{&vlan_ip})}, - {DHCP_CHECK_DST_IP, (const void *)(new std::vector{&broadcast_ip})}, {DHCP_CHECK_GIADDR, (const void *)(new std::vector{&giaddr_ip})}, }; diff --git a/src/dhcp_device.cpp b/src/dhcp_device.cpp index 386af194b..5e8d4ae8a 100644 --- a/src/dhcp_device.cpp +++ b/src/dhcp_device.cpp @@ -219,19 +219,18 @@ static dhcp_mon_status_t dhcp_device_check_negative_health_v6(const std::string } /** - * @code check_counters_delta_expected(ifname, other_ifname, sock, ratio, monitored_msgs, monitored_msg_cnt); - * @brief Check if the delta of counters between current and snapshot for given message types - * match expectation between two interfaces with a given ratio. + * @code check_counters_delta_expected(ifname, other_ifname, sock, member_count, monitored_msgs, monitored_msg_cnt); + * @brief Check if counter deltas match between a parent and its member aggregate. * @param ifname interface name * @param other_ifname other interface name * @param sock socket - * @param ratio expected ratio between two interfaces, ratio = other_ifname / ifname + * @param member_count expected member observations per broadcast packet; one means exact equality * @param monitored_msgs array of monitored message types * @param monitored_msg_cnt number of monitored message types - * @return true if deltas are equal with given ratio, false otherwise + * @return true if deltas match the expected relationship, false otherwise */ static bool check_counters_delta_expected(const std::string &ifname, const std::string &other_ifname, int sock, - size_t ratio, const int *monitored_msgs, size_t monitored_msg_cnt) + size_t member_count, const int *monitored_msgs, size_t monitored_msg_cnt) { const sock_info_t &sock_info = sock_mgr_get_sock_info(sock); const counter_t &counters = sock_info.all_counters.at(ifname); @@ -239,37 +238,28 @@ static bool check_counters_delta_expected(const std::string &ifname, const std:: const counter_t &other_counters = sock_info.all_counters.at(other_ifname); const counter_t &other_counters_snapshot = sock_info.all_counters_snapshot.at(other_ifname); - // for every delta increase in ifname, there is delta * ratio increase in other ifname for (size_t i = 0; i < monitored_msg_cnt; i++) { uint64_t delta = counters.at(monitored_msgs[i]) - counters_snapshot.at(monitored_msgs[i]); uint64_t other_delta = other_counters.at(monitored_msgs[i]) - other_counters_snapshot.at(monitored_msgs[i]); - if (delta * ratio != other_delta) { + if (member_count <= 1) { + if (delta != other_delta) { + return false; + } + continue; + } + + if (other_delta < delta) { + return false; + } + uint64_t additional_member_observations = other_delta - delta; + if (additional_member_observations % (member_count - 1) != 0 || + additional_member_observations / (member_count - 1) > delta) { return false; } } return true; } -/** - * @code get_aggregate_ratio(ifname, sock); - * - * @brief Get the expected aggregate ratio for an interface and packet direction - * - * @param ifname Parent interface name - * @param sock Socket identifying the protocol family and direction - * - * @return One for RX and PortChannel TX; direct VLAN member count for VLAN TX - */ -static size_t get_aggregate_ratio(const std::string &ifname, int sock) -{ - // RX crosses one edge. VLAN TX is observed on every direct member; PortChannel TX is observed on one member. - if (sock_mgr_get_sock_info(sock).is_rx) { - return 1; - } - const auto vlan = rev_vlan_map.find(ifname); - return vlan == rev_vlan_map.end() ? 1 : vlan->second.size(); -} - /** * @code check_aggregate_health(ifname, sock, monitored_msgs, monitored_msg_cnt); * @@ -280,19 +270,22 @@ static size_t get_aggregate_ratio(const std::string &ifname, int sock) * @param monitored_msgs Message types to compare * @param monitored_msg_cnt Number of message types to compare * - * @return HEALTHY when idle or matching, UNHEALTHY when the expected ratio does not match + * @return HEALTHY when idle or matching, UNHEALTHY when the expected relationship does not match */ static dhcp_mon_status_t check_aggregate_health(const std::string &ifname, int sock, const int *monitored_msgs, size_t monitored_msg_cnt) { const std::string agg_ifname = get_agg_counter_ifname(ifname); + const auto vlan = rev_vlan_map.find(ifname); + // Each IPv4 VLAN TX packet is observed on one member for unicast or all direct members for broadcast. + const size_t member_count = sock == tx_sock && vlan != rev_vlan_map.end() && !vlan->second.empty() ? + vlan->second.size() : 1; if (!check_counter_increased(ifname, sock, monitored_msgs, monitored_msg_cnt) && !check_counter_increased(agg_ifname, sock, monitored_msgs, monitored_msg_cnt)) { return DHCP_MON_STATUS_HEALTHY; } return check_counters_delta_expected( - ifname, agg_ifname, sock, get_aggregate_ratio(ifname, sock), - monitored_msgs, monitored_msg_cnt) ? + ifname, agg_ifname, sock, member_count, monitored_msgs, monitored_msg_cnt) ? DHCP_MON_STATUS_HEALTHY : DHCP_MON_STATUS_UNHEALTHY; } From ed3a5f83f6790c21a641297374e3dd5bd986010a Mon Sep 17 00:00:00 2001 From: Xichen96 Date: Sat, 1 Aug 2026 03:55:20 +1000 Subject: [PATCH 6/6] [dhcpmon]: Tolerate one packet across health windows Allow one logical packet to cross either snapshot boundary when validating DORA, DHCPv6 relay transformations, and parent/member counter relationships. Keep the checks stateless so old discrepancies do not persist into later windows. Preserve the existing IPv4 healthy result when no disparity is found, so IPv6-only activity still clears a prior IPv4 unhealthy count. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 39f979be-d826-4d5c-949a-f20abb58bb83 Signed-off-by: Xichen96 --- src/dhcp_device.cpp | 157 +++++++++++++++++++++++++++----------------- 1 file changed, 95 insertions(+), 62 deletions(-) diff --git a/src/dhcp_device.cpp b/src/dhcp_device.cpp index 5e8d4ae8a..704e380d6 100644 --- a/src/dhcp_device.cpp +++ b/src/dhcp_device.cpp @@ -77,38 +77,8 @@ static const char *counter_desc[DHCP_COUNTERS_COUNT] = { [DHCP_COUNTERS_SNAPSHOT_V6] = "Snapshot_V6", }; -/** - * @code check_counter_not_transmitted(ifname, rx_sock, tx_sock, monitored_msgs, monitored_msg_cnt); - * @brief Check if there are received DHCP messages that are not transmitted out - * of this interface/device using its counters. - * @param ifname interface name - * @param rx_sock rx socket - * @param tx_sock tx socket - * @param monitored_msgs array of monitored message types - * @param monitored_msg_cnt number of monitored message types - * @return true if there are received messages not transmitted out, false otherwise - */ -// these helpers use const int * to accept both dhcp_message_type_t and dhcpv6_message_type_t arrays -// without duplicating the function for each enum type; safe on GCC/Linux where unscoped enums use int -static bool check_counter_not_transmitted(const std::string &ifname, int rx_sock, int tx_sock, const int *monitored_msgs, size_t monitored_msg_cnt) -{ - const sock_info_t &rx_sock_info = sock_mgr_get_sock_info(rx_sock); - const counter_t &rx_counters = rx_sock_info.all_counters.at(ifname); - const counter_t &rx_counters_snapshot = rx_sock_info.all_counters_snapshot.at(ifname); - - const sock_info_t &tx_sock_info = sock_mgr_get_sock_info(tx_sock); - const counter_t &tx_counters = tx_sock_info.all_counters.at(ifname); - const counter_t &tx_counters_snapshot = tx_sock_info.all_counters_snapshot.at(ifname); - - // when there is packet in, no packet out - for (size_t i = 0; i < monitored_msg_cnt; i++) { - if (rx_counters.at(monitored_msgs[i]) > rx_counters_snapshot.at(monitored_msgs[i]) && - tx_counters.at(monitored_msgs[i]) <= tx_counters_snapshot.at(monitored_msgs[i])) { - return true; - } - } - return false; -} +/** Logical packets allowed to cross either health snapshot boundary */ +static constexpr uint64_t INFLIGHT_PACKET_TOLERANCE = 1; /** * @code get_counter_delta(ifname, sock, msg_type); @@ -125,6 +95,45 @@ static uint64_t get_counter_delta(const std::string &ifname, int sock, int msg_t sock_info.all_counters_snapshot.at(ifname).at(msg_type); } +/** + * @code check_counter_not_transmitted(ifname, rx_sock, tx_sock, monitored_msgs, monitored_msg_cnt); + * @brief Check whether RX exceeds the tolerance while no same-type TX activity is observed. + * @param ifname interface name + * @param rx_sock RX socket + * @param tx_sock TX socket + * @param monitored_msgs message types to compare + * @param monitored_msg_cnt number of message types + * @return true when any message type exceeds the tolerance + */ +static bool check_counter_not_transmitted(const std::string &ifname, int rx_sock, int tx_sock, + const int *monitored_msgs, size_t monitored_msg_cnt) +{ + for (size_t i = 0; i < monitored_msg_cnt; i++) { + const uint64_t rx_delta = get_counter_delta(ifname, rx_sock, monitored_msgs[i]); + const uint64_t tx_delta = get_counter_delta(ifname, tx_sock, monitored_msgs[i]); + if (rx_delta > INFLIGHT_PACKET_TOLERANCE && tx_delta == 0) { + return true; + } + } + return false; +} + +/** + * @code counter_delta_within_tolerance(input_delta, output_delta, ratio); + * @brief Check an expected ratio while allowing configured logical packets across either snapshot boundary. + * @param input_delta input packet count in the current window + * @param output_delta output packet count in the current window + * @param ratio expected output packets per input packet + * @return true when output is within [(input - tolerance) * ratio, (input + tolerance) * ratio] + */ +static bool counter_delta_within_tolerance(uint64_t input_delta, uint64_t output_delta, size_t ratio) +{ + const uint64_t lower_input = input_delta > INFLIGHT_PACKET_TOLERANCE ? + input_delta - INFLIGHT_PACKET_TOLERANCE : 0; + return output_delta >= lower_input * ratio && + output_delta <= (input_delta + INFLIGHT_PACKET_TOLERANCE) * ratio; +} + /** * @code check_counter_increased(ifname, sock, monitored_msgs, monitored_msg_cnt); * @@ -150,38 +159,45 @@ static bool check_counter_increased(const std::string &ifname, int sock, const i /** * @code dhcp_device_check_positive_health(ifname); - * @brief Check that RX Discover, Offer, Request, and Ack activity has matching same-type TX activity. + * @brief Check DORA RX/TX relationships with configured logical-packet snapshot tolerance. * @param ifname interface name * @return DHCP_MON_STATUS_HEALTHY, DHCP_MON_STATUS_UNHEALTHY, or DHCP_MON_STATUS_INDETERMINATE */ static dhcp_mon_status_t dhcp_device_check_positive_health(const std::string &ifname) { - return check_counter_not_transmitted(ifname, rx_sock, tx_sock, (const int *)monitored_msgs, monitored_msg_sz) ? - DHCP_MON_STATUS_UNHEALTHY : DHCP_MON_STATUS_HEALTHY; + if (check_counter_not_transmitted( + ifname, rx_sock, tx_sock, (const int *)monitored_msgs, monitored_msg_sz)) { + return DHCP_MON_STATUS_UNHEALTHY; + } + return DHCP_MON_STATUS_HEALTHY; } /** * @code dhcp_device_check_positive_health_v6(ifname); - * @brief Check that RX Solicit, Request, or Relay-Forward activity has TX Relay-Forward activity, - * and RX Relay-Reply activity has TX Advertise, Reply, or Relay-Reply activity. + * @brief Check DHCPv6 forward/reply transformations with configured logical-packet snapshot tolerance. * @param ifname interface name * @return DHCP_MON_STATUS_HEALTHY, DHCP_MON_STATUS_UNHEALTHY, or DHCP_MON_STATUS_INDETERMINATE */ static dhcp_mon_status_t dhcp_device_check_positive_health_v6(const std::string &ifname) { - const bool forward_rx = check_counter_increased( - ifname, rx_sock_v6, (const int *)monitored_v6_forward_rx_msgs, - sizeof(monitored_v6_forward_rx_msgs) / sizeof(*monitored_v6_forward_rx_msgs)); - const bool forward_tx = get_counter_delta(ifname, tx_sock_v6, DHCPV6_MESSAGE_TYPE_RELAY_FORW) > 0; - const bool reply_rx = get_counter_delta(ifname, rx_sock_v6, DHCPV6_MESSAGE_TYPE_RELAY_REPL) > 0; - const bool reply_tx = check_counter_increased( - ifname, tx_sock_v6, (const int *)monitored_v6_reply_tx_msgs, - sizeof(monitored_v6_reply_tx_msgs) / sizeof(*monitored_v6_reply_tx_msgs)); - - if ((forward_rx && !forward_tx) || (reply_rx && !reply_tx)) { + uint64_t forward_rx_delta = 0; + for (const auto msg_type : monitored_v6_forward_rx_msgs) { + forward_rx_delta += get_counter_delta(ifname, rx_sock_v6, msg_type); + } + const uint64_t forward_tx_delta = get_counter_delta(ifname, tx_sock_v6, DHCPV6_MESSAGE_TYPE_RELAY_FORW); + const uint64_t reply_rx_delta = get_counter_delta(ifname, rx_sock_v6, DHCPV6_MESSAGE_TYPE_RELAY_REPL); + uint64_t reply_tx_delta = 0; + for (const auto msg_type : monitored_v6_reply_tx_msgs) { + reply_tx_delta += get_counter_delta(ifname, tx_sock_v6, msg_type); + } + + const bool forward_valid = forward_rx_delta <= INFLIGHT_PACKET_TOLERANCE || forward_tx_delta > 0; + const bool reply_valid = reply_rx_delta <= INFLIGHT_PACKET_TOLERANCE || reply_tx_delta > 0; + if (!forward_valid || !reply_valid) { return DHCP_MON_STATUS_UNHEALTHY; } - return forward_rx || reply_rx ? DHCP_MON_STATUS_HEALTHY : DHCP_MON_STATUS_INDETERMINATE; + return forward_rx_delta > 0 || reply_rx_delta > 0 ? + DHCP_MON_STATUS_HEALTHY : DHCP_MON_STATUS_INDETERMINATE; } /** @@ -218,9 +234,38 @@ static dhcp_mon_status_t dhcp_device_check_negative_health_v6(const std::string DHCP_MON_STATUS_UNHEALTHY : DHCP_MON_STATUS_HEALTHY; } +/** + * @code member_delta_within_tolerance(parent_delta, aggregate_delta, member_count); + * @brief Check exact unicast-or-broadcast fan-out with configured snapshot tolerance. + * @param parent_delta parent interface packet count + * @param aggregate_delta aggregate member-interface packet count + * @param member_count direct member count + * @return true when aggregate fan-out is valid + */ +static bool member_delta_within_tolerance(uint64_t parent_delta, uint64_t aggregate_delta, size_t member_count) +{ + if (member_count <= 1) { + return counter_delta_within_tolerance(parent_delta, aggregate_delta, 1); + } + + if (INFLIGHT_PACKET_TOLERANCE > 0) { + const uint64_t lower_parent = parent_delta > INFLIGHT_PACKET_TOLERANCE ? + parent_delta - INFLIGHT_PACKET_TOLERANCE : 0; + return aggregate_delta >= lower_parent && + aggregate_delta <= (parent_delta + INFLIGHT_PACKET_TOLERANCE) * member_count; + } + + if (aggregate_delta < parent_delta) { + return false; + } + const uint64_t additional_member_observations = aggregate_delta - parent_delta; + return additional_member_observations % (member_count - 1) == 0 && + additional_member_observations / (member_count - 1) <= parent_delta; +} + /** * @code check_counters_delta_expected(ifname, other_ifname, sock, member_count, monitored_msgs, monitored_msg_cnt); - * @brief Check if counter deltas match between a parent and its member aggregate. + * @brief Check parent/member counter deltas with configured snapshot tolerance. * @param ifname interface name * @param other_ifname other interface name * @param sock socket @@ -241,19 +286,7 @@ static bool check_counters_delta_expected(const std::string &ifname, const std:: for (size_t i = 0; i < monitored_msg_cnt; i++) { uint64_t delta = counters.at(monitored_msgs[i]) - counters_snapshot.at(monitored_msgs[i]); uint64_t other_delta = other_counters.at(monitored_msgs[i]) - other_counters_snapshot.at(monitored_msgs[i]); - if (member_count <= 1) { - if (delta != other_delta) { - return false; - } - continue; - } - - if (other_delta < delta) { - return false; - } - uint64_t additional_member_observations = other_delta - delta; - if (additional_member_observations % (member_count - 1) != 0 || - additional_member_observations / (member_count - 1) > delta) { + if (!member_delta_within_tolerance(delta, other_delta, member_count)) { return false; } }