Re: svn commit: r240026 - head/sys/kern
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 wrote:
On Sun, Sep 2, 2012 at 10:39 AM, Andrey Zonov 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 > 100 is
obviously insane, since the same immediate division that gives the
fatal trap when hz = 0 also gives tick = 0 when hz > 100, 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-s
Re: svn commit: r240026 - head/sys/kern
On 9/2/12 11:03 PM, Andriy Gapon wrote: > on 02/09/2012 20:39 Andrey Zonov said the following: >> CTLFLAG_RDTUN | CTLFLAG_RW > > This combination looks like nonsense to me (because of "RD"). > > CTLFLAG_RDTUNAdvisory flag that a system tunable also exists for this > variable; however, the run-time variable is read-only. > > Probably you meant CTLFLAG_RW | CTLFLAG_TUN > Yes, thanks for pointing that out. -- Andrey Zonov signature.asc Description: OpenPGP digital signature
Re: svn commit: r240026 - head/sys/kern
On 9/3/12 1:20 AM, Garrett Cooper wrote: > On Sun, Sep 2, 2012 at 2:16 PM, Garrett Cooper wrote: >> On Sun, Sep 2, 2012 at 10:39 AM, Andrey Zonov 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 > > Correction: values == 0 with little effort (missed the part where it > was using TUNABLE_ULONG_FETCH). 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. > >> non-performant (non-multiple of PAGE_SIZE, whacky ratios, etc) values. >> Thanks, >> -Garrett 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. If you have those "magic numbers" I would love to add it. -- Andrey Zonov signature.asc Description: OpenPGP digital signature
Re: svn commit: r240026 - head/sys/kern
On Sun, Sep 2, 2012 at 2:16 PM, Garrett Cooper wrote: > On Sun, Sep 2, 2012 at 10:39 AM, Andrey Zonov 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 Correction: values == 0 with little effort (missed the part where it was using TUNABLE_ULONG_FETCH). 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. > non-performant (non-multiple of PAGE_SIZE, whacky ratios, etc) values. > Thanks, > -Garrett ___ 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"
Re: svn commit: r240026 - head/sys/kern
On Sun, Sep 2, 2012 at 10:39 AM, Andrey Zonov 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 > > Modified: head/sys/kern/subr_param.c > == > --- head/sys/kern/subr_param.c Sun Sep 2 15:27:20 2012(r240025) > +++ head/sys/kern/subr_param.c Sun Sep 2 17:39:02 2012(r240026) > @@ -119,18 +119,18 @@ SYSCTL_LONG(_kern, OID_AUTO, maxswzone, > "Maximum memory for swap metadata"); > SYSCTL_LONG(_kern, OID_AUTO, maxbcache, CTLFLAG_RDTUN, &maxbcache, 0, > "Maximum value of vfs.maxbufspace"); > -SYSCTL_ULONG(_kern, OID_AUTO, maxtsiz, CTLFLAG_RDTUN, &maxtsiz, 0, > +SYSCTL_ULONG(_kern, OID_AUTO, maxtsiz, CTLFLAG_RDTUN | CTLFLAG_RW, &maxtsiz, > 0, > "Maximum text size"); > -SYSCTL_ULONG(_kern, OID_AUTO, dfldsiz, CTLFLAG_RDTUN, &dfldsiz, 0, > +SYSCTL_ULONG(_kern, OID_AUTO, dfldsiz, CTLFLAG_RDTUN | CTLFLAG_RW, &dfldsiz, > 0, > "Initial data size limit"); > -SYSCTL_ULONG(_kern, OID_AUTO, maxdsiz, CTLFLAG_RDTUN, &maxdsiz, 0, > +SYSCTL_ULONG(_kern, OID_AUTO, maxdsiz, CTLFLAG_RDTUN | CTLFLAG_RW, &maxdsiz, > 0, > "Maximum data size"); > -SYSCTL_ULONG(_kern, OID_AUTO, dflssiz, CTLFLAG_RDTUN, &dflssiz, 0, > +SYSCTL_ULONG(_kern, OID_AUTO, dflssiz, CTLFLAG_RDTUN | CTLFLAG_RW, &dflssiz, > 0, > "Initial stack size limit"); > -SYSCTL_ULONG(_kern, OID_AUTO, maxssiz, CTLFLAG_RDTUN, &maxssiz, 0, > +SYSCTL_ULONG(_kern, OID_AUTO, maxssiz, CTLFLAG_RDTUN | CTLFLAG_RW, &maxssiz, > 0, > "Maximum stack size"); > -SYSCTL_ULONG(_kern, OID_AUTO, sgrowsiz, CTLFLAG_RDTUN, &sgrowsiz, 0, > -"Amount to grow stack on a stack fault"); > +SYSCTL_ULONG(_kern, OID_AUTO, sgrowsiz, CTLFLAG_RDTUN | CTLFLAG_RW, > &sgrowsiz, > +0, "Amount to grow stack on a stack fault"); > SYSCTL_PROC(_kern, OID_AUTO, vm_guest, CTLFLAG_RD | CTLTYPE_STRING, > NULL, 0, sysctl_kern_vm_guest, "A", > "Virtual machine guest detected? (none|generic|xen)"); Please add some basic sanity checking to init_param1 -- there's absolutely nothing preventing me from passing in values <= 0 or other non-performant (non-multiple of PAGE_SIZE, whacky ratios, etc) values. Thanks, -Garrett ___ 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"
Re: svn commit: r240026 - head/sys/kern
on 02/09/2012 20:39 Andrey Zonov said the following: > CTLFLAG_RDTUN | CTLFLAG_RW This combination looks like nonsense to me (because of "RD"). CTLFLAG_RDTUNAdvisory flag that a system tunable also exists for this variable; however, the run-time variable is read-only. Probably you meant CTLFLAG_RW | CTLFLAG_TUN -- Andriy Gapon ___ 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"
