fusefs: fix vnode locking violations during execve

[asomers]

Summary

Fix two locking violations that could happen during execve, while
executing a file stored on fusefs. Both would cause panics on an
INVARIANTS kernel after 15.0, or a DEBUG_VFS_LOCKS kernel prior to that.

• Don't assume that the vnode is exclusively locked during VOP_CLOSE. It usually is thanks to !MNTK_LOOKUP_SHARED, but isn't during execve, which locks the vnode outside of the lookup path.

• Totally rewrite fuse_io_invalbuf. It's had a number of problems ever since its original introduction[^1]:
  • • Don't assume that the vnode is exclusively locked. That assumption failed during execve just like the assumption in fuse_vnop_close.
    • Don't livelock forever if vinvalbuf returns ENOSPC or EDQUOT.
    • Don't attempt to handle multiple threads calling this function at the same time. That would be impossible if the vnode truly were exclusively locked. So the code was dead. Or it would've been, if the assumption hadn't been wrong. Furthermore, both vinvalbuf and vnode_pager_clean_sync only require a shared vnode lock, and are already capable of dealing with multiple simultaneous callers.
    • Using fvdat->flag in this way would require some sort of mutex protection, if the vnode weren't exclusively locked.
  • Add a new test case that triggers both of the aforementioned panics.

[^1]: https://github.com/glk/fuse-freebsd/commit/efe6eb3005e7633b4e31d5e453eacbaa0cba42fa

PR: 295957
Reported by: [e-mail]
MFC after: 2 weeks
Sponsored by: ConnectWise

Test Plan

Test case added

[markj]

sys/fs/fuse/fuse_io.c

939

sys/fs/fuse/fuse_vnops.c

920

Won't this recurse on the attributes lock?

945

This function asserts that the vnode is exclusively locked, but it may not be per your description.

tests/sys/fs/fusefs/misc.sh

45 ↗

(On Diff #179586)

[asomers]

sys/fs/fuse/fuse_vnops.c

920

It doesn't normally recurse, because VOP_CLOSE is usually called with the vnode exclusively locked. And it doesn't recurse during execve, because that doesn't trigger an atime update. But you're right that it could recurse, if the file were read (triggering an atime update) and then executed by fexecve, without being closed in between. That's going to take some effort to write a test case for.

945

Similarly, execve won't trigger this, but fexecve theoretically could. I'll try to come up with a test case.

[asomers]

sys/fs/fuse/fuse_vnops.c

920

Actually, it's impossible to recurse, but the reason is complicated:

• If you use fexecve, then the vnode must've been opened via the normal lookup path, which always uses LK_EXCLUSIVE on fusefs. In that case, CACHED_ATTR_LOCK doesn't need to lock the mutex, so there's no recursion.

• If you use execve, then do_execve must open the vnode itself, and it uses LOCKSHARED. That might be considered a bug, because it ignores the file system's MNTK_EXTENDED_SHARED, but that's the way it is. Then fuse_vnop_close will eventually be called with a shared vnode lock. However, in that case it won't need an atime update. Nor will it need to update the vnode's size below.

So I'm inclined to leave the kernel code the way it is. But relying on this behavior is certainly fragile, so I'll add a "pregression" test that will exercise these two potential locking violations, IF fusefs ever drops MNTK_EXTENDED_SHARED.

[asomers]

• Convert tests from sh to C++
• Fix a mutex recursion panic

[asomers]

sys/fs/fuse/fuse_vnops.c

920

Actually, contrary to my prior belief, even when using fexecve it's possible to have LK_SHARED here. The pregression test triggered that. So I'll have to fix the lock recursion.

945

No, fexecve can't trigger that, because it's illegal to execute a file descriptor that is open for writing. And there's no way to dirty the file's size without writing to it.

[asomers]

The new dtrace probes are just to help develop these test cases. I'll remove them before I commit.

[markj]

sys/fs/fuse/fuse_vnops.c

945

I would add a comment which explains this.

tests/sys/fs/fusefs/ext2-misc.sh

45

I'd suggest using atf_check for all of the commands above.

tests/sys/fs/fusefs/misc.cc

111

Do you actually need to handle this cleanup? If so, true_file is leaked.

[asomers]

Respond to @markj's latest comments

[markj]

sys/fs/fuse/fuse_io.c

930–931

I think this comment is stale.