[PATCH v1 0/1] multipathd: don't trust udev WWIDs when moving paths between maps

[PATCH v1 0/1] multipathd: don't trust udev WWIDs when moving paths between maps

[Brian Bunker]

During an array controller failover on an iSCSI host, multipathd
briefly put a path to one LUN into the map of another LUN, and I/O
meant for the first LUN landed on the second. The host ran RHEL 9
device-mapper-multipath 0.8.7, but the code involved is essentially
unchanged in master.

What happened:

1. A change uevent for sdq (verify3) carried verify1's WWID. The next
   uevent 4.5 s later carried the correct one. We don't know whether
   the array answered the INQUIRY (VPD 0x83, run by scsi_id) wrongly
   during the takeover or the bad value arose elsewhere in the udev
   path; we have no packet capture.

2. uev_update_path() acted on the uevent WWID without asking the
   device and removed sdq from verify3's map. That reload blocked for
   4.4 s behind I/O stuck in the failover.

3. uev_update_path() then called uev_add_path() with the same, now
   stale, uevent. ev_add_path() put sdq into verify1's map, where it
   stayed for about 170 ms. Test tool records show a write meant for
   verify1 landing on verify3, and a verify1 read returning verify3's
   data.

recheck_wwid was off, but would not have helped: it is only consulted
when the checker sees a failed path come back and on "multipathd add
path", never for uevents.

The patch reads VPD 0x83 over SG_IO in uev_update_path() and again in
ev_add_path(), for paths where can_recheck_wwid() is true. If the
device still reports the cached WWID, the udev value is ignored. If
the device doesn't confirm the WWID a path is being added with, the
path is left out of the map and udev is retriggered.

Open points:

- If the SG_IO read fails, the add proceeds as before. Refusing the
  add would be stricter; opinions welcome.

- The checks depend on can_recheck_wwid(), not on the recheck_wwid
  option, because a mismatch at add time means data corruption and
  recheck_wwid is off by default.

- "multipathd reconfigure" builds maps through configure(), not
  ev_add_path(), so it isn't covered.

Brian Bunker (1):
  multipathd: verify udev WWIDs with the device before moving paths

 multipathd/main.c | 57 ++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 52 insertions(+), 5 deletions(-)

-- 
2.56.0

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

[Brian Bunker]

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(), refuse to add a path whose WWID the device doesn't
  confirm. 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.

ev_add_path() is the common point for uevent, CLI and checker adds, so
the second check holds even when the udev data is stale by the time it
is used. 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 three checks share it.

Signed-off-by: Brian Bunker <brian@purestorage.com>
---
 multipathd/main.c | 57 ++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 52 insertions(+), 5 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index 9bce4d6b..656efb6e 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
@@ -1315,6 +1344,20 @@ 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 */
 	}
+	/*
+	 * The WWID normally comes from udev, possibly from an event that
+	 * was queued long before we got here. 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 (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);
@@ -1702,6 +1745,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);
-- 
2.56.0

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

[Benjamin Marzinski]

On Thu, Oct 29, 2026 at 10:57:27AM -0700, Brian Bunker wrote:
> 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(), refuse to add a path whose WWID the device doesn't
>   confirm. 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.
> 
> ev_add_path() is the common point for uevent, CLI and checker adds, so
> the second check holds even when the udev data is stale by the time it
> is used. 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 three checks share it.

The code all looks good to me. I must admit, I'm a little paranoid about
forcing perfectly normal ev_add_path() calls to double-check the wwid.
Currently, that only happens if you enable "recheck_wwid", or if
something is going wrong with initializing the path. The issue is that
scsi devices can have multiple wwids, and the multipath code has to be
in sync with the code that udev runs to the wwid, otherwise they can be
different. I don't think these functions differ. I know I've checked
them in the past. But unless there is a problem getting the wwid from
udev or someone turns on recheck_wwid (I don't know how common that is)
paths don't ever run this code. 

We could avoid this by hijacking the need_do_map argument to
uev_add_path() and ev_add_path(). If, instead of just being 0 or 1, it
could also be a third value for reinits (we should probably use an enum
in this case), we could only double-check the wwid on reinits, which
only happen when there is an issue getting the wwid from udev.

Thoughts? Am I just being paranoid here?

-Ben
 
> Signed-off-by: Brian Bunker <brian@purestorage.com>
> ---
>  multipathd/main.c | 57 ++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 52 insertions(+), 5 deletions(-)
> 
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 9bce4d6b..656efb6e 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
> @@ -1315,6 +1344,20 @@ 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 */
>  	}
> +	/*
> +	 * The WWID normally comes from udev, possibly from an event that
> +	 * was queued long before we got here. 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 (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);
> @@ -1702,6 +1745,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);
> -- 
> 2.56.0

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

[Brian Bunker]

On Thu, Oct 29, 2026 at 05:27:39PM -0400, Benjamin Marzinski wrote:
> The code all looks good to me. I must admit, I'm a little paranoid about
> forcing perfectly normal ev_add_path() calls to double-check the wwid.
> Currently, that only happens if you enable "recheck_wwid", or if
> something is going wrong with initializing the path.

That's a fair concern. I made the check unconditional in ev_add_path()
because it is the one place every add passes through, but I didn't
weigh what it costs the normal case.

> The issue is that
> scsi devices can have multiple wwids, and the multipath code has to be
> in sync with the code that udev runs to the wwid, otherwise they can be
> different. I don't think these functions differ. I know I've checked
> them in the past. But unless there is a problem getting the wwid from
> udev or someone turns on recheck_wwid (I don't know how common that is)
> paths don't ever run this code.

can_recheck_wwid() already limits the check to SCSI paths using the
default uid_attribute, but I can't prove the two never pick different
designators on some device. If they did, every add on that device would
be refused and retriggered. That isn't a risk worth taking for a path
that has never had a problem.

> We could avoid this by hijacking the need_do_map argument to
> uev_add_path() and ev_add_path(). If, instead of just being 0 or 1, it
> could also be a third value for reinits (we should probably use an enum
> in this case), we could only double-check the wwid on reinits, which
> only happen when there is an issue getting the wwid from udev.

I like this, and v2 does it. need_do_map becomes enum path_add_mode
(PATH_ADD_NO_MAP, PATH_ADD_MAP, PATH_ADD_REINIT). PATH_ADD_REINIT
behaves like PATH_ADD_MAP and is passed only for the re-add at the end
of uev_update_path(). That covers both cases that set needs_reinit:
udev data not there yet, and udev reporting a changed WWID. The
changed-WWID re-add is the one that put sdq into the wrong map for us.
Ordinary adds, CLI adds and the checker's add_when_online path don't
run the check.

The uev_update_path() check stays as it was. It only runs when udev's
WWID differs from the cached one, and does nothing if the device
doesn't confirm the cached value.

> Thoughts? Am I just being paranoid here?

I don't think so. On how common the problem is: we hit it because a
test tool was opening and closing the sd devices read-write many times
a second. With udev's "watch" rule each close sends a change uevent
that runs scsi_id, so the host was producing far more udev traffic than
a normal one. I expect it to be rare elsewhere, but the result is data
going to the wrong LUN, so I'd rather not act on a changed WWID
without confirming it. Limiting the check to reinits keeps that and
leaves the normal path alone.

v2 follows in a separate thread.

Thanks,
Brian