[v2] btrfs: Add chunk type check in read a chunk
diff mbox

Message ID 1530598800-9174-1-git-send-email-gujx@cn.fujitsu.com
State New
Headers show

Commit Message

Gu Jinxiang July 3, 2018, 6:20 a.m. UTC
Reported in https://bugzilla.kernel.org/show_bug.cgi?id=199839,
which has a invalid chunk, not return error opportunlly.

Add chunk type check in btrfs_check_chunk_valid,
to make error be returned in advance.

Reported-by: Xu Wen <wen.xu@gatech.edu>
Signed-off-by: Gu Jinxiang <gujx@cn.fujitsu.com>
---
changelog:
v2:
deal with comment by Qu, add precise check for chunk type.

 fs/btrfs/volumes.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

Comments

Qu Wenruo July 3, 2018, 6:27 a.m. UTC | #1
On 2018年07月03日 14:20, Gu Jinxiang wrote:
> Reported in https://bugzilla.kernel.org/show_bug.cgi?id=199839,
> which has a invalid chunk, not return error opportunlly.
> 
> Add chunk type check in btrfs_check_chunk_valid,
> to make error be returned in advance.
> 
> Reported-by: Xu Wen <wen.xu@gatech.edu>
> Signed-off-by: Gu Jinxiang <gujx@cn.fujitsu.com>

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu

