libzfs: don't read a dataset handle after closing it in resume send by mkhllr · Pull Request #18883 · openzfs/zfs

mkhllr · 2026-08-02T23:30:53Z

Motivation and Context

zfs_send_resume_impl_cb_impl() closes the dataset handle at
lib/libzfs/libzfs_sendrecv.c:1992, before the switch on the send error, and
the ESRCH case then reads zhp->zfs_name again at :2002. zfs_name is a
char[] declared inside struct zfs_handle, so the free(zhp) at the end of
zfs_close() releases the array along with the handle, and lzc_exists()
copies out of the freed block.

The close came in with resume send (47dfff3, 2016), when nothing below it
touched the handle. The ESRCH case was added three years later by redacted
send (30af21b, 2019), below a handle that was no longer live.

The path is reachable. dsl_bookmark_lookup_impl() returns ESRCH when a
bookmark is missing (dsl_bookmark.c:78,93), and dmu_send() gets there for a
fromsnap that is a bookmark. A resume whose incremental source is a bookmark
can lose a race with that bookmark being destroyed between libzfs resolving it
and the kernel looking it up. I went looking for a non-racy trigger and did not
find one: both guid_to_name_redact_snaps() and find_redact_book() confirm
the object exists client-side first, so the window is the only way in.

Description

@RigelYoung's report proposed two fixes. This takes the second: copy the name
into a local before the close and test that in the ESRCH case, in preference
to hoisting the lzc_exists() call above the close. It keeps the ESRCH-only
work inside the ESRCH case and mirrors the errbuf capture already on the line
above.

The report also warned that the existing local name cannot be reused here,
which is correct: guid_to_name_redact_snaps() overwrites it with the
incremental source when fromguid != 0, so it would test the wrong object.

How Has This Been Tested?

Ubuntu 24.04, --enable-debug, on 74e76f2, in a VM.

Reproduction: pool, dataset, @s1, zfs bookmark ... #bm, destroy @s1 so
only the bookmark carries that guid, more data, @s2, then a truncated
resumable receive to mint a token. Resuming makes libzfs pass
from=/src#bm. To make the interleaving deterministic without touching
the code under test, an LD_PRELOAD shim holds the exported
lzc_send_resume_redacted() for a few seconds while a helper destroys the
bookmark.

Under valgrind on stock master:

Invalid read of size 1
   at strlen
   by strlcpy (strlcpy.c:24)
   by lzc_exists (libzfs_core.c:527)
   by zfs_send_resume_impl_cb_impl (libzfs_sendrecv.c:2002)
 Address 0x548b680 is 16 bytes inside a block of size 616 free'd
   by zfs_send_resume_impl_cb_impl (libzfs_sendrecv.c:1992)
 Block was alloc'd at
   by make_dataset_handle (libzfs_dataset.c:483)
   by zfs_open (libzfs_dataset.c:735)
   by zfs_send_resume_impl_cb_impl (libzfs_sendrecv.c:1884)

Offset 16 is offsetof(struct zfs_handle, zfs_name).

Three runs each way, rebuilding between arms, with every run asserting the
ESRCH arm was actually reached:

	stock	patched
invalid reads at :2002	4, in 3/3 runs	0, in 3/3 runs
conditional jump on uninitialised	165	165
use of uninitialised value	48	48
syscall param uninitialised	1	1
total error contexts	218	214

so the four removed contexts are the use-after-free and nothing else moved.
The remaining reports are pre-existing noise present in both arms. The
user-visible error text is identical either way, and a resume with the bookmark
still in place sends and receives normally.

Worth noting for anyone reaching for it: ASan does not catch this. The only
read is through strlcpy, which resolves to strlcpy@GLIBC_2.38, and libasan
ships no strlcpy interceptor, so the access happens inside uninstrumented
glibc.

ZTS rsend 79 pass / 1 known skip, zfs_send and zfs_receive all pass,
make checkstyle and scripts/commitcheck.sh pass.

No ZTS test is included, and I do not think one is worth adding here. The
user-visible behaviour is the same with and without the patch, so a test could
only ever pass; the defect shows up under memcheck alone, and forcing the
window needs the LD_PRELOAD shim, which is not something ZTS should carry.
Happy to be told otherwise.

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

My code follows the OpenZFS code style requirements.
I have updated the documentation accordingly.
I have read the contributing document.
I have added tests to cover my changes.
I have run the ZFS Test Suite with this change applied.
All commit messages are properly formatted and contain Signed-off-by.

Separate note, not in this patch

Two zfs_handle_t leaks sit in the same function on error paths between the
zfs_open() at :1884 and the first close: :1902 and :1922 both return without
closing the handle. I left them out to keep this diff to the reported bug.
Happy to fold them in or send them separately, whichever you prefer.

zfs_send_resume_impl_cb_impl() closes the dataset handle before its error switch and then reads zhp->zfs_name again in the ESRCH case. zfs_name is an array declared inside struct zfs_handle, so the free() at the end of zfs_close() releases it along with the handle, and lzc_exists() copies out of the freed block. The close dates from the original resume send. The ESRCH case was added three years later with redacted send, below a handle that was no longer live. The path is reachable: dsl_bookmark_lookup() returns ESRCH when the incremental source is a bookmark that has gone away, which a resume can lose a race with. Keep a copy of the name alongside the error message that is already formatted before the close, and test that instead. Reported-by: RigelYoung <43904538+RigelYoung@users.noreply.github.com> Signed-off-by: Michael Heller Closes openzfs#18870

robn

I would probably have done a mild refactor, setting error instead of returning directly through the !dryrun block, and then keeping zhp alive until the the zfs_close() before the the return at the end.

But, this is focused on a fairly rare event, so yeah, fine.

mkhllr · 2026-08-03T15:52:46Z

Thanks both.

Four jobs are red and I do not think any of them reach this change. Notes in
case it saves someone the digging.

zloop is a ztest abort: *** ztest crash found, one core, and
thread 1 coming out of fatal() at cmd/ztest.c:683 with
attach (%s %lu, %s %lu, %d) returned %d, expected %d, called from
ztest_vdev_attach_detach at cmd/ztest.c:3954. Other threads are blocked in
txg_wait_synced on txg 639 at the time, but the deadman thread itself is idle
in poll.

ztest cannot see this patch either way. ztest_LDADD is
libzpool.la libzfs_core.la libnvpair.la (cmd/Makefile.am:51), and
libzfs_sendrecv.c is only compiled into lib/libzfs, so the changed code is
not in that binary. The library list in the core dump has no libzfs.so in it.

I also ran the zloop job's own configuration eight times on unmodified
74e76f2 to see whether it reproduces, matching the workflow down to the
4 vCPU, the sanitiser and debug-kmem flags, the hostid, and
-t 600 -I 6 -l -m 1 -- -T 120 -P 60. Eight passes, 36 ztest iterations, no
cores. Please read that as "did not reproduce" and nothing stronger: eight
clean runs only put the failure rate below about 31% at 95%, which does not
distinguish a rare flake from none.

freebsd15-1s is log_spacemap/log_spacemap_flushall, the only unexpected
failure in that job, failing its own assertion after
zpool condense -t log_spacemap -w:

ERROR: test 1936 -gt 2416 exited 1

The spacemap grew where the test wants it to shrink. That test contains no
zfs send or zfs receive reference.

fedora44 ran to completion, 2050 PASS and 13 expected FAIL, with neither
VM reporting an unexpected result. The red comes from the cleanup step:

.github/workflows/scripts/qemu-6-tests.sh: line 143: kill: (20700) - No such process
##[error]Process completed with exit code 1.

vm2's log reader had died at 01:57 while vm2 itself kept testing until 02:14,
so its summary never reached the live log and the later kill found nothing to
kill. #18885 has a fix for both halves of that.

ubuntu26 timed out: The action 'Run tests' has timed out after 270 minutes. vm1's environment came apart part-way through — mv_files/setup was
killed at its ten minute limit, and from there nearly every following group's
setup failed, ending at 110 FAIL, 224 SKIP and that one KILLED, with all but
two of the per-test logs captured empty. Two mmp failures at 01:45 predate
that, with passing tests in between, so they may be separate. vm2 ran to
completion over the same period.

rsend and send_xdr_encoding are inside the collapse: their setups failed
and every test in them was skipped, so the send coverage there never ran.
cli_root/zfs_send did run, earlier, and all 13 of its tests passed.

