Paths

Table of Contentst

Differential D8236

improve PBKDF2 performance
ClosedPublic
Actions

Authored by allanjude on Oct 13 2016, 1:59 AM.

Tags

None

Referenced Files

	F81614514: D8236.diff
	Fri, Apr 19, 12:04 AM

	Unknown Object (File)
	Fri, Apr 12, 8:37 PM

	Unknown Object (File)
	Mon, Apr 8, 6:51 AM

	Unknown Object (File)
	Sun, Apr 7, 5:04 PM

	Unknown Object (File)
	Thu, Apr 4, 2:18 PM

	Unknown Object (File)
	Wed, Apr 3, 7:11 AM

	Unknown Object (File)
	Tue, Apr 2, 2:32 PM

	Unknown Object (File)
	Sun, Mar 31, 2:32 PM

Subscribers

Details

Reviewers

emaste
jmg
delphij
ed
pjd
oshogbo

Group Reviewers

Commits

rS327856: MFC r313962, r313972-r313973, r315230
rS313962: improve PBKDF2 performance

Summary

The PBKDF2 in sys/geom/eli/pkcs5v2.c is around half the speed than it could be.
https://jbp.io/2015/08/11/pbkdf2-performance-matters/

Submitted by: Joe Pixton
Original Patch by: jmg

I cleaned up the patch to solve issues from files that have moved in the mean time

PR: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=202365

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Skipped

Unit

No Test Coverage

Build Status

Buildable 5583
Build 5837: CI src build	Jenkins
Build 5836: arc lint + arc unit

Event Timeline

allanjude updated this revision to Diff 21336.Oct 13 2016, 1:59 AM

allanjude retitled this revision from to improve PBKDF2 performance.

allanjude updated this object.

allanjude edited the test plan for this revision. (Show Details)

allanjude added reviewers: secteam, jmg, pjd, delphij, emaste, ed, oshogbo.

Herald added a subscriber: imp. · View Herald TranscriptOct 13 2016, 1:59 AM

allanjude updated this object.Oct 13 2016, 2:00 AM

I'm just picky today. ;-)

tests/sys/geom/eli/pbkdf2/gentestvect.py
8	Do we need this comment? I know what you do there and I leave breadcrumbs in my code for myself sometimes, too. ;-)

Looks good.

Quick question: the tests that are added by this commit are actually independent of the change to GELI itself, right? If so, could we please have them committed separately? Thanks!

tests/sys/geom/eli/pbkdf2/gentestvect.py
3	As `pbkdf2_hmac` is only used once in this Python script, you can also just remove the import and use `hashlib.pbkdf2_hmac` instead.
20	Rule of thumb: when concatenating more than two strings, just use `%` or `.format()`: return '"%s"' % result As Python is an interpreted language, performance is typically paid by the operation. String formatting is therefore cheaper than concatenating many strings.
26	What's wrong with Python's own RNG functions? :-)

This revision is now accepted and ready to land.Oct 13 2016, 9:26 AM

allanjude added inline comments.Oct 18 2016, 2:26 AM

tests/sys/geom/eli/pbkdf2/gentestvect.py
8	this bit of code is not mine, it was written by jmg@
26	I am just integrating the code from the PR. We'd need to query jmg@ about the specifics.

ed added inline comments.Oct 18 2016, 6:42 AM

tests/sys/geom/eli/pbkdf2/gentestvect.py
1	By the way, I've noticed that this Python script is formatted like `style(9)`. I think that's not something we should try to do for non-C languages. Could we please follow the PEP-8 style? https://www.python.org/dev/peps/pep-0008/ The https://pypi.python.org/pypi/autopep8 tool can help you to do the formatting automatically.

Looks otherwise good to me. Could you please fix or comment with the reasoning for the comment inline?

sys/geom/eli/pkcs5v2.c
85	Since we already have explicit_bzero now and the intention is to wipe sensitive data here, it should be used (unless you have really compelling reasons not to do so).

This revision now requires changes to proceed.Oct 18 2016, 6:54 AM

pjd accepted this revision.Feb 3 2017, 9:36 PM

pjd edited edge metadata.

Switch the bzero's in pkcs5v2_genkey to explicit_bzero because they are wiping sensitive data

As requested by delphij

allanjude marked 5 inline comments as done.Feb 19 2017, 5:56 AM

Thanks!

This revision is now accepted and ready to land.Feb 19 2017, 6:15 AM

Closed by commit rS313962: improve PBKDF2 performance (authored by allanjude). · Explain WhyFeb 19 2017, 7:30 PM

This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Path

Size

etc/

mtree/

4 lines

sys/

boot/

geli/

4 lines

geom/

eli/

4 lines

39 lines

20 lines

tests/

sys/

geom/

1 line

9 lines

28 lines

62 lines

41 lines

1136 lines

Diff 21336

etc/mtree/BSD.tests.dist

Loading...

sys/boot/geli/Makefile

Loading...

sys/geom/eli/g_eli.h

Loading...

sys/geom/eli/g_eli_hmac.c

Loading...

sys/geom/eli/pkcs5v2.c

Loading...

tests/sys/geom/Makefile

Loading...

tests/sys/geom/eli/

Loading...

tests/sys/geom/eli/Makefile

Loading...

tests/sys/geom/eli/pbkdf2/

Loading...

tests/sys/geom/eli/pbkdf2/Makefile

Loading...

tests/sys/geom/eli/pbkdf2/gentestvect.py

Loading...

tests/sys/geom/eli/pbkdf2/hmactest.c

Loading...

tests/sys/geom/eli/pbkdf2/testvect.h

Loading...