All four SHA-2 variants from HACL* #2
Conversation
| EXPORT_SYMBOL(hacl_sha512_finup); | ||
|
|
||
|
|
||
| static struct shash_alg sha2_hacl_algs[4] = { { |
There was a problem hiding this comment.
my suggestion is to break this file into two:
- module boilerplate code (written by hand), which would internally call into below
- autogenerated implementation code
There was a problem hiding this comment.
@ignatk does it make sense to put the implementation code inside /lib/crypto/, and the module file here /crypto/.
There was a problem hiding this comment.
for "pure C cross platform" implementations - yes
There was a problem hiding this comment.
See info in cf-zeta:
https://github.com/cloudflareresearch/linux/tree/cf-zeta/zeta#readme
| EXPORT_SYMBOL(hacl_sha512_finup); | ||
|
|
||
|
|
||
| static struct shash_alg sha2_hacl_algs[4] = { { |
There was a problem hiding this comment.
@ignatk does it make sense to put the implementation code inside /lib/crypto/, and the module file here /crypto/.
crypto/tcrypt.c
Outdated
| @@ -775,14 +775,14 @@ static int test_ahash_cycles_digest(struct ahash_request *req, int blen, | |||
| int ret, i; | |||
|
|
|||
| /* Warm-up run. */ | |||
| for (i = 0; i < 4; i++) { | |||
| for (i = 0; i < 16; i++) { | |||
There was a problem hiding this comment.
modifications to this file should be in another PR. Also not sure they are needed.
There was a problem hiding this comment.
Undid these changes in this PR, but we note that without them the performance measurements are very unstable.
There was a problem hiding this comment.
Let's see if passing sec=2 helps, this was added in the CI execution.
franziskuskiefer
force-pushed
the
hacl-sha2
branch
from
a3d680b to
d76cc2d
Compare
|
I've added three commits, one is about the license identifiers, multiple file compilation, and manually loading the new module. |
|
@franziskuskiefer Is it possible to tweak the tool that generates code? I'm beginning the process of at least putting together the SHA2 algos, and we have some initial formatting results. If you're able to, it would be nice to comb through these and update the styles for This was generated with the following: I have this in my settings to automatically pickup commit style issues: I can translate this to a github worker/action to catch these style problems sooner. |
|
I confirmed that Are we intending for other modules to use this module? We need a header file in |
It's certainly possible to tweak things. But what exactly are the issues? Can you list some specific issues? Then we can look into it. Skimming the file I see formatting issues. Formatting has been done as instructed. But that's not really a code issue and can be easily fixed by running the tools to get what you want. |
This will be more obvious in the code samples below, but things like: and Granted, there was some cleanup work in previous commits with clang-format such as: b0e0339 but mentioned below, it wont fix all checkpatch problems.
Yes and no. I couldn't get ➜ zeta git:(fred/sha2) ✗ git show diff --staged
diff --git a/crypto/sha2-hacl.c b/crypto/sha2-hacl.c
index d5339c93949e..50c52eb84742 100644
--- a/crypto/sha2-hacl.c
+++ b/crypto/sha2-hacl.c
@@ -9,11 +9,12 @@
#include <crypto/sha512_base.h>
#include "hacl_hash.h"
-#include "hacl_lib.h"
+ #include "hacl_lib.h"
int hacl_sha256_update(struct shash_desc *desc, const u8 *data,
unsigned int len)
{
+ uint32_t test;
struct sha256_state *sctx = shash_desc_ctx(desc);
struct Hacl_Streaming_MD_state_32_s st;
st.block_state = sctx->state;
➜ zeta git:(fred/sha2) ✗ git commit -m 'bad format'
executing checkpatch
No codespell typos will be found - file '/usr/share/codespell/dictionary.txt': No such file or directory
WARNING: Missing commit description - Add an appropriate one
CHECK: Prefer kernel type 'u32' over 'uint32_t'
#21: FILE: crypto/sha2-hacl.c:17:
+ uint32_t test;
ERROR: Missing Signed-off-by: line(s)
total: 1 errors, 1 warnings, 1 checks, 13 lines checked
NOTE: For some of the reported defects, checkpatch may be able to
mechanically convert to the typical style using --fix or --fix-inplace.
"[PATCH] bad format" has style problems, please review.
NOTE: If any of the errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.
[fred/sha2 47aba1dbb9fb] bad format
1 file changed, 2 insertions(+), 1 deletion(-)
➜ zeta git:(fred/sha2) git status
On branch fred/sha2
nothing to commit, working tree clean
➜ zeta git:(fred/sha2) ✗ clang-format -i -style=file:zeta/.clang-format ./crypto/sha2-hacl.c
diff --git a/crypto/sha2-hacl.c b/crypto/sha2-hacl.c
index 50c52eb84742..0cccab1753bb 100644
--- a/crypto/sha2-hacl.c
+++ b/crypto/sha2-hacl.c
@@ -9,12 +9,12 @@
#include <crypto/sha512_base.h>
#include "hacl_hash.h"
- #include "hacl_lib.h"
+#include "hacl_lib.h"
int hacl_sha256_update(struct shash_desc *desc, const u8 *data,
unsigned int len)
{
- uint32_t test;
+ uint32_t test;
struct sha256_state *sctx = shash_desc_ctx(desc);
struct Hacl_Streaming_MD_state_32_s st;
st.block_state = sctx->state;
However, when it comes to That's not necessarily the case: Therefore, the comments were not handled by clang-format. So it would be nice if the code generator can fix somethings up. I'll squash everything and then run through a more realistic checkpatch to see what is specifically left. My previous comment pointed out everything per commit due to the rebase, and that will have out-dated information. With that said, it does provide a very clear indication of what upstream is expecting with these changes. |
|
@franziskuskiefer Sorry for the long wait (busy day). I went ahead and squashed the commits, and reran checkpatch on everything and the results are in: I then ran a diff --git a/crypto/hacl_hash.h b/crypto/hacl_hash.h
index 80d8dd4ef224..74746bc84b60 100644
--- a/crypto/hacl_hash.h
+++ b/crypto/hacl_hash.h
@@ -223,4 +223,4 @@ void Hacl_Streaming_SHA2_finish_384(struct Hacl_Streaming_MD_state_64_s *p,
void Hacl_Streaming_SHA2_hash_384(uint8_t *input, uint32_t input_len,
uint8_t *dst);
-#endif // CRYPTO_HACL_HASH_H_
+#endif // CRYPTO_HACL_HASH_H_
diff --git a/crypto/hacl_lib.h b/crypto/hacl_lib.h
index 12a9a3c2fcac..de645982039f 100644
--- a/crypto/hacl_lib.h
+++ b/crypto/hacl_lib.h
@@ -231,4 +231,4 @@ static inline void store128_be(u8 *buf, u128 x)
#define KRML_MAYBE_FOR16(i, z, n, k, x) KRML_ACTUAL_FOR(i, z, n, k, x)
#endif
-#endif // CRYPTO_HACL_LIB_H_
+#endif // CRYPTO_HACL_LIB_H_
Let me know if this helps clarify what's missing formatting/patch wise. If we can get most of these handled by the code-generator, we'd be in a good spot with the other PR's as well. When I have some more cycles, I'll think about how to incorporate this into the build pipeline. I did try executing some of the automatic cleanup tools the kernel provides: ./scripts/cleanpatch, ./scripts/cleanfile, and --fix-inline with checkpatch. These didn't quite work out the way I was hoping. It covered some of the whitespace issues, but that's about it. It's probably better to address these issues in the code generator for repeatable builds. Lastly, regarding #2 (comment) I'm about to merge in #9 which will cover the lingering compile cases. (And I'll be rebasing all the PRs too) I'm going to make the assumption that we do want to export these functions to other modules, so I'll try to include a header file to hopefully fix the compile. |
No description provided.