Improve msdosfs robustness

[kib]

Summary

Make kernel detect and handle some inconsistencies that could occur on msdosfs volume either mounted without running fsck first, of having buggy disk. The conditions are mainly:

• inconsistent FAT sectors chains resulting in wrong inode number
• paradoxical lookup results
• use of free FAT sector

Besides surviving the inconsistencies, the rw-mounted volume is asynchronously force-remounted to ro to minimize damage.

[kib]

Per commit log:

commit 26b47a51dffa2991070b907bcfc13f4d14d54deb
Author: Konstantin Belousov <[e-mail]>
Date:   Sat Dec 25 21:45:47 2021 +0200

    msdosfs: on integrity error, fire a task to remount filesystem to ro
    
    In collaboration with:  pho
    Sponsored by:   The FreeBSD Foundation
    MFC after:      1 week
    Differential revision:  https://reviews.freebsd.org/D33721

commit e2ebf636aa0ede72feecef0700982056495c568b
Author: Konstantin Belousov <[e-mail]>
Date:   Sat Dec 25 23:20:56 2021 +0200

    msdosfs: add msdosfs_integrity_error()
    
    A function to remount the filesystem from rw to ro on integrity error.
    The work is performed in taskqueue to allow the call to be done from
    almost arbitrary context where erronous state was detected.
    
    Tested by:      pho
    Sponsored by:   The FreeBSD Foundation
    MFC after:      1 week
    Differential revision:  https://reviews.freebsd.org/D33721

commit 8eff42bc36c0793bc4205e78c7f5e5a88afa17f4
Author: Konstantin Belousov <[e-mail]>
Date:   Sat Dec 25 21:44:59 2021 +0200

    Add vfs_remount_ro()
    
    a helper to remount filesystem from rw to ro.
    
    Tested by:      pho
    Sponsored by:   The FreeBSD Foundation
    MFC after:      1 week
    Differential revision:  https://reviews.freebsd.org/D33721

commit 1f2060da0b5158b0a1cb66d109d0d1c653012323
Author: Konstantin Belousov <[e-mail]>
Date:   Thu Dec 30 21:45:40 2021 +0200

    msdosfs: sanity check sector count from BPB
    
    We use sector count to size the FAT inuse bitset.  If sector count is
    corrupted, kernel might be tricked into doing unbound allocation.
    Ensure that the sector count does not exceed the actual volume size.
    
    In collaboration with:  pho
    Sponsored by:   The FreeBSD Foundation
    MFC after:      1 week
    Differential revision:  https://reviews.freebsd.org/D33721

commit ec3a58d8fad4e778360efbe7d079b81cd9fc981a
Author: Konstantin Belousov <[e-mail]>
Date:   Sun Dec 26 23:51:48 2021 +0200

    msdosfs: do no allow lookup to return vdp except for dot lookups
    
    In collaboaration with: pho
    Sponsored by:   The FreeBSD Foundation
    MFC after:      1 week
    Differential revision:  https://reviews.freebsd.org/D33721

commit d0ac8e37f414e79509ef26c17347261a3e831fe2
Author: Konstantin Belousov <[e-mail]>
Date:   Tue Dec 28 15:41:30 2021 +0200

    msdosfs: handle a case when non-dot lookup returned dvp
    
    This means that filesystem is corrupted, there is a loop.
    
    In collaboration with:  pho
    Sponsored by:   The FreeBSD Foundation
    MFC after:      1 week
    Differential revision:  https://reviews.freebsd.org/D33721

commit abff467cfe5ed8f4d2bac620a598b3613f6dec8e
Author: Konstantin Belousov <[e-mail]>
Date:   Sat Dec 25 20:39:15 2021 +0200

    msdosfs: take inusemap inconsistency as an error, not invariants violation
    
    In other words, stop silently accepting freeing free cluster in
    non-debug kernels, but return the error to the caller.  Modify callers
    to handle errors from usemap_free().
    
    In collaboration with:  pho
    Sponsored by:   The FreeBSD Foundation
    MFC after:      1 week
    Differential revision:  https://reviews.freebsd.org/D33721

commit 5a2b078c84ff7ed41af3404f616993567647dd63
Author: Konstantin Belousov <[e-mail]>
Date:   Fri Dec 24 01:21:53 2021 +0200

    msdosfs: handle inconsistently hashed denodes
    
    It is possible, on the corrupted msdosfs volume, to have file which
    denode inode number does not match the one calculated using directory
    cluster.  Instead of asserting the condition as impossible, handle it
    and return error, after reclaiming the aliased vnode.
    
    In collaboration with:  pho
    Sponsored by:   The FreeBSD Foundation
    MFC after:      1 week
    Differential revision:  https://reviews.freebsd.org/D33721

commit 81ce6412854dd728c92dfa479182b2eb75c9694b
Author: Konstantin Belousov <[e-mail]>
Date:   Thu Dec 30 15:17:59 2021 +0200

    geom label msdosfs: sanity check BPB before using it for io request
    
    It must be greater than zero, and be multiple of the device block size.
    
    In collaboration with:  pho
    Sponsored by:   The FreeBSD Foundation
    MFC after:      1 week
    Differential revision:  https://reviews.freebsd.org/D33721

commit 25ed06ec42ade0d053613fbf3b8adeb6f5054e9b
Author: Konstantin Belousov <[e-mail]>
Date:   Thu Dec 30 21:33:41 2021 +0200

    mountmsdosfs(): some style
    
    Sponsored by:   The FreeBSD Foundation
    MFC after:      1 week
    Differential revision:  https://reviews.freebsd.org/D33721

[kib]

Per-commit view is available at https://kib.kiev.ua/git/gitweb.cgi?p=deviant3.git;a=shortlog;h=refs/heads/ufs

