Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[3.6] Reduce performance regression in RSA public operations #9281
[3.6] Reduce performance regression in RSA public operations #9281
Changes from 16 commits
75ed587
e084964
38ff70e
bb3f295
90b4271
0c292b2
a5fc8f3
020b9ab
e0842aa
7342656
2c62441
9d72df8
a112691
8786dd7
afb2079
878af12
6c20869
82976f3
5d16334
5f31697
4c857c4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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'm sorry but I realized I made a mistake in a previous approval:
But as of 878af12 we are adding
mbedtls_mpi_exp_mod_unsafe
to the public API of the 3.6 LTS branch. I'm ok with the prototype but I don't like the name. Also I don't like to rush when extending the API of an LTS branch.Can we make this an internal function instead? It's only meant to be used in
rsa.c
after all.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.
If you don't like the name we need to have a larger discussion because we already have a few functions called
_unsafe
(as Janos pointed out already), so if we make a change it should apply to all functions.But I fully agree about making the new function internal, especially considering it's an LTS and we don't have time to have the naming discussion.
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.
Not in the public API. I don't care nearly as much about internal functions.
We do have one internal function called
xxx_unsafe
(mbedtls_mpi_core_get_mont_r2_unsafe
) and while I'm ok (but not enthusiastic) about the name, I'm bothered that its documentation doesn't explain in what way it's unsafe.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.
Indeed, the other function is not public, so that's an additional reason to keep the new one private.
(I also agree that naming is less important for internal functions, but we can still have a discussion about it some time. I can see arguments for calling them
_leaky
rather than_unsafe
as it's more informative. Also, for the smallstatic
functions that are currently calledoptionally_safe
, I thinkslow_or_leaky
would make anyone think before using them. But let's have this discussion another time.)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 don't like this. A + B + 1 is not a good way to get a number that's neither A nor B - what if A = 0 and B = -1? I know that's not true here, but this expression has a dependency on the values of A and B (and we don't have a static assert that this value = neither of them).
How would you feel about a third value specified in the #defines, of something like MBEDTLS_IS_NETHER_FOR_TESTING (name is horrible, but you get the idea)?
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'm unresolving this as we've reverted past the commit that addressed this
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've already added this commit to the followup PR locally, I just haven't pushed it, because don't want to trigger the CI.
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.
Non-blocker: I don't like relying on auxiliary functions to initialize variables. Unfortunately compilers aren't good at analyzing whether variables are analyzed on all code paths, and we don't run Coverity assiduously enough to notice all problems. I would strongly prefer to initialize to the safe-path values here, and have
exp_mod_calc_first_bit_optionally_safe
modify the values on the unsafe path.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.
mbedtls_mpi_optionally_safe_codepath
gives me hives. It's not at all obvious that turning onMBEDTLS_TEST_HOOKS
doesn't change the functional behavior of the code: I had to carefully check the places wherembedtls_mpi_optionally_safe_codepath
is used and verify that when the library reads it, it only influences how the library might modify it and nothing else.mbedtls_mpi_optionally_safe_codepath
should be documented at the place where it's declared. But the documentation belongs in the test code... The solution to this conundrum is that the variable should be defined in test code. The library code should call a tracing function through a function pointer (doing nothing if the function pointer is null).This would normally be a blocker. I'm prepared to waive it under time pressure for the 3.6.1 release if we schedule a high-priority follow-up with the assurance that it will not get forgotten on the big tech debt heap.
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.
We could declare that the 3.6.1 EPIC can't be closed until that follow-up is done, so that we have to do the follow-up before the end of the quarter.
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.
Reopened the followup to address this: #9493