kern/58837: ffs: Missing locking around fs_fmod/time

[campbell+netbsd%mumble.net@localhost]

>Number:         58837
>Category:       kern
>Synopsis:       ffs: Missing locking around fs_fmod/time
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    kern-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Thu Nov 21 16:15:02 +0000 2024
>Originator:     Taylor R Campbell
>Release:        current, 10, 9, ...
>Organization:
The NetBSD_ffs Fmodation
>Environment:
>Description:
All access to fs_fmod in ffs_alloc.c appears to be serialized by struct ufsmount::um_lock.

However, there is also some access which does not appear to be serialized by um_lock:

- ffs_vfsops.c ffs_mount (update case) -- maybe this is serialized because the caller (mount_update) has suspended the file system?  (Can we assert this?)
- ffs_vfsops.c ffs_mountfs -- new file system so no concurrent activity is possible yet?
- ffs_vfsops.c ffs_unmount -- maybe this is serialized because the caller (dounmount) has suspended the file system?  (Can we assert this?)
- ffs_vfsops.c ffs_sync ***
- ffs_wapbl.c ffs_wapbl_sync_metadata
- ffs_wapbl.c ffs_wapbl_start -- new file system so no concurrent activity is possible yet, or for update case, caller has suspended the file system?

Some of these cases may be safe (but it would be nice to assert that they are so it's obvious for an auditor).  However, I can't see how to prove that ffs_sync's access to fs_fmod (and fs_time) is safe.

There's a comment over ffs_sync (and lfs_sync, ext2fs_sync) saying:

> Note: we are always called with the filesystem marked `MPBUSY'.

I think this means vfs_busy -- MNT_MPBUSY as such went away in the Lite2 merge in 1998.  But unlike MNT_MPBUSY, vfs_busy is not an exclusive lock -- it's just a reference that prevents the mount from going away.  So I don't see what serializes the calls to VFS_SYNC from do_sys_sync (the sync syscall) and sched_sync -- some VFS_SYNC calls are serialized by struct mount::mnt_updating but not all of them.  And I don't see what serializes any of those calls with the logic in ffs_alloc.c -- unlike ffs_alloc.c, ffs_sync doesn't take struct ufsmount::um_lock.
>How-To-Repeat:
code inspection
>Fix:
Yes, please!

1. sprinkle assertions if possible to make this easier to audit
2. properly serialize access to fs_fmod and fs_time if needed (e.g., take um_lock in ffs_sync)
3. fix comments about MPBUSY and/or turn it into an assertion