From 980f5afcac066c509932af64f4820f13b921468b Mon Sep 17 00:00:00 2001 From: Renato Westphal Date: Fri, 3 Nov 2023 21:29:43 -0300 Subject: [PATCH] interface: fix handling of interface renames Whenever an interface netlink message is received from the kernel, the interface should be identified by its ifindex, and not by its name. This is because ifindexes are immutable, whereas interface names can change over time. This fix resolves a regression where holod wasn't working correctly within containerlab, which does interface renaming after their initial creation. Additionally, the `Interfaces` struct is updated to use two BTreeMaps backed by an interface arena. This modification allows for efficient lookups using both interface names and ifindexes. Signed-off-by: Renato Westphal --- holo-interface/Cargo.toml | 1 + holo-interface/src/ibus.rs | 6 +- holo-interface/src/interface.rs | 145 +++++++++++++++++++++++--------- holo-interface/src/netlink.rs | 17 +--- 4 files changed, 114 insertions(+), 55 deletions(-) diff --git a/holo-interface/Cargo.toml b/holo-interface/Cargo.toml index 5ccd2304..ae6bd7a4 100644 --- a/holo-interface/Cargo.toml +++ b/holo-interface/Cargo.toml @@ -10,6 +10,7 @@ async-trait.workspace = true derive-new.workspace = true enum-as-inner.workspace = true futures.workspace = true +generational-arena.workspace = true ipnetwork.workspace = true netlink-packet-route.workspace = true netlink-packet-core.workspace = true diff --git a/holo-interface/src/ibus.rs b/holo-interface/src/ibus.rs index 7542b6ca..51957893 100644 --- a/holo-interface/src/ibus.rs +++ b/holo-interface/src/ibus.rs @@ -20,7 +20,7 @@ use crate::Master; pub(crate) fn process_msg(master: &mut Master, msg: IbusMsg) { match msg { IbusMsg::InterfaceDump => { - for iface in master.interfaces.tree.values() { + for iface in master.interfaces.iter() { notify_interface_update( &master.ibus_tx, iface.name.clone(), @@ -31,7 +31,7 @@ pub(crate) fn process_msg(master: &mut Master, msg: IbusMsg) { } } IbusMsg::InterfaceQuery { ifname, af } => { - if let Some(iface) = master.interfaces.tree.get(&ifname) { + if let Some(iface) = master.interfaces.get_by_name(&ifname) { notify_interface_update( &master.ibus_tx, iface.name.clone(), @@ -58,7 +58,7 @@ pub(crate) fn process_msg(master: &mut Master, msg: IbusMsg) { IbusMsg::RouterIdQuery => { notify_router_id_update( &master.ibus_tx, - master.interfaces.router_id, + master.interfaces.router_id(), ); } // Ignore other events. diff --git a/holo-interface/src/interface.rs b/holo-interface/src/interface.rs index b86a4a9e..02c32de8 100644 --- a/holo-interface/src/interface.rs +++ b/holo-interface/src/interface.rs @@ -4,9 +4,10 @@ // SPDX-License-Identifier: MIT // -use std::collections::{btree_map, BTreeMap}; +use std::collections::{BTreeMap, HashMap}; use std::net::{IpAddr, Ipv4Addr}; +use generational_arena::{Arena, Index}; use holo_utils::ibus::IbusSender; use holo_utils::ip::Ipv4NetworkExt; use holo_utils::southbound::{AddressFlags, InterfaceFlags}; @@ -16,10 +17,14 @@ use crate::ibus; #[derive(Debug, Default)] pub struct Interfaces { - // List of interfaces. - pub tree: BTreeMap, + // Interface arena. + arena: Arena, + // Interface binary tree keyed by name (1:1). + name_tree: BTreeMap, + // Interface hash table keyed by ifindex (1:1). + ifindex_tree: HashMap, // Auto-generated Router ID. - pub router_id: Option, + router_id: Option, } #[derive(Debug)] @@ -40,6 +45,7 @@ pub struct InterfaceAddress { // ===== impl Interfaces ===== impl Interfaces { + // Adds or updates the interface with the specified attributes. pub(crate) fn update( &mut self, ifname: String, @@ -48,22 +54,12 @@ impl Interfaces { flags: InterfaceFlags, ibus_tx: Option<&IbusSender>, ) { - match self.tree.entry(ifname.clone()) { - btree_map::Entry::Vacant(v) => { - // If the interface does not exist, create a new entry. - v.insert(Interface { - name: ifname.clone(), - ifindex, - mtu, - flags, - addresses: Default::default(), - }); - } - btree_map::Entry::Occupied(o) => { - let iface = o.into_mut(); + match self.ifindex_tree.get(&ifindex).copied() { + Some(iface_idx) => { + let iface = &mut self.arena[iface_idx]; // If nothing of interest has changed, return early. - if iface.ifindex == ifindex + if iface.name == ifname && iface.mtu == mtu && iface.flags == flags { @@ -71,10 +67,28 @@ impl Interfaces { } // Update the existing interface with the new information. - iface.ifindex = ifindex; + if iface.name != ifname { + self.name_tree.remove(&iface.name); + iface.name = ifname.clone(); + self.name_tree.insert(ifname.clone(), iface_idx); + } iface.mtu = mtu; iface.flags = flags; } + None => { + // If the interface does not exist, create a new entry. + let iface = Interface { + name: ifname.clone(), + ifindex, + mtu, + flags, + addresses: Default::default(), + }; + + let iface_idx = self.arena.insert(iface); + self.name_tree.insert(ifname.clone(), iface_idx); + self.ifindex_tree.insert(ifindex, iface_idx); + } } // Notify protocol instances about the interface update. @@ -83,25 +97,32 @@ impl Interfaces { } } + // Removes the specified interface identified by its ifindex. pub(crate) fn remove( &mut self, - ifname: String, + ifindex: u32, ibus_tx: Option<&IbusSender>, ) { - // Remove interface. - if self.tree.remove(&ifname).is_none() { + let Some(iface_idx) = self.ifindex_tree.get(&ifindex).copied() else { return; - } + }; // Notify protocol instances. + let iface = &self.arena[iface_idx]; if let Some(ibus_tx) = ibus_tx { - ibus::notify_interface_del(ibus_tx, ifname); + ibus::notify_interface_del(ibus_tx, iface.name.clone()); } + // Remove interface. + self.name_tree.remove(&iface.name); + self.ifindex_tree.remove(&iface.ifindex); + self.arena.remove(iface_idx); + // Check if the Router ID needs to be updated. self.update_router_id(ibus_tx); } + // Adds the specified address to the interface identified by its ifindex. pub(crate) fn addr_add( &mut self, ifindex: u32, @@ -114,11 +135,7 @@ impl Interfaces { } // Lookup interface. - let Some(iface) = self - .tree - .values_mut() - .find(|iface| iface.ifindex == ifindex) - else { + let Some(iface) = self.get_mut_by_ifindex(ifindex) else { return; }; @@ -143,6 +160,7 @@ impl Interfaces { self.update_router_id(ibus_tx); } + // Removes the specified address from the interface identified by its ifindex. pub(crate) fn addr_del( &mut self, ifindex: u32, @@ -150,11 +168,7 @@ impl Interfaces { ibus_tx: Option<&IbusSender>, ) { // Lookup interface. - let Some(iface) = self - .tree - .values_mut() - .find(|iface| iface.ifindex == ifindex) - else { + let Some(iface) = self.get_mut_by_ifindex(ifindex) else { return; }; @@ -176,14 +190,18 @@ impl Interfaces { } } + // Returns the auto-generated Router ID. + pub(crate) fn router_id(&self) -> Option { + self.router_id + } + + // Updates the auto-generated Router ID. fn update_router_id(&mut self, ibus_tx: Option<&IbusSender>) { let loopback_interfaces = self - .tree - .values() + .iter() .filter(|iface| iface.flags.contains(InterfaceFlags::LOOPBACK)); let non_loopback_interfaces = self - .tree - .values() + .iter() .filter(|iface| !iface.flags.contains(InterfaceFlags::LOOPBACK)); // Helper function to find the highest IPv4 address among a list of @@ -222,4 +240,55 @@ impl Interfaces { } } } + + // Returns a reference to the interface corresponding to the given name. + pub(crate) fn get_by_name(&self, ifname: &str) -> Option<&Interface> { + self.name_tree + .get(ifname) + .copied() + .map(|iface_idx| &self.arena[iface_idx]) + } + + // Returns a mutable reference to the interface corresponding to the given + // name. + #[allow(dead_code)] + pub(crate) fn get_mut_by_name( + &mut self, + ifname: &str, + ) -> Option<&mut Interface> { + self.name_tree + .get(ifname) + .copied() + .map(move |iface_idx| &mut self.arena[iface_idx]) + } + + // Returns a reference to the interface corresponding to the given ifindex. + #[allow(dead_code)] + pub(crate) fn get_by_ifindex(&self, ifindex: u32) -> Option<&Interface> { + self.ifindex_tree + .get(&ifindex) + .copied() + .map(|iface_idx| &self.arena[iface_idx]) + } + + // Returns a mutable reference to the interface corresponding to the given + // ifindex. + pub(crate) fn get_mut_by_ifindex( + &mut self, + ifindex: u32, + ) -> Option<&mut Interface> { + self.ifindex_tree + .get(&ifindex) + .copied() + .map(move |iface_idx| &mut self.arena[iface_idx]) + } + + // Returns an iterator visiting all interfaces. + // + // Interfaces are ordered by their names. + pub(crate) fn iter(&self) -> impl Iterator + '_ { + self.name_tree + .values() + .map(|iface_idx| &self.arena[*iface_idx]) + } } diff --git a/holo-interface/src/netlink.rs b/holo-interface/src/netlink.rs index 4174d45f..26fef597 100644 --- a/holo-interface/src/netlink.rs +++ b/holo-interface/src/netlink.rs @@ -62,25 +62,14 @@ fn process_newlink_msg(master: &mut Master, msg: LinkMessage, notify: bool) { } fn process_dellink_msg(master: &mut Master, msg: LinkMessage, notify: bool) { - use netlink_packet_route::link::nlas::Nla; - trace!(?msg, "received RTM_DELLINK message"); - // Fetch interface name. - let mut ifname = None; - for nla in msg.nlas.into_iter() { - match nla { - Nla::IfName(nla_ifname) => ifname = Some(nla_ifname), - _ => (), - } - } - let Some(ifname) = ifname else { - return; - }; + // Fetch interface ifindex. + let ifindex = msg.header.index; // Remove interface. let ibus_tx = notify.then_some(&master.ibus_tx); - master.interfaces.remove(ifname, ibus_tx); + master.interfaces.remove(ifindex, ibus_tx); } fn process_newaddr_msg(master: &mut Master, msg: AddressMessage, notify: bool) {