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>