Date: Thu, 8 Nov 2012 21:41:29 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Marius Strobl <marius@freebsd.org> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r242747 - head/sys/kern Message-ID: <20121108205636.O3941@besplex.bde.org> In-Reply-To: <201211080810.qA88AW8Y027373@svn.freebsd.org> References: <201211080810.qA88AW8Y027373@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, 8 Nov 2012, Marius Strobl wrote:
> Log:
> Make r242655 build on sparc64. While at it, make vm_{max,min}_kernel_address
> vm_offset_t as they should be.
Er, they shouldn't be vm_offset_t.
> Modified: head/sys/kern/kern_malloc.c
> ==============================================================================
> --- head/sys/kern/kern_malloc.c Thu Nov 8 04:02:36 2012 (r242746)
> +++ head/sys/kern/kern_malloc.c Thu Nov 8 08:10:32 2012 (r242747)
> @@ -186,11 +186,13 @@ struct {
> */
> static uma_zone_t mt_zone;
>
> -static u_long vm_min_kernel_address = VM_MIN_KERNEL_ADDRESS;
> +static vm_offset_t vm_min_kernel_address = VM_MIN_KERNEL_ADDRESS;
> SYSCTL_ULONG(_vm, OID_AUTO, min_kernel_address, CTLFLAG_RD,
> &vm_min_kernel_address, 0, "Min kernel address");
SYSCTL_ULONG takes a u_long, not a vm_offset_t. A cast in
SYSCTL_ULONG() prevents possible detection of the type mismatch from
this.
This is most broken on i386's with correctly-sized longs (or almost
any arch with correctly-sized longs). There, vm_offset_t is 1
register wide and longs are 2 registers wide.
The bugs could be moved using SYSCTL_QUAD(). Correctly-sized quads
would be 4 registers wide, except quad has come to mean just a bad
way of spelling long long or int64_t.
> -static u_long vm_max_kernel_address = VM_MAX_KERNEL_ADDRESS;
> +#ifndef __sparc64__
> +static vm_offset_t vm_max_kernel_address = VM_MAX_KERNEL_ADDRESS;
> +#endif
> SYSCTL_ULONG(_vm, OID_AUTO, max_kernel_address, CTLFLAG_RD,
> &vm_max_kernel_address, 0, "Max kernel address");
Since the value is a compile-time constant, u_long has a chance of
holding its value even if u_long != vm_offset_t, and the old code is
closer to being correct than first appeared, and the correct fix is
relatively easy -- just use a uintmax_t vatiable and SYSCTL_UINTMAX()
(*). Unfortunately SYSCT_UINTMAX() doesn't exist, and the bogus
SYSCTL_UQUAD() does exist.
(*) Except a temporary variable is just a style bug in the above and
for this. SYSCTL_UNLONG(), like all integer SYSCTL()'s, has the feature
of supporting either a variable or a constant arg. The above should
use a constant arg and not need a variable. IIRC, the syntax for this is:
SYSCTL_ULONG(_vm, OID_AUTO, max_kernel_address, CTLFLAG_RD,
NULL, VM_MAX_KERNEL_ADDRESS, "Max kernel address");
subr_param.c is careful to use only basic types for all of its variables
so that standard sysctls apply.
Tunables have even more bugs in this area: at least at the input level
kern_environment.c:
- bogus quads are supported, but bogus uquads aren't
- everything goes through getenv_quad(), which uses strtoq(), which is
for signed values, so unsigned values are mishandled in various ways
- worst sign bugs are on 64-bit arches. getenv_ulong() uses getenv_quad(),
with blind casts and no overflow checking, so 64-bit values are silently
truncated to QUAD_MAX (63 bits). Before that, getenv_quad() has overflow
checking in its strtoq() call, but none in its multiplication, and the
API is broken (strtoq(3) indicatates errors in errno, but there is no
errno in the kernel).
The bugs in tunables mean that sysctls that report read-only tunables
can safely use SYSCTL_QUAD().
This is hard to fix cleanly without combinatorial explosion in the number
of types, giving uglyness in the implementation. It is probably best to
have single SYSCTL_INT() that operates on all integral types (signed
and unsigned) according to its the arg type. E.g.:
static vm_offset_t vm_min_kernel_address = VM_MIN_KERNEL_ADDRESS;
SYSCTL_INT(_vm, OID_AUTO, min_kernel_address, CTLFLAG_RD,
&vm_min_kernel_address, 0, "Min kernel address");
This should expand to an uncast &vm_min_kernel_address, with auxilary
data for the size and sign of that. sysctl_handle_int() should operate
atomically on all integer sizes according to the size and sign info.
Some SYSCTL_FOO()'s already generate size info or take a size arg, but
sysctl_handle_int() only supports plain int and is abused to support
u_int and is sometimes misused on sysctl data that has a size.
After fixing the style bug:
SYSCTL_INT(_vm, OID_AUTO, min_kernel_address, CTLFLAG_RD,
NULL, VM_MIN_KERNEL_ADDRESS, "Min kernel address");
The constant can have any integral type, and a the sysctl must generate
size and sign info for it too.
The API can probably be simplified to reduce the 2 parameters to 1.
Hopefully __builtin_constant_p() can be used to decide which case applies.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20121108205636.O3941>