[EVPN-MH] Add kernel patches for EVPN VXLAN Multihoming support#540
[EVPN-MH] Add kernel patches for EVPN VXLAN Multihoming support#540bdfriedman wants to merge 4 commits intosonic-net:masterfrom
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@bdfriedman , are these PR upstreamed to linux kernel already? or in the process of upstream? |
banidoru
left a comment
banidoru
left a comment
There was a problem hiding this comment.
- Bug in
br.c(patch 2):br_switchdev_eventpassesfdb_info->locked(bool) as theprotocolparameter andRTPROT_UNSPECasext_flags— argument order is wrong after the signature change - Bug in
br_fdb_add(patch 2):protocolis initialized toRTPROT_UNSPECbut never parsed fromtb[NDA_PROTOCOL]— the add path silently ignores the user-supplied protocol, unlike the delete path which parses it correctly - Patch 1 (NDA_FLAGS_EXT) and Patch 3 (NTF_EXT_VALIDATED) look structurally sound
- Patches are not upstream-accepted — these appear to be custom SONiC patches carrying significant kernel neighbor/FDB subsystem changes
| } | ||
|
|
||
| - err = __br_fdb_delete(br, p, addr, vid); | ||
| + err = __br_fdb_delete(br, p, addr, vid, protocol); |
There was a problem hiding this comment.
Bug: Wrong argument order. After adding protocol to br_fdb_external_learn_add's signature (as the 5th positional param after vid), this call site passes fdb_info->locked as protocol and appends RTPROT_UNSPEC at the end as ext_flags.
Current: (br, p, addr, vid, fdb_info->locked, false, 0, RTPROT_UNSPEC)
Expected: (br, p, addr, vid, RTPROT_UNSPEC, fdb_info->locked, false, 0)
When fdb_info->locked is true, the entry will get protocol=1 instead of RTPROT_UNSPEC, and locked will silently be forced to false.
There was a problem hiding this comment.
Still open. In the new patch 0002, br_switchdev_event calls br_fdb_external_learn_add(br, p, fdb_info->addr, fdb_info->vid, fdb_info->locked, false, 0, RTPROT_UNSPEC) but the updated signature is (br, p, addr, vid, protocol, locked, swdev_notify, ext_flags). So fdb_info->locked (bool) is still being passed as protocol (u8).
There was a problem hiding this comment.
Still unaddressed in dd2f4ef. br_switchdev_event passes fdb_info->locked as protocol and RTPROT_UNSPEC as ext_flags. Should be: br_fdb_external_learn_add(br, p, fdb_info->addr, fdb_info->vid, RTPROT_UNSPEC, fdb_info->locked, false, 0).
There was a problem hiding this comment.
Still unaddressed in iteration 2. This bug remains in the current diff.
There was a problem hiding this comment.
Fixed in 41a91c8. br_switchdev_event now correctly passes RTPROT_UNSPEC as protocol before fdb_info->locked. ✅
There was a problem hiding this comment.
Fixed in 0ae2cb5. br_switchdev_event now passes RTPROT_UNSPEC as protocol before fdb_info->locked. ✅
There was a problem hiding this comment.
Verified fixed in 0ae2cb5. The argument order is now correct.
| - bool swdev_notify, u32 ext_flags) | ||
| + const unsigned char *addr, u16 vid, u8 protocol, | ||
| + bool locked, bool swdev_notify, u32 ext_flags) | ||
| { |
There was a problem hiding this comment.
Bug: protocol is never parsed from netlink in the add path. The variable is initialized to RTPROT_UNSPEC but there's no if (tb[NDA_PROTOCOL]) protocol = nla_get_u8(tb[NDA_PROTOCOL]); anywhere in br_fdb_add. Compare with br_fdb_delete which correctly parses it. This means bridge fdb add ... proto hw will silently ignore the protocol — it will always store RTPROT_UNSPEC.
There was a problem hiding this comment.
Still open. br_fdb_add initializes u8 protocol = RTPROT_UNSPEC but never parses tb[NDA_PROTOCOL]. The parsing only exists in br_fdb_delete and vxlan_fdb_parse, not in the bridge add path.
There was a problem hiding this comment.
Still unaddressed. br_fdb_add declares u8 protocol = RTPROT_UNSPEC but never parses tb[NDA_PROTOCOL]. The parsing block (if (tb[NDA_PROTOCOL]) protocol = nla_get_u8(...)) exists in br_fdb_delete but is missing from br_fdb_add.
There was a problem hiding this comment.
Still unaddressed in iteration 2. This bug remains in the current diff.
There was a problem hiding this comment.
Fixed in 41a91c8. br_fdb_add now parses tb[NDA_PROTOCOL] via nla_get_u8. ✅
There was a problem hiding this comment.
Still unaddressed in 0ae2cb5. br_fdb_add declares u8 protocol = RTPROT_UNSPEC but never parses tb[NDA_PROTOCOL]. The parsing exists in br_fdb_delete but is still missing from br_fdb_add.
There was a problem hiding this comment.
Fixed in 27a7faf. br_fdb_add now correctly parses tb[NDA_PROTOCOL] via nla_get_u8. ✅
There was a problem hiding this comment.
Verified fixed in 27a7faf. br_fdb_add now parses NDA_PROTOCOL.
banidoru
left a comment
banidoru
left a comment
There was a problem hiding this comment.
- Patch ordering concern: Patch 2 (protocol field) modifies
br_fdb_external_learn_addsignature from patch 1 but gets argument order wrong inbr.c(already noted by prior reviewer). - Missing NDA_PROTOCOL parse in br_fdb_add:
protocolis initialized toRTPROT_UNSPECbut never read fromtb[NDA_PROTOCOL], so bridge fdb add silently ignores user-specified protocol (already noted). - Internal callers can silently overwrite protocol:
vxlan_snoopandvxlan_fdb_external_learn_addpassRTPROT_UNSPEC— if the entry already has a real protocol set, it gets silently cleared on update. - Patches are well-structured overall: Clean kernel patch backports with proper commit messages and sign-offs. The NTF_EXT_VALIDATED and NTF_EXT_MH_PEER_SYNC patches look correct.
| int err; | ||
|
|
||
| if (!(ndm->ndm_state & (NUD_PERMANENT|NUD_REACHABLE))) { | ||
| @@ -1281,7 +1296,7 @@ static int vxlan_fdb_add(struct ndmsg *ndm, struct nlattr *tb[], |
There was a problem hiding this comment.
Protocol overwrite on internal update. When vxlan_fdb_update_existing is called by internal callers like vxlan_snoop or vxlan_fdb_external_learn_add with RTPROT_UNSPEC, this block will overwrite any previously-set protocol value back to 0. Consider guarding with if (protocol != RTPROT_UNSPEC && f->protocol != protocol) to avoid unintentional protocol clearing by internal callers that don't carry protocol context.
There was a problem hiding this comment.
Still open. vxlan_fdb_update_existing unconditionally sets f->protocol = protocol when they differ. Internal callers like vxlan_snoop pass RTPROT_UNSPEC, which will overwrite a previously-set protocol value.
There was a problem hiding this comment.
Still unaddressed. vxlan_fdb_update_existing unconditionally overwrites f->protocol when values differ. Internal callers like vxlan_snoop pass RTPROT_UNSPEC, which will clear a previously-set protocol value (e.g. RTPROT_ZEBRA → RTPROT_UNSPEC).
There was a problem hiding this comment.
Still unaddressed in iteration 2. This bug remains in the current diff.
There was a problem hiding this comment.
Still unaddressed in 41a91c8. vxlan_fdb_update_existing unconditionally overwrites f->protocol when values differ. Internal callers like vxlan_snoop pass RTPROT_UNSPEC, which will clear a previously-set protocol (e.g. RTPROT_ZEBRA → RTPROT_UNSPEC). Needs a guard: if (protocol != RTPROT_UNSPEC && ...).
There was a problem hiding this comment.
Fixed in 0ae2cb5. Guard now uses if (protocol != RTPROT_UNSPEC && f->protocol != protocol). ✅
There was a problem hiding this comment.
Fixed in 0ae2cb5. vxlan_fdb_update_existing now guards with if (protocol != RTPROT_UNSPEC && f->protocol != protocol), preventing internal callers from clobbering a previously-set protocol. ✅
There was a problem hiding this comment.
Verified fixed. vxlan_fdb_update_existing now guards with if (protocol != RTPROT_UNSPEC && f->protocol != protocol).
There was a problem hiding this comment.
Fixed. vxlan_fdb_update_existing now guards with if (protocol != RTPROT_UNSPEC && f->protocol != protocol), preventing snoop from clearing control-plane protocol. ✅
| if (!fdb || READ_ONCE(fdb->dst) != p) | ||
| return -ENOENT; | ||
|
|
||
| + /* If the delete comes from a different protocol type, |
There was a problem hiding this comment.
Non-extern_learn path doesn't propagate protocol. When __br_fdb_add takes the fdb_add_entry path (non-extern_learn), the protocol parameter is unused — fdb_add_entry never receives or sets it. This means bridge fdb add ... proto hw without extern_learn will silently store RTPROT_UNSPEC. Either fdb_add_entry should also accept and set protocol, or the kernel should reject protocol with non-extern_learn entries.
There was a problem hiding this comment.
Still open. __br_fdb_add passes protocol to br_fdb_external_learn_add but the fdb_add_entry path (non-extern_learn) still does not receive or set the protocol field.
There was a problem hiding this comment.
Still unaddressed. fdb_add_entry does not accept or set the protocol field. bridge fdb add without extern_learn will always store RTPROT_UNSPEC regardless of what protocol is specified.
There was a problem hiding this comment.
Still unaddressed in iteration 2. This bug remains in the current diff.
There was a problem hiding this comment.
Still unaddressed in 41a91c8. fdb_add_entry (non-extern_learn path) still does not receive or set the protocol field. bridge fdb add without extern_learn will always store RTPROT_UNSPEC.
There was a problem hiding this comment.
Still unaddressed in 0ae2cb5. fdb_add_entry does not accept or set the protocol field. The non-extern_learn path will always store RTPROT_UNSPEC.
There was a problem hiding this comment.
Fixed in 27a7faf. fdb_add_entry now accepts a protocol parameter and sets fdb->protocol accordingly. Both the add-path parsing and propagation are addressed. ✅
There was a problem hiding this comment.
Verified fixed in 27a7faf. fdb_add_entry now propagates the protocol field.
There was a problem hiding this comment.
Fixed. fdb_add_entry now accepts and sets the protocol field. ✅
banidoru
left a comment
banidoru
left a comment
There was a problem hiding this comment.
- VXLAN FDB protocol field not emitted in netlink dumps:
vxlan_fdb_info()is not updated to includeNDA_PROTOCOLin the protocol patch, so VXLAN FDB dumps won't show the protocol even though it's stored internally. Bridge side (fdb_fill_info) does this correctly. - Existing review comments correctly identify: wrong argument order in
br_switchdev_eventcall, missingNDA_PROTOCOLparsing inbr_fdb_add, protocol not propagated throughfdb_add_entrypath, and protocol overwrite risk from internal VXLAN callers. - NTF_EXT_VALIDATED patch is well-designed — proper state validation, GC exemption, stale-instead-of-failed fallback, and carrier-down protection.
- NTF_EXT_MH_PEER_SYNC / ext_flags plumbing through VXLAN and bridge layers looks correct.
- All three patches use
0001-prefix — consider renaming to0001-/0002-/0003-for clarity in the series.
| 6 files changed, 85 insertions(+), 41 deletions(-) | ||
|
|
||
| diff --git a/drivers/net/vxlan/vxlan_core.c b/drivers/net/vxlan/vxlan_core.c | ||
| index de1b3fa96..c34b9f75c 100644 |
There was a problem hiding this comment.
Missing: vxlan_fdb_info() does not emit NDA_PROTOCOL. The protocol field is stored in vxlan_fdb and parsed/updated correctly, but vxlan_fdb_info() (the function that builds netlink messages for VXLAN FDB dumps) is never updated to include nla_put_u8(skb, NDA_PROTOCOL, fdb->protocol). This means bridge fdb show for VXLAN devices won't display the protocol, even though it's correctly tracked internally.
Compare with the bridge side where fdb_fill_info() correctly adds NDA_PROTOCOL. The same needs to happen in vxlan_fdb_info() — likely right after the existing NDA_VNI put.
There was a problem hiding this comment.
Fixed in the latest push — vxlan_fdb_info() now emits NDA_PROTOCOL. Thanks!
There was a problem hiding this comment.
Confirmed fixed. vxlan_fdb_info() now emits NDA_PROTOCOL via nla_put_u8. ✅
There was a problem hiding this comment.
Verified fixed. vxlan_fdb_info now emits NDA_PROTOCOL.
|
Patch 3 (NTF_EXT_VALIDATED) is upstreamed and merged — great. Patches 1 and 2 appear to be Cisco-specific and not yet submitted upstream. Are there plans to upstream these? Carrying out-of-tree kernel patches long-term creates maintenance burden on every kernel version bump. If there are upstream submissions in progress, linking them here would help reviewers assess the risk. |
|
The |
|
The bugs flagged by banidoru (wrong argument order in |
|
Agree with what Tamer has said about upstreaming patches. There is already a fullcone NAT patch in this repo that hasn't been upstreamed, requires significant maintenance work to update, and is currently not getting applied on the Trixie kernel because no one has updated it. I do not want another instance of that. Additionally, if a patch has been upstreamed, please use the git patch from upstream, adjust for any patch conflicts, and (preferably) add the commit ID in the patch description. |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
banidoru
left a comment
banidoru
left a comment
There was a problem hiding this comment.
Re-review (iteration 2):
- Fixed:
vxlan_fdb_info()now emitsNDA_PROTOCOL— resolved - Still open (bug):
br_switchdev_eventpassesfdb_info->lockedasprotocol— wrong argument order after patch 0002 changes the signature - Still open (bug):
br_fdb_addnever parsestb[NDA_PROTOCOL]— protocol is alwaysRTPROT_UNSPECin the bridge add path - Still open:
vxlan_fdb_update_existingunconditionally overwrites protocol from internal callers passingRTPROT_UNSPEC - Still open: non-extern_learn path in
__br_fdb_addignores the protocol parameter
banidoru
left a comment
banidoru
left a comment
There was a problem hiding this comment.
Re-review (iteration 2) — 4 of 5 previous issues remain unaddressed:
- br_switchdev_event wrong arg order:
fdb_info->lockedpassed asprotocolparam — will interpret bool as protocol type - br_fdb_add missing NDA_PROTOCOL parse: protocol always
RTPROT_UNSPECon add path - vxlan_fdb_update_existing unconditional overwrite: internal callers (snoop) clear previously-set protocol to
RTPROT_UNSPEC - fdb_add_entry ignores protocol: non-extern_learn bridge FDB adds never store protocol
banidoru
left a comment
banidoru
left a comment
There was a problem hiding this comment.
- Patch 0002 bug:
br_switchdev_eventpassesfdb_info->lockedasprotocol— wrong argument order after signature change - Patch 0002 bug:
br_fdb_addnever parsestb[NDA_PROTOCOL]—bridge fdb add ... proto hwsilently ignored - Patch 0002 design:
vxlan_fdb_update_existingunconditionally overwrites protocol — internal callers (e.g.vxlan_snoop) will clear previously-set protocol values - Patch 0002 design:
fdb_add_entry(non-extern_learn path) ignores protocol param — silent data loss - Patch 0002 fix confirmed:
vxlan_fdb_infonow emitsNDA_PROTOCOL✅ - Patches 0001, 0003: No new issues found; well-structured kernel patches with proper state machine handling
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
banidoru
left a comment
banidoru
left a comment
There was a problem hiding this comment.
- ✅ Fixed:
br_switchdev_eventarg order —RTPROT_UNSPECnow correctly passed asprotocol - ✅ Fixed:
br_fdb_addnow parsestb[NDA_PROTOCOL]from netlink - ❌ Still open:
vxlan_fdb_update_existingunconditionally overwritesf->protocol— internal callers likevxlan_snoopwill clear a previously-set protocol value - ❌ Still open:
fdb_add_entry(non-extern_learn path) ignores protocol —bridge fdb addwithoutextern_learnalways storesRTPROT_UNSPEC
banidoru
left a comment
banidoru
left a comment
There was a problem hiding this comment.
Iteration 3 re-review:
- ✅
br_switchdev_eventargument order fixed —RTPROT_UNSPECnow correctly passed asprotocol - ✅
br_fdb_addnow parsestb[NDA_PROTOCOL] - ✅
vxlan_fdb_info()emitsNDA_PROTOCOL(confirmed in prior iteration) - ❌
vxlan_fdb_update_existingstill unconditionally overwritesf->protocol— internal callers passingRTPROT_UNSPECwill clear a previously-set protocol - ❌
fdb_add_entry(non-extern_learn path) still ignores the protocol parameter
| int err; | ||
|
|
||
| if (!(ndm->ndm_state & (NUD_PERMANENT|NUD_REACHABLE))) { | ||
| @@ -1281,7 +1296,7 @@ static int vxlan_fdb_add(struct ndmsg *ndm, struct nlattr *tb[], |
There was a problem hiding this comment.
Still unaddressed in iteration 3. vxlan_fdb_update_existing does if (f->protocol != protocol) { f->protocol = protocol; } but internal callers like vxlan_snoop pass RTPROT_UNSPEC. This will silently clear a protocol value set by the control plane (e.g. RTPROT_ZEBRA → RTPROT_UNSPEC).
Suggested fix: if (protocol != RTPROT_UNSPEC && f->protocol != protocol)
There was a problem hiding this comment.
Fixed in 0ae2cb5. vxlan_fdb_update_existing now correctly guards with if (protocol != RTPROT_UNSPEC && f->protocol != protocol). ✅
| if (!fdb || READ_ONCE(fdb->dst) != p) | ||
| return -ENOENT; | ||
|
|
||
| + /* If the delete comes from a different protocol type, |
There was a problem hiding this comment.
Still unaddressed in iteration 3. fdb_add_entry does not accept or set the protocol field. bridge fdb add without extern_learn will always store RTPROT_UNSPEC regardless of what protocol the user specifies.
There was a problem hiding this comment.
Still unaddressed in 0ae2cb5. The commit message claims "fdb_add_entry: propagate protocol for non-extern_learn path" but __br_fdb_add still calls fdb_add_entry(br, p, addr, ndm, nlh_flags, vid, nfea_tb) without passing protocol. The fdb_add_entry function signature is unchanged.
There was a problem hiding this comment.
Still unaddressed in 0ae2cb5. br_fdb_add never parses tb[NDA_PROTOCOL] so protocol is always RTPROT_UNSPEC — even __br_fdb_add receives it but it is always 0. Both the add-path parsing and fdb_add_entry propagation are missing.
There was a problem hiding this comment.
Confirmed still unaddressed in 0ae2cb5. br_fdb_add declares u8 protocol = RTPROT_UNSPEC at line 1218 but there is no if (tb[NDA_PROTOCOL]) protocol = nla_get_u8(...) anywhere before it's used. The non-extern_learn path (fdb_add_entry) also never receives protocol. Both the add-path parse and fdb_add_entry propagation are missing.
There was a problem hiding this comment.
Fixed in 27a7faf. br_fdb_add now parses tb[NDA_PROTOCOL] and passes it through __br_fdb_add → fdb_add_entry. Both paths (extern_learn and non-extern_learn) now propagate protocol correctly. ✅
There was a problem hiding this comment.
Fixed. The update path now correctly guards with if (protocol != RTPROT_UNSPEC && fdb->protocol != protocol). ✅
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
One or more co-authors of this pull request were not found. You must specify co-authors in commit message trailer via: Supported
Alternatively, if the co-author should not be included, remove the Please update your commit message(s) by doing |
|
/azp run |
|
Azure Pipelines will not run the associated pipelines, because the pull request was updated after the run command was issued. Review the pull request again and issue a new run command. |
|
/azp run |
|
Azure Pipelines will not run the associated pipelines, because the pull request was updated after the run command was issued. Review the pull request again and issue a new run command. |
Add three Linux kernel patches required for EVPN VXLAN Multihoming in SONiC: 1. NDA_FLAGS_EXT with NTF_EXT_MH_PEER_SYNC - Extended FDB flags for multi-homing peer synchronization across EVPN-MH peers. 2. Protocol field in bridge FDB - Distinguishes control plane (zebra) vs data plane (hw) learned MACs for proper EVPN-MH state management. 3. NTF_EXT_VALIDATED for externally validated neighbors - Prevents kernel from invalidating neighbor entries managed by the control plane during ES link transitions. These patches are essential for the EVPN-MH feature described in: https://github.com/sonic-net/SONiC/blob/master/doc/vxlan/EVPN/EVPN_VxLAN_Multihoming.md Co-authored-by: Barry Friedman (friedman) <friedman@cisco.com> Co-authored-by: Mrinmoy Ghosh <mrghosh@cisco.com> Signed-off-by: Tamer Ahmed <tamerahmed@microsoft.com>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
banidoru
left a comment
banidoru
left a comment
There was a problem hiding this comment.
Iteration 4 Re-review (0ae2cb5)
Fixes confirmed:
- ✅
vxlan_fdb_update_existing: now guards withif (protocol != RTPROT_UNSPEC && ...)— prevents internal callers from clearing protocol - ✅
vxlan_fdb_info(): emitsNDA_PROTOCOL - ✅
br_fdb_add: now parsestb[NDA_PROTOCOL] - ✅
br_switchdev_event: correct argument order withRTPROT_UNSPECas protocol
Remaining issue:
- ❌
fdb_add_entry(non-extern_learn path) still does not receive or set protocol.__br_fdb_addpasses protocol tobr_fdb_external_learn_addbut the else branch callsfdb_add_entry(br, p, addr, ndm, nlh_flags, vid, nfea_tb)unchanged. Commit message claims this is fixed but the code does not reflect it.
banidoru
left a comment
banidoru
left a comment
There was a problem hiding this comment.
- APPROVE partial:
vxlan_fdb_update_existingprotocol guard andbr_switchdev_eventarg order both fixed ✅ - Bug:
br_fdb_addnever parsestb[NDA_PROTOCOL]— protocol is alwaysRTPROT_UNSPECon the add path - Bug:
fdb_add_entry(non-extern_learn) still does not receive or set protocol —bridge fdb addwithoutextern_learnignores the proto field - These two issues mean protocol only works for
extern_learnentries added via bridge; regular bridge FDB adds silently lose it - VXLAN side looks correct and complete
banidoru
left a comment
banidoru
left a comment
There was a problem hiding this comment.
br_fdb_adddeclaresu8 protocol = RTPROT_UNSPECbut never parsestb[NDA_PROTOCOL]— protocol is dead code in the add pathfdb_add_entry(non-extern_learn branch) still doesn't receive or set protocol — entries created via this path always haveRTPROT_UNSPEC- VXLAN protocol guard (
RTPROT_UNSPECcheck invxlan_fdb_update_existing) looks correct now - Commit message claims "br_fdb_add: parse NDA_PROTOCOL from netlink add path" but the code doesn't do this
- Patch 3 (NTF_EXT_VALIDATED) looks solid
| - err = vxlan_fdb_update(vxlan, addr, &ip, ndm->ndm_state, flags, | ||
| - port, src_vni, vni, ifindex, | ||
| - ndm->ndm_flags | NTF_VXLAN_ADDED_BY_USER, | ||
| - nhid, true, extack); | ||
| + err = vxlan_fdb_update(vxlan, addr, &ip, ndm->ndm_state, flags, port, | ||
| + src_vni, vni, ifindex, | ||
| + ndm->ndm_flags | NTF_VXLAN_ADDED_BY_USER, nhid, | ||
| + true, extack, ext_flags); |
There was a problem hiding this comment.
Is there a reason the formatting was changed for just adding one argument at the end of the function call? This applies to all instances of this (function declarations and calls).
There was a problem hiding this comment.
Minor style nit — reformatting the entire call when adding one argument is common in kernel patches to stay within 80-col limits. No functional concern here.
There was a problem hiding this comment.
Acknowledged. Style preference; not a blocker.
| /* Extended flags under NDA_FLAGS_EXT: */ | ||
| #define NTF_EXT_MANAGED (1 << 0) | ||
| #define NTF_EXT_LOCKED (1 << 1) | ||
| +#define NTF_EXT_MH_PEER_SYNC (1 << 2) |
There was a problem hiding this comment.
As of the 6.16 kernel, this bit is used for NTF_EXT_EXT_VALIDATED. Can this be changed to 1 << 3, in case SONiC uses some application in the near future that actually uses this bit for something?
There was a problem hiding this comment.
Confirmed: NTF_EXT_MH_PEER_SYNC is already defined as (1 << 3), which avoids the conflict with NTF_EXT_EXT_VALIDATED (1 << 2) in 6.16+. ✅
There was a problem hiding this comment.
Verified. NTF_EXT_MH_PEER_SYNC is defined as (1 << 3), avoiding the conflict.
Signed-off-by: Tamer Ahmed <tamerahmed@microsoft.com>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
banidoru
left a comment
banidoru
left a comment
There was a problem hiding this comment.
- All previously reported bugs (br_fdb_add NDA_PROTOCOL parsing, fdb_add_entry propagation, vxlan protocol overwrite guard, switchdev arg order, vxlan_fdb_info NDA_PROTOCOL emit) are now fixed. ✅
- NTF_EXT_MH_PEER_SYNC correctly uses
1 << 3avoiding conflict with upstreamNTF_EXT_EXT_VALIDATED. - Remaining bug:
br_fdb_external_learn_addupdate path still lacksRTPROT_UNSPECguard — switchdev caller will clobber existing protocol values. - Could not resolve threads due to repo permissions (replies posted acknowledging fixes).
| test_and_clear_bit(BR_FDB_DYNAMIC_LEARNED, &fdb->flags)) | ||
| atomic_dec(&br->fdb_n_learned); | ||
|
|
||
| + if (fdb->protocol != protocol) { |
There was a problem hiding this comment.
Bug: Missing RTPROT_UNSPEC guard in br_fdb_external_learn_add update path.
The existing-entry update block does:
if (fdb->protocol != protocol) {
modified = true;
fdb->protocol = protocol;
}But br_switchdev_event calls this with RTPROT_UNSPEC. If the entry already has RTPROT_ZEBRA (set by control plane), switchdev will clobber it to RTPROT_UNSPEC.
This is the same class of bug that was correctly fixed in vxlan_fdb_update_existing. Apply the same guard:
if (protocol != RTPROT_UNSPEC && fdb->protocol != protocol) {
modified = true;
fdb->protocol = protocol;
}There was a problem hiding this comment.
Still open in 27a7faf. br_fdb_external_learn_add update path does if (fdb->protocol != protocol) { modified = true; fdb->protocol = protocol; } without guarding against RTPROT_UNSPEC. Since br_switchdev_event passes RTPROT_UNSPEC, this will clobber a control-plane-set protocol (e.g. RTPROT_ZEBRA → RTPROT_UNSPEC). Apply the same guard as the VXLAN side: if (protocol != RTPROT_UNSPEC && fdb->protocol != protocol).
There was a problem hiding this comment.
Still open. The update path at line 534 does if (fdb->protocol != protocol) without guarding against RTPROT_UNSPEC. When br_switchdev_event calls this with RTPROT_UNSPEC, it will overwrite a previously-set protocol (e.g. RTPROT_ZEBRA) with RTPROT_UNSPEC. Needs the same protocol != RTPROT_UNSPEC && guard used in vxlan_fdb_update_existing.
There was a problem hiding this comment.
Fixed. br_fdb_external_learn_add update path now guards with if (protocol != RTPROT_UNSPEC && ...). ✅
banidoru
left a comment
banidoru
left a comment
There was a problem hiding this comment.
Re-review of iteration 5 (27a7faf):
- ✅ All previously reported bugs now fixed: argument order in
br_switchdev_event,br_fdb_addNDA_PROTOCOL parsing,fdb_add_entryprotocol propagation,vxlan_fdb_update_existingRTPROT_UNSPEC guard ⚠️ One open issue remains:br_fdb_external_learn_addupdate path still unconditionally overwritesfdb->protocol— needs the sameprotocol != RTPROT_UNSPECguard applied in the VXLAN side- Protocol-aware delete in both bridge and VXLAN (
fdb_delete_by_addr_and_port,vxlan_fdb_delete) is a nice addition - New
NTF_EXT_VALIDATEDpatch (0003) looks correct — proper integration with neigh GC skip and state machine
banidoru
left a comment
banidoru
left a comment
There was a problem hiding this comment.
- All previously raised issues are fixed except one:
br_fdb_external_learn_addupdate path still lacksRTPROT_UNSPECguard — will overwrite control-plane protocol when called frombr_switchdev_event - Patch 1 (NDA_FLAGS_EXT / NTF_EXT_MH_PEER_SYNC): Correct and clean
- Patch 2 (protocol field): Solid implementation; netlink parse, propagation, and delete-path filtering all look good now
- Patch 3 (NTF_EXT_VALIDATED): Well-designed upstream patch, no concerns
- One remaining bug to fix before merge (the RTPROT_UNSPEC guard noted above)
Signed-off-by: Tamer Ahmed <tamerahmed@microsoft.com>
Signed-off-by: Tamer Ahmed <tamerahmed@microsoft.com>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
banidoru
left a comment
banidoru
left a comment
There was a problem hiding this comment.
All previously reported bugs are now fixed. Remaining minor items:
- Unused variable in patch 0002:
vxlan_fdb_external_learn_adddeclaresu32 ext_flagswhich is never used (leftover from refactoring) - Whitespace inconsistency in
vxlan_private.h:ext_flagsandprotocolfields use spaces for alignment while surrounding fields use tabs - Patch 0003 is a clean upstream cherry-pick (NTF_EXT_VALIDATED) — no issues
- Protocol-aware delete semantics look correct and well-guarded
- Series comment about dropping 0003 when kernel >= 6.12 is helpful
| goto out; | ||
| } | ||
| @@ -1301,7 +1316,7 @@ out: | ||
|
|
There was a problem hiding this comment.
Unused variable. ext_flags is declared here but never referenced in this function. This will cause a compiler warning (-Wunused-variable). Remove it.
There was a problem hiding this comment.
Still present — ext_flags is declared but never used in vxlan_fdb_delete. It's only passed to vxlan_fdb_parse which writes it, but it's never read after that. Minor compiler warning risk, but not a blocker.
| + __be32 *vni, u32 *ifindex, u32 *nhid, u8 *protocol, | ||
| struct netlink_ext_ack *extack, u32 *ext_flags) | ||
| { | ||
| struct net *net = dev_net(vxlan->dev); |
There was a problem hiding this comment.
Minor: inconsistent indentation. The surrounding struct members use tab+spaces for alignment (\t\t ), but ext_flags and protocol use just spaces. Match the existing style for consistency:
u32\t\t ext_flags;
u8\t\t protocol;
banidoru
left a comment
banidoru
left a comment
There was a problem hiding this comment.
- All prior blocking issues (protocol guard in update paths, missing propagation in fdb_add_entry, vxlan_fdb_update_existing) confirmed fixed
- Protocol field correctly guarded with
RTPROT_UNSPECcheck to avoid clobbering on unspecified updates - Delete path properly scoped by protocol to prevent cross-protocol removal
- Remaining cosmetic nits (struct indentation) are non-blocking
- LGTM — patches are well-structured and backward compatible
banidoru
left a comment
banidoru
left a comment
There was a problem hiding this comment.
- All previously reported bugs (argument ordering, protocol parsing, RTPROT_UNSPEC guard, fdb_add_entry propagation, vxlan_fdb_info emission) are fixed
br_fdb_external_learn_addupdate path now correctly guards against RTPROT_UNSPEC overwrite- Protocol-aware delete semantics are sound — only deletes matching protocol entries
- Minor:
ext_flagsinvxlan_fdb_deleteis written byvxlan_fdb_parsebut never read — harmless but could suppress with(void)ext_flagsor__always_unused - LGTM — no remaining correctness issues
6 participants
Why I did it
This PR adds three critical Linux kernel patches required to enable EVPN VXLAN Multihoming in SONiC. These kernel enhancements provide the necessary infrastructure for:
These patches are essential for implementing the EVPN-MH feature as described in the EVPN VXLAN Multihoming HLD.
NOTE: The file patches-sonic/0003-neighbor-Add-NTF_EXT_VALIDATED-flag-for-externally-v.patch has been committed upstream to the Linux kernel master branch. See torvalds/linux@03dc03f
Work item tracking
How I did it
Added three kernel patches to patches-sonic directory:
1. NDA_FLAGS_EXT Support with NTF_EXT_MH_PEER_SYNC (0001-vxlan-bridge-Add-NDA_FLAGS_EXT-support-with-NTF_EXT_.patch)
This patch adds extended flags support for VXLAN and bridge FDB entries to enable multi-homing peer synchronization:
ext_flagsinvxlan_fdbstructureNTF_EXT_MH_PEER_SYNC- Indicates FDB entry is synchronized across EVPN-MH peersNEIGH_UPDATE_F_EXT_MH_PEER_SYNCfor propagating sync statevxlan_fdb_alloc()- Initialize ext_flagsvxlan_fdb_create()- Pass ext_flags parametervxlan_fdb_update_existing()- Handle ext_flags updates and notificationsvxlan_fdb_update_create()- Create FDB with ext_flagsvxlan_fdb_info()- Include NDA_FLAGS_EXT in netlink messagesFiles modified:
drivers/net/vxlan/vxlan_core.c(140 lines)drivers/net/vxlan/vxlan_private.h(21 lines)drivers/net/vxlan/vxlan_vnifilter.c(11 lines)include/net/neighbour.h(4 lines)include/uapi/linux/neighbour.h(1 line)net/bridge/br.c(4 lines)net/bridge/br_fdb.c(35 lines)net/bridge/br_private.h(5 lines)net/core/neighbour.c(13 lines)2. Protocol Field in Bridge FDB (0001-net-bridge-vxlan-Protocol-field-in-bridge-fdb.patch)
This patch introduces an optional "protocol" field for bridge FDB entries to distinguish between control plane and data plane learned MAC addresses:
Purpose: In EVPN Multihoming, MAC addresses can be learned via:
This distinction enables:
Implementation:
protocolinnet_bridge_fdb_entryandvxlan_fdbstructuresRTPROT_UNSPECwhen protocol not specified (backward compatible)Usage Example:
Files modified:
drivers/net/vxlan/vxlan_core.c(55 lines)drivers/net/vxlan/vxlan_private.h(5 lines)drivers/net/vxlan/vxlan_vnifilter.c(4 lines)net/bridge/br.c(2 lines)net/bridge/br_fdb.c(55 lines)net/bridge/br_private.h(5 lines)3. NTF_EXT_VALIDATED Flag for External Validation (0001-neighbor-Add-NTF_EXT_VALIDATED-flag-for-externally-v.patch)
This patch adds a new "extern_valid" neighbor flag to indicate entries learned and validated externally that should not be invalidated by the kernel:
Background: In EVPN multi-homing:
Solution (based on draft-rbickhart-evpn-ip-mac-proxy-adv-03):
Implementation:
NTF_EXT_VALIDATED(extern_valid) - Entry is externally validatedUse case: Required for EVPN-MH proxy advertisements where control plane needs full control over neighbor entry validity and removal decisions.
Files modified:
How to verify it
Build kernel with these patches applied:
cd sonic-linux-kernel make BLDENV=bookwormVerify NDA_FLAGS_EXT support:
Verify protocol field support:
Verify extern_valid flag:
Integration testing with EVPN-MH:
Compatibility testing:
Which release branch to backport (provide reason below if selected)
Tested branch (Please provide the tested image version)
Description for the changelog
Add kernel patches for EVPN VXLAN Multihoming: extended FDB flags (NTF_EXT_MH_PEER_SYNC), protocol field for bridge FDB entries, and extern_valid flag for externally validated neighbor entries
Link to config_db schema for YANG model changes
N/A - This PR only adds kernel patches, no CONFIG_DB schema changes
Depends on
Related upstream work
Summary:
Critical for EVPN-MH:
✅ Peer synchronization flag (NTF_EXT_MH_PEER_SYNC)
✅ Control/data plane MAC distinction (protocol field)
✅ External neighbor validation (extern_valid flag)
✅ Proxy advertisement support
✅ Prevents intermittent EVPN-MH failures