Harden import, the kernel task and the RTR client after review

Three independent reviews (hijack resistance, reliability, code health)
of the whole tree. This is what they found that could let a bad route in
or take the daemon down.

Hijacks:
- A route had two paths on a two-octet session, AS_PATH and AS4_PATH,
  read in two places by two rules. A peer could make the origin checked
  on import differ from the one checked at revalidation, and get an
  RPKI-invalid route into the table. AS4_PATH is now merged into AS_PATH
  once, on receipt (RFC 6793 §4.2.3), and nothing else knows of it.
- The next hop is judged on import: it must be usable, agreed on
  (RFC 8950), and by default the peer itself.
- Our own originated prefixes are not taken from other networks.
- A route for a network the machine is on is never put in the kernel.
- Paths with an AS_SET are refused (RFC 9774).
- A configured peer gets one session, not one per BGP Identifier.
- More IPv6 special-purpose prefixes are bogons.

Reliability:
- A batch of requests the kernel refused could overflow the netlink
  socket and end the daemon. The socket has room now, a lost answer or a
  silent kernel means asking what is there, and neither is fatal.
- Working the routes out again within the grace period after a restart
  no longer drops the routes being kept.
- That work can no longer be put off for ever by interface changes.
- Peers whose sessions keep failing are slowed down.
- Unused attribute sets are swept when they pile up, not only each
  minute.
- Log lines a peer can cause are said once in a while.
- An RTR answer is bounded in size and time; data cannot expire between
  two refreshes.
- The shard of a key is taken from a stirred hash: neighbouring bits of
  the hash proved alike for prefixes in a row. The test that showed it,
  failing one run in sixteen, is deterministic now.

The lab has a phase, hijacks, in which FRR tries the first three.
route::apply is a struct with methods instead of nested closures, and
the kernel task's loop is shorter.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

crates/fib/Cargo.toml

@@ -22,6 +22,8 @@ tracing.workspace = true
 # crate builds, and is tested, anywhere.
 [target.'cfg(target_os = "linux")'.dependencies]
 netlink-sys = { version = "0.8", features = ["tokio_socket"] }
+# For a socket option netlink-sys has no name for.
+nix = { workspace = true, features = ["socket"] }
 
 [dev-dependencies]
 proptest.workspace = true

crates/fib/src/interfaces.rs

@@ -85,6 +85,15 @@ impl Interfaces {
         self.addresses.iter().any(|ours| ours.local == address)
     }
 
+    /// Whether `prefix` is the network of one of this machine's addresses,
+    /// or inside one. Whether the interface is up does not matter: the
+    /// network is ours either way.
+    pub fn has_network_around(&self, prefix: &IpPrefix) -> bool {
+        self.addresses
+            .iter()
+            .any(|address| address.network.contains(prefix))
+    }
+
     /// Where `neighbor` can be reached without a gateway: the interface
     /// that is up and has an address in a network `neighbor` is in, the
     /// most specific such network if there are several. `None` if there is

crates/fib/src/kernel.rs

@@ -57,11 +57,27 @@ const READ_BUFFER: usize = 256 * 1024;
 /// are worked out again. Such changes come in bursts.
 const SETTLE: Duration = Duration::from_secs(1);
 
