Refactor encryption to use a read lock during operation

Author: jbaublitz

src/dbus_api/pool/pool_3_9/methods.rs

@@ -86,17 +86,43 @@ pub fn encrypt_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, _, pool) = guard.as_mut_tuple();
-
-    let result = handle_action!(
-        pool.encrypt_pool(&name, pool_uuid, &ei),
+    let read_guard = get_pool!(dbus_context.engine; pool_uuid; default_return; return_message);
+
+    // Check if operation is idempotent and complete all operations before handling result
+    let create_result = handle_action!(
+        read_guard.encrypt_pool_idem_check()
+            .and_then(|action| match action {
+                CreateAction::Identity => Ok(action),
+                CreateAction::Created(_) => {
+                    let mut guard = block_on(dbus_context.engine.upgrade_pool(read_guard));
+                    let result = guard.start_encrypt_pool(pool_uuid, &ei);
+                    let result = result.and_then(|(sector_size, key_info)| {
+                        let guard = guard.downgrade();
+                        guard
+                            .do_encrypt_pool(pool_uuid, sector_size, key_info)
+                            .map(|_| guard)
+                    });
+                    let result = result.and_then(|guard| {
+                        let mut guard = block_on(dbus_context.engine.upgrade_pool(guard));
+                        let (name, _, _) = guard.as_mut_tuple();
+                        guard
+                            .finish_encrypt_pool(&name, pool_uuid)
+                            .map(|_| (guard, action))
+                    });
+                    result.map(|(_, action)| action)
+                }
+            }),
         dbus_context,
         pool_path.get_name()
     );
-    let msg = match result {
+
+    let msg = match create_result {
+        Ok(CreateAction::Identity) => {
+            return_message.append3(default_return, DbusErrorEnum::OK as u16, OK_STRING.to_string())
+        }
         Ok(CreateAction::Created(_)) => {
-            let encryption_info = match pool.encryption_info().clone() {
+            let guard = get_pool!(dbus_context.engine; pool_uuid; default_return; return_message);
+            let encryption_info = match guard.encryption_info().clone() {
                 Some(Either::Left(ei)) => ei,
                 Some(Either::Right(_)) => {
                     unreachable!("online reencryption disabled on metadata V1")
@@ -118,9 +144,6 @@ pub fn encrypt_pool(m: &MethodInfo<'_, MTSync<TData>, TData>) -> MethodResult {
             dbus_context.push_pool_encryption_status_change(pool_path.get_name(), true);
             return_message.append3(true, DbusErrorEnum::OK as u16, OK_STRING.to_string())
         }
-        Ok(CreateAction::Identity) => {
-            return_message.append3(false, 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)

src/engine/engine.rs

@@ -399,13 +399,26 @@ pub trait Pool: Debug + Send + Sync {
         limit: Option<Bytes>,
     ) -> StratisResult<PropChangeAction<Option<Sectors>>>;
 
+    /// Check idempotence of pool encrypt operation.
+    fn encrypt_pool_idem_check(&self) -> StratisResult<CreateAction<EncryptedDevice>>;
+
     /// Encrypted an unencrypted pool.
-    fn encrypt_pool(
+    fn start_encrypt_pool(
         &mut self,
-        name: &Name,
         pool_uuid: PoolUuid,
         encryption_info: &InputEncryptionInfo,
-    ) -> StratisResult<CreateAction<EncryptedDevice>>;
+    ) -> StratisResult<(u32, (u32, SizedKeyMemory))>;
+
+    /// Encrypted an unencrypted pool.
+    fn do_encrypt_pool(
+        &self,
+        pool_uuid: PoolUuid,
+        sector_size: u32,
+        key_info: (u32, SizedKeyMemory),
+    ) -> StratisResult<()>;
+
+    /// Update internal data structures with the result of the encryption operation.
+    fn finish_encrypt_pool(&mut self, name: &Name, pool_uuid: PoolUuid) -> StratisResult<()>;
 
     /// Start reencryption of an encrypted pool.
     ///

src/engine/sim_engine/pool.rs

@@ -14,6 +14,7 @@ use itertools::Itertools;
 use serde_json::{Map, Value};
 
 use devicemapper::{Bytes, Sectors, IEC};
+use libcryptsetup_rs::SafeMemHandle;
 
 use crate::{
     engine::{
@@ -930,20 +931,31 @@ impl Pool for SimPool {
         }
     }
 
-    fn encrypt_pool(
-        &mut self,
-        _: &Name,
-        _: PoolUuid,
-        enc: &InputEncryptionInfo,
-    ) -> StratisResult<CreateAction<EncryptedDevice>> {
+    fn encrypt_pool_idem_check(&self) -> StratisResult<CreateAction<EncryptedDevice>> {
         if self.encryption_info.is_some() {
             Ok(CreateAction::Identity)
         } else {
-            self.encryption_info = convert_encryption_info(Some(enc), None)?;
             Ok(CreateAction::Created(EncryptedDevice))
         }
     }
 
+    fn start_encrypt_pool(
+        &mut self,
+        _: PoolUuid,
+        enc: &InputEncryptionInfo,
+    ) -> StratisResult<(u32, (u32, SizedKeyMemory))> {
+        self.encryption_info = convert_encryption_info(Some(enc), None)?;
+        Ok((0, (0, SizedKeyMemory::new(SafeMemHandle::alloc(1)?, 0))))
+    }
+
+    fn do_encrypt_pool(&self, _: PoolUuid, _: u32, _: (u32, SizedKeyMemory)) -> StratisResult<()> {
+        Ok(())
+    }
+
+    fn finish_encrypt_pool(&mut self, _: &Name, _: PoolUuid) -> StratisResult<()> {
+        Ok(())
+    }
+
     fn start_reencrypt_pool(&mut self) -> StratisResult<Vec<(u32, SizedKeyMemory, u32)>> {
         if self.encryption_info.is_none() {
             Err(StratisError::Msg(

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

@@ -4,11 +4,7 @@
 
 // Code to handle the backing store of a pool.
 
-use std::{
-    cmp,
-    iter::once,
-    path::{Path, PathBuf},
-};
+use std::{cmp, iter::once, path::PathBuf};
 
 use chrono::{DateTime, Utc};
 use either::Either;
@@ -1295,37 +1291,46 @@ impl Backstore {
         self.data_tier.grow(dev)
     }
 
-    pub fn encrypt(
+    pub fn prepare_encrypt(
         &mut self,
         pool_uuid: PoolUuid,
         thinpool: &mut ThinPool<Self>,
         offset: Sectors,
         offset_direction: OffsetDirection,
         encryption_info: &InputEncryptionInfo,
-    ) -> StratisResult<()> {
+    ) -> StratisResult<(u32, (u32, SizedKeyMemory))> {
         let (dm_name, _) = format_backstore_ids(pool_uuid, CacheRole::Cache);
-        let unencrypted_path = PathBuf::from(format!("/dev/mapper/{}", &dm_name.to_string()));
-        let (mut device, sector_size, key_info) =
+        let unencrypted_path = PathBuf::from(DEVICEMAPPER_PATH).join(dm_name.to_string());
+        let (sector_size, key_info) =
             CryptHandle::setup_encrypt(pool_uuid, thinpool, &unencrypted_path, encryption_info)?;
 
         thinpool.suspend()?;
         self.shift_alloc_offset(offset, offset_direction);
-        let set_device_res = get_devno_from_path(Path::new(&format!(
-            "/dev/mapper/{}",
-            &*format_crypt_backstore_name(&pool_uuid)
-        )))
+        let set_device_res = get_devno_from_path(
+            &PathBuf::from(DEVICEMAPPER_PATH)
+                .join(format_crypt_backstore_name(&pool_uuid).to_string()),
+        )
         .and_then(|devno| thinpool.set_device(devno, offset, offset_direction));
         thinpool.resume()?;
         set_device_res?;
 
-        let handle = CryptHandle::do_encrypt(
-            &mut device,
-            &unencrypted_path,
-            pool_uuid,
-            sector_size,
-            key_info,
-        )?;
+        Ok((sector_size, key_info))
+    }
 
+    pub fn do_encrypt(
+        pool_uuid: PoolUuid,
+        sector_size: u32,
+        key_info: (u32, SizedKeyMemory),
+    ) -> StratisResult<()> {
+        let (dm_name, _) = format_backstore_ids(pool_uuid, CacheRole::Cache);
+        let unencrypted_path = PathBuf::from(DEVICEMAPPER_PATH).join(dm_name.to_string());
+        CryptHandle::do_encrypt(&unencrypted_path, pool_uuid, sector_size, key_info)
+    }
+
+    pub fn finish_encrypt(&mut self, pool_uuid: PoolUuid) -> StratisResult<()> {
+        let (dm_name, _) = format_backstore_ids(pool_uuid, CacheRole::Cache);
+        let unencrypted_path = PathBuf::from(DEVICEMAPPER_PATH).join(dm_name.to_string());
+        let handle = CryptHandle::finish_encrypt(&unencrypted_path, pool_uuid)?;
         self.cap_device.enc = Some(Either::Right(handle));
         Ok(())
     }

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

@@ -726,7 +726,7 @@ impl CryptHandle {
         thinpool: &ThinPool<v2::Backstore>,
         unencrypted_path: &Path,
         encryption_info: &InputEncryptionInfo,
-    ) -> StratisResult<(CryptDevice, u32, (u32, SizedKeyMemory))> {
+    ) -> StratisResult<(u32, (u32, SizedKeyMemory))> {
         let mut tmp_file = tempfile::NamedTempFile::new()?;
         tmp_file
             .as_file_mut()
@@ -804,7 +804,7 @@ impl CryptHandle {
             CryptActivate::SHARED,
         )?;
 
-        Ok((device, sector_size, (keyslot, key)))
+        Ok((sector_size, (keyslot, key)))
     }
 
     /// Perform the online encryption operation that was set up.
@@ -813,12 +813,12 @@ impl CryptHandle {
     /// Precondition: crypt device was already added as the backing device for the thin pool
     ///               failure to do so will result in corruption
     pub fn do_encrypt(
-        device: &mut CryptDevice,
         unencrypted_path: &Path,
         pool_uuid: PoolUuid,
         sector_size: u32,
         key_info: (u32, SizedKeyMemory),
-    ) -> StratisResult<Self> {
+    ) -> StratisResult<()> {
+        let mut device = acquire_crypt_device(unencrypted_path)?;
         let activation_name = &format_crypt_backstore_name(&pool_uuid).to_string();
         let (keyslot, key) = key_info;
         device.reencrypt_handle().reencrypt_init_by_passphrase(
@@ -849,7 +849,16 @@ impl CryptHandle {
             },
         )?;
         device.reencrypt_handle().reencrypt2::<()>(None, None)?;
+        Ok(())
+    }
 
+    /// Generate the CryptHandle from the LUKS2 device path.
+    ///
+    /// Precondition: LUKS2 device path was fully encrypted successfully
+    pub fn finish_encrypt(
+        unencrypted_path: &Path,
+        pool_uuid: PoolUuid,
+    ) -> StratisResult<CryptHandle> {
         CryptHandle::setup(unencrypted_path, pool_uuid, TokenUnlockMethod::Any, None)
             .map(|h| h.expect("should have crypt device after online encrypt"))
     }

src/engine/strat_engine/pool/dispatch.rs

@@ -351,15 +351,40 @@ impl Pool for AnyPool {
         }
     }
 
-    fn encrypt_pool(
+    fn encrypt_pool_idem_check(&self) -> StratisResult<CreateAction<EncryptedDevice>> {
+        match self {
+            AnyPool::V1(p) => p.encrypt_pool_idem_check(),
+            AnyPool::V2(p) => p.encrypt_pool_idem_check(),
+        }
+    }
+
+    fn start_encrypt_pool(
         &mut self,
-        name: &Name,
         pool_uuid: PoolUuid,
         encryption_info: &InputEncryptionInfo,
-    ) -> StratisResult<CreateAction<EncryptedDevice>> {
+    ) -> StratisResult<(u32, (u32, SizedKeyMemory))> {
+        match self {
+            AnyPool::V1(p) => p.start_encrypt_pool(pool_uuid, encryption_info),
+            AnyPool::V2(p) => p.start_encrypt_pool(pool_uuid, encryption_info),
+        }
+    }
+
+    fn do_encrypt_pool(
+        &self,
+        pool_uuid: PoolUuid,
+        sector_size: u32,
+        key_info: (u32, SizedKeyMemory),
+    ) -> StratisResult<()> {
+        match self {
+            AnyPool::V1(p) => p.do_encrypt_pool(pool_uuid, sector_size, key_info),
+            AnyPool::V2(p) => p.do_encrypt_pool(pool_uuid, sector_size, key_info),
+        }
+    }
+
+    fn finish_encrypt_pool(&mut self, name: &Name, pool_uuid: PoolUuid) -> StratisResult<()> {
         match self {
-            AnyPool::V1(p) => p.encrypt_pool(name, pool_uuid, encryption_info),
-            AnyPool::V2(p) => p.encrypt_pool(name, pool_uuid, encryption_info),
+            AnyPool::V1(p) => p.finish_encrypt_pool(name, pool_uuid),
+            AnyPool::V2(p) => p.finish_encrypt_pool(name, pool_uuid),
         }
     }
 

src/engine/strat_engine/pool/v1.rs

@@ -1340,12 +1340,33 @@ impl Pool for StratPool {
         }
     }
 
-    fn encrypt_pool(
+    #[pool_mutating_action("NoRequests")]
+    fn encrypt_pool_idem_check(&self) -> StratisResult<CreateAction<EncryptedDevice>> {
+        Err(StratisError::Msg(
+            "Encrypting an unencrypted device is only supported in V2 of the metadata".to_string(),
+        ))
+    }
+
+    #[pool_mutating_action("NoRequests")]
+    fn start_encrypt_pool(
         &mut self,
-        _: &Name,
         _: PoolUuid,
         _: &InputEncryptionInfo,
-    ) -> StratisResult<CreateAction<EncryptedDevice>> {
+    ) -> StratisResult<(u32, (u32, SizedKeyMemory))> {
+        Err(StratisError::Msg(
+            "Encrypting an unencrypted device is only supported in V2 of the metadata".to_string(),
+        ))
+    }
+
+    #[pool_mutating_action("NoRequests")]
+    fn do_encrypt_pool(&self, _: PoolUuid, _: u32, _: (u32, SizedKeyMemory)) -> StratisResult<()> {
+        Err(StratisError::Msg(
+            "Encrypting an unencrypted device is only supported in V2 of the metadata".to_string(),
+        ))
+    }
+
+    #[pool_mutating_action("NoRequests")]
+    fn finish_encrypt_pool(&mut self, _: &Name, _: PoolUuid) -> StratisResult<()> {
         Err(StratisError::Msg(
             "Encrypting an unencrypted device is only supported in V2 of the metadata".to_string(),
         ))