[markj]

I think you should just commit the style fixes, they look ok to me.

sys/fs/msdosfs/msdosfs_fat.c

855 ↗

(On Diff #100832)

What's the purpose of writing the buf here?

sys/fs/msdosfs/msdosfs_vfsops.c

342 ↗

(On Diff #100832)

Isn't this check racy? Why do we do it here, and not, e.g., before the g_access() call?

[kib]

sys/fs/msdosfs/msdosfs_fat.c

855 ↗

(On Diff #100832)

The buffer is potentially dirty, and other changes did not triggered the check. I considered invalidating the buffer, but then decided that it is better to write whatever we modified before noted an already freed cluster.

sys/fs/msdosfs/msdosfs_vfsops.c

342 ↗

(On Diff #100832)

We lock the covered vnode exclusively both for mount/mount update, and in vfs_remount_ro(). So it should be good enough WRT races.

In fact, the reason to check for the flag there is to avoid falling into namei() below for vfs_remount_ro() case (our thread set the flag), which cannot work because "from" option is not supplied, and then a false EINVAL is returned.

[kib]

sys/fs/msdosfs/msdosfs_vfsops.c

342 ↗

(On Diff #100832)

I will add a comment about the reason for the check.

[kib]

Add comment explaining the reason for checking MSDOSFS_ERR_RO in msdosfs_mount().

[emaste]

sys/fs/msdosfs/msdosfs_denode.c

146 ↗

(On Diff #100839)

IMO we should include some reference - e.g. f_mntonname as I see elsewhere or at least the filesystem type itself (msdosfs: wrong dir...) if nothing better is available

[kib]

sys/fs/msdosfs/msdosfs_denode.c

146 ↗

(On Diff #100839)

Isn't f_mntonname included into the line?

[mckusick]

Overall looks good. Glad to see it done.

sys/fs/msdosfs/msdosfs_lookup.c

554 ↗

(On Diff #100839)

Do you still want to set SAVENAME if you are about to return an error and not set vpp?

sys/kern/vfs_mount.c

2871–2872 ↗

(On Diff #100839)

I would like to see a comment describing what this function does.

2879–2880 ↗

(On Diff #100839)

I presume that these "XXX" will have more useful description added.

[kib]

Handle Kirk' notes:

• add coments to vfs_remount_ro(); fill interface assert messages
• do not set SAVENAME in lookup if error is to be returned

[mckusick]

Looks good to me. If inspired a comment over msdosfs_remount_ro() would be nice, though the name pretty much says what it does.

[mckusick]

Spelling nit.

sys/kern/vfs_mount.c

2875 ↗

(On Diff #100845)

Inteded => Intended

[markj]

sys/fs/msdosfs/msdosfs_fat.c

425 ↗

(On Diff #100845)

Should calls be casted to void? Looks like this function is called only in error paths anyway.

855 ↗

(On Diff #100832)

Should we call updatefats() instead?

sys/fs/msdosfs/msdosfs_lookup.c

79 ↗

(On Diff #100845)

vput() seems to be ok for the existing callers...

sys/kern/vfs_mount.c

2895 ↗

(On Diff #100845)

Why is NOWAIT specified here? Isn't it quite possible for the covered vnode to be locked by parallel lookups?

[kib]

sys/fs/msdosfs/msdosfs_fat.c

425 ↗

(On Diff #100845)

I changed the signature for clusterfree() instead. It now returns void, and *oldcnp is gone.

855 ↗

(On Diff #100832)

Right.

sys/fs/msdosfs/msdosfs_lookup.c

79 ↗

(On Diff #100845)

Removed the comment, yes, testing clearly indicates that vput() is the right thing to do since we return error.

sys/kern/vfs_mount.c

2895 ↗

(On Diff #100845)

The mount point is busied (in no-wait mode) from the context where the inconsistency was detected. So when taskqueue task function is running, mp is busy. Lock order is that busy for mp is after all vnode locks for the filesystem where the covered vnode is located, see the comment before vfs_busy().

In practice, it was not a problem, at least in stress2 settings.

[kib]

Add missed vn_start_write() in vfs_remount_ro().
Change clusterfree() signature.
Use updatefats() for writing out fat block.
Fix typo in comment.

[markj]

sys/fs/msdosfs/msdosfsmount.h

120 ↗

(On Diff #100925)

Presumably this should be in the ifndef MAKEFS block.

[kib]

sys/fs/msdosfs/msdosfsmount.h

120 ↗

(On Diff #100925)

There is more, I postponed userspace build until everything is agreed upon.

[kib]

Make buildworld happy, by adjusting makefs msdos to the new kernel headers.

[emaste]

userland bits LGTM

sys/fs/msdosfs/msdosfs_denode.c

146 ↗

(On Diff #100839)

ah it is indeed.
the others seem to use it as a prefix though %s: rather than in the middle of the message

[kib]

sys/fs/msdosfs/msdosfs_denode.c

146 ↗

(On Diff #100839)

Changed to print it as prefix in local branch.

[mckusick]

Latest updates all look good.

[kib]

msdosfs: use mntfs vnode for pm_devvp to prevent races with devfs VCHR vnode reclamation, same as it was done for UFS.

[markj]

sys/kern/vfs_mount.c

2895 ↗

(On Diff #100845)

Maybe it deserves a comment or printf(), but I do not insist. It just seems somewhat fragile to abort the downgrade in this case, even if stress2 does not expose any problems.

[kib]

sys/kern/vfs_mount.c

2895 ↗

(On Diff #100845)

You are right, I will add two printfs, one for vfs_busy() failure, and one for vfs_remount_ro(). I probably will also add some kind of 'persistency' state to the integrity failure, where it is retried if we were unable to proceed the current request to remount ro. But this will be done later.