+/// How long the routes are worked out again after at the latest, however
+/// long the changes keep coming: on a machine whose interfaces come and go
+/// all the time there is never a quiet second.
+const SETTLE_AT_MOST: Duration = Duration::from_secs(5);
+
+/// How much the kernel may have waiting for us on a socket. It answers
+/// every request it refuses, and a write is `WRITE_BATCH` requests that
+/// it deals with before any answer is read. The usual allowance holds
+/// fewer than three hundred such answers.
+const RECEIVE_BUFFER: usize = 8 * 1024 * 1024;
+
+/// How long the kernel is waited for. It answers within microseconds, or
+/// not at all.
+const ANSWER_TIMEOUT: Duration = Duration::from_secs(10);
+
 /// The errors of the kernel that have a meaning here.
 mod errno {
     pub const EPERM: i32 = 1;
     pub const ENOENT: i32 = 2;
     pub const ESRCH: i32 = 3;
+    pub const ENOBUFS: i32 = 105;
 }
 
 static ROUTES: LazyLock<IntGauge> = LazyLock::new(|| {
@@ -114,6 +130,20 @@ impl Socket {
     fn open(groups: u32) -> io::Result<Self> {
         let mut socket = TokioSocket::new(NETLINK_ROUTE)?;
         socket.socket_mut().bind(&SocketAddr::new(0, groups))?;
+        // Room for the answers to a whole write, and answers that do not
+        // repeat the request. Both are for the better and neither is
+        // needed: what is lost for want of room is noticed and made good.
+        let raw = socket.socket_ref();
+        if nix::sys::socket::setsockopt(
+            raw,
+            nix::sys::socket::sockopt::RcvBufForce,
+            &RECEIVE_BUFFER,
+        )
+        .is_err()
+        {
+            let _ = raw.set_rx_buf_sz(RECEIVE_BUFFER);
+        }
+        let _ = raw.set_cap_ack(true);
         Ok(Self {
             socket,
             sequence: 0,
@@ -134,6 +164,19 @@ impl Socket {
         Ok(&self.buf)
     }
 
+    /// Waits for the kernel's answer to something it was asked.
+    async fn answer(&mut self) -> io::Result<&[u8]> {
+        self.buf.clear();
+        self.buf.reserve(READ_BUFFER);
+        match tokio::time::timeout(ANSWER_TIMEOUT, self.socket.recv(&mut self.buf)).await {
+            Ok(received) => received.map(|_| &self.buf[..]),
+            Err(_) => Err(io::Error::new(
+                io::ErrorKind::TimedOut,
+                "the kernel did not answer",
+            )),
+        }
+    }
+
     /// Sends one request and gives each message of the answer to `each`,
     /// up to the one that says what became of the request, whose error
     /// number (zero for none) is returned. A dump ends without one, and
@@ -148,7 +191,7 @@ impl Socket {
         request(&mut wire, sequence);
         self.socket.send(&wire).await?;
         loop {
-            let mut rest = self.read().await?;
+            let mut rest = self.answer().await?;
             while !rest.is_empty() {
                 let (header, message, used) = decode(rest)?;
                 rest = &rest[used..];
@@ -190,11 +233,13 @@ impl Socket {
     /// Asks the kernel for `requests`, each a change to a route, and
     /// returns which of them it refused: the place in `requests` and the
     /// error's number.
-    async fn change(
-        &mut self,
-        requests: &[(Change, Route)],
-    ) -> Result<Vec<(usize, i32)>, FibError> {
+    ///
+    /// The kernel may have refused more than there was room to say
+    /// (`ENOBUFS`), or may not have answered. Then what became of some
+    /// requests is not known, and `Changed::lost` says so.
+    async fn change(&mut self, requests: &[(Change, Route)]) -> Result<Changed, FibError> {
         let mut refused = Vec::new();
+        let mut lost = false;
         for batch_start in (0..requests.len()).step_by(WRITE_BATCH) {
             let batch = &requests[batch_start..requests.len().min(batch_start + WRITE_BATCH)];
             // One write for all of them. The kernel answers a request only
@@ -210,7 +255,17 @@ impl Socket {
             self.socket.send(&wire).await?;
             let mut done = false;
             while !done {
-                let mut rest = self.read().await?;
+                let mut rest = match self.answer().await {
+                    Ok(rest) => rest,
+                    Err(e)
+                        if e.raw_os_error() == Some(errno::ENOBUFS)
+                            || e.kind() == io::ErrorKind::TimedOut =>
+                    {
+                        lost = true;
+                        break;
+                    }
+                    Err(e) => return Err(e.into()),
+                };
                 while !rest.is_empty() {
                     let (header, message, used) = decode(rest)?;
                     rest = &rest[used..];
@@ -228,10 +283,19 @@ impl Socket {
                 }
             }
         }
-        Ok(refused)
+        Ok(Changed { refused, lost })
     }
 }
 
+/// What became of changes the kernel was asked for.
+struct Changed {
+    /// Those it refused: the place among the requests, and the error's
+    /// number.
+    refused: Vec<(usize, i32)>,
+    /// Whether some of its answers were lost.
+    lost: bool,
+}
+
 /// The sockets the task uses.
 pub(crate) struct Sockets {
     /// For asking questions, and for the few changes that are made one at
@@ -262,6 +326,37 @@ impl Sockets {
     }
 }
 
+/// When the routes are next to be worked out again, if they are.
+///
+/// Reasons for it come in bursts, so it waits for a quiet moment; but not
+/// for ever, on a machine that has none.
+#[derive(Default)]
+struct Again {
+    /// When the first reason came, and when the latest.
+    since: Option<(Instant, Instant)>,
+}
+
+impl Again {
+    /// There is a reason to work the routes out again.
+    fn soon(&mut self) {
+        let now = Instant::now();
+        self.since = Some((self.since.map_or(now, |(first, _)| first), now));
+    }
+
+    fn due(&self) -> Option<Instant> {
+        self.since
+            .map(|(first, latest)| (latest + SETTLE).min(first + SETTLE_AT_MOST))
+    }
+}
+
+/// Waits until `when`; for ever if there is no such time.
+async fn at(when: Option<Instant>) {
+    match when {
+        Some(when) => sleep_until(when).await,
+        None => std::future::pending().await,
+    }
+}
+
 /// Changes to routes for the kernel, in the order they are to be made.
 struct Batch {
     ops: Vec<Op>,
@@ -278,6 +373,9 @@ enum Report {
     Refused { op: Op, errno: i32 },
     /// The batch with this mark, and all before it, are done.
     Done(u64),
+    /// Some of the kernel's answers were lost: what it has of ours is no
+    /// longer known, and has to be asked for.
+    Lost,
 }
 
 /// Sends the batches to the kernel as they come, and reports what became
@@ -290,13 +388,17 @@ async fn write(
     reports: mpsc::UnboundedSender<Report>,
 ) -> Result<(), FibError> {
     while let Some(batch) = batches.recv().await {
-        for (at, errno) in socket.change(&batch.requests).await? {
+        let changed = socket.change(&batch.requests).await?;
+        for (at, errno) in changed.refused {
             // Nobody to tell only when everything is being torn down.
             let _ = reports.send(Report::Refused {
                 op: batch.ops[at],
                 errno,
             });
         }
+        if changed.lost {
+            let _ = reports.send(Report::Lost);
+        }
         if let Some(mark) = batch.mark {
             let _ = reports.send(Report::Done(mark));
         }
@@ -697,10 +799,18 @@ impl Task {
                             interface: peer.interface,
                         });
                     let interfaces = &self.interfaces;
-                    let target = *self
-                        .resolved
-                        .entry((next_hop, prefix.address_family(), peer))
-                        .or_insert_with(|| resolve(interfaces, &prefix, next_hop.as_ref(), peer));
+                    // Asked of every route, and not remembered with the
+                    // next hop: it is about the prefix.
+                    let target = if interfaces.has_network_around(&prefix) {
+                        Err(Unresolved::OwnNetwork)
+                    } else {
+                        *self
+                            .resolved
+                            .entry((next_hop, prefix.address_family(), peer))
+                            .or_insert_with(|| {
+                                resolve(interfaces, &prefix, next_hop.as_ref(), peer)
+                            })
+                    };
                     ops.extend(self.plan.best(prefix, target));
                 }
                 News::Withdrawn(prefix) => ops.extend(self.plan.withdrawn(prefix)),
@@ -725,6 +835,13 @@ impl Task {
                 }
                 Ok(false)
             }
+            Report::Lost => {
+                ERRORS.inc();
+                warn!(
+                    "Some of the kernel's answers were lost; asking it what it has of our routes"
+                );
+                Ok(true)
+            }
             Report::Refused { op, errno } => {
                 let instead = self.refused(&op, errno)?;
                 self.send(instead.into_iter().collect(), false).await?;
@@ -743,6 +860,43 @@ impl Task {
         }
     }
 
+    /// Takes in what the kernel says unasked, or the failure to read it.
+    /// Returns whether the routes should be worked out again.
+    async fn on_news(&mut self, read: io::Result<&[u8]>) -> Result<bool, FibError> {
+        let mut rest = match read {
+            Ok(rest) => rest,
+            // The kernel had more to say than the socket had room for:
+            // what was missed is asked for.
+            Err(e) if e.raw_os_error() == Some(errno::ENOBUFS) => {
+                warn!("Missed news from the kernel; asking for the interfaces again");
+                self.learn_interfaces().await?;
+                return Ok(true);
+            }
+            Err(e) => return Err(e.into()),
+        };
+        let mut changed = false;
+        while !rest.is_empty() {
+            let (_, message, used) = decode(rest)?;
+            rest = &rest[used..];
+            if self.interfaces.apply(&message) {
+                self.resolved.clear();
+                changed = true;
+            }
+            // A nexthop object of ours has been removed by someone, and
+            // the routes that named it with it.
+            if let Message::Nexthop {
+                present: false,
+                nexthop,
+            } = &message
+                && self.owned.contains(&nexthop.id)
+            {
+                warn!(id = nexthop.id, "A nexthop object of ours was removed");
+                changed = true;
+            }
+        }
+        Ok(changed)
+    }
+
     /// Finds out anew what the kernel has of ours. When interfaces or
     /// nexthop objects go, the kernel removes the routes that used them
     /// without a word, and what was thought to be there no longer is.
@@ -756,7 +910,9 @@ impl Task {
         self.synced = false;
         self.plan = Plan::default();
         let nexthops = self.kernel_nexthops().await?;
-        self.adopt_routes(&nexthops, false).await?;
+        // While routes of an earlier run are being kept for the peers to
+        // announce again, they are kept through this too.
+        self.adopt_routes(&nexthops, self.keeping).await?;
         self.use_nexthops(&nexthops);
         Ok(())
     }
@@ -924,7 +1080,7 @@ pub(crate) async fn run(
     // A reader is first given the whole table, which is what brings the
     // kernel in step; starting a new one does it over.
     let mut reader = task.rib.reader();
-    let mut again_at: Option<Instant> = None;
+    let mut again = Again::default();
     loop {
         tokio::select! {
             () = reader.changed() => {
@@ -932,60 +1088,26 @@ pub(crate) async fn run(
                 task.publish(false);
             }
             Some(report) = heard.recv() => {
-                if task.hear(report).await? && again_at.is_none() {
-                    again_at = Some(Instant::now() + SETTLE);
+                if task.hear(report).await? {
+                    again.soon();
                 }
                 task.publish(false);
             }
             read = news.read() => {
-                let changed = match read {
-                    Ok(mut rest) => {
-                        let mut changed = false;
-                        while !rest.is_empty() {
-                            let (_, message, used) = decode(rest)?;
-                            rest = &rest[used..];
-                            if task.interfaces.apply(&message) {
-                                task.resolved.clear();
-                                changed = true;
-                            }
-                            // A nexthop object of ours has been removed
-                            // by someone, and the routes that named it
-                            // with it.
-                            if let Message::Nexthop { present: false, nexthop } = &message
-                                && task.owned.contains(&nexthop.id)
-                            {
-                                warn!(id = nexthop.id, "A nexthop object of ours was removed");
-                                changed = true;
-                            }
-                        }
-                        changed
-                    }
-                    // The kernel had more to say than the socket had room
-                    // for (ENOBUFS): what was missed is asked for.
-                    Err(e) => {
-                        warn!(err = %e, "Missed news from the kernel; asking for the interfaces again");
-                        task.learn_interfaces().await?;
-                        true
-                    }
-                };
-                if changed {
+                if task.on_news(read).await? {
                     // What a next hop resolves to may be different now,
                     // for any route.
-                    again_at = Some(Instant::now() + SETTLE);
+                    again.soon();
                     task.publish(true);
                 }
             }
-            () = async { sleep_until(again_at.expect("checked by the guard")).await },
-                if again_at.is_some() =>
-            {
-                again_at = None;
+            () = at(again.due()), if again.due().is_some() => {
+                again = Again::default();
                 info!("Working the kernel's routes out again");
                 task.relearn(&mut heard).await?;
                 reader = task.rib.reader();
             }
-            () = async { sleep_until(release_at.expect("checked by the guard")).await },
-                if release_at.is_some() =>
-            {
+            () = at(release_at), if release_at.is_some() => {
                 release_at = None;
                 task.keeping = false;
                 let stale = task.plan.release();

crates/fib/src/plan.rs

@@ -36,14 +36,20 @@ pub enum Unresolved {
     /// Finding the way to such a next hop through other routes is not
     /// done.
     NotConnected,
+    /// The route is for a network this machine is on itself, or for part
+    /// of one. The kernel reaches that directly; a route from a peer that
+    /// said otherwise would send the neighbors' traffic, and the sessions
+    /// with them, wherever the peer liked.
+    OwnNetwork,
 }
 
 impl Unresolved {
-    pub const ALL: [Self; 4] = [
+    pub const ALL: [Self; 5] = [
         Self::Missing,
         Self::OtherFamily,
         Self::Ourselves,
         Self::NotConnected,
+        Self::OwnNetwork,
     ];
 
     pub fn as_str(self) -> &'static str {
@@ -52,6 +58,7 @@ impl Unresolved {
             Self::OtherFamily => "IPv4 next hop for an IPv6 route",
             Self::Ourselves => "next hop is this machine",
             Self::NotConnected => "next hop not directly connected",
+            Self::OwnNetwork => "inside a network of this machine",
         }
     }
 }
@@ -91,6 +98,9 @@ pub fn resolve(
     next_hop: Option<&NlriNextHop>,
     peer: Option<Neighbor>,
 ) -> Result<Target, Unresolved> {
+    if interfaces.has_network_around(prefix) {
+        return Err(Unresolved::OwnNetwork);
+    }
     let next_hop = next_hop.ok_or(Unresolved::Missing)?;
     let version = match next_hop {
         NlriNextHop::Ipv4(_) => IpVersion::Ipv4,
@@ -208,7 +218,7 @@ pub struct Plan {
     unresolved: HashMap<IpPrefix, Unresolved>,
     /// How many of them for each reason, in the order of
     /// [`Unresolved::ALL`].
-    unresolved_counts: [usize; 4],
+    unresolved_counts: [usize; Unresolved::ALL.len()],
     /// Prefixes whose route the kernel refused.
     refused: HashSet<IpPrefix>,
 }
@@ -308,7 +318,7 @@ impl Plan {
             entry.confirmed = false;
         }
         self.unresolved.clear();
-        self.unresolved_counts = [0; 4];
+        self.unresolved_counts = [0; Unresolved::ALL.len()];
         self.refused.clear();
     }
 
@@ -394,7 +404,7 @@ impl Plan {
     /// Everything goes: the changes that empty the kernel of our routes.
     pub fn clear(&mut self) -> Vec<Op> {
         self.unresolved.clear();
-        self.unresolved_counts = [0; 4];
+        self.unresolved_counts = [0; Unresolved::ALL.len()];
         self.refused.clear();
         self.installed
             .drain()
@@ -485,6 +495,34 @@ mod tests {
             resolve(&interfaces, &v6, Some(&v4_hop("192.0.2.2")), peer),
             Err(Unresolved::OtherFamily)
         );
+        // A route for part of a network we are on is not for the kernel,
+        // however good its next hop; one for more than the network is.
+        for ours in [
+            "192.0.2.0/24",
+            "192.0.2.128/25",
+            "192.0.2.7/32",
+            "2001:db8::/64",
+        ] {
+            let ours = prefix(ours);
+            let hop = match ours.address_family() {
+                IpVersion::Ipv4 => v4_hop("192.0.2.2"),
+                IpVersion::Ipv6 => v6_hop("2001:db8::2"),
+            };
+            assert_eq!(
+                resolve(&interfaces, &ours, Some(&hop), peer),
+                Err(Unresolved::OwnNetwork),
+                "{ours}"
+            );
+        }
+        assert_eq!(
+            resolve(
+                &interfaces,
+                &prefix("192.0.0.0/22"),
+                Some(&v4_hop("192.0.2.2")),
+                peer
+            ),
+            Ok(target("192.0.2.2", 2))
+        );
         // An IPv4 route by way of an IPv6 neighbor: a link with only IPv6
         // addresses (RFC 8950).
         assert_eq!(

crates/fib/tests/kernel.rs

@@ -460,6 +460,59 @@ async fn many_routes_are_installed_and_removed() {
     );
 }
 
+/// Someone empties the table behind our back (`ip route flush`), and then
+/// the peers withdraw everything: the kernel refuses every removal, a
+/// table's worth of refusals at once. That is more than the socket holds
+/// answers for, and must be no more than a reason to look again.
+#[tokio::test(flavor = "multi_thread")]
+#[ignore = "changes the machine's routes; run scripts/kernel-tests.sh"]
+async fn a_flood_of_refusals_does_not_stop_the_daemon() {
+    const ROUTES: u32 = 20_000;
+    let mut machine = Machine::new(11);
+    machine.nexthop_objects = false;
+    let peer = machine.peer(2);
+    let via = machine.via(machine.neighbor(2).into(), 0);
+    let prefixes: Vec<IpPrefix> = (0..ROUTES)
+        .map(|n| {
+            let [_, a, b, c] = n.to_be_bytes();
+            IpPrefix::new(
+                bgp_primitives::constants::IpVersion::Ipv4,
+                [100 + a, b, c, 0],
+                24,
+            )
+            .unwrap()
+        })
+        .collect();
+    machine.rib.announce(peer, &via, &prefixes);
+    let (fib, task) = machine.start();
+    status_becomes(&fib, "all installed", |status| {
+        status.synced && status.installed == ROUTES as usize
+    });
+
+    ip(&format!("route flush table {}", machine.table()));
+    assert!(machine.kernel_routes().is_empty());
+    let routes: Vec<bgp_primitives::ip_prefix::Nlri> =
+        prefixes.iter().map(|prefix| (*prefix).into()).collect();
+    machine.rib.withdraw(peer, &routes);
+    status_becomes(&fib, "nothing installed", |status| {
+        status.synced && status.installed == 0
+    });
+    assert!(
+        !task.is_finished(),
+        "the task that talks to the kernel ended"
+    );
+
+    // And it goes on working.
+    machine.rib.announce(peer, &via, &prefixes[..3]);
+    status_becomes(&fib, "three installed", |status| {
+        status.synced && status.installed == 3
+    });
+    assert_eq!(machine.kernel_routes().len(), 3);
+    assert!(!task.is_finished());
+    fib.stop().await;
+    task.await.unwrap().unwrap();
+}
+
 /// Routes name a nexthop object for their gateway where the kernel has
 /// such objects. The object is ours: made when first needed, put back if
 /// someone removes it, and removed when we stop. Other objects are not