ffs: fix stale newblk lookup in flush_newblk_dep

[sobomax]

Summary

Summary

Fix a panic in flush_newblk_dep() caused by treating the soft-updates newblk hash as though a physical block number uniquely identified a dependency.

The panic was reported in PR 297976:

panic: flush_newblk_dep: Bad newblk

flush_newblk_dep() is called while flushing the body of a newly created directory. It obtains the directory's physical block number and calls newblk_lookup(). The existing code assumes that any dependency returned for that block must be a D_ALLOCDIRECT:

if (newblk->nb_list.wk_type != D_ALLOCDIRECT)
        panic("flush_newblk_dep: Bad newblk %p", newblk);

That uniqueness assumption is not valid. The hash can retain completed dependencies for an earlier allocation after the physical block has been freed and reused. In particular, newblk_find() deliberately ignores converted dependencies when called with DEPALLOC, allowing a new dependency for the same physical block to coexist with an older one.

Analysis of the crash dump showed the following sequence:

• /var/db/pkg/local.sqlite-wal initially had logical blocks 125 through 128 allocated at filesystem blocks 1340656 through 1340680.
• ffs_reallocblks() relocated this cluster to blocks 1393160 through 1393184.
• The live indirect pointer for WAL logical block 128 referenced block 1393184, proving that block 1340680 was no longer owned by the WAL.
• Block 1340680 was subsequently allocated as direct block zero of the newly created /usr/local/lib/X11 directory.
• The old WAL D_ALLOCINDIR for block 1340680 remained in an indirdep ir_completehd, waiting for the parent indirect-block allocation to become durable. This dependency no longer represented live ownership of block 1340680, but it remained in the newblk hash as required by the existing dependency lifecycle.

The panic was then triggered by a race in flush_pagedep_deps():

1. While holding the soft-updates lock, it observed MKDIR_BODY on the diradd for the new directory.
2. It dropped the lock to obtain the directory vnode with get_parent_vp().
3. During this interval, the directory body and its allocation completed. MKDIR_BODY was cleared, the directory's D_ALLOCDIRECT was removed, and the diradd moved to the pagedep's pending list.
4. Without revalidating that state, flush_pagedep_deps() entered flush_newblk_dep().
5. The generic block-number lookup found the retained WAL D_ALLOCINDIR, causing the type assertion to fire.

The crash dump captured the diradd in state ONWORKLIST | ATTACHED | COMPLETE | DEPCOMPLETE, with MKDIR_BODY already clear. It was also present on pd_pendinghd rather than the diraddhd being processed. The cylinder-group bitmap and live vnode pointers were consistent with block 1340680 belonging exclusively to the X11 directory. Thus, this was not a duplicate live allocation or an allocator bitmap inconsistency.

Solution

Change flush_newblk_dep() to search the block's hash bucket for the specific allocation dependency it intends to flush. A matching dependency must have all of the following properties:

• The same physical block number.
• Type D_ALLOCDIRECT.
• An inodedep belonging to the vnode's inode.
• An ad_offset matching the requested logical block.

If no such dependency exists, the allocation has already completed and the function follows its existing "dependency not found" path.

This preserves the important consistency checks. The change does not simply ignore an unexpected dependency type: unrelated or historical hash entries are excluded by identity, while flush_pagedep_deps() retains its existing postcondition check. If the same diradd remains active with MKDIR_BODY still set after the flush attempt, the existing MKDIR_BODY panic will still report a genuine failure to satisfy the directory dependency.

The hash traversal remains protected by the per-filesystem soft-updates lock, as was the previous newblk_lookup() call. It introduces no new lock transitions, object-lifetime requirements, or interface changes. The operation remains a traversal of one hash bucket and occurs only on this forced dependency-flush path.

Testing

• Built the complete UFS kernel module with -Werror.
• Verified the change with git diff --check.
• Evaluated the new matching logic against the captured crash dump. The relevant hash bucket contained only the historical WAL D_ALLOCINDIR, so the new lookup correctly reports that no matching X11 D_ALLOCDIRECT remains.
• Confirmed from the dump that the caller's MKDIR_BODY dependency had completed and the diradd had already moved to the pending list, making the existing not-found behavior the correct result.

PR: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=297976
Follow-up to: https://reviews.freebsd.org/D26136

[sobomax]

Some of the facts supporting this diagnosis:

• The FS in question is ephemeral, using swap-backed md(4), the FS is freshly created after the system has booted with newfs using default options (except of the -L label). This rules out pre-existing metadata inconsistency.
• Issue has been observed so far on two separate AWS instances, making memory corruption highly unlikely. Also at the time of crash both system has been up for less than an hour.
• in both cases pkg-static has been involved during package deployment, once in openat() and another one in the symlinkat().

