[PATCH v2 1/1] multipathd: verify udev WWIDs with the device before moving paths

[Brian Bunker <brian@purestorage.com>]

multipathd takes a path's WWID from udev. When a change uevent reports
a different WWID, uev_update_path() removes the path from its map and
re-adds it through uev_add_path(), which places it in whatever map owns
the new WWID. Nothing checks the new value against the device itself;
recheck_wwid only covers paths coming back up in the checker and
"multipathd add path". If udev reports the wrong WWID, or the uevent is
stale by the time it is processed, the path goes into the map of a
different LUN and I/O for one LUN is silently sent to another.

Use SG_IO to read VPD page 0x83 from the device in two places, for
paths where can_recheck_wwid() is true:

- In uev_update_path(), if the device still reports the cached WWID,
  ignore the udev value and leave the path where it is.

- In ev_add_path(), when a path is being re-added because its udev
  data was missing or changed, refuse to add it if the device doesn't
  confirm its WWID. The path is orphaned, its WWID is cleared and it
  is marked INIT_MISSING_UDEV, so the checker retriggers udev (bounded
  by retrigger_tries) and the path is re-added from fresh data.

To tell re-adds from ordinary adds, turn the need_do_map argument of
uev_add_path() and ev_add_path() into enum path_add_mode, with a new
PATH_ADD_REINIT value that behaves like PATH_ADD_MAP and also requests
the WWID check. Ordinary adds are unchanged and never read the WWID
from the device.

If the SG_IO read fails, behavior is unchanged.

Split the SG_IO read out of check_path_wwid_change() into
get_sgio_wwid() so all the checks share it.

Signed-off-by: Brian Bunker <brian@purestorage.com>
---
 multipathd/cli_handlers.c |  2 +-
 multipathd/main.c         | 79 +++++++++++++++++++++++++++++++--------
 multipathd/main.h         | 12 +++++-
 3 files changed, 76 insertions(+), 17 deletions(-)

diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
index f5a13c46..9b4a04c2 100644
--- a/multipathd/cli_handlers.c
+++ b/multipathd/cli_handlers.c
@@ -673,7 +673,7 @@ cli_add_path (void *v, struct strbuf *reply, void *data)
 			return 1;
 		}
 	}
-	return ev_add_path(pp, vecs, 1);
+	return ev_add_path(pp, vecs, PATH_ADD_MAP);
 blacklisted:
 	append_strbuf_str(reply, "blacklisted\n");
 	condlog(2, "%s: path blacklisted", param);
diff --git a/multipathd/main.c b/multipathd/main.c
index 9bce4d6b..57a3dd9e 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -1086,14 +1086,16 @@ handle_path_wwid_change(struct path *pp, struct vectors *vecs)
 	return removed;
 }
 
