Date: Wed, 5 Sep 2012 15:34:43 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Andrey Zonov <zont@freebsd.org> Cc: Garrett Cooper <yanegomi@gmail.com>, svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r240026 - head/sys/kern Message-ID: <20120905141910.N1053@besplex.bde.org> In-Reply-To: <50445609.8070902@FreeBSD.org> References: <201209021739.q82Hd3CE042578@svn.freebsd.org> <CAGH67wSqqXdC3LiLYsebMC88Kt=H5Hu_MJRR2Ug1zjMS3Et%2BYQ@mail.gmail.com> <CAGH67wQ0P841iwL2hvZ%2BH%2BDfpwmCTZQ9cPwBGZ_f7UCW9wckhw@mail.gmail.com> <50445609.8070902@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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 <yanegomi@gmail.com> wrote: >>> On Sun, Sep 2, 2012 at 10:39 AM, Andrey Zonov <zont@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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120905141910.N1053>