From 82d0539c4eb2634f66458ac1bdd83c616fe595dd Mon Sep 17 00:00:00 2001 From: kellyyeh <42761586+kellyyeh@users.noreply.github.com> Date: Tue, 19 Oct 2021 16:15:21 -0700 Subject: [PATCH] Fix dhcpmon bugs (#9008) * Exclude incrementing on aggregate device if packet received on mgmt interface * Fix DHCPv6 header offset calculation --- src/dhcpmon/src/dhcp_device.c | 45 ++++++++++++++++++++++++++++------- src/dhcpmon/src/dhcp_device.h | 11 +++++++++ src/dhcpmon/src/dhcp_devman.c | 3 +++ 3 files changed, 51 insertions(+), 8 deletions(-) diff --git a/src/dhcpmon/src/dhcp_device.c b/src/dhcpmon/src/dhcp_device.c index 3078295e49a4..e4fbc7f56909 100644 --- a/src/dhcpmon/src/dhcp_device.c +++ b/src/dhcpmon/src/dhcp_device.c @@ -121,6 +121,8 @@ static struct sock_fprog dhcp_sock_bfp = { */ static dhcp_device_context_t aggregate_dev = {0}; +static dhcp_device_context_t *mgmt_intf = NULL; + /** Monitored DHCPv4 message type */ static dhcpv4_message_type_t v4_monitored_msgs[] = { DHCPv4_MESSAGE_TYPE_DISCOVER, @@ -176,6 +178,11 @@ static void handle_dhcp_option_53(dhcp_device_context_t *context, if ((context->giaddr_ip == giaddr && context->is_uplink && dir == DHCP_TX) || (!context->is_uplink && dir == DHCP_RX && iphdr->ip_dst.s_addr == INADDR_BROADCAST)) { context->counters.v4counters[DHCP_COUNTERS_CURRENT][dir][dhcp_option[2]]++; + // If the packet recieved on the mgmt interface, we don't want to increment the aggregate device + if (context == mgmt_intf) + { + break; + } aggregate_dev.counters.v4counters[DHCP_COUNTERS_CURRENT][dir][dhcp_option[2]]++; } break; @@ -186,6 +193,11 @@ static void handle_dhcp_option_53(dhcp_device_context_t *context, if ((context->giaddr_ip == iphdr->ip_dst.s_addr && context->is_uplink && dir == DHCP_RX) || (!context->is_uplink && dir == DHCP_TX)) { context->counters.v4counters[DHCP_COUNTERS_CURRENT][dir][dhcp_option[2]]++; + // If the packet recieved on the mgmt interface, we don't want to increment the aggregate device + if (context == mgmt_intf) + { + break; + } aggregate_dev.counters.v4counters[DHCP_COUNTERS_CURRENT][dir][dhcp_option[2]]++; } break; @@ -224,6 +236,11 @@ static void handle_dhcpv6_option(dhcp_device_context_t *context, case DHCPv6_MESSAGE_TYPE_RECONFIGURE: case DHCPv6_MESSAGE_TYPE_INFORMATION_REQUEST: context->counters.v6counters[DHCP_COUNTERS_CURRENT][dir][dhcp_option]++; + // If the packet recieved on the mgmt interface, we don't want to increment the aggregate device + if (context == mgmt_intf) + { + break; + } aggregate_dev.counters.v6counters[DHCP_COUNTERS_CURRENT][dir][dhcp_option]++; break; default: @@ -309,7 +326,7 @@ static void read_callback(int fd, short event, void *arg) } } else if (!is_ipv4 && dhcpv6_enabled && (buffer_sz > UDPv6_START_OFFSET + sizeof(struct udphdr) + DHCPv6_TYPE_LENGTH)) { - const u_char* dhcp_option = context->buffer + dhcp_option_offset; + const u_char* dhcp_header = context->buffer + dhcp_option_offset; dhcp_packet_direction_t dir = (ethhdr->ether_shost[0] == context->mac[0] && ethhdr->ether_shost[1] == context->mac[1] && ethhdr->ether_shost[2] == context->mac[2] && @@ -319,23 +336,25 @@ static void read_callback(int fd, short event, void *arg) DHCP_TX : DHCP_RX; int offset = 0; uint16_t option = 0; + uint16_t current_option_len = 0; // Get to inner DHCP header from encapsulated RELAY_FORWARD or RELAY_REPLY header - while (dhcp_option[offset] == DHCPv6_MESSAGE_TYPE_RELAY_FORWARD || dhcp_option[offset] == DHCPv6_MESSAGE_TYPE_RELAY_REPLY) + while (dhcp_header[offset] == DHCPv6_MESSAGE_TYPE_RELAY_FORWARD || dhcp_header[offset] == DHCPv6_MESSAGE_TYPE_RELAY_REPLY) { // Get to DHCPv6_OPTION_RELAY_MSG from all options offset += DHCPv6_RELAY_MSG_OPTIONS_OFFSET; - option = htons(*((uint16_t*)(&(dhcp_option[offset])))); + option = htons(*((uint16_t*)(&(dhcp_header[offset])))); while (option != DHCPv6_OPTION_RELAY_MSG) { - offset += DHCPv6_OPTION_LENGTH; // Add to offset the option length and get the next option ID - offset += htons(*((uint16_t*)(&(dhcp_option[offset])))); - option = htons(*((uint16_t*)(&(dhcp_option[offset])))); + current_option_len = htons(*((uint16_t*)(&(dhcp_header[offset + DHCPv6_OPTION_LENGTH])))); + offset += DHCPv6_OPTION_LENGTH + DHCPv6_OPTION_LEN_LENGTH + current_option_len; + option = htons(*((uint16_t*)(&(dhcp_header[offset])))); } + // Set the offset to DHCP-relay-message data offset += DHCPv6_OPTION_LENGTH + DHCPv6_OPTION_LEN_LENGTH; } - handle_dhcpv6_option(context, dhcp_option[offset], dir); + handle_dhcpv6_option(context, dhcp_header[offset], dir); } else { syslog(LOG_WARNING, "read_callback(%s): read length (%ld) is too small to capture DHCP options", context->intf, buffer_sz); @@ -554,7 +573,7 @@ static dhcp_mon_status_t dhcp_device_check_health(dhcp_mon_check_t check_type, { dhcp_mon_status_t rv = DHCP_MON_STATUS_HEALTHY; - if (dhcp_device_is_dhcp_inactive(aggregate_dev.counters.v4counters, aggregate_dev.counters.v6counters, type)) { + if (dhcp_device_is_dhcp_inactive(v4counters, v6counters, type)) { rv = DHCP_MON_STATUS_INDETERMINATE; } else if (check_type == DHCP_MON_CHECK_POSITIVE) { rv = dhcp_device_check_positive_health(v4counters, v6counters, type); @@ -948,3 +967,13 @@ void dhcp_device_active_types(bool dhcpv4, bool dhcpv6) dhcpv4_enabled = dhcpv4; dhcpv6_enabled = dhcpv6; } + +/** + * @code dhcp_device_init_mgmt_intf(mgmt_intf_context); + * + * @brief assign context address of mgmt interface + */ +void dhcp_device_init_mgmt_intf(dhcp_device_context_t *mgmt_intf_context) +{ + mgmt_intf = mgmt_intf_context; +} diff --git a/src/dhcpmon/src/dhcp_device.h b/src/dhcpmon/src/dhcp_device.h index f8aa874bfcce..433eb0907626 100644 --- a/src/dhcpmon/src/dhcp_device.h +++ b/src/dhcpmon/src/dhcp_device.h @@ -255,4 +255,15 @@ void dhcp_device_print_status(dhcp_device_context_t *context, dhcp_counters_type * @return none */ void dhcp_device_active_types(bool dhcpv4, bool dhcpv6); + +/** + * @code dhcp_device_init_mgmt_intf(mgmt_intf_context); + * + * @brief assign context address of mgmt interface + * + * @param mgmt_intf_context MGMT interface context struct address + * + * @return none + */ +void dhcp_device_init_mgmt_intf(dhcp_device_context_t *mgmt_intf_context); #endif /* DHCP_DEVICE_H_ */ diff --git a/src/dhcpmon/src/dhcp_devman.c b/src/dhcpmon/src/dhcp_devman.c index e7a25322207f..b36d926c1d5b 100644 --- a/src/dhcpmon/src/dhcp_devman.c +++ b/src/dhcpmon/src/dhcp_devman.c @@ -148,6 +148,9 @@ int dhcp_devman_add_intf(const char *name, char intf_type) strncpy(agg_dev->intf + sizeof(AGG_DEV_PREFIX) - 1, name, sizeof(agg_dev->intf) - sizeof(AGG_DEV_PREFIX)); agg_dev->intf[sizeof(agg_dev->intf) - 1] = '\0'; } + else if (rv == 0 && intf_type == 'm') { + dhcp_device_init_mgmt_intf(dev->dev_context); + } LIST_INSERT_HEAD(&intfs, dev, entry); }