diff --git a/.github/buildomat/jobs/check-headers.sh b/.github/buildomat/jobs/check-headers.sh index f961c2d7d..5339fb6ea 100644 --- a/.github/buildomat/jobs/check-headers.sh +++ b/.github/buildomat/jobs/check-headers.sh @@ -18,11 +18,13 @@ set -e GATE_REF="$(./tools/check_headers gate_ref)" -# TODO: `--branch` is overly restrictive, but it's what we've got. In git 2.49 -# the --revision flag was added to `git-clone`, and can clone an arbitrary -# revision, which is more appropriate here. We might be tracking an arbitrary -# commit with some changes in illumos-gate that isn't yet merged, after all. -git clone --depth 1 --branch "$GATE_REF" \ - https://code.oxide.computer/illumos-gate ./gate_src +# `git clone --branch` only accepts branches and tags, but GATE_REF may be an +# arbitrary ref (for example, a Gerrit change ref for an illumos-gate change +# that has not yet merged). An init-and-fetch handles any ref the remote +# advertises. +git init ./gate_src +git -C ./gate_src fetch --depth 1 \ + https://code.oxide.computer/illumos-gate "$GATE_REF" +git -C ./gate_src checkout --detach FETCH_HEAD ./tools/check_headers run ./gate_src diff --git a/Cargo.lock b/Cargo.lock index dd1c5fa8b..0a54e16fe 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5774,7 +5774,7 @@ dependencies = [ [[package]] name = "p4rs" version = "0.1.0" -source = "git+https://github.com/oxidecomputer/p4?branch=main#33d646041f13062cc39c6f38725d8c95d4024a2f" +source = "git+https://github.com/oxidecomputer/p4?branch=zl/multicast#0e8a28a2edce0a96dd0ac3a3df95af3d58cee839" dependencies = [ "bitvec", "num", @@ -8809,7 +8809,7 @@ dependencies = [ [[package]] name = "softnpu" version = "0.2.0" -source = "git+https://github.com/oxidecomputer/softnpu#3828225fd5719dc985a300e36850d25a0405eba6" +source = "git+https://github.com/oxidecomputer/softnpu?branch=zl/multicast#284c6830722548714128e63ea04bcca78ee27154" dependencies = [ "p4rs", "serde", diff --git a/Cargo.toml b/Cargo.toml index f5f8d36b7..a8ea457f4 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -78,7 +78,7 @@ dlpi = { git = "https://github.com/oxidecomputer/dlpi-sys", branch = "main" } ispf = { git = "https://github.com/oxidecomputer/ispf" } libloading = "0.7" p9ds = { git = "https://github.com/oxidecomputer/p9fs" } -softnpu = { git = "https://github.com/oxidecomputer/softnpu" } +softnpu = { git = "https://github.com/oxidecomputer/softnpu", branch = "zl/multicast" } # Omicron-related internal-dns-resolver = { git = "https://github.com/oxidecomputer/omicron", rev = "5fd53a9c9ff2b0e47bfef3fe842b7516877b0162" } @@ -149,7 +149,7 @@ newtype-uuid = { version = "1.0.1", features = [ "v4" ] } # "feature" for `utsname`, "poll" for `PollFlags` nix = { version = "0.31", features = [ "feature", "poll" ] } owo-colors = "4" -oxide-tokio-rt = "0.1.2" +oxide-tokio-rt = "0.1.4" paste = "1.0.15" pin-project-lite = "0.2.13" proc-macro2 = "1.0" diff --git a/crates/viona-api/src/ffi.rs b/crates/viona-api/src/ffi.rs index e67f97d11..3e1cfc41f 100644 --- a/crates/viona-api/src/ffi.rs +++ b/crates/viona-api/src/ffi.rs @@ -39,6 +39,7 @@ pub const VNA_IOC_GET_MTU: i32 = vna_ioc(0x27); pub const VNA_IOC_SET_MTU: i32 = vna_ioc(0x28); pub const VNA_IOC_SET_NOTIFY_MMIO: i32 = vna_ioc(0x29); pub const VNA_IOC_INTR_POLL_MQ: i32 = vna_ioc(0x2a); +pub const VNA_IOC_SET_MAC_FILTERS: i32 = vna_ioc(0x2b); /// VirtIO 1.2 queue pair support. pub const VNA_IOC_GET_PAIRS: i32 = vna_ioc(0x30); @@ -135,6 +136,49 @@ pub const VIONA_PROMISC_ALL: i32 = 2; #[cfg(feature = "falcon")] pub const VIONA_PROMISC_ALL_VLAN: i32 = 3; +/// Number of bytes in a filterable MAC address. +pub const VIONA_MAC_FILTER_ADDRL: usize = 6; + +/// Maximum number of unicast MAC address filters accepted by viona. +/// +/// No cross-device convention exists for a separate unicast table. QEMU +/// shares one 64-entry table across both address classes (see +/// [`VIONA_MAX_MCAST_FILTERS`]). Half the multicast table is generous for +/// entries that are rejected unless they match the primary MAC. +pub const VIONA_MAX_UNICAST_FILTERS: usize = 32; + +/// Maximum number of multicast MAC address filters accepted by viona. +/// +/// Matches the 64-entry table convention of other virtio-net devices, per +/// QEMU's [`MAC_TABLE_ENTRIES`]. +/// +/// [`MAC_TABLE_ENTRIES`]: https://github.com/qemu/qemu/blob/f893c46c3931b3684d235d221bf8b7844ddbf1d7/include/hw/virtio/virtio-net.h +pub const VIONA_MAX_MCAST_FILTERS: usize = 64; + +/// Complete unicast and multicast MAC filter tables, replacing any tables +/// previously installed on the link. Counts of zero clear all filters. +/// +/// Viona installs only multicast filters on the underlying MAC client. The +/// unicast table exists for shape parity with `VIRTIO_NET_CTRL_MAC_TABLE_SET`, +/// and entries other than the primary MAC of the link are rejected. +#[repr(C)] +pub struct vioc_mac_filters { + pub vmf_nucast: u32, + pub vmf_nmcast: u32, + pub vmf_ucast: [[u8; VIONA_MAC_FILTER_ADDRL]; VIONA_MAX_UNICAST_FILTERS], + pub vmf_mcast: [[u8; VIONA_MAC_FILTER_ADDRL]; VIONA_MAX_MCAST_FILTERS], +} +impl Default for vioc_mac_filters { + fn default() -> Self { + Self { + vmf_nucast: 0, + vmf_nmcast: 0, + vmf_ucast: [[0; VIONA_MAC_FILTER_ADDRL]; VIONA_MAX_UNICAST_FILTERS], + vmf_mcast: [[0; VIONA_MAC_FILTER_ADDRL]; VIONA_MAX_MCAST_FILTERS], + } + } +} + #[repr(C)] #[derive(Default)] pub struct vioc_get_params { @@ -154,7 +198,7 @@ pub struct vioc_set_params { /// This is the viona interface version which viona_api expects to operate /// against. All constants and structs defined by the crate are done so in /// terms of that specific version. -pub const VIONA_CURRENT_INTERFACE_VERSION: u32 = 6; +pub const VIONA_CURRENT_INTERFACE_VERSION: u32 = 7; /// Maximum size of packed nvlists used in viona parameter ioctls pub const VIONA_MAX_PARAM_NVLIST_SZ: usize = 4096; diff --git a/crates/viona-api/src/lib.rs b/crates/viona-api/src/lib.rs index d8535972c..6572aaacf 100644 --- a/crates/viona-api/src/lib.rs +++ b/crates/viona-api/src/lib.rs @@ -197,6 +197,10 @@ fn minor(meta: &std::fs::Metadata) -> u32 { #[repr(u32)] #[derive(Copy, Clone)] pub enum ApiVersion { + /// Adds support for installing MAC address filter tables on the + /// underlying MAC client. + V7 = 7, + /// Adds multi-queue support and change the data structure for per-queue /// interrupt polling to a compact bitmap. V6 = 6, @@ -218,7 +222,7 @@ pub enum ApiVersion { } impl ApiVersion { pub const fn current() -> Self { - Self::V6 + Self::V7 } } impl PartialEq for u32 { diff --git a/lib/propolis/src/hw/virtio/mod.rs b/lib/propolis/src/hw/virtio/mod.rs index 88894c759..1ed96ec8d 100644 --- a/lib/propolis/src/hw/virtio/mod.rs +++ b/lib/propolis/src/hw/virtio/mod.rs @@ -227,6 +227,16 @@ pub trait VirtioDevice: Send + Sync + 'static + Lifecycle { ) -> Result<(), ()> { Ok(()) } + + /// Notification that the virtio portion of the device has been reset. + /// + /// This fires for guest-initiated status resets as well as full device + /// resets, after the generic virtio state and virtqueues have been reset. + /// + /// Devices should use this to restore any state tied to feature + /// negotiation, since the next driver may negotiate a different feature + /// set. + fn device_reset(&self) {} } pub trait VirtioIntr: Send + 'static { diff --git a/lib/propolis/src/hw/virtio/pci.rs b/lib/propolis/src/hw/virtio/pci.rs index 4c17d5c9a..957dbdbb0 100644 --- a/lib/propolis/src/hw/virtio/pci.rs +++ b/lib/propolis/src/hw/virtio/pci.rs @@ -1190,6 +1190,10 @@ impl PciVirtioState { self.reset_queues_locked(dev, &mut state); state.reset(); let _ = self.isr_state.read_clear(); + // Release the state lock before calling into the device so that its + // reset handling may query generic virtio state. + drop(state); + dev.device_reset(); } pub fn reset(&self, dev: &D) diff --git a/lib/propolis/src/hw/virtio/softnpu.rs b/lib/propolis/src/hw/virtio/softnpu.rs index 66546c543..8804f0d68 100644 --- a/lib/propolis/src/hw/virtio/softnpu.rs +++ b/lib/propolis/src/hw/virtio/softnpu.rs @@ -348,27 +348,16 @@ impl PciVirtioSoftNpuPort { let pkt = packet_in::new(&frame[..n]); - let mut pipeline = match self.pipeline.lock() { - Ok(pipe) => pipe, - Err(e) => { - error!(self.log, "failed to lock pipeline: {}", e); - break; - } - }; - let pl: &mut Box = match &mut *pipeline { - Some(ref mut x) => &mut x.1, - None => { - // This just means no P4 program has been set by the guest. - break; - } - }; - - PacketHandler::process_guest_packet( + if PacketHandler::process_guest_packet( pkt, &self.data_handles, - pl, + &self.pipeline, &self.log, - ); + ) + .is_err() + { + break; + } } if push_used { @@ -446,6 +435,10 @@ impl VirtioDevice for PciVirtioSoftNpuPort { } } +/// Error indicating the pipeline cannot process guest packets because no +/// P4 program has been loaded by the guest. +struct NoP4Program; + struct PacketHandler {} impl PacketHandler { @@ -504,21 +497,13 @@ impl PacketHandler { // // process packet with loaded P4 program // - - // TODO pipeline should not need to be mutable for packet handling? let pkt = packet_in::new(&msg[..n]); - let mut p = pipeline.lock().unwrap(); - let pl = match &mut *p { - Some(ref mut pl) => &mut pl.1, - None => continue, // no program is loaded - }; - Self::process_external_packet( index + 1, pkt, &data_handles, &virtio, - pl, + &pipeline, &log, ) } @@ -531,12 +516,26 @@ impl PacketHandler { mut pkt: packet_in<'_>, data_handles: &Vec, virtio: &Arc, - pipeline: &mut Box, + pipeline: &Mutex>, log: &Logger, ) { - for (mut out_pkt, port) in + // We hold the pipeline lock only while the pipeline computes the + // egress set. Every port worker serializes on this lock, and the + // dlpi and virtio egress I/O can block. Holding the lock across + // that I/O stalls the other workers, which is especially costly + // for multicast where one input frame fans out to several ports. + // The egress packets borrow from `pkt` rather than the pipeline, + // so they remain valid after the lock is released. + let outputs = { + let mut guard = pipeline.lock().unwrap(); + let pipeline = match &mut *guard { + Some(ref mut loaded) => &mut loaded.1, + None => return, // no program is loaded + }; pipeline.process_packet(index as u16, &mut pkt) - { + }; + + for (mut out_pkt, port) in outputs { // packet is going to CPU port if port == 0 { Self::send_packet_to_cpu_port(&mut out_pkt, virtio, &log); @@ -555,20 +554,34 @@ impl PacketHandler { /// Run a packet coming into the ASIC from the guest pci port through the /// loaded pipeline and forward it on to its destination. + /// + /// Errors indicate the pipeline cannot process packets, and the caller + /// should stop reading frames. fn process_guest_packet( mut pkt: packet_in<'_>, data_handles: &Vec, - pipeline: &mut Box, + pipeline: &Mutex>, log: &Logger, - ) { - for (mut out_pkt, port) in pipeline.process_packet(0, &mut pkt) { + ) -> std::result::Result<(), NoP4Program> { + // We hold the pipeline lock only for pipeline processing, not the + // egress I/O (see `process_external_packet` for rationale). + let outputs = { + let mut guard = pipeline.lock().unwrap(); + let pipeline = match &mut *guard { + Some(ref mut loaded) => &mut loaded.1, + None => return Err(NoP4Program), + }; + pipeline.process_packet(0, &mut pkt) + }; + + for (mut out_pkt, port) in outputs { if port == 0 { // no looping packets back to the guest - return; + return Ok(()); } if port == SOFTNPU_CPU_AUX_PORT { // we are not currently emulating this port type - return; + return Ok(()); } Self::send_packet_to_ext_port( &mut out_pkt, @@ -577,6 +590,7 @@ impl PacketHandler { &log, ); } + Ok(()) } /// Send a packet out an external port using dlpi. diff --git a/lib/propolis/src/hw/virtio/viona.rs b/lib/propolis/src/hw/virtio/viona.rs index 4f2dc8ea6..42e0e6e53 100644 --- a/lib/propolis/src/hw/virtio/viona.rs +++ b/lib/propolis/src/hw/virtio/viona.rs @@ -192,10 +192,26 @@ pub mod control { Ack = 0, } + /// Parse bound for a driver-provided MAC filter table. + /// + /// This bounds what a driver-controlled entry count can demand of the + /// device before it reaches an allocation. It sits well above viona's + /// filter limits, so a table that saturates it still exceeds those + /// limits and triggers the fallback to multicast promiscuity. + pub(super) const MAC_TABLE_MAX_ENTRIES: u32 = 1024; + /// Read a MAC filter table from a control queue. /// - /// These are encoded as a `u32` followed by that number of - /// 6-byte MAC addresses. + /// These are encoded as a `u32` followed by that number of 6-byte MAC + /// addresses. A table larger than [`MAC_TABLE_MAX_ENTRIES`] is truncated + /// to that bound, with the remaining entries skipped so that a list + /// following this one in the message still parses. The spec permits + /// a device given too many addresses to silently switch to all-multicast + /// or promiscuous mode, and refusing the table instead would leave the + /// previously installed one enforcing stale filters. See "Setting MAC + /// Address Filtering", footnote 10, in the [virtio spec]. + /// + /// [virtio spec]: https://docs.oasis-open.org/virtio/virtio/v1.2/csd01/virtio-v1.2-csd01.html#fn10x5 pub(super) fn read_mac_list( chain: &mut super::Chain, mem: &super::MemCtx, @@ -205,10 +221,35 @@ pub mod control { return Err(()); } - chain - .read_many_owned(mem, entry_count as usize) - .map_err(|_| ()) - .map(Vec::into_boxed_slice) + // Driver behavior varies above the device's table limits: FreeBSD + // vtnet caps its own table at 128 entries and falls back to + // all-multicast itself, while Linux virtio_net sends the full + // multicast list untruncated and relies on the device to degrade. + let read_count = entry_count.min(MAC_TABLE_MAX_ENTRIES); + let table = chain + .read_many_owned::(mem, read_count as usize) + .map_err(|_| ())?; + + let excess = (entry_count - read_count) as usize + * std::mem::size_of::(); + if excess > 0 && !skip_readable(chain, excess) { + // The driver declared more entries than the message carries, + // which is malformed rather than merely oversized. + return Err(()); + } + + Ok(table.into_boxed_slice()) + } + + /// Advance the chain's read position by `len` bytes without copying. + fn skip_readable(chain: &mut super::Chain, len: usize) -> bool { + let mut remain = len; + let skipped = chain.for_remaining_type(true, |_addr, buf_len| { + let consumed = usize::min(buf_len, remain); + remain -= consumed; + (consumed, remain > 0) + }); + skipped == len } } @@ -222,6 +263,13 @@ pub mod migrate { pub filter: u8, pub unicast_mac_filters: Vec, pub multicast_mac_filters: Vec, + /// Whether the source had accepted a `MAC_TABLE_SET` since its last + /// reset. + /// + /// This is optional for compatibility, as older sources omit it and + /// older destinations ignore it. + #[serde(default)] + pub multicast_table_managed: Option, } impl Schema<'_> for VionaStateV1 { @@ -323,6 +371,20 @@ bitflags! { } } +/// The kernel-side Rx configuration required to satisfy a guest's filter +/// state: a promiscuity level, optionally paired with an explicit multicast +/// filter table. +/// +/// `install_filters` records whether a filterable non-empty table should +/// remain installed. It is independent of `promisc`, so guest-requested +/// promiscuity changes do not unplumb and replumb the MAC client's +/// classified flows. +#[derive(Copy, Clone, Eq, PartialEq)] +struct RxConfig { + promisc: PromiscLevel, + install_filters: bool, +} + struct Inner { poller: Option, iop_state: Option, @@ -333,6 +395,20 @@ struct Inner { filter: FilterState, unicast_mac_filters: Box<[MacAddr]>, multicast_mac_filters: Box<[MacAddr]>, + /// Whether (or not) a (possibly partial) multicast filter table is + /// installed on the in-kernel MAC client via `VNA_IOC_SET_MAC_FILTERS`. + mac_filters_installed: bool, + /// Whether (or not) the next installable multicast table must be written + /// because the guest changed it or a previous installation may have been + /// partial. + mac_filters_dirty: bool, + /// Whether (or not) the driver has issued an accepted `MAC_TABLE_SET` since + /// the last reset. + /// + /// An accepted table, including an empty one, demonstrates that the driver + /// manages its own filtering, which releases the all-multicast lower bound + /// in [`PciVirtioViona::rx_config`]. + mac_table_set: bool, } impl Inner { fn new(max_queues: usize, promisc: PromiscLevel) -> Self { @@ -349,6 +425,9 @@ impl Inner { filter: FilterState::empty(), unicast_mac_filters: Box::new([]), multicast_mac_filters: Box::new([]), + mac_filters_installed: false, + mac_filters_dirty: false, + mac_table_set: false, } } @@ -483,9 +562,16 @@ impl From<[u8; ETHERADDRL]> for MacAddr { } impl MacAddr { + /// Returns true if the group bit (IEEE 802 I/G, the least significant + /// bit of the first octet) is clear. pub const fn is_unicast(&self) -> bool { (self.0[0] & 0b1) == 0 } + + /// Returns true for the Ethernet broadcast address. + pub const fn is_broadcast(&self) -> bool { + matches!(self.0, [0xff, 0xff, 0xff, 0xff, 0xff, 0xff]) + } } /// Represents a connection to the kernel's Viona (VirtIO Network Adapter) @@ -498,6 +584,16 @@ pub struct PciVirtioViona { dev_features: u64, mac_addr: MacAddr, mtu: Option, + /// Whether the viona device supports `VNA_IOC_SET_MAC_FILTERS` + /// ([`viona_api::ApiVersion::V7`]). + kernel_mac_filters: bool, + /// Promiscuity pinned at construction, when the host supports ALL_VLAN. + /// Falcon links may carry VLAN-tagged frames for the emulated fabric, + /// which classified delivery would drop, so no guest or migration state + /// may downgrade below the pin. Hosts without ALL_VLAN support leave the + /// link unpinned, with guest-managed filtering as on non-falcon builds. + #[cfg(feature = "falcon")] + pinned_promisc: Option, hdl: VionaHdl, inner: Mutex, } @@ -535,26 +631,37 @@ impl PciVirtioViona { #[cfg(not(feature = "falcon"))] let promisc_level = PromiscLevel::AllMulti; #[cfg(feature = "falcon")] - let promisc_level = match hdl.set_promisc(PromiscLevel::AllVlan) { - Ok(()) => PromiscLevel::AllVlan, - Err(e) => { - // Until/unless this support is integrated into stlouis/illumos, - // this is an expected failure. This is needed to use vlans, - // but shouldn't affect any other use case. - eprintln!( - "failed to enable promisc mode on {vnic_name}: {e:?}" - ); - PromiscLevel::AllMulti - } - }; + let (promisc_level, pinned_promisc) = + match hdl.set_promisc(PromiscLevel::AllVlan) { + Ok(()) => (PromiscLevel::AllVlan, Some(PromiscLevel::AllVlan)), + Err(e) => { + // ALL_VLAN support only exists on the viona_vlans branch + // of illumos-gate, so this failure is expected elsewhere. + // + // Fall back to the stock all-multicast default with + // guest-managed filtering, leaving the link unpinned. + // The fabric traffic that needs more than a guest would + // request (VLAN-tagged sidecar frames) only exists on + // hosts running the ALL_VLAN-capable kernel, and pinning + // here would force every falcon guest into full + // promiscuity permanently. + eprintln!( + "failed to enable ALL_VLAN promisc on {vnic_name}: \ + {e:?}" + ); + (PromiscLevel::AllMulti, None) + } + }; if let Some(vp) = viona_params { vp.set(&hdl)?; } + let api_version = hdl.api_version()?; + // Do in-kernel configuration of device MTU if let Some(mtu) = info.mtu { - if hdl.api_version().unwrap() >= viona_api::ApiVersion::V4 { + if api_version >= viona_api::ApiVersion::V4 { hdl.set_mtu(mtu)?; } else if mtu != 1500 { // Squawk about MTUs not matching the default of 1500 @@ -579,7 +686,7 @@ impl PciVirtioViona { // purpose. let queues = VirtQueues::new_with_len(2, &queue_sizes); let nqueues = queues.max_capacity(); - hdl.set_pairs(1).unwrap(); + hdl.set_pairs(1)?; // Add one for config space. let msix_count = Some(1 + nqueues as u16); @@ -599,6 +706,9 @@ impl PciVirtioViona { dev_features, mac_addr: info.mac_addr.into(), mtu: info.mtu, + kernel_mac_filters: api_version >= viona_api::ApiVersion::V7, + #[cfg(feature = "falcon")] + pinned_promisc, hdl, inner: Mutex::new(Inner::new(nqueues, promisc_level)), }; @@ -725,7 +835,7 @@ impl PciVirtioViona { if nqueues == self.virtio_state.queues.len() { return Ok(()); } - self.hdl.set_usepairs(requested).unwrap(); + self.hdl.set_usepairs(requested).map_err(|_| ())?; self.virtio_state.queues.set_len(nqueues).expect("num queue pairs"); Ok(()) } @@ -952,10 +1062,19 @@ impl PciVirtioViona { let old_filter = state.filter; state.filter.set(filter, active); - match self.set_promisc(self.needed_promisc(&state), &mut state) { + match self.apply_rx_config(&mut state) { Ok(()) => Ok(()), Err(()) => { state.filter = old_filter; + // The failure may have left a partial filter table on the + // MAC client, which would silently drop traffic at + // PromiscLevel::None. Reconcile the kernel with the restored + // state, and force a reset if that fails as well. + let needs_reset = self.apply_rx_config(&mut state).is_err(); + drop(state); + if needs_reset { + self.virtio_state.set_needs_reset(self); + } Err(()) } } @@ -983,12 +1102,26 @@ impl PciVirtioViona { std::mem::swap(&mut unicast, &mut state.unicast_mac_filters); std::mem::swap(&mut multicast, &mut state.multicast_mac_filters); + let prior_table_set = state.mac_table_set; + state.mac_table_set = true; + state.mac_filters_dirty = true; - match self.set_promisc(self.needed_promisc(&state), &mut state) { + match self.apply_rx_config(&mut state) { Ok(()) => Ok(()), Err(()) => { state.unicast_mac_filters = unicast; state.multicast_mac_filters = multicast; + state.mac_table_set = prior_table_set; + // The failed attempt may have installed all or part of the + // new table, so the restored table must be reissued. + state.mac_filters_dirty = true; + // Reconcile the kernel with the restored tables, as in + // set_filter_state above. + let needs_reset = self.apply_rx_config(&mut state).is_err(); + drop(state); + if needs_reset { + self.virtio_state.set_needs_reset(self); + } Err(()) } } @@ -1000,10 +1133,6 @@ impl PciVirtioViona { level: PromiscLevel, state: &mut Inner, ) -> Result<(), ()> { - if level == state.promisc { - return Ok(()); - } - match self.hdl.set_promisc(level) { Ok(_) => { state.promisc = level; @@ -1011,55 +1140,175 @@ impl PciVirtioViona { } Err(_) => { // Ensure that we return to the old level of promisc. - if self.hdl.set_promisc(state.promisc).is_err() { - self.virtio_state.set_needs_reset(self); - } + let _ = self.hdl.set_promisc(state.promisc); Err(()) } } } - /// Compute whether the driver requires us to move into promiscuous mode - /// based on its MAC filters and explicit filter mode. - fn needed_promisc(&self, state: &Inner) -> PromiscLevel { - // The VLAN tag workaround, if requested, always wins and cannot - // be downgraded. + /// The promiscuity level pinned at construction, if any. + /// + /// See the `pinned_promisc` field. This accessor exists so that callers + /// need no `cfg` gates of their own. + fn pinned_promisc(&self) -> Option { #[cfg(feature = "falcon")] - if state.promisc == PromiscLevel::AllVlan { - return PromiscLevel::AllVlan; + { + self.pinned_promisc + } + + #[cfg(not(feature = "falcon"))] + { + None + } + } + + /// Compute the Rx configuration required to satisfy the driver's MAC + /// filters and explicit filter mode. + /// + /// This is computed solely from the guest's filter state and device + /// capabilities, with no side effects. Effecting the result on the + /// in-kernel device is left to [`Self::apply_rx_config`]. + fn rx_config(&self, state: &Inner) -> RxConfig { + // A pinned link ignores the guest's filter state entirely. The pin + // supersets any level the guest could request, so at worst the + // guest receives traffic it did not ask for. + if let Some(pinned) = self.pinned_promisc() { + return RxConfig { promisc: pinned, install_filters: false }; } // We don't yet have any mechanism to account for ALL_UNICAST or // related filters from VIRTIO_NET_F_CTRL_RX_EXTRA, and the guest will // not request them. - // We don't have an ioctl yet for viona to explicitly install a set of - // filters. For now, we need to apply some level of promiscuous mode - // to give the guest what it asks for. + // An explicit multicast table can be installed on the in-kernel MAC + // client (VNA_IOC_SET_MAC_FILTERS) when the kernel supports it + // (viona_api::ApiVersion::V7) and the table fits. Otherwise + // multicast promiscuity covers the request. // - // Even though the guest can't yet use its parent feature, we can still - // account for NO_MULTICAST here. - let need_mcast = (state.filter.contains(FilterState::ALL_MULTICAST) + // NO_MULTICAST is gated behind VIRTIO_NET_F_CTRL_RX_EXTRA, which we + // do not offer, but the derivation accounts for it regardless. + let mcast_filterable = self.kernel_mac_filters + && state.multicast_mac_filters.len() + <= viona_api::VIONA_MAX_MCAST_FILTERS; + let need_mcast_promisc = (state + .filter + .contains(FilterState::ALL_MULTICAST) && !state.filter.contains(FilterState::NO_MULTICAST)) - || !state.multicast_mac_filters.is_empty(); + || (!state.multicast_mac_filters.is_empty() && !mcast_filterable); // Don't inflict promiscuous mode on drivers which request only their // own MAC address. Most guests *should not pass us any unicast // addresses*, as the config-space MAC is assumed to be included by // default. `.all()` will return `true` for such an empty list. This is // defensive programming against an otherwise well-meaning guest. - let filter_is_self = - state.unicast_mac_filters.iter().all(|mac| mac == &self.mac_addr); + // + // A list that saturates the parse bound may have been truncated, so + // it cannot be shown to contain only the device's own MAC and is + // treated as if it were naming foreign addresses. + let filter_is_self = (state.unicast_mac_filters.len() as u32) + < control::MAC_TABLE_MAX_ENTRIES + && state + .unicast_mac_filters + .iter() + .all(|mac| mac == &self.mac_addr); let need_promisc = state.filter.contains(FilterState::PROMISCUOUS) || !filter_is_self; - if need_promisc { + // A guest that has never issued an accepted MAC_TABLE_SET has not + // demonstrated that it manages its own filtering. illumos vioif + // negotiates CTRL_RX and forwards promiscuity toggles over the + // control queue, but its multicast entry point is a noop, so + // narrowing to classified delivery after a promiscuity cycle + // (snoop on/off) would silently drop its multicast traffic. + // + // Unrequested traffic is permitted by the virtio spec, so we hold the + // all-multicast lower bound until the first accepted table (empty or) + // not. + let promisc = if need_promisc { PromiscLevel::All - } else if need_mcast { + } else if need_mcast_promisc || !state.mac_table_set { PromiscLevel::AllMulti } else { PromiscLevel::None + }; + + RxConfig { + promisc, + // Installation tracks the table alone, not the promiscuity + // level, meaning a guest promiscuity cycle does not unplumb and + // replumb the table's classified flows (which quiesces the + // link). Viona keeps the classified callback installed: under + // ALL it drops classified copies, while under MULTI it drops + // classified multicast copies and delivers multicast through + // the promiscuous callback. + // + // An empty table is never installed. Classified delivery with + // no table covers the device MAC and broadcast, which is + // exactly what an empty table requests. + install_filters: mcast_filterable + && !state.multicast_mac_filters.is_empty(), + } + } + + /// Reconcile the in-kernel Rx configuration with the guest's filter + /// state, as computed by [`Self::rx_config`]. + /// + /// Ordering follows the contract documented for + /// `VNA_IOC_SET_MAC_FILTERS`: a filter table is installed before + /// promiscuity is lowered, and promiscuity is raised before a table is + /// cleared, so new arrivals always have a live delivery path across the + /// transition, with in-kernel dedup suppressing the overlap's duplicate + /// copies. Packets already queued for classified delivery may still be + /// dropped under the new mode. + fn apply_rx_config(&self, state: &mut Inner) -> Result<(), ()> { + let target = self.rx_config(state); + + let update_filters = target.install_filters + && (!state.mac_filters_installed || state.mac_filters_dirty); + let promisc = if update_filters { + match self.hdl.set_mac_filters(&state.multicast_mac_filters) { + Ok(()) => { + state.mac_filters_installed = true; + state.mac_filters_dirty = false; + target.promisc + } + // If the table could not be installed, cover the guest's + // multicast request with multicast promiscuity instead. A + // stronger promiscuity request must remain in force. A failed + // install may leave a partial table on the MAC client, so + // record it as installed and dirty to guarantee later + // replacement or clearing. + Err(_) => { + state.mac_filters_installed = true; + state.mac_filters_dirty = true; + match target.promisc { + PromiscLevel::None => PromiscLevel::AllMulti, + covered => covered, + } + } + } + } else { + target.promisc + }; + + self.set_promisc(promisc, state)?; + + // No table is required when it is empty, cannot be installed, or the + // link is pinned. Promiscuity has already been raised where needed, + // so clearing a lingering table is best-effort. + if !target.install_filters { + if state.mac_filters_installed + && self.hdl.set_mac_filters(&[]).is_err() + { + // The failed clear may leave entries behind, so a later + // apply retries it. + state.mac_filters_dirty = true; + } else { + state.mac_filters_installed = false; + state.mac_filters_dirty = false; + } } + Ok(()) } } impl VirtioDevice for PciVirtioViona { @@ -1123,11 +1372,6 @@ impl VirtioDevice for PciVirtioViona { VIRTIO_NO_MQ_CTRL_Q_INDEX }; - if (feat & VIRTIO_NET_F_CTRL_RX) != 0 { - let mut state = self.inner.lock().unwrap(); - self.set_promisc(PromiscLevel::None, &mut state)?; - } - Some(ctl_q_idx.try_into().expect("queue index must be a valid u16")) }; @@ -1213,6 +1457,42 @@ impl VirtioDevice for PciVirtioViona { } Ok(()) } + + fn device_reset(&self) { + // The next driver may not negotiate `VIRTIO_NET_F_CTRL_RX`, so any + // filter state configured by the previous driver must not outlive the + // reset. This runs for guest status resets as well as full device + // resets. + let mut state = self.inner.lock().unwrap(); + state.filter = FilterState::empty(); + state.unicast_mac_filters = Box::new([]); + state.multicast_mac_filters = Box::new([]); + state.mac_table_set = false; + state.mac_filters_dirty = false; + + // Restore the pre-CTRL_RX default of multicast promiscuity (or the + // pinned level) before clearing any installed filter table so that + // no delivery gap opens. + // + // A guest reaches this path with a status write, so a failed restore + // must not panic. The table is left in place to keep covering delivery + // and the device is flagged as needing reset. + let restore = self.pinned_promisc().unwrap_or(PromiscLevel::AllMulti); + if self.set_promisc(restore, &mut state).is_err() { + state.mac_filters_dirty = state.mac_filters_installed; + drop(state); + self.virtio_state.set_needs_reset(self); + return; + } + + if state.mac_filters_installed { + if self.hdl.set_mac_filters(&[]).is_ok() { + state.mac_filters_installed = false; + } else { + state.mac_filters_dirty = true; + } + } + } } impl Lifecycle for PciVirtioViona { fn type_name(&self) -> &'static str { @@ -1224,15 +1504,16 @@ impl Lifecycle for PciVirtioViona { MqSetPairsCause::Reset as u8, 1 )); - self.set_use_pairs(1).expect("can set viona back to one queue pair"); - self.hdl.set_pairs(1).expect("can set viona back to one queue pair"); + // SET_PAIRS can fail while restoring the kernel's receive callbacks, + // and SET_USEPAIRS can fail independently. Attempt both restorations, + // and on failure surface NEEDS_RESET to the guest rather than + // panicking. + let use_pairs = self.set_use_pairs(1); + let pairs = self.hdl.set_pairs(1); + if use_pairs.is_err() || pairs.is_err() { + self.virtio_state.set_needs_reset(self); + } self.virtio_state.queues.reset_peak(); - - let mut state = self.inner.lock().unwrap(); - state.unicast_mac_filters = Box::new([]); - state.multicast_mac_filters = Box::new([]); - self.set_promisc(PromiscLevel::AllMulti, &mut state) - .expect("can reset viona promiscuous state"); } fn start(&self) -> anyhow::Result<()> { self.run(); @@ -1307,6 +1588,8 @@ impl MigrateMulti for PciVirtioViona { ) -> Result<(), MigrateStateError> { ::export(self, output, ctx)?; + // Exported unconditionally, as importers take this payload without + // consulting negotiated features. let viona_state = { let state = self.inner.lock().unwrap(); @@ -1315,6 +1598,7 @@ impl MigrateMulti for PciVirtioViona { filter: state.filter.bits(), unicast_mac_filters: state.unicast_mac_filters.to_vec(), multicast_mac_filters: state.multicast_mac_filters.to_vec(), + multicast_table_managed: Some(state.mac_table_set), } }; @@ -1337,7 +1621,12 @@ impl MigrateMulti for PciVirtioViona { let has_ctl_queue = (feat & VIRTIO_NET_F_CTRL_VQ) != 0; if (feat & VIRTIO_NET_F_MQ) != 0 { - self.hdl.set_pairs(PROPOLIS_MAX_MQ_PAIRS).unwrap(); + self.hdl.set_pairs(PROPOLIS_MAX_MQ_PAIRS).map_err(|e| { + MigrateStateError::ImportFailed(format!( + "error while restoring viona queue pairs \ + ({PROPOLIS_MAX_MQ_PAIRS}): {e:?}" + )) + })?; } // Queue count is a NonZeroU16; hence `get` and -1 will not underflow. let io_queues = @@ -1358,23 +1647,62 @@ impl MigrateMulti for PciVirtioViona { )) })?; - let input: migrate::VionaStateV1 = offer.take()?; + let input: migrate::VionaStateV1 = match offer.take() { + Ok(input) => input, + // Only a source predating this payload sends nothing. Such a + // source never offered CTRL_RX, so the construction defaults + // already match its state. + Err(MigrateStateError::DataMissing) + if (feat & VIRTIO_NET_F_CTRL_RX) == 0 => + { + return Ok(()); + } + Err(e) => return Err(e), + }; + + let migrate::VionaStateV1 { + // The source's promisc level is a host mechanism, derived from + // the guest state below plus the source's own pin and kernel + // capabilities. It is not restored directly: doing so could + // demand a level this kernel does not support (ALL_VLAN on a + // stock host) or hold a level this host's pin does not require. + promisc: _, + filter, + unicast_mac_filters, + multicast_mac_filters, + multicast_table_managed, + } = input; let mut state = self.inner.lock().unwrap(); - state.filter = - FilterState::from_bits(input.filter).ok_or_else(|| { - MigrateStateError::ImportFailed(format!( - "unrecognised flags in filter state: {:x}", - input.filter & !FilterState::all().bits() - )) - })?; - state.unicast_mac_filters = input.unicast_mac_filters.into(); - state.multicast_mac_filters = input.multicast_mac_filters.into(); - self.set_promisc(input.promisc, &mut state).map_err(|_| { + state.filter = FilterState::from_bits(filter).ok_or_else(|| { MigrateStateError::ImportFailed(format!( - "Could not move device promisc level to {:?}.", - input.promisc + "unrecognised flags in filter state: {:x}", + filter & !FilterState::all().bits() )) + })?; + state.unicast_mac_filters = unicast_mac_filters.into(); + state.multicast_mac_filters = multicast_mac_filters.into(); + state.mac_filters_dirty = true; + + // Older sources do not carry whether they had accepted a + // MAC_TABLE_SET. Absent this marker, a non-empty table proves that + // one arrived, while a guest whose accepted table was empty + // conservatively returns to the all-multicast lower bound until its + // next command. + state.mac_table_set = multicast_table_managed.unwrap_or( + !state.unicast_mac_filters.is_empty() + || !state.multicast_mac_filters.is_empty(), + ); + + // Recompute the Rx configuration from the imported guest state. + // + // This installs any needed multicast table on this host or falls + // back to multicast promiscuity if it cannot be installed, and a + // local pin supersedes the result (see rx_config). + self.apply_rx_config(&mut state).map_err(|_| { + MigrateStateError::ImportFailed( + "Could not apply imported viona Rx configuration.".to_string(), + ) }) } } @@ -1674,6 +2002,34 @@ impl VionaHdl { self.0.ioctl_usize(viona_api::VNA_IOC_SET_PROMISC, usize::from(p))?; Ok(()) } + + /// Replace the explicit multicast MAC filter table on the underlying + /// device. An empty table clears any installed filters. + /// + /// Broadcast entries are omitted. The kernel drops them from the table, + /// as a MAC client is joined to broadcast for the lifetime of its unicast + /// address, so passing them would only waste table slots. + /// + /// The device's primary unicast address is always programmed on the + /// in-kernel MAC client, so no unicast entries are passed and `vmf_nucast` + /// remains zero. + /// + /// This requires [viona_api::ApiVersion::V7] or greater. + fn set_mac_filters(&self, multicast: &[MacAddr]) -> io::Result<()> { + assert!(multicast.len() <= viona_api::VIONA_MAX_MCAST_FILTERS); + + let mut vmf = viona_api::vioc_mac_filters::default(); + let mut count = 0; + for mac in multicast.iter().filter(|mac| !mac.is_broadcast()) { + vmf.vmf_mcast[count] = mac.0; + count += 1; + } + vmf.vmf_nmcast = count as u32; + unsafe { + self.0.ioctl(viona_api::VNA_IOC_SET_MAC_FILTERS, &mut vmf)?; + } + Ok(()) + } } impl AsRawFd for VionaHdl { @@ -1913,10 +2269,11 @@ mod test { use crate::hw::pci::Bdf; use crate::hw::virtio::pci::Status; use crate::hw::virtio::viona::{ + control, FilterState, MacAddr, PromiscLevel, VIRTIO_NET_F_CTRL_RX, VIRTIO_NET_F_CTRL_VQ, VIRTIO_NET_F_MAC, VIRTIO_NET_F_MQ, VIRTIO_NET_F_STATUS, }; - use crate::hw::virtio::PciVirtioViona; + use crate::hw::virtio::{PciVirtioViona, VirtioDevice}; use crate::lifecycle::Lifecycle; use crate::migrate::{ MigrateCtx, MigrateMulti, PayloadOffer, PayloadOffers, PayloadOutputs, @@ -2218,6 +2575,18 @@ mod test { struct DriverState { max_pairs: Option, next_queue_gpa: u64, + queue_locs: std::collections::BTreeMap, + } + + /// Guest-physical layout of a virtqueue as programmed by `init_queue`, + /// retained so tests can build descriptor chains on it later. + #[derive(Copy, Clone)] + struct QueueLoc { + desc_gpa: u64, + avail_gpa: u64, + size: u16, + /// The next available-ring index the driver will publish. + avail_idx: u16, } impl DriverState { @@ -2226,6 +2595,7 @@ mod test { max_pairs: None, // Start virtio-nic queues somewhere other than address 0. next_queue_gpa: 2 * MB as u64, + queue_locs: std::collections::BTreeMap::new(), } } } @@ -2357,7 +2727,14 @@ mod test { self.common_config .write_le64(common_cfg::queue_desc, descriptor_table_gpa); - let avail_gpa = descriptor_table_gpa.next_multiple_of(page_u64); + // Give each ring a page of its own by stepping a full page per + // ring. + // + // Aligning with `next_multiple_of` would not work here: + // `next_queue_gpa` is already page-aligned, so it would be a + // noop and leave the descriptor table, available ring, and used + // ring all at the same address. + let avail_gpa = descriptor_table_gpa + page_u64; // First, flags. // > If the VIRTIO_F_EVENT_IDX feature bit is not negotiated: // > * The driver MUST set flags to 0 or 1. @@ -2370,15 +2747,25 @@ mod test { // negotiated VIRTIO_F_EVENT_IDX so no `used_event` for now. self.common_config.write_le64(common_cfg::queue_driver, avail_gpa); - let used_gpa = avail_gpa.next_multiple_of(page_u64); + let used_gpa = avail_gpa + page_u64; self.common_config.write_le64(common_cfg::queue_device, used_gpa); self.common_config.write_le16(common_cfg::queue_enable, 1); - // Finally, round up so the next queue (if there is one) starts - // page-aligned like it should. - self.state.next_queue_gpa = used_gpa.next_multiple_of(page_u64); + // Skip past the used ring so the next allocation (another queue, + // or a control message scratch buffer) does not share its page. + self.state.next_queue_gpa = used_gpa + page_u64; + + self.state.queue_locs.insert( + queue, + QueueLoc { + desc_gpa: descriptor_table_gpa, + avail_gpa, + size: queue_size.min(chosen_size), + avail_idx: 0, + }, + ); let msi_vector = 0x100 + queue; self.common_config @@ -2550,6 +2937,113 @@ mod test { // thinks everything is OK... assert!(self.status_ok()); } + + /// Submit a command on the control queue and return the device's ack + /// byte. + /// + /// The message is laid out as one device-readable descriptor holding + /// the header and payload, followed by one device-writable descriptor + /// for the ack, mirroring how guest drivers structure control commands. + fn send_ctrl_cmd( + &mut self, + class: u8, + command: u8, + payload: &[u8], + ) -> u8 { + const VIRTQ_DESC_F_NEXT: u16 = 1; + const VIRTQ_DESC_F_WRITE: u16 = 2; + + let qidx = self.ctl_qidx().expect("device has a control queue"); + let loc = *self + .state + .queue_locs + .get(&qidx) + .expect("control queue was initialized"); + + let mut msg = vec![class, command]; + msg.extend_from_slice(payload); + + // Reserve scratch pages for the message and ack byte. + // Descriptors and rings are rewritten on every submission, so + // this also works on the fresh (0'ed) guest memory of a + // migration target. + let buf_gpa = self.state.next_queue_gpa; + self.state.next_queue_gpa += + (msg.len() + 1).next_multiple_of(PAGE_SIZE) as u64; + + let acc_mem = + self.machine.acc_mem.access().expect("can access memory"); + assert_eq!( + acc_mem.write_from(GuestAddr(buf_gpa), &msg, msg.len()), + Some(msg.len()) + ); + + let ack_gpa = buf_gpa + msg.len() as u64; + // Seed the ack with a value the device will never write. + assert!(acc_mem.write::(GuestAddr(ack_gpa), &0xaa)); + + // Descriptor 0: header and payload, device-readable. + let d0 = loc.desc_gpa; + assert!(acc_mem.write::(GuestAddr(d0), &buf_gpa)); + assert!( + acc_mem.write::(GuestAddr(d0 + 8), &(msg.len() as u32)) + ); + assert!( + acc_mem.write::(GuestAddr(d0 + 12), &VIRTQ_DESC_F_NEXT) + ); + assert!(acc_mem.write::(GuestAddr(d0 + 14), &1u16)); + + // Descriptor 1: ack byte, device-writable. + let d1 = loc.desc_gpa + 16; + assert!(acc_mem.write::(GuestAddr(d1), &ack_gpa)); + assert!(acc_mem.write::(GuestAddr(d1 + 8), &1u32)); + assert!( + acc_mem.write::(GuestAddr(d1 + 12), &VIRTQ_DESC_F_WRITE) + ); + assert!(acc_mem.write::(GuestAddr(d1 + 14), &0u16)); + + // Publish the chain head in the available ring and bump the index. + let slot = u64::from(loc.avail_idx % loc.size); + assert!(acc_mem + .write::(GuestAddr(loc.avail_gpa + 4 + 2 * slot), &0u16)); + let new_idx = loc.avail_idx.wrapping_add(1); + assert!( + acc_mem.write::(GuestAddr(loc.avail_gpa + 2), &new_idx) + ); + self.state.queue_locs.get_mut(&qidx).unwrap().avail_idx = new_idx; + + let vq = self + .dev + .virtio_state + .queues + .get(qidx) + .expect("control queue exists") + .clone(); + self.dev.queue_notify(&vq); + + *acc_mem.read::(GuestAddr(ack_gpa)).expect("can read ack byte") + } + + fn ctrl_rx_cmd(&mut self, cmd: control::RxCmd, enable: bool) -> u8 { + self.send_ctrl_cmd(0, cmd as u8, &[u8::from(enable)]) + } + + fn ctrl_mac_table_set( + &mut self, + unicast: &[MacAddr], + multicast: &[MacAddr], + ) -> u8 { + let mut payload = Vec::new(); + payload.extend_from_slice(&(unicast.len() as u32).to_le_bytes()); + for mac in unicast { + payload.extend_from_slice(&mac.0); + } + payload.extend_from_slice(&(multicast.len() as u32).to_le_bytes()); + for mac in multicast { + payload.extend_from_slice(&mac.0); + } + self.send_ctrl_cmd(1, control::MacCmd::TableSet as u8, &payload) + } } fn test_device_status_writes(test_ctx: TestCtx) -> TestCtx { @@ -2629,7 +3123,7 @@ mod test { // try using it or setting any interesting features. // First, we have a fresh device on a fresh VM. The test is playing the - // role of the first use of the device by OVMF, an intiial bootloader, + // role of the first use of the device by OVMF, an initial bootloader, // or maybe the actual guest OS. let mut driver = test_ctx.create_driver(); driver.modern_device_init(expected_feats); @@ -2734,7 +3228,7 @@ mod test { let mut driver = test_ctx.create_driver(); // `basic_operation_multiqueue()` talks about why it's an interesting - // test to shink max pairs. + // test to shrink max pairs. driver.set_max_pairs(Some(4)); driver.modern_device_init(expected_feats | VIRTIO_NET_F_MQ); @@ -2789,7 +3283,7 @@ mod test { driver.modern_device_init(expected_feats | VIRTIO_NET_F_MQ); let mut driver = test_ctx.create_driver(); // `basic_operation_multiqueue()` talks about why it's an interesting - // test to shink max pairs. + // test to shrink max pairs. driver.set_max_pairs(Some(4)); driver.modern_device_init(expected_feats | VIRTIO_NET_F_MQ); @@ -2814,6 +3308,372 @@ mod test { test_ctx } + /// Drive the `VIRTIO_NET_F_CTRL_RX` command set, including the explicit MAC + /// filter tables, class-wide filter flags, and the promiscuity fallbacks + /// they demand from the device. + fn control_rx_filtering(test_ctx: TestCtx) -> TestCtx { + if test_ctx.dev.pinned_promisc().is_some() { + // A link pinned for the emulated fabric ignores guest filter + // state, which makes guest-driven Rx filtering a noop. + return test_ctx; + } + + let mut driver = test_ctx.create_driver(); + driver.modern_device_init( + VIRTIO_NET_F_MAC + | VIRTIO_NET_F_STATUS + | VIRTIO_NET_F_CTRL_VQ + | VIRTIO_NET_F_CTRL_RX, + ); + + let kernel_filters = test_ctx.dev.kernel_mac_filters; + let self_mac = test_ctx.dev.mac_addr; + + // Negotiating CTRL_RX alone leaves the device at its all-multicast + // default. Filtering only engages once the driver issues an + // accepted MAC_TABLE_SET, protecting drivers that negotiate the + // feature but never manage a filter table (illumos vioif among + // them). + { + let state = test_ctx.dev.inner.lock().unwrap(); + assert_eq!(state.promisc, PromiscLevel::AllMulti); + assert!(!state.mac_filters_installed); + assert!(!state.mac_table_set); + } + + // An accepted table releases the bound even when empty: the driver + // has demonstrated that it manages filtering, and an empty table + // requests only the device MAC and broadcast. No kernel table is + // needed for that, so this holds on pre-V7 kernels as well. + let ack = driver.ctrl_mac_table_set(&[], &[]); + assert_eq!(ack, control::Ack::Ok as u8); + { + let state = test_ctx.dev.inner.lock().unwrap(); + assert_eq!(state.promisc, PromiscLevel::None); + assert!(!state.mac_filters_installed); + assert!(state.mac_table_set); + } + + // A table of our own MAC + a small multicast set narrows the + // device to classified delivery. V7 kernels take the explicit + // table, older kernels are covered by multicast promiscuity. + let mcast = [ + MacAddr([0x33, 0x33, 0, 0, 0, 1]), + MacAddr([0x33, 0x33, 0, 0, 0, 2]), + ]; + let ack = driver.ctrl_mac_table_set(&[self_mac], &mcast); + + assert_eq!(ack, control::Ack::Ok as u8); + { + let state = test_ctx.dev.inner.lock().unwrap(); + assert_eq!(&*state.multicast_mac_filters, &mcast[..]); + if kernel_filters { + assert_eq!(state.promisc, PromiscLevel::None); + assert!(state.mac_filters_installed); + } else { + assert_eq!(state.promisc, PromiscLevel::AllMulti); + assert!(!state.mac_filters_installed); + } + } + + // A broadcast entry is kept in the guest-visible table but omitted + // from the kernel filter set, as the MAC client is already joined + // to broadcast. Installation must still succeed. + let with_bcast = [ + MacAddr([0x33, 0x33, 0, 0, 0, 1]), + MacAddr([0xff, 0xff, 0xff, 0xff, 0xff, 0xff]), + ]; + let ack = driver.ctrl_mac_table_set(&[self_mac], &with_bcast); + assert_eq!(ack, control::Ack::Ok as u8); + { + let state = test_ctx.dev.inner.lock().unwrap(); + assert_eq!(&*state.multicast_mac_filters, &with_bcast[..]); + if kernel_filters { + assert_eq!(state.promisc, PromiscLevel::None); + assert!(state.mac_filters_installed); + } else { + assert_eq!(state.promisc, PromiscLevel::AllMulti); + assert!(!state.mac_filters_installed); + } + } + + // Guest requested promiscuity changes leave an installed table in + // place, avoiding needless MAC flow plumbing and link quiescing. + // Dropping each flag returns directly to table-backed delivery. + let filtered_level = if kernel_filters { + PromiscLevel::None + } else { + PromiscLevel::AllMulti + }; + for (cmd, level) in [ + (control::RxCmd::AllMulticast, PromiscLevel::AllMulti), + (control::RxCmd::Promisc, PromiscLevel::All), + ] { + let ack = driver.ctrl_rx_cmd(cmd, true); + assert_eq!(ack, control::Ack::Ok as u8); + { + let state = test_ctx.dev.inner.lock().unwrap(); + assert_eq!(state.promisc, level); + assert_eq!(state.mac_filters_installed, kernel_filters); + } + + let ack = driver.ctrl_rx_cmd(cmd, false); + assert_eq!(ack, control::Ack::Ok as u8); + { + let state = test_ctx.dev.inner.lock().unwrap(); + assert_eq!(state.promisc, filtered_level); + assert_eq!(state.mac_filters_installed, kernel_filters); + } + } + + // A multicast table larger than viona's filter maximum must fall + // back to multicast promiscuity rather than losing traffic. The + // limit is judged on the guest's table before broadcast omission, + // so a broadcast entry does not bring an over-limit table back + // under the maximum. + let mut big: Vec = (0..viona_api::VIONA_MAX_MCAST_FILTERS) + .map(|i| MacAddr([0x33, 0x33, 0, 0, 0, i as u8])) + .collect(); + big.push(MacAddr([0xff, 0xff, 0xff, 0xff, 0xff, 0xff])); + let ack = driver.ctrl_mac_table_set(&[], &big); + assert_eq!(ack, control::Ack::Ok as u8); + { + let state = test_ctx.dev.inner.lock().unwrap(); + assert_eq!(state.promisc, PromiscLevel::AllMulti); + assert!(!state.mac_filters_installed); + assert!(state.mac_table_set); + } + + // An entry count above the parse bound is not refused. The table is + // truncated to the bound, the remainder of the body is skipped, and + // the oversized result falls back to multicast promiscuity. + let huge_mc: Vec = (0..=control::MAC_TABLE_MAX_ENTRIES) + .map(|i| MacAddr([0x33, 0x33, 0, 0, (i >> 8) as u8, i as u8])) + .collect(); + let ack = driver.ctrl_mac_table_set(&[], &huge_mc); + assert_eq!(ack, control::Ack::Ok as u8); + { + let state = test_ctx.dev.inner.lock().unwrap(); + assert_eq!(state.promisc, PromiscLevel::AllMulti); + assert!(!state.mac_filters_installed); + assert_eq!( + state.multicast_mac_filters.len(), + control::MAC_TABLE_MAX_ENTRIES as usize + ); + } + + // Skipping an oversized unicast list leaves the read position at the + // multicast list that follows it, which must still parse. A unicast + // list that saturates the parse bound may have been truncated, so it + // cannot be shown to contain only the device's own MAC and forces + // full promiscuity even though every entry sent matches it. + let huge_uc = + vec![self_mac; control::MAC_TABLE_MAX_ENTRIES as usize + 1]; + let ack = driver.ctrl_mac_table_set(&huge_uc, &mcast); + assert_eq!(ack, control::Ack::Ok as u8); + { + let state = test_ctx.dev.inner.lock().unwrap(); + assert_eq!(state.promisc, PromiscLevel::All); + assert_eq!(&*state.multicast_mac_filters, &mcast[..]); + // The fitting multicast table stays installed under full + // promiscuity, where it has no delivery effect. + assert_eq!(state.mac_filters_installed, kernel_filters); + } + + // A count declared without a matching body is malformed and is still + // rejected, leaving the previous tables untouched. + let mut payload = 0u32.to_le_bytes().to_vec(); + payload.extend_from_slice(&2048u32.to_le_bytes()); + let ack = + driver.send_ctrl_cmd(1, control::MacCmd::TableSet as u8, &payload); + assert_eq!(ack, control::Ack::Err as u8); + { + let state = test_ctx.dev.inner.lock().unwrap(); + assert_eq!(&*state.multicast_mac_filters, &mcast[..]); + } + + // A unicast entry other than the device's own MAC cannot be + // classified and forces full promiscuity. + let foreign = MacAddr([0x02, 0, 0, 0, 0, 0x77]); + let ack = driver.ctrl_mac_table_set(&[foreign], &[]); + assert_eq!(ack, control::Ack::Ok as u8); + { + let state = test_ctx.dev.inner.lock().unwrap(); + assert_eq!(state.promisc, PromiscLevel::All); + // The now-empty multicast table is cleared from the kernel + // even while promiscuity holds. + assert!(!state.mac_filters_installed); + } + + // Clearing both tables narrows delivery back to classified: the + // accepted table update demonstrates that the guest manages + // filtering, and an empty table requests only the device MAC and + // broadcast. + let ack = driver.ctrl_mac_table_set(&[], &[]); + assert_eq!(ack, control::Ack::Ok as u8); + assert_eq!( + test_ctx.dev.inner.lock().unwrap().promisc, + PromiscLevel::None + ); + + // A malformed enable byte is rejected. + let ack = driver.send_ctrl_cmd(0, control::RxCmd::Promisc as u8, &[2]); + assert_eq!(ack, control::Ack::Err as u8); + + // CTRL_RX_EXTRA commands are not advertised and must be rejected. + let ack = driver.ctrl_rx_cmd(control::RxCmd::NoMulticast, true); + assert_eq!(ack, control::Ack::Err as u8); + + test_ctx + } + + /// A guest-initiated status reset must not leak the previous driver's + /// filter state to the next driver, which may not negotiate CTRL_RX at + /// all. + fn control_rx_reset_clears_filters(test_ctx: TestCtx) -> TestCtx { + if test_ctx.dev.pinned_promisc().is_some() { + // Pinned link, see `control_rx_filtering`. + return test_ctx; + } + + let mut driver = test_ctx.create_driver(); + driver.modern_device_init( + VIRTIO_NET_F_MAC + | VIRTIO_NET_F_STATUS + | VIRTIO_NET_F_CTRL_VQ + | VIRTIO_NET_F_CTRL_RX, + ); + + let self_mac = test_ctx.dev.mac_addr; + let mcast = [MacAddr([0x01, 0x00, 0x5e, 0, 0, 0xfb])]; + let ack = driver.ctrl_mac_table_set(&[self_mac], &mcast); + assert_eq!(ack, control::Ack::Ok as u8); + assert!(!test_ctx + .dev + .inner + .lock() + .unwrap() + .multicast_mac_filters + .is_empty()); + + // Reset via device status, as a guest reboot would. All filter state + // must revert to the pre-negotiation default of multicast + // promiscuity. + driver.write_status(Status::RESET); + { + let state = test_ctx.dev.inner.lock().unwrap(); + assert!(state.filter.is_empty()); + assert!(state.unicast_mac_filters.is_empty()); + assert!(state.multicast_mac_filters.is_empty()); + assert_eq!(state.promisc, PromiscLevel::AllMulti); + assert!(!state.mac_filters_installed); + assert!(!state.mac_table_set); + } + + // The next driver comes up without CTRL_RX and keeps that default. + let mut driver = test_ctx.create_driver(); + driver.modern_device_init( + VIRTIO_NET_F_MAC | VIRTIO_NET_F_STATUS | VIRTIO_NET_F_CTRL_VQ, + ); + assert_eq!( + test_ctx.dev.inner.lock().unwrap().promisc, + PromiscLevel::AllMulti + ); + + test_ctx + } + + /// Filter state must survive migration: the guest-visible state (filter + /// flags and MAC tables) is carried in the payload and the target + /// re-derives the kernel configuration from it. + fn control_rx_migration(test_ctx: TestCtx) -> TestCtx { + if test_ctx.dev.pinned_promisc().is_some() { + // Pinned link, see `control_rx_filtering`. + return test_ctx; + } + + let mut driver = test_ctx.create_driver(); + driver.modern_device_init( + VIRTIO_NET_F_MAC + | VIRTIO_NET_F_STATUS + | VIRTIO_NET_F_CTRL_VQ + | VIRTIO_NET_F_CTRL_RX, + ); + + let self_mac = test_ctx.dev.mac_addr; + let mcast = [ + MacAddr([0x33, 0x33, 0, 0, 0, 3]), + MacAddr([0x33, 0x33, 0, 0, 0, 4]), + ]; + let ack = driver.ctrl_mac_table_set(&[self_mac], &mcast); + assert_eq!(ack, control::Ack::Ok as u8); + let ack = driver.ctrl_rx_cmd(control::RxCmd::AllMulticast, true); + assert_eq!(ack, control::Ack::Ok as u8); + + let drv_state = driver.export(); + let test_ctx = test_ctx.migrate(); + let mut driver = VirtioNetDriver::import( + &test_ctx.machine, + &test_ctx.dev, + drv_state, + ); + assert!(driver.status_ok()); + { + let state = test_ctx.dev.inner.lock().unwrap(); + assert!(state.filter.contains(FilterState::ALL_MULTICAST)); + assert_eq!(&*state.multicast_mac_filters, &mcast[..]); + assert_eq!(state.promisc, PromiscLevel::AllMulti); + assert!(state.mac_table_set); + } + + // The imported state must be live, not just carried data. Dropping + // ALL_MULTICAST on the target must re-derive the table-backed + // config. + let kernel_filters = test_ctx.dev.kernel_mac_filters; + let ack = driver.ctrl_rx_cmd(control::RxCmd::AllMulticast, false); + assert_eq!(ack, control::Ack::Ok as u8); + { + let state = test_ctx.dev.inner.lock().unwrap(); + if kernel_filters { + assert_eq!(state.promisc, PromiscLevel::None); + assert!(state.mac_filters_installed); + } else { + assert_eq!(state.promisc, PromiscLevel::AllMulti); + assert!(!state.mac_filters_installed); + } + } + + // An accepted empty table narrows delivery without leaving any + // evidence within the tables themselves, so only the payload marker + // distinguishes this guest from one that never issued a + // MAC_TABLE_SET. + let ack = driver.ctrl_mac_table_set(&[], &[]); + assert_eq!(ack, control::Ack::Ok as u8); + assert_eq!( + test_ctx.dev.inner.lock().unwrap().promisc, + PromiscLevel::None + ); + + let drv_state = driver.export(); + let test_ctx = test_ctx.migrate(); + let driver = VirtioNetDriver::import( + &test_ctx.machine, + &test_ctx.dev, + drv_state, + ); + assert!(driver.status_ok()); + { + let state = test_ctx.dev.inner.lock().unwrap(); + assert!(state.unicast_mac_filters.is_empty()); + assert!(state.multicast_mac_filters.is_empty()); + assert!(state.mac_table_set); + assert_eq!(state.promisc, PromiscLevel::None); + assert!(!state.mac_filters_installed); + } + + test_ctx + } + // Bears an uncanny resemblance to `phd-test`... struct TestCase { name: &'static str, @@ -2846,8 +3706,8 @@ mod test { } // We'll actually create and destroy some vnics so not only do we need - // `dladm`, we need a recent enough viona and everything.. this test is only - // meaningful on an illumos host:; + // `dladm`, we need a recent enough viona and everything. This test is only + // meaningful on an illumos host. #[test] #[cfg_attr(not(target_os = "illumos"), ignore)] fn run_viona_tests() { @@ -2869,6 +3729,9 @@ mod test { testcase!(multiqueue_to_singlequeue_to_multiqueue), testcase!(multiqueue_migration), testcase!(multiqueue_migration_after_boot), + testcase!(control_rx_filtering), + testcase!(control_rx_reset_clears_filters), + testcase!(control_rx_migration), ]; let underlying_nic = match std::env::var("VIONA_TEST_NIC") { diff --git a/tools/check_headers b/tools/check_headers index ee3cbca6d..316441866 100755 --- a/tools/check_headers +++ b/tools/check_headers @@ -8,7 +8,15 @@ set -e # if your changes to Propolis track changes in the OS as well. # # As a default this ref should probably not change. -HEADER_CHECK_REF="stlouis" +# +# This is temporarily pinned to the viona MAC filter table change (stlouis#986, +# https://code.oxide.computer/c/illumos-gate/+/775), which adds the +# VNA_IOC_SET_MAC_FILTERS ioctl and struct vioc_mac_filters checked by the +# viona-api header-check. +# +# Restore to "stlouis" once that change integrates. Note: this ref names a +# specific patchset and must be bumped if a new patchset is uploaded. +HEADER_CHECK_REF="refs/changes/75/775/4" # Directories with `ctest2`-based `header-check` crates. This list should track # the similar exclusions in `Cargo.toml`, and are described more there.