Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
Looks good, however maybe it's time to document both st_bsdflags member and its respective values in the stat(2) as non-portable interface?
something along those lines:
diff --git a/lib/libc/sys/stat.2 b/lib/libc/sys/stat.2 index 6b2f2a7c7eab..472896f8821f 100644 --- a/lib/libc/sys/stat.2 +++ b/lib/libc/sys/stat.2 @@ -27,7 +27,7 @@ .\" .\" @(#)stat.2 8.4 (Berkeley) 5/1/95 .\" -.Dd March 30, 2021 +.Dd September 22, 2026 .Dt STAT 2 .Os .Sh NAME @@ -159,7 +159,7 @@ and into which information is placed concerning the file. The fields of .Vt "struct stat" related to the file system are: -.Bl -tag -width ".Va st_nlink" +.Bl -tag -width ".Va st_bsdflags" .It Va st_dev Numeric ID of the device containing the file. .It Va st_ino @@ -171,6 +171,15 @@ Flags enabled for the file. See .Xr chflags 2 for the list of flags and their description. +.It Va st_bsdflags +Miscellaneous system flags for the file. +The following bits may be set: +.Bl -tag -width Dv +.It Dv SFBSD_NAMEDATTR Pq Li 0x0001 +The file is a named attribute or a named attribute directory. +.It Dv SFBSD_MNTROOT Pq Li 0x0002 +The file is the root directory of a mounted file system. +.El .El .Pp The @@ -467,6 +476,16 @@ system calls are expected to conform to The .Fn fstatat system call follows The Open Group Extended API Set 2 specification. +.Pp +The +.Va st_bsdflags +field and the +.Dv SFBSD_NAMEDATTR +and +.Dv SFBSD_MNTROOT +flags are non-portable +.Fx +interfaces. .Sh HISTORY The .Fn stat
Sure, documenting st_bsdflags would be useful. I suggest you to create a review for the diff, I have some notes.
It's not particularly pretty that this common code is duplicated, even if there are only a few calls to VOP_STAT() in the tree. Could you please put it instead in a new vop_stat_post() function? That would prevent possible future bugs when new calls to VOP_STAT() are introduced.
post hook there would be a significant obfuscation. This one-liner is fine IMO, it happens just in the top-level syscall code.
All people working with the VFS are (or really should be) aware there can be pre- and post- hooks for any VOP_*() call. SFBSD_MNTROOT is expected to always be set on VV_ROOT. A post hook is the most suitable place for that. I'd trade an additional small effort on the part of readers versus the elimination of a consistency risk for other VOP_() callers anytime. And not only for VOP_STAT(), but for all VOP_*() calls, as much as possible.
People know about the VOP hooks, but having them to remember that each time VOP_STAT is called, something relevant is done behind the scene is not reasonable. It is reasonable to use VOP hooks to provide the consistent large feature, two good examples are VOP locking assertions and inotify/kqueue. But having small scattered details hidden from the view of the person reading the code puts the landmine.
P.S. Perhaps bump __FreeBSD_version? Since there is no other way to probe if the kernel supports this feature or not.
This might not work in a chroot or jail.
| lib/libsys/stat.2 | ||
|---|---|---|
| 369 | ||
| sys/fs/nullfs/null_vnops.c | ||
| 599 ↗ | (On Diff #187522) | null_nodeget() will set VV_ROOT on the upper vnode that corresponds to the null lowerrootvp: if (lowervp == MOUNTTONULLMOUNT(mp)->nullm_lowerrootvp)
vp->v_vflag |= VV_ROOT;That being the case, why do we need to set the flag here? It looks like the tests in kern_statat() and vn_statfile() are sufficient. |
Then a more elaborate version of it, like calling getmntinfo(3) and then trying stat() on one of the paths. But the question is why it is needed at all? We do not support running new userspace on old kernel in any way.
| sys/fs/nullfs/null_vnops.c | ||
|---|---|---|
| 599 ↗ | (On Diff #187522) | I did it from caution. |
Perhaps yes, but it is more complicated. It is not useful to check for VV_ROOT there, because then we get the root of the mounted fs and not the mount point. Perhaps if modifying fhtat(), it should be a check for VIRF_MOUNTPOINT. Do we want such semantic?
Sooner-than-expected confirmation of my previous comments.
Perhaps yes, but it is more complicated. It is not useful to check for VV_ROOT there, because then we get the root of the mounted fs and not the mount point. Perhaps if modifying fhtat(), it should be a check for VIRF_MOUNTPOINT. Do we want such semantic?
The check on VIRF_MOUNTPOINT is indeed necessary if the handle points to an underlying vnode, but the check on VV_ROOT should be performed also, if for anything for consistency with fstatat().
No, VV_ROOT vnode is not a mountpoint when it is instantiated by ino number, instead of lookup. It is below mountpoint.
That's certainly true strictly speaking, but was not my point.
I think this can cause confusion to users obtaining first a handle and then calling fhstat() versus calling stat() because they would be inconsistent if both are performed after the mount operation has taken place.
It may well be better to change the semantics to: Reports a mount point or a filesystem root, which would avoid the inconsistency (and corresponds to checking VV_ROOT also).
| lib/libsys/fhopen.2 | ||
|---|---|---|
| 102 ↗ | (On Diff #187585) | |
Reporting root of a file system is different from reporting a mount point. And the request was to provide the feature to identify mount points.
Also, since we are in the fhstat(2) territory, it is by definition filesystem-dependent. Then, there are ways to identify root from the already collected information. For instance, for UFS you would check ino == 2 (and you need to know fs specifics if only to construct the handle).
AFAICT, the request comes from the need expressed in D59906, where this distinction does not matter (and cannot matter, since mountpoint(1) operates on paths).
So I'd much prefer that we offer something with consistent observable behavior to users.
Also, since we are in the fhstat(2) territory, it is by definition filesystem-dependent. Then, there are ways to identify root from the already collected information. For instance, for UFS you would check ino == 2
I guess it wouldn't be too hard to provide a specific flag for this if need be.
(and you need to know fs specifics if only to construct the handle).
No, you can get a handle with getfh() and friends, and if you're using NFS, you don't even need to be root to obtain one.
I think I agree: the flag is named SFBSD_MNTROOT, so the distinction between "mountpoint" and "filesystem root" is already unclear. Either the name should be SFBSD_MOUNTPOINT or we should be more flexible.
No, you can get a handle with getfh() and friends, and if you're using NFS, you don't even need to be root to obtain one.
You need PRIV_VFS_GETFH in any case, so I don't see how this is true.
| sys/kern/vfs_vnops.c | ||
|---|---|---|
| 1841 | What if VIRF_MOUNTPOINT is set here? i.e., someone opened a directory and then later a filesystem was mounted on that directory. | |
| sys/kern/vfs_vnops.c | ||
|---|---|---|
| 1841 | You mean, the dirfd is actually covered by some mount later? I believe it would be semantically wrong to report MNTROOT/MNTPOINT since dirfd looks below. | |
The NFS server runs as root, and will provide a handle to any client that it considers allowed to browse the corresponding path.
Then, a stat(2) on the client will return a different value than a stat(2) on the local machine.
EDIT: Mmm... but this shouldn't be a problem inasmuch as our specific flags are not sent over NFS.
Only talking about the "you don't need to be root part".
Unfortunately, the inconsistency of results between doing the stat on the handle or the path directly for root remains. Can it be addressed properly?
EDIT: No, that would be one more special case, and at the moment I don't see why we would want to treat / differently than any other path. Worse, doing so would introduce a discrepancy with stat("/") in jails with respect to the host for jails rooted in a filesystem's root that is not the host's /.
The inconsistency is this:
- Mount something on, let's say, /mnt (and possibly mount more filesystems on top of it, that doesn't change what's next).
- Do stat("/mnt"), this returns SFBSD_MNTPOINT.
- Do fhget("/mnt") and then fhstat() on the result, and SFBSD_MNTPOINT is not returned.
This is a problem because we naturally expect that 2 and 3 would give the same result, as they actually do for any other values reported by stat() and fhstat(). In other words, the result should depend on the vnode only, and not the way it is accessed. Since we want stat("/mnt") to return SFBSD_MNTPOINT, we have no choice but ensure fhstat(/mnt) does the same. Consequently, we have to test on VV_ROOT, which implies that SFBSD_MNTPOINT also reports filesystem roots. Testing VIRF_MOUNTPOINT cannot be an alternative because it will never be set on vnodes obtained through path resolution.
Of course, if you had obtained a handle on /mnt before step 1, then fhstat() shouldn't return the same values as stat() after the mount, as the handle points at the covered vnode while the path refers to the covering one. Since the covered vnode cannot be accessed through the initial path anymore, in this case there's no need of consistency and there's no contradiction with the previous point. For handles, then, it makes sense to also test the presence of VIRF_MOUNTPOINT.
I agree that reporting SFBSD_MNTPOINT on a vnode that's not actually a mount point, but is at the end of a covered -> covering vnodes chain, is a conceptual problem *if* the flag is restricted to marking only mount points.
To have the best of both worlds, I propose to instead have a flag that only indicates whether a vnode is a mount point or the root of a filesystem; it could be named, e.g., SFBSD_MNTCROSSING. Then, the conceptual problem of the previous paragraph disappears, steps 2 and 3 above can agree, and mountpoint(1) can still be implemented on top of that.
Are you OK with that (SFBSD_MNTPOINT => SFBSD_MOUNTCROSSING, systematic test on VV_ROOT and VIRF_MOUNTPOINT)?
(For properly distinguishing between a mount point and a filesystem's root, we would need to introduce a new flag to fstatat(2) saying not to climb mounts in the last component, and just return SFBSD_MNTPOINT in the case of VIRF_MOUNTPOINT only. SFBSD_MNTPOINT would never be returned on a regular stat(2). mountpoint(1) would need to use the new fstatat(2) flag. SFBSD_MNTROOT would correspond to VV_ROOT only. I'm perfectly fine with all that, but is it worth it? For the need that started all this, it does not seem so. I haven't given a thought on possible future scenarios where this may be useful.)
What is fhget()?
Ignoring this question, if you get handle for "/mnt" (covered vnode), it will report SFBSD_MNTPOINT. If you get the handle for the result of "/mnt" lookup, of course there is nothing mounted on it, so the flag is not returned.
as they actually do for any other values reported by stat() and fhstat(). In other words, the result should depend on the vnode only, and not the way it is accessed. Since we want stat("/mnt") to return SFBSD_MNTPOINT, we have no choice but ensure fhstat(/mnt) does the same. > Consequently, we have to test on VV_ROOT, which implies that SFBSD_MNTPOINT also reports filesystem roots. Testing VIRF_MOUNTPOINT cannot be an alternative because it will never be set on vnodes obtained through path resolution.
Of course, if you had obtained a handle on /mnt before step 1, then fhstat() shouldn't return the same values as stat() after the mount, as the handle points at the covered vnode while the path refers to the covering one. Since the covered vnode cannot be accessed through the initial path anymore, in this case there's no need of consistency and there's no contradiction with the previous point. For handles, then, it makes sense to also test the presence of VIRF_MOUNTPOINT.
I agree that reporting SFBSD_MNTPOINT on a vnode that's not actually a mount point, but is at the end of a covered -> covering vnodes chain, is a conceptual problem *if* the flag is restricted to marking only mount points.
To have the best of both worlds, I propose to instead have a flag that only indicates whether a vnode is a mount point or the root of a filesystem; it could be named, e.g., SFBSD_MNTCROSSING. Then, the conceptual problem of the previous paragraph disappears, steps 2 and 3 above can agree, and mountpoint(1) can still be implemented on top of that.
Are you OK with that (SFBSD_MNTPOINT => SFBSD_MOUNTCROSSING, systematic test on VV_ROOT and VIRF_MOUNTPOINT)?
No, it is useless overcomplication. The 'mount point' is clear and known concept, it is formed by creating a junction point from two vnodes. The covered vnode is reported by the flag. Mixing it with 'fs root' is not needed.
(For properly distinguishing between a mount point and a filesystem's root, we would need to introduce a new flag to fstatat(2) saying not to climb mounts in the last component, and just return SFBSD_MNTPOINT in the case of VIRF_MOUNTPOINT only. SFBSD_MNTPOINT would never be returned on a regular stat(2). mountpoint(1) would need to use the new fstatat(2) flag. SFBSD_MNTROOT would correspond to VV_ROOT only. I'm perfectly fine with all that, but is it worth it? For the need that started all this, it does not seem so. I haven't given a thought on possible future scenarios where this may be useful.)
So we first create the problem, and then add more stuff to cover it. There is no need in that complication.
getfh(2)!
Ignoring this question, if you get handle for "/mnt" (covered vnode), it will report SFBSD_MNTPOINT. If you get the handle for the result of "/mnt" lookup, of course there is nothing mounted on it, so the flag is not returned.
Of course, except that's just not the point. When you do stat("/mnt"), you get a report on the vnode which is the result of the /mnt lookup. Setting SFBSD_MNTPOINT in the result is breaking this very invariant, which is the cause of the differing results between steps 2 and 3 above.
The 'mount point' is clear and known concept, it is formed by creating a junction point from two vnodes.
"Mount crossing" is also clear, it's basically the same as "mount point" except in both directions (in other words, it designates both the covered and covering vnodes), and should be very familiar to people used to the "mount point" concept.
The covered vnode is reported by the flag.
With stat(2), the covering vnode is also reported by the flag, but not with fhstat(2).
Mixing it with 'fs root' is not needed.
It is necessary to avoid the inconsistency, which is a bug.
(For properly distinguishing between a mount point and a filesystem's root, we would need to introduce a new flag to fstatat(2) saying not to climb mounts in the last component, and just return SFBSD_MNTPOINT in the case of VIRF_MOUNTPOINT only. SFBSD_MNTPOINT would never be returned on a regular stat(2). mountpoint(1) would need to use the new fstatat(2) flag. SFBSD_MNTROOT would correspond to VV_ROOT only. I'm perfectly fine with all that, but is it worth it? For the need that started all this, it does not seem so. I haven't given a thought on possible future scenarios where this may be useful.)
So we first create the problem, and then add more stuff to cover it. There is no need in that complication.
It's the opposite. Not understanding or not wanting to understand a problem does not make it disappear.
It's not a huge problem for sure, but it is also trivial to fix. The rename + changes of test I suggested take less than a minute. If you really want to distinguish both cases, then I also proposed a way (this one is more involved for sure).
As is, the current change is not acceptable.
I don't want committing new features with avoidable known bugs to become the new normal in FreeBSD.
There is no bug that you claim. Behavior for normal lookup and and for operations directly on the inode number (AKA fh) have to differ because of their nature, where one operates over the global file namespace, and another acts on the inode identifiers within single filesystem.
Amount of bikeshedding you created from the simple and reasonable feature is astonishing but not surprising. If there is no technical bugs pointed out in the change, I am going to commit this in short time.
It has been explained to death, and I'm not going to start again. "There’s none so blind as those who will not see.”
Behavior for normal lookup and and for operations directly on the inode number (AKA fh) have to differ because of their nature, where one operates over the global file namespace, and another acts on the inode identifiers within single filesystem.
That's a fallacious argument. Whether through a pathname or a file handle, if in the end you get to the same vnode, the fh*() and *() functions then all work the same. In what has been exposed so far, there is absolutely no compelling reason to break that precedent, which would cause the discrepancy exposed above.
Amount of bikeshedding you created from the simple and reasonable feature is astonishing but not surprising.
The proposed implementation/design is wrong.
What is really astonishing is that you prefer to resist the simple changes I'm asking for correctness, making us lose more time in the process, instead of just taking them and be done with it. May I remind you that the initial need is mountpoint(1), and that it can be served as well even if not distinguishing covering and covered vnodes. But I'm not surprised either, since it's not the first time you're acting in a very stubborn manner.
If there is no technical bugs pointed out in the change, I am going to commit this in short time.
There's a bug, whether you recognize it or not (either in the implementation or in the design). Please stop this childish attitude, taking some time to recogne what I've pointed out and engaging the discussion with real arguments if you have some.
I have clearly expressed that I object to this change as is, as well as explaining acceptable alternatives, one of which takes less than a minute to implement. If you commit this as is nonetheless, I'll ask for it to be reverted.
So there is an inconsistency of course: getfh()+fhstat() on a mountpoint will not return SFBSD_MNTPOINT, but open()+fstat() will. The inconsistency itself isn't automatically a bug, and in this case I don't think it is, as there's zero reason to use getfh()+fhstat() instead of open()+fstat() or just stat(). There's basically one user of fhstat() in the base system, stat -H, which does not operate on a path anyway. getfh() is mainly used by mountd, but today it uses statfs() (not fhstatfs()) to determine whether an exports(5) entry corresponds to a mountpoint (as it needs to check this unless -alldirs is set). I'm pretty convinced that the inconsistency will not matter in practice, and there's no invariant which says they have to agree. (And there are existing inconsistencies; note for instance that null_stat() translates st_dev from the lower vnode, whereas null_vptofh() does not perform translation, so nullfs mounts are effectively invisible in the fhandle space; or, note that getfh() simply isn't usable as an unprivileged user or in a jail by default.)
That being the case, I think it's okay to assign different meanings to SFBSD_MNTPOINT depending on whether one is operating on a path, an fd, or a fhandle. I am skeptical now that it is useful to export SFBSD_MNTPOINT from fhstat() at all, and I regret suggesting it. In particular, the interaction with nullfs seems weird: if a vnode is covered by a nullfs mount, fhstat() will report SFBSD_MNTPOINT, but the covering mount may or may not have a different fsid.
Without some concrete use-case for fhstat() returning SFBSD_MNTPOINT, I propose we simply not export it.
As a side note, it is disappointing to see snipes like
I don't want committing new features with avoidable known bugs to become the new normal in FreeBSD.
and
Amount of bikeshedding you created from the simple and reasonable feature is astonishing but not surprising.
appear in a technical discussion. They don't help with anything. If anyone needs to vent, please do it elsewhere.
| sys/kern/vfs_vnops.c | ||
|---|---|---|
| 1841 |
Yes.
In other words, because openat(dirfd, ...) will perform lookups relative to the covered vnode, not the root of the filesystem mounted over the dir. Ok. | |