From 988bddacf773e0a7408082aec17fa2b0c4a27bb8 Mon Sep 17 00:00:00 2001 From: d00f Date: Fri, 14 Aug 2026 15:30:09 +0200 Subject: [PATCH] stp: compare the whole Bridge Identifier, not the priority byte and the MAC A Bridge Identifier is two priority octets followed by the MAC, compared as one unsigned number. The test here read the first priority octet and then went straight to the MAC, so the system ID extension in between was never looked at and two bridges differing only in it were ranked by MAC instead. The field is stored, sent and printed, just not compared. Ordinary single instance RSTP leaves the extension zero on both sides, which is why this has not shown up. Where it is not zero the ranking is simply wrong: same priority octet, extension 0x0a against 0x00, and the worse bridge wins if its MAC happens to be lower. cmpMAC becomes cmpBytes with a length, since the identifier is eight contiguous bytes in both the packet overlay and root_bridge, and the loop was already doing the right thing for six of them. sdcc lays the struct out with no padding, checked, so the eight byte compare is the standard's rule written directly. --- rtl837x_stp.c | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/rtl837x_stp.c b/rtl837x_stp.c index 7a9c894..f8ee6b9 100644 --- a/rtl837x_stp.c +++ b/rtl837x_stp.c @@ -208,9 +208,12 @@ static void stp_record_designated(uint8_t port) __reentrant } -signed char cmpMAC(__xdata uint8_t *m1, __xdata uint8_t *m2) __reentrant +/* Lexicographic compare of n bytes. A MAC is 6 of them; a Bridge Identifier + * is 8, the two priority octets ahead of the MAC, compared as one unsigned + * number per 802.1D. */ +signed char cmpBytes(__xdata uint8_t *m1, __xdata uint8_t *m2, uint8_t n) __reentrant { - for (uint8_t i = 0; i < 6; i++) { + for (uint8_t i = 0; i < n; i++) { if (m1[i] == m2[i]) continue; if (m1[i] < m2[i]) @@ -499,7 +502,7 @@ void stp_in(void) __banked * still receiving. Pull the cable and the re-arming stops, so the loser * comes back on its own after a forward delay - and the link * supervision above gets there first anyway. */ - if (cmpMAC(STP_I->bridge.mac, uip_ethaddr.addr) == 0) { + if (cmpBytes(STP_I->bridge.mac, uip_ethaddr.addr, 6) == 0) { /* Equal means the frame came back on the port it left: a loop * further out, behind an unmanaged switch. There is no pair to * pick from, so that port holds itself down - and since it can @@ -527,9 +530,11 @@ void stp_in(void) __banked stp_record_designated(port); - /* Better root than the one we know? */ - if (STP_I->root.prio < root_bridge.prio - || ((STP_I->root.prio == root_bridge.prio) && cmpMAC(STP_I->root.mac, root_bridge.mac) < 0)) { + /* Better root than the one we know? The identifier is priority, system + * ID extension and MAC in that order: comparing the priority byte and + * then jumping to the MAC skipped the twelve bits in between, so two + * bridges differing only in the extension were ranked by MAC. */ + if (cmpBytes((__xdata uint8_t *)&STP_I->root, (__xdata uint8_t *)&root_bridge, 8) < 0) { /* Root guard: this port must never become our path to the root. */ if (stp_pflags[port] & STP_PF_ROOTGUARD) { print_string("STP: root guard blocking port ");