zed: fix zpool handle leaks in zfs_retire agent

.github/workflows/scripts/qemu-3-deps-vm.sh

@@ -23,8 +23,8 @@ function alpine() {
     linux-headers linux-tools linux-stable linux-stable-dev lsscsi m4 make \
     nfs-utils openssl-dev parted pax procps py3-cffi py3-distlib py3-packaging \
     py3-setuptools python3 python3-dev qemu-guest-agent rng-tools rsync samba \
-    samba-server sed strace sysstat tzdata util-linux util-linux-dev wget \
-    words xfsprogs xxhash zlib-dev pamtester@testing
+    samba-server sed strace sysstat tzdata util-linux util-linux-dev valgrind \
+    wget words xfsprogs xxhash zlib-dev pamtester@testing
   echo "##[endgroup]"
 
   echo "##[group]Switch to eudev"
@@ -63,7 +63,7 @@ function archlinux() {
   sudo pacman -Sy --noconfirm base-devel bc cpio cryptsetup dhclient dkms \
     fakeroot fio gdb inetutils jq less linux linux-headers lsscsi nfs-utils \
     parted pax perf python-packaging python-setuptools qemu-guest-agent ksh \
-    samba strace sysstat rng-tools rsync wget xxhash
+    samba strace sysstat rng-tools rsync valgrind wget xxhash
   echo "##[endgroup]"
 }
 
@@ -90,8 +90,8 @@ function debian() {
     lsscsi nfs-kernel-server pamtester parted python3 python3-all-dev \
     python3-cffi python3-dev python3-distlib python3-packaging libtirpc-dev \
     python3-setuptools python3-sphinx qemu-guest-agent rng-tools rpm2cpio \
-    rsync samba strace sysstat uuid-dev watchdog wget xfslibs-dev xxhash \
-    zlib1g-dev
+    rsync samba strace sysstat uuid-dev valgrind watchdog wget xfslibs-dev \
+    xxhash zlib1g-dev
   echo "##[endgroup]"
 }
 
@@ -135,8 +135,8 @@ function rhel() {
     libuuid-devel lsscsi mdadm nfs-utils openssl-devel pam-devel pamtester \
     parted perf python3 python3-cffi python3-devel python3-packaging \
     kernel-devel python3-setuptools qemu-guest-agent rng-tools rpcgen \
-    rpm-build rsync samba strace sysstat systemd watchdog wget xfsprogs-devel \
-    xxhash zlib-devel
+    rpm-build rsync samba strace sysstat systemd valgrind watchdog wget \
+    xfsprogs-devel xxhash zlib-devel
 
   # These are needed for building Lustre.  We only install these on EL VMs since
   # we don't plan to test build Lustre on other platforms.

cmd/zed/agents/zfs_retire.c

@@ -182,6 +182,7 @@ remove_spares(zpool_handle_t *zhp, void *data)
 			if (vs->vs_state != VDEV_STATE_REMOVED &&
 			    zpool_vdev_remove_wanted(zhp, devname) == 0)
 				cbp->cb_num_spares++;
+			free(devname);
 			break;
 		}
 	}
