From: Ido Schimmel <[email protected]> [ Upstream commit bcdfd9fb109e0c9d76c345b2346b6b75ed1f476d ]
When roaming, the FDB destination port can change without holding the bridge's hash lock. Therefore, add missing READ_ONCE() annotations in both RCU readers and readers that hold the lock. In the latter case, the annotation is not needed in places where the FDB entry was already validated to be a local entry since such entries cannot roam. Acked-by: Nikolay Aleksandrov <[email protected]> Signed-off-by: Ido Schimmel <[email protected]> Link: https://patch.msgid.link/[email protected] Signed-off-by: Jakub Kicinski <[email protected]> Signed-off-by: Sasha Levin <[email protected]> --- LLM Generated explanations, may be completely bogus: # Stable Backport Analysis: `bridge: Add missing READ_ONCE() annotations around FDB destination port` **Local tree:** `v6.18.44` (Makefile: 6.18.44) --- ## PHASE 1: COMMIT MESSAGE FORENSICS **Step 1.1 — Subject line** Record: `[bridge]` `[add]` missing `READ_ONCE()` annotations around FDB destination port during concurrent roaming updates. **Step 1.2 — Tags** Record: - `Acked-by: Nikolay Aleksandrov <[email protected]>` — bridge maintainer ack - `Signed-off-by: Ido Schimmel <[email protected]>` — bridge maintainer/author - `Signed-off-by: Jakub Kicinski <[email protected]>` — networking tree maintainer - `Link: https://patch.msgid.link/[email protected]` - No `Fixes:`, `Reported-by:`, `Cc: [email protected]`, `Tested- by:`, or syzbot tags **Step 1.3 — Body analysis** Record: - **Bug:** `fdb->dst` can change during MAC roaming without holding `br->hash_lock`. - **Symptom:** Readers can observe a changing destination port; without `READ_ONCE()`, loads are not paired with existing `WRITE_ONCE()` writers and may be inconsistent across a read/use sequence. - **Root cause:** `br_fdb_update()` updates `fdb->dst` locklessly on the fast path (`WRITE_ONCE(fdb->dst, source)` at line 1030 in `br_fdb.c`), while several readers still used plain `f->dst` / `dst->dst` loads. - **Version info:** None in the message. **Step 1.4 — Hidden bug fix?** Record: **Yes.** Although labeled as annotation work, this is a real concurrency correctness fix in the bridge forwarding and FDB management paths, completing an established `READ_ONCE`/`WRITE_ONCE` pattern for `fdb->dst`. --- ## PHASE 2: DIFF ANALYSIS **Step 2.1 — Inventory** Record: - `net/bridge/br_device.c`: 1 line changed (`br_dev_xmit`) - `net/bridge/br_input.c`: 1 line changed (`br_handle_frame_finish`) - `net/bridge/br_fdb.c`: 4 lines changed across 3 functions - **Total:** ~6 functional lines, 3 files, surgical scope - **Functions modified:** `br_dev_xmit`, `br_handle_frame_finish`, `br_fdb_changeaddr`, `br_fdb_delete_by_port`, `br_fdb_clear_offload` **Step 2.2 — Code flow changes** Record: | Location | Before | After | |---|---|---| | `br_dev_xmit` | `br_forward(dst->dst, ...)` after RCU FDB lookup | `br_forward(READ_ONCE(dst->dst), ...)` — single stable snapshot of roaming port | | `br_handle_frame_finish` | same pattern on receive/forward path | same fix | | `br_fdb_changeaddr` | `f->dst == p` under `hash_lock` | `READ_ONCE(f->dst) == p` | | `br_fdb_delete_by_port` | `f->dst != p` under `hash_lock` | `READ_ONCE(f->dst) != p` | | `br_fdb_clear_offload` | `f->dst == p` under `hash_lock` | `READ_ONCE(f->dst) == p` | **Step 2.3 — Bug mechanism** Record: **Race condition / data-race correctness fix.** Category (b) synchronization. `br_fdb_update()` changes `fdb->dst` without `hash_lock`: ```1026:1031:net/bridge/br_fdb.c /* fastpath: update of existing entry */ if (unlikely(source != READ_ONCE(fdb->dst) && !test_bit(BR_FDB_STICKY, &fdb->flags))) { br_switchdev_fdb_notify(br, fdb, RTM_DELNEIGH); WRITE_ONCE(fdb->dst, source); ``` Readers on hot forwarding paths and FDB iterators could observe a changing `dst` pointer. `br_forward()` handles `NULL` (`if (unlikely(!to))`), but a stale non-NULL port causes mis-forwarding during roam; FDB iterators can miss or mishandle entries during concurrent updates. **Step 2.4 — Fix quality** Record: **High quality, minimal, obviously correct.** Matches the existing subsystem convention from `3e19ae7c6fd62` and follow-up `5424e678f9b30`. Regression risk is very low — only adds documented single-load snapshots. --- ## PHASE 3: GIT HISTORY INVESTIGATION **Step 3.1 — Blame** Record: - `br_device.c:110` and `br_input.c:226`: original code from 2016 (Nikolay Aleksandrov), predating `READ_ONCE` annotations - `br_fdb.c:473`: from 2019, also predating full annotation coverage - Buggy plain loads have been present since before `3e19ae7c6fd62` (2021) **Step 3.2 — Fixes: tag** Record: Not applicable — no `Fixes:` tag. **Step 3.3 — Related file history** Record: - `3e19ae7c6fd62` — introduced `READ_ONCE`/`WRITE_ONCE` for `fdb->dst` broadly (in tree) - `5424e678f9b30` — “use a stable FDB dst snapshot in RCU readers”; fixed `br_fdb_fillbuf`, `fdb_delete_local` writers, etc.; `Cc: [email protected]` (in tree) - `17071fb5cb9c2` — annotated `fdb->{updated,used}` races (in tree) - This commit fills remaining gaps after those fixes - **Standalone:** yes, no series dependency **Step 3.4 — Author context** Record: Ido Schimmel is an active bridge maintainer (`Reviewed-by` on related stable-bound fix `5424e678`). Nikolay Aleksandrov acked. **Step 3.5 — Prerequisites** Record: - Requires `WRITE_ONCE(fdb->dst, ...)` writers — present since `3e19ae7c6fd62` - Requires roaming fast path in `br_fdb_update()` — present - No additional commits required; patch dry-run applies cleanly --- ## PHASE 4: MAILING LIST AND EXTERNAL RESEARCH **Step 4.1 — Original discussion** Record: **UNVERIFIED** — `b4 dig -c <commit>` could not be run (commit not present locally); lore.kernel.org and patch.msgid.link blocked by bot protection (Anubis). **Step 4.2 — Reviewers** Record: **UNVERIFIED** via `b4 dig -w`. Commit message shows maintainer ack from Nikolay Aleksandrov and merge by Jakub Kicinski. **Step 4.3 — Bug report** Record: Not applicable — no `Reported-by:` or syzbot link. **Step 4.4 — Related patches** Record: Part of ongoing `fdb->dst` concurrency hardening; directly complements in-tree `5424e678f9b30`. **Step 4.5 — Stable list history** Record: **UNVERIFIED** — lore stable search inaccessible. --- ## PHASE 5: CODE SEMANTIC ANALYSIS **Step 5.1 — Key functions** Record: `br_dev_xmit`, `br_handle_frame_finish`, `br_fdb_changeaddr`, `br_fdb_delete_by_port`, `br_fdb_clear_offload` **Step 5.2 — Callers / reachability** Record: - `br_dev_xmit` — bridge device transmit hot path (every locally originated unicast frame) - `br_handle_frame_finish` — bridge receive/forward hot path (every forwarded unicast frame) - `br_fdb_delete_by_port` — port removal/teardown - `br_fdb_changeaddr` — MAC address change on port - `br_fdb_clear_offload` — switchdev offload cleanup All are reachable in normal bridge operation; forwarding paths are among the hottest networking code paths. **Step 5.3 — Callees** Record: `br_forward()` dereferences port and forwards skb; `br_fdb_find_rcu()` provides RCU-protected FDB entry; concurrent writer is `br_fdb_update()`. **Step 5.4 — User triggerability** Record: **Yes.** Any bridge with learned MACs that roam between ports triggers `br_fdb_update()` lockless `fdb->dst` changes while packets are being forwarded. **Step 5.5 — Similar patterns** Record: Most other `fdb->dst` readers in this tree already use `READ_ONCE()` — e.g. `br_fdb_fillbuf`, `br_fdb_test_addr`, `br_switchdev_fdb_notify`, `br_arp_nd_proxy.c`. The patched sites are the remaining outliers. --- ## PHASE 6: CROSS-REFERENCE AGAINST LOCAL TREE (v6.18.44) **Step 6.1 — Buggy code present?** Record: **Yes.** Verified missing annotations at: - `br_device.c:110`: `br_forward(dst->dst, ...)` - `br_input.c:226`: `br_forward(dst->dst, ...)` - `br_fdb.c:473`, `881`, `1663`: plain `f->dst` comparisons Roaming writer path is present (`br_fdb.c:1030`). **Step 6.2 — Backport difficulty** Record: **Clean apply** — `patch --dry-run` succeeded with no conflicts. **Step 6.3 — Related fixes already present?** Record: Partial fix `5424e678f9b30` is already in this tree; this commit is the remaining coverage, not a duplicate. --- ## PHASE 7: SUBSYSTEM CONTEXT **Step 7.1 — Subsystem / criticality** Record: `net/bridge` — **IMPORTANT** (widely deployed in servers, VMs, containers, embedded networking). **Step 7.2 — Activity** Record: Actively maintained; recent stable-relevant bridge fixes in this tree (UAF, sleep-in-atomic, FDB snapshot). --- ## PHASE 8: IMPACT AND RISK **Step 8.1 — Who is affected** Record: All systems using Linux bridge forwarding with dynamic FDB learning and MAC roaming. **Step 8.2 — Trigger conditions** Record: Host moves between bridge ports; concurrent forwarding while `br_fdb_update()` roams `fdb->dst`. Common in Wi-Fi/Ethernet mobility, VM migration, and active L2 networks. **Step 8.3 — Failure mode / severity** Record: - **Forwarding paths:** packet delivered to wrong port (connectivity bug / potential traffic leakage) — **MEDIUM-HIGH** - **FDB management paths:** missed or incorrect entry handling during concurrent roam — **MEDIUM** - **Kernel crash:** unlikely on these specific hunks (`br_forward()` handles `NULL`); sibling fix `5424e678` addressed a confirmed NULL- deref in sysfs path - **KCSAN/data-race:** definite without fix — **MEDIUM** for CI/sanitizer builds **Step 8.4 — Risk/benefit** Record: - **Benefit:** HIGH for bridge users with roaming; completes an already- stable-nominated fix family - **Risk:** VERY LOW — 6-line annotation-only change matching established pattern - **Ratio:** strongly favors backport --- ## PHASE 9: FINAL SYNTHESIS **Step 9.1 — Evidence summary** **FOR:** - Real concurrency bug in hot forwarding path - Small, surgical, maintainer-acked - Prerequisites and related stable fix already in v6.18.44 - Applies cleanly - Follows established subsystem convention since 2021 - Complements already-backported `5424e678f9b30` **AGAINST:** - No syzbot/crash report for these exact sites - Primary user impact is mis-forwarding rather than panic - Mailing list/stable discussion not verified **UNRESOLVED:** - Full lore review thread content - Whether reviewers explicitly nominated for stable **Step 9.2 — Stable rules checklist** | Criterion | Result | |---|---| | 1. Obviously correct and tested? | **PASS** — pattern proven in-tree; maintainer ack | | 2. Fixes a real user-affecting bug? | **PASS** — roaming race in forwarding/FDB paths | | 3. Important issue? | **PASS** — forwarding correctness on common bridge deployments | | 4. Small and contained? | **PASS** — 6 lines, 3 files | | 5. No new features/APIs? | **PASS** — annotation only | | 6. Can apply to local tree? | **PASS** — clean dry-run apply; code and writers present | **Step 9.3 — Exception category** Record: Not a device-ID/quirk/DT/build/doc exception; qualifies as a standard concurrency correctness bug fix. **Step 9.4 — Problem statement for stable users** When a MAC address roams between bridge ports, `fdb->dst` is updated locklessly. The remaining unannotated readers in transmit/receive forwarding and FDB cleanup paths can observe a changing destination port, causing frames to be forwarded to the wrong port or FDB maintenance to behave incorrectly. This patch completes the `READ_ONCE`/`WRITE_ONCE` pairing already used throughout the rest of the bridge FDB code and in the recently backported `5424e678` fix. --- ## Verification - [Phase 1] Parsed commit message and tags from user-provided diff - [Phase 2] Diff analysis: 6 lines across `br_device.c`, `br_input.c`, `br_fdb.c` - [Phase 3] `git describe HEAD` → `v6.18.44` - [Phase 3] `git blame` on lines 110/226/473 → pre-2021 code missing annotations - [Phase 3] `git show 3e19ae7c6fd62` → original `READ_ONCE`/`WRITE_ONCE` introduction - [Phase 3] `git show 5424e678f9b30` → related stable-nominated FDB dst snapshot fix, already in tree - [Phase 3] `git merge-base --is-ancestor` → both `3e19ae7` and `5424e678` are ancestors of HEAD - [Phase 4] **UNVERIFIED:** lore.kernel.org and patch.msgid.link blocked; `b4 dig` not usable without local commit hash - [Phase 5] `grep READ_ONCE.*dst` → most readers annotated; patched sites are outliers - [Phase 5] Read `br_fdb_update()` → confirmed lockless `WRITE_ONCE(fdb->dst, source)` roaming path - [Phase 5] Read `br_forward()` → confirmed `NULL` port handled safely - [Phase 6] Read current sources → buggy plain loads confirmed present - [Phase 6] `patch --dry-run` → applies cleanly - [Phase 8] Assessed failure mode as mis-forwarding during roam, not typical kernel oops **YES** net/bridge/br_device.c | 2 +- net/bridge/br_fdb.c | 7 ++++--- net/bridge/br_input.c | 2 +- 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c index 525d4eccd194a..966fac7017225 100644 --- a/net/bridge/br_device.c +++ b/net/bridge/br_device.c @@ -107,7 +107,7 @@ netdev_tx_t br_dev_xmit(struct sk_buff *skb, struct net_device *dev) else br_flood(br, skb, BR_PKT_MULTICAST, false, true, vid); } else if ((dst = br_fdb_find_rcu(br, dest, vid)) != NULL) { - br_forward(dst->dst, skb, false, true); + br_forward(READ_ONCE(dst->dst), skb, false, true); } else { br_flood(br, skb, BR_PKT_UNICAST, false, true, vid); } diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c index 6eb3ab69a5140..fe85c8f197e67 100644 --- a/net/bridge/br_fdb.c +++ b/net/bridge/br_fdb.c @@ -470,7 +470,8 @@ void br_fdb_changeaddr(struct net_bridge_port *p, const unsigned char *newaddr) spin_lock_bh(&br->hash_lock); vg = nbp_vlan_group(p); hlist_for_each_entry(f, &br->fdb_list, fdb_node) { - if (f->dst == p && test_bit(BR_FDB_LOCAL, &f->flags) && + if (READ_ONCE(f->dst) == p && + test_bit(BR_FDB_LOCAL, &f->flags) && !test_bit(BR_FDB_ADDED_BY_USER, &f->flags)) { /* delete old one */ fdb_delete_local(br, p, f); @@ -878,7 +879,7 @@ void br_fdb_delete_by_port(struct net_bridge *br, spin_lock_bh(&br->hash_lock); hlist_for_each_entry_safe(f, tmp, &br->fdb_list, fdb_node) { - if (f->dst != p) + if (READ_ONCE(f->dst) != p) continue; if (!do_all) @@ -1660,7 +1661,7 @@ void br_fdb_clear_offload(const struct net_device *dev, u16 vid) spin_lock_bh(&p->br->hash_lock); hlist_for_each_entry(f, &p->br->fdb_list, fdb_node) { - if (f->dst == p && f->key.vlan_id == vid) + if (READ_ONCE(f->dst) == p && f->key.vlan_id == vid) clear_bit(BR_FDB_OFFLOADED, &f->flags); } spin_unlock_bh(&p->br->hash_lock); diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c index 2cbae0f9ae1f0..470615675bdc0 100644 --- a/net/bridge/br_input.c +++ b/net/bridge/br_input.c @@ -223,7 +223,7 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb if (now != READ_ONCE(dst->used)) WRITE_ONCE(dst->used, now); - br_forward(dst->dst, skb, local_rcv, false); + br_forward(READ_ONCE(dst->dst), skb, local_rcv, false); } else { if (!mcast_hit) br_flood(br, skb, pkt_type, local_rcv, false, vid); -- 2.53.0
