Refactor decryption to use a read lock during operation

jbaublitz · jbaublitz · commit 48961b2777e2 · 2025-08-26T10:05:00.000-04:00
diff --git a/src/dbus_api/pool/pool_3_9/methods.rs b/src/dbus_api/pool/pool_3_9/methods.rs
@@ -13,7 +13,7 @@ use crate::{
         types::{DbusErrorEnum, EncryptionInfos, TData, OK_STRING},
         util::{engine_to_dbus_err_tuple, get_next_arg, tuple_to_option},
     },
-    engine::{CreateAction, InputEncryptionInfo, KeyDescription},
+    engine::{CreateAction, DeleteAction, InputEncryptionInfo, KeyDescription},
     stratis::StratisError,
 };
 
@@ -90,7 +90,8 @@ pub fn encrypt_pool(m: &MethodInfo<'_, MTSync<TData>, TData>) -> MethodResult {
 
     // Check if operation is idempotent and complete all operations before handling result
     let create_result = handle_action!(
-        read_guard.encrypt_pool_idem_check()
+        read_guard
+            .encrypt_pool_idem_check()
             .and_then(|action| match action {
                 CreateAction::Identity => Ok(action),
                 CreateAction::Created(_) => {
@@ -117,9 +118,11 @@ pub fn encrypt_pool(m: &MethodInfo<'_, MTSync<TData>, TData>) -> MethodResult {
     );
 
     let msg = match create_result {
-        Ok(CreateAction::Identity) => {
-            return_message.append3(default_return, DbusErrorEnum::OK as u16, OK_STRING.to_string())
-        }
+        Ok(CreateAction::Identity) => return_message.append3(
+            default_return,
+            DbusErrorEnum::OK as u16,
+            OK_STRING.to_string(),
+        ),
         Ok(CreateAction::Created(_)) => {
             let guard = get_pool!(dbus_context.engine; pool_uuid; default_return; return_message);
             let encryption_info = match guard.encryption_info().clone() {
@@ -218,22 +221,40 @@ pub fn decrypt_pool(m: &MethodInfo<'_, MTSync<TData>, TData>) -> MethodResult {
         return_message
     );
 
-    let mut guard = get_mut_pool!(dbus_context.engine; pool_uuid; default_return; return_message);
-    let (name, uuid, pool) = guard.as_mut_tuple();
+    let guard = get_pool!(dbus_context.engine; pool_uuid; default_return; return_message);
 
-    let result = handle_action!(
-        pool.decrypt_pool(&name, uuid),
+    let result = match handle_action!(
+        guard.decrypt_pool_idem_check(),
         dbus_context,
         pool_path.get_name()
-    );
+    ) {
+        Ok(DeleteAction::Identity) => Ok(DeleteAction::Identity),
+        Ok(DeleteAction::Deleted(d)) => {
+            match guard.do_decrypt_pool(pool_uuid).and_then(|_| {
+                let mut guard = block_on(dbus_context.engine.upgrade_pool(guard));
+                let (name, _, _) = guard.as_mut_tuple();
+                guard.finish_decrypt_pool(pool_uuid, &name)
+            }) {
+                Ok(_) => Ok(DeleteAction::Deleted(d)),
+                Err(e) => Err(e),
+            }
+        }
+        Err(e) => Err(e),
+    };
     let msg = match result {
-        Ok(_) => {
+        Ok(DeleteAction::Deleted(_)) => {
+            let guard = get_pool!(dbus_context.engine; pool_uuid; default_return; return_message);
             dbus_context.push_pool_key_desc_change(pool_path.get_name(), None);
             dbus_context.push_pool_clevis_info_change(pool_path.get_name(), None);
             dbus_context.push_pool_encryption_status_change(pool_path.get_name(), false);
-            dbus_context.push_pool_last_reencrypt_timestamp(object_path, pool.last_reencrypt());
+            dbus_context.push_pool_last_reencrypt_timestamp(object_path, guard.last_reencrypt());
             return_message.append3(true, DbusErrorEnum::OK as u16, OK_STRING.to_string())
         }
+        Ok(DeleteAction::Identity) => return_message.append3(
+            default_return,
+            DbusErrorEnum::OK as u16,
+            OK_STRING.to_string(),
+        ),
         Err(err) => {
             let (rc, rs) = engine_to_dbus_err_tuple(&err);
             return_message.append3(default_return, rc, rs)
diff --git a/src/engine/engine.rs b/src/engine/engine.rs
@@ -433,12 +433,14 @@ pub trait Pool: Debug + Send + Sync {
     /// Finish reencryption of an encrypted pool.
     fn finish_reencrypt_pool(&mut self, name: &Name) -> StratisResult<ReencryptedDevice>;
 
+    /// Check idempotence of a pool decrypt command.
+    fn decrypt_pool_idem_check(&self) -> StratisResult<DeleteAction<EncryptedDevice>>;
+
     /// Decrypt an encrypted pool.
-    fn decrypt_pool(
-        &mut self,
-        name: &Name,
-        pool_uuid: PoolUuid,
-    ) -> StratisResult<DeleteAction<EncryptedDevice>>;
+    fn do_decrypt_pool(&self, pool_uuid: PoolUuid) -> StratisResult<()>;
+
+    /// Finish pool decryption operation.
+    fn finish_decrypt_pool(&mut self, pool_uuid: PoolUuid, name: &Name) -> StratisResult<()>;
 
     /// Return the metadata that would be written if metadata were written.
     fn current_metadata(&self, pool_name: &Name) -> StratisResult<String>;
diff --git a/src/engine/sim_engine/pool.rs b/src/engine/sim_engine/pool.rs
@@ -975,20 +975,24 @@ impl Pool for SimPool {
         Ok(ReencryptedDevice)
     }
 
-    fn decrypt_pool(
-        &mut self,
-        _: &Name,
-        _: PoolUuid,
-    ) -> StratisResult<DeleteAction<EncryptedDevice>> {
+    fn decrypt_pool_idem_check(&self) -> StratisResult<DeleteAction<EncryptedDevice>> {
         if self.encryption_info.is_none() {
             Ok(DeleteAction::Identity)
         } else {
-            self.encryption_info = None;
-            self.last_reencrypt = None;
             Ok(DeleteAction::Deleted(EncryptedDevice))
         }
     }
 
+    fn do_decrypt_pool(&self, _: PoolUuid) -> StratisResult<()> {
+        Ok(())
+    }
+
+    fn finish_decrypt_pool(&mut self, _: PoolUuid, _: &Name) -> StratisResult<()> {
+        self.encryption_info = None;
+        self.last_reencrypt = None;
+        Ok(())
+    }
+
     fn current_metadata(&self, pool_name: &Name) -> StratisResult<String> {
         serde_json::to_string(&self.record(pool_name)).map_err(|e| e.into())
     }
diff --git a/src/engine/strat_engine/backstore/backstore/v2.rs b/src/engine/strat_engine/backstore/backstore/v2.rs
@@ -1377,12 +1377,13 @@ impl Backstore {
         Ok(())
     }
 
-    pub fn decrypt(&mut self, pool_uuid: PoolUuid) -> StratisResult<()> {
+    pub fn do_decrypt(&self, pool_uuid: PoolUuid) -> StratisResult<()> {
         let handle = self
             .cap_device
             .enc
-            .take()
+            .as_ref()
             .ok_or_else(|| StratisError::Msg("Pool is not encrypted".to_string()))?
+            .as_ref()
             .right()
             .ok_or_else(|| {
                 StratisError::Msg("No space has been allocated from the backstore".to_string())
@@ -1394,6 +1395,25 @@ impl Backstore {
         Ok(())
     }
 
+    pub fn finish_decrypt(
+        &mut self,
+        pool_uuid: PoolUuid,
+        thinpool: &mut ThinPool<Self>,
+        offset: Sectors,
+        direction: OffsetDirection,
+    ) -> StratisResult<()> {
+        thinpool.suspend()?;
+        self.shift_alloc_offset(offset, direction);
+        let (dm_name, _) = format_backstore_ids(pool_uuid, CacheRole::Cache);
+        let set_device_res =
+            get_devno_from_path(&PathBuf::from(DEVICEMAPPER_PATH).join(dm_name.to_string()))
+                .and_then(|devno| thinpool.set_device(devno, offset, direction));
+        thinpool.resume()?;
+        set_device_res?;
+        self.cap_device.enc = None;
+        Ok(())
+    }
+
     /// A summary of block sizes
     pub fn block_size_summary(&self, tier: BlockDevTier) -> Option<BlockSizeSummary> {
         match tier {
diff --git a/src/engine/strat_engine/crypt/handle/v2.rs b/src/engine/strat_engine/crypt/handle/v2.rs
@@ -885,11 +885,12 @@ impl CryptHandle {
     }
 
     /// Decrypt the crypt device for an encrypted pool.
-    pub fn decrypt(self, pool_uuid: PoolUuid) -> StratisResult<()> {
+    pub fn decrypt(&self, pool_uuid: PoolUuid) -> StratisResult<()> {
         let activation_name = format_crypt_backstore_name(&pool_uuid);
         let mut device = acquire_crypt_device(self.luks2_device_path())?;
         let (keyslot, key) = get_passphrase(&mut device, self.encryption_info())?
             .either(|(keyslot, _, key)| (keyslot, key), |tup| tup);
+
         device.reencrypt_handle().reencrypt_init_by_passphrase(
             Some(&activation_name.to_string()),
             key.as_ref(),
diff --git a/src/engine/strat_engine/pool/dispatch.rs b/src/engine/strat_engine/pool/dispatch.rs
@@ -409,14 +409,24 @@ impl Pool for AnyPool {
         }
     }
 
-    fn decrypt_pool(
-        &mut self,
-        name: &Name,
-        pool_uuid: PoolUuid,
-    ) -> StratisResult<DeleteAction<EncryptedDevice>> {
+    fn decrypt_pool_idem_check(&self) -> StratisResult<DeleteAction<EncryptedDevice>> {
+        match self {
+            AnyPool::V1(p) => p.decrypt_pool_idem_check(),
+            AnyPool::V2(p) => p.decrypt_pool_idem_check(),
+        }
+    }
+
+    fn do_decrypt_pool(&self, pool_uuid: PoolUuid) -> StratisResult<()> {
+        match self {
+            AnyPool::V1(p) => p.do_decrypt_pool(pool_uuid),
+            AnyPool::V2(p) => p.do_decrypt_pool(pool_uuid),
+        }
+    }
+
+    fn finish_decrypt_pool(&mut self, pool_uuid: PoolUuid, name: &Name) -> StratisResult<()> {
         match self {
-            AnyPool::V1(p) => p.decrypt_pool(name, pool_uuid),
-            AnyPool::V2(p) => p.decrypt_pool(name, pool_uuid),
+            AnyPool::V1(p) => p.finish_decrypt_pool(pool_uuid, name),
+            AnyPool::V2(p) => p.finish_decrypt_pool(pool_uuid, name),
         }
     }
 
diff --git a/src/engine/strat_engine/pool/v1.rs b/src/engine/strat_engine/pool/v1.rs
@@ -1389,11 +1389,19 @@ impl Pool for StratPool {
         Ok(ReencryptedDevice)
     }
 
-    fn decrypt_pool(
-        &mut self,
-        _: &Name,
-        _: PoolUuid,
-    ) -> StratisResult<DeleteAction<EncryptedDevice>> {
+    fn decrypt_pool_idem_check(&self) -> StratisResult<DeleteAction<EncryptedDevice>> {
+        Err(StratisError::Msg(
+            "Decrypting an encrypted device is only supported in V2 of the metadata".to_string(),
+        ))
+    }
+
+    fn do_decrypt_pool(&self, _: PoolUuid) -> StratisResult<()> {
+        Err(StratisError::Msg(
+            "Decrypting an encrypted device is only supported in V2 of the metadata".to_string(),
+        ))
+    }
+
+    fn finish_decrypt_pool(&mut self, _: PoolUuid, _: &Name) -> StratisResult<()> {
         Err(StratisError::Msg(
             "Decrypting an encrypted device is only supported in V2 of the metadata".to_string(),
         ))
diff --git a/src/engine/strat_engine/pool/v2.rs b/src/engine/strat_engine/pool/v2.rs
@@ -1270,41 +1270,29 @@ impl Pool for StratPool {
     }
 
     #[pool_mutating_action("NoRequests")]
-    fn decrypt_pool(
-        &mut self,
-        name: &Name,
-        pool_uuid: PoolUuid,
-    ) -> StratisResult<DeleteAction<EncryptedDevice>> {
-        let offset = DEFAULT_CRYPT_DATA_OFFSET_V2;
-        let direction = OffsetDirection::Forwards;
+    fn decrypt_pool_idem_check(&self) -> StratisResult<DeleteAction<EncryptedDevice>> {
         match self.backstore.encryption_info() {
             None => Ok(DeleteAction::Identity),
-            Some(_) => {
-                match self.backstore.decrypt(pool_uuid) {
-                    Ok(_) => {
-                        self.last_reencrypt = None;
-                    }
-                    Err(e) => return Err(e),
-                }
-                self.thin_pool.suspend()?;
-                self.backstore.shift_alloc_offset(offset, direction);
-                let set_device_res = self.thin_pool.set_device(
-                    self.backstore.device().expect(
-                        "Since thin pool exists, space must have been allocated \
-                         from the backstore, so backstore must have a cap device",
-                    ),
-                    offset,
-                    direction,
-                );
-                self.thin_pool.resume()?;
-                let metadata_res = self.write_metadata(name);
-                let _ = set_device_res?;
-                metadata_res?;
-                Ok(DeleteAction::Deleted(EncryptedDevice))
-            }
+            Some(_) => Ok(DeleteAction::Deleted(EncryptedDevice)),
         }
     }
 
+    #[pool_mutating_action("NoRequests")]
+    fn do_decrypt_pool(&self, pool_uuid: PoolUuid) -> StratisResult<()> {
+        self.backstore.do_decrypt(pool_uuid)
+    }
+
+    #[pool_mutating_action("NoRequests")]
+    fn finish_decrypt_pool(&mut self, pool_uuid: PoolUuid, name: &Name) -> StratisResult<()> {
+        let offset = DEFAULT_CRYPT_DATA_OFFSET_V2;
+        let direction = OffsetDirection::Forwards;
+        self.backstore
+            .finish_decrypt(pool_uuid, &mut self.thin_pool, offset, direction)?;
+        self.last_reencrypt = None;
+        self.write_metadata(name)?;
+        Ok(())
+    }
+
     fn current_metadata(&self, pool_name: &Name) -> StratisResult<String> {
         serde_json::to_string(&self.record(pool_name)).map_err(|e| e.into())
     }
@@ -2368,12 +2356,13 @@ mod tests {
             .unwrap();
 
             {
-                let mut handle =
-                    test_async!(engine.get_mut_pool(PoolIdentifier::Uuid(pool_uuid))).unwrap();
-                let (name, _, pool) = handle.as_mut_tuple();
-                assert!(pool.is_encrypted());
-                pool.decrypt_pool(&name, pool_uuid).unwrap();
-                assert!(!pool.is_encrypted());
+                let handle = test_async!(engine.get_pool(PoolIdentifier::Uuid(pool_uuid))).unwrap();
+                assert!(handle.is_encrypted());
+                handle.do_decrypt_pool(pool_uuid).unwrap();
+                let (name, _, _) = handle.as_tuple();
+                let mut handle = test_async!(engine.upgrade_pool(handle.into_dyn()));
+                handle.finish_decrypt_pool(pool_uuid, &name).unwrap();
+                assert!(!handle.is_encrypted());
             }
 
             test_async!(engine.stop_pool(PoolIdentifier::Uuid(pool_uuid), true)).unwrap();
