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
[On Hold] Add support for builds without assembly. #256
Conversation
See #199, in particular this comment for more background on this. One thing different from there is I left the digest module in for no_asm builds. It occurs in too many places, and since it will be fixed imminently by #199, didn't seem worth trying to be clever. |
83f2e49
to
b7570c6
Compare
Coverage decreased (-0.5%) to 88.355% when pulling b7570c6cfc84ea074b0d758cd0108d5d7af471bb on samscott89:no_asm into 2c3a95e on briansmith:master. |
7329c18
to
ef27d50
Compare
This looks great. It will be at least a couple of days before I can get to this review. |
Coverage decreased (-0.001%) to 89.924% when pulling 4ac34e8b85631bb906c937317edfe45346913019 on samscott89:no_asm into 08c729e on briansmith:master. |
src/digest/digest.rs
Outdated
block_data_order: unsafe extern fn(state: &mut [u64; MAX_CHAINING_LEN / 8], data: *const u8, | ||
num: c::size_t), | ||
#[cfg(feature="no_asm")] | ||
block_data_order: unsafe fn(state: &mut [u64; MAX_CHAINING_LEN / 8], data: *const u8, |
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.
In the no_asm case, we could actually make block_data_order
a normal, non-unsafe
function, right? In the SHA-1 code, we could skip over the unsafe -> safe adapter function, for example.
This all looks good to me. I'll land it when the Rust SHA-2 code is landed. |
One question: You chose little-endian just because there's places where we have little-endian logic and no big-endian counterpart, right? Or, was there some other reason? |
Correct. Although since the
Wouldn't be a huge amount of additional work. The main reason I opted to keep it marked as unsafe was to be compatible with the |
e8e7487
to
f6a1689
Compare
Ok, made the suggested changes. I re-factored |
0bd2295
to
80994dd
Compare
Coverage decreased (-0.3%) to 89.634% when pulling 80994ddffac0f189a92e362ec273ffbb23ed11a0 on samscott89:no_asm into f87e2e8 on briansmith:master. |
Coverage decreased (-0.3%) to 89.634% when pulling 80994ddffac0f189a92e362ec273ffbb23ed11a0 on samscott89:no_asm into f87e2e8 on briansmith:master. |
Coverage decreased (-0.3%) to 89.634% when pulling 80994ddffac0f189a92e362ec273ffbb23ed11a0 on samscott89:no_asm into f87e2e8 on briansmith:master. |
The simple fact of the matter is that Cargo features that reduce API surface when enabled break Cargo. The two solutions are to make it increase API surface instead ( After all, if the crate you depend on that needs the assembly does so because of the API it causes to be exposed, disabling it will cause spooky build failures at a distance. This is, I think, kind of unacceptable. As a user, I would consider that a huge mark against using this crate - it does not cooperate well with the ecosystem. Such build failures would be attributed to other, downstream crates, not to ring, and would cause nontrivial work for those having to deal with them. |
Keep in mind that the long-term goal is API parity but we'll need to deal with interim states where we don't have API parity.
No library crate should be using the Here are the requirements, as I see them: I have a feeling that there's not a 100% perfect and we'll have to make compromises. I'm open to considering any solution that has a solution that meets the above requirements. |
Yes. Specifically, library
These are covered by making it a default feature.
This is covered by opting out of default features. Note that trying to build a In particular, Either platform-dependent features or scenarios would further improve the situation, allowing Cargo to error out up-front, before building anything. However, those only work with
This either requires that even in the |
Thanks. The problem that causes this is that ring doesn't expose its entire API in There is a serious problem with defining an "asm" feature and then having people who don't want asm code to use So, I feel comfortable right now with the following plan:
Obviously this isn't ideal but the problem is that "asm" vs "no-asm" isn't really a feature choice, but the feature mechanism only way we have to control how ring is built. |
Er, I think you misunderstand what I meant regarding It's not something you build with, it's a dependency constraint you opt out of in Cargo.toml. An example from the Cargo docs: [dependencies.awesome]
version = "1.3.5"
default-features = false # do not include the default features, and optionally
# cherry-pick individual features
features = ["secure-password", "civet"] As a result, your reasoning in that entire second paragraph is erroneous. If any dependent does not opt-out of ring's default features (because it's per-dependency) in its own Cargo.toml, then Similarly, if any dependent enables All it takes is one crate pulled in arbitrarily deep in the dependency graph enabling the "experimental" To build on the example I gave before:
At this point, Even if someone patched them to only use Moreover, the user has to already know ring is at fault in order to even start looking for It's poor citizenship. |
I see you understand what I meant regarding If some crate enables the no-asm feature then that's an unsupported configuration and so I don't care what happens in that situation. If there's an alternative besides using feature flags for doing this, I'd love to use it. We need to find a solution that doesn't cause
Increase your level of politeness, please. |
I'm sorry; I didn't intend the "poor citizenship" comment as impoliteness - I see it as meaningfully descriptive. The reason is that the burden and cost of non-root crates enabling It creates a negative externality - ring behaving in a manner counter to Cargo best-practices then makes more work for other people, and not even the ones who chose to do something unsupported. Instead, it places the burden on people who use ring perfectly as intended.
In that context, the above statement then does parse as "I'm unconcerned with causing negative externalities in the greater Cargo ecosystem" - something that, to me, is the literal definition of "poor citizenship." |
Please suggest a better alternative considering the constraints I gave above. Right now there doesn't seem to be a better alternative. What you've suggested so far is worse insofar as I understand it. People accidentally misusing the no_asm feature isn't worth worrying about. They could easily name a |
I'm actually curious as to why The fact is, the cases in which one is unsuitable are strongly correlated with the cases where neither is suitable - unsupported platforms and constrained environments. Moreover, opting out of default features doesn't mean the dependent crate can't then re-enable some subset of the defaults. For example, a crate that runs in a no-allocation context on a supported architecture might do this: [dependencies.ring]
default-features = false
features = ["asm"] Similarly, a crate that wants to do RSA on an unsupported platform could do this: [dependencies.ring]
default-features = false
features = ["use_heap"] This is the exact model used by pretty much all crates that support both |
@eternaleye, running with your previous example since it's a more concrete way to discuss these implications. It would be expected that none of However, suppose none of these crates strictly require On the other hand, in the |
This seems to be to be the most pragmatic way forwards. In particular, the second bullet point seems to address @eternaleye's main complaint. I can put together a proof of concept, but presumably the build would fail with an Hopefully cargo will support target-specific features at some point (@eternaleye mentioned them earlier in this thread) so we can enforce |
One problem with this approach is running tests. Since Then, the only other place Probably best to change the semantics in this case. Perhaps |
ce24255
to
67ff009
Compare
Ok, so I've made a new commit based on the above. This instead adds a feature Running Enabling |
Enabling feature `native_rust` Will attempt to build native rust code for all components, falling back to assembly when there is a missing implementation. Can be used to attempt to build on targets without assembly support, though will fail at link time if feature currently requires assembly. Also adds the mipsel-unknown-linux-gnu target to the travis build. I agree to license my contributions to each file under the terms given at the top of each file I changed.
This is incorrect. Any crate can opt out of the default features of individual dependencies, separately. That does explain all the other confusion, though. Basically, Essentially, Combined with Cargo features being additive, this results in features only being enabled if things actually need them. @samscott89 Basically, |
That was my understanding, not necessarily shared by @briansmith.
Sorry, to be clearer I meant the
Let's say |
@samscott89: Yes, and this is the exact same situation as Platform specific features, which I linked to, or scenarios, which I also linked to, would improve the situation when implemented. In addition, there's a strong sentiment against link time errors in the rust community, because they generally are unreadable. They also mean that a lot of stuff gets built uselessly, wasting the developer's time. |
To help me understand: why is |
I'm speaking specifically of the case where, depending on a feature flag, a crate may or may not use code from This is, in fact, exactly how the RSA code that requires allocation works in ring today. The situation matches because, just like ring's assembly code doesn't support some platforms, there are platforms not supported by all of The The It also is useless for assembly-lacking platforms in the middle period, where not everything has been ported. |
To follow up on this: The concerns @eternaleye mentioned are valid but unfortunately Rust simply doesn't give us any features we need to do what @eternaleye suggests and still make progress in a useful way. So I'm perfectly happy to take a PR like this and do things the imperfect way until the Rust developers enable us to do things the perfect way. However, I already hate all the |
Thanks for submitting this, long ago. Now is a good time for us to add support for builds without assembly but the code has diverged so much I think it isn't practical to rebase is PR on the current tree, so I'm going to close this. |
The ARMv8 assembly code in this commit is mostly taken from OpenSSL's `ecp_nistz256-armv8.pl` at https://github.com/openssl/openssl/blob/19e277dd19f2897f6a7b7eb236abe46655e575bf/crypto/ec/asm/ecp_nistz256-armv8.pl (see Note 1), adapting it to the implementation in p256-x86_64.c. Most of the assembly functions found in `crypto/fipsmodule/ec/asm/p256-x86_64-asm.pl` required to support that code have their analogous functions in the imported OpenSSL ARMv8 Perl assembly implementation with the exception of the functions: - ecp_nistz256_select_w5 - ecp_nistz256_select_w7 An implementation for these functions was added. Summary of modifications to the imported code: * Renamed to `p256-armv8-asm.pl` * Modified the location of `arm-xlate.pl` and `arm_arch.h` * Replaced the `scatter-gather subroutines` with `select subroutines`. The `select subroutines` are implemented for ARMv8 similarly to their x86_64 counterparts, `ecp_nistz256_select_w5` and `ecp_nistz256_select_w7`. * `ecp_nistz256_add` is removed because it was conflicting during the static build with the function of the same name in p256-nistz.c. The latter calls another assembly function, `ecp_nistz256_point_add`. * `__ecp_nistz256_add` renamed to `__ecp_nistz256_add_to` to avoid the conflict with the function `ecp_nistz256_add` during the static build. * l. 924 `add sp,sp,#256` the calculation of the constant, 32*(12-4), is not left for the assembler to perform. Other modifications: * `beeu_mod_inverse_vartime()` was implemented for AArch64 in `p256_beeu-armv8-asm.pl` similarly to its implementation in `p256_beeu-x86_64-asm.pl`. * The files containing `p256-x86_64` in their name were renamed to, `p256-nistz` since the functions and tests defined in them are hereby running on ARMv8 as well, if enabled. * Updated `delocate.go` and `delocate.peg` to handle the offset calculation in the assembly instructions. * Regenerated `delocate.peg.go`. Notes: 1- The last commit in the history of the file is in master only, the previous commits are in OpenSSL 3.0.1 2- This change focuses on AArch64 (64-bit architecture of ARMv8). It does not support ARMv4 or ARMv7. Testing the performance on Armv8 platform using -DCMAKE_BUILD_TYPE=Release: Before: ``` Did 2596 ECDH P-256 operations in 1093956us (2373.0 ops/sec) Did 6996 ECDSA P-256 signing operations in 1044630us (6697.1 ops/sec) Did 2970 ECDSA P-256 verify operations in 1084848us (2737.7 ops/sec) ``` After: ``` Did 6699 ECDH P-256 operations in 1091684us (6136.4 ops/sec) Did 20000 ECDSA P-256 signing operations in 1012944us (19744.4 ops/sec) Did 7051 ECDSA P-256 verify operations in 1060000us (6651.9 ops/sec) ``` Change-Id: I9fdef12db365967a9264b5b32c07967b55ea48bd Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/51805 Reviewed-by: Adam Langley <agl@google.com> Commit-Queue: Adam Langley <agl@google.com>
Set feature "no_asm" to use. This currently disables use of
aead, ec, and rsa/signature modules due to missing code.
Also adds the mipsel-unknown-linux-gnu target to the travis
builds. These builds currently fail in anticipation of PR#199.