Date: Sun, 09 Dec 2012 23:42:28 +0100 From: Andre Oppermann <andre@freebsd.org> To: Alan Cox <alc@rice.edu> 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: <50C513D4.4000306@freebsd.org> In-Reply-To: <50C4F5F7.7080101@rice.edu> References: <201211290730.qAT7Uhkv016745@svn.freebsd.org> <50C4F5F7.7080101@rice.edu>
next in thread | previous in thread | raw e-mail | index | archive | help
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 > >
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?50C513D4.4000306>