Prevent modunload once underlay has been set (#485)

This PR moves the underlay teardown logic from xde_detach into a new
ioctl (and opteadm command) to unset the XDE underlay. Detach will now
fail so long as the underlay is set -- this successfully prevents
`modunload` from knocking us down after this state has been set.

The semantics are more or less what you'd expect:
* Clearing the underlay will fail if there exists a port.
* Clearing the underlay will fail if there is no underlay.
* Driver detach now requires that you `opteadm clear-xde-underlay`.

bin/opteadm/src/bin/opteadm.rs

@@ -198,6 +198,9 @@ enum Command {
     /// Set up xde underlay devices
     SetXdeUnderlay { u1: String, u2: String },
 
+    /// Clear xde underlay devices
+    ClearXdeUnderlay,
+
     /// Set a virtual-to-physical mapping
     SetV2P { vpc_ip: IpAddr, vpc_mac: MacAddr, underlay_ip: Ipv6Addr, vni: Vni },
 
@@ -655,6 +658,10 @@ fn main() -> anyhow::Result<()> {
             let _ = hdl.set_xde_underlay(&u1, &u2)?;
         }
 
+        Command::ClearXdeUnderlay => {
+            let _ = hdl.clear_xde_underlay()?;
+        }
+
         Command::RmFwRule { port, direction, id } => {
             let request = RemFwRuleReq { port_name: port, dir: direction, id };
             hdl.remove_firewall_rule(&request)?;

bin/opteadm/src/lib.rs

@@ -6,6 +6,7 @@
 
 //! OPTE driver administration library
 
+use opte::api::ClearXdeUnderlayReq;
 use opte::api::NoResp;
 use opte::api::OpteCmd;
 use opte::api::SetXdeUnderlayReq;
@@ -96,6 +97,13 @@ impl OpteAdm {
         run_cmd_ioctl(self.device.as_raw_fd(), cmd, Some(&req))
     }
 
+    /// Clear xde underlay devices
+    pub fn clear_xde_underlay(&self) -> Result<NoResp, Error> {
+        let req = ClearXdeUnderlayReq { _unused: 0 };
+        let cmd = OpteCmd::ClearXdeUnderlay;
+        run_cmd_ioctl(self.device.as_raw_fd(), cmd, Some(&req))
+    }
+
     /// Add a firewall rule
     pub fn add_firewall_rule(
         &self,

crates/opte-api/src/cmd.rs

@@ -40,6 +40,7 @@ pub enum OpteCmd {
     CreateXde = 70,          // create a new xde device
     DeleteXde = 71,          // delete an xde device
     SetXdeUnderlay = 72,     // set xde underlay devices
+    ClearXdeUnderlay = 73,   // clear xde underlay devices
     SetExternalIps = 80,     // set xde external IPs for a port
 }
 
@@ -68,6 +69,7 @@ impl TryFrom<c_int> for OpteCmd {
             70 => Ok(Self::CreateXde),
             71 => Ok(Self::DeleteXde),
             72 => Ok(Self::SetXdeUnderlay),
+            73 => Ok(Self::ClearXdeUnderlay),
             80 => Ok(Self::SetExternalIps),
             _ => Err(()),
         }

crates/opte-api/src/lib.rs

@@ -87,3 +87,9 @@ pub struct SetXdeUnderlayReq {
     pub u1: String,
     pub u2: String,
 }
+
+/// Clear the underlay devices used by the xde kernel module
+#[derive(Clone, Debug, Serialize, Deserialize)]
+pub struct ClearXdeUnderlayReq {
+    pub _unused: u64,
+}

lib/opte-ioctl/src/lib.rs

@@ -4,6 +4,7 @@
 
 // Copyright 2022 Oxide Computer Company
 
+use opte::api::ClearXdeUnderlayReq;
 use opte::api::CmdOk;
 use opte::api::NoResp;
 use opte::api::OpteCmd;
@@ -187,6 +188,13 @@ impl OpteHdl {
         run_cmd_ioctl(self.device.as_raw_fd(), cmd, Some(&req))
     }
 
+    /// Clear xde underlay devices.
+    pub fn clear_xde_underlay(&self) -> Result<NoResp, Error> {
+        let req = ClearXdeUnderlayReq { _unused: 0 };
+        let cmd = OpteCmd::ClearXdeUnderlay;
+        run_cmd_ioctl(self.device.as_raw_fd(), cmd, Some(&req))
+    }
+
     pub fn add_router_entry(
         &self,
         req: &AddRouterEntryReq,

xde-tests/src/lib.rs

@@ -206,6 +206,12 @@ impl Drop for Xde {
     /// When this object is dropped, remove the xde kernel module from the
     /// underlying system.
     fn drop(&mut self) {
+        // The module can no longer be successfully removed until the underlay
+        // has been cleared. This may not have been done, so this is fallible.
+        if let Ok(adm) = OpteAdm::open(OpteAdm::XDE_CTL) {
+            let _ = adm.clear_xde_underlay();
+        }
+
         let mut cmd = Command::new("pfexec");
         cmd.args(["rem_drv", "xde"]);
         if let Err(e) = cmd.output() {

xde/src/xde.rs

@@ -40,6 +40,7 @@ use core::ptr::addr_of;
 use core::ptr::addr_of_mut;
 use core::time::Duration;
 use illumos_sys_hdrs::*;
+use opte::api::ClearXdeUnderlayReq;
 use opte::api::CmdOk;
 use opte::api::Direction;
 use opte::api::NoResp;
@@ -265,7 +266,7 @@ fn get_xde_state() -> &'static XdeState {
     // it was derived from Box::into_raw() during `xde_attach`.
     unsafe {
         let p = ddi_get_driver_private(xde_dip);
-        &mut *(p as *mut XdeState)
+        &*(p as *mut XdeState)
     }
 }
 
@@ -481,6 +482,13 @@ fn set_xde_underlay_hdlr(env: &mut IoctlEnvelope) -> Result<NoResp, OpteError> {
     set_xde_underlay(&req)
 }
 
+fn clear_xde_underlay_hdlr(
+    env: &mut IoctlEnvelope,
+) -> Result<NoResp, OpteError> {
+    let _req: ClearXdeUnderlayReq = env.copy_in_req()?;
+    clear_xde_underlay()
+}
+
 // This is the entry point for all OPTE commands. It verifies the API
 // version and then multiplexes the command to its appropriate handler.
 #[no_mangle]
@@ -534,6 +542,11 @@ unsafe extern "C" fn xde_ioc_opte_cmd(karg: *mut c_void, mode: c_int) -> c_int {
             hdlr_resp(&mut env, resp)
         }
 
+        OpteCmd::ClearXdeUnderlay => {
+            let resp = clear_xde_underlay_hdlr(&mut env);
+            hdlr_resp(&mut env, resp)
+        }
+
         OpteCmd::DumpLayer => {
             let resp = dump_layer_hdlr(&mut env);
             hdlr_resp(&mut env, resp)
@@ -868,6 +881,90 @@ fn set_xde_underlay(req: &SetXdeUnderlayReq) -> Result<NoResp, OpteError> {
     Ok(NoResp::default())
 }
 
+#[no_mangle]
+fn clear_xde_underlay() -> Result<NoResp, OpteError> {
+    let state = get_xde_state();
+    let mut underlay = state.underlay.lock();
+    if underlay.is_none() {
+        return Err(OpteError::System {
+            errno: ENOENT,
+            msg: "underlay not yet initialized".into(),
+        });
+    }
+    if unsafe { xde_devs.read().len() } > 0 {
+        return Err(OpteError::System {
+            errno: EBUSY,
+            msg: "underlay in use by attached ports".into(),
+        });
+    }
+
+    if let Some(underlay) = underlay.take() {
+        // There shouldn't be anymore refs to the underlay given we checked for
+        // 0 ports above.
+        let Some(u1) = Arc::into_inner(underlay.u1) else {
+            return Err(OpteError::System {
+                errno: EBUSY,
+                msg: "underlay u1 has outstanding refs".into(),
+            });
+        };
+        let Some(u2) = Arc::into_inner(underlay.u2) else {
+            return Err(OpteError::System {
+                errno: EBUSY,
+                msg: "underlay u2 has outstanding refs".into(),
+            });
+        };
+
+        for u in [u1, u2] {
+            // Clear all Rx paths
+            u.mch.clear_rx();
+
+            // We have a chain of refs here:
+            //  1. `MacPromiscHandle` holds a ref to `MacClientHandle`, and
+            //  2. `MacUnicastHandle` holds a ref to `MacClientHandle`, and
+            //  3. `MacClientHandle` holds a ref to `MacHandle`.
+            // We explicitly drop them in order here to ensure there are no
+            // outstanding refs.
+
+            // 1. Remove promisc and unicast callbacks
+            drop(u.mph);
+            drop(u.muh);
+
+            // Although `xde_rx` can be called into without any running ports
+            // via the promisc and unicast handles, illumos guarantees that
+            // neither callback will be running here. `mac_promisc_remove` will
+            // either remove the callback immediately (if there are no walkers)
+            // or will mark the callback as condemned and await all active
+            // walkers finishing. Accordingly, no one else will have or try to
+            // clone the MAC client handle.
+
+            // 2. Remove MAC client handle
+            if Arc::into_inner(u.mch).is_none() {
+                warn!(
+                    "underlay {} has outstanding mac client handle refs",
+                    u.name
+                );
+                return Err(OpteError::System {
+                    errno: EBUSY,
+                    msg: format!("underlay {} has outstanding refs", u.name),
+                });
+            }
+
+            // Finally, we can cleanup the MAC handle for this underlay
+            if Arc::into_inner(u.mh).is_none() {
+                return Err(OpteError::System {
+                    errno: EBUSY,
+                    msg: format!(
+                        "underlay {} has outstanding mac handle refs",
+                        u.name
+                    ),
+                });
+            }
+        }
+    }
+
+    Ok(NoResp::default())
+}
+
 const IOCTL_SZ: usize = core::mem::size_of::<OpteCmdIoctl>();
 
 #[no_mangle]
@@ -1141,57 +1238,20 @@ unsafe extern "C" fn xde_detach(
         return DDI_FAILURE;
     }
 
-    let rstate = ddi_get_driver_private(xde_dip);
-    assert!(!rstate.is_null());
-    let state = Box::from_raw(rstate as *mut XdeState);
-    let underlay = state.underlay.into_inner();
+    let state = ddi_get_driver_private(xde_dip) as *mut XdeState;
+    assert!(!state.is_null());
 
-    if let Some(underlay) = underlay {
-        // There shouldn't be anymore refs to the underlay given we checked for
-        // 0 ports above.
-        let Ok(u1) = Arc::try_unwrap(underlay.u1) else {
-            warn!("failed to detach: underlay u1 has outstanding refs");
-            return DDI_FAILURE;
-        };
-        let Ok(u2) = Arc::try_unwrap(underlay.u2) else {
-            warn!("failed to detach: underlay u2 has outstanding refs");
-            return DDI_FAILURE;
-        };
+    let state_ref = &*(state);
+    let underlay = state_ref.underlay.lock();
 
-        for u in [u1, u2] {
-            // Clear all Rx paths
-            u.mch.clear_rx();
-
-            // We have a chain of refs here:
-            //  1. `MacPromiscHandle` holds a ref to `MacClientHandle`, and
-            //  2. `MacUnicastHandle` holds a ref to `MacClientHandle`, and
-            //  3. `MacClientHandle` holds a ref to `MacHandle`.
-            // We explicitly drop them in order here to ensure there are no
-            // outstanding refs.
-
-            // 1. Remove promisc and unicast callbacks
-            drop(u.mph);
-            drop(u.muh);
-
-            // 2. Remove MAC client handle
-            if Arc::strong_count(&u.mch) > 1 {
-                warn!(
-                    "underlay {} has outstanding mac client handle refs",
-                    u.name
-                );
-                return DDI_FAILURE;
-            }
-            drop(u.mch);
-
-            // Finally, we can cleanup the MAC handle for this underlay
-            if Arc::strong_count(&u.mh) > 1 {
-                warn!("underlay {} has outstanding mac handle refs", u.name);
-                return DDI_FAILURE;
-            }
-            drop(u.mh);
-        }
+    if underlay.is_some() {
+        warn!("failed to detach: underlay is set");
+        return DDI_FAILURE;
     }
 
+    // Reattach the XdeState to a Box, which will free it on drop.
+    drop(Box::from_raw(state));
+
     // Remove control device
     ddi_remove_minor_node(xde_dip, XDE_STR);
 

xtask/src/main.rs

@@ -276,8 +276,14 @@ fn cmd_package(
 fn raw_install() -> Result<()> {
     let meta = cargo_meta();
 
-    // NOTE: we don't need to actually check this one, it'll return a
-    // failure code even if we don't care about it.
+    // NOTE: we don't need to actually check either command, they'll
+    // return a failure code even if we don't care about it.
+    // Opteadm may not even be installed/accessible, so also allow it to
+    // fail to run at all.
+    let _ = Command::new("/opt/oxide/opte/bin/opteadm")
+        .arg("clear-xde-underlay")
+        .output();
+
     Command::new("rem_drv").arg("xde").output()?;
 
     let mut conf_path = meta.workspace_root.clone();