Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
Since st_filerev provides similar information to st_gen, should it also be guarded by PRIV_VFS_GENERATION in vop_stat_helper_post? And does it warrant mention in stat.2 ?
I do not know why st_gen is considered privileged information, but it might have
been that knowing it is useful when trying to fake a file handle?
st__filerev is not useful that way. It is really about the same as mtime and ctime, I think?
| sys/kern/kern_descrip.c | ||
|---|---|---|
| 1623 | I believe kern_statat() needs a similar initialization. | |
| sys/kern/kern_descrip.c | ||
|---|---|---|
| 1623 | kern_statat() calls VOP_STAT() and not fo_stat(). I believed that I converted all existing VOP_STATs (ufs, nullfs, default) but apparently missed tmpfs. If adding to kern_statat(), perhaps vop_stat_helper_pre() is more useful place, but I think it is enough to fix tmpfs. | |
| sys/kern/vfs_default.c | ||
|---|---|---|
| 1591 | We need to initialize va_filerev before calling VOP_GETATTR above. | |
| sys/kern/vfs_default.c | ||
|---|---|---|
| 1591 | Aren't filesystems already handle va_filerev? | |
| sys/kern/vfs_default.c | ||
|---|---|---|
| 1591 | Sorry, I don't follow. Suppose I fstatat() a file on a filesystem that uses vop_stdstat(), e.g., fusefs. vap->va_filerev will be left uninitialized since fuse_vnop_getattr() doesn't know about the new field, and it will be copied to st_filerev, so we copy uninitialized bytes to userspace. See the comment "Initialize defaults for new and unusual fields..." at the beginning of this function. | |
Out of curiosity, what's the need behind exposing this info? FYI, it seems va_filerev has been correct on UFS, ZFS and p9fs only, and filesystems using init_va_filerev() seem to set it to a dubious value.
| sys/kern/kern_descrip.c | ||
|---|---|---|
| 1623 | This init here is for the sake of non-vnode files, a situation that can't happen in kern_statat(). | |
| sys/kern/vfs_default.c | ||
| 1591 | It's true that va_filerev has existed from the start (4.4BSD import), but since it isn't well known, I'd prefer it to be initialized for sure one way or the other. Under the "Initialize defaults for new and unusual fields" comment is one possibility. However, some FSes (fuse and ZFS through zfsctl_root_getattr()) do not set va_filerev in VOP_GETATTR(). So perhaps the best course is instead to initialize it to 0 in a pre routine for VOP_GETATTR(). | |
| sys/kern/vfs_default.c | ||
|---|---|---|
| 1591 | Oh, I missed that va_filerev is not new. But yes, some filesystems still do not populate it, so we should either fix them or add this default value. | |
There are two motivations:
- To use in regression tests, to ensure that file systems like fusefs set va_filerev correctly.
- To use for userspace NFS servers. None currently known can make use of this information, but future ones might.
| sys/kern/vfs_default.c | ||
|---|---|---|
| 1591 | I qualify this as a bug in fusefs, in fact. All (as far as I see) other fses do set va_filerev in VOP_GETATTR(). Ok, lets add this line to make the change go in. | |
Zero st_filerev in vop_stat_helper_pre().
Zero va_filerev before calling VOP_GETATTR() in vfs_stdstat()
I'm OK with these changes as they are.
That said, even if I agree there is, e.g., a bug in fusefs as mentioned in inline comments, I'm still wondering how to prevent non-initialized fields bugs in the VFS in a more general fashion. It's unfortunate that C does not allow to specify a default initializer per type. Maybe a follow-up attempt creating a new pre for VOP_GETATTR() would do good, although it will show up as not optimal as the compiler can't elide useless pre-initialization in it (for fields that are effectively filled by the FS' VOP_GETATTR() implementation). We would need a slightly different implementation for it to be able to perform such elisions.
Perhaps some background w.r.t. va_filerev is in order.
I think I added it when I added NFSv4 support to FreeBSD
around FreeBSD 7, although I haven't looked in the commit logs.
It is specifically for NFSv4's change attribute.
change is defined as a value that changes whenever the file's
data or metadata changes. The RFCs do not define how it changes,
just that the value should not be the same.
To implement this correctly it needs to be in the on-disk i-node or
similar (it should persist across cases where the i-node falls out of memory
and when the server reboots).
For file systems that do not store it on-disk, it can only be approximated.
For read-only exports, it need only remain the same, so setting it to 0
is appropriate.
For read-write exports, ctime is a good approximation if that lives on-disk.
Otherwise, setting it to a value that always changes when the i-node is created
in memory is probably the best you can do. (That is what init_va_filerev() does when
it sets it to binuptime.) This makes it look like the file has changed when it might not
have, but is probably preferable to not showing a change when one occurs.
fuse should probably do the best it can, given the above.
In summary, there is no one correct initialization if it is not stored on-disk.