Date: Mon, 26 Jun 2006 00:13:24 +0100 (BST) From: Robert Watson <rwatson@FreeBSD.org> To: Sergey Babkin <babkin@FreeBSD.org> Cc: cvs-src@FreeBSD.org, src-committers@FreeBSD.org, cvs-all@FreeBSD.org Subject: Re: 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 Message-ID: <20060625224804.G8158@fledge.watson.org> In-Reply-To: <200606251837.k5PIbjhd052998@repoman.freebsd.org> References: <200606251837.k5PIbjhd052998@repoman.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20060625224804.G8158>