[PATCH] ext4: Fix mb_find_next_bit not to return larger than max

[PATCH] ext4: Fix mb_find_next_bit not to return larger than max

[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.

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@linux.vnet.ibm.com>
---
 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;
 }
 
 static inline int mb_find_next_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_bit(addr, max, start) - fix;
+	ret = ext4_find_next_bit(addr, tmpmax, start) - fix;
+	if (ret > max)
+		return max;
+	return ret;
 }
 
 static void *mb_find_buddy(struct ext4_buddy *e4b, int order, int *max)
@@ -3633,8 +3639,6 @@ ext4_mb_release_inode_pa(struct ext4_buddy *e4b, struct buffer_head *bitmap_bh,
 		if (bit >= end)
 			break;
 		next = mb_find_next_bit(bitmap_bh->b_data, end, bit);
-		if (next > end)
-			next = end;
 		start = group * EXT4_BLOCKS_PER_GROUP(sb) + bit +
 				le32_to_cpu(sbi->s_es->s_first_data_block);
 		mb_debug("    free preallocated %u/%u in group %u\n",
-- 
1.5.6.2.255.gbed62.dirty

Re: [PATCH] ext4: Fix mb_find_next_bit not to return larger than max

[Mingming Cao]

在 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?

> 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@linux.vnet.ibm.com>
> ---
>  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). 


>  static inline int mb_find_next_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_bit(addr, max, start) - fix;
> +	ret = ext4_find_next_bit(addr, tmpmax, start) - fix;
> +	if (ret > max)
> +		return max;
> +	return ret;
>  }
> 
>  static void *mb_find_buddy(struct ext4_buddy *e4b, int order, int *max)
> @@ -3633,8 +3639,6 @@ ext4_mb_release_inode_pa(struct ext4_buddy *e4b, struct buffer_head *bitmap_bh,
>  		if (bit >= end)
>  			break;
>  		next = mb_find_next_bit(bitmap_bh->b_data, end, bit);
> -		if (next > end)
> -			next = end;
>  		start = group * EXT4_BLOCKS_PER_GROUP(sb) + bit +
>  				le32_to_cpu(sbi->s_es->s_first_data_block);
>  		mb_debug("    free preallocated %u/%u in group %u\n",

--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ext4: Fix mb_find_next_bit not to return larger than max

[Aneesh Kumar K.V]

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@linux.vnet.ibm.com>
> > ---
> >  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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html