[PATCH v2 00/12] fixes for kpartx -d

[PATCH v2 00/12] fixes for kpartx -d

[Martin Wilck]

Working on a bug report about kpartx not properly removing
partitions for loop devices, I realized a number of glitches
and improperly handled corner cases in the kpartx code for
deleting partitions. Some mappings are not deleted although
they should be, and others are deleted although that is clearly
wrong.

This patch series fixes the issues I found. The series starts
with a test program demonstrating the problems. The program
succeeds only after all patches of this series are applied.

Here is my summary of what I think how kpartx should behave:

  1) kpartx should delete all mappings it created beforehand.
  2) kpartx should handle partitions on dm devices and other devices
     (e.g. loop devices) equally well.
  3) kpartx should only delete "partitions", which are single-target
     linear mappings into a block device. Other maps should not be touched.
  4) kpartx should only delete mappings it created itself beforehand.
     In particular, it shouldn't delete LVM LVs, even if they are fully
     contained in the block device at hand and thus look like partitions
     in the first place. (For historical compatibility reasons, allow
     such mappings to be deleted with the -f/--force flag).
  5) DM map names may be changed, thus kpartx shouldn't rely on them to
     check whether a mapping is a partition of a particular device. It is
     legal for a partition of /dev/loop0 to be named "loop0".

One patch has an obvious libdevmapper equivalent and is therefore
included (08/12) although this series is otherwise focused only on kpartx.
Patch 04/12 would also have a libdevmapper equivalent, but I haven't
included it because it would conflict with Ben's previously posted
patch "libmultipath: fix partition detection".

The patches are based on Christophe's tree; Christophe, if you prefer,
I can rebase them on top of Ben's late submissions.

Feedback is welcome.

Changes wrt v1:
- Test program (01/12): improved cleanup, and used "kpartx -s" rather than waiting.
- At Ben's suggestion, removed "no_partitions" support rather than fixing it.
- Previous 04/12 split into two patches (04+05/12), improving and separating
  out the part that would similarly apply to libmultipath (see above).
- New UUID format in patch 09/12 since the previous one wasn't well-received;
  the "-kpartx-" part was superfluous, as partition UUIDs start with "part%s-" anyway.
- Added the trivial fix 12/12.

Martin Wilck (12):
  kpartx: test-kpartx: new unit test program
  kpartx: remove "no_partitions" support
  kpartx: remove is_loop_device
  kpartx: relax and improve UUID check in dm_compare_uuid
  kpartx: dm_remove_partmaps: support non-dm devices
  kpartx: dm_devn: return error for non-existent device
  kpartx: don't treat multi-linear mappings as partitions
  libmultipath: don't treat multi-linear mappings as partitions
  kpartx: use partition UUID for non-DM devices
  kpartx: use absolute path for regular files
  kpartx: find_loop_by_file: use sysfs
  kpartx: include sys/sysmacros.h

 kpartx/devmapper.c       |  80 ++++++---------
 kpartx/devmapper.h       |   2 +-
 kpartx/kpartx.c          |  50 ++++++++--
 kpartx/lopart.c          |  75 ++++++--------
 kpartx/lopart.h          |   1 -
 kpartx/sysmacros.h       |   9 --
 kpartx/test-kpartx       | 254 +++++++++++++++++++++++++++++++++++++++++++++++
 libmultipath/devmapper.c |  15 +--
 8 files changed, 371 insertions(+), 115 deletions(-)
 delete mode 100644 kpartx/sysmacros.h
 create mode 100755 kpartx/test-kpartx

-- 
2.12.2

[PATCH v2 01/12] kpartx: test-kpartx: new unit test program

[Martin Wilck]

This is a unit test program for kpartx, in particular for deleting
partitions.

NOTE: This test program fails with current kpartx; full patch series
needed to make it work.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 kpartx/test-kpartx | 254 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 254 insertions(+)
 create mode 100755 kpartx/test-kpartx

