[RFC PATCH] multipath-tools: Preventing silent swapping of underlying LUNs

[RFC PATCH] multipath-tools: Preventing silent swapping of underlying LUNs

[Jun'ichi Nomura]

Hi,

I found multipathd could unexpectedly swaps underlying LUNs
when it should swap names.

Suppose someone has following /etc/multipath/bindings:
  mpathA  <wwid of LUN0>
  mpathB  <wwid of LUN1>
and created those multipath devices.

If he modified bindings as below and do 'multipathd -kreconfigure':
  mpathB  <wwid of LUN0>
  mpathA  <wwid of LUN1>
I think it is natural to expect the mpath device for <wwid of LUN0>
("mpathA") is renamed to "mpathB" and vice versa.

However, what actually happens is mpathA's underlying device is
changed to LUN1 and mpathB's underlying device is changed to LUN0.

As a result, users of those devices (mounted file systems, LVs, etc.)
could get errors and/or corrupt data.

(This not just about dynamic reconfiguration.
 Similar thing could happen if you forget to rebuild initrd
 after modifying bindings.)

If there is smarter solution such as swapping aliases correctly,
it would be nice.
But I think it's good to have a patch like this at a minimum
to prevent the bad thing from happening.

Comments?
---
Jun'ichi Nomura, NEC Corporation


Given alias/wwid pair in config, if there is a mpath with the wwid
and different alias, the mpath should be renamed to the given alias.
If there is already other mpath with the alias, though, we could not
simply rename it.
However, we must NOT try to create a mpath with the given alias/wwid
by changing the wwid (i.e. mappings) of the other mpath, that could
corrupt data.

The patch checks this case and give up processing.

diff -urp multipath-tools.orig/libmultipath/configure.c multipath-tools.new/libmultipath/configure.c
--- multipath-tools.orig/libmultipath/configure.c	2012-07-19 15:17:40.368622358 +0900
+++ multipath-tools.new/libmultipath/configure.c	2012-07-19 15:29:23.360746687 +0900
@@ -150,6 +150,7 @@ static void
 select_action (struct multipath * mpp, vector curmp, int force_reload)
 {
 	struct multipath * cmpp;
+	struct multipath * cmpp_tmp;
 
 	cmpp = find_mp_by_alias(curmp, mpp->alias);
 
@@ -169,7 +170,8 @@ select_action (struct multipath * mpp, v
 		return;
 	}
 
-	if (!find_mp_by_wwid(curmp, mpp->wwid)) {
+	cmpp_tmp = find_mp_by_wwid(curmp, mpp->wwid);
+	if (!cmpp_tmp) {
 		condlog(2, "%s: remove (wwid changed)", cmpp->alias);
 		dm_flush_map(mpp->alias);
 		strncpy(cmpp->wwid, mpp->wwid, WWID_SIZE);
@@ -180,6 +182,14 @@ select_action (struct multipath * mpp, v
 		return;
 	}
 
+	if (cmpp != cmpp_tmp) {
+		condlog(2, "%s: unable to rename %s to %s (%s is used by %s)",
+			mpp->wwid, cmpp_tmp->alias, mpp->alias,
+			mpp->alias, cmpp->wwid);
+		mpp->action = ACT_NOTHING;
+		return;
+	}
+
 	if (pathcount(mpp, PATH_UP) == 0) {
 		mpp->action = ACT_NOTHING;
 		condlog(3, "%s: set ACT_NOTHING (no usable path)",

[PATCH] multipath-tools: prevent unexpected swapping of underlying LUNs

[Jun'ichi Nomura]

When you want to rename a multipath device
but the new alias is already used by other multipath device,
multipath-tools mistakenly reload a table for the original multipath device
to the other multipath device.
That could lead to very bad result, such as I/O error and data corruption.

This patch checks such a condition and gives up renaming with error log.

For example, suppose you have following 'bindings' file:
   
   # cat /etc/multipath/bindings 
   mpatha 212140084abcd0000
   mpathb 212150084abcd0000

and a logical volume 'VG/LV0' on top of mpathb,
which is on top of /dev/sde(8:64) and /dev/sdk(8:160):

   # dmsetup ls --tree
   mpatha (253:1)
    ├─ (8:144)
    └─ (8:48)
   VG-LV0 (253:2)
    └─mpathb (253:0)
       ├─ (8:160)
       └─ (8:64)

Then you decide to swap their names and change the 'bindings' as follows:

   # cat /etc/multipath/bindings 
   mpathb 212140084abcd0000
   mpatha 212150084abcd0000

you'll get this after 'service multipathd reload':

   # dmsetup ls --tree
   mpatha (253:1)
    ├─ (8:160)
    └─ (8:64)
   VG-LV0 (253:2)
    └─mpathb (253:0)
       ├─ (8:144)
       └─ (8:48)

Now you suddenly have 'VG/LV0' on top of /dev/sdd(8:48) and /dev/sdj(8:144),
that is obviously wrong and will corrupt data if you write to 'VG/LV0'.


diff -urpN multipath-tools.orig/libmultipath/configure.c multipath-tools.new/libmultipath/configure.c
--- multipath-tools.orig/libmultipath/configure.c	2012-07-19 15:17:40.368622358 +0900
+++ multipath-tools.new/libmultipath/configure.c	2012-08-17 17:11:07.691898774 +0900
@@ -150,12 +150,12 @@ static void
 select_action (struct multipath * mpp, vector curmp, int force_reload)
 {
 	struct multipath * cmpp;
+	struct multipath * cmpp_by_name;
 
-	cmpp = find_mp_by_alias(curmp, mpp->alias);
-
-	if (!cmpp) {
-		cmpp = find_mp_by_wwid(curmp, mpp->wwid);
+	cmpp = find_mp_by_wwid(curmp, mpp->wwid);
+	cmpp_by_name = find_mp_by_alias(curmp, mpp->alias);
 
+	if (!cmpp_by_name) {
 		if (cmpp) {
 			condlog(2, "%s: rename %s to %s", mpp->wwid,
 				cmpp->alias, mpp->alias);
@@ -169,17 +169,25 @@ select_action (struct multipath * mpp, v
 		return;
 	}
 
-	if (!find_mp_by_wwid(curmp, mpp->wwid)) {
-		condlog(2, "%s: remove (wwid changed)", cmpp->alias);
+	if (!cmpp) {
+		condlog(2, "%s: remove (wwid changed)", mpp->alias);
 		dm_flush_map(mpp->alias);
-		strncpy(cmpp->wwid, mpp->wwid, WWID_SIZE);
-		drop_multipath(curmp, cmpp->wwid, KEEP_PATHS);
+		strncpy(cmpp_by_name->wwid, mpp->wwid, WWID_SIZE);
+		drop_multipath(curmp, cmpp_by_name->wwid, KEEP_PATHS);
 		mpp->action = ACT_CREATE;
 		condlog(3, "%s: set ACT_CREATE (map wwid change)",
 			mpp->alias);
 		return;
 	}
 
+	if (cmpp != cmpp_by_name) {
+		condlog(2, "%s: unable to rename %s to %s (%s is used by %s)",
+			mpp->wwid, cmpp->alias, mpp->alias,
+			mpp->alias, cmpp_by_name->wwid);
+		mpp->action = ACT_NOTHING;
+		return;
+	}
+
 	if (pathcount(mpp, PATH_UP) == 0) {
 		mpp->action = ACT_NOTHING;
 		condlog(3, "%s: set ACT_NOTHING (no usable path)",

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel