From owner-svn-src-head@FreeBSD.ORG Wed Feb 23 23:36:39 2011 Return-Path: Delivered-To: svn-src-head@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 8ABA610656A4; Wed, 23 Feb 2011 23:36:39 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail07.syd.optusnet.com.au (mail07.syd.optusnet.com.au [211.29.132.188]) by mx1.freebsd.org (Postfix) with ESMTP id 24BE88FC17; Wed, 23 Feb 2011 23:36:38 +0000 (UTC) Received: from c122-107-114-89.carlnfd1.nsw.optusnet.com.au (c122-107-114-89.carlnfd1.nsw.optusnet.com.au [122.107.114.89]) by mail07.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id p1NNaUP4027761 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Thu, 24 Feb 2011 10:36:31 +1100 Date: Thu, 24 Feb 2011 10:36:30 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Bruce Cran In-Reply-To: <1298499116.9366.3.camel@core.nessbank> Message-ID: <20110224102112.P1871@besplex.bde.org> References: <201102231028.p1NASbET045275@svn.freebsd.org> <20110224063233.Y1100@besplex.bde.org> <1298499116.9366.3.camel@core.nessbank> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org, Bruce Evans , Bruce Cran Subject: Re: svn commit: r218966 - head/sys/vm X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.5 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: Wed, 23 Feb 2011 23:36:39 -0000 On Wed, 23 Feb 2011, Bruce Cran wrote: > On Thu, 2011-02-24 at 08:23 +1100, Bruce Evans wrote: > >> The bug seems to have been overflow in this calculation. >> [swap_bcount * SWAP_META_PAGES * n / ] > > I've attached a patch which changes 'n' to be of type vm_ooffset_t. I > think this should fix the overflow bug? I don't like using vm_ooffset_t either. There are no offsets here, and it's bad technique to depend on having a large type to avoid overflows in expressions when the result type is different. I would cast operand(s) in the expression as necessary to prevent overflow of subexpressions. vm_pindex_t would work, but I prefer to use a type related to the subexpressions. Not sure what that is. Maybe just uintmax_t for safety (even that is not safe if the subexpressions have large values). So: (uintmax_t)swap_bcount * SWAP_META_PAGES * n / mumble. I like to cast only the leftmost term if possible, and depend on the larger type propagating to all subexpressions via left-to-right evaluation. This saves a lot of casts. Here this may be sub-optimal and we could probably delay the cast to the final multiplication, which reduces to the same safeness as using uintmax_t for n. Next, there is the return type to consider. I don't see why it needs to be changed from int. The patch in the PR actually changed it to long, while changing n to vm_offset_t. But on 32-bit machines, long is essentially the same as int, and vm_offset_t is not much larger. Even 32-bit machine might actually need a type larger than 32 bits to prevent overflow in expressions like the above. Bruce