Skip site navigation (1)Skip section navigation (2)
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>