On Mon, 3 Sep 2012, Andrey Zonov wrote:
On 9/3/12 1:20 AM, Garrett Cooper wrote:
On Sun, Sep 2, 2012 at 2:16 PM, Garrett Cooper <yaneg...@gmail.com> wrote:
On Sun, Sep 2, 2012 at 10:39 AM, Andrey Zonov <z...@freebsd.org> wrote:
Author: zont
Date: Sun Sep 2 17:39:02 2012
New Revision: 240026
URL: http://svn.freebsd.org/changeset/base/240026
Log:
- Make kern.maxtsiz, kern.dfldsiz, kern.maxdsiz, kern.dflssiz, kern.maxssiz
and kern.sgrowsiz sysctls writable.
Approved by: kib (mentor)
Modified:
head/sys/kern/subr_param.c
...
Please add some basic sanity checking to init_param1 -- there's
absolutely nothing preventing me from passing in values <= 0 or other
Isn't that an old feature, not affected by this commit? init_param1()
runs long before sysctl(8) can run to use the new feature, so it is
not affected by this commit. And it is a feature, not a bug, for it to
use the values passed in -- the user is the sysadmin, who never makes
misteaks and knows what is sane better than init_param1().
Correction: values == 0 with little effort (missed the part where it
was using TUNABLE_ULONG_FETCH).
Hopefully it only uses TUNABLE_ULONG_FETCH() for u_longs. It starts
by using TUNABLE_INT_FIX() for hz. hz == -1 is in-band but is abused
to check for the tunable not being passed in, though hz = -1 can be
passed in. Zero, and other negative values of hz are silently
accepted. This is a feature (unless you want kernel bloat), since it
is too hard for init_param() to tell what a sane value is. To do so,
it would have to know all about the subsystems that use the variables,
and the interactions of the subsystems with each other and with the
system's resources, since sane values of the variables depend on the
other variables and the system's resources. For hz, it happens to be
easy to know that hz = 0 is insane. hz = 0 will in fact be detected
and reported immediately as a fatal trap (division by 0). hz < 0 is
just insane. Large values of hz may or may not be insane, depending
on timecounter and other system capabilities (except hz > 1000000 is
obviously insane, since the same immediate division that gives the
fatal trap when hz = 0 also gives tick = 0 when hz > 1000000, and
tick = 0 can't work).
Next, init_param1() uses some TUNABLE_LONG_FETCH()es for variables not
touched in this commit. Most of the older parameters use plain int
or long.
There is sanity checking only for the newer tunable ngroups_max. This
mainly breaks the ability of the user to create a non-POSIX system for
testing. This sanity checking has a comment about not allowing values
greater than INT_MAX - 1. This comment is wrong and not just
irrelevant. TUNABLE_INT_FETCH() automatically disallows values larger
than INT_MAX. But it allows a value of INT_MAX, and the code does
nothing extra to disallow that. This is probably related to old
off-by-1 bugs near {NGROUPS_MAX} (it is unclear if the effective gid
is "supplementary"). Hopefully we now count the extra 1 in
{NGROUPS_MAX}, so never need to add 1 to {NGROUPS_MAX}.
You could get negative values though
if you overflow the value passed in -- in part because the getenv*
functions in kern_environment.c don't check for/handle overflow
gracefully .. I had a patch out for this a while ago that never made
it in.
The unsigned long variables mainly break any possibility of detecting
overflow, since overflow and negative values can't really happen for
unsigned longs. But it is a feature for init_param1() to not limit
values to say LONG_MAX, since such a limit is almost useless for
preventing overflow when the variables are combined, and limits small
enough to prevent overflow for all possible reasonable combinations of
the variables would probably prevent useful combinations of the
variables (when some of the variables can be larger than usual
provided others are smaller than usual). The limits related to packing
of kva are most interesting here -- even adding up the sizes of the
pieces is not easy. The above discussion of {NGROUPS_MAX} shows that
even adding 1 is not easy.
non-performant (non-multiple of PAGE_SIZE, whacky ratios, etc) values.
Since we have to trust users to pack kva delicately (if they want to
change the defaults at all), why not trust them to know PAGE_SIZE?
I thought of sanity checking here, but there weren't for tunables and I
did't want to add any "magic numbers" in this code. I don't think that
we should check for multiple of PAGE_SIZE, may be only for sgrowsiz and
even not checking, just rounding up.
Indeed. It is possible for sysctl() to do more sanity checking, but
even less useful, since doing so mainly breaks testing of the limits.
If you have those "magic numbers" I would love to add it.
The limits can't be static magic numbers, since they (should) depend
on things like kva size and packing which depend on other limits.
Bruce
_______________________________________________
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
