Skip to content

Fix ownership bug in OPTE port management that can lead to deadlock #1695

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Sep 9, 2022
Merged

Fix ownership bug in OPTE port management that can lead to deadlock #1695

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 11 additions & 5 deletions sled-agent/src/instance.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -517,14 +517,14 @@ impl Instance {
} else {
(None, None)
};
let port = inner.port_manager.create_port(
let (port, port_ticket) = inner.port_manager.create_port(
*inner.id(),
nic,
snat,
external_ips,
)?;
port_tickets.push(port.ticket());
opte_ports.push(port);
port_tickets.push(port_ticket);
}

// Create a zone for the propolis instance, using the previously
Expand Down Expand Up @@ -711,13 +711,19 @@ impl Instance {
running_state.instance_ticket.terminate();

// And remove the OPTE ports from the port manager
let mut result = Ok(());
if let Some(tickets) = running_state.port_tickets.as_mut() {
for ticket in tickets.iter_mut() {
ticket.release()?;
// Release the port from the manager, and store any error. We
// don't return immediately so that we can try to clean up all
// ports, even if early ones fail. Return the last error, which
// is OK for now.
if let Err(e) = ticket.release() {
result = Err(e.into());
}
}
}

Ok(())
result
}

// Monitors propolis until explicitly told to disconnect.
Expand Down
17 changes: 2 additions & 15 deletions sled-agent/src/opte/illumos/port.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
use crate::illumos::dladm::Dladm;
use crate::opte::BoundaryServices;
use crate::opte::Gateway;
use crate::opte::PortTicket;
use crate::opte::Vni;
use crate::params::SourceNatConfig;
use ipnetwork::IpNetwork;
Expand All @@ -18,8 +17,6 @@ use std::sync::Arc;