@@ -267,31 +268,37 @@ is_draid_fdomain_failure(fmd_hdl_t *hdl, libzfs_handle_t *zhdl,
     uint64_t pool_guid, uint64_t vdev_guid)
 {
 	uint64_t guid, top_guid;
-	uint64_t children;
-	nvlist_t *nvtop, *vdev, **child;
+	uint64_t children = 0;
+	zpool_handle_t *zhp;
+	nvlist_t *config, *nvroot, *nvtop, *vdev, **child;
 	vdev_stat_t *vs;
 	uint_t i, c, vdev_i = UINT_MAX, width, *nfaults_map = NULL;
 	boolean_t res = B_FALSE;
 
 	for (int try = 0; try < 4; try++) {
-		if (find_by_guid_impl(zhdl, pool_guid, vdev_guid, &vdev,
-		    &top_guid) == NULL)
-			return (B_FALSE);
-
-		if (find_by_guid_impl(zhdl, pool_guid, top_guid, &nvtop,
-		    NULL) == NULL)
-			return (B_FALSE);
-
-		if (nvlist_lookup_nvlist_array(nvtop, ZPOOL_CONFIG_CHILDREN,
-		    &child, &width) != 0)
-			return (B_FALSE);
-
-		if (nvlist_lookup_uint64(nvtop, ZPOOL_CONFIG_DRAID_NCHILDREN,
-		    &children) != 0) /* not dRAID */
-			return (B_FALSE);
+		/*
+		 * Re-open the pool on every try to get fresh vdev states.
+		 * The vdev and top-level vdev nvlists point into the pool
+		 * config, so the handle must stay open until we are done
+		 * with them.
+		 */
+		if ((zhp = find_by_guid_impl(zhdl, pool_guid, vdev_guid,
+		    &vdev, &top_guid)) == NULL)
+			break;
 
-		if (width == children) /* dRAID without failure domains */
-			return (B_FALSE);
+		config = zpool_get_config(zhp, NULL);
+		if (top_guid == 0 ||
+		    nvlist_lookup_nvlist(config, ZPOOL_CONFIG_VDEV_TREE,
+		    &nvroot) != 0 ||
+		    (nvtop = find_vdev(zhdl, nvroot, top_guid, NULL)) == NULL ||
+		    nvlist_lookup_nvlist_array(nvtop, ZPOOL_CONFIG_CHILDREN,
+		    &child, &width) != 0 ||
+		    nvlist_lookup_uint64(nvtop, ZPOOL_CONFIG_DRAID_NCHILDREN,
+		    &children) != 0 || /* not dRAID */
+		    width == children) { /* dRAID without failure domains */
+			zpool_close(zhp);
+			break;
+		}
 
 		if (nfaults_map == NULL)
 			nfaults_map = fmd_hdl_alloc(hdl,
@@ -311,6 +318,8 @@ is_draid_fdomain_failure(fmd_hdl_t *hdl, libzfs_handle_t *zhdl,
 				vdev_i = (c % children);
 		}
 
+		zpool_close(zhp);
+
 		for (c = 0; c < children; c++) {
 			if (c == vdev_i &&
 			    nfaults_map[c] == (width / children)) {
@@ -331,7 +340,9 @@ is_draid_fdomain_failure(fmd_hdl_t *hdl, libzfs_handle_t *zhdl,
 		sleep(5);
 	}
 
-	fmd_hdl_free(hdl, nfaults_map, children * sizeof (*nfaults_map));
+	if (nfaults_map != NULL)
+		fmd_hdl_free(hdl, nfaults_map,
+		    children * sizeof (*nfaults_map));
 
 	if (res)
 		fmd_hdl_debug(hdl, "vdev %llu belongs to draid fdomain failure",
@@ -595,6 +606,7 @@ zfs_retire_recv(fmd_hdl_t *hdl, fmd_event_t *ep, nvlist_t *nvl,
 		const char *devtype;
 		char *devname;
 		boolean_t skip_removal = B_FALSE;
+		int remove_status = 0;
 
 		if (nvlist_lookup_string(nvl, FM_EREPORT_PAYLOAD_ZFS_VDEV_TYPE,
 		    &devtype) == 0) {
@@ -633,15 +645,15 @@ zfs_retire_recv(fmd_hdl_t *hdl, fmd_event_t *ep, nvlist_t *nvl,
 		    (uint64_t **)&vs, &c);
 
 		if (vs->vs_state == VDEV_STATE_OFFLINE)
-			return;
+			goto removed_out;
 
 		/*
 		 * Resilvering domain failures can take a lot of computing and
 		 * I/O bandwidth resources, only to be wasted when the failed
 		 * domain component (for example enclosure) is replaced.
 		 */
 		if (is_draid_fdomain_failure(hdl, zhdl, pool_guid, vdev_guid))
-			return;
+			goto removed_out;
 
 		/*
 		 * If state removed is requested for already removed vdev,
@@ -654,12 +666,11 @@ zfs_retire_recv(fmd_hdl_t *hdl, fmd_event_t *ep, nvlist_t *nvl,
 			    nvlist_exists(nvl, "by_kernel")) {
 				skip_removal = B_TRUE;
 			} else {
-				return;
+				goto removed_out;
 			}
 		}
 
 		/* Remove the vdev since device is unplugged */
-		int remove_status = 0;
 		if (!skip_removal && (l2arc ||
 		    (strcmp(class, "resource.fs.zfs.removed") == 0))) {
 			remove_status = zpool_vdev_remove_wanted(zhp, devname);
@@ -675,6 +686,7 @@ zfs_retire_recv(fmd_hdl_t *hdl, fmd_event_t *ep, nvlist_t *nvl,
 			fmd_hdl_debug(hdl, "no spare for '%s'", devname);
 		}
 
+removed_out:
 		free(devname);
 		zpool_close(zhp);
 		return;
@@ -823,8 +835,10 @@ zfs_retire_recv(fmd_hdl_t *hdl, fmd_event_t *ep, nvlist_t *nvl,
 		 * I/O bandwidth resources, only to be wasted when the failed
 		 * domain component (for example enclosure) is replaced.
 		 */
-		if (is_draid_fdomain_failure(hdl, zhdl, pool_guid, vdev_guid))
-			return;
+		if (is_draid_fdomain_failure(hdl, zhdl, pool_guid, vdev_guid)) {
+			zpool_close(zhp);
+			continue;
+		}
 
 		/*
 		 * Attempt to substitute a hot spare.

tests/runfiles/linux.run

@@ -144,7 +144,7 @@ tests = ['auto_offline_001_pos', 'auto_online_001_pos', 'auto_online_002_pos',
     'decrypt_fault',
     'decompress_fault', 'fault_limits', 'scrub_after_resilver',
     'suspend_on_probe_errors', 'suspend_resume_single', 'suspend_draid_fgroups',
-    'zpool_status_-s']
+    'zed_retire_leak', 'zpool_status_-s']
 tags = ['functional', 'fault']
 
 [tests/functional/features/large_dnode:Linux]

tests/zfs-tests/include/commands.cfg

@@ -163,6 +163,7 @@ export SYSTEM_FILES_LINUX='attr
     useradd
     userdel
     usermod
+    valgrind
     wipefs'
 
 export ZFS_FILES='zdb

tests/zfs-tests/tests/Makefile.am

@@ -1736,6 +1736,7 @@ nobase_dist_datadir_zfs_tests_tests_SCRIPTS += \
 	functional/fault/suspend_resume_single.ksh \
 	functional/fault/suspend_draid_fgroups.ksh \
 	functional/fault/setup.ksh \
+	functional/fault/zed_retire_leak.ksh \
 	functional/fault/zpool_status_-s.ksh \
 	functional/features/async_destroy/async_destroy_001_pos.ksh \
 	functional/features/async_destroy/cleanup.ksh \