svn commit: r243668 - in head/sys: kern sys

[Andre Oppermann]

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.

-- 
Andre

>>   	maxmbufmem = realmem / 2;
>> -	TUNABLE_LONG_FETCH("kern.maxmbufmem", &maxmbufmem);
>> +	TUNABLE_QUAD_FETCH("kern.maxmbufmem", &maxmbufmem);
>>   	if (maxmbufmem > (realmem / 4) * 3)
>>   		maxmbufmem = (realmem / 4) * 3;
>>
>>
>> Modified: head/sys/sys/mbuf.h
>> ==============================================================================
>> --- head/sys/sys/mbuf.h	Thu Nov 29 06:26:42 2012	(r243667)
>> +++ head/sys/sys/mbuf.h	Thu Nov 29 07:30:42 2012	(r243668)
>> @@ -395,7 +395,7 @@ struct mbstat {
>>    *
>>    * The rest of it is defined in kern/kern_mbuf.c
>>    */
>> -extern long		maxmbufmem;
>> +extern quad_t		maxmbufmem;
>>   extern uma_zone_t	zone_mbuf;
>>   extern uma_zone_t	zone_clust;
>>   extern uma_zone_t	zone_pack;
>>
> D
>
>