Fix double free when cloning a block with a pending free - #18853
Fix double free when cloning a block with a pending free#18853patrickdk77 wants to merge 2 commits into
Conversation
dmu_read_l0_bps() captures the source block pointers for a clone (FICLONE / copy_file_range). For a block with no dirty record it used db->db_blkptr directly, without checking dnode_block_freed(). A block with a pending free -- e.g. a truncate that has recorded the block in the dnode's free ranges but has not yet synced -- still has a valid on-disk block pointer, so it could be cloned. The clone then registers a BRT reference to a block that is about to be returned to the allocator, and the block is freed twice: once for the pending free, and again when the clone reference is later released. On a debug build this trips the metaslab double-free verifier in the free bpobj drain (panic: "segment already in tree"); on a release build the space map is silently corrupted. The buffered read path already guards against this: dbuf_read_hole() treats a block with a pending free as a hole via dnode_block_freed(). Apply the same check in dmu_read_l0_bps() and return EAGAIN when the source block has a pending free. zfs_clone_range() already handles EAGAIN by waiting for the next TXG and retrying (or falling back to a copy), by which point the free has synced and the block reads as a hole. Reproduced with fstests generic/733, which panicked within a minute on a debug build and now runs cleanly across repeated runs; block cloning and BRT space accounting are unaffected (clone data compares identical, bclonesaved is correct, scrub is clean). Signed-off-by: Patrick Domack <patrickdk@patrickdk.com> Closes openzfs#18842
|
Nice work tracking this one down, and thanks for the detailed report on #18842 I'd been chasing the same issue from your repro and had ended up at the same It's necessarily probabilistic: the clone has to be registered in a later txg Two things in the diff I'd look at. The override branch isn't covered. When the head dirty record is a brtwrite the The other is holes. The check sits inside |
|
I was hyper focused on only fixing the panic I observed and didn't branch out On the fstests sweep across pool configs, that was reuse rather than foresight: |
f74a384ae2 made dmu_read_l0_bps() return EAGAIN for a source block with a pending free, so a clone cannot add a BRT reference to a block about to be freed. Review on openzfs#18853 pointed out two cases that check missed; handle both the way dbuf_read_hole() already does. - A hole (or absent BP) has nothing to free. The check ran unconditionally, so a hole lying inside a pending free range returned EAGAIN where the block should just be cloned as a hole. dnode_free_range() records a range whether or not the blocks in it are holes, so this also covers a block never written inside a pending punch range. With zfs_bclone_wait_dirty=1 that costs an extra txg wait and resolves on the retry; with it unset copy_file_range falls back to a plain copy, but FICLONE has nowhere to fall back to and fails outright where master cloned the hole. Guard the check with !BP_IS_HOLE(bp) and let a NULL bp reach the existing hole handling below. - When the head dirty record already overrode the BP (dr_brtwrite -- cloning a clone made in the same txg), the overridden pointer was returned with no check at all. A punch landing in a txg after a still-unsynced clone leaves that override in place: dbuf_free_range() only unoverrides records from its own txg and skips cloned dbufs as DB_NOFILL, so cloning the clone captures the overridden pointer with the newer free pending against it and double frees it through that branch. Only frees after the override count against it, so use dnode_block_freed_after(dn, db->db_blkid, dr->dr_txg). Verified on a debug build with fstests generic/733 and with a FICLONE of a sparse file in a pending free range, which fails before this change (EAGAIN) and succeeds after; block cloning and BRT space accounting are unaffected. Suggested-by: mkhllr Refs: openzfs#18853 Signed-off-by: Patrick Domack <patrickdk@patrickdk.com>
f74a384ae2 made dmu_read_l0_bps() return EAGAIN for a source block with a pending free, so a clone cannot add a BRT reference to a block about to be freed. Review on openzfs#18853 pointed out two cases that check missed; handle both the way dbuf_read_hole() already does. - A hole (or absent BP) has nothing to free. The check ran unconditionally, so a hole lying inside a pending free range returned EAGAIN where the block should just be cloned as a hole. dnode_free_range() records a range whether or not the blocks in it are holes, so this also covers a block never written inside a pending punch range. With zfs_bclone_wait_dirty=1 that costs an extra txg wait and resolves on the retry; with it unset copy_file_range falls back to a plain copy, but FICLONE has nowhere to fall back to and fails outright where master cloned the hole. Guard the check with !BP_IS_HOLE(bp) and let a NULL bp reach the existing hole handling below. - When the head dirty record already overrode the BP (dr_brtwrite, cloning a clone made in the same txg), the overridden pointer was returned with no check at all. A punch landing in a txg after a still-unsynced clone leaves that override in place: dbuf_free_range() only unoverrides records from its own txg and skips cloned dbufs as DB_NOFILL, so cloning the clone captures the overridden pointer with the newer free pending against it and double frees it through that branch. Only frees after the override count against it, so use dnode_block_freed_after(dn, db->db_blkid, dr->dr_txg). Verified on a debug build with fstests generic/733 and with a FICLONE of a sparse file in a pending free range, which fails before this change (EAGAIN) and succeeds after; block cloning and BRT space accounting are unaffected. Suggested-by: mkhllr Refs: openzfs#18853 Signed-off-by: Patrick Domack <patrickdk@patrickdk.com>
The reasons in this file had accumulated from one-off observations, some of them stale and at least one simply wrong. Replace them with verdicts from a full -g auto run on five pool topologies (single disk at ashift 9 and 12, a 3-disk stripe, a 2-way mirror, raidz1) made with only the five block-layer cases excluded, so every test previously listed here has a result from all five. State what the list is calibrated against, which was missing before. Our branch carries 34 commits that are not in upstream OpenZFS, and only one of them has been submitted (the block cloning double free fix behind generic/733, openzfs/zfs#18853). Tests that are absent from this file because they now pass may well fail on stock OpenZFS, so the header says so rather than letting the omissions read as upstream behaviour. Drop fifteen entries the run showed are no longer needed here: 013, 139, 224, 269, 310, 476, 591 and 650 pass on all five. 013, 310, 476 and 650 were gated for a rangelock livelock and an ARC OOM panic, neither of which reproduces. 269 passes only because of our local writepage error commit, and 139 exercises direct I/O over cloned blocks that several local commits touch; both notes say so. 118, 119, 144, 517, 563, 616 and 617 self-skip on all five. Those capability checks are upstream, so these removals do not depend on our commits. Correct the dm-error group. The note claimed 266, 281, 338 and 475 needed pool redundancy, but _zfs_dmerror_init builds the pool on a single dm device whatever the topology, so redundancy never applies and all five wedge identically. Record what 083 actually does: it passes on the four 4-CPU topologies, and only with our writepage commit applied, but panics raidz1 (6 CPUs, 6 GiB) with "System is deadlocked on memory", reproduced twice. It stays excluded, described as a memory deadlock a zfs_arc_max cap avoids rather than the vague OOM it was filed as. Nothing was added. generic/271 and 272 fail on all five in the same way as the sync-write-error group but are left running, with a note saying so, rather than hidden behind a new entry. 27 entries remain, down from 42. Signed-off-by: Patrick Domack <patrickdk@patrickdk.com>
3 participants
Motivation and Context
generic/733reliably panics an--enable-debugbuild within a minute: aBRT-cloned block is double-freed, tripping the metaslab verifier in the
free-bpobj drain (
panic: segment already in tree, viametaslab_check_free<-zio_free_sync<-dsl_scan_free_block_cb). On arelease build this silently corrupts the space map, and the damage persists
on disk. Reproduced on unmodified master
d98fa72ca05a.Closes #18842
Description
dmu_read_l0_bps()captured a clone's source block pointers fromdb->db_blkptrwithout checkingdnode_block_freed(). A block with apending free (a truncate/hole-punch not yet synced) still has a valid BP,
so it could be cloned -- registering a BRT reference to a DVA that is then
returned to the allocator and freed a second time. Deferring the free
doesn't help: the free bpobj drains in the same TXG, before the clone's
brt_pending_apply(), so the fix is on the clone side.Mirror the existing
dbuf_read_hole()guard: returnEAGAINfromdmu_read_l0_bps()when the source block has a pending free.zfs_clone_range()already retries on the next TXG, by which point theblock reads as a hole. One file,
module/zfs/dmu.c(+24).How Has This Been Tested?
generic/733on a debug build: panicked within a minute before, cleanacross repeated runs after.
block_cloning/bclone) and full fstests-g autoacross single-disk (ashift 9/12), stripe, and raidz1 -- no newfailures.
bclonesavedcorrect, scrub clean.Types of changes
Checklist: