[dm-crypt] [PATCH] Speed up keyfile reading

* [dm-crypt] [PATCH] Speed up keyfile reading
@ 2017-08-12 18:26 angelomariafederichini191269
  2017-08-12 19:37 ` Milan Broz
  2017-08-15  9:52 ` Milan Broz
  0 siblings, 2 replies; 11+ messages in thread
From: angelomariafederichini191269 @ 2017-08-12 18:26 UTC (permalink / raw)
  To: dm-crypt


[-- Attachment #1.1: Type: text/plain, Size: 4746 bytes --]

Hi

(Sorry if this mailing list isn't the right place for patches but I
couldn't find any instructions on how to contribute. Maybe that's worth
a FAQ entry by its own.)

I'm using a keyfile instead of a passphrase for my encrypted volumes.
What me bothered a long time is that it takes nearly 30 seconds to open
the volume. Now I had a little time and dug into this itch.

I discovered that the crypt_keyfile_read() function in lib/utils_crypt.c
reads the keyfile one byte at a time "read(fd, &pass[i], 1)".

Attached you find a patch which enhances the loop to use bulk read
operations instead of single byte ones. To set char_to_read to the
minimum of two values I currently use the ternary operator :? because C
does not offer a standard function for this simple job. Usually one
would define an separate helper function, but I wanted to keep the patch
small&simple and also did not know if that's the "style" of cryptsetup
or if a preprocessor definition would be more appreciated.

Another note about the implementation: If we scan for an EOL then we
fall back to reading one char at a time. This way we ensure backwards
compatibility and do not read more than necessary from the file
description.

Anyway, in my test scenario this patch reduces the opening time from
25-26 seconds to 14-15 seconds. A 10 seconds improvement yeah!
(I did a very simple measurement: "time cryptsetup open ..." where the
keyfile as well as the volume file resided on a tmpfs in memory.)

I also attached a second patch which sets the initial buffer size to the
requested keyfile_size. This prevents increasing the buffer in 4k steps
over and over again if the keyfile_size is known in advance (i.e. if the
parameter 'keyfile_size_max' is set to something non-zero).

With this patch (and passing --keyfile-size parameter) opening a volume
now takes 5 seconds. Again a 10 seconds improvement. I split this into a
separate patch because I'm unsure if it is intended to update the buffer
in 4k steps as the comment "/* use 4k for buffer (page divisor but avoid
huge pages) */" suggests.

