From 3752214951ec395bed91ffe397c06945da11a1e0 Mon Sep 17 00:00:00 2001 From: Will Rouesnel Date: Sun, 11 Oct 2026 08:06:54 +1100 Subject: [PATCH 1/3] CI: install valgrind in the qemu test VMs The ZTS test fault/zed_retire_leak runs the ZED under valgrind and is skipped when valgrind is not installed. Install it on the Linux test VMs so that the test runs in CI. Signed-off-by: Will Rouesnel --- .github/workflows/scripts/qemu-3-deps-vm.sh | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/.github/workflows/scripts/qemu-3-deps-vm.sh b/.github/workflows/scripts/qemu-3-deps-vm.sh index da6fc710e771..9e031fb174ee 100755 --- a/.github/workflows/scripts/qemu-3-deps-vm.sh +++ b/.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. From 7292d09a490b63edef035552aeb44042099036d2 Mon Sep 17 00:00:00 2001 From: Will Rouesnel Date: Sun, 11 Oct 2026 08:06:54 +1100 Subject: [PATCH 2/3] ZTS: add fault/zed_retire_leak Add a test that runs the ZED under valgrind, faults vdevs, waits for the retire agent to handle them, stops the ZED, and fails if valgrind reports a definitely lost block allocated by the retire agent. It first faults a vdev of a mirror with "zpool offline -f". Then it creates a dRAID with two failure groups, faults one vdev with "zpool offline -f", and has the ZED diagnose an I/O fault for the vdev at the same index in the other group, so the retire agent handles a failure domain failure, both for the fault and for the state change. The retire agent currently leaks the two pool handles that is_draid_fdomain_failure() opens for every faulted vdev, for any pool type, and on a failure domain failure it returns without closing the pool handle and freeing the vdev name, so this test fails. Only leak records with a zfs_retire.c function in the stack count, so unrelated ZED leaks do not fail the test. If the ZED has no symbols, pool handles leaked from a zpool_iter() call made by the ZED count instead. The test is skipped when valgrind is not installed. Add valgrind to the ZTS command list so the test can find it. Signed-off-by: Will Rouesnel --- tests/runfiles/linux.run | 2 +- tests/zfs-tests/include/commands.cfg | 1 + tests/zfs-tests/tests/Makefile.am | 1 + .../functional/fault/zed_retire_leak.ksh | 195 ++++++++++++++++++ 4 files changed, 198 insertions(+), 1 deletion(-) create mode 100755 tests/zfs-tests/tests/functional/fault/zed_retire_leak.ksh diff --git a/tests/runfiles/linux.run b/tests/runfiles/linux.run index 4b9cfb281e99..17dde4e0768b 100644 --- a/tests/runfiles/linux.run +++ b/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] diff --git a/tests/zfs-tests/include/commands.cfg b/tests/zfs-tests/include/commands.cfg index d66e31cd42ea..a712c85a037f 100644 --- a/tests/zfs-tests/include/commands.cfg +++ b/tests/zfs-tests/include/commands.cfg @@ -163,6 +163,7 @@ export SYSTEM_FILES_LINUX='attr useradd userdel usermod + valgrind wipefs' export ZFS_FILES='zdb diff --git a/tests/zfs-tests/tests/Makefile.am b/tests/zfs-tests/tests/Makefile.am index 969b6179880a..dda9fac531aa 100644 --- a/tests/zfs-tests/tests/Makefile.am +++ b/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 \ diff --git a/tests/zfs-tests/tests/functional/fault/zed_retire_leak.ksh b/tests/zfs-tests/tests/functional/fault/zed_retire_leak.ksh new file mode 100755 index 000000000000..bebc32a6152d --- /dev/null +++ b/tests/zfs-tests/tests/functional/fault/zed_retire_leak.ksh @@ -0,0 +1,195 @@ +#!/bin/ksh -p +# SPDX-License-Identifier: CDDL-1.0 +# +# This file and its contents are supplied under the terms of the +# Common Development and Distribution License ("CDDL"), version 1.0. +# You may only use this file in accordance with the terms of version +# 1.0 of the CDDL. +# +# A full copy of the text of the CDDL should have accompanied this +# source. A copy of the CDDL is also available via the Internet at +# https://opensource.org/license/CDDL-1.0. +# + +# +# Copyright (c) 2026 by Will Rouesnel. All rights reserved. +# + +. $STF_SUITE/include/libtest.shlib +. $STF_SUITE/tests/functional/fault/fault.cfg + +# +# DESCRIPTION: +# The zfs-retire agent in the ZED must not leak memory while handling +# a faulted vdev. +# +# STRATEGY: +# 1. Stop the ZED and start it again under valgrind. +# 2. Create a mirrored pool and fault one of its vdevs. +# 3. Wait for the retire agent to finish handling the fault. +# 4. Create a dRAID pool with two failure groups and fault one of its +# vdevs. Then fault the vdev at the same index in the other group +# through I/O errors, so that the ZED diagnoses the fault and the +# retire agent sees a failure domain failure, both for the fault +# and for the state change. +# 5. Stop the ZED so valgrind writes its leak report. +# 6. Verify that no definitely lost block was allocated by the retire +# agent. +# + +verify_runnable "global" + +if ! command -v valgrind > /dev/null; then + log_unsupported "valgrind is not installed" +fi + +VG_LOG="$TEST_BASE_DIR/zed_retire_leak.valgrind" +VG_ZED_LOG="$TEST_BASE_DIR/zed_retire_leak.zed" +VG_ZED_STATE="$TEST_BASE_DIR/zed_retire_leak.state" +VG_PID="" +DRAID_FILES=$(echo $TEST_BASE_DIR/draid-{0..7}) + +function stop_vg_zed +{ + typeset -i i=0 + + [[ -z "$VG_PID" ]] && return 0 + kill -TERM $VG_PID 2>/dev/null + # valgrind checks for leaks when the ZED exits, which takes a while. + while kill -0 $VG_PID 2>/dev/null && ((i < 180)); do + sleep 1 + ((i++)) + done + if kill -0 $VG_PID 2>/dev/null; then + kill -KILL $VG_PID 2>/dev/null + wait $VG_PID 2>/dev/null + VG_PID="" + return 1 + fi + wait $VG_PID 2>/dev/null + VG_PID="" + return 0 +} + +# Wait until the ZED has logged MSG at least COUNT times. +function wait_zed_log # msg count +{ + typeset msg=$1 + typeset -i count=$2 + typeset -i i=0 + + while (($(grep -c "$msg" $VG_ZED_LOG) < count)); do + if ! kill -0 $VG_PID 2>/dev/null; then + cat $VG_ZED_LOG + log_fail "ZED exited under valgrind" + fi + if ((i >= 300)); then + cat $VG_ZED_LOG + log_fail "Timed out waiting for the ZED to log '$msg'" + fi + sleep 1 + ((i++)) + done +} + +function cleanup +{ + stop_vg_zed + zinject -c all + destroy_pool $TESTPOOL + destroy_pool $TESTPOOL1 + rm -f $VDEV_FILES $DRAID_FILES $VG_LOG $VG_ZED_LOG $VG_ZED_STATE + # Restart the ZED for the rest of the group. + if [[ -z "$(zed_check)" ]]; then + zed_start + fi +} + +log_assert "The ZED retire agent does not leak memory on a vdev fault" +log_onexit cleanup + +# +# Run the ZED under valgrind, with the same arguments zed_start uses. +# In a build tree, zed is a libtool wrapper script that execs the real +# binary, so follow exec, but not into the zedlets. Debian's valgrind +# is a wrapper script that runs "$0.bin", so resolve the symlink to it +# in the constrained path. +# +zed_stop +zed_events_drain +rm -f $VG_LOG $VG_ZED_LOG $VG_ZED_STATE +VALGRIND=$(readlink -f "$(command -v valgrind)") +$VALGRIND --leak-check=full --show-leak-kinds=definite \ + --errors-for-leak-kinds=definite --num-callers=40 \ + --trace-children=yes --trace-children-skip='*.sh' \ + --child-silent-after-fork=yes --log-file=$VG_LOG \ + "$(command -v zed)" -vF -d $ZEDLET_DIR -P $PATH -s $VG_ZED_STATE -j 1 \ + 2>$VG_ZED_LOG & +VG_PID=$! +log_note "Started ZED under valgrind, pid $VG_PID" + +# Fault a leaf vdev of a mirror. This posts a statechange event with +# the FAULTED state, which the retire agent handles by looking for a +# hot spare. +truncate -s $MINVDEVSIZE $VDEV_FILES +log_must zpool create -f $TESTPOOL mirror $VDEV_FILES +log_must zpool offline -f $TESTPOOL $FAULT_FILE +log_must check_vdev_state $TESTPOOL $FAULT_FILE "FAULTED" + +# The retire agent logs this once it has handled the event. +wait_zed_log "Retire Agent: no spare for" 1 + +# +# Fault a vdev of a dRAID with two failure groups of four vdevs: not a +# failure domain failure yet, so the retire agent looks for a spare. +# +log_must truncate -s $MINVDEVSIZE $DRAID_FILES +log_must zpool create -f $TESTPOOL1 draid1:2d:4c:8w $DRAID_FILES +log_must zfs create -o primarycache=none -o recordsize=16k $TESTPOOL1/fs +log_must dd if=/dev/urandom of=/$TESTPOOL1/fs/file bs=1M count=32 +sync_pool $TESTPOOL1 +log_must zpool offline -f $TESTPOOL1 $TEST_BASE_DIR/draid-0 +log_must check_vdev_state $TESTPOOL1 $TEST_BASE_DIR/draid-0 "FAULTED" +wait_zed_log "Retire Agent: no spare for" 2 + +# +# Fail reads from the vdev at the same index in the other group, so the +# ZED diagnoses an I/O fault for it. The retire agent faults it, and +# then sees that every failure group has lost the vdev at this index: +# once while handling the fault, and once for the state change. +# +log_must zinject -d $TEST_BASE_DIR/draid-4 -e io -T read $TESTPOOL1 +log_must cp /$TESTPOOL1/fs/file /dev/null +log_must wait_vdev_state $TESTPOOL1 $TEST_BASE_DIR/draid-4 "FAULTED" 60 +wait_zed_log "belongs to draid fdomain failure" 2 +log_must zinject -c all + +log_must stop_vg_zed +log_must grep -q "LEAK SUMMARY\|All heap blocks were freed" $VG_LOG + +# +# Print each definitely lost record that was allocated by the retire +# agent. If the ZED has no symbols (e.g. a stripped package without its +# debug symbols), its own frames show up as "??? (in .../zed)"; then +# match pool handles leaked from a zpool_iter() call made by the ZED, +# which is how the retire agent looks up pools. +# +LEAKS=$(awk ' + /definitely lost in loss record/ { rec = $0; inrec = 1; next } + inrec && /^==[0-9]+== *$/ { + if (rec ~ /zfs_retire|is_draid_fdomain_failure|find_by_guid|remove_spares/ || + rec ~ /zpool_iter[^\n]*\n[^\n]*\?\?\? \(in [^)]*\/zed\)/) + print rec "\n" + inrec = 0 + next + } + inrec { rec = rec "\n" $0 } +' $VG_LOG) + +grep -A6 "LEAK SUMMARY" $VG_LOG +if [[ -n "$LEAKS" ]]; then + echo "$LEAKS" + log_fail "The ZED retire agent leaked memory" +fi + +log_pass "The ZED retire agent does not leak memory on a vdev fault" From ead3c4002f5e25624d719ff7683ed9a4059e202e Mon Sep 17 00:00:00 2001 From: Will Rouesnel Date: Thu, 1 Oct 2026 05:45:07 +1000 Subject: [PATCH 3/3] zed: fix zpool handle leaks in zfs_retire agent is_draid_fdomain_failure() looks up the pool twice per try, up to four tries, via find_by_guid_impl(), but never closes the returned zpool_handle_t. Since it is called for every removed/faulted vdev, regardless of the pool type (the "not dRAID" check happens after the handles are opened), zed leaks two pool handles, including their config nvlists, on every such event, and up to eight when a dRAID with failure domains is not (yet) in a domain failure state. The nfaults_map buffer was also leaked on the early returns. Rework it to open the pool only once per try, look up the top-level vdev in the same config snapshot, and close the handle at the end of each try. Also guard against a zero top-level guid, which previously left nvtop uninitialized. zfs_retire_recv() had similar leaks: - In the resource.fs.zfs.removed/statechange path, the early returns for an offline vdev, a dRAID domain failure and the loopback removal event leaked both zhp and devname. Route them through the common cleanup. - Commit c21c4ff4b added a "return" inside the fault suspect loop when a dRAID domain failure is detected, which leaked zhp. The intent is only to not kick in a hot spare for this vdev, so close the handle and continue with the next suspect instead. Finally, remove_spares() leaked the vdev name returned by zpool_vdev_name(). This makes the new fault/zed_retire_leak test pass. Signed-off-by: Will Rouesnel --- cmd/zed/agents/zfs_retire.c | 66 ++++++++++++++++++++++--------------- 1 file changed, 40 insertions(+), 26 deletions(-) diff --git a/cmd/zed/agents/zfs_retire.c b/cmd/zed/agents/zfs_retire.c index c7f5277fdafa..11afbac8372e 100644 --- a/cmd/zed/agents/zfs_retire.c +++ b/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,7 +645,7 @@ 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 @@ -641,7 +653,7 @@ zfs_retire_recv(fmd_hdl_t *hdl, fmd_event_t *ep, nvlist_t *nvl, * 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.