Fix dhcpmon bugs (sonic-net#9008)

* Exclude incrementing on aggregate device if packet received on mgmt interface

* Fix DHCPv6 header offset calculation

Author: kellyyeh

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;
+}

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_ */

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);
     }