> ---
> changelog:
> v2:
> deal with comment by Qu, add precise check for chunk type.
> 
>  fs/btrfs/volumes.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index e034ad9e23b4..fcdb1a75d653 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -6401,6 +6401,8 @@ static int btrfs_check_chunk_valid(struct btrfs_fs_info *fs_info,
>  	u16 num_stripes;
>  	u16 sub_stripes;
>  	u64 type;
> +	u64 features;
> +	int mixed = 0;
>  
>  	length = btrfs_chunk_length(leaf, chunk);
>  	stripe_len = btrfs_chunk_stripe_len(leaf, chunk);
> @@ -6439,6 +6441,31 @@ static int btrfs_check_chunk_valid(struct btrfs_fs_info *fs_info,
>  			  btrfs_chunk_type(leaf, chunk));
>  		return -EIO;
>  	}
> +
> +	if (!(type & BTRFS_BLOCK_GROUP_TYPE_MASK)) {
> +		btrfs_err(fs_info, "missing chunk type flag: %llu", type);
> +		return -EIO;
> +	}
> +
> +	if ((type & BTRFS_BLOCK_GROUP_SYSTEM) &&
> +	(type & (BTRFS_BLOCK_GROUP_METADATA | BTRFS_BLOCK_GROUP_DATA))) {
> +		btrfs_err(fs_info, "duplicate chunk type flag: %llu", type);
> +		return -EIO;
> +	}
> +
> +	features = btrfs_super_incompat_flags(fs_info->super_copy);
> +	if (features & BTRFS_FEATURE_INCOMPAT_MIXED_GROUPS)
> +		mixed = 1;
> +
> +	if (!mixed) {
> +		if (type &
> +		(BTRFS_BLOCK_GROUP_METADATA & BTRFS_BLOCK_GROUP_DATA)) {
> +			btrfs_err(fs_info,
> +			"mixed chunk type flag in non-mixed mode: %llu", type);
> +			return -EIO;
> +		}
> +	}
> +
>  	if ((type & BTRFS_BLOCK_GROUP_RAID10 && sub_stripes != 2) ||
>  	    (type & BTRFS_BLOCK_GROUP_RAID1 && num_stripes < 1) ||
>  	    (type & BTRFS_BLOCK_GROUP_RAID5 && num_stripes < 2) ||
>
Nikolay Borisov July 4, 2018, 8:15 a.m. UTC | #2
On  3.07.2018 09:20, Gu Jinxiang wrote:
> Reported in https://bugzilla.kernel.org/show_bug.cgi?id=199839,
> which has a invalid chunk, not return error opportunlly.
> 
> Add chunk type check in btrfs_check_chunk_valid,
> to make error be returned in advance.
> 
> Reported-by: Xu Wen <wen.xu@gatech.edu>
> Signed-off-by: Gu Jinxiang <gujx@cn.fujitsu.com>
> ---
> changelog:
> v2:
> deal with comment by Qu, add precise check for chunk type.
> 
>  fs/btrfs/volumes.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index e034ad9e23b4..fcdb1a75d653 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -6401,6 +6401,8 @@ static int btrfs_check_chunk_valid(struct btrfs_fs_info *fs_info,
>  	u16 num_stripes;
>  	u16 sub_stripes;
>  	u64 type;
> +	u64 features;
> +	int mixed = 0;
>  
>  	length = btrfs_chunk_length(leaf, chunk);
>  	stripe_len = btrfs_chunk_stripe_len(leaf, chunk);
> @@ -6439,6 +6441,31 @@ static int btrfs_check_chunk_valid(struct btrfs_fs_info *fs_info,
>  			  btrfs_chunk_type(leaf, chunk));
>  		return -EIO;
>  	}
> +
> +	if (!(type & BTRFS_BLOCK_GROUP_TYPE_MASK)) {
> +		btrfs_err(fs_info, "missing chunk type flag: %llu", type);
> +		return -EIO;
> +	}
> +
> +	if ((type & BTRFS_BLOCK_GROUP_SYSTEM) &&
> +	(type & (BTRFS_BLOCK_GROUP_METADATA | BTRFS_BLOCK_GROUP_DATA))) {
> +		btrfs_err(fs_info, "duplicate chunk type flag: %llu", type);

This is not really a duplicate chunk type flag but instead an invalid
combination. SYSTEM is always a dedicated chunk, rephrase the error
message.

> +		return -EIO;
> +	}
> +
> +	features = btrfs_super_incompat_flags(fs_info->super_copy);
> +	if (features & BTRFS_FEATURE_INCOMPAT_MIXED_GROUPS)
> +		mixed = 1;
> +
> +	if (!mixed) {
> +		if (type &
> +		(BTRFS_BLOCK_GROUP_METADATA & BTRFS_BLOCK_GROUP_DATA)) {
> +			btrfs_err(fs_info,
> +			"mixed chunk type flag in non-mixed mode: %llu", type);
> +			return -EIO;
> +		}
> +	}
> +
>  	if ((type & BTRFS_BLOCK_GROUP_RAID10 && sub_stripes != 2) ||
>  	    (type & BTRFS_BLOCK_GROUP_RAID1 && num_stripes < 1) ||
>  	    (type & BTRFS_BLOCK_GROUP_RAID5 && num_stripes < 2) ||
> 
--
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
Gu Jinxiang July 4, 2018, 8:39 a.m. UTC | #3
> -----Original Message-----

> From: Nikolay Borisov [mailto:nborisov@suse.com]

> Sent: Wednesday, July 04, 2018 4:15 PM

> To: Gu, Jinxiang/顾 金香 <gujx@cn.fujitsu.com>; linux-btrfs@vger.kernel.org

> Cc: quwenruo.btrfs@gmx.com; wen.xu@gatech.edu; xuwen.sjtu@gmail.com

> Subject: Re: [PATCH v2] btrfs: Add chunk type check in read a chunk

> 

> 

> 

> On  3.07.2018 09:20, Gu Jinxiang wrote:

> > Reported in https://bugzilla.kernel.org/show_bug.cgi?id=199839,

> > which has a invalid chunk, not return error opportunlly.

> >

> > Add chunk type check in btrfs_check_chunk_valid,

> > to make error be returned in advance.

> >

> > Reported-by: Xu Wen <wen.xu@gatech.edu>

> > Signed-off-by: Gu Jinxiang <gujx@cn.fujitsu.com>

> > ---

> > changelog:

> > v2:

> > deal with comment by Qu, add precise check for chunk type.

> >

> >  fs/btrfs/volumes.c | 27 +++++++++++++++++++++++++++

> >  1 file changed, 27 insertions(+)

> >

> > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c

> > index e034ad9e23b4..fcdb1a75d653 100644

> > --- a/fs/btrfs/volumes.c

> > +++ b/fs/btrfs/volumes.c

> > @@ -6401,6 +6401,8 @@ static int btrfs_check_chunk_valid(struct btrfs_fs_info *fs_info,

> >  	u16 num_stripes;

> >  	u16 sub_stripes;

> >  	u64 type;

> > +	u64 features;

> > +	int mixed = 0;

> >

> >  	length = btrfs_chunk_length(leaf, chunk);

> >  	stripe_len = btrfs_chunk_stripe_len(leaf, chunk);

> > @@ -6439,6 +6441,31 @@ static int btrfs_check_chunk_valid(struct btrfs_fs_info *fs_info,

> >  			  btrfs_chunk_type(leaf, chunk));

> >  		return -EIO;

> >  	}

> > +

> > +	if (!(type & BTRFS_BLOCK_GROUP_TYPE_MASK)) {

> > +		btrfs_err(fs_info, "missing chunk type flag: %llu", type);

> > +		return -EIO;

> > +	}

> > +

> > +	if ((type & BTRFS_BLOCK_GROUP_SYSTEM) &&

> > +	(type & (BTRFS_BLOCK_GROUP_METADATA | BTRFS_BLOCK_GROUP_DATA))) {

> > +		btrfs_err(fs_info, "duplicate chunk type flag: %llu", type);

> 

> This is not really a duplicate chunk type flag but instead an invalid

> combination. SYSTEM is always a dedicated chunk, rephrase the error

> message.


Yes. This message should be updated.
But [PATCH 1/5] btrfs: tree-checker: Verify block_group_item sent by Qu, checks the chunk type
in tree checker, which be called when read a bio. So this patch should not be merged.

> 

> > +		return -EIO;

> > +	}

> > +

> > +	features = btrfs_super_incompat_flags(fs_info->super_copy);

> > +	if (features & BTRFS_FEATURE_INCOMPAT_MIXED_GROUPS)

> > +		mixed = 1;

> > +

> > +	if (!mixed) {

> > +		if (type &

> > +		(BTRFS_BLOCK_GROUP_METADATA & BTRFS_BLOCK_GROUP_DATA)) {

> > +			btrfs_err(fs_info,

> > +			"mixed chunk type flag in non-mixed mode: %llu", type);

> > +			return -EIO;

> > +		}

> > +	}

> > +

> >  	if ((type & BTRFS_BLOCK_GROUP_RAID10 && sub_stripes != 2) ||

> >  	    (type & BTRFS_BLOCK_GROUP_RAID1 && num_stripes < 1) ||

> >  	    (type & BTRFS_BLOCK_GROUP_RAID5 && num_stripes < 2) ||

> >

>
Qu Wenruo July 4, 2018, 9:49 a.m. UTC | #4
On 2018年07月04日 16:39, Gu, Jinxiang wrote:
> 
> 
>> -----Original Message-----
>> From: Nikolay Borisov [mailto:nborisov@suse.com]
>> Sent: Wednesday, July 04, 2018 4:15 PM
>> To: Gu, Jinxiang/顾 金香 <gujx@cn.fujitsu.com>; linux-btrfs@vger.kernel.org
>> Cc: quwenruo.btrfs@gmx.com; wen.xu@gatech.edu; xuwen.sjtu@gmail.com
>> Subject: Re: [PATCH v2] btrfs: Add chunk type check in read a chunk
>>
>>
>>
>> On  3.07.2018 09:20, Gu Jinxiang wrote:
>>> Reported in https://bugzilla.kernel.org/show_bug.cgi?id=199839,
>>> which has a invalid chunk, not return error opportunlly.
>>>
>>> Add chunk type check in btrfs_check_chunk_valid,
>>> to make error be returned in advance.
>>>
>>> Reported-by: Xu Wen <wen.xu@gatech.edu>
>>> Signed-off-by: Gu Jinxiang <gujx@cn.fujitsu.com>
>>> ---
>>> changelog:
>>> v2:
>>> deal with comment by Qu, add precise check for chunk type.
>>>
>>>  fs/btrfs/volumes.c | 27 +++++++++++++++++++++++++++
>>>  1 file changed, 27 insertions(+)
>>>
>>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>>> index e034ad9e23b4..fcdb1a75d653 100644
>>> --- a/fs/btrfs/volumes.c
>>> +++ b/fs/btrfs/volumes.c
>>> @@ -6401,6 +6401,8 @@ static int btrfs_check_chunk_valid(struct btrfs_fs_info *fs_info,
>>>  	u16 num_stripes;
>>>  	u16 sub_stripes;
>>>  	u64 type;
>>> +	u64 features;
>>> +	int mixed = 0;
>>>
>>>  	length = btrfs_chunk_length(leaf, chunk);
>>>  	stripe_len = btrfs_chunk_stripe_len(leaf, chunk);
>>> @@ -6439,6 +6441,31 @@ static int btrfs_check_chunk_valid(struct btrfs_fs_info *fs_info,
>>>  			  btrfs_chunk_type(leaf, chunk));
>>>  		return -EIO;
>>>  	}
>>> +
>>> +	if (!(type & BTRFS_BLOCK_GROUP_TYPE_MASK)) {
>>> +		btrfs_err(fs_info, "missing chunk type flag: %llu", type);
>>> +		return -EIO;
>>> +	}
>>> +
>>> +	if ((type & BTRFS_BLOCK_GROUP_SYSTEM) &&
>>> +	(type & (BTRFS_BLOCK_GROUP_METADATA | BTRFS_BLOCK_GROUP_DATA))) {
>>> +		btrfs_err(fs_info, "duplicate chunk type flag: %llu", type);
>>
>> This is not really a duplicate chunk type flag but instead an invalid
>> combination. SYSTEM is always a dedicated chunk, rephrase the error
>> message.
> 
> Yes. This message should be updated.
> But [PATCH 1/5] btrfs: tree-checker: Verify block_group_item sent by Qu, checks the chunk type
> in tree checker, which be called when read a bio. So this patch should not be merged.

Nope, this patch is still needed.

Although I added block group -> chunk checker, and it indeed detects all
the crafted images, doing chunk verification is nothing wrong.

If chunk verification can detect anything wrong, we can error out even
earlier.

So I still think this patch is needed.
(Although I'd like to move a lot of chunk verification to tree-checker
later)

Thanks,
Qu

> 
>>
>>> +		return -EIO;
>>> +	}
>>> +
>>> +	features = btrfs_super_incompat_flags(fs_info->super_copy);
>>> +	if (features & BTRFS_FEATURE_INCOMPAT_MIXED_GROUPS)
>>> +		mixed = 1;
>>> +
>>> +	if (!mixed) {
>>> +		if (type &
>>> +		(BTRFS_BLOCK_GROUP_METADATA & BTRFS_BLOCK_GROUP_DATA)) {
>>> +			btrfs_err(fs_info,
>>> +			"mixed chunk type flag in non-mixed mode: %llu", type);
>>> +			return -EIO;
>>> +		}
>>> +	}
>>> +
>>>  	if ((type & BTRFS_BLOCK_GROUP_RAID10 && sub_stripes != 2) ||
>>>  	    (type & BTRFS_BLOCK_GROUP_RAID1 && num_stripes < 1) ||
>>>  	    (type & BTRFS_BLOCK_GROUP_RAID5 && num_stripes < 2) ||
>>>
>>
> 
> 
> 
> N�����r��y���b�X��ǧv�^�)޺{.n�+����{�n�߲)���w*jg��������ݢj/���z�ޖ��2�ޙ���&�)ߡ�a�����G���h��j:+v���w�٥
> 
--
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
David Sterba July 4, 2018, 1:40 p.m. UTC | #5
On Wed, Jul 04, 2018 at 05:49:50PM +0800, Qu Wenruo wrote:
> On 2018年07月04日 16:39, Gu, Jinxiang wrote:
> >> -----Original Message-----
> >> From: Nikolay Borisov [mailto:nborisov@suse.com]
> >> Sent: Wednesday, July 04, 2018 4:15 PM
> >> To: Gu, Jinxiang/顾 金香 <gujx@cn.fujitsu.com>; linux-btrfs@vger.kernel.org
> >> Cc: quwenruo.btrfs@gmx.com; wen.xu@gatech.edu; xuwen.sjtu@gmail.com
> >> Subject: Re: [PATCH v2] btrfs: Add chunk type check in read a chunk
> >> On  3.07.2018 09:20, Gu Jinxiang wrote:
> >>> Reported in https://bugzilla.kernel.org/show_bug.cgi?id=199839,
> >>> which has a invalid chunk, not return error opportunlly.
> >>>
> >>> Add chunk type check in btrfs_check_chunk_valid,
> >>> to make error be returned in advance.
> >>>
> >>> Reported-by: Xu Wen <wen.xu@gatech.edu>
> >>> Signed-off-by: Gu Jinxiang <gujx@cn.fujitsu.com>
> >>> ---
> >>> changelog:
> >>> v2:
> >>> deal with comment by Qu, add precise check for chunk type.
> >>>
> >>>  fs/btrfs/volumes.c | 27 +++++++++++++++++++++++++++
> >>>  1 file changed, 27 insertions(+)
> >>>
> >>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> >>> index e034ad9e23b4..fcdb1a75d653 100644
> >>> --- a/fs/btrfs/volumes.c
> >>> +++ b/fs/btrfs/volumes.c
> >>> @@ -6401,6 +6401,8 @@ static int btrfs_check_chunk_valid(struct btrfs_fs_info *fs_info,
> >>>  	u16 num_stripes;
> >>>  	u16 sub_stripes;
> >>>  	u64 type;
> >>> +	u64 features;
> >>> +	int mixed = 0;
> >>>
> >>>  	length = btrfs_chunk_length(leaf, chunk);
> >>>  	stripe_len = btrfs_chunk_stripe_len(leaf, chunk);
> >>> @@ -6439,6 +6441,31 @@ static int btrfs_check_chunk_valid(struct btrfs_fs_info *fs_info,
> >>>  			  btrfs_chunk_type(leaf, chunk));
> >>>  		return -EIO;
> >>>  	}
> >>> +
> >>> +	if (!(type & BTRFS_BLOCK_GROUP_TYPE_MASK)) {
> >>> +		btrfs_err(fs_info, "missing chunk type flag: %llu", type);
> >>> +		return -EIO;
> >>> +	}
> >>> +
> >>> +	if ((type & BTRFS_BLOCK_GROUP_SYSTEM) &&
> >>> +	(type & (BTRFS_BLOCK_GROUP_METADATA | BTRFS_BLOCK_GROUP_DATA))) {
> >>> +		btrfs_err(fs_info, "duplicate chunk type flag: %llu", type);
> >>
> >> This is not really a duplicate chunk type flag but instead an invalid
> >> combination. SYSTEM is always a dedicated chunk, rephrase the error
> >> message.
> > 
> > Yes. This message should be updated.
> > But [PATCH 1/5] btrfs: tree-checker: Verify block_group_item sent by Qu, checks the chunk type
> > in tree checker, which be called when read a bio. So this patch should not be merged.
> 
> Nope, this patch is still needed.
> 
> Although I added block group -> chunk checker, and it indeed detects all
> the crafted images, doing chunk verification is nothing wrong.
> 
> If chunk verification can detect anything wrong, we can error out even
> earlier.
> 
> So I still think this patch is needed.
> (Although I'd like to move a lot of chunk verification to tree-checker
> later)

I'd rather add that to the tree-checker.c now and the enhanced checks.
btrfs_check_chunk_valid is self-contained and does not use anything
that's local to volumes.c.
--
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

Patch
diff mbox

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index e034ad9e23b4..fcdb1a75d653 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -6401,6 +6401,8 @@  static int btrfs_check_chunk_valid(struct btrfs_fs_info *fs_info,
 	u16 num_stripes;
 	u16 sub_stripes;
 	u64 type;
+	u64 features;
+	int mixed = 0;
 
 	length = btrfs_chunk_length(leaf, chunk);
 	stripe_len = btrfs_chunk_stripe_len(leaf, chunk);
@@ -6439,6 +6441,31 @@  static int btrfs_check_chunk_valid(struct btrfs_fs_info *fs_info,
 			  btrfs_chunk_type(leaf, chunk));
 		return -EIO;
 	}
+
+	if (!(type & BTRFS_BLOCK_GROUP_TYPE_MASK)) {
+		btrfs_err(fs_info, "missing chunk type flag: %llu", type);
+		return -EIO;
+	}
+
+	if ((type & BTRFS_BLOCK_GROUP_SYSTEM) &&
+	(type & (BTRFS_BLOCK_GROUP_METADATA | BTRFS_BLOCK_GROUP_DATA))) {
+		btrfs_err(fs_info, "duplicate chunk type flag: %llu", type);
+		return -EIO;
+	}
+
+	features = btrfs_super_incompat_flags(fs_info->super_copy);
+	if (features & BTRFS_FEATURE_INCOMPAT_MIXED_GROUPS)
+		mixed = 1;
+
+	if (!mixed) {
+		if (type &
+		(BTRFS_BLOCK_GROUP_METADATA & BTRFS_BLOCK_GROUP_DATA)) {
+			btrfs_err(fs_info,
+			"mixed chunk type flag in non-mixed mode: %llu", type);
+			return -EIO;
+		}
+	}
+
 	if ((type & BTRFS_BLOCK_GROUP_RAID10 && sub_stripes != 2) ||
 	    (type & BTRFS_BLOCK_GROUP_RAID1 && num_stripes < 1) ||
 	    (type & BTRFS_BLOCK_GROUP_RAID5 && num_stripes < 2) ||