#[derive(Debug)]
struct PortInner {
// Contains instance ID and a pointer to the parent manager
ticket: PortTicket,
// Name of the port as identified by OPTE
name: String,
// IP address within the VPC Subnet
Expand All @@ -36,7 +33,7 @@ struct PortInner {
_underlay_ip: Ipv6Addr,
// The external IP address and port range provided for this port, to allow
// outbound network connectivity.
source_nat: Option<SourceNatConfig>,
_source_nat: Option<SourceNatConfig>,
// The external IP addresses provided to this port, to allow _inbound_
// network connectivity.
external_ips: Option<Vec<IpAddr>>,
Expand Down Expand Up @@ -105,7 +102,6 @@ pub struct Port {
impl Port {
#[allow(clippy::too_many_arguments)]
pub fn new(
ticket: PortTicket,
name: String,
ip: IpAddr,
subnet: IpNetwork,
Expand All @@ -121,15 +117,14 @@ impl Port {
) -> Self {
Self {
inner: Arc::new(PortInner {
ticket,
name,
_ip: ip,
_subnet: subnet,
mac,
slot,
_vni: vni,
_underlay_ip: underlay_ip,
source_nat,
_source_nat: source_nat,
external_ips,
_gateway: gateway,
_boundary_services: boundary_services,
Expand All @@ -138,10 +133,6 @@ impl Port {
}
}

pub fn source_nat(&self) -> &Option<SourceNatConfig> {
&self.inner.source_nat
}

pub fn external_ips(&self) -> &Option<Vec<IpAddr>> {
&self.inner.external_ips
}
Expand All @@ -157,8 +148,4 @@ impl Port {
pub fn slot(&self) -> u8 {
self.inner.slot
}

pub fn ticket(&self) -> PortTicket {
self.inner.ticket.clone()
}
}
27 changes: 12 additions & 15 deletions sled-agent/src/opte/illumos/port_manager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,7 @@ impl PortManager {
nic: &NetworkInterface,
source_nat: Option<SourceNatConfig>,
external_ips: Option<Vec<IpAddr>>,
) -> Result<Port, Error> {
) -> Result<(Port, PortTicket), Error> {
// TODO-completess: Remove IPv4 restrictions once OPTE supports virtual
// IPv6 networks.
let private_ip = match nic.ip {
Expand Down Expand Up @@ -335,15 +335,14 @@ impl PortManager {
vnic_name
};

let port = {
let (port, ticket) = {
let mut ports = self.inner.ports.lock().unwrap();
let ticket = PortTicket::new(
instance_id,
port_name.clone(),
self.inner.clone(),
);
let port = Port::new(
ticket,
port_name.clone(),
nic.ip,
subnet,
Expand All @@ -366,7 +365,7 @@ impl PortManager {
&port_name,
);
self.inner.update_secondary_macs(&mut ports)?;
port
(port, ticket)
};

// Add a router entry for this interface's subnet, directing traffic to the
Expand Down Expand Up @@ -433,11 +432,10 @@ impl PortManager {
"Created OPTE port for guest";
"port" => ?&port,
);
Ok(port)
Ok((port, ticket))
}
}

#[derive(Clone)]
pub struct PortTicket {
id: Uuid,
port_name: String,
Expand All @@ -446,15 +444,14 @@ pub struct PortTicket {

impl std::fmt::Debug for PortTicket {
fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
const SOME: &str = "Some(_)";
const NONE: &str = "None";
f.debug_struct("PortTicket")
.field("id", &self.id)
.field(
"manager",
if self.manager.is_some() { &SOME } else { &NONE },
)
.finish()
if self.manager.is_some() {
f.debug_struct("PortTicket")
.field("id", &self.id)
.field("manager", &"{ .. }")
.finish()
} else {
f.debug_struct("PortTicket").field("id", &self.id).finish()
}
}
}

Expand Down
13 changes: 0 additions & 13 deletions sled-agent/src/opte/non_illumos/port.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@

use crate::opte::BoundaryServices;
use crate::opte::Gateway;
use crate::opte::PortTicket;
use crate::opte::Vni;
use crate::params::SourceNatConfig;
use ipnetwork::IpNetwork;
Expand All @@ -18,8 +17,6 @@ use std::sync::Arc;
#[derive(Debug)]
#[allow(dead_code)]
struct PortInner {
// Contains instance ID and a pointer to the parent manager
ticket: PortTicket,
// Name of the port as identified by OPTE
name: String,
// IP address within the VPC Subnet
Expand Down Expand Up @@ -70,7 +67,6 @@ pub struct Port {
impl Port {
#[allow(clippy::too_many_arguments)]
pub fn new(
ticket: PortTicket,
name: String,
ip: IpAddr,
subnet: IpNetwork,
Expand All @@ -86,7 +82,6 @@ impl Port {
) -> Self {
Self {
inner: Arc::new(PortInner {
ticket,
name,
_ip: ip,
_subnet: subnet,
Expand All @@ -103,10 +98,6 @@ impl Port {
}
}

pub fn source_nat(&self) -> &Option<SourceNatConfig> {
&self.inner.source_nat
}

pub fn external_ips(&self) -> &Option<Vec<IpAddr>> {
&self.inner.external_ips
}
Expand All @@ -122,8 +113,4 @@ impl Port {
pub fn slot(&self) -> u8 {
self.inner.slot
}

pub fn ticket(&self) -> PortTicket {
self.inner.ticket.clone()
}
}
27 changes: 12 additions & 15 deletions sled-agent/src/opte/non_illumos/port_manager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ impl PortManager {
nic: &NetworkInterface,
source_nat: Option<SourceNatConfig>,
external_ips: Option<Vec<IpAddr>>,
) -> Result<Port, Error> {
) -> Result<(Port, PortTicket), Error> {
// TODO-completess: Remove IPv4 restrictions once OPTE supports virtual
// IPv6 networks.
let _ = match nic.ip {
Expand Down Expand Up @@ -142,15 +142,14 @@ impl PortManager {
let boundary_services = BoundaryServices::default();
let port_name = self.inner.next_port_name();
let vnic = format!("v{}", port_name);
let port = {
let (port, ticket) = {
let mut ports = self.inner.ports.lock().unwrap();
let ticket = PortTicket::new(
instance_id,
port_name.clone(),
self.inner.clone(),
);
let port = Port::new(
ticket,
port_name.clone(),
nic.ip,
subnet,
Expand All @@ -172,19 +171,18 @@ impl PortManager {
instance_id,
&port_name,
);
port
(port, ticket)
};

info!(
self.inner.log,
"Created OPTE port for guest";
"port" => ?&port,
);
Ok(port)
Ok((port, ticket))
}
}

#[derive(Clone)]
pub struct PortTicket {
id: Uuid,
port_name: String,
Expand All @@ -193,15 +191,14 @@ pub struct PortTicket {

impl std::fmt::Debug for PortTicket {
fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
const SOME: &str = "Some(_)";
const NONE: &str = "None";
f.debug_struct("PortTicket")
.field("id", &self.id)
.field(
"manager",
if self.manager.is_some() { &SOME } else { &NONE },
)
.finish()
if self.manager.is_some() {
f.debug_struct("PortTicket")
.field("id", &self.id)
.field("manager", &"{ .. }")
.finish()
} else {
f.debug_struct("PortTicket").field("id", &self.id).finish()
}
}
}

Expand Down