Changed local_ids for Atomic counter and removed key_slot_semaphore. #191
Conversation
| // If storing key failed and no other keys were created in the mean time, it is safe to | ||
| // decrement the key counter. | ||
| let _ = max_current_id.compare_and_swap(new_key_id, new_key_id - 1, Relaxed); | ||
| return Err(ResponseStatus::PsaErrorBadState); |
There was a problem hiding this comment.
Wasn't sure on the error to return if there are no more key IDs available.
There was a problem hiding this comment.
I think BadState is an error referring to multi-parts operations. It might be better to use InsufficientMemory. Also, could you please add an error! log to note that we've reached the limit?
There was a problem hiding this comment.
Is it a good idea to return InsufficientMemory? I guess in conjuction with a log message it would be easy to understand what's going on, but based on the error code alone it might be confusing.
There was a problem hiding this comment.
I was thinking about adding a new error but didn't want to touch parsec-interface without discussing first.
Something like 'MaxKeyIdReached'.
There was a problem hiding this comment.
I think the problem with a new response code like that is we shouldn't be exposing internal issues (or, not ones that mean nothing to a client). InsufficientMemory is probably ok if we document what it means in our context - we just add an entry here
| // If storing key failed and no other keys were created in the mean time, it is safe to | ||
| // decrement the key counter. | ||
| let _ = max_current_id.compare_and_swap(new_key_id, new_key_id - 1, Relaxed); | ||
| return Err(ResponseStatus::PsaErrorBadState); |
There was a problem hiding this comment.
I think BadState is an error referring to multi-parts operations. It might be better to use InsufficientMemory. Also, could you please add an error! log to note that we've reached the limit?
| if new_key_id > key::PSA_KEY_ID_USER_MAX { | ||
| // If storing key failed and no other keys were created in the mean time, it is safe to | ||
| // decrement the key counter. | ||
| let _ = max_current_id.compare_and_swap(new_key_id, new_key_id - 1, Relaxed); |
There was a problem hiding this comment.
As you said in your issue comment, do you think this is always safe?
There was a problem hiding this comment.
There's one problem I've thought of (see second paragraph) but in general, yes. In that particular case, the new ID would be created with the value of PSA_KEY_ID_USER_MAX + 1, and max_current_id would be set to that. If no other thread tries to create a new ID, max_current_id will still be PSA_KEY_ID_USER_MAX + 1, which will then be decremented back down to PSA_KEY_ID_USER_MAX. If a new thread does try to create a new ID, it will increment it to PSA_KEY_ID_USER_MAX + 2. The original thread will then fail the compare and not touch PSA_KEY_ID_USER_MAX.
It could overflow though, theoretically there's theoretically a chance that mac_current_id could keep going up if new threads stop any older threads from decrementing it. Maybe it'd be better to just hard reset it to PSA_KEY_ID_USER_MAX if it fails the check? No key will ever be created if max_current_id is at the ID limit so we should be able to set it back.
Thread #1 comes in, increments it to MAX + 1 and realises its too high. Then thread #2 comes in, increments it to MAX + 2 and also realises its too high. Thread #1 runs again and resets to MAX. Then thread #2 does the same (though it's already at MAX).
Decrementing in the other case (line 74) should also be safe. If no threads have created key IDs in the mean time, the compare will match and the counter will be decremented correctly. If a thread has come in in the mean time, the compare will fail and the counter will not be decremented.
Am I missing something?
There was a problem hiding this comment.
Maybe it'd be better to just hard reset it to PSA_KEY_ID_USER_MAX if it fails the check? No key will ever be created if max_current_id is at the ID limit so we should be able to set it back.
Yeah, just commented the same thing in the issue, that sounds sensible!
As for the second decrement - it works, but is it needed? I'd say that having an operation that isn't deterministic isn't a good idea (regardless of correctness) and if you end up crashing through the handle limit then you have bigger issues on your system.
There was a problem hiding this comment.
Yeah, I saw 😋
Ok. Yeah if you're going that high, saving a key ID here and there isn't going to make a difference, I'll remove it.
| while local_ids_handle.contains(&key_id) { | ||
| key_id = rng.gen_range(key::PSA_KEY_ID_USER_MIN, key::PSA_KEY_ID_USER_MAX + 1); | ||
| // fetch_add adds 1 to the old value and returns the old value, so add 1 for new ID | ||
| let new_key_id = max_current_id.fetch_add(1, Relaxed) + 1; |
There was a problem hiding this comment.
I guess it is fine to not care about overflows as PSA_KEY_ID_USER_MAX is equal to the defined value of 0x3FFFFFFF.
There was a problem hiding this comment.
Better safe than sorry or overkill? 😉
src/providers/mbed_provider/mod.rs
Outdated
| if key_id > mbed_provider.id_counter.load(Relaxed) { | ||
| mbed_provider.id_counter.store(key_id, Relaxed); | ||
| } |
There was a problem hiding this comment.
It might be more efficient to find the maximum locally, in a psa_key_id_t variable and then store it in the id_counter at the end. I believe the atomic operations impose restrictions on compiler optimizations.
There was a problem hiding this comment.
Ah, didn't realise there would be much overhead in just using the atomic variable. I will change it.
Signed-off-by: Samuel Bailey <[email protected]>
sbailey-arm
force-pushed
the
further-simplify-mbedcrypto-provider
branch
from
0f5db9b to
014bd8a
Compare
key_slot_semaphoreremoved - #187 1.local_idschanged to anAtomicU32that store the current highest key ID including destroyed keys). On load, loops through all stored keys and sets the counter to the max ID. On creating a key ID, adds 1 to the counter. If key store fails and no thread has created another key in the mean time, decrements the counter to the value before the attempt to create a new key ID failed. On key destroy, counter not altered as extra complication deemed unnecessary. #187 2.Signed-off-by: Samuel Bailey [email protected]