cvs commit: src/sys/amd64/conf GENERIC src/sys/conf NOTES
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
rwatson at FreeBSD.org
Mon Jun 26 11:22:03 UTC 2006
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
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
University of Cambridge
More information about the cvs-src