On Wed, Apr 25, 2018 at 7:34 PM, Assaf Gordon <assafgor...@gmail.com> wrote:
> Hello Matteo,
>
> On Wed, Apr 25, 2018 at 01:26:08PM +0200, Matteo Croce wrote:
>> Linux supports accessing kernel crypto API via AF_ALG since
>> version 2.6.38. Coreutils uses libcrypto when available and fallbacks to
>> generic C implementation of various hashing functions.
>
> Not exactly: coreutils (and every project which uses gnulib's md5/sha*
> modules)
> defaults to using the generic C implementation.
> gnulib automatically adds the option "--with-openssl" to "./configure",
> which can be set explicitly to 'yes' or 'auto'.
>
> To be conservative, I would suggest following the same method:
> Add a "configure" flag instead of enabling a completely new interface
> by default for every downstream project,
> especially that it is tied to external components (the kernel / modules /
> etc).
>
> If a distribution wants to enable by default, they can do it
> for their package (and ensure it works well with their kernel).
>
Hi Assaf,
I will have a look at the autotools to add such option to configure
and check for something like HAVE_LINUX_CRYPTO
>> Use afalg_stream() only in sha1sum for now, but other hashes are possible.
>> The speed gain really depends on the CPU type, on systems which doesn't use
>> libcrypto ranges from ~10% to 320%.
>
> It would be beneficial to know the improvements against coreutils+libcrypto
> on the same CPUs, because if users build with "--openssl=yes" (which is
> currently
> considered the "fast" implementation), they would not benefit from your patch.
> So they'll need to decide which one is preferable (and OpenSSL/libreSSL is
> faster
> on more CPUs and OSs, not just linux on a subset of CPUs).
>
I did comparations against libcrypto too, the results depends on the
CPU and file size but generally libcrypto and af_alg are paragonable.
Of course they are way faster than the generic C implementation.
on x86_64 and file smaller than 2 GB af_alg is slightly faster than
libcrypto, probably because of the sendfile usage:
$ grep -m1 '^model name' /proc/cpuinfo
model name : Intel(R) Core(TM) i7-6700K CPU @ 4.00GHz
$ uname -a
Linux turbo 4.16.2 #71 SMP Sat Apr 21 20:14:08 CEST 2018 x86_64 x86_64
x86_64 GNU/Linux
$ time ./sha1sum-plain 2g.bin
752ef2367f479e79e4f0cded9c270c2890506ab0 2g.bin
real 0m2,718s
user 0m2,548s
sys 0m0,170s
$ time ./sha1sum-libcrypto 2g.bin
752ef2367f479e79e4f0cded9c270c2890506ab0 2g.bin
real 0m1,707s
user 0m1,538s
sys 0m0,170s
$ time ./sha1sum-afalg 2g.bin
752ef2367f479e79e4f0cded9c270c2890506ab0 2g.bin
real 0m1,680s
user 0m0,000s
sys 0m1,668s
on x86_64 and huge files, af_alg is slightly slower than libcrypto,
probably because two syscalls are needed per block:
$ time ./sha1sum-plain 16g.bin
6b9df4760e7308efe6d9def1293b1653ff24ace1 16g.bin
real 0m22,410s
user 0m20,159s
sys 0m2,250s
$ time ./sha1sum-libcrypto 16g.bin
6b9df4760e7308efe6d9def1293b1653ff24ace1 16g.bin
real 0m13,873s
user 0m12,263s
sys 0m1,610s
$ time ./sha1sum-afalg 16g.bin
6b9df4760e7308efe6d9def1293b1653ff24ace1 16g.bin
real 0m14,124s
user 0m0,080s
sys 0m14,044s
I have similar results on an arm64 board running a quad core Cortex
A72, small file:
$ cat /proc/cpuinfo
processor : 0
BogoMIPS : 50.00
Features : fp asimd evtstrm aes pmull sha1 sha2 crc32 cpuid
CPU implementer : 0x41
CPU architecture: 8
CPU variant : 0x0
CPU part : 0xd08
CPU revision : 1
$ uname -a
Linux macchiatobin 4.16.0 #8 SMP Sat Apr 21 17:55:12 UTC 2018 aarch64
aarch64 aarch64 GNU/Linux
$ time ./sha1sum-plain 2g.bin
752ef2367f479e79e4f0cded9c270c2890506ab0 2g.bin
real 0m9.348s
user 0m8.176s
sys 0m1.171s
$ time ./sha1sum-libcrypto 2g.bin
752ef2367f479e79e4f0cded9c270c2890506ab0 2g.bin
real 0m2.232s
user 0m1.721s
sys 0m0.510s
$ time ./sha1sum-afalg 2g.bin
752ef2367f479e79e4f0cded9c270c2890506ab0 2g.bin
real 0m2.219s
user 0m0.000s
sys 0m2.219s
and a bigger one:
$ time ./sha1sum-plain 16g.bin
6b9df4760e7308efe6d9def1293b1653ff24ace1 16g.bin
real 1m14.968s
user 1m5.860s
sys 0m9.106s
$ time ./sha1sum-libcrypto 16g.bin
6b9df4760e7308efe6d9def1293b1653ff24ace1 16g.bin
real 0m20.706s
user 0m14.674s
sys 0m6.001s
$ time ./sha1sum-afalg 16g.bin
6b9df4760e7308efe6d9def1293b1653ff24ace1 16g.bin
real 0m21.608s
user 0m0.142s
sys 0m21.462s
Unfortunately I don't have a board with a crypto HW engine available,
but given the speed of such chips, the results will vary a lot in
favour of af_alg.
Also, keep in mind that some distribution don't want or can't use
libssl for some reasons (licensing, firmware footprint, etc.)
>> This is a test on a Intel(R) Xeon(R) CPU E3-1265L V2 and Debian stretch:
>>
>> $ truncate -s 2GB 2g.bin
>> $ time sha1sum 2g.bin
>> 752ef2367f479e79e4f0cded9c270c2890506ab0 2g.bin
>>
>> real 0m4.829s
>> user 0m4.437s
>> sys 0m0.391s
>> $ time ./sha1sum-afalg 2g.bin
>> 752ef2367f479e79e4f0cded9c270c2890506ab0 2g.bin
>>
>> real 0m3.164s
>> user 0m0.000s
>> sys 0m3.162s
>
> Regarding the above measurement:
> On most modern filesystems "truncate -s" would create a sparse file,
> requiring very little I/O.
> And if the measurements above are indeed mostly CPU-bound
> and not IO-bound, it indicates that hasing in the kernel
> is only somewhat faster then in user-mode, but not significantly so
> if you exclude I/O (not as dramatic difference as measured in
> your previous email).
>
That was intended behaviour. Reading from a slow disk would have
reported similar times between versions.
> This also brings the question: in the previous emails you report
> the improved *-afalg versions after the default versions - did
> you clear the cache to exclude I/O effects?
> (e.g. "sync; echo 3 > /proc/sys/vm/drop_caches")
>
as before, that would clear the block cache and the I/O could be a
bottleneck, probably not what we want.
>> + {
>> + /* sendfile() not possible, do a classic read-write loop */
>> + while ((size = fread (buf, 1, sizeof (buf), stream)))
>> + {
>> + if (send (ofd, buf, size, size == sizeof (buf) ? MSG_MORE : 0) ==
>> -1)
>> + {
>
> Few comments:
> 1. In the above loop, wouldn't a file that's an exact multiple of BLOCKSIZE
> never be sent 0 as the 4th paramater?
> And if so, assuming hashing still works because "MSG_MORE" is just a hint,
> why not always send MSG_MORE ?
>
I tried it before posting the patch, it seems that doing the last
write with MSG_MORE still generates a valid hash. As you guessed, it's
just an hint:
$ dd status=none if=/dev/zero bs=32k count=4 |sha1sum
67dfd19f3eb3649d6f3f6631e44d0bd36b8d8d19 -
$ dd status=none if=/dev/zero bs=32k count=4 |strace -e trace=%network
./sha1sum-afalg
socket(AF_ALG, SOCK_SEQPACKET, 0) = 3
bind(3, {sa_family=AF_ALG,
sa_data="hash\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0sha1\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"},
88) = 0
accept(3, NULL, NULL) = 4
sendto(4, "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"...,
32768, MSG_MORE, NULL, 0) = 32768
sendto(4, "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"...,
32768, MSG_MORE, NULL, 0) = 32768
sendto(4, "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"...,
32768, MSG_MORE, NULL, 0) = 32768
sendto(4, "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"...,
32768, MSG_MORE, NULL, 0) = 32768
67dfd19f3eb3649d6f3f6631e44d0bd36b8d8d19 -
+++ exited with 0 +++
Yes, we can always pass MSG_MORE to the kernel, the hash will
correctly generated when calling read()
> 2. The code for sha*_stream() functions is a bit more elaborate,
> and checks ferror/feof to accomodate EAGAIN - it might be useful
> to replicate it here.
>
>> + if (size != hashlen)
>> + {
>> + fprintf (stderr, "Error from read (%zd vs %zd bytes): %s\n",
>> + size, hashlen, strerror (errno));
>> + ret = -EIO;
>
> The fprintf might be problematic:
> First, this is a cryptic error message, in the sense that
> it indicate an I/O error (hinted by the word "read"),
> but is actually a kernel crypto/driver issue.
> If a user sees this message, there's no chance they'll know
> what the problem is. It's better to be more explicit that
> this relates to kernel crypto API problem.
>
Indeed, it should be reworded better.
> Second, all other sha*_stream functions never output anything
> to STDERR. Their interface is returning 0 for success, 1 for failure,
> and setting errno for failure. Not sure if this is an official API
> agreement or just de-facto, but it means that users of sha*_stream
> functions should now be aware there might be output.
>
> Lastly, if the decision is to keep the error message,
> it's likely better to use gnulib's error() function, like so:
>
> error (0, errno, _("some error message %zd"), size);
>
> Which will take care of strerror, and also enable translation.
>
I wasn't sure if put the error message or not. I think that a library
should not clobber the output with text, probably I did a mistake,
returning 1 and set errno seems a better idea.
> ----
>
> On more general note, while the Crypto API is available since version 2.6.38,
> There are still some issues popping here and there.
> for example:
> https://security-tracker.debian.org/tracker/CVE-2016-8646
> https://bugzilla.redhat.com/show_bug.cgi?id=1395896
>
That's bad. I'm not a security expert at all, but I did a quick search
and I've found that also libcrypt had some CVEs.
Nobody's perfect :)
> Another example, the libkcapi [1] library (referenced from the kernel crypto
> api documentation)
> contains a work-around for a bug in kernels pre-4.9 [2].
> I don't know if this is relevant for your implementation or not,
> but if it is, it's worth checking for this and other issues.
> [1] http://www.chronox.de/libkcapi.html
> [2]
> https://github.com/smuellerDD/libkcapi/commit/b8d5941addb15fe5a716eef24060fbd306c06ec9
>
That's bad too. In this case probably the distribution kernel
maintainers should backport such fix anyway, regardless of af_alg
support in coreutils.
> It also seems OpenSSL version 1.1.0 added support for AF_ALG.
> Perhaps it's better to leave it to them (since gnulib can already
> use openssl easily)?
>
I've just checked the openssl version on my two reference systems
which are an arm64 Ubuntu 18.04:
$ dpkg -l libssl1.1 |grep ^ii
ii libssl1.1:arm64 1.1.0g-2ubuntu4 arm64
and an x86_64 Fedora 27:
$ rpm -q openssl-libs
openssl-libs-1.1.0h-3.fc27.x86_64
strace revealed that none of them seems to use af_alg, at least in my
tests. Probably the package maintainer had to enable it?
Regards,
--
Matteo Croce
per aspera ad upstream
