All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] Btrfs: fix divide error upon chunk's stripe_len
@ 2016-04-27  0:53 Liu Bo
  2016-04-27 16:39 ` David Sterba
       [not found] ` <877ffgp7g2.fsf@gmail.com>
  0 siblings, 2 replies; 10+ messages in thread
From: Liu Bo @ 2016-04-27  0:53 UTC (permalink / raw)
  To: linux-btrfs; +Cc: dsterba

The struct 'map_lookup' uses type int for @stripe_len, while
btrfs_chunk_stripe_len() can return a u64 value, and it may end up with
@stripe_len being undefined value and it can lead to 'divide error' in
 __btrfs_map_block().

This changes 'map_lookup' to use type u64 for stripe_len, also right now
we only use BTRFS_STRIPE_LEN for stripe_len, so this adds a valid checker for
BTRFS_STRIPE_LEN.

Reported-by: Vegard Nossum <vegard.nossum@oracle.com>
Reported-by: Quentin Casasnovas <quentin.casasnovas@oracle.com>
Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
---
 fs/btrfs/volumes.c | 2 +-
 fs/btrfs/volumes.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index e2b54d5..b5cb859 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -6242,7 +6242,7 @@ static int read_one_chunk(struct btrfs_root *root, struct btrfs_key *key,
 			"invalid chunk length %llu", length);
 		return -EIO;
 	}
