Fix for automount -c inappropriately clearing important mount flags

[andrew_tao173.riddles.org.uk]

Summary

If you try and enable both the -media and -noauto maps from the sample auto_master, and enable use of automount -c from devd as suggested in the comments, then on every devd event, the flags of all the -noauto filesystems are reset (clearing the "automounted" flag and, more critically, flags such as "readonly" and "nosuid").

This happens because when traversing the mount list, there are two entries for each -noauto map entry that is currently active: the autofs filesystem itself, and the real filesystem mounted on top of it. The automount program sees the first of those, and issues the equivalent of "mount -u /path" on it, WITHOUT checking whether "/path" is currently mounted over.

This can be partially fixed in the userland code, but that obviously leaves a race condition. Accordingly this review includes both a kernel and a userland change: the kernel change is to require that the fstype field passed to nmount does actually match the VFS type of the filesystem to be updated.

bug link: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=272446

Test Plan

Tested that mount -u ... still works and that automount -c correctly skips over mounted-over autofs dirs.

[andrew_tao173.riddles.org.uk]

Maybe the added requirement that fstype must match when MNT_UPDATE is used should go in the manpage? (I can add that)

I did look at whether MNT_BYFSID could be made usable with MNT_UPDATE, but it can't because its value conflicts with that of another flag.

[andrew_tao173.riddles.org.uk]

Updated to include manpage fix.

[kevans]

Let's start with kib, mjg, rmacklem; maybe they can point to someone else if there's a better candidate to review.

[mjg]

The mount update path should grow support for FSID, which would close the problem.

Check kern_unmount to see what I mean.

[andrew_tao173.riddles.org.uk]

In D40961#942915, @mjg wrote:

The mount update path should grow support for FSID, which would close the problem.

Check kern_unmount to see what I mean.

Already looked at that, if you read the previous comments. The problem is that MNT_BYFSID overlaps in value with a flag that is valid for update, namely MNT_ACLS.

See comments in sys/mount.h

[andrew_tao173.riddles.org.uk]

Possible approach: rename existing MNT_BYFSID to MNT_BYFSID_OLD and add a new MNT_BYFSID with a non-colliding value; have nmount support (only) the new value, but leave umount supporting both.

[andrew_tao173.riddles.org.uk]

No, I don't think even that is possible; on 32-bit arch, there are literally zero flag bits available.

Instead, nmount would have to accept an "fsid" option (in place of fspath? or as a boolean flag?).

[kib]

It is nmount, right? What is the problem to add an option containing fsid as value?

[mjg]

So the fsid thing has a problem of being tied to NFS from security standpoint and is not even exported for unprivileged users.

Some time ago I proposed implementing per-mount point ids guaranteed to be unique (a 64bit ever-increasing counter) and not tied to anything. So they are race free in the sense if someone unmounted/mounted on top, id lookup fails. At that time kib mentioned considering adding something like '\0FSID:...' as a path, so that you could query by fsid and not path. As noted above, fsid is not unfortunately a questionable thing to rely on and I find it regrettable this is part of the current interface.

Mount point can be covered by another one, thus *currently* there is no way to statfs it for example. You ran into an equivalent problem here and it would be immediately solved with mount checking if path starts with \0 or some other \0-starting magic value indicating the value is the id.

I would say adding the guaranteed-unique facility would be a longer term of working this out.

[andrew_tao173.riddles.org.uk]

If the long-term solution is going to involve something new, then how about committing this as it stands in the meantime?

[mjg]

but this is only trying to paper over part of the problem and flush_autofs still does not work?

[andrew_tao173.riddles.org.uk]

In D40961#943360, @mjg wrote:

but this is only trying to paper over part of the problem and flush_autofs still does not work?

It's not "trying to paper over part of the problem", it's making it actually usable. Right now, what happens if you use both media and noauto maps is that the noauto maps don't get autounmounted (because the automounted flag gets turned off), they become writable even if declared readonly (because the readonly flag gets turned off), they become a serious security problem because nosuid and noexec get turned off, etc.

If a direct map is currently mounted over, then doing a cache flush on it isn't going to have any effect anyway since it is inaccessible until unmounted, and in any event it has nothing to cache. The only time caching comes into play is when the map entries actually correspond to subdirectories of the autofs mountpoint, which is not the case for the kind of direct map that "noauto" uses, and if the autofs mountpoint isn't mounted over, then the code still works just as it always did.