The change alters only the ESRCH arm of zfs_send_resume_impl_cb_impl(),
reached from the resume and saved send paths (zfs send -t and
zfs send --saved). On zloop the changed code is not in the binary; the
freebsd failure is in a test that never calls send; fedora44's red comes from
the cleanup step; and on ubuntu26 the send groups were skipped
after the environment went down, while the send tests that did run passed.

On the refactor: keeping zhp open until a single zfs_close() at the end
would also pick up two leaks in the same function, at libzfs_sendrecv.c:1902
and :1922, where the error returns after the zfs_open() at :1884 do not
close the handle. I left those out to keep this diff to the reported bug, and
I am happy to send them as a follow-up.

behlendorf · 2026-08-04T20:24:30Z

Those builders are all passing after second run. Some flaky tests.

zfs_send_resume_impl_cb_impl() closes the dataset handle before its error switch and then reads zhp->zfs_name again in the ESRCH case. zfs_name is an array declared inside struct zfs_handle, so the free() at the end of zfs_close() releases it along with the handle, and lzc_exists() copies out of the freed block. The close dates from the original resume send. The ESRCH case was added three years later with redacted send, below a handle that was no longer live. The path is reachable: dsl_bookmark_lookup() returns ESRCH when the incremental source is a bookmark that has gone away, which a resume can lose a race with. Keep a copy of the name alongside the error message that is already formatted before the close, and test that instead. Reported-by: RigelYoung <43904538+RigelYoung@users.noreply.github.com> Reviewed-by: Rob Norris Reviewed-by: Alexander Motin Reviewed-by: Brian Behlendorf Signed-off-by: Michael Heller Closes openzfs#18870 Closes openzfs#18883

