From 1e88b1e6a9b72a90dc46ade790b063e899bfec96 Mon Sep 17 00:00:00 2001 From: Stuart McLaren Date: Tue, 19 May 2026 10:01:16 +0000 Subject: [PATCH] teardown: clean up host resources when netns is gone Change the NetworkDriver::teardown() signature to accept an Option for the container netns netlink socket. When the container network namespace is no longer available (e.g. the process exited), drivers can still clean up host-side resources such as firewall rules and bridge interfaces. In teardown.rs, failure now falls back to opening only the host netlink socket and passes None for the container socket. Each driver handles the None case: - bridge: skips veth/route deletion, still removes firewall rules, sysctl files, and empty bridges - vlan/macvlan: returns Ok(()) since all state is netns-side Note: netns error handling: for "does not exist" no error is raised, other (potentially unrecoverable) errors are raised. Note: plugin is still invoked when netns does not exist. Note: there may be a general principle here: teardown should continue rather than error if something it should remove is already gone. Fixes: #1451 Signed-off-by: Stuart McLaren --- src/commands/setup.rs | 4 +- src/commands/teardown.rs | 24 ++++- src/network/bridge.rs | 178 ++++++++++++++++++++-------------- src/network/core_utils.rs | 28 ++++++ src/network/driver.rs | 4 +- src/network/plugin.rs | 2 +- src/network/vlan.rs | 17 +++- test/250-bridge-nftables.bats | 55 +++++++++++ test/500-plugin.bats | 9 ++ 9 files changed, 234 insertions(+), 87 deletions(-) diff --git a/src/commands/setup.rs b/src/commands/setup.rs index 33125e6d2..532ffbfb6 100644 --- a/src/commands/setup.rs +++ b/src/commands/setup.rs @@ -86,7 +86,7 @@ impl Setup { container_hostname: &network_options.container_hostname, container_dns_servers: &network_options.dns_servers, netns_host: hostns.file.as_fd(), - netns_container: netns.file.as_fd(), + netns_container: Some(netns.file.as_fd()), netns_path: &self.network_namespace_path, network, per_network_opts, @@ -175,7 +175,7 @@ fn teardown_drivers<'a, I>( I: Iterator>, { for driver in drivers { - if let Err(e) = driver.teardown((host, netns)) { + if let Err(e) = driver.teardown((host, Some(netns))) { error!( "failed to cleanup network {} after setup failed: {}", driver.network_name(), diff --git a/src/commands/teardown.rs b/src/commands/teardown.rs index 65fc9da7d..67caeff8c 100644 --- a/src/commands/teardown.rs +++ b/src/commands/teardown.rs @@ -92,8 +92,24 @@ impl Teardown { let firewall_driver = firewall::get_supported_firewall_driver(firewall_driver)?; - let (mut hostns, mut netns) = - core_utils::open_netlink_sockets(&self.network_namespace_path)?; + let mut hostns = core_utils::open_host_netlink_socket()?; + let (netns_file, mut netns_netlink) = + match core_utils::open_netns_netlink_socket(&self.network_namespace_path, &hostns) { + Ok(netns) => (Some(netns.file), Some(netns.netlink)), + Err(e) => { + match e.unwrap() { + NetavarkError::Io(io_err) + if io_err.kind() == std::io::ErrorKind::NotFound => + { + log::warn!("container netns not found: {e}"); + } + _ => { + error_list.push(e); + } + } + (None, None) + } + }; for named_network_opts in &network_options.networks { let per_network_opts = &named_network_opts.opts; @@ -116,7 +132,7 @@ impl Teardown { container_hostname: &network_options.container_hostname, container_dns_servers: &network_options.dns_servers, netns_host: hostns.file.as_fd(), - netns_container: netns.file.as_fd(), + netns_container: netns_file.as_ref().map(|f| f.as_fd()), // None when no netns netns_path: &self.network_namespace_path, network, per_network_opts, @@ -134,7 +150,7 @@ impl Teardown { } }; - match driver.teardown((&mut hostns.netlink, &mut netns.netlink)) { + match driver.teardown((&mut hostns.netlink, netns_netlink.as_mut())) { Ok(_) => {} Err(err) => { error_list.push(err); diff --git a/src/network/bridge.rs b/src/network/bridge.rs index d7b083858..336c9ecc7 100644 --- a/src/network/bridge.rs +++ b/src/network/bridge.rs @@ -214,7 +214,9 @@ impl driver::NetworkDriver for Bridge<'_> { self.info.network.internal, self.info.rootless, self.info.netns_host, - self.info.netns_container, + self.info + .netns_container + .ok_or_else(|| NetavarkError::msg("netns_container required for setup"))?, )?; // StatusBlock response @@ -374,33 +376,116 @@ impl driver::NetworkDriver for Bridge<'_> { fn teardown( &self, - netlink_sockets: (&mut Socket, &mut Socket), + netlink_sockets: (&mut Socket, Option<&mut Socket>), ) -> NetavarkResult<()> { - let mode: Option = parse_option(&self.info.network.options, OPTION_MODE)?; - let mode = get_bridge_mode_from_string(mode.as_deref())?; let (host_sock, netns_sock) = netlink_sockets; let mut error_list = NetavarkErrorList::new(); - dhcp_teardown(&self.info, netns_sock)?; + if let Some(netns_sock) = netns_sock { + self.netns_teardown(netns_sock, &mut error_list); + } + self.host_teardown(host_sock, &mut error_list); - let routes = core_utils::create_route_list(&self.info.network.routes)?; - for route in routes.iter() { - netns_sock - .del_route(route) - .unwrap_or_else(|err| error_list.push(err)) + if !error_list.is_empty() { + return Err(NetavarkError::List(error_list)); } - let bridge_name = get_interface_name(self.info.network.network_interface.clone())?; + Ok(()) + } +} - let complete_teardown = match remove_link( - host_sock, - netns_sock, - mode, - &bridge_name, - &self.info.per_network_opts.interface_name, - ) { - Ok(teardown) => teardown, +fn get_interface_name(name: Option) -> NetavarkResult { + let name = match name { + None => return Err(NetavarkError::msg(NO_BRIDGE_NAME_ERROR)), + Some(n) => { + if n.is_empty() { + return Err(NetavarkError::msg(NO_BRIDGE_NAME_ERROR)); + } + n + } + }; + Ok(name) +} + +impl<'a> Bridge<'a> { + /// Remove container-side resources: DHCP lease, routes, and veth. + fn netns_teardown(&self, netns: &mut Socket, error_list: &mut NetavarkErrorList) { + if let Err(err) = dhcp_teardown(&self.info, netns) { + error_list.push(err); + return; + } + + match core_utils::create_route_list(&self.info.network.routes) { + Ok(routes) => { + for route in routes.iter() { + netns + .del_route(route) + .unwrap_or_else(|err| error_list.push(err)) + } + } + Err(err) => error_list.push(err), + } + + netns + .del_link(LinkID::Name( + self.info.per_network_opts.interface_name.to_string(), + )) + .wrap(format!( + "failed to delete container veth {}", + self.info.per_network_opts.interface_name + )) + .unwrap_or_else(|err| error_list.push(err)); + } + + /// Remove host resources: bridge, sysctl config, and firewall rules. + fn host_teardown(&self, host: &mut Socket, error_list: &mut NetavarkErrorList) { + let mode = match parse_option(&self.info.network.options, OPTION_MODE) + .and_then(|m: Option| get_bridge_mode_from_string(m.as_deref())) + { + Ok(mode) => mode, + Err(err) => { + error_list.push(err); + return; + } + }; + let bridge_name = match get_interface_name(self.info.network.network_interface.clone()) { + Ok(name) => name, + Err(err) => { + error_list.push(err); + return; + } + }; + + let complete_teardown = match host + .get_link(LinkID::Name(bridge_name.to_string())) + .wrap("failed to get bridge interface") + { + Ok(br) => { + match host + .dump_links(&mut vec![LinkAttribute::Controller(br.header.index)]) + .wrap("failed to get connected bridge interfaces") + { + Ok(links) if links.is_empty() && matches!(mode, BridgeMode::Managed) => { + log::info!("removing bridge {bridge_name}"); + match host + .del_link(LinkID::ID(br.header.index)) + .wrap(format!("failed to delete bridge {bridge_name}")) + { + Ok(_) => true, + Err(err) => { + error_list.push(err); + false + } + } + } + Ok(_) => false, + Err(err) => { + error_list.push(err); + false + } + } + } Err(err) => { error_list.push(err); false @@ -418,7 +503,7 @@ impl driver::NetworkDriver for Bridge<'_> { e.into(), )); } - }; + } } match self.teardown_firewall(complete_teardown, bridge_name) { @@ -428,29 +513,8 @@ impl driver::NetworkDriver for Bridge<'_> { } } } - - if !error_list.is_empty() { - return Err(NetavarkError::List(error_list)); - } - - Ok(()) } -} - -fn get_interface_name(name: Option) -> NetavarkResult { - let name = match name { - None => return Err(NetavarkError::msg(NO_BRIDGE_NAME_ERROR)), - Some(n) => { - if n.is_empty() { - return Err(NetavarkError::msg(NO_BRIDGE_NAME_ERROR)); - } - n - } - }; - Ok(name) -} -impl<'a> Bridge<'a> { fn get_firewall_conf( &'a self, container_addresses: &Vec, @@ -1114,38 +1178,6 @@ fn validate_vrf_link(msg: LinkMessage, vrf_name: &str) -> NetavarkResult<(u32, O ))) } -fn remove_link( - host: &mut Socket, - netns: &mut Socket, - mode: BridgeMode, - br_name: &str, - container_veth_name: &str, -) -> NetavarkResult { - netns - .del_link(LinkID::Name(container_veth_name.to_string())) - .wrap(format!( - "failed to delete container veth {container_veth_name}" - ))?; - - let br = host - .get_link(LinkID::Name(br_name.to_string())) - .wrap("failed to get bridge interface")?; - - let links = host - .dump_links(&mut vec![LinkAttribute::Controller(br.header.index)]) - .wrap("failed to get connected bridge interfaces")?; - // no connected interfaces on that bridge we can remove it - if links.is_empty() { - if let BridgeMode::Managed = mode { - log::info!("removing bridge {br_name}"); - host.del_link(LinkID::ID(br.header.index)) - .wrap(format!("failed to delete bridge {container_veth_name}"))?; - return Ok(true); - } - } - Ok(false) -} - fn get_isolate_option(opts: &Option>) -> NetavarkResult { let isolate: String = match parse_option(opts, OPTION_ISOLATE)? { Some(i) => i, diff --git a/src/network/core_utils.rs b/src/network/core_utils.rs index 52727120a..e1141f84d 100644 --- a/src/network/core_utils.rs +++ b/src/network/core_utils.rs @@ -347,6 +347,34 @@ pub fn open_netlink_sockets( )) } +/// Open the host netlink socket. Used during teardown when the +/// container network namespace is no longer available. +pub fn open_host_netlink_socket() -> NetavarkResult { + let hostns = open_netlink_socket("/proc/self/ns/net").wrap("open host netns")?; + let host_socket = netlink::Socket::::new().wrap("host netlink socket")?; + Ok(NamespaceOptions { + file: hostns, + netlink: host_socket, + }) +} + +/// Open the container netns netlink socket, given an already-opened host ns. +pub fn open_netns_netlink_socket( + netns_path: &str, + hostns: &NamespaceOptions, +) -> NetavarkResult { + let netns = open_netlink_socket(netns_path).wrap("open container netns")?; + let netns_sock = exec_netns!( + hostns.file.as_fd(), + netns.as_fd(), + netlink::Socket::::new().wrap("netns netlink socket") + )?; + Ok(NamespaceOptions { + file: netns, + netlink: netns_sock, + }) +} + fn open_netlink_socket(netns_path: &str) -> NetavarkResult { wrap!(File::open(netns_path), format!("open {netns_path}")) } diff --git a/src/network/driver.rs b/src/network/driver.rs index 119da0c56..7116fc7fb 100644 --- a/src/network/driver.rs +++ b/src/network/driver.rs @@ -23,7 +23,7 @@ pub struct DriverInfo<'a> { pub container_name: &'a String, pub container_dns_servers: &'a Option>, pub netns_host: BorrowedFd<'a>, - pub netns_container: BorrowedFd<'a>, + pub netns_container: Option>, pub netns_path: &'a str, pub network: &'a Network, pub per_network_opts: &'a PerNetworkOptions, @@ -45,7 +45,7 @@ pub trait NetworkDriver { /// teardown the network interfaces/firewall rules for this driver fn teardown( &self, - netlink_sockets: (&mut Socket, &mut Socket), + netlink_sockets: (&mut Socket, Option<&mut Socket>), ) -> NetavarkResult<()>; /// return the network name diff --git a/src/network/plugin.rs b/src/network/plugin.rs index 596338d1a..18d2546bf 100644 --- a/src/network/plugin.rs +++ b/src/network/plugin.rs @@ -118,7 +118,7 @@ impl NetworkDriver for PluginDriver<'_> { fn teardown( &self, - _netlink_sockets: (&mut Socket, &mut Socket), + _netlink_sockets: (&mut Socket, Option<&mut Socket>), ) -> NetavarkResult<()> { self.exec_plugin(false, self.info.netns_path).wrap(format!( "plugin {:?} failed", diff --git a/src/network/vlan.rs b/src/network/vlan.rs index 2c0da2e88..961f8daee 100644 --- a/src/network/vlan.rs +++ b/src/network/vlan.rs @@ -166,7 +166,9 @@ impl driver::NetworkDriver for Vlan<'_> { &self.info.per_network_opts.interface_name, data, self.info.netns_host, - self.info.netns_container, + self.info + .netns_container + .expect("netns_container required for setup"), &data.kind, )?; @@ -218,16 +220,21 @@ impl driver::NetworkDriver for Vlan<'_> { fn teardown( &self, - netlink_sockets: (&mut Socket, &mut Socket), + netlink_sockets: (&mut Socket, Option<&mut Socket>), ) -> NetavarkResult<()> { - dhcp_teardown(&self.info, netlink_sockets.1)?; + let netns_sock = match netlink_sockets.1 { + Some(s) => s, + None => return Ok(()), + }; + + dhcp_teardown(&self.info, netns_sock)?; let routes = core_utils::create_route_list(&self.info.network.routes)?; for route in routes.iter() { - netlink_sockets.1.del_route(route)?; + netns_sock.del_route(route)?; } - netlink_sockets.1.del_link(LinkID::Name( + netns_sock.del_link(LinkID::Name( self.info.per_network_opts.interface_name.to_string(), ))?; Ok(()) diff --git a/test/250-bridge-nftables.bats b/test/250-bridge-nftables.bats index 5216afa85..79e4d8abf 100644 --- a/test/250-bridge-nftables.bats +++ b/test/250-bridge-nftables.bats @@ -1206,6 +1206,61 @@ net/ipv4/conf/podman1/rp_filter = 2" run_netavark --file ${TESTSDIR}/testfiles/simplebridge.json teardown $(get_container_netns_path) } +# Regression test for https://github.com/containers/netavark/issues/1451 +@test "$fw_driver - bridge teardown with missing netns cleans up firewall" { + run_netavark --file ${TESTSDIR}/testfiles/simplebridge.json setup $(get_container_netns_path) + + # Verify firewall rules were created + check_simple_bridge_nftables + + # Destroy the container netns to simulate the real scenario: + # when the netns is gone, the kernel destroys the veth pair, leaving + # only host-side state (firewall rules, bridge with no interfaces). + kill -9 "${CONTAINER_NS_PIDS[0]}" + wait "${CONTAINER_NS_PIDS[0]}" 2>/dev/null || true + unset 'CONTAINER_NS_PIDS[0]' + sleep 1 + + # Kernel destroyed the veth pair when the netns died - bridge has no + # connected interfaces now (this is why teardown can't rely on the veth). + run_in_host_netns ip link show master podman0 + assert "${#lines[@]}" = 0 "bridge should have no connected interfaces" + + # Verify firewall rules still exist after netns destruction (they would + # leak without this fix) + run_in_host_netns nft list chain inet netavark nv_53ce4390_10_88_0_0_nm16 + run_in_host_netns nft list chain inet netavark FORWARD + assert "${#lines[@]}" = 9 "FORWARD rules should still exist before teardown" + run_in_host_netns nft list chain inet netavark POSTROUTING + assert "${#lines[@]}" = 7 "POSTROUTING rules should still exist before teardown" + run_in_host_netns ip link show podman0 + + # The netns path is now invalid. Teardown still cleans up host-side + # rules (missing netns is not an error, the kernel already cleaned up). + RUST_LOG=netavark=warn run_netavark --file ${TESTSDIR}/testfiles/simplebridge.json \ + teardown "/proc/does-not-exist/ns/net" + assert "$output" =~ "container netns not found" "missing netns logged as warning" + + # Verify nftables rules are gone + # The per-network chain should no longer exist + expected_rc=1 run_in_host_netns nft list chain inet netavark nv_53ce4390_10_88_0_0_nm16 + + # Bridge should be removed (it was the only container) + expected_rc=1 run_in_host_netns ip addr show podman0 + + # FORWARD rules should be back to baseline + run_in_host_netns nft list chain inet netavark FORWARD + assert "${#lines[@]}" = 7 "too many FORWARD rules after teardown with missing netns" + + # POSTROUTING rules should be back to baseline + run_in_host_netns nft list chain inet netavark POSTROUTING + assert "${#lines[@]}" = 6 "too many POSTROUTING rules after teardown with missing netns" + + # Isolation chain should no longer reference podman0 + run_in_host_netns nft list chain inet netavark NETAVARK-ISOLATION-1 + assert "$output" "!~" "podman0" "isolation chain should not reference podman0" +} + function check_simple_bridge_nftables() { # check nftables POSTROUTING chain run_in_host_netns nft list chain inet netavark POSTROUTING diff --git a/test/500-plugin.bats b/test/500-plugin.bats index 39b7f1411..87ba030cb 100644 --- a/test/500-plugin.bats +++ b/test/500-plugin.bats @@ -73,3 +73,12 @@ function run_netavark_plugins() { run_netavark_plugins teardown $(get_container_netns_path) <<<"$config" assert 'stderr teardown' "stderr log" } + +@test "plugin - teardown with missing netns still calls plugin" { + config=$(get_conf stderr-plugin) + + run_netavark_plugins setup $(get_container_netns_path) <<<"$config" + + run_netavark_plugins teardown "/proc/does-not-exist/ns/net" <<<"$config" + assert "$output" =~ 'stderr teardown' "plugin teardown was called" +}