From 73f7dcb704a12630334dbf45fac961525686628f Mon Sep 17 00:00:00 2001 From: Ram Hegde Date: Thu, 9 Jul 2026 11:31:23 +0000 Subject: [PATCH 1/7] stp : fix: Prevent duplication of stp ipc structures. The STP IPC structures exchanged between stpmgr-daemon and stp-daemon were duplicated. Once defined in sonic-swss/cfgmgr/stpmgr.h and again in sonic-stp/include/stp_ipc.h. The definition in the first file has been removed and stp_ipc.h is included in stpmgr.h Signed-off-by: Ram Hegde --- cfgmgr/stpmgr.h | 186 +----------------------------------------------- 1 file changed, 1 insertion(+), 185 deletions(-) diff --git a/cfgmgr/stpmgr.h b/cfgmgr/stpmgr.h index 7360dc63453..b1685917a1b 100644 --- a/cfgmgr/stpmgr.h +++ b/cfgmgr/stpmgr.h @@ -17,13 +17,7 @@ #include "producerstatetable.h" #include #include - -// We remove PACKED definitions, only keep ALIGNED -#if defined(__GNUC__) -#define ALIGNED(x) __attribute__((aligned(x))) -#else -#define ALIGNED(x) -#endif +#include #define STPMGRD_SOCK_NAME "/var/run/stpmgrd.sock" @@ -48,184 +42,6 @@ #define STPD_SOCK_NAME "/var/run/stpipc.sock" -// Enumerations must match stp_ipc.h -typedef enum L2_PROTO_MODE { - L2_NONE, - L2_PVSTP, - L2_MSTP -}L2_PROTO_MODE; - - -typedef enum LinkType { - AUTO = 0, // Auto - POINT_TO_POINT = 1, // Point-to-point - SHARED = 2 // Shared -} LinkType; - -typedef enum STP_MSG_TYPE { - STP_INVALID_MSG, - STP_INIT_READY, - STP_BRIDGE_CONFIG, - STP_VLAN_CONFIG, - STP_VLAN_PORT_CONFIG, - STP_PORT_CONFIG, - STP_VLAN_MEM_CONFIG, - STP_STPCTL_MSG, - STP_MST_GLOBAL_CONFIG, - STP_MST_INST_CONFIG, - STP_MST_INST_PORT_CONFIG, - STP_MAX_MSG -} STP_MSG_TYPE; - -typedef enum STP_CTL_TYPE { - STP_CTL_HELP, - STP_CTL_DUMP_ALL, - STP_CTL_DUMP_GLOBAL, - STP_CTL_DUMP_VLAN_ALL, - STP_CTL_DUMP_VLAN, - STP_CTL_DUMP_INTF, - STP_CTL_SET_LOG_LVL, - STP_CTL_DUMP_NL_DB, - STP_CTL_DUMP_NL_DB_INTF, - STP_CTL_DUMP_LIBEV_STATS, - STP_CTL_SET_DBG, - STP_CTL_CLEAR_ALL, - STP_CTL_CLEAR_VLAN, - STP_CTL_CLEAR_INTF, - STP_CTL_CLEAR_VLAN_INTF, - STP_CTL_MAX -} STP_CTL_TYPE; - -// Remove PACKED, add ALIGNED(4) -typedef struct STP_IPC_MSG { - int msg_type; - unsigned int msg_len; - L2_PROTO_MODE proto_mode; - char data[0]; -} ALIGNED(4) STP_IPC_MSG; - -#define STP_SET_COMMAND 1 -#define STP_DEL_COMMAND 0 - -// Add padding for alignment if needed (compare to stp_ipc.h) -typedef struct STP_INIT_READY_MSG { - uint8_t opcode; // enable/disable - uint16_t max_stp_instances; - // Example: potential extra padding if alignment warnings arise - // uint8_t padding[1]; -} ALIGNED(4) STP_INIT_READY_MSG; - -// Add padding for alignment if needed -typedef struct STP_BRIDGE_CONFIG_MSG { - uint8_t opcode; // enable/disable - uint8_t stp_mode; - int rootguard_timeout; - uint8_t base_mac_addr[6]; - // Potential padding for alignment: - // uint8_t padding[2]; -} ALIGNED(4) STP_BRIDGE_CONFIG_MSG; - -// Must match the version in stp_ipc.h exactly -typedef struct PORT_ATTR { - char intf_name[IFNAMSIZ]; // 16 bytes typically - int8_t mode; - uint8_t enabled; - // Add padding to align to 4 bytes - uint16_t padding; -} ALIGNED(4) PORT_ATTR; - -// Must match the version in stp_ipc.h exactly -typedef struct STP_VLAN_CONFIG_MSG { - uint8_t opcode; // enable/disable - uint8_t newInstance; - int vlan_id; - int inst_id; - int forward_delay; - int hello_time; - int max_age; - int priority; - int count; - PORT_ATTR port_list[0]; -} ALIGNED(4) STP_VLAN_CONFIG_MSG; - -typedef struct STP_VLAN_PORT_CONFIG_MSG { - uint8_t opcode; // enable/disable - int vlan_id; - char intf_name[IFNAMSIZ]; - int inst_id; - int path_cost; - int priority; -} ALIGNED(4) STP_VLAN_PORT_CONFIG_MSG; - -typedef struct VLAN_ATTR { - int inst_id; - int vlan_id; - int8_t mode; - // Add padding to align to 4 bytes - uint8_t padding[3]; -} ALIGNED(4) VLAN_ATTR; - -typedef struct VLAN_LIST{ - uint16_t vlan_id; -}VLAN_LIST; - -typedef struct STP_PORT_CONFIG_MSG { - uint8_t opcode; // enable/disable - char intf_name[IFNAMSIZ]; - uint8_t enabled; - uint8_t root_guard; - uint8_t loop_guard; - uint8_t bpdu_guard; - uint8_t bpdu_guard_do_disable; - uint8_t portfast; // PVST only - uint8_t uplink_fast; // PVST only - uint8_t edge_port; // MSTP only - LinkType link_type; // MSTP only - int path_cost; - int priority; - int count; - VLAN_ATTR vlan_list[0]; -} STP_PORT_CONFIG_MSG; - -typedef struct STP_VLAN_MEM_CONFIG_MSG { - uint8_t opcode; // enable/disable - int vlan_id; - int inst_id; - char intf_name[IFNAMSIZ]; - uint8_t enabled; - int8_t mode; - // Add 1 byte padding - uint8_t padding; - int path_cost; - int priority; -} ALIGNED(4) STP_VLAN_MEM_CONFIG_MSG; - -typedef struct STP_MST_GLOBAL_CONFIG_MSG { - uint8_t opcode; // enable/disable - uint32_t revision_number; - char name[32]; - uint8_t forward_delay; - uint8_t hello_time; - uint8_t max_age; - uint8_t max_hops; -}__attribute__ ((packed))STP_MST_GLOBAL_CONFIG_MSG; - -typedef struct STP_MST_INST_CONFIG_MSG { - uint8_t opcode; // enable/disable - uint16_t mst_id; // MST instance ID - int priority; // Bridge priority - uint16_t vlan_count; // Number of VLANs in this instance - VLAN_LIST vlan_list[0]; // Flexible array for VLAN IDs -}__attribute__((packed)) STP_MST_INST_CONFIG_MSG; - -typedef struct STP_MST_INST_PORT_CONFIG_MSG { - uint8_t opcode; // enable/disable - char intf_name[IFNAMSIZ]; // Interface name - uint16_t mst_id; // MST instance ID - int path_cost; // Path cost - int priority; // Port priority -} __attribute__((packed)) STP_MST_INST_PORT_CONFIG_MSG; - namespace swss { class StpMgr : public Orch From 4218f96e4aa33902cfa96676fab26f73b604a09b Mon Sep 17 00:00:00 2001 From: Ram Hegde Date: Thu, 9 Jul 2026 11:46:02 +0000 Subject: [PATCH 2/7] stp : fix : Populate "proto_mode" in STP_IPC_MSG instance. In sendMsgStpd(), STP_IPC_MSG message is created and sent to the STP daemon. Here "proto_mode" in the IPC message is not populated. Due to this the message in STP daemon is ignored since the proto_mode check fails. Signed-off-by: Ram Hegde --- cfgmgr/stpmgr.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/cfgmgr/stpmgr.cpp b/cfgmgr/stpmgr.cpp index b27bb5c15af..1d5cabb4959 100644 --- a/cfgmgr/stpmgr.cpp +++ b/cfgmgr/stpmgr.cpp @@ -1227,7 +1227,8 @@ int StpMgr::sendMsgStpd(STP_MSG_TYPE msgType, uint32_t msgLen, void *data) int rc; len = msgLen + (offsetof(struct STP_IPC_MSG, data)); - SWSS_LOG_INFO("tx_msg len %d msglen %d", (int)len, msgLen); + SWSS_LOG_INFO( "tx_msg len %d msglen %d for proto_mode %d", + (int)len, msgLen, l2ProtoEnabled ); tx_msg = (STP_IPC_MSG *)calloc(1, len); if (tx_msg == NULL) @@ -1238,6 +1239,7 @@ int StpMgr::sendMsgStpd(STP_MSG_TYPE msgType, uint32_t msgLen, void *data) tx_msg->msg_type = msgType; tx_msg->msg_len = msgLen; + tx_msg->proto_mode = l2ProtoEnabled; memcpy(tx_msg->data, data, msgLen); bzero(&addr, sizeof(addr)); From b0e906d437720a4e866b5c7c9278e86c273ddda1 Mon Sep 17 00:00:00 2001 From: Ram Hegde Date: Thu, 9 Jul 2026 11:56:26 +0000 Subject: [PATCH 3/7] stp: fix: Fix parameter reinitialization and code duplication * In doVlanMemUpdateTask(), prevented reinitlization of priority and path_cost of STP_VLAN_MEM_CONFIG_MSG message instance * In doTask() prevented duplicate call to doStpPortTask() * In doStpMstInstTask(), moved call to updateVlanInstanceMap() outside the for-loop which fetches the attributes. Signed-off-by: Ram Hegde --- cfgmgr/stpmgr.cpp | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/cfgmgr/stpmgr.cpp b/cfgmgr/stpmgr.cpp index 1d5cabb4959..3127acadf9a 100644 --- a/cfgmgr/stpmgr.cpp +++ b/cfgmgr/stpmgr.cpp @@ -72,8 +72,6 @@ void StpMgr::doTask(Consumer &consumer) doStpMstInstTask(consumer); else if (table == "STP_MST_PORT") doStpMstInstPortTask(consumer); - else if (table == CFG_STP_PORT_TABLE_NAME) - doStpPortTask(consumer); else SWSS_LOG_ERROR("Invalid table %s", table.c_str()); } @@ -689,6 +687,7 @@ void StpMgr::doVlanMemUpdateTask(Consumer &consumer) { STP_VLAN_MEM_CONFIG_MSG msg; memset(&msg, 0, sizeof(STP_VLAN_MEM_CONFIG_MSG)); + msg.priority = -1; KeyOpFieldsValuesTuple t = it->second; @@ -749,8 +748,6 @@ void StpMgr::doVlanMemUpdateTask(Consumer &consumer) msg.vlan_id = vlan_id; msg.inst_id = m_vlanInstMap[vlan_id]; msg.mode = tagging_mode; - msg.priority = -1; - msg.path_cost = 0; strncpy(msg.intf_name, intfName.c_str(), IFNAMSIZ-1); @@ -1068,9 +1065,10 @@ void StpMgr::doStpMstInstTask(Consumer &consumer) vlan_list_str = fvValue(i); vlan_ids = parseVlanList(vlan_list_str); } - updateVlanInstanceMap(instance_id, vlan_ids, true); } + updateVlanInstanceMap(instance_id, vlan_ids, true); + uint32_t vlan_count = static_cast(vlan_ids.size()); len = sizeof(STP_MST_INST_CONFIG_MSG) + static_cast(vlan_count * sizeof(VLAN_LIST)); From eef4646f4e30d43807e60c1e50275338acd7c753 Mon Sep 17 00:00:00 2001 From: Ram Hegde Date: Thu, 9 Jul 2026 12:04:20 +0000 Subject: [PATCH 4/7] stp : fix : Updated stpmgr daemon main loop to return success ( 0 ) on completion instead of failure ( -1 ). Signed-off-by: Ram Hegde --- cfgmgr/stpmgrd.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cfgmgr/stpmgrd.cpp b/cfgmgr/stpmgrd.cpp index 1234632f440..340d27993eb 100644 --- a/cfgmgr/stpmgrd.cpp +++ b/cfgmgr/stpmgrd.cpp @@ -121,5 +121,5 @@ int main(int argc, char **argv) SWSS_LOG_ERROR("Runtime error: %s", e.what()); } - return -1; -} \ No newline at end of file + return 0; +} From cbe969ebcde2ba891e843a2ffa6ddd8ab37d8862 Mon Sep 17 00:00:00 2001 From: Ram Hegde Date: Thu, 9 Jul 2026 12:12:51 +0000 Subject: [PATCH 5/7] stp:fix: Replace ebtables filtering with nft ebtables filtering is not supported in kernel so replace the same with nft. Signed-off-by: Ram Hegde --- cfgmgr/stpmgr.cpp | 57 ++++++++++++++++++++++++++++++++++------------- 1 file changed, 41 insertions(+), 16 deletions(-) diff --git a/cfgmgr/stpmgr.cpp b/cfgmgr/stpmgr.cpp index 3127acadf9a..f3bc3c50216 100644 --- a/cfgmgr/stpmgr.cpp +++ b/cfgmgr/stpmgr.cpp @@ -19,6 +19,42 @@ using namespace std; using namespace swss; +namespace { + +// Dedicated bridge nft table: drop PVST+ PDUs (dst 01:00:0c:cc:cc:cd) on forward. +// Table is removed on restore so add path stays idempotent. +constexpr char pvstNftBridgeTable[] = "sonic_stp_pvst"; + +void preventPvstPduFlooding() +{ + std::string res; + const std::string tableName(pvstNftBridgeTable); + const std::string cmd = + "nft delete table bridge " + tableName + " ; " + "nft add table bridge " + tableName + " && " + "nft 'add chain bridge " + tableName + + " forward { type filter hook forward priority 0; policy accept; }' && " + "nft add rule bridge " + tableName + " forward ether daddr 01:00:0c:cc:cc:cd drop"; + + int ret = swss::exec(cmd, res); + if ( ret != 0 ) { + SWSS_LOG_ERROR( "Failed to install nft rule to prevent PVST PDU flooding - %d", ret ); + } +} + +void restorePvstPduFlooding() +{ + std::string res; + const std::string cmd = + std::string("nft delete table bridge ") + pvstNftBridgeTable; + int ret = swss::exec(cmd, res); + if ( ret != 0 ) { + SWSS_LOG_DEBUG( "Failed to remove nft rule which prevented PVST PDU flooding - %d", ret ); + } +} + +} // namespace + // StpMgr::StpMgr(DBConnector *confDb, DBConnector *applDb, DBConnector *statDb, const vector &tables) : Orch(tables), @@ -44,8 +80,7 @@ StpMgr::StpMgr(DBConnector *confDb, DBConnector *applDb, DBConnector *statDb, // Initialize all VLANs to Invalid instance fill_n(m_vlanInstMap, MAX_VLANS, INVALID_INSTANCE); - int ret = system("ebtables -D FORWARD -d 01:00:0c:cc:cc:cd -j DROP"); - SWSS_LOG_DEBUG("ebtables ret %d", ret); + restorePvstPduFlooding(); } void StpMgr::doTask(Consumer &consumer) @@ -107,14 +142,9 @@ void StpMgr::doStpGlobalTask(Consumer &consumer) { if (l2ProtoEnabled == L2_NONE) { - const std::string cmd = std::string("") + - " ebtables -A FORWARD -d 01:00:0c:cc:cc:cd -j DROP"; - std::string res; - int ret = swss::exec(cmd, res); - if (ret != 0) - SWSS_LOG_ERROR("ebtables add failed for PVST %d", ret); - + preventPvstPduFlooding(); l2ProtoEnabled = L2_PVSTP; + SWSS_LOG_ERROR( "Enabling PVST" ); } msg.stp_mode = L2_PVSTP; } @@ -125,6 +155,7 @@ void StpMgr::doStpGlobalTask(Consumer &consumer) l2ProtoEnabled = L2_MSTP; } msg.stp_mode = L2_MSTP; + SWSS_LOG_DEBUG( "Enabling MSTP" ); // Assign all VLANs to zero instance for MSTP fill_n(m_vlanInstMap, MAX_VLANS, 0); @@ -152,15 +183,9 @@ void StpMgr::doStpGlobalTask(Consumer &consumer) // Initialize all VLANs to Invalid instance fill_n(m_vlanInstMap, MAX_VLANS, INVALID_INSTANCE); - // Remove ebtables rule based on protocol mode if (l2ProtoEnabled == L2_PVSTP) { - const std::string pvst_cmd = - "ebtables -D FORWARD -d 01:00:0c:cc:cc:cd -j DROP"; - std::string res_pvst; - int ret_pvst = swss::exec(pvst_cmd, res_pvst); - if (ret_pvst != 0) - SWSS_LOG_ERROR("ebtables del failed for PVST %d", ret_pvst); + restorePvstPduFlooding(); } l2ProtoEnabled = L2_NONE; } From 4371ebfd8a7a68c84cc239911cecf9e8225af54c Mon Sep 17 00:00:00 2001 From: Ram Hegde Date: Tue, 14 Jul 2026 14:17:06 +0000 Subject: [PATCH 6/7] Handled review comments in * cfgmgr/stpmgr.cpp * cfgmgr/stpmgrd.cpp Which involved using the proper debug level and format specifier Removed the macro STPD_SOCK_NAME from cfgmgr/stpmgr.h as it is already defined in stp_ipc.h Signed-off-by: Ram Hegde --- cfgmgr/stpmgr.cpp | 8 ++++---- cfgmgr/stpmgr.h | 2 -- cfgmgr/stpmgrd.cpp | 2 +- 3 files changed, 5 insertions(+), 7 deletions(-) diff --git a/cfgmgr/stpmgr.cpp b/cfgmgr/stpmgr.cpp index f3bc3c50216..8565579f0c9 100644 --- a/cfgmgr/stpmgr.cpp +++ b/cfgmgr/stpmgr.cpp @@ -49,12 +49,12 @@ void restorePvstPduFlooding() std::string("nft delete table bridge ") + pvstNftBridgeTable; int ret = swss::exec(cmd, res); if ( ret != 0 ) { - SWSS_LOG_DEBUG( "Failed to remove nft rule which prevented PVST PDU flooding - %d", ret ); + SWSS_LOG_ERROR( "Failed to remove nft rule which prevented PVST PDU flooding - %d", ret ); } } } // namespace - // + StpMgr::StpMgr(DBConnector *confDb, DBConnector *applDb, DBConnector *statDb, const vector &tables) : Orch(tables), @@ -144,7 +144,7 @@ void StpMgr::doStpGlobalTask(Consumer &consumer) { preventPvstPduFlooding(); l2ProtoEnabled = L2_PVSTP; - SWSS_LOG_ERROR( "Enabling PVST" ); + SWSS_LOG_DEBUG( "Enabling PVST" ); } msg.stp_mode = L2_PVSTP; } @@ -1250,7 +1250,7 @@ int StpMgr::sendMsgStpd(STP_MSG_TYPE msgType, uint32_t msgLen, void *data) int rc; len = msgLen + (offsetof(struct STP_IPC_MSG, data)); - SWSS_LOG_INFO( "tx_msg len %d msglen %d for proto_mode %d", + SWSS_LOG_INFO( "tx_msg len %d msglen %u for proto_mode %d", (int)len, msgLen, l2ProtoEnabled ); tx_msg = (STP_IPC_MSG *)calloc(1, len); diff --git a/cfgmgr/stpmgr.h b/cfgmgr/stpmgr.h index b1685917a1b..8a8b0f94cd6 100644 --- a/cfgmgr/stpmgr.h +++ b/cfgmgr/stpmgr.h @@ -40,8 +40,6 @@ #define FREE_ALL_INST_ID() l2InstPool.reset() #define IS_INST_ID_AVAILABLE() (l2InstPool.count() < max_stp_instances) -#define STPD_SOCK_NAME "/var/run/stpipc.sock" - namespace swss { class StpMgr : public Orch diff --git a/cfgmgr/stpmgrd.cpp b/cfgmgr/stpmgrd.cpp index 340d27993eb..30e52e472ad 100644 --- a/cfgmgr/stpmgrd.cpp +++ b/cfgmgr/stpmgrd.cpp @@ -121,5 +121,5 @@ int main(int argc, char **argv) SWSS_LOG_ERROR("Runtime error: %s", e.what()); } - return 0; + return -1; } From cf9ef3486526a7ddd1ed057a50edeaef57fe9ee5 Mon Sep 17 00:00:00 2001 From: Ram Hegde Date: Tue, 28 Jul 2026 12:46:41 +0530 Subject: [PATCH 7/7] Revert unnecessary stpmgrd.cpp main() return value change Signed-off-by: Ram Hegde --- cfgmgr/stpmgrd.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cfgmgr/stpmgrd.cpp b/cfgmgr/stpmgrd.cpp index 30e52e472ad..1234632f440 100644 --- a/cfgmgr/stpmgrd.cpp +++ b/cfgmgr/stpmgrd.cpp @@ -122,4 +122,4 @@ int main(int argc, char **argv) } return -1; -} +} \ No newline at end of file