cvs commit: src/sys/amd64/conf GENERIC src/sys/conf NOTES optionssrc/sys/i386/conf GENERIC src/sys/ia64/conf GENERIC src/sys/kernvfs_subr.c src/sys/pc98/conf GENERIC src/sys/powerpc/conf GENERICsrc/sys/sparc64/conf GENERIC

[Sergey Babkin]

Robert Watson wrote:
> 
> 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

Granted, 1999 was a long time ago. But at that time this was
a more or less formal review on arch@, not just a passing conversation.

>    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.

Agreed.
 
> - 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.

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

There are some typos but I can't see much other style(9) problems.
Well, Maxim has found some spaces at the end of lines. Some
careful reading of style(9) shows the recommendation against
using comments at #endif but even that qualifies under the
over-20-lines rule.
 
> - 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.

Good point. I disagree about the incorrect behavior though: 
by a lucky coincidence when compared to uid/gid values of 
unsigned types, the signed values would get promoted to unsigned. 
But yes, it's not right, and using the unsigned type is right.
 
> - 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.

Agreed with critique.
 
> - 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.

Could you please point where should it go? Sysctl(8) does not seem
to contain any such documentation. NOTES seems like the place
where the other kernel options are documented.
I agree that having a man page is better, and it was in the original
intention, just I couldn't find a man page to stick it to.
Make a new one?
 
> - 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

Supposedly someone who enables this option does understand that.

>    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.

Yes.
 
> 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.

OK, done.

-SB