Looks fine to me.

Oh, and I think just copying fid_data would fix cd9660
as well, for those who don't like __packed.

Btw, I replaced "sizeof(struct fid)" with MAXFIDSZ in the _Static_assert()
and it passed (at least for amd64).

Good catch w.r.t. the _Static_assert(). It does need to be fixed,
even if these changes don't go in.

I'll leave it someone familiar with tmpfs to decide if having
tn_gen wrap around to zero is a real concern.

Just fyi, whenever the "struct fid" changes, the file handles
change. As such, an upgrade of the NFS server requires that
all clients be unmounted/remounted.

Added upper bound check for port# as suggested
by delphi@.

Looks fine to me.

Btw, ext2fs and tarfs both are broken w.r.t. *fid size.

In D47879#1091962, @markj wrote:

In D47879#1091930, @olce wrote:

In D47879#1091920, @kib wrote:

I think more relevant question there are compilers generate proper code to access unaligned ifid_ino and ifid_start on sensitive arches.

They have to, else the packed attribute would just be useless. Bugs should be filed against those that don't.

Yes, that would be a major compiler bug, I hope..

It might be that the better fix is to re-order members without introducing __packed. There is no ABI involved.

That doesn't seem possible as len and pad must stay first.

As far as I can see, this isn't true - the layout is up to the filesystem. The filesystem just needs to guarantee that fh1 == fh2 implies that the two FIDs refer to the same file.

I can't see any NFS code which peeks into the fid structure. Perhaps @rmacklem can confirm?

As far as I know, the NFS server code does not look inside the fid structure.

Looks fine to me. You might ask manpages for a review as well.

In D47204#1076708, @brooks wrote:

This seems fine. Is this fix the result of a bug report or just something you noticed? If the latter I wonder if just fixing the documentation would be simpler if the feature never worked.

Looks ok to me.

This version looks fine to me.

I'll click Accept, although I do not think it makes the code
clearer.
Just make it clear in the commit message that there are
no semantics change/bugfix done by this commit.

In D46918#1072932, @rmacklem wrote:

In D46918#1071702, @olce wrote:

In D46918#1071324, @rmacklem wrote:

Yuck! I never noticed this in the man page (I'm not going to look and
see who the author was, it might have been me long ago;-).

So, it says that an exports mapping credential can have no groups.

The easiest fix might be to just add a couple of lines at the beginning of grouplist()
retuning failed when cr_ngroups == 0.

I think you need to ask the "collective" (on a mailing list, such as freebsd-stable@
or freebsd-current@) what should be done about this.
I.e:
--> Fix grouplist() and any others to handle the case of cr_ngroups == 0 correctly.
OR
--> Fix the exports(5) man page and do not allow the case of "no groups".

In D46918#1071326, @rmacklem wrote:

Oops, when I said grouplist() above, I meant groupmember(), which I
think is the only function that checks to see if a credential has a group
that matches the group ownership of the file (or the ACE in the ACL for
the file).

groupmember() (and realgroupmember() I added a while ago) are just the tip of the iceberg. The harder problem is that cr_groups[0] (or its alias, cr_gid) is accessed in multiple places without any checks. And even adding checks can only go so far, i.e., will cause crash (or malfunction in production if checks are under INVARIANTS) only in very specific cases that may be hard to trigger. There are simply places where not having a GID is not a possibility, especially for process credentials. struct ucred is arguably used for other purposes that may not formally require a GID (and other stuff like MAC labels), which would plead in favor of having other structures/simplified access check functions for other cases, which you had to do for the NFS interfaces. I doubt that trying to remove the one-group minimum (i.e., compulsory effective GID) requirement is worth it without also doing the latter part. Just moving the required one minimum slot from cr_groups to a really separate cr_gid field alone is probably not worth the trouble. In any case, I don't have time to devote to that for now.

Well, the NFS server does not use these mapped exported credentials in many ways.
As far as I can see at a quick look, groupmember(), which is called from VOP_ACCESS()/VOP_ACCESSX()
is about it.
Since the exports(5) man page suggests that this is a valid case, then I think the code should be fixed
to handle cr_ngroups == 0 for this specific case (not all uses of cr_ngroups).
Note that this appears to have been broken by commit 7f92e57 on Jun 20, 2009.
(Prior to that, groupmember would always return failure for cr_ngroups == 0.)

I think the first step is to fix groupmember() by adding something like:

/*
 * The NFS server can use a credential with cr_ngroups == 0 when using a mapped
 * exported credential.
 */
if (cred->cr_ngroups == 0)
    return (false);

at the beginning of groupmember().
Since the code in vfs_domount_update() sets export.ex_groups = NULL, it should
be easy to find any other places that need to be fixed by running some testing.
(I will start doing that to-day.)

Making the credentials into a grouplist of one element set to GID_NOGROUP is
not the best plan, since it is possible to have a file with a gid ownership of GID_NOGROUP.
(It can happen frequently when NFSv4 is misconfigured, so the Owner_group translates
to GID_NOGROUP, since it cannot do the correct translation.)

The above change is obviously safe and can be MFC'd.

I did test the above patch along with a "hack" that set cr_groups = NULL,
so any access would cause a crash.
The only one I got was from a totally bogus line in nfsd_excred(0 which
looks like: nd->nd_cred->cr_gid = credanon->cr_gid;
just before crsetgroups(), so it is harmless noise (left over from code meant
to work for BSDen that had a separate cr_gid.)

In D46918#1071702, @olce wrote:

In D46918#1071324, @rmacklem wrote:

Yuck! I never noticed this in the man page (I'm not going to look and
see who the author was, it might have been me long ago;-).

So, it says that an exports mapping credential can have no groups.

The easiest fix might be to just add a couple of lines at the beginning of grouplist()
retuning failed when cr_ngroups == 0.

I think you need to ask the "collective" (on a mailing list, such as freebsd-stable@
or freebsd-current@) what should be done about this.
I.e:
--> Fix grouplist() and any others to handle the case of cr_ngroups == 0 correctly.
OR
--> Fix the exports(5) man page and do not allow the case of "no groups".

In D46918#1071326, @rmacklem wrote:

Oops, when I said grouplist() above, I meant groupmember(), which I
think is the only function that checks to see if a credential has a group
that matches the group ownership of the file (or the ACE in the ACL for
the file).

groupmember() (and realgroupmember() I added a while ago) are just the tip of the iceberg. The harder problem is that cr_groups[0] (or its alias, cr_gid) is accessed in multiple places without any checks. And even adding checks can only go so far, i.e., will cause crash (or malfunction in production if checks are under INVARIANTS) only in very specific cases that may be hard to trigger. There are simply places where not having a GID is not a possibility, especially for process credentials. struct ucred is arguably used for other purposes that may not formally require a GID (and other stuff like MAC labels), which would plead in favor of having other structures/simplified access check functions for other cases, which you had to do for the NFS interfaces. I doubt that trying to remove the one-group minimum (i.e., compulsory effective GID) requirement is worth it without also doing the latter part. Just moving the required one minimum slot from cr_groups to a really separate cr_gid field alone is probably not worth the trouble. In any case, I don't have time to devote to that for now.

Looks fine to me, since ngroups_max can never
be set < NGROUPS_MAX.

In D46918#1071324, @rmacklem wrote:

In D46918#1070838, @olce wrote:

In D46918#1070158, @rmacklem wrote:

You claim there are bugs that result in no groups
(cr_ngroups == 0), but you do not show a specific
test case that finds this or describe exactly how it
happens.

See, e.g., current processing of a list uid:gid:.. when just uid: is passed (no GID, but a colon at the end), and the below patch. This possibility is incidentally documented as the way to specify no groups in the exports(5) man page (under -maproot).

Yuck! I never noticed this in the man page (I'm not going to look and
see who the author was, it might have been me long ago;-).

So, it says that an exports mapping credential can have no groups.

The easiest fix might be to just add a couple of lines at the beginning of grouplist()
retuning failed when cr_ngroups == 0.

I think you need to ask the "collective" (on a mailing list, such as freebsd-stable@
or freebsd-current@) what should be done about this.
I.e:
--> Fix grouplist() and any others to handle the case of cr_ngroups == 0 correctly.
OR
--> Fix the exports(5) man page and do not allow the case of "no groups".

Oops, when I said grouplist() above, I meant groupmember(), which I
think is the only function that checks to see if a credential has a group
that matches the group ownership of the file (or the ACE in the ACL for
the file).

In D46918#1070838, @olce wrote:

In D46918#1070158, @rmacklem wrote:

You claim there are bugs that result in no groups
(cr_ngroups == 0), but you do not show a specific
test case that finds this or describe exactly how it
happens.

See, e.g., current processing of a list uid:gid:.. when just uid: is passed (no GID, but a colon at the end), and the below patch. This possibility is incidentally documented as the way to specify no groups in the exports(5) man page (under -maproot).

Yuck! I never noticed this in the man page (I'm not going to look and
see who the author was, it might have been me long ago;-).

In D46920#1070863, @olce wrote:

In D46920#1069916, @rmacklem wrote:

Sun RPC has a gid plus a list of additional groups (MAX 16) on the wire.
Broken code existed for a long time, that did the following:

cr_gid = gid
copy groups list

Step two overwrote the first gid, since cr_gid was just cr_groups[0] for BSD

The workaround was to put the same gid# in gid and element 0 of the Sun RPC
on-the-wire gid list.

I wasn't aware, thanks for providing these elements. That said, what you describe is only a problem if that Sun RPC code is used verbatim on a BSD, which has not been the case for a long time on FreeBSD AFAICT, and the remote doesn't put the same GID in the first element of the groups array.

Are any of these broken systems still out there (several very old variants of *BSD)?
If they are, you can easily break interoperability if you take this "put the same gid
in gid and element 0 of the gid list on the wire".

I see. However, I think the situation here in mountd is the opposite: We were just getting rid of one occurrence of a GID if both present as the effective GID (slot 0) and the first supplementary group (slot 1), and my point is that doing so is unnecessary (as explained in the commit message).

--> The code that looks for and gets rid of the duplicate on the receiving side

probably doesn't matter.

Not sure why you refer to the receiving side here. mountd, AFAICT, is always the sending side for such information. Or are you referring to some implementations of getgrouplist() such as NIS, or some other method?

Yes. The historical background wass just FYI for whoever was reading this
and wondering why the code would check for a duplicate.
I agree with you that the duplicate is unlikely to still exist in passwd/group databases
used by FreeBSD, so removing the code seems reasonable and does simplify the
code slightly.

Remember this code is setting up exports. The "credentials" are created
from the exports in the kernel. It has nothing to do with setuid()/setgid()
etc. for processes.

You claim there are bugs that result in no groups
(cr_ngroups == 0), but you do not show a specific
test case that finds this or describe exactly how it
happens.
--> If there is such a case (I am not aware of one),

we should try to isolate/fix that such that there
is always at least one group in the credentials.

(There is always at least one group in the credentials
that come on on-the-wire, since there is a separate gid
in the Sun RPC AUTH_UNIX authenticator from the
list of additional groups.)

I will look at the patches later, but just in case you are not aware of
why a lot of this weirdness exists in the code...

Looks ok to me.

Just fyi, I have created a Pull Request for this on OpenZFS,
so please review it over there.