-
Notifications
You must be signed in to change notification settings - Fork 2.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ssl_tls12_populate_transform optim #6072
ssl_tls12_populate_transform optim #6072
Conversation
67498df
to
6811c33
Compare
There are quite some failures in two test families on the CI:
|
6811c33
to
d2a4e4c
Compare
I needed to move an |
@AndrzejKurek CI tests now pass. Ready for review. Thanks. |
FYI: these simplifications in this PR were uncovered while attempting to port hostap to have an option to use mbedtls, and I needed the keyblock size to implement the hostap API |
d2a4e4c
to
92d69b9
Compare
branch has been rebased on HEAD of 'development' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with everything but the part that moves taglen
setting and makes it so that there are two calls to mbedtls_ssl_cipher_to_psa
in case of AEAD ciphersuites. What's the gain here?
Previously, there were always two calls to Edit: The PR replaces a call to |
Separately, and not included in this PR,
Then, |
@AndrzejKurek you approved this 7 weeks ago. Would you please nominate a second approver? Thank you. |
I'm sorry, but that's not my authority. This PR is on our "todo" board for any volunteer to pick up and review, but the decision on what to review is governed by personal preference and higher-level priorities. Sorry that you're waiting for so long! I'll bring our team's attention to it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change is correct, but now that I've reviewed it I don't understand why it's desirable. I don't find it particularly more or less readable than the previous code. This is presented as an optimization, but what is being optimized? There's very little computation going on, so performance is irrelevant. Memory usage isn't changing. Code size might be changing, but with in the configuration of tests/scripts/all.sh build_arm_none_eabi_gcc_m0plus
I get:
commit | size ssl_tls.o |
---|---|
52f83dc | 24715 |
92d69b9 | 24727 |
1e1bf341fb6283f11bab1c7e1c0f0be56492bbd6 | 24719 |
So where's the optimization? Is this for a specific configuration with a reduced feature set where the new code makes a difference?
library/ssl_tls.c
Outdated
mbedtls_ssl_cipher_to_psa( ciphersuite_info->cipher, transform->taglen, | ||
&alg, &key_type, &key_bits ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why recalculate alg
, key_type
and key_bits
here? Above you've already calculated them, but possibly with a different tag length, which only influences the algorithm. So here key_type
and key_bits
are correct but alg
is the full-tag-length version of the algorithm.
mbedtls_ssl_cipher_to_psa( ciphersuite_info->cipher, transform->taglen, | |
&alg, &key_type, &key_bits ); | |
alg = PSA_ALG_AEAD_WITH_SHORTENED_TAG( alg, transform->taglen ); |
or
transform->taglen = 16;
if( ciphersuite_info->flags & MBEDTLS_CIPHERSUITE_SHORT_TAG )
{
transform->taglen = 8;
alg = PSA_ALG_AEAD_WITH_SHORTENED_TAG( alg, 8 );
}
This isn't just or even mainly about optimization, it's also about reducing the complexity of the code by having fewer mutated variables.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, as noted below, the code changes highlight where the code could be simplied.
I did not specify sufficiently that the optimization is more a code-flow simplification, rather than a performance optimization.
If I understand the rest of the code correctly, the change you suggested above applies only to AEAD.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
As noted in the original post and also above the code changes aim to make the code simpler, disentangling some previously entangled code flows: separating some of the PSA-specific code from the non-PSA mbedtls_cipher code, collecting some fo the CBC-ETM code, and separating some work that needs to be done for AEAD (for which you suggested improvements). |
ae71576
to
ca21ca0
Compare
- remove redundant calls to retrieve info - defer calls to where needed for specific ssl_modes Signed-off-by: Glenn Strauss <gstrauss@gluelogic.com>
- ssl_tls12_populate_transform using PSA_ALG_AEAD_WITH_SHORTENED_TAG() instead of calling mbedtls_ssl_cipher_to_psa() Signed-off-by: Glenn Strauss <gstrauss@gluelogic.com>
ca21ca0
to
5e819ae
Compare
@AndrzejKurek you last reviewed this over 3 months ago, and I made the small changes requested then, over 3 months ago. I also responded the same day in #6072 (comment) to @gilles-peskine-arm question, repeating information already provided in the original post. If removing redundant function calls, untangling PSA and non-PSA code to reduce ifdefs, and moving work to scopes where it is needed is not deemed to be cleaner code, then maybe I do not understand what @gilles-peskine-arm considers cleaner code. Maybe @gilles-peskine-arm had an issue with my use of the abbreviation "optim" for removed redundancies, localized work, and more cleanly structured code? |
I'm sorry for missing this PR, I'll re-review this today. Please re-request a review once you think that the PR is ready (as you did in other PRs), as otherwise my "review requested" list won't show it. |
#endif /* MBEDTLS_USE_PSA_CRYPTO */ | ||
|
||
if (ssl_mode == MBEDTLS_SSL_MODE_AEAD) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually think that his version of the code (lines 8214 - 8253) is less readable and does more work to get the same result as before, at the benefit of not calling mbedtls_ssl_cipher_to_psa
twice. I can approve this PR if this is a performance optimization or if a second reviewer will determine that this code is better this way, but I would like to voice my concerns.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gstrauss - have you done any performance tests? As I believe that this was the reason you added this as an optimization?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason I submitted this PR was a result of porting hostap to use mbedtls where I needed to get a specific piece of data. I had to trace through what I considered convoluted code in ssl_tls.c in order to find out how to obtain this data.
No, I do not have specific performance numbers. My visual analysis is that the cleaner code flow should be faster, and more importantly, should not be appreciably slower. Any performance improvement would come from avoiding redundant calls or caching effects of doing work in a tighter scope where the results are used, but I am more interested in slightly cleaner code, which may contribute to later code cleanup once PSA is the default in mbedtls 3.x and the older interface is deprecated/removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, the work done in the current patch is done for PSA_ALG_CCM and is the same work that would be done by calling mbedtls_ssl_cipher_to_psa()
, but only doing the work for PSA_ALG_CCM and without the mbedtls_ssl_cipher_to_psa()
switch()
and without filling in variables that are then not used.
Please let me know whatever you and the second reviewer think is best.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part of the code is a few more lines than the previous, but not only avoids the second call to mbedtls_ssl_cipher_to_psa()
, but also separates out MBEDTLS_SSL_SOME_SUITES_USE_CBC_ETM
and whether or not PSA is used, rather than having a preprocessor conditional for an extra arg to a function whether or not MBEDTLS_SSL_SOME_SUITES_USE_CBC_ETM
is defined.
As I've mentioned before, I do not find the new code more readable, less redundant, or otherwise cleaner. So I won't block it, but as far as I'm concerned, this is taking review bandwidth away from more important things. We're likely to change how TLS code calculates hashes soon, following some refactoring of legacy interfaces (design not yet done for cipher). I don't think the changes in this pull request will particularly help or hinder, but they will likely conflict. |
@gilles-peskine-arm this PR was filed almost 8 months ago, whereas #7120 was created two weeks ago. My crystal ball must be broken. 🤷 |
It looks like this is unlikely to be accepted - closing. |
Description
ssl_tls12_populate_transform optimizations
(
mbedtls_ssl_cipher_to_psa()
andmbedtls_cipher_info_from_type()
are also called frommbedtls_ssl_get_mode_from_ciphersuite()
)Status
READY
Requires Backporting
NO
Migrations
NO
Todos