#430) * ZTS: Pass dec instead of hex to mknod On Ubuntu 26.04 the default mknod command returns an error when provided the major and minor numbers in hex. Switch to passing decimal values. Reviewed-by: Tino Reichardt Reviewed-by: George Melikov Reviewed-by: Tony Hutter Signed-off-by: Brian Behlendorf Closes #18547 * zbookmark_compare: handle "marker" bookmarks with negative levels "Marker" bookmarks (those with zb_level == ZB_ROOT_LEVEL, ZB_ZIL_LEVEL or ZB_DNODE_LEVEL) represent valid blocks, but are associated with a dataset directly rather than with a specific object within it. They end up on bookmark lists during scan prefetch, and so need to be sorted ahead of any "true" object blocks. The problem is that for negative levels, BP_SPANB produces a negative shift, which is not legal C. Fortunately the results are used only for comparison, so the worst possible behaviour in a forgiving compilation environment is a mis-sort, which for the scan/traverse cases, means that we haven't prefetched certain metadata before we actually need it. But there _is_ UB in there, and UBSAN does rightly complain. Here we fix all this by handling these bookmarks directly - sorting them ahead of "true" object blocks, which is usually what scan/traverse will prefer. And we don't do any interesting math on these bookmarks, so we sidestep the whole UB thing. Sponsored-by: TrueNAS Reviewed-by: Alexander Motin Reviewed-by: Brian Behlendorf Signed-off-by: Rob Norris Closes #14777 Closes #18652 * CI: Have zfs-build-packages workflow build tarballs on Alma (#18662) Previously, zfs-build-packages would only build source tarballs on Fedora due to problems with building them on RHEL 7. That's a relic of the past now, as we haven't supported RHEL 7 since it went EOL in 2024. With this change, we now build the tarballs on both Alma and Fedora. Signed-off-by: Tony Hutter Reviewed-by: Olaf Faaland Reviewed-by: Chris Longros * Update our CI runners to the newest FreeBSD 15.1 RELEASE (#18667) Signed-off-by: Christos Longros Reviewed-by: Alexander Motin Reviewed-by: Tony Hutter * Linux 7.1 compat: META (#18682) Update the META file to reflect compatibility with the 7.1 kernel. Signed-off-by: Tony Hutter Signed-off-by: Rob Norris Reviewed-by: Chris Longros * CI: Re-allow workflow_dispatch on zfs-qemu Allow zfs-qemu to be invoked from a workflow_dispatch event (a.k.a, manually running a workflow). This may have been accidentally disabled in 1916c2c55. Reviewed-by: Chris Longros Reviewed-by: Brian Behlendorf Signed-off-by: Tony Hutter Closes #18680 * initramfs-zfs should not try to copy directories We had find only return files from the beginning for libgcc.so, but not libfetch/libcurl. This oversight affected a user when vmware installed its own libcurl.so.4 in a directory called libcurl.so.4, since our code then tried to copy a directory, which fails. Reviewed-by: Chris Longros Reviewed-by: Brian Behlendorf Suggested-by: Carsten Härle Signed-off-by: Richard Yao Closes #18582 Closes #18686 * Clean up embedded slog metaslab across txgs On a read-write import, metaslab_set_fragmentation() can dirty a metaslab via vdev_dirty() while still in the txg==0 load path when its space map has an unexpected bonus size (e.g. a makefs-created pool whose space-map dnodes use the boot loader's 24-byte space_map_phys_t with nblkptr=3, giving db_size=64). If that metaslab is then selected as the embedded slog, vdev_metaslab_init() only removed it from vdev_ms_list when txg != 0, so the txg==0 case left it queued and metaslab_fini() tripped VERIFY(!txg_list_member(&vd->vdev_ms_list, msp, t)). Remove slog_ms from the dirty list for every TXG_SIZE slot before metaslab_fini() so the cleanup is correct regardless of txg. Reported on FreeBSD as PR 281520: External-issue: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=281520 Reviewed-by: Alexander Motin Reviewed-by: Brian Behlendorf Signed-off-by: Nick Price Closes #18693 * README: update supported FreeBSD release to 15.1 Our CI runners moved to FreeBSD 15.1 in 0a4b59765 (#18667), but the README still lists 15.0. Update it to match the CI version. Reviewed-by: Brian Behlendorf Reviewed-by: Alexander Motin Signed-off-by: Christos Longros Closes #18696 * honor file argument in file_wait_event grep the log path passed by the caller instead of always using ZED_DEBUG_LOG. Reviewed-by: Alexander Motin Reviewed-by: Brian Behlendorf Signed-off-by: Alek Pinchuk Closes #18700 * Update mtime/ctime when fallocate grows a file Growing a file with fallocate updated its size but left mtime/ctime unchanged and didn't log the change. A fallocate that changes the file size should update mtime/ctime, and the change should be logged so it survives a crash. Pass log=TRUE to zfs_freesp() on the extend path so it updates the timestamps and logs the size change, matching zfs_space(). Punch-hole and zero-range already use this path and are unaffected. Reviewed-by: Rob Norris Reviewed-by: Alexander Motin Reviewed-by: Brian Behlendorf Signed-off-by: Ameer Hamza Closes #18573 * ddt_log: Fix refcount tagging for begin/commit Sponsored-by: Klara, Inc. Sponsored-by: Wasabi Technology, Inc. Reviewed-by: Rob Norris Reviewed-by: Alexander Motin Reviewed-by: Brian Behlendorf Signed-off-by: Igor Ostapenko Closes #18706 * CI: Increase default watchdog NMI timeout on Linux When the watchdog driver is configured and enabled an NMI will be generated when the watchdogd process fails to regularly reset the watchdog timer. Given the heavily virtualized and potentially over-subscribed nature of the CI environment increase the default timeout to 120 seconds (normally defaults to 30 seconds). Reviewed-by: Christos Longros Signed-off-by: Brian Behlendorf Closes #18704 * Fix race between device removal completion and pool export vdev_remove_complete() finalizes a device removal in two phases under the spa lock framework. Between the two phases it called spa_vdev_exit(), which drops both the config locks (SCL_ALL) and spa_namespace_lock and blocks on a txg sync. By that point vdev_remove_replace_with_indirect() has already set svr->svr_thread = NULL, and that is the only thing the export path (spa_export_common() -> spa_async_suspend() -> spa_vdev_remove_suspend()) waits on. Once the namespace lock is dropped, a concurrent export or destroy can acquire it and set spa->spa_export_thread. When the removal thread re-enters for its second phase via spa_vdev_enter(), it trips the ASSERT0P(spa->spa_export_thread) assertion. Hold spa_namespace_lock across both phases instead of dropping and re-taking it: the intermediate spa_vdev_exit() becomes spa_vdev_config_exit(), which drops only SCL_ALL and syncs the txg while keeping the namespace lock held, and the second spa_vdev_enter() becomes spa_vdev_config_enter(). Because the namespace lock is never dropped between the phases, a concurrent export blocks in spa_namespace_enter() and cannot set spa_export_thread until removal finalization is done. This mirrors the multi-phase locking pattern already used by the attach/detach and split paths in spa.c. Reviewed-by: Brian Behlendorf Signed-off-by: Prakash Surya Closes #18657 * linux: handle mmap read beyond file size When performing a mmap read past the end of a file there is no data to read, so simply zero-fill the page and return success. zfs_getpage() limits the range lock appropriately to cover the offset being read. Reported-by: Iliya Polihronov (@vnsavage) (Automattic) Reviewed-by: Alexander Motin Signed-off-by: Brian Behlendorf Closes #18715 * Using net/cloud-init to unpin specific python These days freebsd 15/16 fail when fetching py311-cloud-init. Switch to net/cloud-init to avoid python version pinning. Reviewed-by: Brian Behlendorf Signed-off-by: tiehexue Closes #18717 * Linux 7.2: zpl_super: convert to sget_fc() The old sget() superblock matcher has been removed in favour of the fscontext-based sget_fc(). This converts to it. Its largely a signature change, no functional change. sget_fc() has existed since fscontext was introduced, so there's no need for separate feature tests. Sponsored-by: TrueNAS Reviewed-by: Alexander Motin Reviewed-by: Brian Behlendorf Signed-off-by: Rob Norris Closes #18677 * zpl_ctldir: remove comments describing ancient kernel behaviour Sponsored-by: TrueNAS Reviewed-by: Brian Behlendorf Signed-off-by: Rob Norris Closes #18722 * libzfs: fix MS_CRYPT/MS_OVERLAY collision with umount2(2) flags MS_CRYPT and MS_OVERLAY are libzfs-internal mount flags, but their values (0x8 and 0x4) aliased the umount2(2) flags UMOUNT_NOFOLLOW and MNT_EXPIRE. A consumer that legitimately set UMOUNT_NOFOLLOW therefore had that bit read as MS_CRYPT, so libzfs unloaded the dataset's encryption key as a side effect. Move both flags to high bits unused by umount2(2) and strip them before the unmount syscall in do_unmount() (umount2(2) on Linux, unmount(2) on FreeBSD) and in cmd/zfs. MS_CRYPT is a compile-time macro, so consumers that set it (for example truenas_pylibzfs) must be rebuilt against the new header. Reviewed-by: Brian Behlendorf Signed-off-by: Ameer Hamza Closes #18713 * ZTS: ctime_001_pos increase tolerance The ctime_001_pos test checks that timestamp updates occur for a file after performing certain operations (read, write, chown, etc). The test case allowed for a +4 second tolerance in the timestamp value which is generous but up to +7 second discrepencies have been seen in the CI. Bump the tolerance to +10 seconds to prevent these false positives. As long as the value increases and is reasonably close to the expected value consider that to be sufficient. Reviewed-by: Christos Longros Reviewed-by: Alexander Motin Signed-off-by: Brian Behlendorf Closes #18733 * build: Fix for building dist target outside of project root Do not change the working directory when doing copying because $distdir is a path relative to the working directory, and thus not valid after changing the working directory. This previously worked, when configure was run from the project root, because @srcdir@ was the same as the build working directory. Reviewed-by: Brian Behlendorf Signed-off-by: Glenn Washburn Closes #18744 * Linux: fix zfs_write() infinite loop on unfaultable buffer On Linux, zfs_write() copies from the source buffer with page faults disabled while the transaction is open, relying on zfs_uio_prefaultpages() to make the pages resident beforehand. When dmu_write_uio_dbuf() returns EFAULT, the retry path only subtracts the bytes consumed so far from the prefault accounting. On zero progress pfbytes does not change, the "pfbytes < nbytes" check never triggers another prefault, and the loop retries the same failing copy. If the buffer can never be faulted in again, e.g. the owning process was torn down while a thread was in pwrite(2), that thread spins unkillably at 100% CPU holding the file range lock, blocking every other accessor of the file. This is a regression from commit b0cbc1aa9a ("Use big transactions for small recordsize writes."), which dropped the unconditional re-prefault the EFAULT path had carried since commit 779a6c0bf6 ("deadlock between mm_sem and tx assign in zfs_write() and page fault"). Restore those semantics by resetting pfbytes on EFAULT, as suggested in the issue analysis: the next iteration then faults the pages back in before retrying, and for a permanently inaccessible buffer zfs_uio_prefaultpages() fails, breaking the loop with EFAULT, which is already propagated to userspace. Transient faults, such as mmap'ed source pages evicted under memory pressure, retry as before. Built and tested on Linux aarch64: the ZTS mmap and write-path groups pass and a munmap-versus-write stress run leaves no stuck writers. The teardown race itself is not reproducible on demand, so the retry paths were also checked by inspection against the reproducers in the issue. Reviewed-by: Alexander Motin Reviewed-by: Brian Behlendorf Signed-off-by: MorganaFuture <103630661+MorganaFuture@users.noreply.github.com> Closes #17129 Closes #18740 * linux: batch DMU reads of non-resident pages in mappedread() When a range being read has at least one page in the page cache, zfs_read() routes the whole chunk through mappedread(), which falls back to a separate dmu_read_uio_dbuf() call for every non-resident PAGE_SIZE piece. Since cached pages outlive munmap(), a file which was mapped at some point may sit mostly outside the page cache and still pay this cost: one DMU call per 4K page instead of one per chunk, measured in #16031 as a 4-10x sequential read slowdown. Commit 39be46f43 ("Linux 5.18+ compat: Detect filemap_range_has_page") fixed the detection side so fully uncached chunks bypass mappedread() again, but a chunk holding even one resident page still degrades to page-sized DMU reads for everything else. Instead of issuing one DMU read per absent page, probe the page cache with find_get_page() and extend the read over the whole run of non-resident pages which follows, restoring chunk-sized DMU reads for the uncached parts of a mapped file. A page can be faulted in after it was observed absent and before the DMU read covering it completes, but this is safe for the same reason the existing single-page window is. zfs_read() holds the znode rangelock as reader across mappedread(), so the DMU contents of the range are stable: zfs_write(), zfs_putpage() and truncation all require the writer lock, and zfs_getpage() fills concurrently faulted pages from those same contents. A faulted page can only diverge from the DMU once dirtied through a writable mapping, making that store concurrent with this read, for which returning the pre-store data is a valid outcome. Stores which completed before the read began cannot be missed: a dirty page cannot be cleaned and reclaimed while the reader lock is held (writeback takes the writer lock), so it is still found resident, or zfs_putpage() already copied its data into the DMU. FreeBSD's mappedread() has the same per-page fallback and could be batched the same way in a follow-up. Measured in a VM with a 1 GiB file held in the ARC and read sequentially with dd: one resident page per 1 MiB read request degrades throughput from ~11.4 GB/s (no resident pages) to ~5.7 GB/s on the baseline, and this change restores ~11.4 GB/s; one resident page per 32 MiB chunk, read in 32 MiB requests, improves from ~5.4 GB/s to ~7.3 GB/s. Reads of a fully resident file are unaffected (~20 GB/s before and after). All tests in the ZTS mmap group pass, including the mmap_read and mmap_seek cases. Reviewed-by: Brian Behlendorf Signed-off-by: MorganaFuture <103630661+MorganaFuture@users.noreply.github.com> Closes #16031 Closes #18741 * Add SECURITY.md policy file Add a basic SECURITY.md file to establish the repository's security reporting policy. Includes guidance for reporting security issues and what to expect. Reviewed-by: Allan Jude Reviewed-by: George Melikov Signed-off-by: Brian Behlendorf Closes #18766 * Fix receive -x according to comment The old condition skipped the -x for ANY property whose source wasn't explicitly ZPROP_SOURCE_VAL_RECVD - which caught inherited/default properties too, not just locally-set ones. The new condition correctly skips only when the property is locally-set on the destination (source == fsname), which is the documented intent. Reviewed-by: Brian Behlendorf Signed-off-by: Richard Kojedzinszky Closes #18737 Closes #18738 * ZTS: migration/setup: clear stale zfs_member label before new_fs During a full ZTS run functional/migration/setup fails intermittently when it mounts the non-ZFS device. That device is often one an earlier test used as a pool vdev. 'zpool destroy' leaves the vdev labels in place and new_fs only overwrites the front of the device, so the trailing labels can survive. libblkid then probes the device as ambiguous (both the new filesystem and zfs_member) and the auto-detecting mount refuses, which setup reports as a spurious failure. Wipe any residual signatures with wipefs before laying down the new filesystem so the device carries a single, unambiguous type, and let udev settle before the mount. Skip the wipe in the single-disk case, where the non-ZFS device is the same one the test pool was just created on, so the live pool is left untouched. Verified on Linux: after a pool create and destroy the scratch device still carries a zfs_member label (blkid -p reports zfs_member); a wipefs -a removes it so the following new_fs is the only signature and the mount succeeds. The functional/migration group passes with the change. Reviewed-by: Brian Behlendorf Signed-off-by: MorganaFuture <103630661+MorganaFuture@users.noreply.github.com> Closes #18492 Closes #18753 * zpl_inode: remove zpl_rename no-flags variants Removed in 4.9. Sponsored-by: TrueNAS Reviewed-by: Brian Behlendorf Signed-off-by: Rob Norris Closes #18769 * CI: Fix race caused by shared ctr file updates CTR is shared between the VMs and used as a global counter. This uncoordinated shared access can result in a CI failure due to the racing updates. From the log: `qemu-6-tests.sh: line 27: 1` `6: syntax error in expression (error token is "6")` Resolve the issue by using separate ctr files by appending the ID. The output now prints the total test cases count along with each VMs individual count. Reviewed-by: Tino Reichardt Reviewed-by: Brian Behlendorf Signed-off-by: tiehexue Closes #18778 * Remove libuutil from the pull request template and fix headings - libuutil was removed in adb316f41. - Normalize section headers to title case. - Drop the trailing colon on "Checklist". Reviewed-by: Brian Behlendorf Reviewed-by: George Melikov Signed-off-by: Alexander Moch Closes #18791 * CI: Update Alpine Linux runner to 3.24.1 Update the Alpine Linux CI runner from 3.23.2 to 3.24.1. This refreshes the runner to the latest Alpine release while keeping the existing CI configuration unchanged. Reviewed-by: Brian Behlendorf Signed-off-by: Alexander Moch Closes #18790 * Rate limit Direct I/O verify zevents Each vdev initializes a vdev_dio_verify_rl rate limiter (governed by zfs_dio_write_verify_events_per_second), but zio_dio_chksum_verify_error_report() never consults it, so dio_verify_rd and dio_verify_wr zevents are posted with no rate limiting. A workload that repeatedly trips the Direct I/O verify can therefore produce an unbounded flood of zevents. Gate both ereport posts through zfs_ratelimit(&vd->vdev_dio_verify_rl), as is already done for the other per-vdev ereports (checksum, delay, deadman). The vs_dio_verify_errors vdev stat still increments on every event, so the true count remains observable via zpool status -d. The dio_write_verify test checks on every iteration that a dio_verify_wr zevent was posted. With rate limiting now in effect the shared limiter window is exhausted after the first iteration, so later iterations observe zero events and the test fails. Raise zfs_dio_write_verify_events_per_second for the duration of that test (restored in cleanup), adding the matching tunables.cfg alias, so the events stay observable. Reviewed-by: Brian Behlendorf Signed-off-by: Michael Heller Closes #18795 * Fix reads for blocks freed after being cloned PR #18421 fixed a case when reads for blocks cloned after being freed could return zeroes. But it created an opposite problem, when reads for blocks freed after being cloned could return non- zero content from the cloning. This patch fixes the problem by creating a more specialized form of dnode_block_freed(), taking into account the TXG when the cloning has happened and checking frees only in TXGs after. Reviewed-by: Brian Behlendorf Reviewed-by: Gary Guo Signed-off-by: Alexander Motin Closes #18421 Closes #18724 * libzfs: fallback VDEV_UPATH to VDEV_PATH for non-DM devices When zfs_get_underlying_path() returns NULL for a non-DM device (e.g. NVMe), the zfs_prepare_disk script was getting an empty VDEV_UPATH. Per the man page, VDEV_UPATH should fall back to VDEV_PATH when there is no underlying path. Reviewed-by: Brian Behlendorf Signed-off-by: MISAPOR LAB Closes #18439 Closes #18802 * L2ARC: bound the rebuild by the write hand on a first sweep l2arc_log_blkptr_valid() ends with (!evicted || dev->l2ad_first), which disables the eviction-overlap test entirely on a first sweep. That test is meaningless in that state, since l2arc_evict() returns immediately and l2ad_evict never advances off l2ad_start, but dropping it leaves the log block bounded only by device geometry. On a first sweep only the region below the write hand has been written, so a block at or beyond l2ad_hand describes data this incarnation never wrote. l2arc_hdr_restore() performs no per-entry validation, so every such entry inflates arcstat_l2_psize and, via vdev_space_update(), the cache vdev's vs_alloc. Nothing reconciles it because l2arc_evict() never runs on a first sweep, and once vs_alloc exceeds vs_space the unclamped subtraction in zpool(8) wraps and the device reports 16.0E free. Stale entries also let the L2ARC read offsets that were never written, which then fail checksum verification. Removing and re-adding a cache vdev is enough to set this up: l2arc_add_vdev() resets l2ad_hand to l2ad_start while the previous incarnation's log blocks remain higher up the device, and the backward walk then crosses l2ad_start and wraps into them. Bound the first-sweep case by the write hand instead of disabling the check, and state the geometry conditions once rather than duplicating them across both branches. The 16.0E symptom has been reported since 2015. #10224 carries the only prior analysis, which suspected a leak in l2arc_evict() and was closed incidentally by #9789 rather than by a fix. It is also open on FreeBSD as PR 250323. External-issue: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=250323 Reviewed-by: Ameer Hamza Reviewed-by: Alexander Motin Signed-off-by: Nick Price Closes #3114 Closes #3400 Closes #5583 Closes #10224 Closes #12779 Closes #18827 * zdb: output refcounts from verify_spacemap_refcounts() Output information about refcounts when there is refcount mismatch. Use plain uint64_t even as the values are expected to not be large. Also use unsigned as we should never get negative refcounts there. Reviewed-by: Allan Jude Reviewed-by: Brian Behlendorf Reviewed-by: Alexander Motin Signed-off-by: Toomas Soome Closes #18809 * mmp: skip non-writeable vdevs during activity check The import-time MMP activity check added by c710f8792 writes an uberblock to each top-level vdev and requires a matching number of good writes before the pool is claimed. Two vdev types that carry no writeable device break this: - A hole vdev (left by removing a log) and an indirect vdev (left by removing a data device) are counted in the required-write total but can never be written, so good_writes never reaches req_writes. The activity check then spuriously fails and the pool is reported as held by another host with hostid 0. - mmp_claim_uberblock() also issues a zio_flush() to the root vdev after the writes. zio_flush() recurses to every leaf, and an indirect vdev is a childless top-level vdev, so it is issued a ZIO_TYPE_FLUSH. That trips the ZIO_TYPE_WRITE assertion in vdev_indirect_io_start() and panics. Skip hole and indirect vdevs when counting required writes, and skip non-concrete vdevs in zio_flush() as they have no device to flush. Add hole, indirect, log, cache, and spare vdevs to the pool used by the mmp_inactive_import, mmp_exported_import, and mmp_concurrent_import tests so the activity check exercises these vdev types. The enriched layout is opt-in, leaving multihost_history on the simple two-device pool it relies on. Reviewed-by: Brian Behlendorf Signed-off-by: Michael Heller Closes #18823 Closes #18835 * libspl: Implement VERIFY_IMPLY and VERIFY_EQUIV The libspl debug header is missing VERIFY_IMPLY and VERIFY_EQUIV macros and instead directly implements IMPLY and EQUIV. Break out the VERIFY definitions to match the kernel macros and facilitate code sharing between kernel and userland. Sponsored-by: Cybersecure Pty Ltd Reviewed-by: Brian Behlendorf Reviewed-by: Alexander Motin Signed-off-by: Ryan Moeller Closes #18822 * cstyle: better tolerance for struct literals `cstyle.pl` currently doesn't have much patience for code such as: ``` *myvar = (mystruct_t) { .ms_field = 42, .ms_other_field = "chow time" }; ``` The first line is a Catch-22. If there's a space before the curly brace, then it's an illegal cast because of the trailing space. If there isn't a space, then it's an illegal curly brace without a preceding space. Either way, tagging the first line as `/* CSTYLED */` gets you nowhere because `cstyle.pl` doesn't understand the structure. It sees the fields as continuation lines and complains about the indentation. This PR makes three changes: - It allows the first line with a space between the type and the brace. - It adds first lines of this type to the same category as structs, enums, and unions. Indentation is tracked, but no particular style of indentation is enforced. - It modifies a few clauses in `lib/libefi/rdwr_efi.c` that used to squeak through `cstyle.pl` but are now (correctly?) detected. These are of the form `(int) sizeof (type_t)`. I've changed these to `(int)(sizeof (type_t))`. Easy to reverse if the original form is in fact preferred. Reviewed-by: Brian Behlendorf Signed-off-by: Garth Snyder Closes: #18839 * DDT: Fix several bugs in pruning - Fix variables types to avoid overflows after 2B entries. - Make ddt_prune_walk() code some more symmetrical. - Fix zero oldest on exact target to histogram value match. - Make bin 0 properly start from 0, not 1 hour. - Take as a cutoff base a time of histogram build start. Reviewed-by: Brian Behlendorf Signed-off-by: Alexander Motin Closes #18838 * dmu_recv: Avoid potential null deref Compilers are smart enough to deref only if the first && operand is true, so this is mostly to avoid false positives from sanitizers. Sponsored-by: Klara, Inc. Sponsored-by: Wasabi Technology, Inc. Reviewed-by: Brian Behlendorf Reviewed-by: Alexander Motin Signed-off-by: Igor Ostapenko Closes #18848 * ZTS: make file_check actually compare the resume test results file_check guards every comparison with a check that the snapshot directory exists on both sides, and the resume tests receive with -u, so the receive side is never mounted and the .zfs snapshot paths never exist. The function has been quietly comparing nothing in rsend_019-022, rsend_024, rsend_030 and send-c_resume, so the resume test family verified that receives succeed but not that the received data matches. Mount both sides before diffing (some tests also unmount the send side), still compare only the snapshots both sides carry since several tests send just one of them, and fail loudly when nothing at all was compared so the check cannot rot back into a no-op. Two callers needed their expectations fixed once the checks came alive: rsend_024 streams from the head rather than a snapshot, so it now diffs the mounted heads directly, and the first file_check in send_partial_dataset pointed at a partial dataset with no snapshots, so it now compares against the dataset the stream came from. Tests: rsend group passes with the comparisons active, twice in a row on one module load. Reviewed-by: Brian Behlendorf Signed-off-by: MorganaFuture <103630661+MorganaFuture@users.noreply.github.com> Closes #18834 * zed: let autoexpand see capacity changes on partitioned disks Growing a disk under a whole-disk vdev never triggers autoexpand (#12505). The kernel reports a capacity change on the disk itself and nothing for the partitions, whose sizes did not change. But since zfs owns the whole disk it carries a partition table, and zed_udev_monitor() drops any disk-with-partitions event on the assumption that a partition event will follow. For a resize none ever does, so the ESC_DEV_DLE event that zfsdle_vdev_online() needs is never generated and the pool stays at the old size until someone runs zpool online -e by hand. This is the common case for cloud disks grown online. Pass change events through when udev marks them RESIZE=1. On the matching side a disk-level event has no vdev guid to search by (the label lives on the partition), and udev provides no ID_PATH on some buses, so the physical path lookup can also come up empty. When that happens, read the ZFS label off the whole-disk partition and match by the pool and vdev guids stored in it. Unlike matching the config path textually, this works no matter which name the pool was imported with (by-id, by-path or a bare device node), and a stale device path in an unrelated pool's config cannot steal the event, since the label names the owning pool. The fallback only runs for guid-less disk events and only accepts a whole-disk vdev that is not a spare or l2cache device. The new zpool_expand_006_pos test covers this end to end on a scsi_debug disk. zpool_expand_001_pos already grows a scsi_debug disk the same way but keeps passing on an unpatched zed, because block_device_wait issues a bare udevadm trigger, which re-sends change events for the zfs_member partitions and hands zed the vdev guid the resize itself never delivered. The new test drains the pool-creation udev traffic and then only settles, so zed sees what a production resize generates: one RESIZE=1 change event on the disk. Multipath maps take a different path through zed_udev_monitor() and still need the manual online; that is unchanged here, and the same goes for other device-mapper vdevs, whose partitions live on separate dm nodes the fallback cannot derive from the map's name. Disks with no devid source at all (virtio-blk, Xen) also stay out of scope: their events are dropped earlier for lack of any device identifier, and widening that is its own discussion. Tests: zpool_expand_006_pos fails against unpatched zed and passes with the fix; a pool imported by /dev/disk/by-id expands from the bare disk event with "matched vdev ... by the label" in the zed log; the zpool_expand group passes. Reviewed-by: Brian Behlendorf Signed-off-by: MorganaFuture <103630661+MorganaFuture@users.noreply.github.com> Closes #12505 Closes #18833 * zfs: fix stale POSIX ACL cache after rollback An online zfs rollback rezgets live znodes and clears the OpenZFS ACL cache, but leaves the Linux VFS inode POSIX ACL cache intact. A later non-root permission check can use an ACL added after the snapshot. Reproduce by snapshotting a POSIX ACL file whose group mode bits require a VFS ACL check, granting a named user read access, holding the inode active, and rolling the mounted filesystem back. The named user remains able to read until cache eviction. Invalidate both the access and default VFS POSIX ACL caches from zfs_rezget(), alongside the existing private cache invalidation, so subsequent permission checks reload the recovered on-disk ACL. Reviewed-by: Brian Behlendorf Signed-off-by: Wang Zhaolong Closes #18837 * Add missing checks to zfs_clone_range_replay() zfs_clone_range() does a few error checks that we are missing in zfs_clone_range_replay(). Reported-by: Grok 4.5 Build Beta Reviewed-by: Brian Behlendorf Reviewed-by: Alexander Motin Signed-off-by: Richard Yao Closes #18867 * libzfs: Do not call munmap() when mmap() fails Reported-by: Grok 4.5 Build Beta Reviewed-by: Igor Kozhukhov Reviewed-by: Rob Norris Reviewed-by: Brian Behlendorf Reviewed-by: Alexander Motin Signed-off-by: Richard Yao Closes #18869 * libzfs: don't truncate a resolved vdev path in zpool_vdev_name() zpool_vdev_name() copied the result of realpath() into a 64 byte stack buffer shared with the short formatted names, so a vdev whose resolved path was longer than 63 bytes was reported cut short by zpool status -L and anything else asking for VDEV_NAME_FOLLOW_LINKS. The cut is made at a byte boundary, so a multi-byte character straddling it is left as invalid UTF-8, which also makes zpool status -j -L emit JSON a parser rejects. Resolve straight into a buffer of the right size. realpath() fills a caller supplied buffer of at least PATH_MAX, as it is used elsewhere in libzutil, which also removes the intermediate allocation. The other three users of that buffer format a guid, a raidz name, or a draid name, and none of them can exceed its length. Reviewed-by: Brian Behlendorf Signed-off-by: Michael Heller Closes #18851 Closes #18871 * libzfs: String trimming should not operate out of bounds Forward slashes are trimmed from ZPOOL_IMPORT_PATH, but if someone sets a ZPOOL_IMPORT_PATH that is only forward slashes, our trim code will underflow the string, causing an out of bounds operation. Similarly, the SMB code could potentially trim a string consisting of only new line characters until it experiences the same bug. The same fix is applied to it. These are memory bugs, but I suspect that it is very unlikely that they would cause a problem, since the probability that an out of bounds write would follow the out of bounds read should be low. That said, going out of bounds is undefined behavior, which could cause incorrect code generation should a compiler look at it wrong, so let us fix this. Reported-by: Grok 4.5 Build Beta Signed-off-by: Richard Yao Reviewed-by: Rob Norris Reviewed-by: Alexander Motin Reviewed-by: Brian Behlendorf Closes #18868 * mmp: do not require writes to mirror legs the config marks absent The MMP uberblock claim requires one good write per configured leaf of each top-level vdev. For a mirror it required two writes unconditionally (MIN(MAX(children, 1), 2)), so a mirror with a leg that is persistently offline, faulted, or removed could produce only one good write and the activity-check claim failed with EIO. A degraded mirror could therefore not be imported with multihost=on, blocking HA failover. Count only the legs the pool config still expects to be present, and require a write to every one of them. A leg taken out of service is recorded persistently in the config and is seen the same way by every host, so it is not required. A leg merely unreachable from the importing host keeps none of those states and stays required, so a host that can see only some of the legs of an otherwise healthy mirror still fails the claim and cannot split the pool. The previous cap of two writes was a compromise made because requiring every child was too strict for wide mirrors, in particular where a leg is left offline for long periods as part of a backup strategy. Consulting the config covers that case directly, so the cap is no longer needed: on a three-way mirror with one leg unreachable and not marked absent, a cap of two would accept the claim while another host holding the third leg could accept it as well. Reviewed-by: Brian Behlendorf Signed-off-by: Michael Heller Closes #18855 * ZTS: add coverage for the MMP claim on a degraded mirror Add mmp_degraded_import, which verifies the uberblock claim requires a write only to those mirror legs the pool configuration still expects to be present. A healthy mirror is claimed, a mirror with an offlined leg is claimed and imports degraded, and a mirror with a leg this host cannot open, and which the configuration does not mark absent, is refused. The degraded and unreachable cases repeat on a three-way mirror, where the number of legs the configuration expects and the number this host can reach come apart. Reviewed-by: Brian Behlendorf Signed-off-by: Michael Heller Closes #18855 * libzfs: don't read a dataset handle after closing it in resume send zfs_send_resume_impl_cb_impl() closes the dataset handle before its error switch and then reads zhp->zfs_name again in the ESRCH case. zfs_name is an array declared inside struct zfs_handle, so the free() at the end of zfs_close() releases it along with the handle, and lzc_exists() copies out of the freed block. The close dates from the original resume send. The ESRCH case was added three years later with redacted send, below a handle that was no longer live. The path is reachable: dsl_bookmark_lookup() returns ESRCH when the incremental source is a bookmark that has gone away, which a resume can lose a race with. Keep a copy of the name alongside the error message that is already formatted before the close, and test that instead. Reported-by: RigelYoung <43904538+RigelYoung@users.noreply.github.com> Reviewed-by: Rob Norris Reviewed-by: Alexander Motin Reviewed-by: Brian Behlendorf Signed-off-by: Michael Heller Closes #18870 Closes #18883 * man: zvol_request_sync is not ignored under blk-mq The zfs.4 entry for zvol_request_sync claims it "is ignored when running on a kernel that supports block multiqueue (blk-mq)". This has never been true. The sentence and the blk-mq support it describes landed in the same commit (6f73d0216 "zvol: Support blk-mq for better performance"), which added if (zvol_request_sync) force_sync = 1; to zvol_request_impl() -- the function shared by both submission paths, reached from zvol_submit_bio() and from zvol_mq_queue_rq() alike. No blk-mq guard was added there then, and none exists now. Taken literally the sentence is worse than inaccurate: HAVE_BLK_MQ was removed in 9601eeea1 because every supported kernel has blk-mq, so the documented condition is always satisfied and the parameter would never do anything. Verified on 6.12.101 by counting call sites with kprobes during an fio run. zvol_write() is reached either via the taskq, through zvol_write_task(), or directly, so the two are a clean discriminator: config zvol_write zvol_write_task zvol_tq CPU bio, default 1477413 1477413 7902 jiffies bio, sync=1 3873491 0 0 blk-mq, default 2787187 2787187 7948 jiffies blk-mq, sync=1 3816106 0 0 With zvol_request_sync=1 no task is ever dispatched and the zvol taskq threads get no CPU, on either path. Replace the sentence with an accurate one. Reviewed-by: Brian Behlendorf Signed-off-by: George Melikov Closes #18887 * CI: publish the per-VM test counter atomically Each VM's log reader keeps a running test count in /tmp/ctr-vm$ID and reads every other VM's counter to print the combined progress figure. The counter is published with a plain redirect, which truncates the file before it writes, so a reader can see it empty. `read` then returns 1, and because the reader inherits set -eu that ends the reader subshell. The VM keeps running and its results are recovered later from the artifact, but its output stops being prefixed into the live log from that point on, with nothing said about why. Seen on fedora44 in https://github.com/openzfs/zfs/actions/runs/30772421272. vm2's last prefixed line is refreserv/cleanup at 01:57, carrying counter 745, while vm1 keeps reporting vm2 frozen at 746 for the remaining 38 minutes: the reader incremented the counter and died before printing that line. vm2 itself ran on until 02:14 and its 899/11/6 summary never reached the live log. Publish through a temporary file and rename instead. A reader then sees either the old value or the new one. Racing a writer against a reader 20000 times reproduces 10805 failed reads with the redirect and none with the rename. Reviewed-by: Brian Behlendorf Signed-off-by: Michael Heller Closes #18885 * CI: don't fail a passing job when a log reader has already exited Once a VM's tests finish the runner kills that VM's log reader. If the reader is already gone, kill reports ESRCH, and since the script runs under set -eu that ends it and the job is reported failed after the tests have already passed. That is what turned the fedora44 run in https://github.com/openzfs/zfs/actions/runs/30772421272 red: vm1: Results Summary vm1: PASS 1151 vm1: FAIL 2 vm1: SKIP 6 ... qemu-6-tests.sh: line 143: kill: (20700) - No such process ##[error]Process completed with exit code 1. All 13 failures in that run were on the expected list and neither VM reported an unexpected one, so no test result was affected. Only the exit status was wrong. The preceding commit removes the race that killed the reader, so this should no longer be reachable, but a reader can still die for reasons this script does not control and losing a whole run to the cleanup step is a poor trade. The message is left on stderr rather than discarded, because a reader exiting early means live output was lost and that is worth seeing. Reviewed-by: Brian Behlendorf Signed-off-by: Michael Heller Closes #18885 * build: Fix release detection when build dir is not source dir When building outside of the root source directory, configure fails to detect that the source for the build is a git repository because the build directory is checked if it is a git repository. Instead check source directory and use the source directory for generating the release. Check for the .nogitrelease file in the source directory too. Reviewed-by: Brian Behlendorf Signed-off-by: Glenn Washburn Closes #18891 * ZTS: don't read a command's exit status as a missing binary log_neg_expect() treats an exit status of 127 as a missing binary and fails before it looks at what the command printed. That is only a convention of the shell, and a command is free to return 127 for its own reasons. fio returns the number of jobs which failed, so a run of 127 failing jobs is reported as though fio were not installed. no_space/enospc_rm fills a pool with 200 fio jobs and requires them to fail with ENOSPC. How many of them get that far varies with timing, and on the occasions it comes to exactly 127 the test fails with fio ... unexpectedly exited 127 (File not found) even though the output holds the expected message. A dozen runs here landed between 183 and 199, and the failure seen in CI reported 127. Only read 127 as a missing binary when the expected output is absent. A command which printed what was asked of it plainly ran, so nothing which passed before can start failing. Reviewed-by: Brian Behlendorf Signed-off-by: Michael Heller Closes #18726 Closes #18882 * nvpair: Fix operator precedence 59dc88602e23a436440e4164c6d9401da8f0dff2 made a mistake when doing a check, which can cause us to continue processing when we should return EFAULT. Reported-by: Grok 4.5 Build Beta Reviewed-by: Alexander Motin Reviewed-by: Brian Behlendorf Signed-off-by: Richard Yao Closes #18874 * Linux 6.18 compat: vfs_parse_fs_string() takes 3 args Sponsored-by: TrueNAS Reviewed-by: Brian Behlendorf Signed-off-by: Rob Norris Closes #18847 * Linux 6.3: follow_down() gains flags arg We need the flags arg to trigger the snapshot mount. For earlier kernels, we can emulate it with vfs_path_lookup() Sponsored-by: TrueNAS Reviewed-by: Brian Behlendorf Signed-off-by: Rob Norris Closes #18847 * Linux 5.19/6.17: handle differences in how to flush delay workqueue Sponsored-by: TrueNAS Reviewed-by: Brian Behlendorf Signed-off-by: Rob Norris Closes #18847 * CodeQL: Flag implicit compare-then-assign in branch conditions Implicit compare-then-assign in branch conditions is buggy since developers often mean assign-then-compare, but sometimes actually mean compare-then-assign. GCC's -Wparentheses was originally meant to catch assignment in place of comparison, requiring an extra set of parentheses to turn this off. This had the happy coincidence of making developers explicit about assign-then-compare vs compare-then-assign. An outer level of extra parentheses will inhibit -Wparentheses warnings. This often results in assign-then-compare being made explicit, but instead of turning `if (x = foo() < 0)` into `if ((x = foo()) < 0)`, a developer might write `if ((x = foo() < 0))`, which turns off the warning, without fixing the problem. This happened in openzfs/zfs#18874. There are other potential variations, such as `if ((x = (foo()) < 0))`, which also suppresses GCC's warning, but fails to actually do anything since the intended explicit parentheses to specify compare-then-assign are around the right operand of the boolean operator, rather than around the boolean operator, yet we have the additional parentheses needed to silence GCC's -Wparentheses. In the `if ((x = (foo()) < 0))` case, the intent was to make compare-then-assign explicit, and a typo caused it to fail to become explicit. That is not a bug, but it makes it unclear what the developer intended, which is problematic in itself. This probably merits a bug report to GCC requesting a more intelligent diagnostic that will treat compare-then-assign differently from assignment in a branch condition. However, that is a slow process, this has already bitten us once and with CodeQL, we can add our own check to the PR process so that we catch other instances of this issue during review, rather than some time later. Given that assign-then-compare in branch conditions requires that parentheses be added in such a way that the compiler AST no longer contains an implicit compare-then-assign, we only need to check for an implicit compare-then-assign in order to implement this check. Although the likelihood of compound assignment being present in this bug pattern is low, the same logic follows, so the check also will catch this pattern on compound assignment. This check handles conditions in if, while, do, for, ?:, && and ||. switch statements are intentionally ignored, since using assign-then-compare in a switch statement would turn the switch statement into a if-else. That is pointless, so allowing an implicit compare-then-assign in switch statements is problem-free. Coincidentally, GCC's -Wparentheses does not apply to switch statements either. Finally, this considers all comparison operators, rather than just the < operator used in the examples in this commit message. The CodeQL check was written by Grok 4.5 Build Beta after several iterations of prompt engineering and follow-up prompts to give it corrections. It has also been subjected to a test suite of 19 true positives and 17 true negatives to verify its behavior. It successfully detected all true positives and fails to detect any true negatives. It has also been applied not only to the OpenZFS codebase, but also the Linux kernel and curl codebases, where it had zero detections. Related queries in CodeQL were also run against the test suite, but had zero detections. The query appears to be a well made query that has a very high signal-to-noise ratio. It might be worth submitting to upstream CodeQL for inclusion, but I would rather add it to our own repository so we can begin benefiting from it today. Assisted-by: Grok 4.5 Build Beta Reviewed-by: Brian Behlendorf Signed-off-by: Richard Yao Closes #18899 * nvpair: Improve native handling of unterminated strings This continues the work done in 59dc88602e23a436440e4164c6d9401da8f0dff2 and parallels what is already done for XDR encoding. Reported-by: Grok 4.5 Build Beta Reviewed-by: Brian Behlendorf Reviewed-by: Alek Pinchuk Signed-off-by: Richard Yao Closes #18876 * nvpair: i_get_value_size() string array handling tweak The strnlen() function needs to be given the length of the remaining region to behave as intended, but it was given the length of the total region on packed strings. Reported-by: Grok 4.5 Build Beta Reviewed-by: Brian Behlendorf Reviewed-by: Alek Pinchuk Signed-off-by: Richard Yao Closes #18877 * Linux 7.2 compat: META Update the META file to reflect compatibility with the 7.2 kernel. Reviewed-by: Brian Behlendorf Signed-off-by: Tony Hutter Closes #18941 * Fix ddtprune causing space leak In zio_ddt_free, if a pruned dde is still in ddt, it would do nothing and cause space leak. Reviewed-by: Rob Norris Reviewed-by: Brian Behlendorf Reviewed-by: Allan Jude Signed-off-by: Chunwei Chen Closes #17982 Closes #17983 * Make systemd-udev-settle optional for the import units systemd-udev-settle.service has been deprecated for years, recent systemd releases warn about it at boot, and distributions have begun shipping without it, which turns the hard Requires= in zfs-import-cache and zfs-import-scan into a broken import: a Requires= on a masked or removed unit keeps the service from ever starting. Issue #10891. Demote the dependency to Wants= and keep the After= ordering. Where the settle unit exists and completes, the boot is what it always was: Wants= pulls settle in, the import waits for it, and the device-symlink guarantee it provided is intact. Where it is masked or gone the wish is quietly dropped and the import runs anyway, which beats not importing at all. One deliberate behavior change: if settle itself fails, on a system so large that enumeration overruns its timeout, the old Requires= cancelled the import while the new units go ahead at the timeout mark with whatever has been enumerated by then. Settle-less boots lose the wait for the udev queue, so the import units gain two ordering edges in its place. After=systemd-udev-trigger.service makes sure the coldplug events are at least queued. After=systemd-modules-load.service closes a condition race the settle wait used to hide: both import units gate on ConditionPathIsDirectory=/sys/module/zfs, and without the multi-second settle delay that condition could be evaluated before modules-load.d had finished loading zfs.ko, silently skipping the import on an otherwise healthy boot. Beyond that, a device whose symlink appears a moment too late is only covered by the short zfs_vdev_open_timeout_ms open-retry window; waiting for exactly the devices a pool needs is what the per-pool import work (#18486) is shaped to solve, and this stays the minimal step that keeps settle-less systems importing today. The stray After=systemd-udev-settle lines in zfs-mount, zfs-mount@ and zfs-volume-wait were only ordering hints against a unit that may not exist, so they simply go away. Tests: on a systemd 259 VM with a cachefile pool, rebooted with the rendered unit: settle available, the boot pulls it in and the pool imports as before; settle masked, the boot comes up with no failed units and the pool still imports, where a masked settle previously kept zfs-import-cache from starting at all. Reviewed-by: Brian Behlendorf Signed-off-by: MorganaFuture <103630661+MorganaFuture@users.noreply.github.com> Issue #10891 Closes #18832 * zhack: add "mmp reclaim" to recover a pool stranded by MMP When a host fails together with the mirror legs attached to it, the surviving labels still describe those legs as present, so the MMP uberblock claim keeps demanding a write to every one of them and no later import can satisfy it. The pool cannot be imported by any host again. Add "zhack mmp reclaim", which imports once with the claim's required write count relaxed for the mirror legs this host cannot open, marks those leaves offline so that the ordinary imports which follow succeed, and exports. The relaxation is confined to userspace. mmp_claim_relaxed is declared under #ifndef _KERNEL, and module/Kbuild.in builds the module with -D_KERNEL, so the flag cannot exist in a kernel module. libzpool does not define _KERNEL and so gets the check. This follows the zfeature_checks_disable pattern zhack already uses around the same import, and is stronger, since that flag does exist in the kernel. Only the number of required writes changes. The write, the wait and the re-read of the activity check are untouched, so a competing host which shares any leg with this one is still detected and the import is refused. A live host whose legs are all invisible from here cannot be detected by any write-and-read scheme, so this stays a manual operation which assumes the peer has been fenced. Legs are forgiven only under a top-level mirror, which is where the relaxation lives, and exactly those legs are marked offline. A raidz or draid member is required as parity+1 in aggregate and never demanded individually, so an absent one does not raise the requirement and is left alone. Offline is used rather than removed because it persists unconditionally, is already excluded from the claim, and has "zpool online" as its inverse when the hardware returns. Reviewed-by: Brian Behlendorf Suggested-by: Brian Behlendorf Signed-off-by: Michael Heller Closes #18892 * ZTS: add coverage for zhack mmp reclaim Six scenarios: a stranded pool is recovered and the claim then accepts it, a live host sharing a leg is still refused, both top-level vdevs are counted after a recovery, a pool without multihost is left alone, a log vdev leg is not touched, and a raidz member is not touched. Every recovery assertion re-imports as a third hostid. zhack exports cleanly under its own hostid, so importing again as the same host takes the exported-and-matching-hostid path, skips the activity check entirely, and would leave the claim unexercised and the test vacuous. The assertions read req_writes and good_writes from the claim's own dbgmsg line, which 20176224e added. That is the only observable of the claim arithmetic, at the cost of coupling the test to a debug message this change does not control. mmp_pool_destroy() used a bare "pgrep zhack", which matches any process whose name merely contains zhack. A ksh script named mmp_zhack_reclaim.ksh has comm "mmp_zhack_recla", so the helper found the running test and killed it. Match the process name exactly. Reviewed-by: Brian Behlendorf Signed-off-by: Michael Heller Closes #18892 * mmp: tell a failed uberblock claim apart from remote activity When the claim could not write to every device the config expects present, spa_activity_check_claim() replaced the error from mmp_claim_uberblock() with EREMOTEIO, so an operator whose peer died together with its mirror legs was told another host holds the pool, which sends them looking for a host that is not there. Report the cause instead. A shortfall has two causes worth telling apart, so mmp_claim_uberblock() now counts the writes it issues alongside the ones that succeed. A leaf the config expects present but which cannot be written is never issued one, so too few issued means a device is absent, which persists across retries and is what "zhack mmp reclaim" recovers; that returns ENODEV. Enough issued but too few good means the writes reached present devices and failed, which a retry may clear; that stays EIO. The issued count is gated exactly as the good count is so the two describe the same set of leaves. Both get a case in spa_ld_activity_result() and both still return EREMOTEIO to userspace, as the ENXIO case already does, and the cause travels to userspace in ZPOOL_CONFIG_MMP_RESULT so zpool(8) can say which one it was and, for ENODEV, name the recovery. ZPOOL_CONFIG_MMP_STATE stays MMP_STATE_ACTIVE for both even though nothing is active. An older zpool(8) knows only the two existing states and reaches zfs_error_aux() with an uninitialized buffer for anything else, so the state is kept as one it understands. An older zpool(8) against this kernel therefore prints what it prints today, and a newer zpool(8) against an older kernel finds no cause reported and falls back to the same text. The paths where the claim genuinely detects another host still return EREMOTEIO and are unaffected. Also correct the comment above the write count, which still described the fixed two writes per mirror that 8cdd9b2b7 replaced with one write per leg the config expects present. mmp_degraded_import.ksh asserted on the old message in the two cases which are now ENODEV, and is updated with them. The zhack case asserts both directions, since a message that stops being emitted fails silently: the shortfall must be reported, and it must not be reported as another host holding the pool. Reviewed-by: Brian Behlendorf Signed-off-by: Michael Heller Closes #18892 * config: detect idmap method via generic_permission test Since the switch from implicit to explicit userns, and to idmap, happened right across the kernel in major releases, so it is enough to use a single test and apply the results everywhere. generic_permission() is a nice simple function with a simple interface, so useful for an unambiguous test. Sponsored-by: TrueNAS Reviewed-by: Brian Behlendorf Signed-off-by: Rob Norris Closes #18769 * ZTS: test secpolicy_sys_config correctly limits namespace access Sponsored-by: TrueNAS Reviewed-by: Brian Behlendorf Reviewed-by: Alexander Motin Signed-off-by: Rob Norris Closes #18959 * ZTS: test secpolicy_zinject correctly limits namespace access Sponsored-by: TrueNAS Reviewed-by: Brian Behlendorf Reviewed-by: Alexander Motin Signed-off-by: Rob Norris Closes #18959 * secpolicy_nfs: remove, not used Sponsored-by: TrueNAS Reviewed-by: Brian Behlendorf Reviewed-by: Alexander Motin Signed-off-by: Rob Norris Closes #18959 * secpolicy_zinject: only permit a global zone credential Sponsored-by: TrueNAS Reviewed-by: Brian Behlendorf Reviewed-by: Alexander Motin Signed-off-by: Rob Norris Closes #18959 * secpolicy_sys_config: only permit a global zone credential Sponsored-by: TrueNAS Reviewed-by: Brian Behlendorf Reviewed-by: Alexander Motin Signed-off-by: Rob Norris Closes #18959 * secpolicy_zfs: add a note about the power of CAP_SYS_ADMIN Sponsored-by: TrueNAS Reviewed-by: Brian Behlendorf Reviewed-by: Alexander Motin Signed-off-by: Rob Norris Closes #18959 * vdev_open: pass credential to check for permission to open device This commit adds a cred_t parameter to vdev_open() and threads it through to all the vdev_op_open callbacks. The default is CRED(), ie the credential of the calling task, which is usually some userspace control process calling ioctl(). To handle the parallel vdev open case, we take additional and additional reference to the cred for each task, and pass it down to vdev_open(). Sponsored-by: TrueNAS Reviewed-by: Brian Behlendorf Reviewed-by: Alexander Motin Signed-off-by: Rob Norris Closes #18960 * zfs_file_open: add cred arg, use it to check access If we're opening a file on behalf of the user, we need to ensure that that user actually has access to it. Add a credential parameter to zfs_file_open() and use it when opening the file. Existing callers use kcred for now to get the same behaviour as before. Sponsored-by: TrueNAS Reviewed-by: Brian Behlendorf Reviewed-by: Alexander Motin Signed-off-by: Rob Norris Closes #18960 * vdev_file: use calling cred to check for device access Sponsored-by: TrueNAS Reviewed-by: Brian Behlendorf Reviewed-by: Alexander Motin Signed-off-by: Rob Norris Closes #18960 * vdev_disk: use calling cred to check for device access bdev_file_open_by_path() does not do any kind of credential check on the given device path, so we need to do our own. We temporarily swap in the passed in credential as the task credential, then call kern_path() and inode_permission(), which together will ensure the credential can both see and access the given path. For the reopening case, we use the kernel credential. The idea here is that since the device was already open, we shouldn't fail to reopen just because the calling user can't see it, which would prevent device removal, offline, online, etc. Include some light reorganising in the error paths, since we might not always have a device handle to carry the current error. Sponsored-by: TrueNAS Reviewed-by: Brian Behlendorf Reviewed-by: Alexander Motin Signed-off-by: Rob Norris Closes #18960 * ZTS: device access tests Tests that zpool create, add, attach and import all properly enforce the restrictions on the calling user to access device nodes. Sponsored-by: TrueNAS Reviewed-by: Brian Behlendorf Reviewed-by: Alexander Motin Signed-off-by: Rob Norris Closes #18960 * [zfs-2.3.9] Add 'capsh' to commands.cfg Add missing 'capsh' to commands.cfg. It was included in master with 7839c4b5e1 but that was not backported to this branch. Signed-off-by: Tony Hutter * [zfs-2.3.9] Add workaround for device_access ZTS test Add workaround to get the device_access ZTS tests working on 2.3.9. Signed-off-by: Tony Hutter * Tag zfs-2.3.9 META file and changelog updated. Signed-off-by: Tony Hutter * vde…