Below is the "test case code" (I wouldn't call it code). It is not a patch
because I did not understand the test suite. So someone with more knowledge
might want to convert these into more suitable test cases. The tests also need
manual supervision (looking at the output to decide if the test succeeded).

Kind regards

#!/bin/sh
set -e # abort on error

truncate -s 100M test_volume.vol
cryptsetup luksFormat test_volume.vol
# Password: "somesome"

printf "1: should fail since 'somesomething' is consumed completely \n"
#printf "somesomething\n"   | (cryptsetup open test_volume.vol test_volume; cat)
#cryptsetup close test_volume
printf "2: should succeed and print 'thing' (which is not consumed by cryptsetup)\n"
printf "somesome\nthing\n" | (cryptsetup open test_volume.vol test_volume; cat)
cryptsetup close test_volume
printf "3: should fail since 'somesomething\\n' is consumed completely\n"
#printf "somesomething\n"   | (cryptsetup --key-file - open test_volume.vol test_volume; cat)
#cryptsetup close test_volume
printf "4: should fail since 'somesome\\nthing\\n' is consumed completely\n"
#printf "somesome\nthing\n" | (cryptsetup --key-file - open test_volume.vol test_volume; cat)
#cryptsetup close test_volume
printf "5: should fail\n"
mkfifo key.fifo
printf "somesome\nthing\n" > key.fifo &
# should fail
#cryptsetup --key-file key.fifo open test_volume.vol test_volume
#cryptsetup close test_volume
printf "END\n" > key.fifo & # make sure some data is in the fifo otherwise the following cat will block
cat key.fifo
rm key.fifo

printf "6: should succeed due to --keyfile-size passed correctly\n"
printf "somesomething\n"   | (cryptsetup --keyfile-size 8 open test_volume.vol test_volume; cat)
cryptsetup close test_volume
printf "7: should succeed due to --keyfile-size passed correctly\n"
printf "somesome\nthing\n" | (cryptsetup --keyfile-size 8 open test_volume.vol test_volume; cat)
cryptsetup close test_volume
printf "8: should succeed due to --keyfile-size passed correctly\n"
printf "somesomething\n"   | (cryptsetup --keyfile-size 8 --key-file - open test_volume.vol test_volume; cat)
cryptsetup close test_volume
printf "9: should succeed due to --keyfile-size passed correctly\n"
printf "somesome\nthing\n" | (cryptsetup --keyfile-size 8 --key-file - open test_volume.vol test_volume; cat)
cryptsetup close test_volume

printf "10: should succeed\n"
mkfifo key.fifo
printf "somesome\nthing\n" > key.fifo &
cryptsetup --keyfile-size 8 --key-file key.fifo open test_volume.vol test_volume
cryptsetup close test_volume
printf "END\n" > key.fifo & # make sure some data is in the fifo otherwise the following cat will block
cat key.fifo
rm key.fifo

[-- Attachment #1.2: Type: text/html, Size: 6284 bytes --]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Use-bulk-read-when-reading-keyfile.patch --]
[-- Type: text/x-patch; name="0001-Use-bulk-read-when-reading-keyfile.patch", Size: 2442 bytes --]

From bdc5cf5c0b666cff07629c6920c2d910e4b7e1fd Mon Sep 17 00:00:00 2001
From: angelomariafederichini191269@protonmail.com
Date: Sat, 12 Aug 2017 09:11:12 +0200
Subject: [PATCH] Use bulk read when reading keyfile

If reading a keyfile use bulk read operations instead of reading one
character at the time. This speeds up reading larger keyfiles.

If read should stop at a EOL, then fallback to reading one character at
the time to not read anything beyond the EOL character.
---
 lib/utils_crypt.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/lib/utils_crypt.c b/lib/utils_crypt.c
index 8021d05..9063865 100644
--- a/lib/utils_crypt.c
+++ b/lib/utils_crypt.c
@@ -246,7 +246,7 @@ int crypt_keyfile_read(struct crypt_device *cd,  const char *keyfile,
 		       size_t keyfile_offset, size_t keyfile_size_max,
 		       uint32_t flags)
 {
-	int fd, regular_file, char_read, unlimited_read = 0;
+	int fd, regular_file, char_to_read = 0, char_read = 0, unlimited_read = 0;
 	int r = -EINVAL, newline;
 	char *pass = NULL;
 	size_t buflen, i, file_read_size;
@@ -311,7 +311,7 @@ int crypt_keyfile_read(struct crypt_device *cd,  const char *keyfile,
 		goto out_err;
 	}
 
-	for(i = 0, newline = 0; i < keyfile_size_max; i++) {
+	for(i = 0, newline = 0; i < keyfile_size_max; i += char_read) {
 		if(i == buflen) {
 			buflen += 4096;
 			pass = crypt_safe_realloc(pass, buflen);
@@ -322,16 +322,28 @@ int crypt_keyfile_read(struct crypt_device *cd,  const char *keyfile,
 			}
 		}
 
-		char_read = read(fd, &pass[i], 1);
+		if (flags & CRYPT_KEYFILE_STOP_EOL) {
+			/* If we should stop on newline, we must read the input
+			 * one character at the time. Otherwise we might end up
+			 * having read some bytes after the newline, which we
+			 * promised not to do.
+			 */
+			char_to_read = 1;
+		} else {
+			/* char_to_read = min(keyfile_size_max - i, buflen - i) */
+			char_to_read = keyfile_size_max < buflen ?
+				keyfile_size_max - i : buflen - i;
+		}
+		char_read = read(fd, &pass[i], char_to_read);
 		if (char_read < 0) {
 			log_err(cd, _("Error reading passphrase.\n"));
 			r = -EPIPE;
 			goto out_err;
 		}
 
-		/* Stop on newline only if not requested read from keyfile */
 		if (char_read == 0)
 			break;
+		/* Stop on newline only if not requested read from keyfile */
 		if ((flags & CRYPT_KEYFILE_STOP_EOL) && pass[i] == '\n') {
 			newline = 1;
 			pass[i] = '\0';
-- 
2.13.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-Allocate-suitable-sized-buffer-when-reading-a-keyfil.patch --]
[-- Type: text/x-patch; name="0002-Allocate-suitable-sized-buffer-when-reading-a-keyfil.patch", Size: 1183 bytes --]

From 3a7bf02cfe4097c66638f7bbd40a776abe4a329f Mon Sep 17 00:00:00 2001
From: angelomariafederichini191269@protonmail.com
Date: Sat, 12 Aug 2017 16:01:23 +0200
Subject: [PATCH] Allocate suitable sized buffer when reading a keyfile

If the keyfile size is explicitly given, then allocate a suitable sized
buffer right from the start instead of increasing it in 4k steps. This
speeds up reading larger keyfiles.
---
 lib/utils_crypt.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/lib/utils_crypt.c b/lib/utils_crypt.c
index 9063865..0ba469c 100644
--- a/lib/utils_crypt.c
+++ b/lib/utils_crypt.c
@@ -271,10 +271,12 @@ int crypt_keyfile_read(struct crypt_device *cd,  const char *keyfile,
 	if (keyfile_size_max == 0) {
 		keyfile_size_max = DEFAULT_KEYFILE_SIZE_MAXKB * 1024 + 1;
 		unlimited_read = 1;
+		/* use 4k for buffer (page divisor but avoid huge pages) */
+		buflen = 4096 - sizeof(struct safe_allocation);
+	} else {
+		buflen = keyfile_size_max;
 	}
 
-	/* use 4k for buffer (page divisor but avoid huge pages) */
-	buflen = 4096 - sizeof(struct safe_allocation);
 	regular_file = 0;
 	if (keyfile) {
 		if(stat(keyfile, &st) < 0) {
-- 
2.13.0


^ permalink raw reply related	[flat|nested] 11+ messages in thread