On Thu, Oct 06, 2011 at 05:54:18PM -0500, Alex Elder wrote:
> On Fri, 2011-10-07 at 09:15 +1100, Dave Chinner wrote:
> > On Thu, Oct 06, 2011 at 07:17:52AM -0500, Alex Elder wrote:
> > > On Thu, 2011-10-06 at 12:01 +1100, Dave Chinner wrote:
> > > > @@ -218,6 +244,15 @@ blkmap_grow(
> > > > }
> > > >
> > > > blkmap->naexts += 4;
> > >
> > > The check needs to go *before* you update naexts.
> > It's in the right place - adding 4 to the unchecked extent count can
> > push it over the limit, so we have to check it after adding 4 to it.
> My point was that I'd rather see the check be whether it *will*
> overflow, rather than allowing it to overflow.
> The other reason though, even if you do the calculation
> at that spot, is that you shouldn't update what blkmap
> records as the number of allocated extents unless you
> have actually updated it the amount of allocated memory.
> As it stands, you will have updated blkmap->naexts, then
> if it overflows you return before actually attempting
> to reallocate.
> In other words, blkmap->naexts should reflect the actual
> amount of memory allotted in the blkmap->exts array, which
> is not going to be the case if it returns early (and this
> can be avoided).
And just checking the single caller of blkmap_grow indicates that it
doesn't handle allocation failure *at all*, so I need to fix that as
> > IOWs, we can't use BLKMAP_SIZE to detect an overflow, because it
> > is the code that overflows...
> I understand that argument. I'm in a hurry at the moment so
> I haven't thought through this very well right now.
> But if you do have BLKMAP_NENTS_MAX, you could check nex
> against that before attempting to use BLKMAP_SIZE to
> compute how many bytes need to be allocated.
Yup, that's what the patch does, just without the BLKMAP_NENTS_MAX
macro. Which I've already added. ;)
FWIW, because the extent count fields are all signed ints,
overflow checks also need to check for values <= 0. So I've added
And I think now I need to split this into separate patches for each
issue, as there are now about 5 different problems this one patch