cvs commit: src/sys/amd64/conf GENERIC src/sys/conf NOTES options src/sys/i386/conf GENERIC src/sys/ia64/conf GENERIC src/sys/kern vfs_subr.c src/sys/pc98/conf GENERIC src/sys/powerpc/conf GENERIC src/sys/sparc64/conf GENERIC

[Robert Watson]

On Sun, 25 Jun 2006, Sergey Babkin wrote:

>  The common UID/GID space implementation. It has been discussed on -arch
>  in 1999, and there are changes to the sysctl names compared to PR,
>  according to that discussion. The description is in sys/conf/NOTES.
>  Lines in the GENERIC files are added in commented-out form.
>  I'll attach the test script I've used to PR.
>
>  PR:             kern/14584
>  Submitted by:   babkin

I would really like to have seen a change like this brought up again on arch@ 
before being committed, as I am quite uncomfortable with what has been added. 
A few specific comments:

- A passing conversation in 1999 on the arch@ mailing list is not a substitute
   for a conversation in 2006 when it comes to committing a change in 2006.  A
   lack of a "Reviewed by:" when changing critical file system access control
   code is also worrying.

- In this change, commonid.low is validated at every access control check, and
   an invalid value is silently coerced to another value.  It would be much
   preferable to reject the new value, providing immediate feedback when the
   sysctl is set, rather than quietly masking the error.

- Please don't define typedefs for kernel-only data structures, especially
   ones used only in one source file, especially given that it is referenced in
   only one place.

- There are a number of other style(9) problems with this patch, and many
   spelling and grammatical errors.

- uid_t and gid_t are unsigned 32-bit integers, and this change uses signed
   32-bit integers for the sysctl values and structure types, which will result
   in incorrect behavior in the presence of very high uid and gid values.

- Since 1999, the security.* name space has been introduced, and should be
   used instead of vfs.* for security settings.

- vaccess_acl_posix1e() has not been updated, so if you mount the file system
   with the ACLs flag enabled, the commonid code isn't executed.

- NOTES is not where documentation goes, and is not a substitute for proper
   documentation.  The comments in NOTES contain factual (and other) errors,
   such as complaining that ACLs are not supported by tar.

- The notes complain about the ugliness of ACLs, but seem not to fully
   consider the fact that pretending users are groups (and vice versa) in
   violation of widely published documentation (including our system
   documentation), breaking ls, not working properly with NFS, odd behavior in
   the presence of setuid, setgid, sticky bits, quotas, etc, are also signs of
   possible ugliness.

I would like this change to be backed out until it can be properly discussed 
and reviewed, especially in light of changes that have taken place since 1999.

Robert N M Watson
Computer Laboratory
University of Cambridge