tmpfs_rename(): fix a locking bug

[kib]

Summary

tmpfs_rename(): use tmpfs_access_locked instead of VOP_ACCESS()

Protect the call with the node lock. We cannot lock the fvp vnode
sleepable there, because we already own other participating vnode's
locks. Taking it without sleeping require unwinding the whole locking
state in one more place.

Note that the liveness of the node is guaranteed by the lock on the
parent directory vnode.

Fixes:  cbac1f3464956185cf95955344b6009e2cc3ae40ESC

tmpfs_access_locked(): extract tmpfs-specific part of tmpfs_access() into helper

The helper tmpfs_access_locked() requires either the vnode or node
locked for consistency of the access check, unlike the pure vnode op.

tmpfs_access(): style fixes and remove redundand assertions

Note that MPASS(VOP_ISLOCKED(vp)) is simply broken.

[kib]

Correct the patch base, so it is applicable to main.

[markj]

sys/fs/tmpfs/tmpfs_vnops.c

392

Why doesn't this access require any synchronization?

1130

tmpfs_access() checks for a read-only mount - where does that get handled in this code path? In the VFS?

[markj]

sys/fs/tmpfs/tmpfs_vnops.c

392

Sorry, ignore this, I misread vnode_if.src.

[kib]

sys/fs/tmpfs/tmpfs_vnops.c

1130

namei/lookup for rename would catch it much earlier, even before the VOP is entered.

[mjg]

perhaps it is time to follow linux, netbsd and dragonflybsd and add a per-mount point rename lock, which in turn would drastically simplify the op, including dropping the potential to need to relookup stuff

note the existing vfs rename code is already wrong in post rename ops -- the vnodes which get knotes posted may very well not be the ones which got any renames

sys/fs/tmpfs/tmpfs_vnops.c

382

does this bit compile away if ASSERT_VOP_LOCKED expands to nothing?

maybe it should hide behind DEBUG_VFS_LOCKS

[mjg]

forgot to ask, is there a problem at *this* stage to just lock the target vnode? commit message should probably explain why the code resorts to something else

[kib]

sys/fs/tmpfs/tmpfs_vnops.c

382

This is a common practice to leave empty branches under some options, e.g. in sys/vm. I will add #if DEBUG_VFS_LOCKS around.

[kib]

Add DEBUG_VFS_LOCKS braces. Update summary to explain the reason to use node lock instead the vnode lock.

[mjg]

sys/fs/tmpfs/tmpfs_vnops.c

382

the point is that mtx_owned ultimately expands to a load from a volatile pointer, i don't know if these magically go away if the result is unused