Bug#722898: wording change and wrap up
As the last pending item, I've implemented the suggested wording to
explain the cancellation of wiping.
All changes have been tested. From my point of view, this bug now has
been fully dealt with and can be closed once the patches have been
committed and uploaded. Yet, if there are any suggestions for
improvement, I'm happy to incorporate them.
To ease wrapping up, I'm attaching the complete patch set (git logs
are given below).
Cheers,
Thiemo
#1 blockdev-wipe: Reduce progress indicator granularity to 1/1000
#2 blockdev-wipe: Allocate buffer from heap instead of stack
This allows to use buffers larger than 8M, also it fails more
gracefully in case the memory can't be allocated.
#3 blockdev-wipe: Sync at most once per second
Don't open output devices with O_SYNC, instead sync manually
every time the progress indicator is updated, but not more
often than once per second. This yields performance gains
of up to factor 10 in setups with dm-crypt on dm-raid.
Note: Seven years ago, O_SYNC was added to fix OOM issues
(cf. bug #381135), however it is believed that this problem
has been addressed in the kernel by now.
#4 blockdev-wipe: Set blocksize to 512k
This should be large enough to avoid excessive system call
overhead and small enough to prevent problems on systems with
very little RAM.
#5 Add description to explain cancellation of blockdev-wipe
Mostly as discussed in bug 722898. However, as it has turned out that the
different kinds of erase (non-crypto-volumes are overwritten with zeroes,
crypto-volumes are overwritten with random data) require different messages,
separate templates are used now.
The debconf progress bar frame doesn't print extended descriptions (at least
not in newt), therefore the message is squeezed into the short description
slightly awkwardly.
From 76ac03df4fcd48bfcbb842f8fe83f4c0f409cec9 Mon Sep 17 00:00:00 2001
From: Thiemo Nagel <thiemo.nagel@gmail.com>
Date: Mon, 16 Sep 2013 18:00:19 +0200
Subject: [PATCH 1/8] blockdev-wipe: Reduce progress indicator granularity to
1/1000
---
blockdev-wipe/blockdev-wipe.c | 2 +-
lib/crypto-base.sh | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/blockdev-wipe/blockdev-wipe.c b/blockdev-wipe/blockdev-wipe.c
index 268d969..2b18ed1 100644
--- a/blockdev-wipe/blockdev-wipe.c
+++ b/blockdev-wipe/blockdev-wipe.c
@@ -30,7 +30,7 @@
/* Progress indicator to output */
# define PROGRESS_INDICATOR "*\n"
/* How many progress indicators to output in total */
-#define PROGRESS_PARTS 65536
+#define PROGRESS_PARTS 1000
/* How many bytes to write at a time (default) */
#define DEFAULT_WSIZE 4096
/* Debugging output */
diff --git a/lib/crypto-base.sh b/lib/crypto-base.sh
index e0f8e77..8103adb 100644
--- a/lib/crypto-base.sh
+++ b/lib/crypto-base.sh
@@ -292,7 +292,7 @@ crypto_do_wipe () {
cancelled=0
db_capb backup align progresscancel
- db_progress START 0 65536 $template
+ db_progress START 0 1000 $template
while read x <&9; do
db_progress STEP 1
if [ $? -eq 30 ]; then
--
1.7.10.4
From 9d7955254d9d9303bea7fbe81fb39f93f96ed562 Mon Sep 17 00:00:00 2001
From: Thiemo Nagel <thiemo.nagel@gmail.com>
Date: Mon, 16 Sep 2013 18:38:26 +0200
Subject: [PATCH 2/8] blockdev-wipe: Allocate buffer from heap instead of
stack
This allows to use buffers larger than 8M, also it fails more
gracefully in case the memory can't be allocated.
---
blockdev-wipe/blockdev-wipe.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/blockdev-wipe/blockdev-wipe.c b/blockdev-wipe/blockdev-wipe.c
index 2b18ed1..b1e5a55 100644
--- a/blockdev-wipe/blockdev-wipe.c
+++ b/blockdev-wipe/blockdev-wipe.c
@@ -79,11 +79,13 @@ static int do_wipe(int source, int target, size_t wsize)
unsigned long long done = 0;
unsigned int previous_progress = 0;
unsigned int progress = 0;
- char buf[wsize];
int i;
ssize_t count;
+ char *buf = calloc(wsize, 1);
+
+ if (buf == NULL)
+ die("buffer allocation failed", 1);
- memset(buf, '\0', wsize);
size = dev_size(target);
dprintf("Block size in bytes is %llu\n", size);
/* From now on, try to make sure stdout is unbuffered */
@@ -112,6 +114,7 @@ static int do_wipe(int source, int target, size_t wsize)
previous_progress = progress;
}
+ free(buf);
return 0;
}
--
1.7.10.4
From b497b8866f0463a9825d86926255aa400c26b03a Mon Sep 17 00:00:00 2001
From: Thiemo Nagel <thiemo.nagel@gmail.com>
Date: Mon, 16 Sep 2013 18:57:56 +0200
Subject: [PATCH 3/8] blockdev-wipe: Sync at most once per second
Don't open output devices with O_SYNC, instead sync manually
every time the progress indicator is updated, but not more
often than once per second. This yields performance gains
of up to factor 10 in setups with dm-crypt on dm-raid.
Note: Seven years ago, O_SYNC was added to fix OOM issues
(cf. bug #381135), however it is believed that this problem
has been addressed in the kernel by now.
---
blockdev-wipe/blockdev-wipe.c | 19 +++++++++++++++----
1 file changed, 15 insertions(+), 4 deletions(-)
diff --git a/blockdev-wipe/blockdev-wipe.c b/blockdev-wipe/blockdev-wipe.c
index b1e5a55..068fe28 100644
--- a/blockdev-wipe/blockdev-wipe.c
+++ b/blockdev-wipe/blockdev-wipe.c
@@ -26,6 +26,7 @@
#include <sys/mount.h> /* for BLKGETSIZE definitions */
#include <getopt.h>
#include <string.h>
+#include <time.h>
/* Progress indicator to output */
# define PROGRESS_INDICATOR "*\n"
@@ -79,6 +80,7 @@ static int do_wipe(int source, int target, size_t wsize)
unsigned long long done = 0;
unsigned int previous_progress = 0;
unsigned int progress = 0;
+ time_t last_sync = 0;
int i;
ssize_t count;
char *buf = calloc(wsize, 1);
@@ -109,12 +111,21 @@ static int do_wipe(int source, int target, size_t wsize)
done += count;
progress = ((done * PROGRESS_PARTS)/ size);
dprintf("We just wrote %zi, done %llu\n", count, done);
- for (i = 0; i < progress - previous_progress; i++)
- printf(PROGRESS_INDICATOR);
- previous_progress = progress;
+ if (progress > previous_progress) {
+ /* sync on every progress output, but at most once per second */
+ time_t now = time(NULL);
+ if (now != last_sync) {
+ fdatasync(target);
+ last_sync = now;
+ }
+ for (i = 0; i < progress - previous_progress; i++)
+ printf(PROGRESS_INDICATOR);
+ previous_progress = progress;
+ }
}
free(buf);
+ fdatasync(target);
return 0;
}
@@ -147,7 +158,7 @@ int main(int argc, char **argv, char **envp)
usage("you must specify one target device", argv[0]);
/* Get target */
- target = open(argv[optind], O_WRONLY | O_SYNC);
+ target = open(argv[optind], O_WRONLY);
if (target < 0)
die("failed to open device", 1);
--
1.7.10.4
From b29adacadd34412d248b765694aaeb1b9f34a906 Mon Sep 17 00:00:00 2001
From: Thiemo Nagel <thiemo.nagel@gmail.com>
Date: Mon, 16 Sep 2013 19:44:33 +0200
Subject: [PATCH 4/8] blockdev-wipe: Set blocksize to 512k
This should be large enough to avoid excessive system call
overhead and small enough to prevent problems on systems with
very little RAM.
---
lib/crypto-base.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/crypto-base.sh b/lib/crypto-base.sh
index 8103adb..d2eb9eb 100644
--- a/lib/crypto-base.sh
+++ b/lib/crypto-base.sh
@@ -287,7 +287,7 @@ crypto_do_wipe () {
fifo=/var/run/wipe_progress
mknod $fifo p
- /bin/blockdev-wipe -s 65536 $dev > $fifo &
+ /bin/blockdev-wipe -s $((512*1024)) $dev > $fifo &
pid=$!
cancelled=0
--
1.7.10.4
From 1deb9b23df6e3688c07df0d190c0e3110d67a80b Mon Sep 17 00:00:00 2001
From: Thiemo Nagel <thiemo.nagel@gmail.com>
Date: Tue, 22 Oct 2013 18:23:25 +0200
Subject: [PATCH 5/8] Add description to explain cancellation of blockdev-wipe
Mostly as discussed in bug 722898. However, as it has turned out that the
different kinds of erase (non-crypto-volumes are overwritten with zeroes,
crypto-volumes are overwritten with random data) require different messages,
separate templates are used now.
The debconf progress bar frame doesn't print extended descriptions (at least
not in newt), therefore the message is squeezed into the short description
slightly awkwardly.
---
debian/partman-crypto.templates | 46 ++++++++++++++++++++++++++++++++++-----
lib/crypto-base.sh | 22 +++++++++++++------
2 files changed, 56 insertions(+), 12 deletions(-)
diff --git a/debian/partman-crypto.templates b/debian/partman-crypto.templates
index 3728a61..05253fa 100644
--- a/debian/partman-crypto.templates
+++ b/debian/partman-crypto.templates
@@ -144,7 +144,36 @@ Type: text
# :sl3:
_Description: Erase data on this partition
-Template: partman-crypto/warn_erase
+# Overwrite with zeroes
+Template: partman-crypto/plain_warn_erase
+Type: boolean
+Default: false
+# :sl3:
+_Description: Really erase the data on ${DEVICE}?
+ The data on ${DEVICE} will be overwritten with zeroes. It can no longer be
+ recovered after this step has completed. This is the last opportunity to
+ abort the erase.
+
+Template: partman-crypto/progress/plain_erase_title
+Type: text
+# :sl3:
+_Description: Erasing data on ${DEVICE}
+
+Template: partman-crypto/progress/plain_erase_text
+Type: text
+# :sl3:
+_Description: The installer is now overwriting ${DEVICE} with zeroes to delete its previous contents. This step may be skipped by cancelling this action.
+
+Template: partman-crypto/plain_erase_failed
+Type: error
+# :sl3:
+_Description: Erasing data on ${DEVICE} failed
+ An error occurred while trying to overwrite the data on ${DEVICE} with
+ zeroes. The data has not been erased.
+
+
+# Cryptographic overwrite to protect metadata
+Template: partman-crypto/crypto_warn_erase
Type: boolean
Default: false
# :sl3:
@@ -153,17 +182,24 @@ _Description: Really erase the data on ${DEVICE}?
longer be recovered after this step has completed. This is the last
opportunity to abort the erase.
-Template: partman-crypto/progress/erase
+Template: partman-crypto/progress/crypto_erase_title
Type: text
# :sl3:
_Description: Erasing data on ${DEVICE}
-Template: partman-crypto/erase_failed
+Template: partman-crypto/progress/crypto_erase_text
+Type: text
+# :sl3:
+_Description: The installer is now overwriting ${DEVICE} with random data to prevent meta-information leaks from the encrypted volume. This step may be skipped by cancelling this action, albeit at the expense of a slight reduction of the quality of the encryption.
+
+Template: partman-crypto/crypto_erase_failed
Type: error
# :sl3:
_Description: Erasing data on ${DEVICE} failed
- An error occurred trying to erase the data on ${DEVICE}. The data has
- not been erased.
+ An error occurred while trying to overwrite ${DEVICE} with random
+ data. Recovery of the device's previous contents is possible and
+ meta-information of its new contents may be leaked.
+
Template: partman/progress/init/crypto
Type: text
diff --git a/lib/crypto-base.sh b/lib/crypto-base.sh
index d2eb9eb..b0037b3 100644
--- a/lib/crypto-base.sh
+++ b/lib/crypto-base.sh
@@ -292,7 +292,8 @@ crypto_do_wipe () {
cancelled=0
db_capb backup align progresscancel
- db_progress START 0 1000 $template
+ db_progress START 0 1000 ${template}_title
+ db_progress INFO ${template}_text
while read x <&9; do
db_progress STEP 1
if [ $? -eq 30 ]; then
@@ -322,9 +323,15 @@ crypto_wipe_device () {
fi
ret=1
+ if [ -r $part/crypto_type ] && [ "$(cat $part/crypto_type)" = dm-crypt ]; then
+ type=crypto
+ else
+ type=plain
+ fi
+
if [ $interactive = yes ]; then
# Confirm before erasing
- template="partman-crypto/warn_erase"
+ template="partman-crypto/${type}_warn_erase"
db_set $template false
db_subst $template DEVICE $(humandev $device)
db_input critical $template || true
@@ -336,7 +343,7 @@ crypto_wipe_device () {
fi
# Setup crypto
- if [ $method = dm-crypt ]; then
+ if [ "$type" = crypto ]; then
targetdevice=$(get_free_mapping)
setup_dmcrypt $targetdevice $device aes xts-plain64 plain 128 /dev/urandom || return 1
targetdevice="/dev/mapper/$targetdevice"
@@ -346,10 +353,11 @@ crypto_wipe_device () {
fi
# Erase
- template="partman-crypto/progress/erase"
- db_subst $template DEVICE $(humandev $device)
+ template="partman-crypto/progress/${type}_erase"
+ db_subst ${template}_title DEVICE $(humandev $device)
+ db_subst ${template}_text DEVICE $(humandev $device)
if ! crypto_do_wipe $template $targetdevice; then
- template="partman-crypto/erase_failed"
+ template="partman-crypto/${type}_erase_failed"
db_subst $template DEVICE $(humandev $device)
db_input critical $template || true
db_go
@@ -358,7 +366,7 @@ crypto_wipe_device () {
fi
# Teardown crypto
- if [ $method = dm-crypt ]; then
+ if [ "$type" = crypto ]; then
log-output -t partman-crypto /sbin/cryptsetup remove ${targetdevice##/dev/mapper/}
fi
--
1.7.10.4
Reply to: