If unionfs_domount() fails, the mount path will not call VFS_UNMOUNT()
to clean up after it. If this failure happens during upper vnode
registration, the unionfs root vnode will already be allocated.
vflush() it in order to prevent the vnode from being leaked and the
subsequent vfs_mount_destroy() call from getting stuck waiting for
the mountpoint reference count to drain.
Reviewed by: kib, markj
Tested by: pho
Differential Revision: https://reviews.freebsd.org/D39767
If upperrootvp is doomed by a concurrent unmount, unionfs_nodeget()
may return without a reference or lock on it. unionfs_domount() must
prevent the vnode from being recycled for use by a different file until
it is finished with the vnode, namely once vfs_register_upper_from_vp()
fails. Accomplish this by holding the reference returned by namei()
a bit longer.
Reviewed by: kib, markj
Tested by: pho
Differential Revision: https://reviews.freebsd.org/D39767
If either the lower or upper vnode is found to be doomed after
locking it, the newly-created unionfs node won't be associated
with it and its lock will be dropped. In that case, clear the
uppervp and lowervp locals as necessary to avoid further use
of the vnode in unionfs_nodeget(). If the upper vnode is doomed
but the lower vnode remains valid, additionally reset the unionfs
node's v_vnlock field to point to the lower vnode lock.
Reviewed by: kib, markj
Tested by: pho
Differential Revision: https://reviews.freebsd.org/D39767
The underlying VOP_MKDIR() implementation may temporarily drop the
parent directory vnode's lock. If the vnode is reclaimed during that
window, the unionfs vnode will effectively become unlocked because
the its v_vnlock field will be reset. To uphold the locking
requirements of VOP_MKDIR() and to avoid triggering various VFS
assertions, explicitly re-lock the unionfs vnode before returning
in this case.
Note that there are almost certainly other cases in which we'll
similarly need to handle vnode relocking by the underlying FS; this
is the only one that's caused problems in stress testing so far.
A more general solution, such as that employed for nullfs in
null_bypass(), will likely need to be implemented.
Tested by: pho
Reviewed by: kib, markj
Differential Revision: https://reviews.freebsd.org/D39272
The implementation is racy; if the unionfs vnode is not in fact
locked, vnode private data may be concurrently altered or freed.
Instead, simply rely upon the standard implementation to query the
v_vnlock field, which is type-stable and will reflect the correct
lower/upper vnode configuration for the unionfs node.
Tested by: pho
Reviewed by: kib, markj
Differential Revision: https://reviews.freebsd.org/D39272
We hold the vnode interlock, so vnode private data cannot suddenly
become NULL.
Tested by: pho
Reviewed by: kib, markj
Differential Revision: https://reviews.freebsd.org/D39272
The LK_UPGRADE operation may have temporarily dropped the upper or
lower vnode's lock. If the unionfs vnode was reclaimed during that
window, its lock field will be reset to no longer point at the
upper/lower vnode lock, so the lock operation will use the standard
lock stored in v_lock. Remove LK_UPGRADE from the flags in this case
to avoid a lockmgr assertion, as this lock has not been previously
owned by the calling thread.
Reported by: pho
Tested by: pho
Reviewed by: kib, markj
Differential Revision: https://reviews.freebsd.org/D39272
If either of vnodes is shared locked, lock must not be recursed.
Requested by: rmacklem
Reviewed by: markj, rmacklem
Tested by: pho
Sponsored by: The FreeBSD Foundation
MFC after: 1 week
Differential revision: https://reviews.freebsd.org/D39444
To quote from a comment above vput_final:
<quote>
* XXX Some filesystems pass in an exclusively locked vnode and strongly depend
* on the lock being held all the way until VOP_INACTIVE. This in particular
* happens with UFS which adds half-constructed vnodes to the hash, where they
* can be found by other code.
</quote>
As is there is no mechanism which allows filesystems to denote that a
vnode is fully initialized, consequently problems like the above are
only found the hard way(tm).
Add rudimentary support for state transitions, which in particular allow
to assert the vnode is not legally unlocked until its fate is decided
(either construction finishes or vgone is called to abort it).
The new field lands in a 1-byte hole, thus it does not grow the struct.
Bump __FreeBSD_version to 1400077
Reviewed by: kib (previous version)
Tested by: pho
Differential Revision: https://reviews.freebsd.org/D37759
instead of looking at SAVESTART
This is a step towards removing the flag.
Reviewed by: mckusick
Tested by: pho
Differential Revision: https://reviews.freebsd.org/D34468
When taking the covered vnode lock during mount and unmount operations,
specify LK_CANRECURSE as the existing lock state of the covered vnode
is not guaranteed (AFAIK) either by assertion or documentation for
these code paths.
For the mount path, this is done only for completeness as the covered
vnode lock is not currently held when VFS_MOUNT() is called.
For the unmount path, the covered vnode is currently held across
VFS_UNMOUNT(), and the existing code only happens to work when unionfs
is mounted atop FFS because FFS sets LO_RECURSABLE on its vnode locks.
This of course doesn't cover a hypothetical case in which the covered
vnode may be held shared, but for the mount and unmount paths such a
scenario seems unlikely to materialize.
Reviewed by: kib
Tested by: pho
Differential Revision: https://reviews.freebsd.org/D37458
When a lookup operation crosses into a new mountpoint, the mountpoint
must first be busied before the root vnode can be locked. When a
filesystem is unmounted, the vnode covered by the mountpoint must
first be locked, and then the busy count for the mountpoint drained.
Ordinarily, these two operations work fine if executed concurrently,
but with a stacked filesystem the root vnode may in fact use the
same lock as the covered vnode. By design, this will always be
the case for unionfs (with either the upper or lower root vnode
depending on mount options), and can also be the case for nullfs
if the target and mount point are the same (which admittedly is
very unlikely in practice).
In this case, we have LOR. The lookup path holds the mountpoint
busy while waiting on what is effectively the covered vnode lock,
while a concurrent unmount holds the covered vnode lock and waits
for the mountpoint's busy count to drain.
Attempt to resolve this LOR by allowing the stacked filesystem
to specify a new flag, VV_CROSSLOCK, on a covered vnode as necessary.
Upon observing this flag, the vfs_lookup() will leave the covered
vnode lock held while crossing into the mountpoint. Employ this flag
for unionfs with the caveat that it can't be used for '-o below' mounts
until other unionfs locking issues are resolved.
Reported by: pho
Tested by: pho
Reviewed by: kib
Differential Revision: https://reviews.freebsd.org/D35054
This removes some of the complexity needed to maintain HASBUF and
allows for removing injecting SAVENAME by filesystems.
Reviewed by: kib (previous version)
Differential Revision: https://reviews.freebsd.org/D36542
VOP_GETWRITEMOUNT() is called on the vn_start_write() path without any
vnode locks guaranteed to be held. It's therefore unsafe to blindly
access per-mount and per-vnode data. Instead, follow the approach taken
by nullfs and use the vnode interlock coupled with the hold count to
ensure the mount and the vnode won't be recycled while they are being
accessed.
Reviewed by: kib (earlier version), markj, pho
Tested by: pho
Differential Revision: https://reviews.freebsd.org/D34282
I see no apparent need to avoid waiting on the lock just because
vinactive() may be called on another thread while the thread that
cleared the vnode refcount has the lock dropped. In fact, this
can at least lead to a panic of the form "vn_lock: error <errno>
incompatible with flags" if LK_RETRY was passed to VOP_LOCK().
In this case LK_NOWAIT may cause the underlying FS to return an
error which is incompatible with LK_RETRY.
Reported by: pho
Reviewed by: kib, markj, pho
Differential Revision: https://reviews.freebsd.org/D34109
The unionfs root vnode will always share a lock with its lower vnode.
If unionfs was mounted with the 'below' option, this will also be the
vnode covered by the unionfs mount. During unmount, the covered vnode
will be locked by dounmount() while the unionfs root vnode will be
locked by vgone(). This effectively requires recursion on the same
underlying like, albeit through two different vnodes.
Reported by: pho
Reviewed by: kib, markj, pho
Differential Revision: https://reviews.freebsd.org/D34109
VOP_LOCK() may be handed a vnode that is concurrently reclaimed.
unionfs_lock() accounts for this by checking for empty vnode private
data under the interlock. But it incorrectly asserts that the vnode
is using the unionfs dispatch table before making this check.
Reverse the order, and also update KASSERT_UNIONFS_VNODE() to provide
more useful information.
Reported by: pho
Reviewed by: kib, markj, pho
Differential Revision: https://reviews.freebsd.org/D34109
Also remove once-used functions to clean up after failed insmntque1(),
which were destructor callbacks in previous life.
Reviewed by: markj
Tested by: pho
Sponsored by: The FreeBSD Foundation
Differential revision: https://reviews.freebsd.org/D34071
This avoids a potentially wild reference to the mount object.
Additionally, simplify some of the checks around VV_ROOT in
unionfs_nodeget().
Reviewed by: kib
Differential Revision: https://reviews.freebsd.org/D33914
I was somehow convinced that insmntque calls insmntque1 with a NULL
destructor. Unfortunately this worked well enough to not immediately
blow up in simple testing.
Keep not using the destructor in previously patched filesystems though
as it avoids unnecessary casts.
Noted by: kib
Reported by: pho
do_execve() will hold the vnode lock shared when it calls VOP_OPEN(),
but unionfs_open() requires the lock to be held exclusive to
correctly synchronize node status updates. This requirement is
asserted in unionfs_get_node_status().
Change unionfs_open() to temporarily upgrade the lock as is already
done in unionfs_close(). Related to this, fix various cases throughout
unionfs in which vnodes are not checked for reclamation following lock
upgrades that may have temporarily dropped the lock. Also fix another
related issue in which unionfs_lock() can incorrectly add LK_NOWAIT
during a downgrade operation, which trips a lockmgr assertion.
Reviewed by: kib (prior version), markj, pho
Reported by: pho
Differential Revision: https://reviews.freebsd.org/D33729
The implementation simply passes the text ref to the appropriate
underlying vnode. Without this, the default [un]set_text
implementation will only manage the text ref on the unionfs vnode,
causing it to be out of sync with the underlying filesystems and
potentially allowing corruption of executable file contents.
On INVARIANTS kernels, it also readily produces a panic on process
termination because the VM object representing the executable mapping
is backed by the underlying vnode, not the unionfs vnode.
PR: 251342
Reviewed by: kib
Differential Revision: https://reviews.freebsd.org/D33611
Use atomics to track the writecount granted to the underlying FS,
and avoid holding the vnode interlock while calling the underling FS'
VOP_ADD_WRITECOUNT(). This also fixes a WITNESS warning about nesting
the same lock type. Also add comments explaining why we need to track
the writecount on the unionfs vnode in the first place. Finally,
simplify writecount management to only use the upper vnode and assert
that we shouldn't have an active writecount on the lower vnode through
unionfs.
Reviewed by: kib
Differential Revision: https://reviews.freebsd.org/D33611
The cookies argument is only used by the NFS server. NFSv2 defines the
cookie as 32 bits on the wire, but NFSv3 increased it to 64 bits. Our
VOP_READDIR, however, has always defined it as u_long, which is 32 bits
on some architectures. Change it to 64 bits on all architectures. This
doesn't matter for any in-tree file systems, but it matters for some
FUSE file systems that use 64-bit directory cookies.
PR: 260375
Reviewed by: rmacklem
Differential Revision: https://reviews.freebsd.org/D33404
unionfs must pass VOP_VPUT_PAIR directly to the underlying FS so that
it can have a chance to manage any special locking considerations that
may be necessary. The unionfs implementation is based heavily on the
corresponding nullfs implementation.
Also note some outstanding issues with the unionfs locking scheme, as
a first step in fixing those issues in a future change.
Discussed with: kib
Tested by: pho
Differential Revision: https://reviews.freebsd.org/D33008
Also remove a couple of write-only variables found by the recent clang
update. No functional change intended.
Discussed with: kib
Differential Revision: https://reviews.freebsd.org/D33008
VOP_RMDIR() is called with both parent and child directory vnodes
locked. The relookup operation performed by the unionfs implementation
may relock both vnodes. Accordingly, unionfs_relookup() drops the
parent vnode lock, but the child vnode lock is never dropped.
Although relookup() will very likely try to relock the child vnode
which is already locked, in most cases this doesn't produce a deadlock
because unionfs_lock() forces LK_CANRECURSE (!). However, relocking
of the parent vnode while the child vnode remains locked effectively
reverses the expected parent->child lock order, which can produce
a deadlock e.g. in the presence of a concurrent unionfs_lookup()
operation. Address the issue by dropping the child lock around
the unionfs_relookup() call in unionfs_rmdir().
Reported by: pho
Reviewed by: kib, markj
Differential Revision: https://reviews.freebsd.org/D32986
Instead of validating that a vnode belongs to unionfs only when the
caller attempts to extract the upper or lower vnode pointers, do this
validation any time the caller tries to extract a unionfs_node from
the vnode private data.
Reviewed by: kib
Differential Revision: https://reviews.freebsd.org/D32629
The lower FS VOP_READDIR() shouldn't return an empty read without
setting EOF; don't try to handle this case only for non-DIAGNOSTIC
builds.
Noted by: kib
Reviewed by: kib
Differential Revision: https://reviews.freebsd.org/D32629
Add an assertion to unionfs_node_update() that the upper vnode is
exclusively locked; we already make the same assertion for the lower
vnode.
Also, assert in unionfs_noderem() that the vnode lock is not recursed
and acquire v_lock with LK_NOWAIT. Since v_lock is not the active
lock for the vnode at this point, it should not be contended.
Finally, remove VDIR assertions from unionfs_get_cached_vnode().
lvp/uvp will be referenced but not locked at this point, so v_type
may concurrently change due to vgonel(). The cached unionfs node,
if one exists, would only have made it into the cache if lvp/uvp
were of type VDIR at the time of insertion; the corresponding
VDIR assert in unionfs_ins_cached_vnode() should be safe because
lvp/uvp will be locked by that time and will not be used if either
is doomed.
Noted by: kib
Reviewed by: kib
Differential Revision: https://reviews.freebsd.org/D32629
--Clearing cached subdirectories in unionfs_noderem() should be done
under the vnode interlock
--When preparing to switch the vnode lock in both unionfs_node_update()
and unionfs_noderem(), the incoming lock should be acquired before
updating the v_vnlock field to point to it. Otherwise we effectively
break the locking contract for a brief window.
Reviewed by: kib
Differential Revision: https://reviews.freebsd.org/D32629
unionfs uses a per-directory hashtable to cache subdirectory nodes.
Currently this hashtable is looked up using the directory name, but
since unionfs nodes aren't removed from the cache until they're
reclaimed, this poses some problems. For example, if a directory is
created on a unionfs mount shortly after deleting a previous directory
with the same path, the cache may end up reusing the node for the
previous directory, including its upper/lower FS vnodes. Operations
against those vnodes with then likely fail because the vnodes
represent deleted files; for example UFS will reject VOP_MKDIR()
against such a vnode because its effective link count is 0. This may
then manifest as e.g. mkdir(2) or open(2) returning ENOENT for an
attempt to create a file under the re-created directory.
While it would be possible to fix this by explicitly managing the
name-based cache during delete or rename operations, or by rejecting
cache hits if the underlying FS vnodes don't match those passed to
unionfs_nodeget(), it seems cleaner to instead hash the unionfs nodes
based on their underlying FS vnodes. Since unionfs prefers to operate
against the upper vnode if one is present, the lower vnode will only
be used for hashing as long as the upper vnode is NULL. This should
also make hashing faster by eliminating string traversal and using
the already-computed hash index stored in each vnode.
While here, fix a couple of other cache-related issues:
--Remove 8 bytes of unnecessary baggage from each unionfs node by
getting rid of the stored hash mask field. The mask is knowable
at compile time.
--When a matching node is found in the cache, reference its vnode
using vrefl() while still holding the vnode interlock. Previously
unionfs_nodeget() would vref() the vnode after the interlock was
dropped, but the vnode may be reclaimed during that window. This
caused intermittent panics from vn_lock(9) during unionfs stress
testing.
Reviewed by: kib, markj
Tested by: pho
Differential Revision: https://reviews.freebsd.org/D32533
"rm-style" system calls such as kern_frmdirat() and kern_funlinkat()
don't supply SAVENAME to preserve the pathname buffer for subsequent
vnode ops. For unionfs this poses an issue because the pathname may
be needed for a relookup operation in unionfs_remove()/unionfs_rmdir().
Currently unionfs doesn't check for this case, leading to a panic on
DIAGNOSTIC kernels and use-after-free of cn_nameptr otherwise.
The unionfs node's stored buffer would suffice as a replacement for
cnp->cn_nameptr in some (but not all) cases, but it's cleaner to just
ensure that unionfs vnode ops always have a valid cn_nameptr by setting
SAVENAME in unionfs_lookup().
While here, do some light cleanup in unionfs_lookup() and assert that
HASBUF is always present in the relevant relookup calls.
Reported by: pho
Reviewed by: markj
Differential Revision: https://reviews.freebsd.org/D32148
This fixes an insta-panic when attempting to use unionfs with
DEBUG_VFS_LOCKS. Note that unionfs still has a long way to
go before it's generally stable or usable.
Reviewed by: kib (prior version), markj
Tested by: pho
Differential Revision: https://reviews.freebsd.org/D31917
Running stress2 unionfs tests reliably produces a namei_zone corruption
panic due to unionfs_relookup() attempting to NUL-terminate a newly-
allocate pathname buffer without first validating the buffer length.
Instead, avoid allocating new pathname buffers in unionfs entirely,
using already-provided buffers while ensuring the the correct flags
are set in struct componentname to prevent freeing or manipulation
of those buffers at lower layers.
While here, also compute and store the path length once in the unionfs
node instead of constantly invoking strlen() on it.
Reviewed by: kib, markj
Differential Revision: https://reviews.freebsd.org/D31728
In certain emergency cases such as media failure or removal, UFS will
initiate a forced unmount in order to prevent dirty buffers from
accumulating against the no-longer-usable filesystem. The presence
of a stacked filesystem such as nullfs or unionfs above the UFS mount
will prevent this forced unmount from succeeding.
This change addreses the situation by allowing stacked filesystems to
be recursively unmounted on a taskqueue thread when the MNT_RECURSE
flag is specified to dounmount(). This call will block until all upper
mounts have been removed unless the caller specifies the MNT_DEFERRED
flag to indicate the base filesystem should also be unmounted from the
taskqueue.
To achieve this, the recently-added vfs_pin_from_vp()/vfs_unpin() KPIs
have been combined with the existing 'mnt_uppers' list used by nullfs
and renamed to vfs_register_upper_from_vp()/vfs_unregister_upper().
The format of the mnt_uppers list has also been changed to accommodate
filesystems such as unionfs in which a given mount may be stacked atop
more than one lower mount. Additionally, management of lower FS
reclaim/unlink notifications has been split into a separate list
managed by a separate set of KPIs, as registration of an upper FS no
longer implies interest in these notifications.
Reviewed by: kib, mckusick
Tested by: pho
Differential Revision: https://reviews.freebsd.org/D31016
