From owner-svn-src-head@FreeBSD.ORG Mon Dec 10 09:15:34 2012 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 89294DF2; Mon, 10 Dec 2012 09:15:34 +0000 (UTC) (envelope-from alc@rice.edu) Received: from mh1.mail.rice.edu (mh1.mail.rice.edu [128.42.201.20]) by mx1.freebsd.org (Postfix) with ESMTP id 4CCFE8FC12; Mon, 10 Dec 2012 09:15:34 +0000 (UTC) Received: from mh1.mail.rice.edu (localhost.localdomain [127.0.0.1]) by mh1.mail.rice.edu (Postfix) with ESMTP id 921E2460147; Mon, 10 Dec 2012 03:15:33 -0600 (CST) Received: from mh1.mail.rice.edu (localhost.localdomain [127.0.0.1]) by mh1.mail.rice.edu (Postfix) with ESMTP id 8FFCC46013B; Mon, 10 Dec 2012 03:15:33 -0600 (CST) X-Virus-Scanned: by amavis-2.7.0 at mh1.mail.rice.edu, auth channel Received: from mh1.mail.rice.edu ([127.0.0.1]) by mh1.mail.rice.edu (mh1.mail.rice.edu [127.0.0.1]) (amavis, port 10026) with ESMTP id ThRFQF2CO2kH; Mon, 10 Dec 2012 03:15:33 -0600 (CST) Received: from adsl-216-63-78-18.dsl.hstntx.swbell.net (adsl-216-63-78-18.dsl.hstntx.swbell.net [216.63.78.18]) (using TLSv1 with cipher RC4-MD5 (128/128 bits)) (No client certificate requested) (Authenticated sender: alc) by mh1.mail.rice.edu (Postfix) with ESMTPSA id 0CEBE46011D; Mon, 10 Dec 2012 03:15:32 -0600 (CST) Message-ID: <50C5A832.2080801@rice.edu> Date: Mon, 10 Dec 2012 03:15:30 -0600 From: Alan Cox User-Agent: Mozilla/5.0 (X11; FreeBSD i386; rv:16.0) Gecko/20121111 Thunderbird/16.0.2 MIME-Version: 1.0 To: Andre Oppermann Subject: Re: svn commit: r243668 - in head/sys: kern sys References: <201211290730.qAT7Uhkv016745@svn.freebsd.org> <50C4F5F7.7080101@rice.edu> <50C513D4.4000306@freebsd.org> In-Reply-To: <50C513D4.4000306@freebsd.org> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 10 Dec 2012 09:15:34 -0000 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