CFT: final patches for NGROUPS>>16

From: Brooks Davis <brooks_at_freebsd.org>
Date: Wed, 17 Jun 2009 19:04:08 -0500
Please find attached three patches which result in raising NGROUPS to
1024, making programs in the base system immune to changing values of
NGROUPS, and pave the way for NGROUPS to be boot time configurable.

The first two patches (ngroups-catman.diff and ngroups-posix.diff)
are userland cleanups.  The third (ngroups-main.diff) is the core of
the change and primairly in the kernel.  It is strongly based on work
contributed by Isilon Systems.  The proposed commit messages appear to
the bottom of this message.

I've been running the basic code on my FreeBSD laptop for a while and
I've tested basic NFS mounts, but more areas need testing before this
hits a release.  Key things to hit include:
 - NFS mount and export
 - IPFW uid, gid, and jail rules
 - portalfs (ideally portalfs should be extended to not truncate groups)

In practice, expect appliations to not care about this change unless
they are run with more than 16 groups.  If that happens, calls to
getgroups() will fail if they use statically sized buffers recompilation
should fix them.

Please test, review, etc.  I'd like to get this in before code freeze if
at all possible.

-- Brooks

ngroups-catman.diff:

  When checking if we can write to a file, use access() instead of a
  manual permission check based on stat output.  Also, get rid of the
  executability check since it is not used.

  MFC after:	2 weeks

ngroups-posix.diff

  In preparation for raising NGROUPS and NGROUPS_MAX, change base
  system callers of getgroups(), getgrouplist(), and setgroups() to
  allocate buffers dynamically.  Specifically, allocate a buffer of size
  sysconf(_SC_NGROUPS_MAX)+1 (+2 in a few cases to allow for overflow).

  This (or similar gymnastics) is required for the code to actually follow
  the POSIX.1-2008 specification where {NGROUPS_MAX} may differ at runtime
  and where getgroups may return {NGROUPS_MAX}+1 results on systems like
  FreeBSD which include the primary group.

  In id(1), don't pointlessly add the primary group to the list of all
  groups, it is always the first result from getgroups().  In principle
  the old code was more portable, but this was only done in one of the two
  places where getgroups() was called to the overall effect was pointless.

  Document the actual POSIX requirements in the getgroups(2) and
  setgroups(2) manpages.  We do not yet support a dynamic NGROUPS, but we
  may in the future.

  MFC after:	2 weeks

ngroups-main.diff:

  Rework the credential code to support larger values of NGROUPS and
  NGROUPS_MAX, eliminate ABI dependencies on them, and raise the to 1024
  and 1023 respectively.  (Previously they were equal, but under a close
  reading of POSIX, NGROUPS_MAX was defined to be too large by 1 since it
  is the number of supplemental groups, not total number of groups.)

  The bulk of the change consists of converting the struct ucred member
  cr_groups from a static array to a pointer.  Do the equivalent in
  kinfo_proc.

  Introduce new interfaces crcopysafe() and crsetgroups() for duplicating
  a process credential before modifying it and for setting group lists
  respectively.  Both interfaces take care for the details of allocating
  groups array. crsetgroups() takes care of truncating the group list
  to the current maximum (NGROUPS) if necessary.  In the future,
  crsetgroups() may be responsible for insuring invariants such as sorting
  the supplemental groups to allow groupmember() to be implemented as a
  binary search.

  Because we can not change struct xucred without breaking application
  ABIs, we leave it alone and introduce a new XU_NGROUPS value which is
  always 16 and is to be used or NGRPS as appropriate for things such as
  NFS which need to use no more than 16 groups.  When feasible, truncate
  the group list rather than generating an error.

  Minor changes:
    - Reduce the number of hand rolled versions of groupmember().
    - Do not assign to both cr_gid and cr_groups[0].
    - Modify ipfw to cache ucreds instead of part of their contents since
      they are immutable once referenced by more than one entity.

  Submitted by:	Isilon Systems
  X-MFC after:	never

Received on Wed Jun 17 2009 - 22:03:57 UTC

This archive was generated by hypermail 2.4.0 : Wed May 19 2021 - 11:39:50 UTC