diff --git a/kpartx/test-kpartx b/kpartx/test-kpartx
new file mode 100755
index 00000000..7c45cd14
--- /dev/null
+++ b/kpartx/test-kpartx
@@ -0,0 +1,254 @@
+#! /bin/bash
+
+# This is a unit test program for kpartx, in particular for deleting partitions.
+#
+# The rationale is the following:
+#
+#  1) kpartx should delete all mappings it created beforehand.
+#  2) kpartx should handle partitions on dm devices and other devices
+#     (e.g. loop devices) equally well.
+#  3) kpartx should only delete "partitions", which are single-target
+#     linear mappings into a block device. Other maps should not be touched.
+#  4) kpartx should only delete mappings it created itself beforehand.
+#     In particular, it shouldn't delete LVM LVs, even if they are fully
+#     contained in the block device at hand and thus look like partitions
+#     in the first place. (For historical compatibility reasons, we allow
+#     such mappings to be deleted with the -f/--force flag).
+#  5) DM map names may be changed, thus kpartx shouldn't rely on them to
+#     check whether a mapping is a partition of a particular device. It is
+#     legal for a partition of /dev/loop0 to be named "loop0".
+
+# Note: This program tries hard to clean up, but if tests fail,
+# stale DM or loop devices may keep lurking around.
+
+# Set WORKDIR in environment to existing dir to for persistence
+# WARNING:  exisiting files will be truncated.
+# If empty, test will be done in temporary dir
+: ${WORKDIR:=}
+# Set this environment variable to test an alternative kpartx executable
+: ${KPARTX:=}
+# Options to pass to kpartx always
+: ${KPARTX_OPTS:=-s}
+# Time to wait for device nodes to appear (microseconds)
+# Waiting is only needed if "s" is not in $KPARTX_OPTS
+: ${WAIT_US:=0}
+
+# IMPORTANT: The ERR trap is essential for this program to work correctly!
+trap 'LINE=$LINENO; trap - ERR; echo "== error in $BASH_COMMAND on line $LINE ==" >&2; exit 1' ERR
+trap 'cleanup' 0
+
+CLEANUP=:
+cleanup() {
+    trap - ERR
+    trap - 0
+    if [[ $OK ]]; then
+	echo == all tests completed successfully == >&2
+    else
+	echo == step $STEP failed == >&2
+    fi
+    eval "$CLEANUP" &>/dev/null
+}
+
+push_cleanup() {
+    CLEANUP="$@;$CLEANUP"
+}
+
+pop_cleanup() {
+    # CAUTION: simplistic
+    CLEANUP=${CLEANUP#*;}
+}
+
+step() {
+    STEP="$@"
+    echo == Test step: $STEP == >&2
+}
+
+mk_partitions() {
+    parted -s $1 mklabel msdos
+    parted -s -- $1 mkpart prim ext2 1MiB -1s
+}
+
+step preparation
+
+[[ $UID -eq 0 ]]
+[[ $KPARTX ]] || {
+    if [[ -x $PWD/kpartx/kpartx ]]; then
+	KPARTX=$PWD/kpartx/kpartx
+    else
+	KPARTX=$(which kpartx)
+    fi
+}
+[[ $KPARTX ]]
+
+FILE1=kpartx1
+FILE2=kpartx2
+FILE3=kpartx3
+SIZE=$((1024*1024*1024))  # use bytes as units here
+SECTSIZ=512
+OFFS=32                # offset of linear mapping into dev, sectors
+VG=kpvg  # volume group name
+LV=kplv  # logical vol name
+LVMCONF='devices { filter = [ "a|/dev/loop.*|", r".*" ] }'
+
+OK=
+
+[[ $WORKDIR ]] || {
+    WORKDIR=$(mktemp -d /tmp/kpartx-XXXXXX)
+    push_cleanup 'rm -rf $WORKDIR'
+}
+
+push_cleanup "cd $PWD"
+cd "$WORKDIR"
+
+step "create loop devices"
+truncate -s $SIZE $FILE1
+truncate -s $SIZE $FILE2
+truncate -s $SIZE $FILE3
+
+LO1=$(losetup -f $FILE1 --show)
+push_cleanup 'losetup -d $LO1'
+LO2=$(losetup -f $FILE2 --show)
+push_cleanup 'losetup -d $LO2'
+LO3=$(losetup -f $FILE3 --show)
+push_cleanup 'losetup -d $LO3'
+
+[[ $LO1 && $LO2 && $LO3 && -b $LO1 && -b $LO2 && -b $LO3 ]]
+DEV1=$(stat -c "%t:%T" $LO1)
+DEV2=$(stat -c "%t:%T" $LO2)
+DEV3=$(stat -c "%t:%T" $LO3)
+
+usleep $WAIT_US
+
+step "create DM devices (spans)"
+# Create two linear mappings spanning two loopdevs.
+# One of them gets a pathological name colliding with
+# the loop device name.
+# These mappings must not be removed by kpartx.
+# They also serve as DM devices to test partition removal on those.
+
+TABLE="\
+0 $((SIZE/SECTSIZ-OFFS)) linear $DEV1 $OFFS
+$((SIZE/SECTSIZ-OFFS)) $((SIZE/SECTSIZ-OFFS)) linear $DEV2 $OFFS"
+
+SPAN1=kpt
+SPAN2=$(basename $LO2)
+dmsetup create $SPAN1 <<<"$TABLE"
+push_cleanup 'dmsetup remove -f $SPAN1'
+
+dmsetup create $SPAN2 <<<"$TABLE"
+push_cleanup 'dmsetup remove -f $SPAN2'
+
+usleep $WAIT_US
+[[ -b /dev/mapper/$SPAN1 ]]
+[[ -b /dev/mapper/$SPAN2 ]]
+
+step "create vg on $LO3"
+# On the 3rd loop device, we create a VG and an LV
+# The LV should not be removed by kpartx.
+pvcreate --config "$LVMCONF" -f $LO3
+vgcreate --config "$LVMCONF" $VG $LO3
+push_cleanup 'vgremove --config "$LVMCONF" -f $VG'
+lvcreate --config "$LVMCONF" -L $((SIZE/2))B -n $LV $VG
+push_cleanup 'lvremove --config "$LVMCONF" -f $VG/$LV'
+usleep $WAIT_US
+
+[[ -b /dev/mapper/$VG-$LV ]]
+
+# dmsetup table /dev/mapper/$VG-$LV
+# dmsetup info /dev/mapper/$VG-$LV
+
+step "create partitions on loop devices"
+
+mk_partitions $LO1
+mk_partitions $LO2
+
+# Test invocation of kpartx with regular file here
+LO2P1=/dev/mapper/$(basename $LO2)-foo1
+$KPARTX $KPARTX_OPTS -a -p -foo $FILE2
+push_cleanup 'dmsetup remove -f $(basename $LO2P1)'
+
+LO1P1=/dev/mapper/$(basename $LO1)-eggs1
+$KPARTX $KPARTX_OPTS -a -p -eggs $LO1
+push_cleanup 'dmsetup remove -f $(basename $LO1P1)'
+
+usleep $WAIT_US
+[[ -b $LO1P1 ]]
+[[ -b $LO2P1 ]]
+
+# dmsetup info $LO2P1
+
+# Set pathological name for partition on $LO1 (same as loop device itself)
+dmsetup rename $(basename $LO1P1) $(basename $LO1)
+LO1P1=/dev/mapper/$(basename $LO1)
+pop_cleanup
+push_cleanup 'dmsetup remove -f $(basename $LO1P1)'
+
+# dmsetup info $LO1P1
+
+step "create partitions on DM devices"
+mk_partitions /dev/mapper/$SPAN2
+
+$KPARTX $KPARTX_OPTS -a -p -bar /dev/mapper/$SPAN2
+SPAN2P1=/dev/mapper/${SPAN2}-bar1
+
+# udev rules may have created partition mappings without UUIDs
+# which aren't removed by default (if system standard kpartx doesn't
+# set the UUID). Remove them using -f
+push_cleanup '$KPARTX $KPARTX_OPTS -f -d /dev/mapper/$SPAN2'
+push_cleanup 'dmsetup remove -f $(basename $SPAN2P1)'
+
+$KPARTX $KPARTX_OPTS -a -p -spam /dev/mapper/$SPAN1
+SPAN1P1=/dev/mapper/${SPAN1}-spam1
+# see above
+push_cleanup '$KPARTX $KPARTX_OPTS -f -d /dev/mapper/$SPAN1'
+push_cleanup 'dmsetup remove -f $(basename $SPAN1P1)'
+
+usleep $WAIT_US
+[[ -b $SPAN2P1 ]]
+[[ -b $SPAN1P1 ]]
+
+step "delete partitions on DM devices"
+$KPARTX $KPARTX_OPTS -d /dev/mapper/$SPAN1 >&2
+usleep $WAIT_US
+
+[[ -b $SPAN2P1 ]]
+[[ -b $LO1P1 ]]
+[[ -b $LO2P1 ]]
+[[ ! -b $SPAN1P1 ]]
+
+$KPARTX $KPARTX_OPTS -d /dev/mapper/$SPAN2
+usleep $WAIT_US
+
+[[ -b $LO1P1 ]]
+[[ -b $LO2P1 ]]
+[[ ! -b $SPAN2P1 ]]
+
+step "delete partitions on loop devices"
+
+$KPARTX $KPARTX_OPTS -d $LO3
+
+# This will also delete the loop device
+$KPARTX $KPARTX_OPTS -d $FILE2
+$KPARTX $KPARTX_OPTS -d $LO1
+usleep $WAIT_US
+
+# ls -l /dev/mapper
+[[ ! -b $LO1P1 ]]
+pop_cleanup
+[[ ! -b $LO2P1 ]]
+pop_cleanup
+# spans should not have been removed
+[[ -b /dev/mapper/$SPAN1 ]]
+[[ -b /dev/mapper/$SPAN2 ]]
+# LVs neither
+[[ -b /dev/mapper/$VG-$LV ]]
+
+step "delete partitions on $LO3 with -f"
+
+$KPARTX $KPARTX_OPTS -f -d $LO3
+# -d -f should delete the LV, too
+[[ ! -b /dev/mapper/$VG-$LV ]]
+[[ -b /dev/mapper/$SPAN1 ]]
+[[ -b /dev/mapper/$SPAN2 ]]
+
+OK=yes
-- 
2.12.2

[PATCH v2 02/12] kpartx: remove "no_partitions" support

[Martin Wilck]

The kernel does not support the "no_partitions" feature - remove it.
Distributions who want to keep support for this feature should
re-enable it with a distro-specific patch.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 kpartx/devmapper.c | 29 -----------------------------
 kpartx/kpartx.c    |  5 -----
 2 files changed, 34 deletions(-)

diff --git a/kpartx/devmapper.c b/kpartx/devmapper.c
index cf6650c6..8aca9592 100644
--- a/kpartx/devmapper.c
+++ b/kpartx/devmapper.c
@@ -542,32 +542,3 @@ dm_remove_partmaps (char * mapname, char *uuid, int verbose)
 	struct remove_data rd = { verbose };
 	return do_foreach_partmaps(mapname, uuid, remove_partmap, &rd);
 }
