Re: [PATCH v3 0/4] Use AF_ALG in checksum utilities

[Assaf Gordon]

Hello Bruno and all,

On Sun, May 06, 2018 at 12:31:36PM +0200, Bruno Haible wrote: 
> Assaf suggested to let it turned off by default, but I prefer to turn it on
> by default because

I'd still suggest turning it off by default to be more conservative.

Few things to consider:

>   * All known past bugs of this API are with empty inputs, and the gnulib
>     code is careful to avoid this scenario.
>   * The crypto code is in the kernel for quite a long time already, thus
>     the likelihood of new kernel bugs is small.

The bug has been lurking for about 6 years. To me it hints that this 
specific kernel code was not well tested in practice or in the real 
world. Other bugs might also be there, and it'll be a shame if 
coreutils (or other gnu software) is the one that will be affected 
from this bug if we end up being the most common user of it.

>   * The feature does not require linking with additional libraries.

True, but it requires a completely different set of syscalls 
(socket/accept/bind/sendfile). Users who use restricted environments 
(e.g. apparmor or other ways to limit syscalls) will be surprised.

>   * It shouldn't be the business of consumers of these modules (e.g. GNU 
> clisp)
>     to think about whether it's safe to enable its use. The module itself
>     (i.e. we as gnulib maintainers) should make the best choice.

I think that it's not the consumer of these modules who need to make 
the decision (e.g. GNU clisp developers), but mostly packagers in 
linux distributions. They are in the best position to decide how they 
want to deploy the programs, together with the kernel they give.

End-users of GNU packages (e.g. clisp/coreutils) who want the highest 
performance can build the packages themselves (which is anyhow how 
it's done today with OpenSSL).

I think that for the same reason that --with-openssl defaults to 'no' 
and not to 'auto' applies here.

>   * At the end of the day, new features are there to be used, not to be 
> ignored.

I agree, but this needs to be balanaced with reliability and the 
principle of "least suprise". The sha1/256/512 in coreutils have been 
ubiquitous on all gnu/linux sytems, and have been "just working" for a 
long time (perhaps slow by default or faster with openssl). It would 
be a shame if we introduce a radical change that can mess this up.

---

Two more things to think about:

1. performance aspect:
I'm not sure there is significant improvement of using af_alg
compared to using OpenSSL, unless one has additional crypto hardware
(which is perhaps more common in embedded systems? but them their
users compile dedicated packages anyhow).

And in fact, with the latest 'bench-sha512', if SIZE is small
but REPETITON is high, afalg takes longer than openssl due to wasted
kernel context switches.

2. Unexpected high %SYS load:
Without AF-ALG, running sha* consumes %USR cpu cycle, as expected.
With AF-ALG, it consumes kernel time (%SYS).
With 1 cpu/core, running sha512 with afalg consumes 100 %SYS resources.
This would be unexpected unless you realize the technical change.

It won't matter to a single user on a desktop, but imagine
a shared server at a university, when all of a sudden a sysadmin
might see a very very high %SYS load - with almost no way to disagnose
what's going on.

Someone replied when this interface was first suggested:
 "doing crypto in kernel for userspace consumers is simply insane."
 https://lwn.net/Articles/410850/
And the only good reason to do so is if the user actually
has a hardware crypto device.

While this is expected and acceptable if one understand how it happens,
it might cause another backlash like the ls quoting thing - perhaps
it would be better to make this change slower and more public.

---

I suggest we keep the default to 'no' at least for the next release of 
coreutils. If we then get positive feedback (or no negative feedback) 
from users who actually tried it, then making it the default might be 
warrented (though I'd still not sure).

---

Lastly, 
few minor code issues:

1.
Compiling this gnulib under coreutils from git with all warnings
shows the following warnings:
---
lib/af_alg.c:136:7: error: jump skips variable initialization 
[-Werror=jump-misses-init]
       goto out_cfd;
       ^~~~
lib/sha512.c:187:1: error: no previous declaration for 'shaxxx_stream' 
[-Werror=missing-declarations]
 shaxxx_stream (FILE *stream, char const *alg, void *resblock,
 ^~~~~~~~~~~~~
---

2.
If I'm not mistaken, running
   "./configure --with-openssl=yes"
Still detects and uses crypto API (but also links with libcrypto).
Only "--with-openssl=yes --without-linux-crypto" disables it.
I think this is quite unexpected, perhaps even a regression:
if someone explicitly asked to use OpenSSL, they certainly don't
want to not use it... 

3.
To improve the comments, I think the linux kernel bug was introduced here:
  commit   fe869cdb89c95d060c77eea20204d6c91f233b53
  Date:    Tue Oct 19 21:23:00 2010 +0800
  Subject:   crypto: algif_hash - User-space interface for hash operations

Then fixed in:
  Commit 493b2ed3f7603a15ff738553384d5a4510ffeb95
  Date:   Thu Sep 1 17:16:44 2016 +0800
  Subject: crypto: algif_hash - Handle NULL hashes correctly

And the current link in the gnulib comments refers to this commit,
which fixes a NULL dereference (important but not directly related):
  commit   a8348bca2944d397a528772f5c0ccb47a8b58af4
  Date:    Thu Nov 17 22:07:58 2016 +0800
  Subject: crypto: algif_hash - Fix NULL hash crash with shash



(This email became quite long, so thanks for reading so far.)
regards,
 - assaf