From 537765268cc827497d7a6bdd2dec5c5253679104 Mon Sep 17 00:00:00 2001 From: Lun Yue <17232861+lunyue-ms@users.noreply.github.com> Date: Thu, 30 Jul 2026 02:16:31 +0000 Subject: [PATCH] vpp: fix sub-port datapath for port-channel sub-interfaces Four defects broke L3 sub-interfaces created on a PortChannel (SAI_ROUTER_INTERFACE_TYPE_SUB_PORT over SAI_OBJECT_TYPE_LAG): 1. The connected-route address landed on the bond main interface instead of the sub-interface. vpp_add_del_intf_ip_addr_norif() parsed the bond id with std::stoi(full_if_name.substr(strlen("PortChannel"))). For "PortChannel1.20" std::stoi() stops at the '.' and silently yields 1, so the helper programmed BondEthernet1 instead of BondEthernet1.20. Every sub-port on the same bond then contended for the parent's address space and VPP rejected the later ones as overlapping prefixes. Parse the id from if_name -- the name without the VLAN suffix, which get_intf_vlanid() has already split out -- and append "." when a VLAN id is present. 2. The linux-cp host tap for the sub-port collided with the kernel's own 8021q netdev. vpp_create_router_interface() bound the LCP pair to "PortChannel.", a name the Linux bond/team stack already owns. Use "be." instead, a VLAN netdev on the be bond tap, so linux-cp-punt-xc has an unambiguous target. vpp_remove_router_interface() builds the same name for symmetry. 3. The host tap was left admin-down, silently dropping punted traffic. lcp-auto-subint and lcp-sync are both disabled (vlan-bvi HLD 3.6), so nothing brought the freshly created host netdev up. A down netdev makes the kernel discard the for-us punt, so the sub-port never answered ARP/ND and never terminated traffic. Bring it up explicitly from the SAI path through the new vs_set_dev_admin_up() helper. 4. LAG members were left non-promiscuous after being enslaved. VPP clears a hardware interface's promiscuous flag when the interface is enslaved into a bond, so a member programmed by vpp_create_lag_member() only accepted frames addressed to its own MAC. Traffic forwarded over the port channel carries the common SONiC router MAC rather than the member's, and port-channel sub-interface traffic is VLAN tagged against the bond rather than the member, so neither was accepted. Re-apply interface_set_promiscuous() right after create_bond_member(), and again on the re-attach path in vpp_set_lag_member_egress_disable(), which enslaves the port a second time. Also drop the be -> PortChannel tc mirred redirect from vpp_ensure_lag_lcp(). Control-plane punt for a port channel now goes through the sonic_ext plugin's punt-via-member path (sonic-ext-capture plus aggr-tap-redirect), which delivers the punted copy to the originating member tap. Sub-port IP programming stays entirely on the next_hop=PORT path via vpp_add_del_intf_ip_addr_norif(); the ROUTER_INTERFACE variant remains disabled as upstream had it. With the parsing fix above that single path covers physical and port-channel sub-interfaces alike, and no duplicate or overlapping-prefix rejection is observed. Tested on the vms-kvm-vpp-t1-lag KVM testbed with loganalyzer enabled: sub_port_interfaces/test_sub_port_interfaces.py reports 27 passed, 5 skipped, 0 failed, 0 errors. syslog over the run contains no overlapping-prefix rejection and no interface_ip_address_add_del failure. All 16 members of the 8 port channels report "promiscuous: unicast on" in "show hardware-interfaces"; before this change only the first member of each bond was promiscuous. Signed-off-by: Lun Yue <17232861+lunyue-ms@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6c4e1533-56e4-4d1d-8c98-9b000f07a580 --- vslib/vpp/SwitchVpp.h | 4 +++ vslib/vpp/SwitchVppFdb.cpp | 23 +++++++++++--- vslib/vpp/SwitchVppHostif.cpp | 54 ++++++++++++++++++++++++++++++++ vslib/vpp/SwitchVppRif.cpp | 59 ++++++++++++++++++++++++++++++----- 4 files changed, 128 insertions(+), 12 deletions(-) diff --git a/vslib/vpp/SwitchVpp.h b/vslib/vpp/SwitchVpp.h index 8a3b2ed8e0..0bab2691c8 100644 --- a/vslib/vpp/SwitchVpp.h +++ b/vslib/vpp/SwitchVpp.h @@ -323,6 +323,10 @@ namespace saivs _In_ const char *dev, _In_ const sai_mac_t& mac); + static int vs_set_dev_admin_up( + _In_ const char *dev, + _In_ bool up); + static int promisc( _In_ const char *dev); diff --git a/vslib/vpp/SwitchVppFdb.cpp b/vslib/vpp/SwitchVppFdb.cpp index e1000ec58a..8fa0ee3e99 100644 --- a/vslib/vpp/SwitchVppFdb.cpp +++ b/vslib/vpp/SwitchVppFdb.cpp @@ -1364,6 +1364,13 @@ sai_status_t SwitchVpp::vpp_create_lag_member( return SAI_STATUS_FAILURE; } + // Enslaving a port into the bond clears its promiscuous flag in VPP, so + // re-apply it here. Traffic forwarded over the PortChannel arrives with the + // common SONiC router MAC (different from the member's hardware MAC), and + // PortChannel sub-interface traffic is VLAN tagged against the bond rather + // than the member, neither of which the member accepts unless promiscuous. + interface_set_promiscuous(hwifname, true); + CHECK_STATUS(vpp_ensure_lag_lcp(lag_oid)); return SAI_STATUS_SUCCESS; @@ -1396,14 +1403,17 @@ sai_status_t SwitchVpp::vpp_ensure_lag_lcp( configure_lcp_interface(hw_ifname, tap.c_str(), true); - std::string portchannel = std::string("PortChannel") + std::to_string(bond_id); - std::string be = std::string("be") + std::to_string(bond_id); - CHECK_STATUS(add_tc_filter_redirect(be, portchannel)); - + /* + * Control-plane punt for the port channel is handled by the sonic_ext + * plugin's punt-via-member path (sonic-ext-capture + aggr-tap-redirect), + * which steers the punted copy to the originating member tap. The legacy + * be -> PortChannel tc mirred redirect is no longer needed and is + * intentionally not installed (SONiC PR #2440 §5.3). + */ bond_info.lcp_created = true; m_lag_bond_map[lag_oid] = bond_info; - SWSS_LOG_NOTICE("Created LCP and tc redirect for LAG %s", sai_serialize_object_id(lag_oid).c_str()); + SWSS_LOG_NOTICE("Created LCP for LAG %s", sai_serialize_object_id(lag_oid).c_str()); return SAI_STATUS_SUCCESS; } @@ -1596,6 +1606,9 @@ sai_status_t SwitchVpp::vpp_set_lag_member_egress_disable( } else { + // Re-attaching enslaves the port again, which clears its promiscuous + // flag in VPP, so re-apply it just like vpp_create_lag_member() does. + interface_set_promiscuous(hwif_name, true); m_egress_disabled_lag_member_ports.erase(port_oid); } diff --git a/vslib/vpp/SwitchVppHostif.cpp b/vslib/vpp/SwitchVppHostif.cpp index 5f490e58ed..4c73e6f83f 100644 --- a/vslib/vpp/SwitchVppHostif.cpp +++ b/vslib/vpp/SwitchVppHostif.cpp @@ -98,6 +98,60 @@ int SwitchVpp::vs_set_dev_mac_address( return err; } +int SwitchVpp::vs_set_dev_admin_up( + _In_ const char *dev, + _In_ bool up) +{ + SWSS_LOG_ENTER(); + + int s = socket(AF_INET, SOCK_DGRAM, 0); + + if (s < 0) + { + SWSS_LOG_ERROR("failed to create socket, errno: %d", errno); + + return -1; + } + + struct ifreq ifr; + + memset(&ifr, 0, sizeof(ifr)); + + strncpy(ifr.ifr_name, dev, MAX_INTERFACE_NAME_LEN); + + int err = ioctl(s, SIOCGIFFLAGS, &ifr); + + if (err < 0) + { + SWSS_LOG_ERROR("ioctl SIOCGIFFLAGS on %s failed, err %d", dev, err); + + close(s); + + return err; + } + + if (up) + { + ifr.ifr_flags |= IFF_UP; + } + else + { + ifr.ifr_flags &= ~IFF_UP; + } + + err = ioctl(s, SIOCSIFFLAGS, &ifr); + + if (err < 0) + { + SWSS_LOG_ERROR("ioctl SIOCSIFFLAGS %s on %s failed, err %d", + (up ? "UP" : "DOWN"), dev, err); + } + + close(s); + + return err; +} + int SwitchVpp::promisc( _In_ const char *dev) { diff --git a/vslib/vpp/SwitchVppRif.cpp b/vslib/vpp/SwitchVppRif.cpp index 6283370878..ccf856b404 100644 --- a/vslib/vpp/SwitchVppRif.cpp +++ b/vslib/vpp/SwitchVppRif.cpp @@ -1166,8 +1166,19 @@ sai_status_t SwitchVpp::vpp_add_del_intf_ip_addr_norif ( snprintf(hw_bviifname, sizeof(hw_bviifname), "%s%d","bvi",vlan_id); hw_ifname = hw_bviifname; } else if (full_if_name.compare(0, strlen(PORTCHANNEL_PREFIX), PORTCHANNEL_PREFIX) == 0) { - uint32_t bond_id = std::stoi(full_if_name.substr(strlen(PORTCHANNEL_PREFIX))); - snprintf(hw_bondifname, sizeof(hw_bondifname), "%s%d", BONDETHERNET_PREFIX, bond_id); + /* full_if_name is PortChannel[.]; get_intf_vlanid() above + * already split it into if_name / vlan_id. Parse the id from the + * name *without* the VLAN suffix: std::stoi("1.20") stops at the + * '.' and silently yields 1, which used to drop the sub-interface + * and program the address onto the bond main interface. */ + uint32_t bond_id = std::stoi(if_name.substr(strlen(PORTCHANNEL_PREFIX))); + if (vlan_id) { + snprintf(hw_bondifname, sizeof(hw_bondifname), "%s%u.%u", + BONDETHERNET_PREFIX, bond_id, vlan_id); + } else { + snprintf(hw_bondifname, sizeof(hw_bondifname), "%s%u", + BONDETHERNET_PREFIX, bond_id); + } hw_ifname = hw_bondifname; } else { hwifname = tap_to_hwif_name(if_name.c_str()); @@ -1748,11 +1759,22 @@ sai_status_t SwitchVpp::vpp_create_router_interface( const char *parent_hwif; char hw_subif_parent[32]; + char lcp_host_subif[64]; if (ot == SAI_OBJECT_TYPE_LAG) { snprintf(hw_subif_parent, sizeof(hw_subif_parent), "%s%u", BONDETHERNET_PREFIX, bond_info.id); parent_hwif = hw_subif_parent; + /* + * For a port-channel sub-port the LCP host tap must be be. + * (a VLAN netdev on the be bond tap), NOT PortChannel.: + * that name collides with the kernel 8021q netdev owned by the Linux + * bond/team stack. linux-cp-punt-xc lands on be. and the + * sonic_ext aggr-tap-redirect steers the punted copy to the + * originating member tap (SONiC PR #2440 §5.3). + */ + snprintf(lcp_host_subif, sizeof(lcp_host_subif), "be%u.%u", bond_info.id, vlan_id); } else { parent_hwif = tap_to_hwif_name(dev); + snprintf(lcp_host_subif, sizeof(lcp_host_subif), "%s", host_subifname); } create_sub_interface(parent_hwif, vlan_id, vlan_id); @@ -1760,13 +1782,28 @@ sai_status_t SwitchVpp::vpp_create_router_interface( * lcp-auto-subint is disabled in VPP startup config (vlan-bvi HLD §3.6), * so the VPP sub-interface does NOT get an automatic linux-cp pair. * Explicitly create the LCP pair binding . (VPP side) - * to the kernel sub-vlan netdev (.). Without this the - * sub-interface will not show up in `vppctl show lcp` and host punt - * will not work for the SUB_PORT RIF. + * to its host tap (Ethernet. for a port, be. for a + * port-channel). Without this the sub-interface will not show up in + * `vppctl show lcp` and host punt will not work for the SUB_PORT RIF. */ char vpp_subif_name[64]; snprintf(vpp_subif_name, sizeof(vpp_subif_name), "%s.%u", parent_hwif, vlan_id); - configure_lcp_interface(vpp_subif_name, host_subifname, true); + configure_lcp_interface(vpp_subif_name, lcp_host_subif, true); + + /* + * lcp-auto-subint is disabled and linux-cp lcp-sync is off, so nothing + * brings the freshly created kernel sub-interface host netdev UP: it is + * created admin-down and stays down. A down host netdev drops the + * for-us punt (the kernel never processes it / generates no reply), so + * SUB_PORT datapath silently breaks. Explicitly bring the host tap UP + * here (SAI-driven, consistent with the no-lcp-sync design). + */ + if (vs_set_dev_admin_up(lcp_host_subif, true) < 0) + { + SWSS_LOG_ERROR("Failed to bring host sub-interface %s admin up; " + "for-us traffic punted to this SUB_PORT RIF will be dropped", + lcp_host_subif); + } /* Get new list of physical interfaces from VS */ refresh_interfaces_list(); @@ -2102,7 +2139,15 @@ sai_status_t SwitchVpp::vpp_remove_router_interface(sai_object_id_t rif_id) char vpp_subif_name[64]; char host_subifname[64]; snprintf(vpp_subif_name, sizeof(vpp_subif_name), "%s.%u", parent_hwif, vlan_id); - snprintf(host_subifname, sizeof(host_subifname), "%s.%u", dev, vlan_id); + if (ot == SAI_OBJECT_TYPE_LAG) { + /* Symmetric with create (SONiC PR #2440 §5.3): the host tap is + * be.. The host name is ignored by the LCP plugin on + * delete (the pair is keyed by the VPP sub-if), but keep it + * symmetric for log clarity. */ + snprintf(host_subifname, sizeof(host_subifname), "be%u.%u", bond_info.id, vlan_id); + } else { + snprintf(host_subifname, sizeof(host_subifname), "%s.%u", dev, vlan_id); + } configure_lcp_interface(vpp_subif_name, host_subifname, false); delete_sub_interface(parent_hwif, vlan_id);