svn commit: r218966 - head/sys/vm

[Bruce Evans]

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