Date: Thu, 24 Feb 2011 10:36:30 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Bruce Cran <bruce@cran.org.uk> Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org, Bruce Evans <brde@optusnet.com.au>, Bruce Cran <brucec@FreeBSD.org> Subject: Re: svn commit: r218966 - head/sys/vm Message-ID: <20110224102112.P1871@besplex.bde.org> In-Reply-To: <1298499116.9366.3.camel@core.nessbank> References: <201102231028.p1NASbET045275@svn.freebsd.org> <20110224063233.Y1100@besplex.bde.org> <1298499116.9366.3.camel@core.nessbank>
next in thread | previous in thread | raw e-mail | index | archive | help
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 / <non-overflowing division>] > > 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20110224102112.P1871>