-bool
-check_path_wwid_change(struct path *pp)
+/*
+ * Read the WWID directly from the device with SG_IO, bypassing udev
+ * and sysfs. Returns true and fills in wwid on success.
+ */
+static bool get_sgio_wwid(struct path *pp, char *wwid)
 {
-	char wwid[WWID_SIZE];
-	int len = 0;
+	int len;
 	size_t i;
 
-	if (!strlen(pp->wwid))
+	if (pp->fd < 0)
 		return false;
 
 	/* Get the real fresh device wwid by sgio. sysfs still has old
@@ -1111,6 +1113,15 @@ check_path_wwid_change(struct path *pp)
 		/* no-op */
 	wwid[i] = '\0';
 	condlog(4, "%s: Got wwid %s by sgio", pp->dev, wwid);
+	return true;
+}
+
+bool check_path_wwid_change(struct path *pp)
+{
+	char wwid[WWID_SIZE];
+
+	if (!strlen(pp->wwid) || !get_sgio_wwid(pp, wwid))
+		return false;
 
 	if (strncmp(wwid, pp->wwid, WWID_SIZE)) {
 		condlog(0, "%s: wwid '%s' doesn't match wwid '%s' from device",
@@ -1121,6 +1132,24 @@ check_path_wwid_change(struct path *pp)
 	return false;
 }
 
+/*
+ * udev reported a new WWID for a path. Returns true if the device
+ * itself still reports old_wwid, in which case the udev value must
+ * not be trusted.
+ */
+static bool device_kept_wwid(struct path *pp, const char *old_wwid)
+{
+	char wwid[WWID_SIZE];
+
+	if (!can_recheck_wwid(pp) || !get_sgio_wwid(pp, wwid) ||
+	    strncmp(wwid, old_wwid, WWID_SIZE))
+		return false;
+
+	condlog(0, "%s: ignoring wwid '%s' from udev, device still reports '%s'",
+		pp->dev, pp->wwid, wwid);
+	return true;
+}
+
 /*
  * uev_add_path can call uev_update_path, and uev_update_path can call
  * uev_add_path
@@ -1128,7 +1157,8 @@ check_path_wwid_change(struct path *pp)
 static int uev_update_path (struct uevent *uev, struct vectors * vecs);
 
 static int
-uev_add_path (struct uevent *uev, struct vectors * vecs, int need_do_map)
+uev_add_path (struct uevent *uev, struct vectors * vecs,
+	      enum path_add_mode mode)
 {
 	struct path *pp;
 	int ret = 0, i;
@@ -1180,7 +1210,7 @@ uev_add_path (struct uevent *uev, struct vectors * vecs, int need_do_map)
 				     DI_ALL | DI_BLACKLIST);
 			pthread_cleanup_pop(1);
 			if (r == PATHINFO_OK && !prev_mpp)
-				ret = ev_add_path(pp, vecs, need_do_map);
+				ret = ev_add_path(pp, vecs, mode);
 			else if (r == PATHINFO_OK &&
 				 !strncmp(pp->wwid, prev_mpp->wwid, WWID_SIZE)) {
 				/*
@@ -1255,7 +1285,7 @@ uev_add_path (struct uevent *uev, struct vectors * vecs, int need_do_map)
 		conf = get_multipath_config();
 		pp->checkint = conf->checkint;
 		put_multipath_config(conf);
-		ret = ev_add_path(pp, vecs, need_do_map);
+		ret = ev_add_path(pp, vecs, mode);
 	} else {
 		condlog(0, "%s: failed to store path info, "
 			"dropping event",
@@ -1298,7 +1328,7 @@ sysfs_get_ro (struct path *pp)
  * 1: error
  */
 int
-ev_add_path (struct path * pp, struct vectors * vecs, int need_do_map)
+ev_add_path (struct path * pp, struct vectors * vecs, enum path_add_mode mode)
 {
 	struct multipath * mpp;
 	char *params __attribute__((cleanup(cleanup_charp))) = NULL;
@@ -1315,6 +1345,21 @@ ev_add_path (struct path * pp, struct vectors * vecs, int need_do_map)
 		condlog(0, "%s: failed to get path uid", pp->dev);
 		goto fail; /* leave path added to pathvec */
 	}
+	/*
+	 * This path is being re-added because its udev data was missing or
+	 * changed. The uevent may have been queued long before we got here,
+	 * and adding a path to the map of a different LUN corrupts data, so
+	 * confirm the WWID with the device first. On a mismatch, leave the
+	 * path out and let the checker retrigger udev to get a fresh WWID.
+	 */
+	if (mode == PATH_ADD_REINIT && can_recheck_wwid(pp) &&
+	    check_path_wwid_change(pp)) {
+		condlog(0, "%s: not adding path, wwid '%s' not confirmed by device",
+			pp->dev, pp->wwid);
+		memset(pp->wwid, 0, WWID_SIZE);
+		pp->initialized = INIT_MISSING_UDEV;
+		goto fail;
+	}
 	mpp = find_mp_by_wwid(vecs->mpvec, pp->wwid);
 	if (mpp && pp->size && mpp->size != pp->size) {
 		condlog(0, "%s: failed to add new path %s, device size mismatch", mpp->alias, pp->dev);
@@ -1375,7 +1420,7 @@ rescan:
 	if (ro == 1)
 		mpp->force_readonly = 1;
 
-	if (!need_do_map)
+	if (mode == PATH_ADD_NO_MAP)
 		return 0;
 
 	if (!dm_map_present(mpp->alias)) {
@@ -1702,6 +1747,10 @@ uev_update_path (struct uevent *uev, struct vectors * vecs)
 			strcpy(pp->wwid, wwid);
 		else if (strlen(wwid) &&
 			 strncmp(wwid, pp->wwid, WWID_SIZE) != 0) {
+			if (device_kept_wwid(pp, wwid)) {
+				strcpy(pp->wwid, wwid);
+				goto out;
+			}
 			condlog(0, "%s: path wwid changed from '%s' to '%s'",
 				uev->kernel, wwid, pp->wwid);
 			ev_remove_path(pp, vecs, 1);
@@ -1789,7 +1838,7 @@ out:
 	}
 	/* pp->initialized must not be INIT_PARTIAL if needs_reinit is set */
 	if (needs_reinit)
-		retval = uev_add_path(uev, vecs, 1);
+		retval = uev_add_path(uev, vecs, PATH_ADD_REINIT);
 	return retval;
 }
 
@@ -1905,13 +1954,13 @@ uev_trigger (struct uevent * uev, void * trigger_data)
 	 */
 	list_for_each_entry_safe(merge_uev, tmp, &uev->merge_node, node) {
 		if (!strncmp(merge_uev->action, "add", 3))
-			r += uev_add_path(merge_uev, vecs, 0);
+			r += uev_add_path(merge_uev, vecs, PATH_ADD_NO_MAP);
 		if (!strncmp(merge_uev->action, "remove", 6))
 			r += uev_remove_path(merge_uev, vecs, 0);
 	}
 
 	if (!strncmp(uev->action, "add", 3))
-		r += uev_add_path(uev, vecs, 1);
+		r += uev_add_path(uev, vecs, PATH_ADD_MAP);
 	if (!strncmp(uev->action, "remove", 6))
 		r += uev_remove_path(uev, vecs, 1);
 	if (!strncmp(uev->action, "change", 6))
@@ -3015,7 +3064,7 @@ update_uninitialized_path(struct vectors * vecs, struct path * pp)
 		pthread_cleanup_pop(1);
 		/* INIT_OK implies ret == PATHINFO_OK */
 		if (pp->initialized == INIT_OK) {
-			ev_add_path(pp, vecs, 1);
+			ev_add_path(pp, vecs, PATH_ADD_MAP);
 			pp->tick = 1;
 		} else if (ret == PATHINFO_SKIPPED) {
 			int i;
@@ -3036,7 +3085,7 @@ update_uninitialized_path(struct vectors * vecs, struct path * pp)
 					CHECK_PATH_REMOVED :
 					CHECK_PATH_SKIPPED;
 		}
-		ev_add_path(pp, vecs, 1);
+		ev_add_path(pp, vecs, PATH_ADD_MAP);
 		pp->tick = 1;
 	}
 	return CHECK_PATH_CHECKED;
diff --git a/multipathd/main.h b/multipathd/main.h
index b66208ec..34a12ad7 100644
--- a/multipathd/main.h
+++ b/multipathd/main.h
@@ -36,7 +36,17 @@ enum daemon_status wait_for_state_change_if(enum daemon_status oldstate,
 					    unsigned long ms);
 void schedule_reconfigure(enum force_reload_types requested_type);
 int need_to_delay_reconfig (struct vectors *);
-int ev_add_path (struct path *, struct vectors *, int);
+/*
+ * PATH_ADD_REINIT is PATH_ADD_MAP for a path that is being re-added
+ * because its udev data was missing or changed. Its WWID is verified
+ * with the device before the path is put into a map.
+ */
+enum path_add_mode {
+	PATH_ADD_NO_MAP,
+	PATH_ADD_MAP,
+	PATH_ADD_REINIT,
+};
+int ev_add_path (struct path *, struct vectors *, enum path_add_mode);
 int ev_remove_path (struct path *, struct vectors *, int);
 int ev_add_map (char *, const char *, struct vectors *);
 int flush_map(struct multipath *, struct vectors *);
-- 
2.56.0