-
-#define FEATURE_NO_PART "no_partitions"
-
-int
-dm_no_partitions(char *mapname)
-{
-	char params[PARAMS_SIZE], *ptr;
-	int i, num_features;
-
-	if (dm_get_map(mapname, params))
-		return 0;
-
-	ptr = params;
-	num_features = strtoul(params, &ptr, 10);
-	if ((ptr == params) || num_features == 0) {
-		/* No features found, return success */
-		return 0;
-	}
-	for (i = 0; (i < num_features); i++) {
-		if (!ptr || ptr > params + strlen(params))
-			break;
-		/* Skip whitespaces */
-		while(ptr && *ptr == ' ') ptr++;
-		if (!strncmp(ptr, FEATURE_NO_PART, strlen(FEATURE_NO_PART)))
-			return 1;
-		ptr = strchr(ptr, ' ');
-	}
-	return 0;
-}
diff --git a/kpartx/kpartx.c b/kpartx/kpartx.c
index 58e60ffe..e0c105f4 100644
--- a/kpartx/kpartx.c
+++ b/kpartx/kpartx.c
@@ -362,11 +362,6 @@ main(int argc, char **argv){
 
 	if (!mapname)
 		mapname = device + off;
-	if (!force_devmap &&
-		 dm_no_partitions(mapname)) {
-		/* Feature 'no_partitions' is set, return */
-		return 0;
-	}
 
 	if (delim == NULL) {
 		delim = malloc(DELIM_SIZE);
-- 
2.12.2

[PATCH v2 03/12] kpartx: remove is_loop_device

[Martin Wilck]

This function is not used any more.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 kpartx/lopart.c | 31 -------------------------------
 kpartx/lopart.h |  1 -
 2 files changed, 32 deletions(-)

diff --git a/kpartx/lopart.c b/kpartx/lopart.c
index 2eb3f631..44f0c277 100644
--- a/kpartx/lopart.c
+++ b/kpartx/lopart.c
@@ -62,37 +62,6 @@ xstrdup (const char *s)
 	return t;
 }
 
-int is_loop_device(const char *device)
-{
-	struct stat statbuf;
-	int loopmajor;
-#if 1
-	loopmajor = 7;
-#else
-	FILE *procdev;
-	char line[100], *cp;
-
-	loopmajor = 0;
-
-	if ((procdev = fopen(PROC_DEVICES, "r")) != NULL) {
-
-		while (fgets (line, sizeof(line), procdev)) {
-
-			if ((cp = strstr (line, " loop\n")) != NULL) {
-				*cp='\0';
-				loopmajor=atoi(line);
-				break;
-			}
-		}
-
-		fclose(procdev);
-	}
-#endif
-	return (loopmajor && stat(device, &statbuf) == 0 &&
-		S_ISBLK(statbuf.st_mode) &&
-		major(statbuf.st_rdev) == loopmajor);
-}
-
 #define SIZE(a) (sizeof(a)/sizeof(a[0]))
 
 char *find_loop_by_file(const char *filename)
diff --git a/kpartx/lopart.h b/kpartx/lopart.h
index a512353b..d3bad10a 100644
--- a/kpartx/lopart.h
+++ b/kpartx/lopart.h
@@ -1,6 +1,5 @@
 extern int verbose;
 extern int set_loop (const char *, const char *, int, int *);
 extern int del_loop (const char *);
-extern int is_loop_device (const char *);
 extern char * find_unused_loop_device (void);
 extern char * find_loop_by_file (const char *);
-- 
2.12.2

[PATCH v2 04/12] kpartx: relax and improve UUID check in dm_compare_uuid

[Martin Wilck]

It is wrong to assume that UUIDs of parent devices always contain
"mpath". Fix that, and make the check for the kpartx-specific prefix
"part%d-" stricter and more explicit.

Moreover, avoid duplication of string constants and properly express
the dependencies of the various constants.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 kpartx/devmapper.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/kpartx/devmapper.c b/kpartx/devmapper.c
index 8aca9592..8b125be5 100644
--- a/kpartx/devmapper.c
+++ b/kpartx/devmapper.c
@@ -10,8 +10,10 @@
 #include <errno.h>
 #include "devmapper.h"
 
-#define UUID_PREFIX "part%d-"
-#define MAX_PREFIX_LEN 8
+#define _UUID_PREFIX "part"
+#define UUID_PREFIX _UUID_PREFIX "%d-"
+#define _UUID_PREFIX_LEN (sizeof(_UUID_PREFIX) - 1)
+#define MAX_PREFIX_LEN (_UUID_PREFIX_LEN + 4)
 #define PARAMS_SIZE 1024
 
 int dm_prereq(char * str, int x, int y, int z)
@@ -417,9 +419,13 @@ dm_compare_uuid(const char *mapuuid, const char *partname)
 	if (!partuuid)
 		return 1;
 
-	if (!strncmp(partuuid, "part", 4)) {
-		char *p = strstr(partuuid, "mpath-");
-		if (p && !strcmp(mapuuid, p))
+	if (!strncmp(partuuid, _UUID_PREFIX, _UUID_PREFIX_LEN)) {
+		char *p = partuuid + _UUID_PREFIX_LEN;
+		/* skip partition number */
+		while (isdigit(*p))
+			p++;
+		if (p != partuuid + _UUID_PREFIX_LEN && *p == '-' &&
+		    !strcmp(mapuuid, p + 1))
 			r = 0;
 	}
 	free(partuuid);
-- 
2.12.2

[PATCH v2 05/12] kpartx: dm_remove_partmaps: support non-dm devices

[Martin Wilck]

Commit  3d709241 improved partition removal, but broke support
for handling partitions of non-dm devices such as loop devices
or RAM disks.

This requires passing the dev_t of the device down to
do_foreach_partmap(). Doing so, there's little use in trying
to derive major/minor numbers from the "mapname" any more
(which actually is the device name for non-DM devices).
But we can use this to find out whether the device in question
is a dm device.

The test for UUID match doesn't work for non-DM devices (this
shall be improved in a later patch in this series).

The test for equal name of parent dev and partition is only valid
for dm devices. For non-dm devices such as loop, "/dev/mapper/loop0"
could, theoretically, be a partition of "/dev/loop0"
(and we don't want to rely on map names).

Fixes: 3d709241 "kpartx: sanitize delete partitions"
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 kpartx/devmapper.c | 22 +++++++++++++++-------
 kpartx/devmapper.h |  2 +-
 kpartx/kpartx.c    |  3 ++-
 3 files changed, 18 insertions(+), 9 deletions(-)

diff --git a/kpartx/devmapper.c b/kpartx/devmapper.c
index 8b125be5..b7a7390c 100644
--- a/kpartx/devmapper.c
+++ b/kpartx/devmapper.c
@@ -8,6 +8,7 @@
 #include <libdevmapper.h>
 #include <ctype.h>
 #include <errno.h>
+#include <sys/sysmacros.h>
 #include "devmapper.h"
 
 #define _UUID_PREFIX "part"
@@ -438,6 +439,7 @@ struct remove_data {
 
 static int
 do_foreach_partmaps (const char * mapname, const char *uuid,
+		     dev_t devt,
 		     int (*partmap_func)(const char *, void *),
 		     void *data)
 {
@@ -449,6 +451,7 @@ do_foreach_partmaps (const char * mapname, const char *uuid,
 	int major, minor;
 	char dev_t[32];
 	int r = 1;
+	int is_dmdev = 1;
 
 	if (!(dmt = dm_task_create(DM_DEVICE_LIST)))
 		return 1;
@@ -466,15 +469,20 @@ do_foreach_partmaps (const char * mapname, const char *uuid,
 		goto out;
 	}
 
-	if (dm_devn(mapname, &major, &minor))
-		goto out;
+	if (dm_devn(mapname, &major, &minor) ||
+	    (major != major(devt) || minor != minor(devt)))
+		/*
+		 * The latter could happen if a dm device "/dev/mapper/loop0"
+		 * exits while kpartx is called on "/dev/loop0".
+		 */
+		is_dmdev = 0;
 
-	sprintf(dev_t, "%d:%d", major, minor);
+	sprintf(dev_t, "%d:%d", major(devt), minor(devt));
 	do {
 		/*
 		 * skip our devmap
 		 */
-		if (!strcmp(names->name, mapname))
+		if (is_dmdev && !strcmp(names->name, mapname))
 			goto next;
 
 		/*
@@ -502,7 +510,7 @@ do_foreach_partmaps (const char * mapname, const char *uuid,
 		/*
 		 * skip if uuids don't match
 		 */
-		if (dm_compare_uuid(uuid, names->name)) {
+		if (is_dmdev && uuid && dm_compare_uuid(uuid, names->name)) {
 			if (rd->verbose)
 				printf("%s: is not a kpartx partition. Not removing\n",
 				       names->name);
@@ -543,8 +551,8 @@ remove_partmap(const char *name, void *data)
 }
 
 int
-dm_remove_partmaps (char * mapname, char *uuid, int verbose)
+dm_remove_partmaps (char * mapname, char *uuid, dev_t devt, int verbose)
 {
 	struct remove_data rd = { verbose };
-	return do_foreach_partmaps(mapname, uuid, remove_partmap, &rd);
+	return do_foreach_partmaps(mapname, uuid, devt, remove_partmap, &rd);
 }
diff --git a/kpartx/devmapper.h b/kpartx/devmapper.h
index 9988ec0f..2e28c780 100644
--- a/kpartx/devmapper.h
+++ b/kpartx/devmapper.h
@@ -18,7 +18,7 @@ char * dm_mapname(int major, int minor);
 dev_t dm_get_first_dep(char *devname);
 char * dm_mapuuid(const char *mapname);
 int dm_devn (const char * mapname, int *major, int *minor);
-int dm_remove_partmaps (char * mapname, char *uuid, int verbose);
+int dm_remove_partmaps (char * mapname, char *uuid, dev_t devt, int verbose);
 int dm_no_partitions(char * mapname);
 
 #endif /* _KPARTX_DEVMAPPER_H */
diff --git a/kpartx/kpartx.c b/kpartx/kpartx.c
index e0c105f4..ea728b42 100644
--- a/kpartx/kpartx.c
+++ b/kpartx/kpartx.c
@@ -446,7 +446,8 @@ main(int argc, char **argv){
 			break;
 
 		case DELETE:
-			r = dm_remove_partmaps(mapname, uuid, verbose);
+			r = dm_remove_partmaps(mapname, uuid, buf.st_rdev,
+					       verbose);
 			if (loopdev) {
 				if (del_loop(loopdev)) {
 					if (verbose)
-- 
2.12.2

[PATCH v2 06/12] kpartx: dm_devn: return error for non-existent device

[Martin Wilck]

For non-existent maps (ENXIO from ioctl()), dm_task_run and
dm_task_get_info return success. We need to check info.exists.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 kpartx/devmapper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kpartx/devmapper.c b/kpartx/devmapper.c
index b7a7390c..6cee120d 100644
--- a/kpartx/devmapper.c
+++ b/kpartx/devmapper.c
@@ -295,7 +295,7 @@ dm_devn (const char * mapname, int *major, int *minor)
 	if (!dm_task_run(dmt))
 		goto out;
 
-	if (!dm_task_get_info(dmt, &info))
+	if (!dm_task_get_info(dmt, &info) || info.exists == 0)
 		goto out;
 
 	*major = info.major;
-- 
2.12.2

[PATCH v2 07/12] kpartx: don't treat multi-linear mappings as partitions

[Martin Wilck]

kpartx -d treats any map that has a "linear" mapping into a
device as first target as a partition of this device.
This is wrong, because linear mappings may consist of
several pieces, combining multiple devices into one
(LVM logical volumes are an example of such a
mapping). Partitions have to be single-target mappings.

Fix this by returning an error in dm_type() if a map with
multiple targets is encountered. The "type" of a map with
two or more targets is a difficult concept, anyway.

test case:
truncate -s 1G test0 test1
losetup /dev/loop0 test0
losetup /dev/loop1 test1
dmsetup create <<EOF
0 1000000 linear 7:0 4096
1000000 1000000 linear 7:1 4096
EOF
kpartx -d /dev/loop0 -v

The map "join" should NOT be deleted.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 kpartx/devmapper.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/kpartx/devmapper.c b/kpartx/devmapper.c
index 6cee120d..b714ba4e 100644
--- a/kpartx/devmapper.c
+++ b/kpartx/devmapper.c
@@ -392,10 +392,11 @@ dm_type(const char * name, char * type)
 		goto out;
 
 	/* Fetch 1st target */
-	dm_get_next_target(dmt, NULL, &start, &length,
-			   &target_type, &params);
-
-	if (!target_type)
+	if (dm_get_next_target(dmt, NULL, &start, &length,
+			       &target_type, &params) != NULL)
+		/* more than one target */
+		r = -1;
+	else if (!target_type)
 		r = -1;
 	else if (!strcmp(target_type, type))
 		r = 1;
@@ -500,7 +501,7 @@ do_foreach_partmaps (const char * mapname, const char *uuid,
 		/*
 		 * skip if devmap target is not "linear"
 		 */
-		if (!dm_type(names->name, "linear")) {
+		if (dm_type(names->name, "linear") != 1) {
 			if (rd->verbose)
 				printf("%s: is not a linear target. Not removing\n",
 				       names->name);
-- 
2.12.2

[PATCH v2 08/12] libmultipath: don't treat multi-linear mappings as partitions

[Martin Wilck]

dm_type is used in libmultipath only to check whether a mapping
is "linear", with the intention to test if it represents a
"partition". This test returns TRUE also for mappings with
multiple targets, the first of which happens to be a linear
mapping into the target device. This is questionable, it's
hard to assign a "type" to such maps anyway.

Fix this by returning an error for multi-target maps.
This is analogous to the patch
"kpartx: don't treat multi-linear mappings as partitions".

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/devmapper.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
index 5fb9d9ac..c19dcb62 100644
--- a/libmultipath/devmapper.c
+++ b/libmultipath/devmapper.c
@@ -572,7 +572,7 @@ out:
  * returns:
  *    1 : match
  *    0 : no match
- *   -1 : empty map
+ *   -1 : empty map, or more than 1 target
  */
 int dm_type(const char *name, char *type)
 {
@@ -594,10 +594,11 @@ int dm_type(const char *name, char *type)
 		goto out;
 
 	/* Fetch 1st target */
-	dm_get_next_target(dmt, NULL, &start, &length,
-			   &target_type, &params);
-
-	if (!target_type)
+	if (dm_get_next_target(dmt, NULL, &start, &length,
+			       &target_type, &params) != NULL)
+		/* multiple targets */
+		r = -1;
+	else if (!target_type)
 		r = -1;
 	else if (!strcmp(target_type, type))
 		r = 1;
@@ -1185,9 +1186,9 @@ do_foreach_partmaps (const char * mapname,
 	do {
 		if (
 		    /*
-		     * if devmap target is "linear"
+		     * if there is only a single "linear" target
 		     */
-		    (dm_type(names->name, TGT_PART) > 0) &&
+		    (dm_type(names->name, TGT_PART) == 1) &&
 
 		    /*
 		     * and both uuid end with same suffix starting
-- 
2.12.2

[PATCH v2 09/12] kpartx: use partition UUID for non-DM devices

[Martin Wilck]

For dm devices, kpartx uses an UUID check at partition removal
to make sure it only deletes partitions it previously created.
Non-DM parent devices such as loop devices don't generally have
a UUID.

Introduce a "fake" UUID for these devices to make sure kpartx
deletes only devices it had created previously. Otherwise kpartx
might e.g. delete LVM LVs that are inside a device it is trying
to delete partitions for. It seems to be wiser to make sure the
user delete these manually before running "kpartx -d".

With the fake UUID in place, we can re-introduce the UUID check
for non-DM device that was removed in the earlier patch
"kpartx: dm_remove_partmaps: support non-dm devices".

This disables also deletion of partition mappings created
by earlier versions of kpartx. If kpartx has been updated after
partition mappings were created, the "-f" flag can be used
to force delting these partitions, too.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 kpartx/devmapper.c |  2 +-
 kpartx/kpartx.c    | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 35 insertions(+), 1 deletion(-)

diff --git a/kpartx/devmapper.c b/kpartx/devmapper.c
index b714ba4e..4ab58ce9 100644
--- a/kpartx/devmapper.c
+++ b/kpartx/devmapper.c
@@ -511,7 +511,7 @@ do_foreach_partmaps (const char * mapname, const char *uuid,
 		/*
 		 * skip if uuids don't match
 		 */
-		if (is_dmdev && uuid && dm_compare_uuid(uuid, names->name)) {
+		if (uuid && dm_compare_uuid(uuid, names->name)) {
 			if (rd->verbose)
 				printf("%s: is not a kpartx partition. Not removing\n",
 				       names->name);
diff --git a/kpartx/kpartx.c b/kpartx/kpartx.c
index ea728b42..587c3dfe 100644
--- a/kpartx/kpartx.c
+++ b/kpartx/kpartx.c
@@ -60,6 +60,31 @@ struct pt {
 int ptct = 0;
 int udev_sync = 0;
 
+/*
+ * UUID format for partitions created on non-DM devices
+ * ${UUID_PREFIX}devnode_${MAJOR}:${MINOR}_${NONDM_UUID_SUFFIX}"
+ * where ${UUID_PREFIX} is "part${PARTNO}-" (see devmapper.c).
+ *
+ * The suffix should be sufficiently unique to avoid incidental conflicts;
+ * the value below is a base64-encoded random number.
+ * The UUID format shouldn't be changed between kpartx releases.
+ */
+#define NONDM_UUID_PREFIX "devnode"
+#define NONDM_UUID_SUFFIX "Wh5pYvM"
+
+static char *
+nondm_create_uuid(dev_t devt)
+{
+#define NONDM_UUID_BUFLEN (34 + sizeof(NONDM_UUID_PREFIX) + \
+			   sizeof(NONDM_UUID_SUFFIX))
+	static char uuid_buf[NONDM_UUID_BUFLEN];
+	snprintf(uuid_buf, sizeof(uuid_buf), "%s_%u:%u_%s",
+		 NONDM_UUID_PREFIX, major(devt), minor(devt),
+		 NONDM_UUID_SUFFIX);
+	uuid_buf[NONDM_UUID_BUFLEN-1] = '\0';
+	return uuid_buf;
+}
+
 static void
 addpts(char *t, ptreader f)
 {
@@ -360,6 +385,15 @@ main(int argc, char **argv){
 			uuid = dm_mapuuid(mapname);
 	}
 
+	/*
+	 * We are called for a non-DM device.
+	 * Make up a fake UUID for the device, unless "-d -f" is given.
+	 * This allows deletion of partitions created with older kpartx
+	 * versions which didn't use the fake UUID during creation.
+	 */
+	if (!uuid && !(what == DELETE && force_devmap))
+		uuid = nondm_create_uuid(buf.st_rdev);
+
 	if (!mapname)
 		mapname = device + off;
 
-- 
2.12.2

[PATCH v2 10/12] kpartx: use absolute path for regular files

[Martin Wilck]

kpartx supports being called for a regular file, in which
case it assumes that it should set up a loop device or use
an existing one. Because the loopinfo.lo_name field contains
an absolute path, matching with existing loop devices fails
for relative paths. Matching by basename only would be unreliable.

Therefore, convert relative to absolute path for regular files.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 kpartx/kpartx.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/kpartx/kpartx.c b/kpartx/kpartx.c
index 587c3dfe..c76b5b46 100644
--- a/kpartx/kpartx.c
+++ b/kpartx/kpartx.c
@@ -355,7 +355,13 @@ main(int argc, char **argv){
 
 	if (S_ISREG (buf.st_mode)) {
 		/* already looped file ? */
-		loopdev = find_loop_by_file(device);
+		char rpath[PATH_MAX];
+		if (realpath(device, rpath) == NULL) {
+			fprintf(stderr, "Error: %s: %s\n", device,
+				strerror(errno));
+			exit (1);
+		}
+		loopdev = find_loop_by_file(rpath);
 
 		if (!loopdev && what == DELETE)
 			exit (0);
-- 
2.12.2

[PATCH v2 11/12] kpartx: find_loop_by_file: use sysfs

[Martin Wilck]

Rather then searching through all of /dev, look up loop devices
in /sys/devices/virtual/block. This is cleaner and more robust
(/dev/loop$Xp$Y symlinks may confuse kpartx).

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 kpartx/lopart.c | 42 +++++++++++++++++++++++++++++++-----------
 1 file changed, 31 insertions(+), 11 deletions(-)

diff --git a/kpartx/lopart.c b/kpartx/lopart.c
index 44f0c277..0521d7dc 100644
--- a/kpartx/lopart.c
+++ b/kpartx/lopart.c
@@ -68,25 +68,46 @@ char *find_loop_by_file(const char *filename)
 {
 	DIR *dir;
 	struct dirent *dent;
-	char dev[64], *found = NULL;
+	char dev[64], *found = NULL, *p;
 	int fd;
 	struct stat statbuf;
 	struct loop_info loopinfo;
+	const char VIRT_BLOCK[] = "/sys/devices/virtual/block";
+	char path[PATH_MAX];
 
-	dir = opendir("/dev");
+	dir = opendir(VIRT_BLOCK);
 	if (!dir)
 		return NULL;
 
 	while ((dent = readdir(dir)) != NULL) {
 		if (strncmp(dent->d_name,"loop",4))
 			continue;
-		if (!strcmp(dent->d_name, "loop-control"))
-			continue;
-		sprintf(dev, "/dev/%s", dent->d_name);
 
-		fd = open (dev, O_RDONLY);
+		if (snprintf(path, PATH_MAX, "%s/%s/dev", VIRT_BLOCK,
+			     dent->d_name) >= PATH_MAX)
+			continue;
+
+		fd = open(path, O_RDONLY);
 		if (fd < 0)
-			break;
+			continue;
+
+		if (read(fd, dev, sizeof(dev)) <= 0) {
+			close(fd);
+			continue;
+		}
+
+		close(fd);
+
+		dev[sizeof(dev)-1] = '\0';
+		p = strchr(dev, '\n');
+		if (p != NULL)
+			*p = '\0';
+		if (snprintf(path, PATH_MAX, "/dev/block/%s", dev) >= PATH_MAX)
+			continue;
+
+		fd = open (path, O_RDONLY);
+		if (fd < 0)
+			continue;
 
 		if (fstat (fd, &statbuf) != 0 ||
 		    !S_ISBLK(statbuf.st_mode)) {
@@ -99,13 +120,12 @@ char *find_loop_by_file(const char *filename)
 			continue;
 		}
 
+		close (fd);
+
 		if (0 == strcmp(filename, loopinfo.lo_name)) {
-			close (fd);
-			found = xstrdup(dev);
+			found = realpath(path, NULL);
 			break;
 		}
-
-		close (fd);
 	}
 	closedir(dir);
 	return found;
-- 
2.12.2

[PATCH v2 12/12] kpartx: include sys/sysmacros.h

[Martin Wilck]

Finish the work started in ea436715. Fixes a compilation warning.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 kpartx/lopart.c    | 2 +-
 kpartx/sysmacros.h | 9 ---------
 2 files changed, 1 insertion(+), 10 deletions(-)
 delete mode 100644 kpartx/sysmacros.h

diff --git a/kpartx/lopart.c b/kpartx/lopart.c
index 0521d7dc..70054459 100644
--- a/kpartx/lopart.c
+++ b/kpartx/lopart.c
@@ -27,7 +27,7 @@
 #include <sys/mman.h>
 #include <sys/types.h>
 #include <dirent.h>
-#include "sysmacros.h"
+#include <sys/sysmacros.h>
 #include <linux/loop.h>
 
 #include "lopart.h"
diff --git a/kpartx/sysmacros.h b/kpartx/sysmacros.h
deleted file mode 100644
index 171b33d4..00000000
--- a/kpartx/sysmacros.h
+++ /dev/null
@@ -1,9 +0,0 @@
-/* versions to be used with > 16-bit dev_t - leave unused for now */
-
-#ifndef major
-#define major(dev)	((dev) >> 8)
-#endif
-
-#ifndef minor
-#define minor(dev)	((dev) & 0xff)
-#endif
-- 
2.12.2

Re: [PATCH v2 00/12] fixes for kpartx -d

[Benjamin Marzinski]

On Mon, May 15, 2017 at 05:37:10PM +0200, Martin Wilck wrote:

ACK for the set

-Ben

> Working on a bug report about kpartx not properly removing
> partitions for loop devices, I realized a number of glitches
> and improperly handled corner cases in the kpartx code for
> deleting partitions. Some mappings are not deleted although
> they should be, and others are deleted although that is clearly
> wrong.
> 
> This patch series fixes the issues I found. The series starts
> with a test program demonstrating the problems. The program
> succeeds only after all patches of this series are applied.
> 
> Here is my summary of what I think how kpartx should behave:
> 
>   1) kpartx should delete all mappings it created beforehand.
>   2) kpartx should handle partitions on dm devices and other devices
>      (e.g. loop devices) equally well.
>   3) kpartx should only delete "partitions", which are single-target
>      linear mappings into a block device. Other maps should not be touched.
>   4) kpartx should only delete mappings it created itself beforehand.
>      In particular, it shouldn't delete LVM LVs, even if they are fully
>      contained in the block device at hand and thus look like partitions
>      in the first place. (For historical compatibility reasons, allow
>      such mappings to be deleted with the -f/--force flag).
>   5) DM map names may be changed, thus kpartx shouldn't rely on them to
>      check whether a mapping is a partition of a particular device. It is
>      legal for a partition of /dev/loop0 to be named "loop0".
> 
> One patch has an obvious libdevmapper equivalent and is therefore
> included (08/12) although this series is otherwise focused only on kpartx.
> Patch 04/12 would also have a libdevmapper equivalent, but I haven't
> included it because it would conflict with Ben's previously posted
> patch "libmultipath: fix partition detection".
> 
> The patches are based on Christophe's tree; Christophe, if you prefer,
> I can rebase them on top of Ben's late submissions.
> 
> Feedback is welcome.
> 
> Changes wrt v1:
> - Test program (01/12): improved cleanup, and used "kpartx -s" rather than waiting.
> - At Ben's suggestion, removed "no_partitions" support rather than fixing it.
> - Previous 04/12 split into two patches (04+05/12), improving and separating
>   out the part that would similarly apply to libmultipath (see above).
> - New UUID format in patch 09/12 since the previous one wasn't well-received;
>   the "-kpartx-" part was superfluous, as partition UUIDs start with "part%s-" anyway.
> - Added the trivial fix 12/12.
> 
> Martin Wilck (12):
>   kpartx: test-kpartx: new unit test program
>   kpartx: remove "no_partitions" support
>   kpartx: remove is_loop_device
>   kpartx: relax and improve UUID check in dm_compare_uuid
>   kpartx: dm_remove_partmaps: support non-dm devices
>   kpartx: dm_devn: return error for non-existent device
>   kpartx: don't treat multi-linear mappings as partitions
>   libmultipath: don't treat multi-linear mappings as partitions
>   kpartx: use partition UUID for non-DM devices
>   kpartx: use absolute path for regular files
>   kpartx: find_loop_by_file: use sysfs
>   kpartx: include sys/sysmacros.h
> 
>  kpartx/devmapper.c       |  80 ++++++---------
>  kpartx/devmapper.h       |   2 +-
>  kpartx/kpartx.c          |  50 ++++++++--
>  kpartx/lopart.c          |  75 ++++++--------
>  kpartx/lopart.h          |   1 -
>  kpartx/sysmacros.h       |   9 --
>  kpartx/test-kpartx       | 254 +++++++++++++++++++++++++++++++++++++++++++++++
>  libmultipath/devmapper.c |  15 +--
>  8 files changed, 371 insertions(+), 115 deletions(-)
>  delete mode 100644 kpartx/sysmacros.h
>  create mode 100755 kpartx/test-kpartx
> 
> -- 
> 2.12.2

Re: [PATCH v2 00/12] fixes for kpartx -d

[Christophe Varoqui]

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

The set is merged, thanks.

On Wed, May 17, 2017 at 7:06 PM, Benjamin Marzinski <bmarzins@redhat.com>
wrote:

> On Mon, May 15, 2017 at 05:37:10PM +0200, Martin Wilck wrote:
>
> ACK for the set
>
> -Ben
>
> > Working on a bug report about kpartx not properly removing
> > partitions for loop devices, I realized a number of glitches
> > and improperly handled corner cases in the kpartx code for
> > deleting partitions. Some mappings are not deleted although
> > they should be, and others are deleted although that is clearly
> > wrong.
> >
> > This patch series fixes the issues I found. The series starts
> > with a test program demonstrating the problems. The program
> > succeeds only after all patches of this series are applied.
> >
> > Here is my summary of what I think how kpartx should behave:
> >
> >   1) kpartx should delete all mappings it created beforehand.
> >   2) kpartx should handle partitions on dm devices and other devices
> >      (e.g. loop devices) equally well.
> >   3) kpartx should only delete "partitions", which are single-target
> >      linear mappings into a block device. Other maps should not be
> touched.
> >   4) kpartx should only delete mappings it created itself beforehand.
> >      In particular, it shouldn't delete LVM LVs, even if they are fully
> >      contained in the block device at hand and thus look like partitions
> >      in the first place. (For historical compatibility reasons, allow
> >      such mappings to be deleted with the -f/--force flag).
> >   5) DM map names may be changed, thus kpartx shouldn't rely on them to
> >      check whether a mapping is a partition of a particular device. It is
> >      legal for a partition of /dev/loop0 to be named "loop0".
> >
> > One patch has an obvious libdevmapper equivalent and is therefore
> > included (08/12) although this series is otherwise focused only on
> kpartx.
> > Patch 04/12 would also have a libdevmapper equivalent, but I haven't
> > included it because it would conflict with Ben's previously posted
> > patch "libmultipath: fix partition detection".
> >
> > The patches are based on Christophe's tree; Christophe, if you prefer,
> > I can rebase them on top of Ben's late submissions.
> >
> > Feedback is welcome.
> >
> > Changes wrt v1:
> > - Test program (01/12): improved cleanup, and used "kpartx -s" rather
> than waiting.
> > - At Ben's suggestion, removed "no_partitions" support rather than
> fixing it.
> > - Previous 04/12 split into two patches (04+05/12), improving and
> separating
> >   out the part that would similarly apply to libmultipath (see above).
> > - New UUID format in patch 09/12 since the previous one wasn't
> well-received;
> >   the "-kpartx-" part was superfluous, as partition UUIDs start with
> "part%s-" anyway.
> > - Added the trivial fix 12/12.
> >
> > Martin Wilck (12):
> >   kpartx: test-kpartx: new unit test program
> >   kpartx: remove "no_partitions" support
> >   kpartx: remove is_loop_device
> >   kpartx: relax and improve UUID check in dm_compare_uuid
> >   kpartx: dm_remove_partmaps: support non-dm devices
> >   kpartx: dm_devn: return error for non-existent device
> >   kpartx: don't treat multi-linear mappings as partitions
> >   libmultipath: don't treat multi-linear mappings as partitions
> >   kpartx: use partition UUID for non-DM devices
> >   kpartx: use absolute path for regular files
> >   kpartx: find_loop_by_file: use sysfs
> >   kpartx: include sys/sysmacros.h
> >
> >  kpartx/devmapper.c       |  80 ++++++---------
> >  kpartx/devmapper.h       |   2 +-
> >  kpartx/kpartx.c          |  50 ++++++++--
> >  kpartx/lopart.c          |  75 ++++++--------
> >  kpartx/lopart.h          |   1 -
> >  kpartx/sysmacros.h       |   9 --
> >  kpartx/test-kpartx       | 254 ++++++++++++++++++++++++++++++
> +++++++++++++++++
> >  libmultipath/devmapper.c |  15 +--
> >  8 files changed, 371 insertions(+), 115 deletions(-)
> >  delete mode 100644 kpartx/sysmacros.h
> >  create mode 100755 kpartx/test-kpartx
> >
> > --
> > 2.12.2
>

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

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]