nfsport.h: Expand stat structure to encompass NFS 4.1 operations

[cem]

Summary

We index this stat structure with NFS4.1 ops, but it only had room for ops up to NFS4.0.

There is a secondary question of whether the fake ops should now start at NFSV41_NOPS instead of NFSV4OP_NOPS; I suspect the answer is "yes" (that's not included in this diff yet, though).

Coverity CID 1229963

[cem]

fs/nfs/nfsport.h

258–279

I suspect these should start at NFSV41_NOPS now rather than NFSV4OP_NOPS.

[rmacklem]

Changing this structure breaks the API used by nfsstat(8). To change it without
causing a POLA violation (breaking older nfsstat binaries working with a new
kernel), nfsstat(8) must be modified to use a different syscall and a shim that
copies the new variant of the structure to the old variant for the current syscall
must be done. I do have a flag called NFSSVC_NEWSTRUCT that can be set for
the syscall variant used by the new syscall.

There are a number of other changes that the structure should get if/when it is
revised. All fields should become uint64_t so they don't wrap around and there
is some stuff (in D1603 I think?) that also goes in.

As it stands, this patch should not go in head, imho, rick.

[cem]

Yeah, it is an ABI break, but that's what CURRENT is for. If ABI is a big concern, even on CURRENT, maybe this should go in a sysctl tree?

Not breaking the ABI means we're trampling arbitrary memory (probably a later stat, but still).

Would you prefer a patch that kept ABI, but bounds-checked srvrpccnt[] indices (dropping stats for NFS41 ops)?

[rmacklem]

It is my understanding that, even for head/current, a new kernel is supposed
to work with older userland binaries (such as nfsstat(8)) and those older
userland binaries will still work correctly. (ie. Replacing a kernel with a newer
one doesn't break anything.)

If an old nfsstat(8) binary still functions correctly when a kernel with this
patch is booted, then it would be ok. (I think this won't be the case because
the structure isn't being extended at the end of the structure.)

[rmacklem]

Yes, the code is broken. Good catch.
Just before line#774 in fs/nfsserver/nfs_nfsdsocket.c,
there needs to be a check to avoid doing #774 for the 4.1 ops.
Until then, the stats later in the structure get corrupted, but I don't think
it ever goes past the end of the structure.

I agree the structure should be updated. I've had this on my "to do" list for some
time. I would like to do all the changes at once, to avoid multiple revisions.
(Similar to what you said, I don't get paid to do this and I've been playing with the
pNFS stuff lately.)

I will commit a patch for the above soon and hopefully get around to revising the
nfsstats structure (I don't know if it will happen before the FreeBSD11 code slush).

I don't know enough about sysctl to know if it could be easily used instead of
the nfssvc(8) syscall.

[cem]

In D6113#130137, @rmacklem wrote:

Yes, the code is broken. Good catch.
Just before line#774 in fs/nfsserver/nfs_nfsdsocket.c,
there needs to be a check to avoid doing #774 for the 4.1 ops.
Until then, the stats later in the structure get corrupted, but I don't think
it ever goes past the end of the structure.

Yeah, exactly.

I agree the structure should be updated. I've had this on my "to do" list for some
time. I would like to do all the changes at once, to avoid multiple revisions.
(Similar to what you said, I don't get paid to do this and I've been playing with the
pNFS stuff lately.)

Yeah, that's reasonable. And hey, thanks for your work on NFS!

I will commit a patch for the above soon and hopefully get around to revising the
nfsstats structure (I don't know if it will happen before the FreeBSD11 code slush).

I don't know enough about sysctl to know if it could be easily used instead of
the nfssvc(8) syscall.

The advantage of sysctl is that you can easily add new stats without breaking ABI of the old ones. The disadvantage is that it would be many more syscalls (1 per stat vs 1 for the whole struct). If nfsstat is performance sensitive, that would be a problem.

A versioned stat syscall with encoders would work okay. I don't know if that's what we have today (unfamiliar with this area of the code).

[cem]

Obviated by r304026, I believe.