Skip to content

zed: fix zpool handle leaks in zfs_retire agent - #19333

Open
wrouesnel wants to merge 3 commits into
openzfs:masterfrom
wrouesnel:fix-zed-retire-leaks
Open

wrouesnel wants to merge 3 commits into
openzfs:masterfrom
wrouesnel:fix-zed-retire-leaks

Conversation

@wrouesnel

Copy link
Copy Markdown
Contributor

Motivation and Context

The ZED's retire agent leaks memory for every faulted vdev. With the ZED under valgrind, faulting one vdev of a mirror with zpool offline -f leaves two pool handles definitely lost (352 bytes each, plus the pool config they hold, about 107 KB each, counted as indirectly lost). Both come from is_draid_fdomain_failure(), which looks the pool up twice through find_by_guid_impl() and never closes the handles. It does this for every faulted vdev, before it checks whether the vdev is in a dRAID at all.

On a dRAID with failure groups there is more:

  • A fault that is not (yet) a failure domain failure retries the lookup four times, so it leaks eight handles.
  • When it is a failure domain failure, zfs_retire_recv() returns without closing its own pool handle: in the state change path it also leaks the vdev name, and in the fault (suspect list) path the return that c21c4ff added leaks the handle.

valgrind shows each of these on master in the test below.

Found during a bug hunt I'm running with Claude Opus 5.5; the analysis, fix, test and testing were AI-generated, and reviewed and tested in VMs. Any "Extra Notes" section is a human-written addendum.

Description

This PR has three commits. The first one is not a test-only commit, so zfs-precheck takes the second as the failing test and the third as its fix:

  1. CI: install valgrind in the qemu test VMs: the new test runs the ZED under valgrind, and is skipped when valgrind is not installed. This adds valgrind to the package lists in qemu-3-deps-vm.sh (Alpine, Arch, Debian/Ubuntu, RHEL/Fedora), so the test runs in the Linux test VMs. It comes first so that every commit after it is tested with valgrind.

  2. ZTS: add fault/zed_retire_leak (Linux only): starts the ZED under valgrind, faults a vdev of a mirror with zpool offline -f, then creates a draid1:2d:4c:8w pool (two failure groups), faults its first vdev with zpool offline -f and makes the ZED diagnose an I/O fault for the vdev at the same index in the other group (zinject -e io -T read), so the retire agent sees a failure domain failure for both the fault and the state change. It then stops the ZED and fails if valgrind reports a definitely lost block allocated by the retire agent (a zfs_retire.c function in the stack). Leaks elsewhere in the ZED don't count. When the ZED has no symbols (as in the packages the CI installs), it counts pool handles leaked from a zpool_iter() call made by the ZED, which is how the retire agent looks up pools. valgrind is added to the ZTS command list.

  3. zed: fix zpool handle leaks in zfs_retire agent:

    • is_draid_fdomain_failure() opens the pool once per try, finds the top-level vdev in the same config, and closes the handle at the end of each try.
    • The early returns in the state change path of zfs_retire_recv() go through its common cleanup, which closes the handle and frees the vdev name.
    • In the suspect list loop, a failure domain failure closes the handle and continues with the next suspect instead of returning.

    Reading the code, the fix also frees the vdev name in remove_spares(), frees nfaults_map on the early exits of is_draid_fdomain_failure(), and checks for a zero top-level vdev guid there. The test does not reach these paths.

How Has This Been Tested?

Ubuntu 24.04 VM, Linux 6.8, debug build, valgrind 3.22, on master 4542a31:

  • Test commit alone: fault/zed_retire_leak failed 3 times in 3 runs, with 19 to 21 blocks definitely lost (5,841 to 6,609 bytes). In the full valgrind report of one such run, the leak records with zfs_retire.c frames were the pool handles from is_draid_fdomain_failure() (called from both the state change and the suspect list paths), the pool handles opened in both paths of zfs_retire_recv(), and the vdev name from zpool_vdev_name() in the state change path.
  • With this change: passed 3 times in 3 runs. The one definitely lost block left (104 bytes) is a case that zfs_diagnosis still has open when the ZED exits, not in the retire agent.
  • With zed and the libraries stripped as dh_strip does for the Debian packages, the test failed without the fix, where it found the leaks through the zpool_iter() fallback, and passed with it (one run each).

With this change, in the same VM: the events group passed (16 passed), and the fault group had 20 passed and 3 failed: auto_replace_001_pos, auto_replace_002_pos and auto_spare_multiple, which fail the same way without this change in this VM (master plus the test commit).

Types of Changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Quality assurance (non-breaking change which makes the code more robust against bugs)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Library ABI change (libzfs, libzfs_core, libnvpair and libzfsbootenv)
  • Documentation (a change to man pages or other documentation)

Checklist

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 <wrouesnel@wrouesnel.com>
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 <wrouesnel@wrouesnel.com>
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 c21c4ff 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 <wrouesnel@wrouesnel.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants