The experiment to consolidate some crypto functions, shared between zfs/geli.
AcceptedPublic

Authored by tsoome on Aug 22 2016, 10:09 PM.

Details

Summary

The goal is to reduce code duplication in loader by creating a separate
instance of crypto library, which can be linked if either zfs or geli
is needed. Incidentally it also resulted in some related changes, such as
geli + zfsboot (totally untested however). Also there is still some
confusion about _STAND and _STANDALONE defines, which probably will need
some attention. Also there seems to be some confusion about bsd.stand.mk
use cases.

The resulting zfsloader binary is only slightly reduced in size:
-r-xr-xr-x 1 root wheel 446464 Aug 19 08:46 zfsloader
-rw-r--r-- 1 root wheel 438272 Aug 23 00:30 zfsloader.b

in this sample, the zfsloader.b is built based on this work. Since only
sha256/sha512-256 code was duplicated, the result is quite good, I think.

There also seems to be some sort of issue preventing removal of unused
symbols from resulting binaries, the specific case was observed regarding
SHA* functions, eventually I just used preprocessor to exclude unused ones.

Most likely I have missed some related parts, only tested x86 build.

Test Plan

Tested with sha256/sha512 checksum set on zfs datasets.

The current best size for zfsloader with -Os is:
-rw-r--r-- 1 root wheel 380928 Aug 23 19:55 zfsloader

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 9675
Build 10115: arc lint + arc unit
tsoome retitled this revision from to The experiment to consolidate some crypto functions, shared between zfs/geli..Aug 22 2016, 10:09 PM
tsoome updated this object.
tsoome edited the test plan for this revision. (Show Details)
tsoome added subscribers: allanjude, imp.
emaste added a subscriber: emaste.Aug 23 2016, 4:15 PM
tsoome updated this revision to Diff 19589.Aug 23 2016, 4:53 PM

I hope I got most makefiles right now, bsd.stand.mk defaults to use -Os,
-Oz did break ficl (i think).
Updated to revision 304689.

tsoome edited the test plan for this revision. (Show Details)Aug 23 2016, 4:57 PM
avg added a subscriber: avg.Aug 23 2016, 5:24 PM
tsoome updated this revision to Diff 19614.Aug 24 2016, 6:40 AM
tsoome edited the test plan for this revision. (Show Details)

zfsboot has leftover libbootcrypto reference in Makefile

allanjude removed a subscriber: allanjude.
tsoome updated this revision to Diff 21122.EditedOct 6 2016, 6:21 PM

Updated to revision 306775.

resulting file sizes:
tsoome@freebsd:/usr/obj/usr/src/sys/boot % ls -l i386/zfsloader/zfsloader efi/boot1/boot1.efi efi/loader/loader.efi userboot/userboot/userboot.so
-rwxr-xr-x 1 root wheel 117248 Oct 6 21:10 efi/boot1/boot1.efi
-rwxr-xr-x 1 root wheel 414720 Oct 6 21:10 efi/loader/loader.efi
-rw-r--r-- 1 root wheel 380928 Oct 6 21:11 i386/zfsloader/zfsloader
-rwxr-xr-x 1 root wheel 376128 Oct 6 21:11 userboot/userboot/userboot.so
tsoome@freebsd:/usr/obj/usr/src/sys/boot %

tsoome updated this revision to Diff 21857.Oct 31 2016, 10:41 PM

Updated to revision 308153.

If you have time, could you refresh this, I'd like to look at it again.

share/mk/bsd.stand.mk
8

could we maybe also do CFLAGS.clang+= -Oz

tsoome updated this revision to Diff 29225.Jun 5 2017, 6:06 PM

Updated to revision 319604.

delphij accepted this revision.Jun 5 2017, 8:21 PM

Thanks for the work! (I thought this was already landed last year :)

The change looks good to me (and I appreciate it as a step forward -- duplicated copies of cryptographic code is a big headache for us) overall.

(Totally optional: I wonder if the cryptoboot library could be folded into libstand? Or is there some specific reasons we have to create a new library instead? I'm asking because doing so would avoid building it multiple times, and the license of the source are all BSD compatible.)

This revision is now accepted and ready to land.Jun 5 2017, 8:21 PM
tsoome added a comment.Jun 5 2017, 9:06 PM

Thanks for the work! (I thought this was already landed last year :)

The change looks good to me (and I appreciate it as a step forward -- duplicated copies of cryptographic code is a big headache for us) overall.

(Totally optional: I wonder if the cryptoboot library could be folded into libstand? Or is there some specific reasons we have to create a new library instead? I'm asking because doing so would avoid building it multiple times, and the license of the source are all BSD compatible.)

I did not want to push it too hastily, one thing is the same concern about if it is good to have separate lib - the problem there is all this mess with options to pick/drop the features and for first step I just wanted to make it (libcryptoboot) very visible, so that maybe some better ideas will pop. The second issue is really about the build options (-Os versus -Oz). And then we have the alternate platforms next to x86:)

So in an sense, it still may be good idea to split this patch more. The libstand by itself does not really help about multiple builds, because we need 32-bit for bios and 64 (+32 in future?) for UEFI, + one for userboot (bhyve). But in any case, I did name it an experiment for purpose:)