libzutil: keep valid spare and l2cache paths on import

Unlike the vdev tree, whose paths are read from the scanned labels
and whose label paths are added to the candidate name list (so the
exact match in fix_paths() preserves a persistent name whenever its
symlink is still alive), spare and l2cache configs are obtained from
the pool's MOS by a tryimport.  When the on-disk aux label carries
no path (labels last written before eb4a36bce) or a stale one, the
MOS path never appears among the scanned names and fix_paths()
rewrites it to the best scanned candidate.  With the default
libblkid discovery that is the bare /dev basename, which is not
stable across reboots.  Since 5536c0dee ("Sync AUX label during pool
import") the rewritten path is also synced back to the MOS and the
aux labels, making the reversion permanent.

This is the remaining piece of #13170, where a cache device added by
a /dev/disk/by-* name comes back as e.g. nvme0n1p4 after an export
and import.  It also answers the question raised there about #15817:
that change fixed the reversion for aux vdevs whose labels contain a
matching path, but pools carried over from older releases regress on
their first import and are then stuck with the unstable name.

Keep an existing spare or l2cache path when the device can still be
opened exclusively and the label found there matches the expected
vdev guid, mirroring the checks applied to scanned candidates.  The
MOS-supplied path is not trusted beyond that: like the device scan,
only a path naming a device type which is safe to probe is opened,
and the type is re-checked on the opened descriptor so a symlink
repointed between the check and the open cannot make the listing read
a label from an unexpected node.  Only keep paths when the search
locations were not chosen by the caller, since importing from user
supplied directories is the documented way to deliberately rewrite
a pool's device paths; -d and -s imports keep the current behavior.
Pools whose aux path was already rewritten to a bare /dev name by an
earlier import keep that name, since it is still valid; a one-time
import from the wanted directory, e.g. -d /dev/disk/by-id, remains
the way to restore a persistent path there.

Add a ZTS case covering the new and the preserved behavior: an aux
path which still refers to the expected device is kept on a default
import, is rewritten on a -d import, and is replaced by a scanned
name once it goes stale.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Ameer Hamza <[email protected]>
Signed-off-by: MorganaFuture <[email protected]>
Closes #13170
Closes #18742

lib/libzutil/os/freebsd/zutil_import_os.c

@@ -99,13 +99,83 @@ static const char * const excluded_devs[] = {
 #define	EXCLUDED_DIR		"/dev/"
 #define	EXCLUDED_DIR_LEN	5
 
+static boolean_t
+excluded_dev(const char *name)
+{
+	size_t i;
+
+	if (strncmp(name, EXCLUDED_DIR, EXCLUDED_DIR_LEN) != 0)
+		return (B_FALSE);
+
+	name += EXCLUDED_DIR_LEN;
+	for (i = 0; i < nitems(excluded_devs); ++i) {
+		const char *excluded_name = excluded_devs[i];
+		size_t len = strlen(excluded_name);
+		if (strncmp(name, excluded_name, len) == 0)
+			return (B_TRUE);
+	}
+
+	return (B_FALSE);
+}
+
+/*
+ * Common predicate for zpool_dev_probe_ok() and its fd variant: only a
+ * disk device (character or block), or a regular file large enough to
+ * hold a label, may be probed.  Anything else is refused.
+ */
+static boolean_t
+dev_stat_probe_ok(const struct stat64 *statbuf)
+{
+	if (S_ISREG(statbuf->st_mode))
+		return (statbuf->st_size >= SPA_MINDEVSIZE);
+
+	return (S_ISCHR(statbuf->st_mode) || S_ISBLK(statbuf->st_mode));
+}
+
+/*
+ * Determine if a path may be safely opened to probe for a vdev label.
+ * Only regular files large enough to hold a label and disk devices
+ * (character or block) are acceptable.  Anything else is refused,
+ * opening other nodes can have side effects.  stat64() never blocks,
+ * even on a FIFO.
+ */
+boolean_t
+zpool_dev_probe_ok(const char *path)
+{
+	struct stat64 statbuf;
+
+	if (excluded_dev(path))
+		return (B_FALSE);
+
+	if (stat64(path, &statbuf) != 0)
+		return (B_FALSE);
+
+	return (dev_stat_probe_ok(&statbuf));
+}
+
+/*
+ * As zpool_dev_probe_ok(), but re-check the type of an object already
+ * opened.  A path naming a symlink may have been repointed at a different
+ * node between the stat64() above and the open(), so only trust a
+ * descriptor which is still a disk device or a large enough regular file.
+ */
+boolean_t
+zpool_dev_probe_ok_fd(int fd)
+{
+	struct stat64 statbuf;
+
+	if (fstat64(fd, &statbuf) != 0)
+		return (B_FALSE);
+
+	return (dev_stat_probe_ok(&statbuf));
+}
+
 void
 zpool_open_func(void *arg)
 {
 	rdsk_node_t *rn = arg;
 	struct stat64 statbuf;
 	nvlist_t *config;
-	size_t i;
 	int num_labels;
 	int fd;
 	off_t mediasize = 0;
@@ -113,16 +183,8 @@ zpool_open_func(void *arg)
 	/*
 	 * Do not even look at excluded devices.
 	 */
-	if (strncmp(rn->rn_name, EXCLUDED_DIR, EXCLUDED_DIR_LEN) == 0) {
-		char *name = rn->rn_name + EXCLUDED_DIR_LEN;
-		for (i = 0; i < nitems(excluded_devs); ++i) {
-			const char *excluded_name = excluded_devs[i];
-			size_t len = strlen(excluded_name);
-			if (strncmp(name, excluded_name, len) == 0) {
-				return;
-			}
-		}
-	}
+	if (excluded_dev(rn->rn_name))
+		return;
 
 	/*
 	 * O_NONBLOCK so we don't hang trying to open things like serial ports.

lib/libzutil/os/linux/zutil_import_os.c

@@ -92,6 +92,57 @@ should_skip_dev(const char *dev)
 	    (strcmp(dev, "hpet") == 0));
 }
 
+/*
+ * Common predicate for zpool_dev_probe_ok() and its fd variant: only a
+ * block device, or a regular file large enough to hold a label, may be
+ * probed.  Anything else is refused.
+ */
+static boolean_t
+dev_stat_probe_ok(const struct stat64 *statbuf)
+{
+	return (S_ISBLK(statbuf->st_mode) ||
+	    (S_ISREG(statbuf->st_mode) && statbuf->st_size >= SPA_MINDEVSIZE));
+}
+
+/*
+ * Determine if a path may be safely opened to probe for a vdev label.
+ * Only regular files large enough to hold a label and block devices are
+ * acceptable.  Anything else is refused: opening other device nodes can
+ * have side effects (e.g. arming a watchdog) and opening a FIFO blocks
+ * indefinitely.  stat64() never blocks, even on a FIFO.
+ */
+boolean_t
+zpool_dev_probe_ok(const char *path)
+{
+	struct stat64 statbuf;
+
+	if (should_skip_dev(zfs_basename(path)))
+		return (B_FALSE);
+
+	/* Ignore failed stats. */
+	if (stat64(path, &statbuf) != 0)
+		return (B_FALSE);
+
+	return (dev_stat_probe_ok(&statbuf));
+}
+
+/*
+ * As zpool_dev_probe_ok(), but re-check the type of an object already
+ * opened.  A path naming a symlink may have been repointed at a different
+ * node between the stat64() above and the open(), so only trust a
+ * descriptor which is still a block device or a large enough regular file.
+ */
+boolean_t
+zpool_dev_probe_ok_fd(int fd)
+{
+	struct stat64 statbuf;
+
+	if (fstat64(fd, &statbuf) != 0)
+		return (B_FALSE);
+
+	return (dev_stat_probe_ok(&statbuf));
+}
+
 int
 zfs_dev_flush(int fd)
 {
@@ -103,23 +154,13 @@ zpool_open_func(void *arg)
 {
 	rdsk_node_t *rn = arg;
 	libpc_handle_t *hdl = rn->rn_hdl;
-	struct stat64 statbuf;
 	nvlist_t *config;
 	uint64_t vdev_guid = 0;
 	int error;
 	int num_labels = 0;
 	int fd;
 
-	if (should_skip_dev(zfs_basename(rn->rn_name)))
-		return;
-
-	/*
-	 * Ignore failed stats.  We only want regular files and block devices.
-	 * Ignore files that are too small to hold a zpool.
-	 */
-	if (stat64(rn->rn_name, &statbuf) != 0 ||
-	    (!S_ISREG(statbuf.st_mode) && !S_ISBLK(statbuf.st_mode)) ||
-	    (S_ISREG(statbuf.st_mode) && statbuf.st_size < SPA_MINDEVSIZE))
+	if (!zpool_dev_probe_ok(rn->rn_name))
 		return;
 
 	/*

lib/libzutil/zutil_import.c

@@ -305,6 +305,78 @@ fix_paths(libpc_handle_t *hdl, nvlist_t *nv, name_entry_t *names)
 	return (0);
 }
 
+/*
+ * Determine if the path in the given spare or l2cache vdev config still
+ * refers to the expected device.  Unlike the vdev tree, which is built
+ * from the scanned labels, these configs are read from the pool's MOS
+ * by a tryimport.  Their path may be a persistent name (by-id, by-vdev,
+ * multipath, ...) which is perfectly valid yet absent from the list of
+ * scanned names, in which case fix_paths() would needlessly rewrite it
+ * to some scanned name of last resort (e.g. a bare /dev basename which
+ * is not stable across reboots).  The device is verified much as the
+ * scanned candidates are: the path must name a device type which is
+ * safe to probe, a label must be readable from it and the label vdev
+ * guid must match the expected one.  As in zpool_find_import_impl(),
+ * the device must also be openable exclusively, otherwise it may be
+ * an in-use multipath component.
+ */
+static boolean_t
+aux_path_active(nvlist_t *nv)
+{
+	const char *path;
+	uint64_t guid, label_guid;
+	nvlist_t *label = NULL;
+	int fd, num_labels;
+	boolean_t active = B_FALSE;
+
+	if (nvlist_lookup_string(nv, ZPOOL_CONFIG_PATH, &path) != 0 ||
+	    nvlist_lookup_uint64(nv, ZPOOL_CONFIG_GUID, &guid) != 0)
+		return (B_FALSE);
+
+	/*
+	 * The path comes from the pool's MOS and may name anything, so
+	 * check it is safe to probe first.  O_NONBLOCK below keeps the open
+	 * of a FIFO swapped in after this check from blocking, and the type
+	 * of the opened descriptor is re-checked below before it is used.
+	 */
+	if (!zpool_dev_probe_ok(path))
+		return (B_FALSE);
+
+	/*
+	 * Preferentially open using O_DIRECT to bypass the block device
+	 * cache which may be stale for multipath devices.  An EINVAL errno
+	 * indicates O_DIRECT is unsupported so fallback to just O_RDONLY.
+	 */
+	fd = open(path, O_RDONLY | O_EXCL | O_NONBLOCK | O_DIRECT | O_CLOEXEC);
+	if (fd < 0 && errno == EINVAL)
+		fd = open(path, O_RDONLY | O_EXCL | O_NONBLOCK | O_CLOEXEC);
+	if (fd < 0)
+		return (B_FALSE);
+
+	/*
+	 * zpool_dev_probe_ok() stat'd the name, but if the path was a
+	 * symlink it could have been repointed at a different node before
+	 * the open() above.  Re-check the type of the descriptor we now
+	 * hold so a crafted MOS path cannot make us read a label from an
+	 * unexpected node.
+	 */
+	if (!zpool_dev_probe_ok_fd(fd)) {
+		(void) close(fd);
+		return (B_FALSE);
+	}
+
+	if (zpool_read_label(fd, &label, &num_labels) == 0 && label != NULL) {
+		if (nvlist_lookup_uint64(label, ZPOOL_CONFIG_GUID,
+		    &label_guid) == 0 && label_guid == guid)
+			active = B_TRUE;
+		nvlist_free(label);
+	}
+
+	(void) close(fd);
+
+	return (active);
+}
+
 /*
  * Add the given configuration to the list of known devices.
  */
@@ -488,7 +560,7 @@ vdev_is_hole(uint64_t *hole_array, uint_t holes, uint_t id)
  */
 static nvlist_t *
 get_configs(libpc_handle_t *hdl, pool_list_t *pl, boolean_t active_ok,
-    nvlist_t *policy)
+    boolean_t keep_aux_path, nvlist_t *policy)
 {
 	pool_entry_t *pe;
 	vdev_entry_t *ve;
@@ -846,13 +918,20 @@ get_configs(libpc_handle_t *hdl, pool_list_t *pl, boolean_t active_ok,
 
 		/*
 		 * Go through and update the paths for spares, now that we have
-		 * them.
+		 * them.  A path which still refers to the expected device is
+		 * kept as is, it may well be more persistent than any of the
+		 * scanned names.
 		 */
 		verify(nvlist_lookup_nvlist(config, ZPOOL_CONFIG_VDEV_TREE,
 		    &nvroot) == 0);
 		if (nvlist_lookup_nvlist_array(nvroot, ZPOOL_CONFIG_SPARES,
 		    &spares, &nspares) == 0) {
 			for (i = 0; i < nspares; i++) {
+				if (keep_aux_path &&
+				    aux_path_active(spares[i])) {
+					update_vdev_config_dev_strs(spares[i]);
+					continue;
+				}
 				if (fix_paths(hdl, spares[i], pl->names) != 0)
 					goto nomem;
 			}
@@ -864,6 +943,11 @@ get_configs(libpc_handle_t *hdl, pool_list_t *pl, boolean_t active_ok,
 		if (nvlist_lookup_nvlist_array(nvroot, ZPOOL_CONFIG_L2CACHE,
 		    &l2cache, &nl2cache) == 0) {
 			for (i = 0; i < nl2cache; i++) {
+				if (keep_aux_path &&
+				    aux_path_active(l2cache[i])) {
+					update_vdev_config_dev_strs(l2cache[i]);
+					continue;
+				}
 				if (fix_paths(hdl, l2cache[i], pl->names) != 0)
 					goto nomem;
 			}
@@ -1556,7 +1640,15 @@ zpool_find_import_impl(libpc_handle_t *hdl, importargs_t *iarg,
 	avl_destroy(cache);
 	free(cache);
 
-	ret = get_configs(hdl, &pools, iarg->can_be_active, iarg->policy);
+	/*
+	 * Existing spare and l2cache paths may only be trusted when the
+	 * search locations were not chosen by the caller.  An import from
+	 * user supplied directories (or a scan of them) is the documented
+	 * way to deliberately rewrite all of the pool's device paths, so
+	 * in that case they must be derived from the scanned names alone.
+	 */
+	ret = get_configs(hdl, &pools, iarg->can_be_active,
+	    iarg->paths == 0 && !iarg->scan, iarg->policy);
 
 	for (pe = pools.pools; pe != NULL; pe = penext) {
 		penext = pe->pe_next;

lib/libzutil/zutil_import.h

@@ -57,6 +57,8 @@ typedef struct rdsk_node {
 
 int slice_cache_compare(const void *, const void *);
 
+boolean_t zpool_dev_probe_ok(const char *path);
+boolean_t zpool_dev_probe_ok_fd(int fd);
 void zpool_open_func(void *);
 
 #endif /* _LIBZUTIL_ZUTIL_IMPORT_H_ */

tests/runfiles/linux.run

@@ -74,7 +74,7 @@ tests = ['zpool_expand_001_pos', 'zpool_expand_002_pos',
 tags = ['functional', 'cli_root', 'zpool_expand']
 
 [tests/functional/cli_root/zpool_import:Linux]
-tests = ['zpool_import_hostid_changed',
+tests = ['zpool_import_aux_paths', 'zpool_import_hostid_changed',
     'zpool_import_hostid_changed_unclean_export',
     'zpool_import_hostid_changed_cachefile',
     'zpool_import_hostid_changed_cachefile_unclean_export']

tests/zfs-tests/tests/Makefile.am

@@ -1234,6 +1234,7 @@ nobase_dist_datadir_zfs_tests_tests_SCRIPTS += \
 	functional/cli_root/zpool_import/zpool_import_016_pos.ksh \
 	functional/cli_root/zpool_import/zpool_import_017_pos.ksh \
 	functional/cli_root/zpool_import/zpool_import_all_001_pos.ksh \
+	functional/cli_root/zpool_import/zpool_import_aux_paths.ksh \
 	functional/cli_root/zpool_import/zpool_import_encrypted.ksh \
 	functional/cli_root/zpool_import/zpool_import_encrypted_load.ksh \
 	functional/cli_root/zpool_import/zpool_import_errata3.ksh \

tests/zfs-tests/tests/functional/cli_root/zpool_import/zpool_import_aux_paths.ksh

@@ -0,0 +1,123 @@
+#!/bin/ksh -p
+# SPDX-License-Identifier: CDDL-1.0
+#
+# CDDL HEADER START
+#
+# The contents of this file are subject to the terms of the
+# Common Development and Distribution License (the "License").
+# You may not use this file except in compliance with the License.
+#
+# You can obtain a copy of the license at usr/src/OPENSOLARIS.LICENSE
+# or https://opensource.org/licenses/CDDL-1.0.
+# See the License for the specific language governing permissions
+# and limitations under the License.
+#
+# When distributing Covered Code, include this CDDL HEADER in each
+# file and include the License file at usr/src/OPENSOLARIS.LICENSE.
+# If applicable, add the following below this CDDL HEADER, with the
+# fields enclosed by brackets "[]" replaced with your own identifying
+# information: Portions Copyright [yyyy] [name of copyright owner]
+#
+# CDDL HEADER END
+#
+
+#
+# Copyright (c) 2026, MorganaFuture. All rights reserved.
+#
+
+. $STF_SUITE/include/libtest.shlib
+
+#
+# DESCRIPTION:
+#	Spare and l2cache vdev paths read from the MOS must survive an
+#	import which searches the default locations, as long as they still
+#	refer to the expected device.  Importing from user supplied
+#	directories (-d) must keep rewriting them, and paths which no
+#	longer refer to the device must be replaced with a scanned name.
+#
+# STRATEGY:
+#	1. Create a pool on scsi_debug partitions whose cache and spare
+#	   devices are addressed by custom persistent symlinks, i.e. names
+#	   the default device scan will never report.
+#	2. Export and import the pool without -d, then verify the cache
+#	   and spare paths were kept.
+#	3. Import the pool from a directory of differently named symlinks
+#	   with -d, then verify the cache and spare paths were rewritten.
+#	4. Remove the symlinks the cache and spare paths now refer to,
+#	   import without -d, then verify the stale paths were replaced
+#	   with reachable ones and the pool is healthy.
+#
+# NOTE:
+#	This locks the aux path handling of the import listing.  A pool
+#	created by a current release records the persistent path in the aux
+#	label, so step 2 already passes on unpatched code; the fix also
+#	keeps paths for pools whose aux label predates that path being
+#	recorded, which the standard tools cannot reproduce here, so that
+#	case is not exercised directly.  Steps 3 and 4 cover the -d rewrite
+#	and stale-path replacement behavior the fix must preserve.
+#
+
+verify_runnable "global"
+
+DEVLINK_DIR=$TEST_BASE_DIR/devlink_aux-test
+DEVLINK_DIR2=$TEST_BASE_DIR/devlink_aux-test.2
+SDSIZE_MB=400
+
+function cleanup
+{
+	destroy_pool $TESTPOOL1
+	rm -rf $DEVLINK_DIR $DEVLINK_DIR2
+	if lsmod | grep -q scsi_debug; then
+		unload_scsi_debug
+	fi
+}
+
+log_assert "Import keeps valid aux vdev paths, rewrites stale or -d ones"
+
+log_onexit cleanup
+
+load_scsi_debug $SDSIZE_MB 1 1 1 '512b'
+SDDEVICE=$(get_debug_device)
+[[ -n $SDDEVICE && $SDDEVICE != "-" ]] || \
+    log_unsupported "Could not find scsi_debug device"
+
+log_must parted /dev/$SDDEVICE --script mklabel gpt
+log_must parted /dev/$SDDEVICE --script mkpart primary 1MiB 133MiB
+log_must parted /dev/$SDDEVICE --script mkpart primary 133MiB 266MiB
+log_must parted /dev/$SDDEVICE --script mkpart primary 266MiB 399MiB
+block_device_wait /dev/${SDDEVICE}1 /dev/${SDDEVICE}2 /dev/${SDDEVICE}3
+
+log_must mkdir -p $DEVLINK_DIR $DEVLINK_DIR2
+log_must ln -s /dev/${SDDEVICE}2 $DEVLINK_DIR/cache-link
+log_must ln -s /dev/${SDDEVICE}3 $DEVLINK_DIR/spare-link
+log_must ln -s /dev/${SDDEVICE}1 $DEVLINK_DIR2/main2
+log_must ln -s /dev/${SDDEVICE}2 $DEVLINK_DIR2/cache2
+log_must ln -s /dev/${SDDEVICE}3 $DEVLINK_DIR2/spare2
+
+log_must zpool create -f $TESTPOOL1 /dev/${SDDEVICE}1 \
+    cache $DEVLINK_DIR/cache-link spare $DEVLINK_DIR/spare-link
+
+# A default (blkid) import must keep the custom aux names: they still
+# refer to the right devices even though no scan reports them.
+log_must zpool export $TESTPOOL1
+log_must zpool import $TESTPOOL1
+log_must eval "zpool status -P $TESTPOOL1 | grep -q $DEVLINK_DIR/cache-link"
+log_must eval "zpool status -P $TESTPOOL1 | grep -q $DEVLINK_DIR/spare-link"
+
+# An import from user supplied directories must keep rewriting the aux
+# paths to the scanned names, even though the old names are still valid.
+log_must zpool export $TESTPOOL1
+log_must zpool import -d $DEVLINK_DIR2 $TESTPOOL1
+log_must eval "zpool status -P $TESTPOOL1 | grep -q $DEVLINK_DIR2/cache2"
+log_must eval "zpool status -P $TESTPOOL1 | grep -q $DEVLINK_DIR2/spare2"
+
+# Aux paths which no longer refer to the device must not be kept by a
+# default import; they are rewritten to a name found by the scan.
+log_must zpool export $TESTPOOL1
+log_must rm $DEVLINK_DIR2/cache2 $DEVLINK_DIR2/spare2
+log_must zpool import $TESTPOOL1
+log_mustnot eval "zpool status -P $TESTPOOL1 | grep -q $DEVLINK_DIR2/cache2"
+log_mustnot eval "zpool status -P $TESTPOOL1 | grep -q $DEVLINK_DIR2/spare2"
+log_mustnot eval "zpool status $TESTPOOL1 | grep -q UNAVAIL"
+
+log_pass "Import keeps valid aux vdev paths, rewrites stale or -d ones"