zfs_send_resume_impl_cb_impl() closes the dataset handle before its error switch and then reads zhp->zfs_name again in the ESRCH case. zfs_name is an array declared inside struct zfs_handle, so the free() at the end of zfs_close() releases it along with the handle, and lzc_exists() copies out of the freed block. The close dates from the original resume send. The ESRCH case was added three years later with redacted send, below a handle that was no longer live. The path is reachable: dsl_bookmark_lookup() returns ESRCH when the incremental source is a bookmark that has gone away, which a resume can lose a race with. Keep a copy of the name alongside the error message that is already formatted before the close, and test that instead. Reported-by: RigelYoung <43904538+RigelYoung@users.noreply.github.com> Reviewed-by: Rob Norris Reviewed-by: Alexander Motin Reviewed-by: Brian Behlendorf Signed-off-by: Michael Heller Closes openzfs#18870 Closes openzfs#18883

robn approved these changes Aug 3, 2026

View reviewed changes

amotin approved these changes Aug 3, 2026

View reviewed changes

amotin added the Status: Accepted Ready to integrate (reviewed, tested) label Aug 3, 2026

behlendorf approved these changes Aug 4, 2026

View reviewed changes

behlendorf merged commit 3bd8cef into openzfs:master Aug 4, 2026
45 of 49 checks passed

Navigation Menu

Repository navigation

libzfs: don't read a dataset handle after closing it in resume send - #18883

libzfs: don't read a dataset handle after closing it in resume send#18883
behlendorf merged 1 commit into
openzfs:masterfrom
mkhllr:fix-18870-uaf

mkhllr commented Aug 2, 2026

Uh oh!

robn left a comment

Uh oh!

mkhllr commented Aug 3, 2026

Uh oh!

behlendorf commented Aug 4, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

mkhllr commented Aug 2, 2026

Motivation and Context

Description

How Has This Been Tested?

Types of Changes

Checklist

Separate note, not in this patch

Uh oh!

robn left a comment

Choose a reason for hiding this comment

Uh oh!

mkhllr commented Aug 3, 2026

Uh oh!

behlendorf commented Aug 4, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants