mountd(8): parsecred(): Groups limit: NGROUPS_MAX => NGROUPS_MAX + 1

[olce]

Summary

Lift this unnecessary limitation.

[emaste]

Presumably this can be removed because getgrouplist will set ngroups at most NGROUPS_MAX, returning -1 if it would be too large (and ngroups was > NGROUPS_MAX on input). IMO we should replace this check with an assertion in that case as documentation.

[rmacklem]

usr.sbin/mountd/mountd.c

3653

Taking this out breaks the code. Note that ngroups is
set to NGROUPS_MAX + 1, so it is possible that it is
still NGROUPS_MAX+1.

If you are going to remove the code that takes out the
duplicate (that is another patch you presented and I will
comment on it there), then line#3648 should set

ngroups = NGROUPS_MAX

and

gid_t groups[NGROUPS_MAX];

However, this should only be done if the other patch
has been applied.

[olce]

In D46921#1069880, @emaste wrote:

Presumably this can be removed because getgrouplist will set ngroups at most NGROUPS_MAX, returning -1 if it would be too large (and ngroups was > NGROUPS_MAX on input). IMO we should replace this check with an assertion in that case as documentation.

No, this can be removed because storage is allocated for NGROUPS_MAX + 1 elements. ngroups is used by getgrouplist() in input to know what the size of the array is. In output, it returns the number of elements it has filled, or would have liked to fill in case there's not enough room. In this latter case (only), it returns 1. So, if getgrouplist() returned 1, we have to reset ngroups to the maximum value, which is already done in the if above. If 0 was returned instead, we have nothing to do, since getgrouplist() updated ngroups to the number of valid elements in the array. In both cases, setting ngroups in the removed if is thus superfluous, and even wrong given that we have storage for NGROUPS_MAX + 1 elements.

usr.sbin/mountd/mountd.c

3653

I don't understand what you are saying. The number of slots should stay at NGROUPS_MAX + 1. Why change it?

[emaste]

Oh - I was mistaken in my read of __getgroupmembership - maxgrp will be the passed in NGROUPS_MAX + 1

[rmacklem]

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.

usr.sbin/mountd/mountd.c

3653

The reason the size is set to NGROUPS_MAX + 1 is
that the deduplication can reduce the size by one.
(NGROUPS_MAX is the maximum that can be passed
into the kernel for exports)

Since you are removing deduplication, asking for
NGROUPS_MAX + 1 and then throwing it away
is just slightly less efficient and makes the code
more confusing.
--> Someone else down the road wil go "Why did the

code ask for up to NGROUPS_MAX + 1 and then
limit it to NGROUPS_MAX?

[olce]

usr.sbin/mountd/mountd.c

3653

NGROUPS_MAX is the maximum that can be passed into the kernel for exports

Ok, then that's the "problem", such limit is too conservative. I'll upload another revision to fix the corresponding part.

[rmacklem]

usr.sbin/mountd/mountd.c

3653

Yea. The kernel code is in vfs_mount.c.
Just search for NGROUPS_MAX and you'll
find it.

I think that should be replaced by

ngroups_max + 1

(It's confusing, since NGROUPS is now
NGROUPS_MAX + 1 and ngroups_max
is a tunable, so NGROUPS_MAX is not
only a default for ngroups_max.)

Then it exposes ngroups_max as a
tunable called X.ngroups.

This code should probably get the tunable
and use that instead of NGROUPS_MAX (or NGROUPS).

[olce]

usr.sbin/mountd/mountd.c

3653

(Late send of this comment. Should have been sent when D47013 was added as child.)

New revision that lifts this limitation: D47013.

To clear any confustion, you might want to have a look at both at D46913 on the relationship between NGROUPS_MAX and our tunable. The standard way to get this tunable according to POSIX is sysconf(_SC_NGROUPS_MAX), and is what is used in the new D47016.

It is unfortunate that POSIX defines {NGROUPS_MAX} as the maximum number of *supplementary groups*, and that we have the effective GID as the first position in the groups arrays internally, but we'll have to live with that I'd say (we might want to change the second part, but I'm not sure it's worth the trouble).