Page MenuHomeFreeBSD

Add support for mounting single files in nullfs
Needs ReviewPublic

Authored by dfr on Wed, Nov 23, 4:35 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Dec 2, 9:58 PM
Unknown Object (File)
Wed, Nov 30, 5:52 PM
Unknown Object (File)
Wed, Nov 30, 5:31 AM

Details

Reviewers
mjg
Summary

My main use-case for this is to support mounting config files and secrets
into OCI containers. My current workaround copies the files into the
container which is messy and risks secrets leaking into container images
if the cleanup fails.

Allow mounting files as well as directories

This adds a VFCF flag to indicate whether the filesystem supports file
mounts and allows fspath to be either a directory or a file if the flag
is set.

Test Plan

$ sudo mkdir -p /mnt
$ sudo touch /mnt/foo
$ sudo mount -t nullfs /COPYRIGHT /mnt/foo

  1. head -1 /mnt/foo
  2. @(#)COPYRIGHT 8.2 (Berkeley) 3/21/94

The compilation of software known as FreeBSD is distributed under the
following terms:

Copyright (c) 1992-2021 The FreeBSD Project.

Redistribution and use in source and binary forms, with or without
modification, are permitted provided that the following conditions
are met:

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 48608
Build 45494: arc lint + arc unit

Event Timeline

dfr requested review of this revision.Wed, Nov 23, 4:35 PM

This is a work in progress but I wanted a sanity check for the changes to vfs_mount.c while I figure out how to write ATF tests for it.

THere were a couple of places in vfs_cache.c which assumed that mount
points are directories and paniced if they were files. I relaxes these
checks to allow file mounts.

I have tested this with my main use-cases (podman and buildah) and simple tests are working. I plan to run some of the podman integration tests to get a better signal.

The two commits (user and kernel) should probably be squashed together before merging.

The invocation suggested is definitely a problem: sudo mount -t nullfs /COPYRIGHT /mnt/foo

should someone try to mount a regular file by accident on stock system their attempt is going to fail, patched one will succeed when it should not have.

I think there should be an explicit option (say -o regfile or similar) which would *require* the target to be VREG when used, VDIR otherwise.

sbin/mount/getmntopts.c
155

this and other changes to userspace tooling should not be necessary and I would argue should not be done.

There is the general problem that mount() can fail for a multitude of reasons, but they are not conveyed to the user. There was a project some time ago to export a string with the actual error which perhaps should be resurrected, but I'm not going to insist.

sys/fs/nullfs/null_vfsops.c
163

this needs to assert only VDIR or VREG is passed

sys/kern/vfs_mount.c
1564

vn_path_to_global_path performs an extra lookup to validate nothing has changed, your patch loses that behavior. You would need to add vn_path_to_global_path_hardlink or similar to remedy this.

1578

you should only get here if namei succeeded, which means ni_dvp must not be NULL

dfr marked 2 inline comments as done.

Updated to include requested changes. I'm still working on a regular-file equivalent to vn_path_to_global_path.

In D37478#852943, @mjg wrote:

The invocation suggested is definitely a problem: sudo mount -t nullfs /COPYRIGHT /mnt/foo

should someone try to mount a regular file by accident on stock system their attempt is going to fail, patched one will succeed when it should not have.

I think there should be an explicit option (say -o regfile or similar) which would *require* the target to be VREG when used, VDIR otherwise.

I'm not sure what the problem is here. Assuming that /mnt/foo is a regular file, why should the example mount fail? Surely the checks to enforce that the two objects are either both file or both directory is sufficient.

sbin/mount/getmntopts.c
155

I am mostly attempting to follow existing here but there is clearly some redundancy. This kind of checking is happening in mount, mount_nullfs and kernel. It would be nice to have only kernel checking but it seems like that is best left until we can get better error messages from kernel.

sys/kern/vfs_mount.c
1564

I will work on adding a vn_path_to_global_path_hardlink with similar semantics, ideally with a simpler interface to vn_fullpath_hardlink.

Add vn_path_to_global_path_hardlink. This works for trivial tests and makes
the code in vfs_domount more readable. I'll update my podman test VM and
stress test it a little.

dfr marked an inline comment as done.Mon, Nov 28, 4:07 PM
In D37478#852960, @dfr wrote:
In D37478#852943, @mjg wrote:

The invocation suggested is definitely a problem: sudo mount -t nullfs /COPYRIGHT /mnt/foo

should someone try to mount a regular file by accident on stock system their attempt is going to fail, patched one will succeed when it should not have.

I think there should be an explicit option (say -o regfile or similar) which would *require* the target to be VREG when used, VDIR otherwise.

I'm not sure what the problem is here. Assuming that /mnt/foo is a regular file, why should the example mount fail? Surely the checks to enforce that the two objects are either both file or both directory is sufficient.

I mean they made a mistake and with your patch it will happen to "work" even though it is not what was meant.

sys/kern/vfs_mount.c
1558

this needs to test on VREG? a switch statement is probably best here

In D37478#853071, @mjg wrote:
In D37478#852960, @dfr wrote:
In D37478#852943, @mjg wrote:

The invocation suggested is definitely a problem: sudo mount -t nullfs /COPYRIGHT /mnt/foo

should someone try to mount a regular file by accident on stock system their attempt is going to fail, patched one will succeed when it should not have.

I think there should be an explicit option (say -o regfile or similar) which would *require* the target to be VREG when used, VDIR otherwise.

I'm not sure what the problem is here. Assuming that /mnt/foo is a regular file, why should the example mount fail? Surely the checks to enforce that the two objects are either both file or both directory is sufficient.

I mean they made a mistake and with your patch it will happen to "work" even though it is not what was meant.

That kind of makes sense - the closest equivalent elsewhere is on Linux where bind mounts ignore the mount type but require the MS_BIND flag. I'm not sure gating this with an option is the best interface but don't feel that strongly. A possible alternative would be a sysctl to globally enable/disable this new feature?

sys/kern/vfs_mount.c
1558

I don't see the need for a switch - the only reason I needed something other than vn_path_to_global_path here was that non-directories need a little help to resolve, e.g. parent directory plus leaf component. This would be the same for types other than VREG. Perhaps this can be reduced to just vn_path_to_global_path_hardlink since that should worok just as well for directories.

I can't think of a scenario where something other than VDIR or VREG is useful here but I would prefer to leave that kind of policy enforcement to vfs_domount_first which is where all the other sanity checks are applied.

Added a couple of comments above

you should ask pho@ to test this, I would not be surprised if an assert tripped over

addition of vn_path_to_global_path_hardlink has to be a standalone commit

sys/kern/vfs_mount.c
1558

my point is that that you may find yourself with neither VDIR nor VREG here and pass it down to vfs_domount_first. bare minimum this requires a comment that the type is checked in that routine.

Added a comment to note that vnode type validation happens in vfs_domount_first. Also fixed a couple of line-too-long formatting issues.

I will also reach out to pho@ for advice on stress testing.

I'm going to have to take a closer look. v_mountedhere is in a union which can be used for other purposes for non-VDIR vnodes, chances are decent this trips over.

Split out vn_path_to_global_path_hardlink into its own commit.

Added an ugly workaround for a panic caused by running realpath on a file mount.