Page MenuHomeFreeBSD

Replace sys/crypto/sha2/sha2.c with lib/libmd/sha512c.c

Authored by allanjude on Oct 17 2015, 8:52 PM.



cperciva's libmd implementation is 5-30% faster (also seen here: )

cperciva's implementation was lacking SHA-384 which I implemented
Extend sbin/md5 to create sha384(1)

In a separate commit, I will also add SHA-512/256 (required by new ZFS features)

Chase dependancies on sys/crypto/sha2/sha2.{c,h} and replace them with sha512{c.c,.h}

Will extend reviewers to include secteam@ once the first round of issues are dealt with

Test Plan

benchmark results:

The output of the new SHA-384 implementation were verified against openssl sha384 and

Diff Detail

rS FreeBSD src repository
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

allanjude updated this revision to Diff 9476.Oct 17 2015, 8:52 PM
allanjude retitled this revision from to Replace sys/crypto/sha2/sha2.c with lib/libmd/sha512c.c.
allanjude updated this object.
allanjude edited the test plan for this revision. (Show Details)
allanjude added reviewers: jmg, cperciva.
allanjude updated this object.Oct 17 2015, 8:55 PM
allanjude edited edge metadata.
jmg edited edge metadata.Oct 17 2015, 9:14 PM

Just a few comments.

40 ↗(On Diff #9476)

is this tab/space matching others or is this just a phab misformating issue?

559 ↗(On Diff #9476)

I don't see bde using SHA256, we should probably remove it, though maybe as a different commit?

Looks like the module also compiles it. A quick compile of the module does look like it uses it.

561 ↗(On Diff #9476)

I don't think you need zfs here yet. I'd add ZFS when you finally enable sha512/256 support.

allanjude marked an inline comment as done.Oct 17 2015, 9:30 PM
allanjude added inline comments.
40 ↗(On Diff #9476)

i put a tab when I shouldn't have, fixed in my local copy, will update with other changes

559 ↗(On Diff #9476)

yeah, that was already there, I didn't change it. geom_eli uses both 256 and 512, should it be listed in both places?

561 ↗(On Diff #9476)

the zfs SRCS= line references both, and again, I just updated sha2.c -> sha512c.c (and changed the order to make sense). But yeah, I could remove ZFS here and readd it in the next commit.

allanjude updated this revision to Diff 9482.Oct 18 2015, 12:44 AM
allanjude marked an inline comment as done.
allanjude edited edge metadata.

Minor updates to address feedback from jmg@

emaste added a subscriber: emaste.Oct 19 2015, 4:23 PM
jmg added inline comments.Oct 20 2015, 11:34 PM
559 ↗(On Diff #9482)

bde != eli.. They are different, so commenting that eli uses 256 and 512 is unrelated to my bde comment.

561 ↗(On Diff #9482)

Since you're adding the line fresh, it's best to keep it minimal.

allanjude added inline comments.Oct 22 2015, 3:09 PM
559 ↗(On Diff #9482)

that should have been a separate comment.

Unrelated to geom_bde:

geom_eli uses both SHA256 and SHA512, should it be listed in this file?

bapt added a subscriber: bapt.Oct 24 2015, 2:12 PM
allanjude updated this revision to Diff 10551.Nov 28 2015, 7:05 AM
allanjude edited edge metadata.

Update the diff to put sha384 prototypes in their own file as requested by cperciva

cperciva accepted this revision.Dec 1 2015, 7:18 PM
cperciva edited edge metadata.

Looks good to me.

des accepted this revision.Dec 18 2015, 3:30 AM
des added a reviewer: des.
bapt accepted this revision.Dec 24 2015, 1:33 PM
bapt added a reviewer: bapt.
delphij accepted this revision.Dec 27 2015, 5:19 PM
delphij added a reviewer: delphij.

Approved by so, too. Sorry for the delay and thanks for working on this.

This revision is now accepted and ready to land.Dec 27 2015, 5:19 PM
This revision was automatically updated to reflect the committed changes.