Refactor decryption to use a read lock during operation

Author: jbaublitz

src/dbus/pool/pool_3_9/methods.rs

@@ -201,12 +201,25 @@ pub async fn decrypt_pool_method(
         .get_mut_pool(PoolIdentifier::Uuid(pool_uuid))
         .await
         .ok_or_else(|| StratisError::Msg(format!("No pool associated with uuid {pool_uuid}")));
+    let cloned_engine = Arc::clone(engine);
     match tokio::task::spawn_blocking(move || {
         let mut guard = guard_res?;
 
-        let (name, _, pool) = guard.as_mut_tuple();
-
-        handle_action!(pool.decrypt_pool(&name, pool_uuid))
+        handle_action!(match guard.decrypt_pool_idem_check(pool_uuid) {
+            Ok(DeleteAction::Identity) => Ok(DeleteAction::Identity),
+            Ok(DeleteAction::Deleted(d)) => {
+                let guard = guard.downgrade();
+                guard
+                    .do_decrypt_pool(pool_uuid)
+                    .and_then(|_| {
+                        let mut guard = block_on(cloned_engine.upgrade_pool(guard));
+                        let (name, _, _) = guard.as_mut_tuple();
+                        guard.finish_decrypt_pool(pool_uuid, &name)
+                    })
+                    .map(|_| DeleteAction::Deleted(d))
+            }
+            Err(e) => Err(e),
+        })
     })
     .await
     {

src/engine/engine.rs

@@ -441,13 +441,21 @@ pub trait Pool: Debug + Send + Sync {
         pool_uuid: PoolUuid,
     ) -> StratisResult<ReencryptedDevice>;
 
-    /// Decrypt an encrypted pool.
-    fn decrypt_pool(
+    /// Check idempotence of a pool decrypt command.
+    ///
+    /// This method does not require interior mutability, but a write lock is required
+    /// to ensure that no other decryption operations have already started.
+    fn decrypt_pool_idem_check(
         &mut self,
-        name: &Name,
         pool_uuid: PoolUuid,
     ) -> StratisResult<DeleteAction<EncryptedDevice>>;
 
+    /// Decrypt an encrypted pool.
+    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>;
 

src/engine/sim_engine/pool.rs

@@ -1018,20 +1018,27 @@ impl Pool for SimPool {
         Ok(ReencryptedDevice(pool_uuid))
     }
 
-    fn decrypt_pool(
+    fn decrypt_pool_idem_check(
         &mut self,
-        _: &Name,
         pool_uuid: PoolUuid,
     ) -> 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(pool_uuid)))
         }
     }
 
+    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())
     }

src/engine/strat_engine/backstore/backstore/v2.rs

@@ -1381,12 +1381,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())
@@ -1398,6 +1399,25 @@ impl Backstore {
         Ok(())
     }
 
+    pub fn finish_decrypt(
+        &mut self,
+        pool_uuid: PoolUuid,
+        thinpool: &mut ThinPool<Self>,
+        offset: Sectors,
+        direction: OffsetDirection,
+    ) -> StratisResult<()> {
+        thinpool.suspend()?;
+        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()?;
+        self.shift_alloc_offset(offset, direction);
+        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 {

src/engine/strat_engine/crypt/handle/v2.rs

@@ -893,11 +893,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(),

src/engine/strat_engine/pool/dispatch.rs

@@ -410,14 +410,27 @@ impl Pool for AnyPool {
         }
     }
 
-    fn decrypt_pool(
+    fn decrypt_pool_idem_check(
         &mut self,
-        name: &Name,
         pool_uuid: PoolUuid,
     ) -> StratisResult<DeleteAction<EncryptedDevice>> {
         match self {
-            AnyPool::V1(p) => p.decrypt_pool(name, pool_uuid),
-            AnyPool::V2(p) => p.decrypt_pool(name, pool_uuid),
+            AnyPool::V1(p) => p.decrypt_pool_idem_check(pool_uuid),
+            AnyPool::V2(p) => p.decrypt_pool_idem_check(pool_uuid),
+        }
+    }
+
+    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.finish_decrypt_pool(pool_uuid, name),
+            AnyPool::V2(p) => p.finish_decrypt_pool(pool_uuid, name),
         }
     }
 

src/engine/strat_engine/pool/v1.rs

@@ -1390,16 +1390,30 @@ impl Pool for StratPool {
         Ok(ReencryptedDevice(pool_uuid))
     }
 
-    fn decrypt_pool(
+    #[pool_mutating_action("NoRequests")]
+    fn decrypt_pool_idem_check(
         &mut self,
-        _: &Name,
         _: PoolUuid,
     ) -> StratisResult<DeleteAction<EncryptedDevice>> {
         Err(StratisError::Msg(
             "Decrypting an encrypted device is only supported in V2 of the metadata".to_string(),
         ))
     }
 
+    #[pool_mutating_action("NoRequests")]
+    fn do_decrypt_pool(&self, _: PoolUuid) -> StratisResult<()> {
+        Err(StratisError::Msg(
+            "Decrypting an encrypted device is only supported in V2 of the metadata".to_string(),
+        ))
+    }
+
+    #[pool_mutating_action("NoRequests")]
+    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(),
+        ))
+    }
+
     fn current_metadata(&self, pool_name: &Name) -> StratisResult<String> {
         serde_json::to_string(&self.record(pool_name)).map_err(|e| e.into())
     }

src/engine/strat_engine/pool/v2.rs

@@ -1277,41 +1277,32 @@ impl Pool for StratPool {
     }
 
     #[pool_mutating_action("NoRequests")]
-    fn decrypt_pool(
+    fn decrypt_pool_idem_check(
         &mut self,
-        name: &Name,
         pool_uuid: PoolUuid,
     ) -> StratisResult<DeleteAction<EncryptedDevice>> {
-        let offset = DEFAULT_CRYPT_DATA_OFFSET_V2;
-        let direction = OffsetDirection::Forwards;
         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()?;
-                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()?;
-                self.backstore.shift_alloc_offset(offset, direction);
-                let metadata_res = self.write_metadata(name);
-                let _ = set_device_res?;
-                metadata_res?;
-                Ok(DeleteAction::Deleted(EncryptedDevice(pool_uuid)))
-            }
+            Some(_) => Ok(DeleteAction::Deleted(EncryptedDevice(pool_uuid))),
         }
     }
 
+    #[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())
     }
@@ -2382,12 +2373,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();