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). Fair point. 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 well. > > 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 that too. 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 fixes. Cheers, Dave. -- Dave Chinner david@xxxxxxxxxxxxx _______________________________________________ xfs mailing list xfs@xxxxxxxxxxx http://oss.sgi.com/mailman/listinfo/xfs
