feat(encryption): allow pools on same node to share keys #1848
Conversation
The pools in same node(io-engine) can now share encryption key. Earlier, we used to set a crypto vbdev as key owner which means that during crypto vbdev destroy, the key was also destroyed. Now we don't set a crypto vbdev as owner and hence need to manage the key lifecycle using a hashmap that keeps track of which key is being used by which all crypto vbdevs. Signed-off-by: Diwakar Sharma <[email protected]>
|
Tested by creating 15 pools distributed among 3 keys. Shutdown the io-engine, recreate/import of pools worked ok. Destroyed pools, and keys were destroyed by the last user of a particular key. |
dsharma-dc
requested review from
tiagolobocastro,
Abhinandan-Purkait and
abhilashshetty04
|
Opening for review. If necessary we can take this for MVP, otherwise for the next drop. |
This change rather intends to detach the lifecycle of key from the lifecycle of any particular pool/cryptobdev. |
Does this mean in an Io-engine, When the first crypto bdev it creates the encryption key and the same key is used for all subsequent crypto bdev that gets created? |
Yes. with this change the |
| } else { | ||
| let err = spdk_accel_crypto_key_destroy(key); | ||
| if err != 0 { | ||
| error!("Failed to destroy spdk accel crypto key"); |
There was a problem hiding this comment.
| error!("Failed to destroy spdk accel crypto key"); | |
| error!("Failed to destroy spdk accel crypto key: {err}"); |
| } | ||
|
|
||
| /// Gets all crypto vbdev names using a given key_name. | ||
| #[allow(dead_code)] |
| // Check if the key with this name exists. If it does, then we reuse that. | ||
| let existing_key = spdk_accel_crypto_key_get(create_key_params.key_name); | ||
| if !existing_key.is_null() { | ||
| info!("Key {:?} exists already, reusing it.", key_params.key_name); |
There was a problem hiding this comment.
Maybe add the pool name as well to make it clear
| @@ -271,6 +369,13 @@ pub fn create_crypto_vbdev_on_base_bdev( | |||
| }) | |||
| } else { | |||
| info!("crypto vbdev {crypto_vbdev_name} created on base bdev {base_bdev_name}"); | |||
| add_key_user(key_params.key_name.clone(), crypto_vbdev_name.to_string()); | |||
There was a problem hiding this comment.
Shouldn't the key be added on the create_crypto_key?
If the pool create fails, don't we need to delete it?
There was a problem hiding this comment.
Shouldn't the key be added on the create_crypto_key?
Seems more logical to add once the user i.e. the crypto vbdev is also created, which happens after create_crypto_key.
If the pool create fails, don't we need to delete it?
When the pool create fails, we usually destroy bdevs that are already created. This means we also destroy crypto vbdev via destroy_crypto_vbdev, where the remove call is placed.
There was a problem hiding this comment.
Ah I see, so the cleanup would take care of it.
Though is there a race condition here when:
pool1 with secret A exists
pool2 create with secret A, secret A exists, but name not added yet
pool1 destroy, remove it's bdev name, and now list is empty so it destroys the the secret key?
There was a problem hiding this comment.
This window is possible. This can be plugged by making a recheck-and-create step for key inside the add_key_user. OR
create_crypto_key can be always called from inside add_key_user, which will make map entry before crypto bdev is created. If bdev creation fails later, cleanup path will attempt entry removal which we'll have to do special handling for ENOENT(because bdev doesn't exist).
There was a problem hiding this comment.
The second approach seems more solid if we handle the create crypto from the locked path
The pools in same node(io-engine) can now share encryption key. Earlier, we used to set a crypto vbdev as key owner which means that during crypto vbdev destroy, the key was also destroyed. Now we don't set a crypto vbdev as owner and hence need to manage the key lifecycle using a hashmap that keeps track of which key is being used by which all crypto vbdevs.