On Wed, Jul 09, 2008 at 05:27:23PM -0700, Mingming Cao wrote: > > 在 2008-07-08二的 22:14 +0530，Aneesh Kumar K.V写道： > > Some architectures implement ext4_find_next_bit and > > ext4_find_next_zero_bit in such a way that they return > > greater than max for some input values. Make sure > > mb_find_next_bit and mb_find_next_zero_bit return the > > right values. > > > > I am not quite clear what bad things happen when ext4_find_next_bit() > returns greater than max? Not sure why we need to force it return > within the range.. > > Just looking at the code it seems all handles properly that if the > return > max, only one is missing is checking in t ext4_find_next_bit > (), (as the lustr e patch does), no? If we return more than max in ext4_mb_generate_buddy we end up with wrong length for the free block extent. That would mean we could end up corrupting the file system. I guess this would be related to that http://bugzilla.kernel.org/show_bug.cgi?id=11053 We don't have a clear reproducer for that bug. The max argument implies that we cannot search beyond max. > > > On 2.6.25 we have include/asm-x86/bitops_32.h > > static inline unsigned find_first_bit(const unsigned long *addr, unsigned size) > > { > > unsigned x = 0; > > > > while (x < size) { > > unsigned long val = *addr++; > > if (val) > > return __ffs(val) + x; > > x += (sizeof(*addr)<<3); > > } > > return x; > > } > > > > This can return value greater than size. > > > > Reported and fixed here for lustre > > > > https://bugzilla.lustre.org/show_bug.cgi?id=15932 > > https://bugzilla.lustre.org/attachment.cgi?id=17205 > > > > Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@xxxxxxxxxxxxxxxxxx> > > --- > > fs/ext4/mballoc.c | 20 ++++++++++++-------- > > 1 files changed, 12 insertions(+), 8 deletions(-) > > > > diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c > > index a1e58fb..d2c61eb 100644 > > --- a/fs/ext4/mballoc.c > > +++ b/fs/ext4/mballoc.c > > @@ -381,22 +381,28 @@ static inline void mb_clear_bit_atomic(spinlock_t *lock, int bit, void *addr) > > > > static inline int mb_find_next_zero_bit(void *addr, int max, int start) > > { > > - int fix = 0; > > + int fix = 0, ret, tmpmax; > > addr = mb_correct_addr_and_bit(&fix, addr); > > - max += fix; > > + tmpmax = max + fix; > > start += fix; > > > > - return ext4_find_next_zero_bit(addr, max, start) - fix; > > + ret = ext4_find_next_zero_bit(addr, tmpmax, start) - fix; > > + if (ret > max) > > + return max; > > + return ret; > > } > > > > I missed something basic here, why we need to check ret against the > tmpmax instead of the max after adjust? (Actually not quite sure why we > need to correct the address in the first place). Some architectures implement ext4_find_next_zero_bit to take unsinged long pointers. That means the address should be divisible by sizeof(long). The mb_correct_addr_and_bit just adjust address by moving it back to an address divisible by sizeof(long). Now that we adjust address we need to update bit and the max. But in any case we need to make sure the value returned is not greater than max because rest of the code assumes and even the API is supposed to guarantee that returned value is <= max. Instead of auditing all the implementation of ext4_find_next_zero_bit we just ensures that mb_find_next_zero_bit returns <= max. -aneesh -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
