Skip to content

Commit

Permalink
Fix bug in partially unlocked pool setup
Browse files Browse the repository at this point in the history 
This resolves a bug where a panic could occur in a partially unlocked
pool if it is started after failing the first time.
  • Loading branch information
@jbaublitz
jbaublitz committed Jul 18, 2023
1 parent df8584c commit 71c60bf
Show file tree
Hide file tree
Showing 4 changed files with 26 additions and 124 deletions.
4 changes: 1 addition & 3 deletions src/engine/strat_engine/backstore/crypt/shared.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ use crate::{
STRATIS_TOKEN_ID, STRATIS_TOKEN_POOLNAME_KEY, STRATIS_TOKEN_POOL_UUID_KEY,
STRATIS_TOKEN_TYPE, TOKEN_KEYSLOTS_KEY, TOKEN_TYPE_KEY,
},
handle::CryptHandle,
handle::{CryptHandle, CryptMetadata},
},
devices::get_devno_from_path,
},
Expand All @@ -62,8 +62,6 @@ use crate::{
stratis::{StratisError, StratisResult},
};

use super::handle::CryptMetadata;

/// Set up crypt logging to log cryptsetup debug information at the trace level.
pub fn set_up_crypt_logging() {
fn logging_callback(level: CryptLogLevel, msg: &str, _: Option<&mut ()>) {
Expand Down
135 changes: 18 additions & 117 deletions src/engine/strat_engine/liminal/liminal.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,7 @@ use crate::{
engine::{DumpState, Pool, StateDiff},
strat_engine::{
backstore::{find_stratis_devs_by_uuid, CryptHandle, StratBlockDev},
dm::{
has_leftover_devices, list_of_crypt_devices, remove_optional_devices,
stop_partially_constructed_pool,
},
dm::{has_leftover_devices, stop_partially_constructed_pool},
liminal::{
device_info::{
reconstruct_stratis_infos, split_stratis_infos, stratis_infos_ref, DeviceSet,
Expand Down Expand Up @@ -92,13 +89,10 @@ impl LiminalDevices {
pools: &Table<PoolUuid, StratPool>,
pool_uuid: PoolUuid,
unlock_method: UnlockMethod,
) -> StratisResult<Vec<(DevUuid, CryptHandle)>> {
fn handle_luks(
luks_info: &LLuksInfo,
unlock_method: UnlockMethod,
) -> StratisResult<CryptHandle> {
if let Some(h) = CryptHandle::setup(&luks_info.dev_info.devnode, Some(unlock_method))? {
Ok(h)
) -> StratisResult<Vec<DevUuid>> {
fn handle_luks(luks_info: &LLuksInfo, unlock_method: UnlockMethod) -> StratisResult<()> {
if CryptHandle::setup(&luks_info.dev_info.devnode, Some(unlock_method))?.is_some() {
Ok(())
} else {
Err(StratisError::Msg(format!(
"Block device {} does not appear to be formatted with
Expand Down Expand Up @@ -135,16 +129,8 @@ impl LiminalDevices {
match info {
LInfo::Stratis(_) => (),
LInfo::Luks(ref luks_info) => match handle_luks(luks_info, unlock_method) {
Ok(handle) => unlocked.push((*dev_uuid, handle)),
Err(e) => {
return Err(handle_unlock_rollback(
e,
unlocked
.into_iter()
.map(|(dev_uuid, _)| dev_uuid)
.collect::<Vec<_>>(),
));
}
Ok(()) => unlocked.push(*dev_uuid),
Err(e) => return Err(e),
},
}
}
Expand Down Expand Up @@ -216,24 +202,13 @@ impl LiminalDevices {
(Err(e), _) => return Err(e),
};

let (uuids, handles) = unlocked_devices.into_iter().fold(
(Vec::new(), HashMap::new()),
|(mut uuids, mut handles), (uuid, handle)| {
uuids.push(uuid);
handles.insert(uuid, handle);
(uuids, handles)
},
);
let uuids = unlocked_devices.into_iter().collect::<Vec<_>>();

let mut stopped_pool = self
.stopped_pools
.remove(&pool_uuid)
.or_else(|| self.partially_constructed_pools.remove(&pool_uuid))
.expect("Checked above");
let all_uuids = stopped_pool
.iter()
.map(|(dev_uuid, _)| *dev_uuid)
.collect::<Vec<_>>();
match find_stratis_devs_by_uuid(pool_uuid, &uuids) {
Ok(infos) => infos.into_iter().for_each(|(dev_uuid, (path, devno))| {
if let Ok(Ok(Some(bda))) = bda_wrapper(&path) {
Expand All @@ -256,15 +231,12 @@ impl LiminalDevices {
}),
Err(e) => {
warn!("Failed to scan for newly unlocked Stratis devices: {}", e);
let err = handle_unlock_rollback(e, all_uuids);
return Err(err);
return Err(e);
}
};

match self.try_setup_pool(pools, pool_uuid, stopped_pool, handles) {
Ok((name, pool)) => Ok((name, pool_uuid, pool, uuids)),
Err(e) => Err(handle_unlock_rollback(e, all_uuids)),
}
self.try_setup_pool(pools, pool_uuid, stopped_pool)
.map(|(name, pool)| (name, pool_uuid, pool, uuids))
}

/// Stop a pool, tear down the devicemapper devices, and store the pool information
Expand Down Expand Up @@ -554,15 +526,13 @@ impl LiminalDevices {
pools: &Table<PoolUuid, StratPool>,
pool_uuid: PoolUuid,
device_set: DeviceSet,
handles: HashMap<DevUuid, CryptHandle>,
) -> StratisResult<(Name, StratPool)> {
fn try_setup_pool_failure(
pools: &Table<PoolUuid, StratPool>,
pool_uuid: PoolUuid,
luks_info: StratisResult<(Option<PoolEncryptionInfo>, MaybeInconsistent<Option<Name>>)>,
infos: &HashMap<DevUuid, LStratisDevInfo>,
bdas: HashMap<DevUuid, BDA>,
handles: HashMap<DevUuid, CryptHandle>,
meta_res: StratisResult<(DateTime<Utc>, PoolSave)>,
) -> BDARecordResult<(Name, StratPool)> {
let (timestamp, metadata) = match meta_res {
Expand All @@ -571,7 +541,7 @@ impl LiminalDevices {
};

setup_pool(
pools, pool_uuid, luks_info, infos, bdas, handles, timestamp, metadata,
pools, pool_uuid, luks_info, infos, bdas, timestamp, metadata,
)
}

Expand All @@ -595,7 +565,7 @@ impl LiminalDevices {

let res = load_stratis_metadata(pool_uuid, stratis_infos_ref(&infos));
let (infos, bdas) = split_stratis_infos(infos);
match try_setup_pool_failure(pools, pool_uuid, luks_info, &infos, bdas, handles, res) {
match try_setup_pool_failure(pools, pool_uuid, luks_info, &infos, bdas, res) {
Ok((name, pool)) => {
self.uuid_lookup = self
.uuid_lookup
Expand Down Expand Up @@ -652,22 +622,8 @@ impl LiminalDevices {
Err(e) => return Err((e, bdas)),
};
if let Some(true) | None = metadata.started {
let mut handles = HashMap::default();
for (dev_uuid, info) in infos {
if let Some((dev_uuid, handle)) = match info.luks.as_ref() {
Some(l) => match CryptHandle::setup(&l.dev_info.devnode, None) {
Ok(Some(handle)) => Some((dev_uuid, handle)),
Ok(None) => None,
Err(e) => return Err((e, bdas)),
},
None => None,
} {
handles.insert(*dev_uuid, handle);
}
}

setup_pool(
pools, pool_uuid, luks_info, infos, bdas, handles, timestamp, metadata,
pools, pool_uuid, luks_info, infos, bdas, timestamp, metadata,
)
.map(Either::Left)
} else {
Expand Down Expand Up @@ -824,6 +780,7 @@ impl LiminalDevices {
let mut devices = self
.stopped_pools
.remove(&pool_uuid)
.or_else(|| self.partially_constructed_pools.remove(&pool_uuid))
.unwrap_or_else(DeviceSet::new);

self.uuid_lookup
Expand Down Expand Up @@ -914,44 +871,8 @@ impl LiminalDevices {
.map(|(dev_uuid, _)| *dev_uuid)
.collect::<Vec<_>>();
if has_leftover_devices(pool_uuid, &device_uuids) {
let dev_uuids = device_set
.iter()
.map(|(dev_uuid, _)| *dev_uuid)
.collect::<Vec<_>>();
let res = stop_partially_constructed_pool(pool_uuid, &dev_uuids);
let device_set = device_set
.into_iter()
.map(|(dev_uuid, info)| {
(
dev_uuid,
match info {
LInfo::Luks(l) => LInfo::Luks(l),
LInfo::Stratis(mut s) => {
if let Some(l) = s.luks {
if !s.dev_info.devnode.exists() {
LInfo::Luks(l)
} else {
s.luks = Some(l);
LInfo::Stratis(s)
}
} else {
LInfo::Stratis(s)
}
}
},
)
})
.collect::<DeviceSet>();
match res {
Ok(_) => {
assert!(!has_leftover_devices(pool_uuid, &device_uuids));
self.stopped_pools.insert(pool_uuid, device_set);
}
Err(_) => {
self.partially_constructed_pools
.insert(pool_uuid, device_set);
}
}
self.partially_constructed_pools
.insert(pool_uuid, device_set);
} else {
self.stopped_pools.insert(pool_uuid, device_set);
}
Expand Down Expand Up @@ -1056,14 +977,12 @@ fn load_stratis_metadata(
///
/// If there is a name conflict between the set of devices in devices
/// and some existing pool, return an error.
#[allow(clippy::too_many_arguments)]
fn setup_pool(
pools: &Table<PoolUuid, StratPool>,
pool_uuid: PoolUuid,
luks_info: StratisResult<(Option<PoolEncryptionInfo>, MaybeInconsistent<Option<Name>>)>,
infos: &HashMap<DevUuid, LStratisDevInfo>,
bdas: HashMap<DevUuid, BDA>,
handles: HashMap<DevUuid, CryptHandle>,
timestamp: DateTime<Utc>,
metadata: PoolSave,
) -> BDARecordResult<(Name, StratPool)> {
Expand All @@ -1077,7 +996,7 @@ fn setup_pool(
)), bdas));
}

let (datadevs, cachedevs) = match get_blockdevs(&metadata.backstore, infos, bdas, handles) {
let (datadevs, cachedevs) = match get_blockdevs(&metadata.backstore, infos, bdas) {
Err((err, bdas)) => return Err(
(StratisError::Chained(
format!(
Expand Down Expand Up @@ -1148,21 +1067,3 @@ fn setup_pool(
), bdas)
})
}

/// Rollback an unlock operation for some or all devices of a pool that have been
/// unlocked prior to the failure occurring.
fn handle_unlock_rollback(causal_error: StratisError, uuids: Vec<DevUuid>) -> StratisError {
let devices = list_of_crypt_devices(&uuids);
if let Err(e) = remove_optional_devices(devices) {
warn!("Failed to roll back encrypted pool unlock; some previously locked encrypted devices may be left in an unlocked state");
return StratisError::NoActionRollbackError {
causal_error: Box::new(causal_error),
rollback_error: Box::new(StratisError::Chained(
"Failed to roll back encrypted pool unlock; some previously locked encrypted devices may be left in an unlocked state".to_string(),
Box::new(e),
)),
};
}

causal_error
}
7 changes: 4 additions & 3 deletions src/engine/strat_engine/liminal/setup.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,6 @@ pub fn get_blockdevs(
backstore_save: &BackstoreSave,
infos: &HashMap<DevUuid, LStratisDevInfo>,
mut bdas: HashMap<DevUuid, BDA>,
mut handles: HashMap<DevUuid, CryptHandle>,
) -> BDARecordResult<(Vec<StratBlockDev>, Vec<StratBlockDev>)> {
let recorded_data_map: HashMap<DevUuid, (usize, &BaseBlockDevSave)> = backstore_save
.data_tier
Expand Down Expand Up @@ -185,7 +184,6 @@ pub fn get_blockdevs(
fn get_blockdev(
info: &LStratisDevInfo,
bda: BDA,
handle: Option<CryptHandle>,
data_map: &HashMap<DevUuid, (usize, &BaseBlockDevSave)>,
cache_map: &HashMap<DevUuid, (usize, &BaseBlockDevSave)>,
segment_table: &HashMap<DevUuid, Vec<(Sectors, Sectors)>>,
Expand Down Expand Up @@ -249,6 +247,10 @@ pub fn get_blockdevs(
Some(luks) => &luks.dev_info.devnode,
None => &info.dev_info.devnode,
};
let handle = match CryptHandle::setup(physical_path, None) {
Ok(h) => h,
Err(e) => return Err((e, bda)),
};
let underlying_device = match handle {
Some(handle) => UnderlyingDevice::Encrypted(handle),
None => UnderlyingDevice::Unencrypted(match DevicePath::new(physical_path) {
Expand All @@ -275,7 +277,6 @@ pub fn get_blockdevs(
match get_blockdev(
infos.get(dev_uuid).expect("bdas.keys() == infos.keys()"),
bdas.remove(dev_uuid).expect("bdas.keys() == infos.keys()"),
handles.remove(dev_uuid),
&recorded_data_map,
&recorded_cache_map,
&segment_table,
Expand Down
4 changes: 3 additions & 1 deletion tests/client-dbus/tests/udev/test_udev.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -526,7 +526,9 @@ def test_duplicate_pool_name(
if exit_code == StratisdErrors.OK and changed:
blockdevs = blockdevs_tmp

wait_for_udev_count(len(blockdevs) + len(non_luks_tokens))
wait_for_udev_count(
len(blockdevs) + len(non_luks_tokens) + len(luks_tokens)
)

# The number of pools should never exceed one, since all the pools
# previously formed in the test have the same name.
Expand Down

0 comments on commit 71c60bf

Please sign in to comment.