[kib]

The new 'find' logic essentially reorders the processing of the newblk dependencies, and I am not sure that this is fine in this case (it is generally not).

IMO the correct solution for the race in flush_pagedep_deps() as described in the summary is to memoize the vnode returned by get_parent_vp(), and then restart the global loop. When you see the MKDIR_BODY dependency, and the inode number matches the memoized result from get_parent_vp(), you can process it without relocking sd_fslock, which closes the race. If the vp is NULL or does not match the inode number, you vput() it, call get_parent_vp(), and restart the loop.

[sobomax]

In D59356#1362256, @kib wrote:

The new 'find' logic essentially reorders the processing of the newblk dependencies, and I am not sure that this is fine in this case (it is generally not).

IMO the correct solution for the race in flush_pagedep_deps() as described in the summary is to memoize the vnode returned by get_parent_vp(), and then restart the global loop. When you see the MKDIR_BODY dependency, and the inode number matches the memoized result from get_parent_vp(), you can process it without relocking sd_fslock, which closes the race. If the vp is NULL or does not match the inode number, you vput() it, call get_parent_vp(), and restart the loop.

Thanks, I agree that scanning past the first matching newblk is not appropriate because it changes dependency-processing order.

I will rework flush_pagedep_deps() to retain the vnode returned by get_parent_vp(), reacquire sd_fslock, and restart dependency selection. The MKDIR_BODY operation will only proceed when the newly selected dependency's inode matches the retained vnode, avoiding use of the dap that was selected before get_parent_vp() dropped the lock.

One detail I want to confirm: flush_newblk_dep() also drops and reacquires sd_fslock internally around bwrite() and ffs_update(), then repeats newblk_lookup(). If the D_ALLOCDIRECT completes during one of those operations, the next lookup could expose the retained D_ALLOCINDIR in the same way. Should flush_newblk_dep() return to the outer dependency-selection loop after such an operation, or revalidate the associated MKDIR_BODY dependency after reacquiring sd_fslock?

[sobomax]

Rework to preserve dependencies as suggested by @kib

[kib]

In D59356#1362297, @sobomax wrote:

In D59356#1362256, @kib wrote:

The new 'find' logic essentially reorders the processing of the newblk dependencies, and I am not sure that this is fine in this case (it is generally not).

IMO the correct solution for the race in flush_pagedep_deps() as described in the summary is to memoize the vnode returned by get_parent_vp(), and then restart the global loop. When you see the MKDIR_BODY dependency, and the inode number matches the memoized result from get_parent_vp(), you can process it without relocking sd_fslock, which closes the race. If the vp is NULL or does not match the inode number, you vput() it, call get_parent_vp(), and restart the loop.

Thanks, I agree that scanning past the first matching newblk is not appropriate because it changes dependency-processing order.

I will rework flush_pagedep_deps() to retain the vnode returned by get_parent_vp(), reacquire sd_fslock, and restart dependency selection. The MKDIR_BODY operation will only proceed when the newly selected dependency's inode matches the retained vnode, avoiding use of the dap that was selected before get_parent_vp() dropped the lock.

One detail I want to confirm: flush_newblk_dep() also drops and reacquires sd_fslock internally around bwrite() and ffs_update(), then repeats newblk_lookup(). If the D_ALLOCDIRECT completes during one of those operations, the next lookup could expose the retained D_ALLOCINDIR in the same way. Should flush_newblk_dep() return to the outer dependency-selection loop after such an operation, or revalidate the associated MKDIR_BODY dependency after reacquiring sd_fslock?

I suspect that yes, any relock of the softdep lock should restart the lookup.

[kib]

From the first look, this is fine.
I believe the next step is to help Peter (pho@) to create the reproducer, and get the stress2 testing.
Also, please wait for Kirk' feedback.

[pho]

In D59356#1363214, @kib wrote:

From the first look, this is fine.
I believe the next step is to help Peter (pho@) to create the reproducer, and get the stress2 testing.
Also, please wait for Kirk' feedback.

That would be nice. I have been seeing the panic a few times over the years, but not after Feb 10 2021.

[sobomax]

In D59356#1363271, @pho wrote:

In D59356#1363214, @kib wrote:

From the first look, this is fine.
I believe the next step is to help Peter (pho@) to create the reproducer, and get the stress2 testing.
Also, please wait for Kirk' feedback.

That would be nice. I have been seeing the panic a few times over the years, but not after Feb 10 2021.

In our case it's swap-backed UFS+SU on a machine with a lot of RAM>=working set. As such I/O operation might and will complete fast exposing the race. However the failure only seen in one of 30-40 builds.