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