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.

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.

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?

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

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.

The proposed hack in kern___realpathat is definitely not the way to do it -- by the time the second lookup is performed the name may be different.

I figured this corner case should have an obvious solution: the target vnode has v_mountedhere, from there you should be able to reliably find the vnode from the mounted on filesystem, and that should be unique and consequently easily sorted out with reverse path resolution.

Except I realized there is another issue: hardlinks to the mount point losing the uniqueness which sounds like a huge can of worms. You can definitely check there are no hardlinks at mount time and deny creating them when mounted.

Another potential can of worms is stacked mounts, one after another. Should probably also get disallowed.

That said, I'm going to have to chew on this, there are more corner cases to consider that I initially thought.

In D37478#854930, @mjg wrote:

The proposed hack in kern___realpathat is definitely not the way to do it -- by the time the second lookup is performed the name may be different.

I figured this corner case should have an obvious solution: the target vnode has v_mountedhere, from there you should be able to reliably find the vnode from the mounted on filesystem, and that should be unique and consequently easily sorted out with reverse path resolution.

Except I realized there is another issue: hardlinks to the mount point losing the uniqueness which sounds like a huge can of worms. You can definitely check there are no hardlinks at mount time and deny creating them when mounted.

Another potential can of worms is stacked mounts, one after another. Should probably also get disallowed.

That said, I'm going to have to churn on this, there are more corner cases to consider that I initially thought.

I realised my change to kern___realpathat was broken shortly after I updated the diff. For one thing, it returns the wrong path if the input path is relative. At the moment, I'm conditionally disabling the vp_crossmp replacement for lookups where the mount point is the last path element but that probably opens up the same LOR from commit 7f92c4e which crossmp was introduced to fix.

Thanks for the hint about v_mountedhere - I will try to make that work. Also, restricting these mounts to files without hardlinks etc. makes sense to me so I will add a check for that.

I'm not confident the LOR is really there and even if it is, crossmp is a hack solution. unmount can instead be patched around to not suffer the problem, but I don't have the time right now to look into it.

In D37478#854931, @dfr wrote:

Thanks for the hint about v_mountedhere - I will try to make that work. Also, restricting these mounts to files without hardlinks etc. makes sense to me so I will add a check for that.

Erm, I meant v_mount. The idea being you can test this is a nullfs file mount, and if so, v_mount can be used to walk up as it contains a pointer to the covered vnode.

I get a panic with D37478.id113832.diff : https://people.freebsd.org/~pho/stress/log/log0394.txt

In D37478#855162, @pho wrote:

I get a panic with D37478.id113832.diff : https://people.freebsd.org/~pho/stress/log/log0394.txt

This one looks like an attempt to mount a file onto an existing file mount. I'm going to disable that.

I have not observed any problems while testing with D37478.113884.patch

Thanks for all your work Peter!

Assuming that we complete the review process and get this change into -current, does anyone have concerns merging this into 13-stable? It would be really nice to have this feature available in 13.2-RELEASE.

BTW, I remember Solaris had the ability to mount a regular file over a regular file, without stacking helpers.

I mean, why requiring nullfs there? Just keep a reference to the lower vnode as v_mountedthere and make lookup() handle it. This is probably as close to Linux' bind mount as it is possible.

In D37478#856005, @kib wrote:

BTW, I remember Solaris had the ability to mount a regular file over a regular file, without stacking helpers.

I mean, why requiring nullfs there? Just keep a reference to the lower vnode as v_mountedthere and make lookup() handle it. This is probably as close to Linux' bind mount as it is possible.

I think using nullfs here simplifies the change quite a bit - doing things the bind way would involve inventing a way of expressing a non-filesystem mount to nmount, plumbing that through in /sbin/mount etc.

One thing we do get from nullfs is that the mounted file gets a new st_dev which prevents hardlinks if thats useful.

Thanks Konstantin, Mateusz for all your help in the review process. Are there any other areas which need work here? I remember that Mateusz was interested in ensuring that existing erroneous attempts to mount files would succeed with this change. How serious is this potential problem? Perhaps we can mitigate by gating this with a sysctl? I plan to merge this to stable/13 so perhaps have this feature opt in for 13 and opt out for 14 and later?

In D37478#856652, @dfr wrote:

I remember that Mateusz was interested in ensuring that existing erroneous attempts to mount files would succeed with this change. How serious is this potential problem? Perhaps we can mitigate by gating this with a sysctl? I plan to merge this to stable/13 so perhaps have this feature opt in for 13 and opt out for 14 and later?

I personally do not think that this is a serious issue. But if you want to provide a gatekeeping, I would suggest adding a mount option (like mount -o allowfile -t nullfs /from /to). This option is not even needs to be passed to kernel, check for it in mount_null(8). IMO it is more explicit than a hidden sysctl.

I do not see why this patch, in any form, cannot be merged to 13, as is.

Personally, I don't think its necessary to be bug-for-bug compatible in most cases. If Mateusz feels strongly about it I will add a check in mount_nullfs as suggested but if not I'm happy with to move forwards as-is.

What "bug to bug compatiblity"?

I noted mount -t nullfs /some-file /some-other-file would fail on stock kernel and succeed on the patched one. If someone messes up and specifies regular files by accident, this is going to work out when it should not have from their perspective and this is why I suggested a switch to denote what they want, but I'm not going to insist on it.

code-wise, I'm deeply unsatisfied with this crossmp vnode business. I'll look around, perhaps it is fixable, in which case the above patch will be able to drop the realpath kludge.

Past that I don't see problems with the code and I'll ack in a day or two if the crossmp endeavor wont show anything promising.

In D37478#856863, @mjg wrote:

What "bug to bug compatiblity"?

I noted mount -t nullfs /some-file /some-other-file would fail on stock kernel and succeed on the patched one. If someone messes up and specifies regular files by accident, this is going to work out when it should not have from their perspective and this is why I suggested a switch to denote what they want, but I'm not going to insist on it.

I used the wrong term. I was just trying to point out that if an application has a bug (mistakenly mounting a file) or is trying to verify that FreeBSD does not have the ability to mount files, it should not stop us from implementating that feature.

code-wise, I'm deeply unsatisfied with this crossmp vnode business. I'll look around, perhaps it is fixable, in which case the above patch will be able to drop the realpath kludge.

Past that I don't see problems with the code and I'll ack in a day or two if the crossmp endeavor wont show anything promising.

I am also unhappy with the crossmp parts but did not want to attempt to change that in this diff - working around it seemed like the lower-risk option. Looking forward to seeing if you can find a cleaner way.

I ran a full stress2 test with D37478.114064.patch. No problems seen.

@markj is there an easy way to convince syzkaller to play with file mounts?

In D37478#857428, @mjg wrote:

@markj is there an easy way to convince syzkaller to play with file mounts?

I'm not sure about "easy", but it should be pretty straightforward. You'd add a pseudo-syscall probably modeled after syz_mount_image() in executor/common_linux.h. That'd be a C function which wraps nmount(2), and from syzkaller's perspective it's just another system call. Then add some syzlang definitions which bind specific parameter values for syz_mount_image, e.g., by specifying "nullfs" as the filesystem type. These would go in sys/freebsd/mount.txt or so. Again, Linux has some examples here in sys/linux/filesystem.h.

If anyone would like to take this on, let me know and I can provide more details.

Thanks everyone. I'm going to rebase and land this today.