[PATCH 0/4] pvcreate & detection of foreign signatures

[PATCH 0/4] pvcreate & detection of foreign signatures

[Milan Broz]

After the third report of broken LUKS after pvcreate (just this week...)
I added LUKS detection to lvm. But the current code is messy and in fact
it wipes superblocks without asking by default.

This is attempt to change that to something more user friendly.

Milan Broz (4):
  Change the pvcreate swap/md logic
  Remove assumption that --ye must be used only in --force mode
  Fix file open leak in swap signature detection
  Detect LUKS signature in pvcreate

 lib/Makefile.in         |    1 +
 lib/device/dev-luks.c   |   43 ++++++++++++++++++++++++++++
 lib/device/dev-swap.c   |   12 +++----
 lib/device/device.h     |    1 +
 lib/metadata/metadata.c |   71 +++++++++++++++++++++++++----------------------
 tools/pvremove.c        |    5 ---
 tools/toollib.c         |    5 ---
 7 files changed, 88 insertions(+), 50 deletions(-)
 create mode 100644 lib/device/dev-luks.c

[PATCH 1/4] Change the pvcreate swap/md logic

[Milan Broz]

pvcreate detects MD and swap signature.

The logic hidden there is not only documented but it is also
user unfriendly. Who invented this logic should run pvcreate
on its own critical MD device to see why;-)

This patch
 - create one function instead of duplication code
 - asks if user want to overwrite signature
 - allow aborting (!)
 (Please note that writing LVM signarute without wiping old
 is wrong, it confuses blkid, MD will not work anyway and
 swap and LUKS is broken too.)

Question is ignore if --yes is given or --force switch is used.
(See later patch also.)

Signed-off-by: Milan Broz <mbroz@redhat.com>
---
 lib/metadata/metadata.c |   70 ++++++++++++++++++++++++-----------------------
 1 files changed, 36 insertions(+), 34 deletions(-)

diff --git a/lib/metadata/metadata.c b/lib/metadata/metadata.c
index 6cd7793..db196d6 100644
--- a/lib/metadata/metadata.c
+++ b/lib/metadata/metadata.c
@@ -1526,6 +1526,40 @@ int vg_split_mdas(struct cmd_context *cmd __attribute__((unused)),
 	return 1;
 }
 
+static int _wipe_sb(struct device *dev, const char *type, const char *name,
+		    int wipe_len, struct pvcreate_params *pp,
+		    int (*func)(struct device *dev, uint64_t *signature))
+{
+	int wipe;
+	uint64_t superblock;
+
+	wipe = func(dev, &superblock);
+	if (wipe == -1) {
+		log_error("Fatal error while trying to detect %s on %s.",
+			  type, name);
+		return 0;
+	}
+
+	if (wipe == 0)
+		return 1;
+
+	/* Specifying --yes => do not ask. */
+	if (!pp->yes && (pp->force == PROMPT) &&
+	    yes_no_prompt("WARNING: %s detected on %s. Wipe it? [y/n] ",
+			  type, name) != 'y') {
+		log_error("Aborting pvcreate on %s.", name);
+		return 0;
+	}
+
+	log_print("Wiping %s on %s.", type, name);
+	if (!dev_set(dev, superblock, wipe_len, 0)) {
+		log_error("Failed to wipe %s on %s.", type, name);
+		return 0;
+	}
+
+	return 1;
+}
+
 /*
  * See if we may pvcreate on this device.
  * 0 indicates we may not.
@@ -1535,8 +1569,6 @@ static int pvcreate_check(struct cmd_context *cmd, const char *name,
 {
 	struct physical_volume *pv;
 	struct device *dev;
-	uint64_t md_superblock, swap_signature;
-	int wipe_md, wipe_swap;
 	struct dm_list mdas;
 
 	dm_list_init(&mdas);
@@ -1602,41 +1634,11 @@ static int pvcreate_check(struct cmd_context *cmd, const char *name,
 		return 0;
 	}
 
-	/* Wipe superblock? */
-	if ((wipe_md = dev_is_md(dev, &md_superblock)) == 1 &&
-	    ((!pp->idp && !pp->restorefile) || pp->yes ||
-	     (yes_no_prompt("Software RAID md superblock "
-			    "detected on %s. Wipe it? [y/n] ", name) == 'y'))) {
-		log_print("Wiping software RAID md superblock on %s", name);
-		if (!dev_set(dev, md_superblock, 4, 0)) {
-			log_error("Failed to wipe RAID md superblock on %s",
-				  name);
-			return 0;
-		}
-	}
-
-	if (wipe_md == -1) {
-		log_error("Fatal error while trying to detect software "
-			  "RAID md superblock on %s", name);
+	if (!_wipe_sb(dev, "software RAID md superblock", name, 4, pp, dev_is_md))
 		return 0;
-	}
-
-	if ((wipe_swap = dev_is_swap(dev, &swap_signature)) == 1 &&
-	    ((!pp->idp && !pp->restorefile) || pp->yes ||
-	     (yes_no_prompt("Swap signature detected on %s. Wipe it? [y/n] ",
-			    name) == 'y'))) {
-		log_print("Wiping swap signature on %s", name);
-		if (!dev_set(dev, swap_signature, 10, 0)) {
-			log_error("Failed to wipe swap signature on %s", name);
-			return 0;
-		}
-	}
 
