Skip site navigation (1)Skip section navigation (2)
Date:      Mon, 10 Dec 2012 03:15:30 -0600
From:      Alan Cox <alc@rice.edu>
To:        Andre Oppermann <andre@freebsd.org>
Cc:        svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org
Subject:   Re: svn commit: r243668 - in head/sys: kern sys
Message-ID:  <50C5A832.2080801@rice.edu>
In-Reply-To: <50C513D4.4000306@freebsd.org>
References:  <201211290730.qAT7Uhkv016745@svn.freebsd.org> <50C4F5F7.7080101@rice.edu> <50C513D4.4000306@freebsd.org>

next in thread | previous in thread | raw e-mail | index | archive | help
On 12/09/2012 16:42, Andre Oppermann wrote:
> On 09.12.2012 21:35, Alan Cox wrote:
>> Andre,
>>
>> I believe that this change did not actually correct the overflow
>> problem.  See below for an explanation.
>>
>> On 11/29/2012 01:30, Andre Oppermann wrote:
>>> Author: andre
>>> Date: Thu Nov 29 07:30:42 2012
>>> New Revision: 243668
>>> URL: http://svnweb.freebsd.org/changeset/base/243668
>>>
>>> Log:
>>>    Using a long is the wrong type to represent the realmem and
>>> maxmbufmem
>>>    variable as they may overflow on i386/PAE and i386 with > 2GB RAM.
>>>
>>>    Use 64bit quad_t instead.  It has broader kernel infrastructure
>>> support
>>>    with TUNABLE_QUAD_FETCH() and qmin/qmax() than other available
>>> types.
>>>
>>>    Pointed out by:    alc, bde
>>>
>>> Modified:
>>>    head/sys/kern/subr_param.c
>>>    head/sys/sys/mbuf.h
>>>
>>> Modified: head/sys/kern/subr_param.c
>>> ==============================================================================
>>>
>>> --- head/sys/kern/subr_param.c    Thu Nov 29 06:26:42 2012    (r243667)
>>> +++ head/sys/kern/subr_param.c    Thu Nov 29 07:30:42 2012    (r243668)
>>> @@ -93,7 +93,7 @@ int    ncallout;            /* maximum # of timer ev
>>>   int    nbuf;
>>>   int    ngroups_max;            /* max # groups per process */
>>>   int    nswbuf;
>>> -long    maxmbufmem;            /* max mbuf memory */
>>> +quad_t    maxmbufmem;            /* max mbuf memory */
>>>   pid_t    pid_max = PID_MAX;
>>>   long    maxswzone;            /* max swmeta KVA storage */
>>>   long    maxbcache;            /* max buffer cache KVA storage */
>>> @@ -271,7 +271,7 @@ init_param1(void)
>>>   void
>>>   init_param2(long physpages)
>>>   {
>>> -    long realmem;
>>> +    quad_t realmem;
>>>
>>>       /* Base parameters */
>>>       maxusers = MAXUSERS;
>>> @@ -332,10 +332,10 @@ init_param2(long physpages)
>>>        * available kernel memory (physical or kmem).
>>>        * At most it can be 3/4 of available kernel memory.
>>>        */
>>> -    realmem = lmin(physpages * PAGE_SIZE,
>>> +    realmem = qmin(physpages * PAGE_SIZE,
>>>               VM_MAX_KERNEL_ADDRESS - VM_MIN_KERNEL_ADDRESS);
>>
>>
>> "physpages" is a signed long.  Suppose it is 1,000,000.  On i386/PAE,
>> the product of 1,000,000 and PAGE_SIZE will be a negative number.
>> Likewise, quad_t is a signed type.  So, the negative product of
>> 1,000,000 and PAGE_SIZE will be sign extended to a 64-bit signed value
>> when it is passed to qmin(), and qmin() will return a negative number.
>
> Thank you taking a second look it.
>
> To be honest I got a bit confused on which automatic type expansion
> applies here.
>
> qmax() is defined as this in libkern.h:
> static __inline quad_t qmax(quad_t a, quad_t b) { return (a > b ? a :
> b); }
>
> Wouldn't physpages be expanded to quad_t?  Hmm, no, only the result of
> the
> computation, which happens in long space, is passed on.  This is a
> function,
> not a macro.  Dang...
>
> This change will force the computation to be in quad_t space:
>
> -       realmem = qmin(physpages * PAGE_SIZE,
> +       realmem = qmin((quad_t)physpages * PAGE_SIZE,
>                         VM_MAX_KERNEL_ADDRESS - VM_MIN_KERNEL_ADDRESS);
>
> VM_[MAX|MIN]_KERNEL_ADDRESS is safe as it is of the unsigned vm_offset_t
> type.
>


This change looks ok.  As Bruce mentioned, can you also change the
indentation of the next line to match style(9) before you commit the change?

Alan




Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?50C5A832.2080801>