[PATCH v3 3/4] sha512sum: use kernel crypto API

[Matteo Croce]

Use AF_ALG for sha384 and sha512 too

Signed-off-by: Matteo Croce <***@redhat.com>
---
lib/sha512.c | 32 ++++++++++++++++++++++++++++++--
modules/crypto/sha512 | 6 +++++-
2 files changed, 35 insertions(+), 3 deletions(-)

diff --git a/lib/sha512.c b/lib/sha512.c
index 8a6dd4e83..72e5fdd24 100644
--- a/lib/sha512.c
+++ b/lib/sha512.c
@@ -32,6 +32,10 @@
#include <stdlib.h>
#include <string.h>

+#ifdef HAVE_LINUX_IF_ALG_H
+# include "af_alg.h"
+#endif
+
#if USE_UNLOCKED_IO
# include "unlocked-io.h"
#endif
@@ -185,8 +189,20 @@ sha512_stream (FILE *stream, void *resblock)
{
struct sha512_ctx ctx;
size_t sum;
+ char *buffer;
+
+#ifdef HAVE_LINUX_IF_ALG_H
+ int ret;
+
+ ret = afalg_stream(stream, resblock, "sha512", SHA512_DIGEST_SIZE);
+ if (!ret)
+ return 0;
+
+ if (ret == -EIO)
+ return 1;
+#endif

- char *buffer = malloc (BLOCKSIZE + 72);
+ buffer = malloc (BLOCKSIZE + 72);
if (!buffer)
return 1;

@@ -256,8 +272,20 @@ sha384_stream (FILE *stream, void *resblock)
{
struct sha512_ctx ctx;
size_t sum;
+ char *buffer;
+
+#ifdef HAVE_LINUX_IF_ALG_H
+ int ret;
+
+ ret = afalg_stream(stream, resblock, "sha384", SHA384_DIGEST_SIZE);
+ if (!ret)
+ return 0;
+
+ if (ret == -EIO)
+ return 1;
+#endif

- char *buffer = malloc (BLOCKSIZE + 72);
+ buffer = malloc (BLOCKSIZE + 72);
if (!buffer)
return 1;

diff --git a/modules/crypto/sha512 b/modules/crypto/sha512
index 4c97604cd..6d0dd1019 100644
--- a/modules/crypto/sha512
+++ b/modules/crypto/sha512
@@ -5,8 +5,11 @@ Files:
lib/gl_openssl.h
lib/sha512.h
lib/sha512.c
+lib/af_alg.h
+lib/af_alg.c
m4/gl-openssl.m4
m4/sha512.m4
+m4/linux-if-alg.m4

Depends-on:
extern-inline
@@ -16,9 +19,10 @@ u64

configure.ac:
gl_SHA512
+gl_LINUX_IF_ALG_H

Makefile.am:
-lib_SOURCES += sha512.c
+lib_SOURCES += sha512.c af_alg.c

Include:
"sha512.h"

[Matteo Croce]

Use AF_ALG for md5 too

Signed-off-by: Matteo Croce <***@redhat.com>
---
lib/md5.c | 18 +++++++++++++++++-
modules/crypto/md5 | 6 +++++-
2 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/lib/md5.c b/lib/md5.c
index 68d00a6c7..307abbbe7 100644
--- a/lib/md5.c
+++ b/lib/md5.c
@@ -32,6 +32,10 @@
#include <string.h>
#include <sys/types.h>

+#ifdef HAVE_LINUX_IF_ALG_H
+# include "af_alg.h"
+#endif
+
#if USE_UNLOCKED_IO
# include "unlocked-io.h"
#endif
@@ -142,8 +146,20 @@ md5_stream (FILE *stream, void *resblock)
{
struct md5_ctx ctx;
size_t sum;
+ char *buffer;
+
+#ifdef HAVE_LINUX_IF_ALG_H
+ int ret;
+
+ ret = afalg_stream(stream, resblock, "md5", MD5_DIGEST_SIZE);
+ if (!ret)
+ return 0;
+
+ if (ret == -EIO)
+ return 1;
+#endif

- char *buffer = malloc (BLOCKSIZE + 72);
+ buffer = malloc (BLOCKSIZE + 72);
if (!buffer)
return 1;

diff --git a/modules/crypto/md5 b/modules/crypto/md5
index e5fb39ced..415cf0523 100644
--- a/modules/crypto/md5
+++ b/modules/crypto/md5
@@ -5,8 +5,11 @@ Files:
lib/gl_openssl.h
lib/md5.h
lib/md5.c
+lib/af_alg.h
+lib/af_alg.c
m4/gl-openssl.m4
m4/md5.m4
+m4/linux-if-alg.m4

Depends-on:
extern-inline
@@ -15,9 +18,10 @@ stdint

configure.ac:
gl_MD5
+gl_LINUX_IF_ALG_H

Makefile.am:
-lib_SOURCES += md5.c
+lib_SOURCES += md5.c af_alg.c

Include:
"md5.h"

[Matteo Croce]

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.

Add a generic afalg_stream() function which uses AF_ALG to calculate the
hash of a stream and use sendfile() when possible (regular file with size
less or equal than 0x7ffff000 (2,147,479,552) bytes, AKA MAX_RW_COUNT).

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%.

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

Signed-off-by: Matteo Croce <***@redhat.com>
---
lib/af_alg.c | 115 ++++++++++++++++++++++++++++++++++++++++++++++++++++
lib/af_alg.h | 49 ++++++++++++++++++++++
lib/sha1.c | 17 +++++++-
m4/linux-if-alg.m4 | 29 +++++++++++++
modules/crypto/sha1 | 6 ++-
5 files changed, 214 insertions(+), 2 deletions(-)
create mode 100644 lib/af_alg.c
create mode 100644 lib/af_alg.h
create mode 100644 m4/linux-if-alg.m4

diff --git a/lib/af_alg.c b/lib/af_alg.c
new file mode 100644
index 000000000..3af099e43
--- /dev/null
+++ b/lib/af_alg.c
@@ -0,0 +1,115 @@
+/* af_alg.c - Functions to compute message digest from file streams using
+ Linux kernel crypto API.
+ Copyright (C) 2018 Free Software Foundation, Inc.
+
+ This program is free software; you can redistribute it and/or modify it
+ under the terms of the GNU General Public License as published by the
+ Free Software Foundation; either version 2, or (at your option) any
+ later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with this program; if not, see <https://www.gnu.org/licenses/>. */
+
+/* Written by Matteo Croce <***@redhat.com>, 2018. */
+
+#include <config.h>
+
+#ifdef HAVE_LINUX_IF_ALG_H
+
+#include <unistd.h>
+#include <string.h>
+#include <linux/if_alg.h>
+#include <sys/stat.h>
+#include <sys/sendfile.h>
+#include <sys/socket.h>
+
+#include "af_alg.h"
+
+/* from linux/include/linux/fs.h: (INT_MAX & PAGE_MASK). */
+#define MAX_RW_COUNT 0x7FFFF000
+#define BLOCKSIZE 32768
+
+int
+afalg_stream (FILE * stream, void *resblock, const char *alg, ssize_t hashlen)
+{
+ struct sockaddr_alg salg = {
+ .salg_family = AF_ALG,
+ .salg_type = "hash",
+ };
+ int ret, cfd, ofd;
+ struct stat st;
+
+ cfd = socket (AF_ALG, SOCK_SEQPACKET, 0);
+ if (cfd < 0)
+ return -EAFNOSUPPORT;
+
+ /* avoid calling both strcpy and strlen. */
+ for (int i = 0; (salg.salg_name[i] = alg[i]); i++)
+ if (i == sizeof salg.salg_name - 1)
+ return -EINVAL;
+
+ ret = bind (cfd, (struct sockaddr *) &salg, sizeof salg);
+ if (ret < 0)
+ {
+ ret = -EAFNOSUPPORT;
+ goto out_cfd;
+ }
+
+ ofd = accept (cfd, NULL, 0);
+ if (ofd < 0)
+ {
+ ret = -EAFNOSUPPORT;
+ goto out_cfd;
+ }
+
+ /* if file is a regular file, attempt sendfile to pipe the data. */
+ if (!fstat (fileno (stream), &st)
+ && (S_ISREG (st.st_mode) || S_TYPEISSHM (&st) || S_TYPEISTMO (&st))
+ && st.st_size && st.st_size <= MAX_RW_COUNT)
+ {
+ if (sendfile (ofd, fileno (stream), NULL, st.st_size) != st.st_size)
+ {
+ ret = -EIO;
+ goto out_ofd;
+ }
+ else
+ ret = 0;
+ }
+ else
+ {
+ /* sendfile not possible, do a classic read-write loop. */
+ ssize_t size;
+ char buf[BLOCKSIZE];
+ while ((size = fread (buf, 1, sizeof buf, stream)))
+ {
+ if (send (ofd, buf, size, MSG_MORE) != size)
+ {
+ ret = -EIO;
+ goto out_ofd;
+ }
+ }
+ if (ferror (stream))
+ {
+ ret = -EIO;
+ goto out_ofd;
+ }
+ }
+
+ if (read (ofd, resblock, hashlen) != hashlen)
+ ret = -EIO;
+ else
+ ret = 0;
+
+out_ofd:
+ close (ofd);
+out_cfd:
+ close (cfd);
+ return ret;
+}
+
+#endif
diff --git a/lib/af_alg.h b/lib/af_alg.h
new file mode 100644
index 000000000..978e21d36
--- /dev/null
+++ b/lib/af_alg.h
@@ -0,0 +1,49 @@
+/* af_alg.h - Declaration of functions to compute message digest from
+ file streams using Linux kernel crypto API.
+ Copyright (C) 2018 Free Software Foundation, Inc.
+
+ This program is free software; you can redistribute it and/or modify it
+ under the terms of the GNU General Public License as published by the
+ Free Software Foundation; either version 2, or (at your option) any
+ later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with this program; if not, see <https://www.gnu.org/licenses/>. */
+
+/* Written by Matteo Croce <***@redhat.com>, 2018. */
+
+#ifndef AF_ALG_H
+# define AF_ALG_H 1
+
+# include <stdio.h>
+# include <errno.h>
+
+# ifdef __cplusplus
+extern "C" {
+# endif
+
+# ifdef HAVE_LINUX_IF_ALG_H
+
+int
+afalg_stream (FILE * stream, void *resblock, const char *alg, ssize_t hashlen);
+
+# else
+
+static int
+afalg_stream (FILE * stream, void *resblock, const char *alg, ssize_t hashlen)
+{
+ return -EAFNOSUPPORT;
+}
+
+# endif
+
+# ifdef __cplusplus
+}
+# endif
+
+#endif
diff --git a/lib/sha1.c b/lib/sha1.c
index 37d46b68e..a84bf211a 100644
--- a/lib/sha1.c
+++ b/lib/sha1.c
@@ -33,6 +33,10 @@
#include <stdlib.h>
#include <string.h>

+#ifdef HAVE_LINUX_IF_ALG_H
+# include "af_alg.h"
+#endif
+
#if USE_UNLOCKED_IO
# include "unlocked-io.h"
#endif
@@ -130,8 +134,19 @@ sha1_stream (FILE *stream, void *resblock)
{
struct sha1_ctx ctx;
size_t sum;
+ char *buffer;
+#ifdef HAVE_LINUX_IF_ALG_H
+ int ret;
+
+ ret = afalg_stream(stream, resblock, "sha1", SHA1_DIGEST_SIZE);
+ if (!ret)
+ return 0;
+
+ if (ret == -EIO)
+ return 1;
+#endif

- char *buffer = malloc (BLOCKSIZE + 72);
+ buffer = malloc (BLOCKSIZE + 72);
if (!buffer)
return 1;

diff --git a/m4/linux-if-alg.m4 b/m4/linux-if-alg.m4
new file mode 100644
index 000000000..3ed18ae33
--- /dev/null
+++ b/m4/linux-if-alg.m4
@@ -0,0 +1,29 @@
+dnl Check whether linux/if_alg.h has needed features.
+
+dnl Copyright 2018 Free Software Foundation, Inc.
+dnl This file is free software; the Free Software Foundation
+dnl gives unlimited permission to copy and/or distribute it,
+dnl with or without modifications, as long as this notice is preserved.
+
+dnl From Matteo Croce.
+
+AC_DEFUN_ONCE([gl_LINUX_IF_ALG_H],
+[
+ AC_REQUIRE([gl_HEADER_SYS_SOCKET])
+ AC_CACHE_CHECK([whether linux/if_alg.h has struct sockaddr_alg.],
+ [gl_cv_header_linux_if_alg_salg],
+ [AC_COMPILE_IFELSE(
+ [AC_LANG_PROGRAM([[#include <sys/socket.h>
+ #include <linux/if_alg.h>
+ struct sockaddr_alg salg = {
+ .salg_family = AF_ALG,
+ .salg_type = "hash",
+ .salg_name = "sha1",
+ };]])],
+ [gl_cv_header_linux_if_alg_salg=yes],
+ [gl_cv_header_linux_if_alg_salg=no])])
+ if test "$gl_cv_header_linux_if_alg_salg" = yes; then
+ AC_DEFINE([HAVE_LINUX_IF_ALG_H], [1],
+ [Define to 1 if you have `struct sockaddr_alg' defined.])
+ fi
+])
diff --git a/modules/crypto/sha1 b/modules/crypto/sha1
index d65f99418..dbd41d21b 100644
--- a/modules/crypto/sha1
+++ b/modules/crypto/sha1
@@ -5,8 +5,11 @@ Files:
lib/gl_openssl.h
lib/sha1.h
lib/sha1.c
+lib/af_alg.h
+lib/af_alg.c
m4/gl-openssl.m4
m4/sha1.m4
+m4/linux-if-alg.m4

Depends-on:
extern-inline
@@ -15,9 +18,10 @@ stdint

configure.ac:
gl_SHA1
+gl_LINUX_IF_ALG_H

Makefile.am:
-lib_SOURCES += sha1.c
+lib_SOURCES += sha1.c af_alg.c

Include:
"sha1.h"

[Matteo Croce]

Use AF_ALG for sha224 and sha256 too

Signed-off-by: Matteo Croce <***@redhat.com>
---
lib/sha256.c | 32 ++++++++++++++++++++++++++++++--
modules/crypto/sha256 | 6 +++++-
2 files changed, 35 insertions(+), 3 deletions(-)

diff --git a/lib/sha256.c b/lib/sha256.c
index 85405b20f..578f43e4d 100644
--- a/lib/sha256.c
+++ b/lib/sha256.c
@@ -32,6 +32,10 @@
#include <stdlib.h>
#include <string.h>

+#ifdef HAVE_LINUX_IF_ALG_H
+# include "af_alg.h"
+#endif
+
#if USE_UNLOCKED_IO
# include "unlocked-io.h"
#endif
@@ -177,8 +181,20 @@ sha256_stream (FILE *stream, void *resblock)
{
struct sha256_ctx ctx;
size_t sum;
+ char *buffer;
+
+#ifdef HAVE_LINUX_IF_ALG_H
+ int ret;
+
+ ret = afalg_stream(stream, resblock, "sha256", SHA256_DIGEST_SIZE);
+ if (!ret)
+ return 0;
+
+ if (ret == -EIO)
+ return 1;
+#endif

- char *buffer = malloc (BLOCKSIZE + 72);
+ buffer = malloc (BLOCKSIZE + 72);
if (!buffer)
return 1;

@@ -248,8 +264,20 @@ sha224_stream (FILE *stream, void *resblock)
{
struct sha256_ctx ctx;
size_t sum;
+ char *buffer;
+
+#ifdef HAVE_LINUX_IF_ALG_H
+ int ret;
+
+ ret = afalg_stream(stream, resblock, "sha224", SHA224_DIGEST_SIZE);
+ if (!ret)
+ return 0;
+
+ if (ret == -EIO)
+ return 1;
+#endif

- char *buffer = malloc (BLOCKSIZE + 72);
+ buffer = malloc (BLOCKSIZE + 72);
if (!buffer)
return 1;

diff --git a/modules/crypto/sha256 b/modules/crypto/sha256
index 37fabfd90..9d4f11578 100644
--- a/modules/crypto/sha256
+++ b/modules/crypto/sha256
@@ -5,8 +5,11 @@ Files:
lib/gl_openssl.h
lib/sha256.h
lib/sha256.c
+lib/af_alg.h
+lib/af_alg.c
m4/gl-openssl.m4
m4/sha256.m4
+m4/linux-if-alg.m4

Depends-on:
extern-inline
@@ -15,9 +18,10 @@ stdint

configure.ac:
gl_SHA256
+gl_LINUX_IF_ALG_H

Makefile.am:
-lib_SOURCES += sha256.c
+lib_SOURCES += sha256.c af_alg.c

Include:
"sha256.h"

[Bruno Haible]

Thanks for the patches. There were no further comments in a week,
therefore I pushed them in your name.

Still to be addressed:
- Lack of documentation.
- Code duplication.
- Lack of unit tests.
- Lack of configure option (suggested by Assaf Gordon in [1]).

Bruno

[1] https://lists.gnu.org/archive/html/bug-gnulib/2018-04/msg00088.html

[Bruno Haible]

Lack of unit tests is addressed through these patches.

[Bruno Haible]

Lack of documentation is addressed by this patch.


2018-05-05 Bruno Haible <***@clisp.org>

af_alg: Add documentation.
* lib/af_alg.h: Add comments.

diff --git a/lib/af_alg.h b/lib/af_alg.h
index 978e21d..e9580d4 100644
--- a/lib/af_alg.h
+++ b/lib/af_alg.h
@@ -15,7 +15,16 @@
You should have received a copy of the GNU General Public License
along with this program; if not, see <https://www.gnu.org/licenses/>. */

-/* Written by Matteo Croce <***@redhat.com>, 2018. */
+/* Written by Matteo Croce <***@redhat.com>, 2018.
+ Documentation by Bruno Haible <***@clisp.org>, 2018. */
+
+/* This file declares specific functions for computing message digests
+ using the Linux kernel crypto API, if available. This kernel API gives
+ access to specialized crypto instructions (that would also be available
+ in user space) or to crypto devices (not directly available in user space).
+
+ For a more complete set of facilities that use the Linux kernel crypto API,
+ look at libkcapi. */

#ifndef AF_ALG_H
# define AF_ALG_H 1
@@ -29,13 +38,32 @@ extern "C" {

# ifdef HAVE_LINUX_IF_ALG_H

+/* Computes a message digest of the contents of a file.
+ STREAM is an open file stream. Regular files are handled more efficiently
+ than other types of files.
+ ALG is the message digest algorithm. The supported algorithms are listed in
+ the file /proc/crypto.
+ RESBLOCK points to a block of HASHLEN bytes, for the result. HASHLEN must be
+ the length of the message digest, in bytes, in particular:
+
+ alg | hashlen
+ -------+--------
+ md5 | 16
+ sha1 | 20
+ sha224 | 28
+ sha256 | 32
+ sha384 | 48
+ sha512 | 64
+
+ If successful, this function fills RESBLOCK and returns 0.
+ Upon failure, it returns a negated error code. */
int
-afalg_stream (FILE * stream, void *resblock, const char *alg, ssize_t hashlen);
+afalg_stream (FILE *stream, void *resblock, const char *alg, ssize_t hashlen);

# else

static int
-afalg_stream (FILE * stream, void *resblock, const char *alg, ssize_t hashlen)
+afalg_stream (FILE *stream, void *resblock, const char *alg, ssize_t hashlen)
{
return -EAFNOSUPPORT;
}
@@ -46,4 +74,4 @@ afalg_stream (FILE * stream, void *resblock, const char *alg, ssize_t hashlen)
}
# endif

-#endif
+#endif /* AF_ALG_H */

[Bruno Haible]

While writing the documentation, I noticed that the function has an
argument list that is hard to remember:
- an input argument (stream),
- an output buffer (resblock),
- an input argument (alg),
- the size of the output buffer.

It is better to reorder the arguments so as to:
1. group the input arguments together,
2. group the output buffer and its size together.


2018-05-05 Bruno Haible <***@clisp.org>

af_alg: Improve function signature.
* lib/af_alg.h (afalg_stream): Swap second and third argument.
* lib/af_alg.c (afalg_stream): Likewise.
* lib/md5.c, lib/sha1.c, lib/sha256.c, lib/sha512.c: Callers changed.

diff --git a/lib/af_alg.c b/lib/af_alg.c
index 3af099e..91f565d 100644
--- a/lib/af_alg.c
+++ b/lib/af_alg.c
@@ -35,7 +35,7 @@
#define BLOCKSIZE 32768

int
-afalg_stream (FILE * stream, void *resblock, const char *alg, ssize_t hashlen)
+afalg_stream (FILE * stream, const char *alg, void *resblock, ssize_t hashlen)
{
struct sockaddr_alg salg = {
.salg_family = AF_ALG,
diff --git a/lib/af_alg.h b/lib/af_alg.h
index e9580d4..aacb8f6 100644
--- a/lib/af_alg.h
+++ b/lib/af_alg.h
@@ -58,12 +58,12 @@ extern "C" {
If successful, this function fills RESBLOCK and returns 0.
Upon failure, it returns a negated error code. */
int
-afalg_stream (FILE *stream, void *resblock, const char *alg, ssize_t hashlen);
+afalg_stream (FILE *stream, const char *alg, void *resblock, ssize_t hashlen);

# else

static int
-afalg_stream (FILE *stream, void *resblock, const char *alg, ssize_t hashlen)
+afalg_stream (FILE *stream, const char *alg, void *resblock, ssize_t hashlen)
{
return -EAFNOSUPPORT;
}
diff --git a/lib/md5.c b/lib/md5.c
index 307abbb..e6bd209 100644
--- a/lib/md5.c
+++ b/lib/md5.c
@@ -151,7 +151,7 @@ md5_stream (FILE *stream, void *resblock)
#ifdef HAVE_LINUX_IF_ALG_H
int ret;

- ret = afalg_stream(stream, resblock, "md5", MD5_DIGEST_SIZE);
+ ret = afalg_stream (stream, "md5", resblock, MD5_DIGEST_SIZE);
if (!ret)
return 0;

diff --git a/lib/sha1.c b/lib/sha1.c
index a84bf21..b670267 100644
--- a/lib/sha1.c
+++ b/lib/sha1.c
@@ -138,7 +138,7 @@ sha1_stream (FILE *stream, void *resblock)
#ifdef HAVE_LINUX_IF_ALG_H
int ret;

- ret = afalg_stream(stream, resblock, "sha1", SHA1_DIGEST_SIZE);
+ ret = afalg_stream (stream, "sha1", resblock, SHA1_DIGEST_SIZE);
if (!ret)
return 0;

diff --git a/lib/sha256.c b/lib/sha256.c
index 578f43e..fab119a 100644
--- a/lib/sha256.c
+++ b/lib/sha256.c
@@ -186,7 +186,7 @@ sha256_stream (FILE *stream, void *resblock)
#ifdef HAVE_LINUX_IF_ALG_H
int ret;

- ret = afalg_stream(stream, resblock, "sha256", SHA256_DIGEST_SIZE);
+ ret = afalg_stream(stream, "sha256", resblock, SHA256_DIGEST_SIZE);
if (!ret)
return 0;

@@ -269,7 +269,7 @@ sha224_stream (FILE *stream, void *resblock)
#ifdef HAVE_LINUX_IF_ALG_H
int ret;

- ret = afalg_stream(stream, resblock, "sha224", SHA224_DIGEST_SIZE);
+ ret = afalg_stream (stream, "sha224", resblock, SHA224_DIGEST_SIZE);
if (!ret)
return 0;

diff --git a/lib/sha512.c b/lib/sha512.c
index 72e5fdd..ff7c0cb 100644
--- a/lib/sha512.c
+++ b/lib/sha512.c
@@ -194,7 +194,7 @@ sha512_stream (FILE *stream, void *resblock)
#ifdef HAVE_LINUX_IF_ALG_H
int ret;

- ret = afalg_stream(stream, resblock, "sha512", SHA512_DIGEST_SIZE);
+ ret = afalg_stream (stream, "sha512", resblock, SHA512_DIGEST_SIZE);
if (!ret)
return 0;

@@ -277,7 +277,7 @@ sha384_stream (FILE *stream, void *resblock)
#ifdef HAVE_LINUX_IF_ALG_H
int ret;

- ret = afalg_stream(stream, resblock, "sha384", SHA384_DIGEST_SIZE);
+ ret = afalg_stream (stream, "sha384", resblock, SHA384_DIGEST_SIZE);
if (!ret)
return 0;

[Bruno Haible]

Creating a testdir produces an error:

$ ./gnulib-tool --create-testdir --dir=t2 --single-configure --symlink crypto/md5 crypto/sha1 crypto/sha256 crypto/sha512
...
configure.ac:197: warning: gl_HEADER_SYS_SOCKET is m4_require'd but not m4_defun'd
configure.ac:29: gl_INIT is expanded from...
configure.ac:197: the top level
executing autoconf
configure.ac:197: warning: gl_HEADER_SYS_SOCKET is m4_require'd but not m4_defun'd
configure.ac:29: gl_INIT is expanded from...
configure.ac:197: the top level
configure:5335: error: possibly undefined macro: gl_HEADER_SYS_SOCKET
If this token and others are legitimate, please use m4_pattern_allow.
See the Autoconf documentation.

This fixes it.


2018-05-05 Bruno Haible <***@clisp.org>

crypto/{md5,sha1,sha256,sha512}: Fix module description.
* modules/crypto/md5 (Depends-on): Add 'sys_socket'.
* modules/crypto/sha1 (Depends-on): Likewise.
* modules/crypto/sha256 (Depends-on): Likewise.
* modules/crypto/sha512 (Depends-on): Likewise.

diff --git a/modules/crypto/md5 b/modules/crypto/md5
index 415cf05..6fc2477 100644
--- a/modules/crypto/md5
+++ b/modules/crypto/md5
@@ -15,6 +15,7 @@ Depends-on:
extern-inline
stdalign
stdint
+sys_socket

configure.ac:
gl_MD5
diff --git a/modules/crypto/sha1 b/modules/crypto/sha1
index dbd41d2..3ae3ebc 100644
--- a/modules/crypto/sha1
+++ b/modules/crypto/sha1
@@ -15,6 +15,7 @@ Depends-on:
extern-inline
stdalign
stdint
+sys_socket

configure.ac:
gl_SHA1
diff --git a/modules/crypto/sha256 b/modules/crypto/sha256
index 9d4f115..79d03bf 100644
--- a/modules/crypto/sha256
+++ b/modules/crypto/sha256
@@ -15,6 +15,7 @@ Depends-on:
extern-inline
stdalign
stdint
+sys_socket

configure.ac:
gl_SHA256
diff --git a/modules/crypto/sha512 b/modules/crypto/sha512
index 6d0dd10..f08c249 100644
--- a/modules/crypto/sha512
+++ b/modules/crypto/sha512
@@ -15,6 +15,7 @@ Depends-on:
extern-inline
stdalign
stdint
+sys_socket
u64

configure.ac:

[Bruno Haible]

Also, I get this compilation warning:

af_alg.c: In function ‘afalg_stream’:
af_alg.c:72:56: warning: implicit declaration of function ‘S_TYPEISTMO’ [-Wimplicit-function-declaration]
&& (S_ISREG (st.st_mode) || S_TYPEISSHM (&st) || S_TYPEISTMO (&st))
^

2018-05-05 Bruno Haible <***@clisp.org>

crypto/{md5,sha1,sha256,sha512}: Fix compilation error (S_TYPEISTMO).
* modules/crypto/md5 (Depends-on): Add 'sys_stat'.
* modules/crypto/sha1 (Depends-on): Likewise.
* modules/crypto/sha256 (Depends-on): Likewise.
* modules/crypto/sha512 (Depends-on): Likewise.

diff --git a/modules/crypto/md5 b/modules/crypto/md5
index 6fc2477..3a4c756 100644
--- a/modules/crypto/md5
+++ b/modules/crypto/md5
@@ -16,6 +16,7 @@ extern-inline
stdalign
stdint
sys_socket
+sys_stat

configure.ac:
gl_MD5
diff --git a/modules/crypto/sha1 b/modules/crypto/sha1
index 3ae3ebc..a4d2950 100644
--- a/modules/crypto/sha1
+++ b/modules/crypto/sha1
@@ -16,6 +16,7 @@ extern-inline
stdalign
stdint
sys_socket
+sys_stat

configure.ac:
gl_SHA1
diff --git a/modules/crypto/sha256 b/modules/crypto/sha256
index 79d03bf..4acb27a 100644
--- a/modules/crypto/sha256
+++ b/modules/crypto/sha256
@@ -16,6 +16,7 @@ extern-inline
stdalign
stdint
sys_socket
+sys_stat

configure.ac:
gl_SHA256
diff --git a/modules/crypto/sha512 b/modules/crypto/sha512
index f08c249..1fa3f36 100644
--- a/modules/crypto/sha512
+++ b/modules/crypto/sha512
@@ -16,6 +16,7 @@ extern-inline
stdalign
stdint
sys_socket
+sys_stat
u64

configure.ac:

[Bruno Haible]

The 4 tests that I added all fail on the empty file. Example:

$ ./test-sha1
sha1_stream produced wrong result.
Expected: \xda\x39\xa3\xee\x5e\x6b\x4b\x0d\x32\x55\xbf\xef\x95\x60\x18\x90\xaf\xd8\x07\x09
Got: \x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00

Seen on Ubuntu 16.04 LTS with a kernel version 4.4.0-119, which somewhat
corresponds to the kernel.org version 4.4.114 [1].

[1] https://people.canonical.com/~kernel/info/kernel-version-map.html

This workaround makes the tests pass:


2018-05-05 Bruno Haible <***@clisp.org>

af_alg: Fix bug on empty files.
* lib/af_alg.c (afalg_stream): Ignore the kernel's result if the input
stream is empty.

diff --git a/lib/af_alg.c b/lib/af_alg.c
index 1f4a6aa..06344b1 100644
--- a/lib/af_alg.c
+++ b/lib/af_alg.c
@@ -83,10 +83,12 @@ afalg_stream (FILE * stream, const char *alg, void *resblock, ssize_t hashlen)
else
{
/* sendfile not possible, do a classic read-write loop. */
+ int non_empty = 0;
ssize_t size;
char buf[BLOCKSIZE];
while ((size = fread (buf, 1, sizeof buf, stream)))
{
+ non_empty = 1;
if (send (ofd, buf, size, MSG_MORE) != size)
{
ret = -EIO;
@@ -98,6 +100,13 @@ afalg_stream (FILE * stream, const char *alg, void *resblock, ssize_t hashlen)
ret = -EIO;
goto out_ofd;
}
+ /* On Linux 4.4.0 at least, the value for an empty stream is wrong
+ (all zeroes). */
+ if (!non_empty)
+ {
+ ret = -EAFNOSUPPORT;
+ goto out_ofd;
+ }
}

if (read (ofd, resblock, hashlen) != hashlen)

[Bruno Haible]

This patch addresses the code duplication issue in the module descriptions.
At the same time, it enables some of the best practices for gnulib modules.
In particular, the best practice is not to write

#if some_condition
#include "af_alg.h"
#endif

but instead

#include "af_alg.h"

and make sure that af_alg.h defines trivial replacement stubs on platforms
that don't implement the functionality.


2018-05-05 Bruno Haible <***@clisp.org>

af_alg: New module.
* lib/af_alg.h: Test HAVE_* macro through '#if', not '#ifdef'.
* lib/af_alg.c: Include "af_alg.h" before the other header files.
* lib/md5.c: Include "af_alg.h" unconditionally.
(md5_stream): Invoke afalg_stream unconditionally.
* lib/sha1.c: Include "af_alg.h" unconditionally.
(sha1_stream): Invoke afalg_stream unconditionally.
* lib/sha256.c: Include "af_alg.h" unconditionally.
(sha256_stream, sha224_stream): Invoke afalg_stream unconditionally.
* lib/sha512.c: Include "af_alg.h" unconditionally.
(sha512_stream, sha384_stream): Invoke afalg_stream unconditionally.
* m4/af_alg.m4: Renamed from m4/linux-if-alg.m4.
(gl_AF_ALG): Renamed from gl_LINUX_IF_ALG_H.
* modules/crypto/af_alg: New file.
* modules/crypto/md5 (Files): Remove files that are now in the
'crypto/af_alg' module.
(Depends-on): Add crypto/af_alg.
(configure.ac): Remove gl_LINUX_IF_ALG_H invocation.
(Makefile.am): Don't mention af_alg.c here.
* modules/crypto/sha1 (Files): Remove files that are now in the
'crypto/af_alg' module.
(Depends-on): Add crypto/af_alg.
(configure.ac): Remove gl_LINUX_IF_ALG_H invocation.
(Makefile.am): Don't mention af_alg.c here.
* modules/crypto/sha256 (Files): Remove files that are now in the
'crypto/af_alg' module.
(Depends-on): Add crypto/af_alg.
(configure.ac): Remove gl_LINUX_IF_ALG_H invocation.
(Makefile.am): Don't mention af_alg.c here.
* modules/crypto/sha512 (Files): Remove files that are now in the
'crypto/af_alg' module.
(Depends-on): Add crypto/af_alg.
(configure.ac): Remove gl_LINUX_IF_ALG_H invocation.
(Makefile.am): Don't mention af_alg.c here.

diff --git a/lib/af_alg.c b/lib/af_alg.c
index 06344b1..73aa6e7 100644
--- a/lib/af_alg.c
+++ b/lib/af_alg.c
@@ -19,7 +19,9 @@

#include <config.h>

-#ifdef HAVE_LINUX_IF_ALG_H
+#if HAVE_LINUX_IF_ALG_H
+
+#include "af_alg.h"

#include <unistd.h>
#include <string.h>
@@ -28,8 +30,6 @@
#include <sys/sendfile.h>
#include <sys/socket.h>

-#include "af_alg.h"
-
#include "sys-limits.h"

#define BLOCKSIZE 32768
diff --git a/lib/af_alg.h b/lib/af_alg.h
index aacb8f6..295f0f9 100644
--- a/lib/af_alg.h
+++ b/lib/af_alg.h
@@ -36,7 +36,7 @@
extern "C" {
# endif

-# ifdef HAVE_LINUX_IF_ALG_H
+# if HAVE_LINUX_IF_ALG_H

/* Computes a message digest of the contents of a file.
STREAM is an open file stream. Regular files are handled more efficiently
diff --git a/lib/md5.c b/lib/md5.c
index e6bd209..2bf2f0c 100644
--- a/lib/md5.c
+++ b/lib/md5.c
@@ -32,9 +32,7 @@
#include <string.h>
#include <sys/types.h>

-#ifdef HAVE_LINUX_IF_ALG_H
-# include "af_alg.h"
-#endif
+#include "af_alg.h"

#if USE_UNLOCKED_IO
# include "unlocked-io.h"
@@ -148,16 +146,14 @@ md5_stream (FILE *stream, void *resblock)
size_t sum;
char *buffer;

-#ifdef HAVE_LINUX_IF_ALG_H
- int ret;
-
- ret = afalg_stream (stream, "md5", resblock, MD5_DIGEST_SIZE);
- if (!ret)
+ {
+ int ret = afalg_stream (stream, "md5", resblock, MD5_DIGEST_SIZE);
+ if (!ret)
return 0;

- if (ret == -EIO)
+ if (ret == -EIO)
return 1;
-#endif
+ }

buffer = malloc (BLOCKSIZE + 72);
if (!buffer)
diff --git a/lib/sha1.c b/lib/sha1.c
index b670267..e7cd2a3 100644
--- a/lib/sha1.c
+++ b/lib/sha1.c
@@ -33,9 +33,7 @@
#include <stdlib.h>
#include <string.h>

-#ifdef HAVE_LINUX_IF_ALG_H
-# include "af_alg.h"
-#endif
+#include "af_alg.h"

#if USE_UNLOCKED_IO
# include "unlocked-io.h"
@@ -127,7 +125,7 @@ sha1_finish_ctx (struct sha1_ctx *ctx, void *resbuf)
#endif

/* Compute SHA1 message digest for bytes read from STREAM. The
- resulting message digest number will be written into the 16 bytes
+ resulting message digest number will be written into the 20 bytes
beginning at RESBLOCK. */
int
sha1_stream (FILE *stream, void *resblock)
@@ -135,16 +133,15 @@ sha1_stream (FILE *stream, void *resblock)
struct sha1_ctx ctx;
size_t sum;
char *buffer;
-#ifdef HAVE_LINUX_IF_ALG_H
- int ret;

- ret = afalg_stream (stream, "sha1", resblock, SHA1_DIGEST_SIZE);
- if (!ret)
+ {
+ int ret = afalg_stream (stream, "sha1", resblock, SHA1_DIGEST_SIZE);
+ if (!ret)
return 0;

- if (ret == -EIO)
+ if (ret == -EIO)
return 1;
-#endif
+ }

buffer = malloc (BLOCKSIZE + 72);
if (!buffer)
diff --git a/lib/sha256.c b/lib/sha256.c
index fab119a..410bd98 100644
--- a/lib/sha256.c
+++ b/lib/sha256.c
@@ -32,9 +32,7 @@
#include <stdlib.h>
#include <string.h>

-#ifdef HAVE_LINUX_IF_ALG_H
-# include "af_alg.h"
-#endif
+#include "af_alg.h"

#if USE_UNLOCKED_IO
# include "unlocked-io.h"
@@ -183,16 +181,14 @@ sha256_stream (FILE *stream, void *resblock)
size_t sum;
char *buffer;

-#ifdef HAVE_LINUX_IF_ALG_H
- int ret;
-
- ret = afalg_stream(stream, "sha256", resblock, SHA256_DIGEST_SIZE);
- if (!ret)
+ {
+ int ret = afalg_stream (stream, "sha256", resblock, SHA256_DIGEST_SIZE);
+ if (!ret)
return 0;

- if (ret == -EIO)
+ if (ret == -EIO)
return 1;
-#endif
+ }

buffer = malloc (BLOCKSIZE + 72);
if (!buffer)
@@ -266,16 +262,14 @@ sha224_stream (FILE *stream, void *resblock)
size_t sum;
char *buffer;

-#ifdef HAVE_LINUX_IF_ALG_H
- int ret;
-
- ret = afalg_stream (stream, "sha224", resblock, SHA224_DIGEST_SIZE);
- if (!ret)
+ {
+ int ret = afalg_stream(stream, "sha224", resblock, SHA224_DIGEST_SIZE);
+ if (!ret)
return 0;

- if (ret == -EIO)
+ if (ret == -EIO)
return 1;
-#endif
+ }

buffer = malloc (BLOCKSIZE + 72);
if (!buffer)
diff --git a/lib/sha512.c b/lib/sha512.c
index ff7c0cb..7a02348 100644
--- a/lib/sha512.c
+++ b/lib/sha512.c
@@ -32,9 +32,7 @@
#include <stdlib.h>
#include <string.h>

-#ifdef HAVE_LINUX_IF_ALG_H
-# include "af_alg.h"
-#endif
+#include "af_alg.h"

#if USE_UNLOCKED_IO
# include "unlocked-io.h"
@@ -191,16 +189,14 @@ sha512_stream (FILE *stream, void *resblock)
size_t sum;
char *buffer;

-#ifdef HAVE_LINUX_IF_ALG_H
- int ret;
-
- ret = afalg_stream (stream, "sha512", resblock, SHA512_DIGEST_SIZE);
- if (!ret)
+ {
+ int ret = afalg_stream (stream, "sha512", resblock, SHA512_DIGEST_SIZE);
+ if (!ret)
return 0;

- if (ret == -EIO)
+ if (ret == -EIO)
return 1;
-#endif
+ }

buffer = malloc (BLOCKSIZE + 72);
if (!buffer)
@@ -274,16 +270,14 @@ sha384_stream (FILE *stream, void *resblock)
size_t sum;
char *buffer;

-#ifdef HAVE_LINUX_IF_ALG_H
- int ret;
-
- ret = afalg_stream (stream, "sha384", resblock, SHA384_DIGEST_SIZE);
- if (!ret)
+ {
+ int ret = afalg_stream(stream, "sha384", resblock, SHA384_DIGEST_SIZE);
+ if (!ret)
return 0;

- if (ret == -EIO)
+ if (ret == -EIO)
return 1;
-#endif
+ }

buffer = malloc (BLOCKSIZE + 72);
if (!buffer)
diff --git a/m4/linux-if-alg.m4 b/m4/af_alg.m4
similarity index 90%
rename from m4/linux-if-alg.m4
rename to m4/af_alg.m4
index 3ed18ae..1c57e2c 100644
--- a/m4/linux-if-alg.m4
+++ b/m4/af_alg.m4
@@ -1,3 +1,4 @@
+# af_alg.m4 serial 1
dnl Check whether linux/if_alg.h has needed features.

dnl Copyright 2018 Free Software Foundation, Inc.
@@ -7,7 +8,7 @@ dnl with or without modifications, as long as this notice is preserved.

dnl From Matteo Croce.

-AC_DEFUN_ONCE([gl_LINUX_IF_ALG_H],
+AC_DEFUN_ONCE([gl_AF_ALG],
[
AC_REQUIRE([gl_HEADER_SYS_SOCKET])
AC_CACHE_CHECK([whether linux/if_alg.h has struct sockaddr_alg.],
@@ -24,6 +25,6 @@ AC_DEFUN_ONCE([gl_LINUX_IF_ALG_H],
[gl_cv_header_linux_if_alg_salg=no])])
if test "$gl_cv_header_linux_if_alg_salg" = yes; then
AC_DEFINE([HAVE_LINUX_IF_ALG_H], [1],
- [Define to 1 if you have `struct sockaddr_alg' defined.])
+ [Define to 1 if you have 'struct sockaddr_alg' defined.])
fi
])
diff --git a/modules/crypto/af_alg b/modules/crypto/af_alg
new file mode 100644
index 0000000..c9602dc
--- /dev/null
+++ b/modules/crypto/af_alg
@@ -0,0 +1,27 @@
+Description:
+Compute message digest using kernel-supported cryptography algorithms.
+
+Files:
+lib/af_alg.h
+lib/af_alg.c
+lib/sys-limits.h
+m4/af_alg.m4
+
+Depends-on:
+sys_socket
+sys_stat
+
+configure.ac:
+gl_AF_ALG
+
+Makefile.am:
+lib_SOURCES += af_alg.c
+
+Include:
+"af_alg.h"
+
+License:
+LGPLv2+
+
+Maintainer:
+all
diff --git a/modules/crypto/md5 b/modules/crypto/md5
index 80f14c2..1d5130b 100644
--- a/modules/crypto/md5
+++ b/modules/crypto/md5
@@ -5,14 +5,11 @@ Files:
lib/gl_openssl.h
lib/md5.h
lib/md5.c
-lib/af_alg.h
-lib/af_alg.c
-lib/sys-limits.h
m4/gl-openssl.m4
m4/md5.m4
-m4/linux-if-alg.m4

Depends-on:
+crypto/af_alg
extern-inline
stdalign
stdint
@@ -21,10 +18,9 @@ sys_stat

configure.ac:
gl_MD5
-gl_LINUX_IF_ALG_H

Makefile.am:
-lib_SOURCES += md5.c af_alg.c
+lib_SOURCES += md5.c

Include:
"md5.h"
diff --git a/modules/crypto/sha1 b/modules/crypto/sha1
index 3bdf0ea..90bd7e4 100644
--- a/modules/crypto/sha1
+++ b/modules/crypto/sha1
@@ -5,14 +5,11 @@ Files:
lib/gl_openssl.h
lib/sha1.h
lib/sha1.c
-lib/af_alg.h
-lib/af_alg.c
-lib/sys-limits.h
m4/gl-openssl.m4
m4/sha1.m4
-m4/linux-if-alg.m4

Depends-on:
+crypto/af_alg
extern-inline
stdalign
stdint
@@ -21,10 +18,9 @@ sys_stat

configure.ac:
gl_SHA1
-gl_LINUX_IF_ALG_H

Makefile.am:
-lib_SOURCES += sha1.c af_alg.c
+lib_SOURCES += sha1.c

Include:
"sha1.h"
diff --git a/modules/crypto/sha256 b/modules/crypto/sha256
index d7c3735..633bf04 100644
--- a/modules/crypto/sha256
+++ b/modules/crypto/sha256
@@ -5,14 +5,11 @@ Files:
lib/gl_openssl.h
lib/sha256.h
lib/sha256.c
-lib/af_alg.h
-lib/af_alg.c
-lib/sys-limits.h
m4/gl-openssl.m4
m4/sha256.m4
-m4/linux-if-alg.m4

Depends-on:
+crypto/af_alg
extern-inline
stdalign
stdint
@@ -21,10 +18,9 @@ sys_stat

configure.ac:
gl_SHA256
-gl_LINUX_IF_ALG_H

Makefile.am:
-lib_SOURCES += sha256.c af_alg.c
+lib_SOURCES += sha256.c

Include:
"sha256.h"
diff --git a/modules/crypto/sha512 b/modules/crypto/sha512
index d6f9b99..cedc31c 100644
--- a/modules/crypto/sha512
+++ b/modules/crypto/sha512
@@ -5,14 +5,11 @@ Files:
lib/gl_openssl.h
lib/sha512.h
lib/sha512.c
-lib/af_alg.h
-lib/af_alg.c
-lib/sys-limits.h
m4/gl-openssl.m4
m4/sha512.m4
-m4/linux-if-alg.m4

Depends-on:
+crypto/af_alg
extern-inline
stdalign
stdint
@@ -22,10 +19,9 @@ u64

configure.ac:
gl_SHA512
-gl_LINUX_IF_ALG_H

Makefile.am:
-lib_SOURCES += sha512.c af_alg.c
+lib_SOURCES += sha512.c

Include:
"sha512.h"

[Bruno Haible]

Oops, I had left some unneeded module dependencies.


2018-05-06 Bruno Haible <***@clisp.org>

Followup to 'af_alg: New module.'.
* modules/crypto/md5 (Depends-on): Remove sys_socket, sys_stat.
* modules/crypto/sha1 (Depends-on): Likewise.
* modules/crypto/sha256 (Depends-on): Likewise.
* modules/crypto/sha512 (Depends-on): Likewise.

diff --git a/modules/crypto/md5 b/modules/crypto/md5
index 1d5130b..7d8b3fa 100644
--- a/modules/crypto/md5
+++ b/modules/crypto/md5
@@ -13,8 +13,6 @@ crypto/af_alg
extern-inline
stdalign
stdint
-sys_socket
-sys_stat

configure.ac:
gl_MD5
diff --git a/modules/crypto/sha1 b/modules/crypto/sha1
index 90bd7e4..e9fc83a 100644
--- a/modules/crypto/sha1
+++ b/modules/crypto/sha1
@@ -13,8 +13,6 @@ crypto/af_alg
extern-inline
stdalign
stdint
-sys_socket
-sys_stat

configure.ac:
gl_SHA1
diff --git a/modules/crypto/sha256 b/modules/crypto/sha256
index 633bf04..2343940 100644
--- a/modules/crypto/sha256
+++ b/modules/crypto/sha256
@@ -13,8 +13,6 @@ crypto/af_alg
extern-inline
stdalign
stdint
-sys_socket
-sys_stat

configure.ac:
gl_SHA256
diff --git a/modules/crypto/sha512 b/modules/crypto/sha512
index cedc31c..127e67c 100644
--- a/modules/crypto/sha512
+++ b/modules/crypto/sha512
@@ -13,8 +13,6 @@ crypto/af_alg
extern-inline
stdalign
stdint
-sys_socket
-sys_stat
u64

configure.ac:

[Bruno Haible]

This patch adds the configure option.

Assaf suggested to let it turned off by default, but I prefer to turn it on
by default because
* 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 feature does not require linking with additional libraries.
* 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.
* At the end of the day, new features are there to be used, not to be ignored.


2018-05-06 Bruno Haible <***@clisp.org>

af_alg: Add configure option to enable/disable use of Linux crypto API.
Suggested by Assaf Gordon <***@gmail.com>.
* m4/af_alg.m4 (gl_AF_ALG): Add AC_ARG_WITH invocation. Define C macro
USE_LINUX_CRYPTO_API.
* lib/af_alg.h: Test USE_LINUX_CRYPTO_API, not HAVE_LINUX_IF_ALG_H.
* lib/af_alg.c: Likewise.

diff --git a/lib/af_alg.c b/lib/af_alg.c
index 3b35e01..97bdff5 100644
--- a/lib/af_alg.c
+++ b/lib/af_alg.c
@@ -19,7 +19,7 @@

#include <config.h>

-#if HAVE_LINUX_IF_ALG_H
+#if USE_LINUX_CRYPTO_API

#include "af_alg.h"

diff --git a/lib/af_alg.h b/lib/af_alg.h
index 2545ec6..a15a956 100644
--- a/lib/af_alg.h
+++ b/lib/af_alg.h
@@ -35,7 +35,7 @@
extern "C" {
# endif

-# if HAVE_LINUX_IF_ALG_H
+# if USE_LINUX_CRYPTO_API

/* Compute a message digest of the contents of a file.
STREAM is an open file stream. Regular files are handled more efficiently.
diff --git a/m4/af_alg.m4 b/m4/af_alg.m4
index 1c57e2c..f7176f3 100644
--- a/m4/af_alg.m4
+++ b/m4/af_alg.m4
@@ -1,6 +1,4 @@
-# af_alg.m4 serial 1
-dnl Check whether linux/if_alg.h has needed features.
-
+# af_alg.m4 serial 2
dnl Copyright 2018 Free Software Foundation, Inc.
dnl This file is free software; the Free Software Foundation
dnl gives unlimited permission to copy and/or distribute it,
@@ -11,6 +9,8 @@ dnl From Matteo Croce.
AC_DEFUN_ONCE([gl_AF_ALG],
[
AC_REQUIRE([gl_HEADER_SYS_SOCKET])
+
+ dnl Check whether linux/if_alg.h has needed features.
AC_CACHE_CHECK([whether linux/if_alg.h has struct sockaddr_alg.],
[gl_cv_header_linux_if_alg_salg],
[AC_COMPILE_IFELSE(
@@ -27,4 +27,25 @@ AC_DEFUN_ONCE([gl_AF_ALG],
AC_DEFINE([HAVE_LINUX_IF_ALG_H], [1],
[Define to 1 if you have 'struct sockaddr_alg' defined.])
fi
+
+ dnl The default is to use AF_ALG if available.
+ use_af_alg=yes
+ AC_ARG_WITH([linux-crypto],
+ [AS_HELP_STRING([[--without-linux-crypto]],
+ [Do not use Linux kernel cryptographic API (default is to use it if available)])
+ ],
+ [use_af_alg=$withval],
+ [use_af_alg=yes])
+ dnl We cannot use it if it is not available.
+ if test "$gl_cv_header_linux_if_alg_salg" != yes; then
+ use_af_alg=no
+ fi
+
+ if test "$use_af_alg" != no; then
+ USE_AF_ALG=1
+ else
+ USE_AF_ALG=0
+ fi
+ AC_DEFINE_UNQUOTED([USE_LINUX_CRYPTO_API], [$USE_AF_ALG],
+ [Define to 1 if you want to use the Linux kernel cryptographic API.])
])

[Assaf Gordon]

Hello Bruno and all,

Post by Bruno Haible
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.

Post by Bruno Haible
* 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.

Post by Bruno Haible
* 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.

Post by Bruno Haible
* 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.

Post by Bruno Haible
* 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

[Paul Eggert]

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

This is a good point; it's easier to understand and explain if the two options
have similar defaults.

Post by Assaf Gordon
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).

Is there some easy way to detect whether the hardware supports crypto directly?
If so, we could use the new interface only on platforms where that is true. This
should alleviate many of the qualms you express.

Post by Assaf Gordon
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.

Why won't sysadmins be able to diagnose this? Isn't %SYS time accounted for?

Post by Assaf Gordon
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.
if someone explicitly asked to use OpenSSL, they certainly don't
want to not use it...

Good catch, thanks.

Perhaps we should have one --with option rather than two? This might help reduce
the confusion mentioned above.

I installed the attached patch to try to fix the minor glitches you mentioned.
The code glitches were my fault (blush).