[mjg]

ok, fair. in that case i have no opinion about the posted patch and will defer to other people

[kib]

Am I right that e.g. the following usage is broken by the patch 'as-is' : mount -u -w /?

[andrew_tao173.riddles.org.uk]

In D40961#943507, @kib wrote:

Am I right that e.g. the following usage is broken by the patch 'as-is' : mount -u -w /?

No. That would make booting rather hard.

(mount -u gets the fstype from getmntinfo.)

[rmacklem]

Well, I know nothing about autofs, so I cannot comment on that part.
As for checking for same fstype during a mount update, I cannot see
a problem with doing that.

If the fstype is not the same, that would indicate a mount has covered
the mount point between the application deciding to update the mount
and the update happening.
Simply put, I cannot think why this would be legitimately done?

If the kernel patch is applied, is the automount.c change needed?
(Or does the nmount(2) fail with EINVAL?)

[andrew_tao173.riddles.org.uk]

In D40961#943728, @rmacklem wrote:

If the kernel patch is applied, is the automount.c change needed?
(Or does the nmount(2) fail with EINVAL?)

If the kernel patch is done, then the automount.c change just serves to avoid a spurious error message.

[rmacklem]

lib/libc/sys/mount.2

302 ↗

(On Diff #124762)

Although not listed in the man page, it looks like
nmount(2) can generate EINVAL for many things.
Not specifying the "fstype" argument being one of
many.

This should be fixed, but I'd suggest it be left for
a separate commit.
(Do a search for EINVAL in sys/kern/vfs_mount.c
and you'll see what I mean.)

[rmacklem]

sys/kern/vfs_mount.c

1378 ↗

(On Diff #124762)

Just a possible suggestion that others might
not like...
ENXIO (Device not configured) could sorta fit
this case.

The advantage is that nmount(2) never generates
this error now (although VFS_MOUNT() methods
for file systems can).
--> I think only this case would generate it for MNT_UPDATE
and automount.c could not bother to log an error message
for this nmount(MNT_UPDATE) return.
This avoids the "spurious error logged" that your patch to
automount.c below only partially avoids.

It might also be easier to add a ENXIO entry for nmount(2)
in the man page, although I tend to do man pages as separate
reviews with manpages included for group review.

Again, just a possible suggestion. It is up to you.

[andrew_tao173.riddles.org.uk]

sys/kern/vfs_mount.c

1378 ↗

(On Diff #124762)

I don't really like ENXIO for this, simply in terms of the usual meanings of errors. For one thing, ENXIO tends to show up if devices get improperly detached, which makes it a much too serious error to go around randomly ignoring. I went with EINVAL after failing to find any reasonable alternative.

With both user and kernel changes, automount should only report an error if there's an actual race between automount -c doing statfs() on the mountpoint and automountd triggering an actual mount because something accessed the directory contents. I don't see any particular reason to go to extreme lengths to suppress the (basically harmless) error in this case.

If the idea of doing MNT_UPDATE via an identifier rather than by path is implemented later, that would remove the race anyway.

[rmacklem]

I'd suggest a further review of the man page changes,
but unless others find issues with the src changes,
I think they are ok.

[andrew_tao173.riddles.org.uk]

Unless there is any further comment, would someone commit this please? (w/MFC)

[kevans]

In D40961#947814, @andrew_tao173.riddles.org.uk wrote:

Unless there is any further comment, would someone commit this please? (w/MFC)

The manpage part looks fine to me, if nobody objects I'll commit it in three days (Friday, 9/15).

[kib]

It feels too hackish, in particular the fact that the type is preserved. No action were done to investigate what would make the version to pass uuid through nmount option.

[andrew_tao173.riddles.org.uk]

In D40961#953488, @kib wrote:

It feels too hackish, in particular the fact that the type is preserved.

I do not understand this complaint. That fstype is a mandatory parameter was always documented and its presence was always enforced even on MNT_UPDATE, the only new requirement is that it actually be correct, and there is no legitimate case where an incorrect value would be present.

No action were done to investigate what would make the version to pass uuid through nmount option.

If you want to add a new system of mount ids then by all means go ahead, but it seems to me that that would be a solution only for -current, and this is a BUG FIX that needs to go into stable versions.

[kib]

I do not propose any 'new' system for mount ids, I propose to allow userspace to reuse fsid as filesystem identifier. See D42023, it is quite simple.