From 076df9aaa5a1df1a05382f156bbbab741046d64b Mon Sep 17 00:00:00 2001 From: Xichen96 Date: Sat, 1 Aug 2026 03:54:09 +1000 Subject: [PATCH 1/2] [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/2] [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; } /**