-	if (wipe_swap == -1) {
-		log_error("Fatal error while trying to detect swap "
-			  "signature on %s", name);
+	if (!_wipe_sb(dev, "swap signature", name, 10, pp, dev_is_swap))
 		return 0;
-	}
 
 	if (sigint_caught())
 		return 0;
-- 
1.7.1

[PATCH 2/4] Remove assumption that --ye must be used only in --force mode

[Milan Broz]

This is not only undocumented but is is also in violation with --help
documentation.

Using --yes without --force is useful in pvcreate when it detects
old signature.

Signed-off-by: Milan Broz <mbroz@redhat.com>
---
 tools/pvremove.c |    5 -----
 tools/toollib.c  |    5 -----
 2 files changed, 0 insertions(+), 10 deletions(-)

diff --git a/tools/pvremove.c b/tools/pvremove.c
index 8242993..c57df64 100644
--- a/tools/pvremove.c
+++ b/tools/pvremove.c
@@ -143,11 +143,6 @@ int pvremove(struct cmd_context *cmd, int argc, char **argv)
 		return EINVALID_CMD_LINE;
 	}
 
-	if (arg_count(cmd, yes_ARG) && !arg_count(cmd, force_ARG)) {
-		log_error("Option y can only be given with option f");
-		return EINVALID_CMD_LINE;
-	}
-
 	for (i = 0; i < argc; i++) {
 		r = pvremove_single(cmd, argv[i], NULL);
 		if (r > ret)
diff --git a/tools/toollib.c b/tools/toollib.c
index bdb52d5..5da30f4 100644
--- a/tools/toollib.c
+++ b/tools/toollib.c
@@ -1344,11 +1344,6 @@ int pvcreate_params_validate(struct cmd_context *cmd,
 		return 0;
 	}
 
-	if (arg_count(cmd, yes_ARG) && !arg_count(cmd, force_ARG)) {
-		log_error("Option y can only be given with option f");
-		return 0;
-	}
-
 	pp->yes = arg_count(cmd, yes_ARG);
 	pp->force = arg_count(cmd, force_ARG);
 
-- 
1.7.1

[PATCH 3/4] Fix file open leak in swap signature detection

[Milan Broz]

Signed-off-by: Milan Broz <mbroz@redhat.com>
---
 lib/device/dev-swap.c |   12 +++++-------
 1 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/lib/device/dev-swap.c b/lib/device/dev-swap.c
index 62c2bb5..b8ebcca 100644
--- a/lib/device/dev-swap.c
+++ b/lib/device/dev-swap.c
@@ -42,7 +42,7 @@ int dev_is_swap(struct device *dev, uint64_t *signature)
 {
 	char buf[10];
 	uint64_t size;
-	int page;
+	int page, ret = 0;
 
 	if (!dev_get_size(dev, &size)) {
 		stack;
@@ -66,11 +66,12 @@ int dev_is_swap(struct device *dev, uint64_t *signature)
 			break;
 		if (!dev_read(dev, page - SIGNATURE_SIZE,
 			      SIGNATURE_SIZE, buf)) {
-			stack;
-			return -1;
+			ret = -1;
+			break;
 		}
 		if (_swap_detect_signature(buf)) {
 			*signature = page - SIGNATURE_SIZE;
+			ret = 1;
 			break;
 		}
 	}
@@ -78,10 +79,7 @@ int dev_is_swap(struct device *dev, uint64_t *signature)
 	if (!dev_close(dev))
 		stack;
 
-	if (*signature)
-		return 1;
-
-	return 0;
+	return ret;
 }
 
 #endif
-- 
1.7.1

[PATCH 4/4] Detect LUKS signature in pvcreate

[Milan Broz]

One shiny day we should use libblkid here. But now using LUKS is
very common together with LVM and pvcreate destroys LUKS completely.

So for user's convenience, try to detect LUKS signature and allow abort.

Signed-off-by: Milan Broz <mbroz@redhat.com>
---
 lib/Makefile.in         |    1 +
 lib/device/dev-luks.c   |   43 +++++++++++++++++++++++++++++++++++++++++++
 lib/device/device.h     |    1 +
 lib/metadata/metadata.c |    3 +++
 4 files changed, 48 insertions(+), 0 deletions(-)
 create mode 100644 lib/device/dev-luks.c

diff --git a/lib/Makefile.in b/lib/Makefile.in
index ae81040..1c58bdd 100644
--- a/lib/Makefile.in
+++ b/lib/Makefile.in
@@ -47,6 +47,7 @@ SOURCES =\
 	device/dev-io.c \
 	device/dev-md.c \
 	device/dev-swap.c \
+	device/dev-luks.c \
 	device/device.c \
 	display/display.c \
 	error/errseg.c \
diff --git a/lib/device/dev-luks.c b/lib/device/dev-luks.c
new file mode 100644
index 0000000..6337992
--- /dev/null
+++ b/lib/device/dev-luks.c
@@ -0,0 +1,43 @@
+/*
+ * Copyright (C) 2010 Red Hat, Inc. All rights reserved.
+ *
+ * This file is part of LVM2.
+ *
+ * This copyrighted material is made available to anyone wishing to use,
+ * modify, copy, or redistribute it subject to the terms and conditions
+ * of the GNU Lesser General Public License v.2.1.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program; if not, write to the Free Software Foundation,
+ * Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ */
+
+#include "lib.h"
+#include "metadata.h"
+
+#define LUKS_SIGNATURE "LUKS\xba\xbe"
+#define LUKS_SIGNATURE_SIZE 6
+
+int dev_is_luks(struct device *dev, uint64_t *signature)
+{
+	char buf[LUKS_SIGNATURE_SIZE];
+	int ret = -1;
+
+	if (!dev_open(dev)) {
+		stack;
+		return -1;
+	}
+
+	*signature = 0;
+
+	if (!dev_read(dev, 0, LUKS_SIGNATURE_SIZE, buf))
+		goto_out;
+
+	ret = memcmp(buf, LUKS_SIGNATURE, LUKS_SIGNATURE_SIZE) ? 0 : 1;
+
+out:
+	if (!dev_close(dev))
+		stack;
+
+	return ret;
+}
diff --git a/lib/device/device.h b/lib/device/device.h
index 8bf33e6..5a59950 100644
--- a/lib/device/device.h
+++ b/lib/device/device.h
@@ -96,6 +96,7 @@ const char *dev_name_confirmed(struct device *dev, int quiet);
 /* Does device contain md superblock?  If so, where? */
 int dev_is_md(struct device *dev, uint64_t *sb);
 int dev_is_swap(struct device *dev, uint64_t *signature);
+int dev_is_luks(struct device *dev, uint64_t *signature);
 unsigned long dev_md_stripe_width(const char *sysfs_dir, struct device *dev);
 
 int is_partitioned_dev(struct device *dev);
diff --git a/lib/metadata/metadata.c b/lib/metadata/metadata.c
index db196d6..ed00ffa 100644
--- a/lib/metadata/metadata.c
+++ b/lib/metadata/metadata.c
@@ -1640,6 +1640,9 @@ static int pvcreate_check(struct cmd_context *cmd, const char *name,
 	if (!_wipe_sb(dev, "swap signature", name, 10, pp, dev_is_swap))
 		return 0;
 
+	if (!_wipe_sb(dev, "LUKS signature", name, 8, pp, dev_is_luks))
+		return 0;
+
 	if (sigint_caught())
 		return 0;
 
-- 
1.7.1

[PATCH 0/4] pvcreate & detection of foreign signatures

[Petr Rockai]

Hi,

Milan Broz <mbroz@redhat.com> writes:
> After the third report of broken LUKS after pvcreate (just this week...)
> I added LUKS detection to lvm. But the current code is messy and in fact
> it wipes superblocks without asking by default.
>
> This is attempt to change that to something more user friendly.
Looks OK to me. I am trusting you on the LUKS detection bit, the rest

Reviewed-By: Petr Rockai <prockai@redhat.com>

> Milan Broz (4):
>   Change the pvcreate swap/md logic
>   Remove assumption that --ye must be used only in --force mode
>   Fix file open leak in swap signature detection
>   Detect LUKS signature in pvcreate
>
>  lib/Makefile.in         |    1 +
>  lib/device/dev-luks.c   |   43 ++++++++++++++++++++++++++++
>  lib/device/dev-swap.c   |   12 +++----
>  lib/device/device.h     |    1 +
>  lib/metadata/metadata.c |   71 +++++++++++++++++++++++++----------------------
>  tools/pvremove.c        |    5 ---
>  tools/toollib.c         |    5 ---
>  7 files changed, 88 insertions(+), 50 deletions(-)
>  create mode 100644 lib/device/dev-luks.c