Details

Reviewers

emaste
mckusick
markj

Commits

rGaaaa4fb54e5c: msdosfs: use mntfs vnode for pm_devvp
rG41e85eeab90d: msdosfs: on integrity error, fire a task to remount filesystem to ro
rGb2e4b6358440: msdosfs: add msdosfs_integrity_error()
rG4a4b059a9795: Add vfs_remount_ro()
rGba2c98389b78: msdosfs: sanity check sector count from BPB
rG65990b68a2cd: msdosfs: clusterfree() is used only in error handling cases
rGaec97963cd03: msdosfs: do no allow lookup to return vdp except for dot lookups
rG2c9a1c22c323: msdosfs: take inusemap inconsistency as an error, not invariants violation
rG1319c433f4e5: msdosfs: handle a case when non-dot lookup returned dvp
rG595ed4d76713: msdosfs: handle inconsistently hashed denodes
rG9f4073d44628: geom label msdosfs: sanity check BPB before using it for io request
rG04fd468da0d0: mountmsdosfs(): some style

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.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Herald added a subscriber: imp. · View Herald TranscriptJan 2 2022, 4:34 PM

kib requested review of this revision.Jan 2 2022, 4:34 PM

Per commit log:

commit 26b47a51dffa2991070b907bcfc13f4d14d54deb
Author: Konstantin Belousov <kib@FreeBSD.org>
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 <kib@FreeBSD.org>
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 <kib@FreeBSD.org>
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 <kib@FreeBSD.org>
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 <kib@FreeBSD.org>
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 <kib@FreeBSD.org>
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 <kib@FreeBSD.org>
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 <kib@FreeBSD.org>
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 <kib@FreeBSD.org>
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 <kib@FreeBSD.org>
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

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

kib added reviewers: emaste, mckusick, markj.Jan 2 2022, 4:38 PM

kib added a subscriber: pho.

emaste added subscribers: fsu, delphij.Jan 2 2022, 6:28 PM

emaste removed a subscriber: fsu.Jan 2 2022, 6:37 PM

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 marked 2 inline comments as done.Jan 2 2022, 8:30 PM

kib added inline comments.

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 marked 2 inline comments as done.Jan 2 2022, 8:40 PM

kib added inline comments.

sys/fs/msdosfs/msdosfs_vfsops.c
342 ↗	(On Diff #100832)	I will add a comment about the reason for the check.

This revision was not accepted when it landed; it landed in state Needs Review.Jan 2 2022, 8:44 PM

Closed by commit rG04fd468da0d0: mountmsdosfs(): some style (authored by kib). · Explain Why

This revision was automatically updated to reflect the committed changes.

kib added a commit: rG04fd468da0d0: mountmsdosfs(): some style.

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

emaste added inline comments.Jan 2 2022, 8:52 PM

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 added inline comments.Jan 2 2022, 9:00 PM

sys/fs/msdosfs/msdosfs_denode.c
146 ↗	(On Diff #100839)	Isn't f_mntonname included into the line?

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.

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

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

This revision is now accepted and ready to land.Jan 2 2022, 10:43 PM

Spelling nit.

sys/kern/vfs_mount.c
2875 ↗	(On Diff #100845)	Inteded => Intended

markj added inline comments.Jan 4 2022, 2:13 PM

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 marked 5 inline comments as done.Jan 4 2022, 2:51 PM

kib added inline comments.

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 marked 4 inline comments as done.Jan 4 2022, 2:57 PM

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

This revision now requires review to proceed.Jan 4 2022, 3:03 PM

markj accepted this revision.Jan 4 2022, 10:07 PM

markj added inline comments.

sys/fs/msdosfs/msdosfsmount.h
120 ↗	(On Diff #100925)	Presumably this should be in the `ifndef MAKEFS` block.

This revision is now accepted and ready to land.Jan 4 2022, 10:07 PM

kib marked an inline comment as done.Jan 5 2022, 12:42 AM

kib added inline comments.

sys/fs/msdosfs/msdosfsmount.h
120 ↗	(On Diff #100925)	There is more, I postponed userspace build until everything is agreed upon.

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

This revision now requires review to proceed.Jan 5 2022, 12:43 AM

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 marked an inline comment as done.Jan 5 2022, 5:33 AM

kib added inline comments.

sys/fs/msdosfs/msdosfs_denode.c
146 ↗	(On Diff #100839)	Changed to print it as prefix in local branch.

jlduran added a subscriber: jlduran.Jan 5 2022, 2:02 PM

markj accepted this revision.Jan 5 2022, 3:13 PM

This revision is now accepted and ready to land.Jan 5 2022, 3:13 PM

Latest updates all look good.

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

This revision now requires review to proceed.Jan 7 2022, 8:32 AM

markj accepted this revision.Jan 7 2022, 3:16 PM

markj added inline comments.

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.

This revision is now accepted and ready to land.Jan 7 2022, 3:16 PM

kib marked an inline comment as done.Jan 8 2022, 3:40 AM

kib added inline comments.

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.