Date: Thu, 8 Nov 2012 14:50:48 +0100 From: Marius Strobl <marius@alchemy.franken.de> To: Bruce Evans <brde@optusnet.com.au> Cc: svn-src-head@freebsd.org, Marius Strobl <marius@freebsd.org>, src-committers@freebsd.org, svn-src-all@freebsd.org Subject: Re: svn commit: r242747 - head/sys/kern Message-ID: <20121108135048.GQ80490@alchemy.franken.de> In-Reply-To: <20121108205636.O3941@besplex.bde.org> References: <201211080810.qA88AW8Y027373@svn.freebsd.org> <20121108205636.O3941@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, Nov 08, 2012 at 09:41:29PM +1100, Bruce Evans wrote:
> 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.
Eh, FreeBSD/i386 is ILP32, so longs are 32-bit there, as is its
__vm_offset_t.
>
> 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.
Right, SYSCTL_UINTMAX() unfortunately doesn't exist. SYSCTL_UQUAD()
seemed inappropriate as it is 64-bit and we want 32-bit for ILP32
for an exact match. Using uintmax_t with SYSCTL_UQUAD() also just
happens to be of the same width.
>From the available combinations, using vm_offset_t with SYSCTL_ULONG()
just seemed to suck the least.
>
> (*) 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");
Actually, for sparc64 VM_MAX_KERNEL_ADDRESS isn't constant (and can't
be).
>
> 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.
>
Marius
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20121108135048.GQ80490>