-	if (!is_power_of_2(stripe_len)) {
+	if (!is_power_of_2(stripe_len) || stripe_len != BTRFS_STRIPE_LEN) {
 		btrfs_err(root->fs_info, "invalid chunk stripe length: %llu",
 			  stripe_len);
 		return -EIO;
diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index 1939ebd..7507be7 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -347,7 +347,7 @@ struct map_lookup {
 	u64 type;
 	int io_align;
 	int io_width;
-	int stripe_len;
+	u64 stripe_len;
 	int sector_size;
 	int num_stripes;
 	int sub_stripes;
-- 
2.5.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH] Btrfs: fix divide error upon chunk's stripe_len
  2016-04-27  0:53 [PATCH] Btrfs: fix divide error upon chunk's stripe_len Liu Bo
@ 2016-04-27 16:39 ` David Sterba
  2016-04-27 17:23   ` Liu Bo
       [not found] ` <877ffgp7g2.fsf@gmail.com>
  1 sibling, 1 reply; 10+ messages in thread
From: David Sterba @ 2016-04-27 16:39 UTC (permalink / raw)
  To: Liu Bo; +Cc: linux-btrfs, dsterba

On Tue, Apr 26, 2016 at 05:53:31PM -0700, Liu Bo wrote:
> The struct 'map_lookup' uses type int for @stripe_len, while
> btrfs_chunk_stripe_len() can return a u64 value, and it may end up with
> @stripe_len being undefined value and it can lead to 'divide error' in
>  __btrfs_map_block().
> 
> This changes 'map_lookup' to use type u64 for stripe_len, also right now
> we only use BTRFS_STRIPE_LEN for stripe_len, so this adds a valid checker for
> BTRFS_STRIPE_LEN.
> 
> Reported-by: Vegard Nossum <vegard.nossum@oracle.com>
> Reported-by: Quentin Casasnovas <quentin.casasnovas@oracle.com>

I smell some fuzzing :) do you have the image available? I'll add it to
the rest in btrfsprogs.

> Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
> ---
>  fs/btrfs/volumes.c | 2 +-
>  fs/btrfs/volumes.h | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index e2b54d5..b5cb859 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -6242,7 +6242,7 @@ static int read_one_chunk(struct btrfs_root *root, struct btrfs_key *key,
>  			"invalid chunk length %llu", length);
>  		return -EIO;
>  	}
> -	if (!is_power_of_2(stripe_len)) {
> +	if (!is_power_of_2(stripe_len) || stripe_len != BTRFS_STRIPE_LEN) {

Unfortunatelly this will break current state, as mkfs does not set the
stripe length to 64k but to 4k. But the value is otherwise ignored in
kernel.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Btrfs: fix divide error upon chunk's stripe_len
  2016-04-27 16:39 ` David Sterba
@ 2016-04-27 17:23   ` Liu Bo
  2016-04-27 17:33     ` David Sterba
  0 siblings, 1 reply; 10+ messages in thread
From: Liu Bo @ 2016-04-27 17:23 UTC (permalink / raw)
  To: dsterba, linux-btrfs, dsterba

On Wed, Apr 27, 2016 at 06:39:03PM +0200, David Sterba wrote:
> On Tue, Apr 26, 2016 at 05:53:31PM -0700, Liu Bo wrote:
> > The struct 'map_lookup' uses type int for @stripe_len, while
> > btrfs_chunk_stripe_len() can return a u64 value, and it may end up with
> > @stripe_len being undefined value and it can lead to 'divide error' in
> >  __btrfs_map_block().
> > 
> > This changes 'map_lookup' to use type u64 for stripe_len, also right now
> > we only use BTRFS_STRIPE_LEN for stripe_len, so this adds a valid checker for
> > BTRFS_STRIPE_LEN.
> > 
> > Reported-by: Vegard Nossum <vegard.nossum@oracle.com>
> > Reported-by: Quentin Casasnovas <quentin.casasnovas@oracle.com>
> 
> I smell some fuzzing :) do you have the image available? I'll add it to
> the rest in btrfsprogs.

Sure, it's on the way, I'll send it along with a patch for btrfsck (we have to add the same validation check for superblock and chunk in btrfsck.)

> 
> > Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
> > ---
> >  fs/btrfs/volumes.c | 2 +-
> >  fs/btrfs/volumes.h | 2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> > index e2b54d5..b5cb859 100644
> > --- a/fs/btrfs/volumes.c
> > +++ b/fs/btrfs/volumes.c
> > @@ -6242,7 +6242,7 @@ static int read_one_chunk(struct btrfs_root *root, struct btrfs_key *key,
> >  			"invalid chunk length %llu", length);
> >  		return -EIO;
> >  	}
> > -	if (!is_power_of_2(stripe_len)) {
> > +	if (!is_power_of_2(stripe_len) || stripe_len != BTRFS_STRIPE_LEN) {
> 
> Unfortunatelly this will break current state, as mkfs does not set the
> stripe length to 64k but to 4k. But the value is otherwise ignored in
> kernel.

This is chunk's stripe_len, not superblock's stripe_len:

make_btrfs() {
	...
	btrfs_set_super_stripesize(&super, cfg->stripesize); --> 4096
	...
	btrfs_set_chunk_stripe_len(buf, chunk, 64 * 1024);
}

Thanks,

-liubo

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Btrfs: fix divide error upon chunk's stripe_len
  2016-04-27 17:23   ` Liu Bo
@ 2016-04-27 17:33     ` David Sterba
  2016-04-28 17:48       ` Liu Bo
  0 siblings, 1 reply; 10+ messages in thread
From: David Sterba @ 2016-04-27 17:33 UTC (permalink / raw)
  To: Liu Bo; +Cc: dsterba, linux-btrfs, dsterba

On Wed, Apr 27, 2016 at 10:23:35AM -0700, Liu Bo wrote:
> On Wed, Apr 27, 2016 at 06:39:03PM +0200, David Sterba wrote:
> > On Tue, Apr 26, 2016 at 05:53:31PM -0700, Liu Bo wrote:
> > > The struct 'map_lookup' uses type int for @stripe_len, while
> > > btrfs_chunk_stripe_len() can return a u64 value, and it may end up with
> > > @stripe_len being undefined value and it can lead to 'divide error' in
> > >  __btrfs_map_block().
> > > 
> > > This changes 'map_lookup' to use type u64 for stripe_len, also right now
> > > we only use BTRFS_STRIPE_LEN for stripe_len, so this adds a valid checker for
> > > BTRFS_STRIPE_LEN.
> > > 
> > > Reported-by: Vegard Nossum <vegard.nossum@oracle.com>
> > > Reported-by: Quentin Casasnovas <quentin.casasnovas@oracle.com>
> > 
> > I smell some fuzzing :) do you have the image available? I'll add it to
> > the rest in btrfsprogs.
> 
> Sure, it's on the way, I'll send it along with a patch for btrfsck (we
> have to add the same validation check for superblock and chunk in
> btrfsck.)

Great!

> > > Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
> > > ---
> > >  fs/btrfs/volumes.c | 2 +-
> > >  fs/btrfs/volumes.h | 2 +-
> > >  2 files changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> > > index e2b54d5..b5cb859 100644
> > > --- a/fs/btrfs/volumes.c
> > > +++ b/fs/btrfs/volumes.c
> > > @@ -6242,7 +6242,7 @@ static int read_one_chunk(struct btrfs_root *root, struct btrfs_key *key,
> > >  			"invalid chunk length %llu", length);
> > >  		return -EIO;
> > >  	}
> > > -	if (!is_power_of_2(stripe_len)) {
> > > +	if (!is_power_of_2(stripe_len) || stripe_len != BTRFS_STRIPE_LEN) {
> > 
> > Unfortunatelly this will break current state, as mkfs does not set the
> > stripe length to 64k but to 4k. But the value is otherwise ignored in
> > kernel.
> 
> This is chunk's stripe_len, not superblock's stripe_len:
> 
> make_btrfs() {
> 	...
> 	btrfs_set_super_stripesize(&super, cfg->stripesize); --> 4096
> 	...
> 	btrfs_set_chunk_stripe_len(buf, chunk, 64 * 1024);
> }

Oh right, and the hardcoded stripe chunk size would need to be fixed in
a lot more places so it's fine. Consider this

Reviewed-by: David Sterba <dsterba@suse.com>

and on the way to for-next.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Btrfs: fix divide error upon chunk's stripe_len
  2016-04-27 17:33     ` David Sterba
@ 2016-04-28 17:48       ` Liu Bo
  2016-04-29  3:20         ` Qu Wenruo
  2016-04-29 16:22         ` David Sterba
  0 siblings, 2 replies; 10+ messages in thread
From: Liu Bo @ 2016-04-28 17:48 UTC (permalink / raw)
  To: dsterba, linux-btrfs, dsterba

On Wed, Apr 27, 2016 at 07:33:18PM +0200, David Sterba wrote:
> On Wed, Apr 27, 2016 at 10:23:35AM -0700, Liu Bo wrote:
> > On Wed, Apr 27, 2016 at 06:39:03PM +0200, David Sterba wrote:
> > > On Tue, Apr 26, 2016 at 05:53:31PM -0700, Liu Bo wrote:
> > > > The struct 'map_lookup' uses type int for @stripe_len, while
> > > > btrfs_chunk_stripe_len() can return a u64 value, and it may end up with
> > > > @stripe_len being undefined value and it can lead to 'divide error' in
> > > >  __btrfs_map_block().
> > > > 
> > > > This changes 'map_lookup' to use type u64 for stripe_len, also right now
> > > > we only use BTRFS_STRIPE_LEN for stripe_len, so this adds a valid checker for
> > > > BTRFS_STRIPE_LEN.
> > > > 
> > > > Reported-by: Vegard Nossum <vegard.nossum@oracle.com>
> > > > Reported-by: Quentin Casasnovas <quentin.casasnovas@oracle.com>
> > > 
> > > I smell some fuzzing :) do you have the image available? I'll add it to
> > > the rest in btrfsprogs.
> > 
> > Sure, it's on the way, I'll send it along with a patch for btrfsck (we
> > have to add the same validation check for superblock and chunk in
> > btrfsck.)
> 
> Great!
> 
> > > > Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
> > > > ---
> > > >  fs/btrfs/volumes.c | 2 +-
> > > >  fs/btrfs/volumes.h | 2 +-
> > > >  2 files changed, 2 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> > > > index e2b54d5..b5cb859 100644
> > > > --- a/fs/btrfs/volumes.c
> > > > +++ b/fs/btrfs/volumes.c
> > > > @@ -6242,7 +6242,7 @@ static int read_one_chunk(struct btrfs_root *root, struct btrfs_key *key,
> > > >  			"invalid chunk length %llu", length);
> > > >  		return -EIO;
> > > >  	}
> > > > -	if (!is_power_of_2(stripe_len)) {
> > > > +	if (!is_power_of_2(stripe_len) || stripe_len != BTRFS_STRIPE_LEN) {

We don't need the first 'is_power_of_2' check.

And I think we may need to have another helper, such as btrfs_check_chunk_valid(),
to cover all these (both current and future) validation checks.  What do you think?

Thanks,

-liubo

> > > 
> > > Unfortunatelly this will break current state, as mkfs does not set the
> > > stripe length to 64k but to 4k. But the value is otherwise ignored in
> > > kernel.
> > 
> > This is chunk's stripe_len, not superblock's stripe_len:
> > 
> > make_btrfs() {
> > 	...
> > 	btrfs_set_super_stripesize(&super, cfg->stripesize); --> 4096
> > 	...
> > 	btrfs_set_chunk_stripe_len(buf, chunk, 64 * 1024);
> > }
> 
> Oh right, and the hardcoded stripe chunk size would need to be fixed in
> a lot more places so it's fine. Consider this
> 
> Reviewed-by: David Sterba <dsterba@suse.com>
> 
> and on the way to for-next.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Btrfs: fix divide error upon chunk's stripe_len
  2016-04-28 17:48       ` Liu Bo
@ 2016-04-29  3:20         ` Qu Wenruo
  2016-04-29 17:09           ` Liu Bo
  2016-04-29 16:22         ` David Sterba
  1 sibling, 1 reply; 10+ messages in thread
From: Qu Wenruo @ 2016-04-29  3:20 UTC (permalink / raw)
  To: bo.li.liu, dsterba, linux-btrfs, dsterba



Liu Bo wrote on 2016/04/28 10:48 -0700:
> On Wed, Apr 27, 2016 at 07:33:18PM +0200, David Sterba wrote:
>> On Wed, Apr 27, 2016 at 10:23:35AM -0700, Liu Bo wrote:
>>> On Wed, Apr 27, 2016 at 06:39:03PM +0200, David Sterba wrote:
>>>> On Tue, Apr 26, 2016 at 05:53:31PM -0700, Liu Bo wrote:
>>>>> The struct 'map_lookup' uses type int for @stripe_len, while
>>>>> btrfs_chunk_stripe_len() can return a u64 value, and it may end up with
>>>>> @stripe_len being undefined value and it can lead to 'divide error' in
>>>>>  __btrfs_map_block().
>>>>>
>>>>> This changes 'map_lookup' to use type u64 for stripe_len, also right now
>>>>> we only use BTRFS_STRIPE_LEN for stripe_len, so this adds a valid checker for
>>>>> BTRFS_STRIPE_LEN.
>>>>>
>>>>> Reported-by: Vegard Nossum <vegard.nossum@oracle.com>
>>>>> Reported-by: Quentin Casasnovas <quentin.casasnovas@oracle.com>
>>>>
>>>> I smell some fuzzing :) do you have the image available? I'll add it to
>>>> the rest in btrfsprogs.
>>>
>>> Sure, it's on the way, I'll send it along with a patch for btrfsck (we
>>> have to add the same validation check for superblock and chunk in
>>> btrfsck.)
>>
>> Great!
>>
>>>>> Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
>>>>> ---
>>>>>  fs/btrfs/volumes.c | 2 +-
>>>>>  fs/btrfs/volumes.h | 2 +-
>>>>>  2 files changed, 2 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>>>>> index e2b54d5..b5cb859 100644
>>>>> --- a/fs/btrfs/volumes.c
>>>>> +++ b/fs/btrfs/volumes.c
>>>>> @@ -6242,7 +6242,7 @@ static int read_one_chunk(struct btrfs_root *root, struct btrfs_key *key,
>>>>>  			"invalid chunk length %llu", length);
>>>>>  		return -EIO;
>>>>>  	}
>>>>> -	if (!is_power_of_2(stripe_len)) {
>>>>> +	if (!is_power_of_2(stripe_len) || stripe_len != BTRFS_STRIPE_LEN) {
>
> We don't need the first 'is_power_of_2' check.
>
> And I think we may need to have another helper, such as btrfs_check_chunk_valid(),
> to cover all these (both current and future) validation checks.  What do you think?
>
> Thanks,
>
> -liubo
>

+1 for a btrfs_check_chunk_valid().

And I'm OK to remove is_power_of_2 check, as it's only used for future 
stripe_len.
But we only support fix STRIPE_LEN, it's OK to remove it.

BTW, did it crash btrfs-progs?

Thanks,
Qu

>>>>
>>>> Unfortunatelly this will break current state, as mkfs does not set the
>>>> stripe length to 64k but to 4k. But the value is otherwise ignored in
>>>> kernel.
>>>
>>> This is chunk's stripe_len, not superblock's stripe_len:
>>>
>>> make_btrfs() {
>>> 	...
>>> 	btrfs_set_super_stripesize(&super, cfg->stripesize); --> 4096
>>> 	...
>>> 	btrfs_set_chunk_stripe_len(buf, chunk, 64 * 1024);
>>> }
>>
>> Oh right, and the hardcoded stripe chunk size would need to be fixed in
>> a lot more places so it's fine. Consider this
>>
>> Reviewed-by: David Sterba <dsterba@suse.com>
>>
>> and on the way to for-next.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Btrfs: fix divide error upon chunk's stripe_len
  2016-04-28 17:48       ` Liu Bo
  2016-04-29  3:20         ` Qu Wenruo
@ 2016-04-29 16:22         ` David Sterba
  1 sibling, 0 replies; 10+ messages in thread
From: David Sterba @ 2016-04-29 16:22 UTC (permalink / raw)
  To: Liu Bo; +Cc: linux-btrfs

On Thu, Apr 28, 2016 at 10:48:36AM -0700, Liu Bo wrote:
> On Wed, Apr 27, 2016 at 07:33:18PM +0200, David Sterba wrote:
> > On Wed, Apr 27, 2016 at 10:23:35AM -0700, Liu Bo wrote:
> > > On Wed, Apr 27, 2016 at 06:39:03PM +0200, David Sterba wrote:
> > > > On Tue, Apr 26, 2016 at 05:53:31PM -0700, Liu Bo wrote:
> > > > > The struct 'map_lookup' uses type int for @stripe_len, while
> > > > > btrfs_chunk_stripe_len() can return a u64 value, and it may end up with
> > > > > @stripe_len being undefined value and it can lead to 'divide error' in
> > > > >  __btrfs_map_block().
> > > > > 
> > > > > This changes 'map_lookup' to use type u64 for stripe_len, also right now
> > > > > we only use BTRFS_STRIPE_LEN for stripe_len, so this adds a valid checker for
> > > > > BTRFS_STRIPE_LEN.
> > > > > 
> > > > > Reported-by: Vegard Nossum <vegard.nossum@oracle.com>
> > > > > Reported-by: Quentin Casasnovas <quentin.casasnovas@oracle.com>
> > > > 
> > > > I smell some fuzzing :) do you have the image available? I'll add it to
> > > > the rest in btrfsprogs.
> > > 
> > > Sure, it's on the way, I'll send it along with a patch for btrfsck (we
> > > have to add the same validation check for superblock and chunk in
> > > btrfsck.)
> > 
> > Great!
> > 
> > > > > Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
> > > > > ---
> > > > >  fs/btrfs/volumes.c | 2 +-
> > > > >  fs/btrfs/volumes.h | 2 +-
> > > > >  2 files changed, 2 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> > > > > index e2b54d5..b5cb859 100644
> > > > > --- a/fs/btrfs/volumes.c
> > > > > +++ b/fs/btrfs/volumes.c
> > > > > @@ -6242,7 +6242,7 @@ static int read_one_chunk(struct btrfs_root *root, struct btrfs_key *key,
> > > > >  			"invalid chunk length %llu", length);
> > > > >  		return -EIO;
> > > > >  	}
> > > > > -	if (!is_power_of_2(stripe_len)) {
> > > > > +	if (!is_power_of_2(stripe_len) || stripe_len != BTRFS_STRIPE_LEN) {
> 
> We don't need the first 'is_power_of_2' check.
> 
> And I think we may need to have another helper, such as btrfs_check_chunk_valid(),
> to cover all these (both current and future) validation checks.  What do you think?

Sounds good.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Btrfs: fix divide error upon chunk's stripe_len
  2016-04-29  3:20         ` Qu Wenruo
@ 2016-04-29 17:09           ` Liu Bo
  0 siblings, 0 replies; 10+ messages in thread
From: Liu Bo @ 2016-04-29 17:09 UTC (permalink / raw)
  To: Qu Wenruo; +Cc: dsterba, linux-btrfs, dsterba

On Fri, Apr 29, 2016 at 11:20:31AM +0800, Qu Wenruo wrote:
> 
> 
> Liu Bo wrote on 2016/04/28 10:48 -0700:
> >On Wed, Apr 27, 2016 at 07:33:18PM +0200, David Sterba wrote:
> >>On Wed, Apr 27, 2016 at 10:23:35AM -0700, Liu Bo wrote:
> >>>On Wed, Apr 27, 2016 at 06:39:03PM +0200, David Sterba wrote:
> >>>>On Tue, Apr 26, 2016 at 05:53:31PM -0700, Liu Bo wrote:
> >>>>>The struct 'map_lookup' uses type int for @stripe_len, while
> >>>>>btrfs_chunk_stripe_len() can return a u64 value, and it may end up with
> >>>>>@stripe_len being undefined value and it can lead to 'divide error' in
> >>>>> __btrfs_map_block().
> >>>>>
> >>>>>This changes 'map_lookup' to use type u64 for stripe_len, also right now
> >>>>>we only use BTRFS_STRIPE_LEN for stripe_len, so this adds a valid checker for
> >>>>>BTRFS_STRIPE_LEN.
> >>>>>
> >>>>>Reported-by: Vegard Nossum <vegard.nossum@oracle.com>
> >>>>>Reported-by: Quentin Casasnovas <quentin.casasnovas@oracle.com>
> >>>>
> >>>>I smell some fuzzing :) do you have the image available? I'll add it to
> >>>>the rest in btrfsprogs.
> >>>
> >>>Sure, it's on the way, I'll send it along with a patch for btrfsck (we
> >>>have to add the same validation check for superblock and chunk in
> >>>btrfsck.)
> >>
> >>Great!
> >>
> >>>>>Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
> >>>>>---
> >>>>> fs/btrfs/volumes.c | 2 +-
> >>>>> fs/btrfs/volumes.h | 2 +-
> >>>>> 2 files changed, 2 insertions(+), 2 deletions(-)
> >>>>>
> >>>>>diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> >>>>>index e2b54d5..b5cb859 100644
> >>>>>--- a/fs/btrfs/volumes.c
> >>>>>+++ b/fs/btrfs/volumes.c
> >>>>>@@ -6242,7 +6242,7 @@ static int read_one_chunk(struct btrfs_root *root, struct btrfs_key *key,
> >>>>> 			"invalid chunk length %llu", length);
> >>>>> 		return -EIO;
> >>>>> 	}
> >>>>>-	if (!is_power_of_2(stripe_len)) {
> >>>>>+	if (!is_power_of_2(stripe_len) || stripe_len != BTRFS_STRIPE_LEN) {
> >
> >We don't need the first 'is_power_of_2' check.
> >
> >And I think we may need to have another helper, such as btrfs_check_chunk_valid(),
> >to cover all these (both current and future) validation checks.  What do you think?
> >
> >Thanks,
> >
> >-liubo
> >
> 
> +1 for a btrfs_check_chunk_valid().
> 
> And I'm OK to remove is_power_of_2 check, as it's only used for future
> stripe_len.
> But we only support fix STRIPE_LEN, it's OK to remove it.

OK.

> 
> BTW, did it crash btrfs-progs?

Yes, not really crash progs, but btrfsck doesn't detect errors.

I've made a patch and I'm testing it to make sure everything works.

Thanks,

-liubo

> 
> Thanks,
> Qu
> 
> >>>>
> >>>>Unfortunatelly this will break current state, as mkfs does not set the
> >>>>stripe length to 64k but to 4k. But the value is otherwise ignored in
> >>>>kernel.
> >>>
> >>>This is chunk's stripe_len, not superblock's stripe_len:
> >>>
> >>>make_btrfs() {
> >>>	...
> >>>	btrfs_set_super_stripesize(&super, cfg->stripesize); --> 4096
> >>>	...
> >>>	btrfs_set_chunk_stripe_len(buf, chunk, 64 * 1024);
> >>>}
> >>
> >>Oh right, and the hardcoded stripe chunk size would need to be fixed in
> >>a lot more places so it's fine. Consider this
> >>
> >>Reviewed-by: David Sterba <dsterba@suse.com>
> >>
> >>and on the way to for-next.
> >--
> >To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> >the body of a message to majordomo@vger.kernel.org
> >More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >
> >
> 
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Btrfs: fix divide error upon chunk's stripe_len
       [not found] ` <877ffgp7g2.fsf@gmail.com>
@ 2016-04-30  4:09   ` Liu Bo
  2016-05-02  8:16   ` David Sterba
  1 sibling, 0 replies; 10+ messages in thread
From: Liu Bo @ 2016-04-30  4:09 UTC (permalink / raw)
  To: Güngör Erseymen; +Cc: dsterba, linux-btrfs

On Fri, Apr 29, 2016 at 09:59:25PM +0300, Güngör Erseymen wrote:
> On 2016-04-27 at 03:53:31 +0300 (EEST), Liu Bo <bo.li.liu@oracle.com> wrote:
> > The struct 'map_lookup' uses type int for @stripe_len, while
> > btrfs_chunk_stripe_len() can return a u64 value, and it may end up with
> > @stripe_len being undefined value and it can lead to 'divide error' in
> >  __btrfs_map_block().
> >
> > This changes 'map_lookup' to use type u64 for stripe_len, also right now
> > we only use BTRFS_STRIPE_LEN for stripe_len, so this adds a valid checker for
> > BTRFS_STRIPE_LEN.
> >
> > Reported-by: Vegard Nossum <vegard.nossum@oracle.com>
> > Reported-by: Quentin Casasnovas <quentin.casasnovas@oracle.com>
> > Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
> > ---
> >  fs/btrfs/volumes.c | 2 +-
> >  fs/btrfs/volumes.h | 2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> > index e2b54d5..b5cb859 100644
> > --- a/fs/btrfs/volumes.c
> > +++ b/fs/btrfs/volumes.c
> > @@ -6242,7 +6242,7 @@ static int read_one_chunk(struct btrfs_root *root, struct btrfs_key *key,
> >  			"invalid chunk length %llu", length);
> >  		return -EIO;
> >  	}
> > -	if (!is_power_of_2(stripe_len)) {
> > +	if (!is_power_of_2(stripe_len) || stripe_len != BTRFS_STRIPE_LEN) {
> >  		btrfs_err(root->fs_info, "invalid chunk stripe length: %llu",
> >  			  stripe_len);
> >  		return -EIO;
> > diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
> > index 1939ebd..7507be7 100644
> > --- a/fs/btrfs/volumes.h
> > +++ b/fs/btrfs/volumes.h
> > @@ -347,7 +347,7 @@ struct map_lookup {
> >  	u64 type;
> >  	int io_align;
> >  	int io_width;
> > -	int stripe_len;
> > +	u64 stripe_len;
> >  	int sector_size;
> >  	int num_stripes;
> >  	int sub_stripes;
> > -- 
> > 2.5.0
> 
> Type change of map->stripe_len to u64 cause build error "undefined
> reference to __udivdi3" on 32bit systems. Using explicit 64 bit division
> fixes it and would be better to combine into this commit for
> continuity.

Thanks a lot for fixing this.

Thanks,

-liubo

> 
> Thanks.
> 
> diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
> index 4678f03e878e..d9ef1c4f91aa 100644
> --- a/fs/btrfs/scrub.c
> +++ b/fs/btrfs/scrub.c
> @@ -2860,7 +2860,7 @@ static noinline_for_stack int scrub_raid56_parity(struct scrub_ctx *sctx,
>         int extent_mirror_num;
>         int stop_loop = 0;
>  
> -       nsectors = map->stripe_len / root->sectorsize;
> +       nsectors = div_u64(map->stripe_len, root->sectorsize);
>         bitmap_len = scrub_calc_parity_bitmap_len(nsectors);
>         sparity = kzalloc(sizeof(struct scrub_parity) + 2 * bitmap_len,
>                           GFP_NOFS);
> 
> --
> Güngör Erseymen

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Btrfs: fix divide error upon chunk's stripe_len
       [not found] ` <877ffgp7g2.fsf@gmail.com>
  2016-04-30  4:09   ` Liu Bo
@ 2016-05-02  8:16   ` David Sterba
  1 sibling, 0 replies; 10+ messages in thread
From: David Sterba @ 2016-05-02  8:16 UTC (permalink / raw)
  To: Güngör Erseymen; +Cc: Liu Bo, linux-btrfs

On Fri, Apr 29, 2016 at 09:59:25PM +0300, Güngör Erseymen wrote:
> > --- a/fs/btrfs/volumes.h
> > +++ b/fs/btrfs/volumes.h
> > @@ -347,7 +347,7 @@ struct map_lookup {
> >  	u64 type;
> >  	int io_align;
> >  	int io_width;
> > -	int stripe_len;
> > +	u64 stripe_len;
> >  	int sector_size;
> >  	int num_stripes;
> >  	int sub_stripes;
> > -- 
> > 2.5.0
> 
> Type change of map->stripe_len to u64 cause build error "undefined
> reference to __udivdi3" on 32bit systems. Using explicit 64 bit division
> fixes it and would be better to combine into this commit for
> continuity.

Done, thanks for the report.

JFYI, the branches pushed to kernel.org get automated build tests so I
get the email reports from kbuild eventually.

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2016-05-02  8:17 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-04-27  0:53 [PATCH] Btrfs: fix divide error upon chunk's stripe_len Liu Bo
2016-04-27 16:39 ` David Sterba
2016-04-27 17:23   ` Liu Bo
2016-04-27 17:33     ` David Sterba
2016-04-28 17:48       ` Liu Bo
2016-04-29  3:20         ` Qu Wenruo
2016-04-29 17:09           ` Liu Bo
2016-04-29 16:22         ` David Sterba
     [not found] ` <877ffgp7g2.fsf@gmail.com>
2016-04-30  4:09   ` Liu Bo
2016-05-02  8:16   ` David Sterba

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.