[PATCH v2 0/3] Lowmem fsck repair to fix filetype mismatch

[PATCH v2 0/3] Lowmem fsck repair to fix filetype mismatch

[Qu Wenruo]

Sebastian reported a filesystem corruption where DIR_INDEX has wrong
filetype against INODE_ITEM.

Lowmem mode normally handles such problem by checking DIR_INDEX,
DIR_ITEM and INODE_REF/INODE_ITEM to determine the correct file type.
In such case, lowmem mode fsck can get the correct filetype.

When fixing the problem, lowmem mode will try to re-insert correct
(DIR_INDEX, DIR_ITEM, INODE_REF) tuple, and if existing correct
DIR_ITEM and INODE_REF is found, btrfs_link() will just skip and only
insert correct DIR_INDEX.

However, when inserting correct DIR_INDEX, due to extra DIR_INDEX
validation, incorrect one will be skiped and correct one will be
inserted after invalid one.

This leads to lowmem mode repair to create duplicated DIR_INDEX.

This patch will fix it by removing the whole (DIR_INDEX, DIR_ITEM,
INODE_REF) tuple before inserting correct tuple.
And the removing part, btrfs_unlink(), will be enhanced to handle
incorrect tuple member more robust.

Please note that, due a bug in lowmem mode repair, btrfs check will
still show "error(s) found in fs tree" even repair is done successfully.

And test case for this repair still needs extra work for original mode
to support such repair, or test case won't pass original mode test.

Changelog:
v2:
  No longer play tricks to add new parameters to let btrfs_unlink() to
  locate invalid dir_index, but remove the unnecessary filetype check in
  verify_dir_item().
  Since user of functions in dir-items.c are all btrfs check, either
  repairing or checking, and both original mode and lowmem mode can
  handle it well.

Qu Wenruo (3):
  btrfs-progs: lowmem fsck: Remove corupted link before re-add correct
    link
  btrfs-progs: dir-item: Don't do extra filetype validaction check for
    btrfs_match_dir_item_name
  btrfs-progs: dir-item: Make btrfs_delete_one_dir_name more robust to
    handle corrupted name len

 cmds-check.c |  4 ++++
 dir-item.c   | 17 +++++++++--------
 2 files changed, 13 insertions(+), 8 deletions(-)

-- 
2.15.1

[PATCH v2 1/3] btrfs-progs: lowmem fsck: Remove corupted link before re-add correct link

[Qu Wenruo]

For repair_ternary_lowmem() used in lowmem mode, if it found 1 of
DIR_INDEX/DIR_ITEM/INODE_REF missing, it will try to insert correct
link.

However for case like invalid type in DIR_INDEX, we should delete the
corrupted DIR_INDEX first before inserting the correct link.

This patch will remove the corrupted link before re-insert.
This should solve the duplicated DIR_INDEX problem in old lowmem mode
repair.

Cc: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 cmds-check.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/cmds-check.c b/cmds-check.c
index 7fc30da83ea1..f302724dd840 100644
--- a/cmds-check.c
+++ b/cmds-check.c
@@ -4997,6 +4997,10 @@ int repair_ternary_lowmem(struct btrfs_root *root, u64 dir_ino, u64 ino,
 		goto out;
 	}
 	if (stage == 1) {
+		ret = btrfs_unlink(trans, root, ino, dir_ino, index, name,
+				   name_len, 0);
+		if (ret)
+			goto out;
 		ret = btrfs_add_link(trans, root, ino, dir_ino, name, name_len,
 			       filetype, &index, 1, 1);
 		goto out;
-- 
2.15.1

[PATCH v2 2/3] btrfs-progs: dir-item: Don't do extra filetype validaction check for btrfs_match_dir_item_name

[Qu Wenruo]

btrfs_match_dir_item_name() will check if its filetype is valid before
doing search, this makes btrfs-progs unable to locate and remove invalid
dir_index for btrfs_unlink().

This function only affects btrfs_link() and btrfs_unlink() in upper
layer, and normal check can find invalid filetype by itself.

So remove the filetype check is completely safe in this case, and will
enhance btrfs_unlink() to remove invalid dir_index/dir_item for repair.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 dir-item.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/dir-item.c b/dir-item.c
index 462546c0eaf4..e0a0ab4d7a5d 100644
--- a/dir-item.c
+++ b/dir-item.c
@@ -294,12 +294,6 @@ static int verify_dir_item(struct btrfs_root *root,
 	u16 namelen = BTRFS_NAME_LEN;
 	u8 type = btrfs_dir_type(leaf, dir_item);
 
-	if (type >= BTRFS_FT_MAX) {
-		fprintf(stderr, "invalid dir item type: %d\n",
-		       (int)type);
-		return 1;
-	}
-
 	if (type == BTRFS_FT_XATTR)
 		namelen = XATTR_NAME_MAX;
 
-- 
2.15.1

[PATCH v2 3/3] btrfs-progs: dir-item: Make btrfs_delete_one_dir_name more robust to handle corrupted name len

[Qu Wenruo]

Function btrfs_delete_one_dir_name() will check if the dir_item is the
last content of the item, and delete the whole item if needed.

However if @name_len of one dir_item/dir_index is corrupted and larger
than the item size, the function will still try to treat it as partly
remove, which will screw up the whole leaf.

This patch will enhance the item deletion check, to cover corrupted name
len, so in that case we just delete the whole item.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 dir-item.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/dir-item.c b/dir-item.c
index e0a0ab4d7a5d..35e0615fb423 100644
--- a/dir-item.c
+++ b/dir-item.c
@@ -263,7 +263,6 @@ int btrfs_delete_one_dir_name(struct btrfs_trans_handle *trans,
 			      struct btrfs_path *path,
 			      struct btrfs_dir_item *di)
 {
-
 	struct extent_buffer *leaf;
 	u32 sub_item_len;
 	u32 item_len;
@@ -273,7 +272,15 @@ int btrfs_delete_one_dir_name(struct btrfs_trans_handle *trans,
 	sub_item_len = sizeof(*di) + btrfs_dir_name_len(leaf, di) +
 		btrfs_dir_data_len(leaf, di);
 	item_len = btrfs_item_size_nr(leaf, path->slots[0]);
-	if (sub_item_len == item_len) {
+
+	/*
+	 * If @sub_item_len is longer than @item_len, then it means the
+	 * name_len is just corrupted.
+	 * No good idea to know if there is anything we can recover from
+	 * the corrupted item.
+	 * Just delete the item.
+	 */
+	if (sub_item_len >= item_len) {
 		ret = btrfs_del_item(trans, root, path);
 	} else {
 		unsigned long ptr = (unsigned long)di;
-- 
2.15.1

Re: [PATCH v2 2/3] btrfs-progs: dir-item: Don't do extra filetype validaction check for btrfs_match_dir_item_name

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 2164 bytes --]



On 2018年01月19日 15:39, Su Yue wrote:
> 
> 
> On 01/19/2018 03:25 PM, Qu Wenruo wrote:
>> btrfs_match_dir_item_name() will check if its filetype is valid before
>> doing search, this makes btrfs-progs unable to locate and remove invalid
>> dir_index for btrfs_unlink().
>>
>> This function only affects btrfs_link() and btrfs_unlink() in upper
>> layer, and normal check can find invalid filetype by itself.
>>
> Lowmem mode can't handles wrong filetype well now.
> I'm working on it. And this change is okay for me.

I think you mean *original* mode can't handle it.

As v4.14.1 lowmem mode can detect such problem without problem:
-------
checking fs roots
ERROR: root 5 INODE_ITEM[258] index 2 name file1 filetype 34 mismath
ERROR: root 5 DIR INDEX[257 2] missing name file1 filetype 1
ERROR: errors found in fs roots
found 131072 bytes used, error(s) found
-------

And patch 1/3 will handle the repair, so it shouldn't be a problem for
lowmem.

Thanks,
Qu
> 
> Reviewed-by: Su Yue <suy.fnst@cn.fujitsu.com>
> 
>> So remove the filetype check is completely safe in this case, and will
>> enhance btrfs_unlink() to remove invalid dir_index/dir_item for repair.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>   dir-item.c | 6 ------
>>   1 file changed, 6 deletions(-)
>>
>> diff --git a/dir-item.c b/dir-item.c
>> index 462546c0eaf4..e0a0ab4d7a5d 100644
>> --- a/dir-item.c
>> +++ b/dir-item.c
>> @@ -294,12 +294,6 @@ static int verify_dir_item(struct btrfs_root *root,
>>       u16 namelen = BTRFS_NAME_LEN;
>>       u8 type = btrfs_dir_type(leaf, dir_item);
>>   -    if (type >= BTRFS_FT_MAX) {
>> -        fprintf(stderr, "invalid dir item type: %d\n",
>> -               (int)type);
>> -        return 1;
>> -    }
>> -
>>       if (type == BTRFS_FT_XATTR)
>>           namelen = XATTR_NAME_MAX;
>>  
> 
> 
> -- 
> 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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 520 bytes --]

Re: [PATCH v2 2/3] btrfs-progs: dir-item: Don't do extra filetype validaction check for btrfs_match_dir_item_name

[Su Yue]

On 01/19/2018 03:25 PM, Qu Wenruo wrote:
> btrfs_match_dir_item_name() will check if its filetype is valid before
> doing search, this makes btrfs-progs unable to locate and remove invalid
> dir_index for btrfs_unlink().
> 
> This function only affects btrfs_link() and btrfs_unlink() in upper
> layer, and normal check can find invalid filetype by itself.
> 
Lowmem mode can't handles wrong filetype well now.
I'm working on it. And this change is okay for me.

Reviewed-by: Su Yue <suy.fnst@cn.fujitsu.com>

> So remove the filetype check is completely safe in this case, and will
> enhance btrfs_unlink() to remove invalid dir_index/dir_item for repair.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>   dir-item.c | 6 ------
>   1 file changed, 6 deletions(-)
> 
> diff --git a/dir-item.c b/dir-item.c
> index 462546c0eaf4..e0a0ab4d7a5d 100644
> --- a/dir-item.c
> +++ b/dir-item.c
> @@ -294,12 +294,6 @@ static int verify_dir_item(struct btrfs_root *root,
>   	u16 namelen = BTRFS_NAME_LEN;
>   	u8 type = btrfs_dir_type(leaf, dir_item);
>   
> -	if (type >= BTRFS_FT_MAX) {
> -		fprintf(stderr, "invalid dir item type: %d\n",
> -		       (int)type);
> -		return 1;
> -	}
> -
>   	if (type == BTRFS_FT_XATTR)
>   		namelen = XATTR_NAME_MAX;
>   
> 

Re: [PATCH v2 1/3] btrfs-progs: lowmem fsck: Remove corupted link before re-add correct link

[Su Yue]

On 01/19/2018 03:25 PM, Qu Wenruo wrote:
> For repair_ternary_lowmem() used in lowmem mode, if it found 1 of
> DIR_INDEX/DIR_ITEM/INODE_REF missing, it will try to insert correct
> link.
> 
> However for case like invalid type in DIR_INDEX, we should delete the
> corrupted DIR_INDEX first before inserting the correct link.
> 
> This patch will remove the corrupted link before re-insert.
> This should solve the duplicated DIR_INDEX problem in old lowmem mode
> repair.
> 
> Cc: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>

Reviewed-by: Su Yue <suy.fnst@cn.fujitsu.com>
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>   cmds-check.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/cmds-check.c b/cmds-check.c
> index 7fc30da83ea1..f302724dd840 100644
> --- a/cmds-check.c
> +++ b/cmds-check.c
> @@ -4997,6 +4997,10 @@ int repair_ternary_lowmem(struct btrfs_root *root, u64 dir_ino, u64 ino,
>   		goto out;
>   	}
>   	if (stage == 1) {
> +		ret = btrfs_unlink(trans, root, ino, dir_ino, index, name,
> +				   name_len, 0);
> +		if (ret)
> +			goto out;
>   		ret = btrfs_add_link(trans, root, ino, dir_ino, name, name_len,
>   			       filetype, &index, 1, 1);
>   		goto out;
> 

Re: [PATCH v2 3/3] btrfs-progs: dir-item: Make btrfs_delete_one_dir_name more robust to handle corrupted name len

[Su Yue]

On 01/19/2018 03:25 PM, Qu Wenruo wrote:
> Function btrfs_delete_one_dir_name() will check if the dir_item is the
> last content of the item, and delete the whole item if needed.
> 
> However if @name_len of one dir_item/dir_index is corrupted and larger
> than the item size, the function will still try to treat it as partly
> remove, which will screw up the whole leaf.
> 
> This patch will enhance the item deletion check, to cover corrupted name
> len, so in that case we just delete the whole item.
> 

Reviewed-by: Su Yue <suy.fnst@cn.fujitsu.com>
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>   dir-item.c | 11 +++++++++--
>   1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/dir-item.c b/dir-item.c
> index e0a0ab4d7a5d..35e0615fb423 100644
> --- a/dir-item.c
> +++ b/dir-item.c
> @@ -263,7 +263,6 @@ int btrfs_delete_one_dir_name(struct btrfs_trans_handle *trans,
>   			      struct btrfs_path *path,
>   			      struct btrfs_dir_item *di)
>   {
> -
>   	struct extent_buffer *leaf;
>   	u32 sub_item_len;
>   	u32 item_len;
> @@ -273,7 +272,15 @@ int btrfs_delete_one_dir_name(struct btrfs_trans_handle *trans,
>   	sub_item_len = sizeof(*di) + btrfs_dir_name_len(leaf, di) +
>   		btrfs_dir_data_len(leaf, di);
>   	item_len = btrfs_item_size_nr(leaf, path->slots[0]);
> -	if (sub_item_len == item_len) {
> +
> +	/*
> +	 * If @sub_item_len is longer than @item_len, then it means the
> +	 * name_len is just corrupted.
> +	 * No good idea to know if there is anything we can recover from
> +	 * the corrupted item.
> +	 * Just delete the item.
> +	 */
> +	if (sub_item_len >= item_len) {
>   		ret = btrfs_del_item(trans, root, path);
>   	} else {
>   		unsigned long ptr = (unsigned long)di;
> 

Re: [PATCH v2 2/3] btrfs-progs: dir-item: Don't do extra filetype validaction check for btrfs_match_dir_item_name

[Su Yue]

On 01/19/2018 03:38 PM, Qu Wenruo wrote:
> 
> 
> On 2018年01月19日 15:39, Su Yue wrote:
>>
>>
>> On 01/19/2018 03:25 PM, Qu Wenruo wrote:
>>> btrfs_match_dir_item_name() will check if its filetype is valid before
>>> doing search, this makes btrfs-progs unable to locate and remove invalid
>>> dir_index for btrfs_unlink().
>>>
>>> This function only affects btrfs_link() and btrfs_unlink() in upper
>>> layer, and normal check can find invalid filetype by itself.
>>>
>> Lowmem mode can't handles wrong filetype well now.
>> I'm working on it. And this change is okay for me.
> 
> I think you mean *original* mode can't handle it.
> 
Sorry, I mean that lowmem mode can't handle more complex cases.
This patchset does well enough for the single case.
I will fix it for original mode and new test-case will be
submitted.

Thanks,
Su
> As v4.14.1 lowmem mode can detect such problem without problem:
> -------
> checking fs roots
> ERROR: root 5 INODE_ITEM[258] index 2 name file1 filetype 34 mismath
> ERROR: root 5 DIR INDEX[257 2] missing name file1 filetype 1
> ERROR: errors found in fs roots
> found 131072 bytes used, error(s) found
> -------
> 
> And patch 1/3 will handle the repair, so it shouldn't be a problem for
> lowmem.
> 
> Thanks,
> Qu
>>
>> Reviewed-by: Su Yue <suy.fnst@cn.fujitsu.com>
>>
>>> So remove the filetype check is completely safe in this case, and will
>>> enhance btrfs_unlink() to remove invalid dir_index/dir_item for repair.
>>>
>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>> ---
>>>    dir-item.c | 6 ------
>>>    1 file changed, 6 deletions(-)
>>>
>>> diff --git a/dir-item.c b/dir-item.c
>>> index 462546c0eaf4..e0a0ab4d7a5d 100644
>>> --- a/dir-item.c
>>> +++ b/dir-item.c
>>> @@ -294,12 +294,6 @@ static int verify_dir_item(struct btrfs_root *root,
>>>        u16 namelen = BTRFS_NAME_LEN;
>>>        u8 type = btrfs_dir_type(leaf, dir_item);
>>>    -    if (type >= BTRFS_FT_MAX) {
>>> -        fprintf(stderr, "invalid dir item type: %d\n",
>>> -               (int)type);
>>> -        return 1;
>>> -    }
>>> -
>>>        if (type == BTRFS_FT_XATTR)
>>>            namelen = XATTR_NAME_MAX;
>>>   
>>
>>
>> -- 
>> 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
> 

Re: [PATCH v2 2/3] btrfs-progs: dir-item: Don't do extra filetype validaction check for btrfs_match_dir_item_name

[Nikolay Borisov]

On 19.01.2018 09:25, Qu Wenruo wrote:
> btrfs_match_dir_item_name() will check if its filetype is valid before
> doing search, this makes btrfs-progs unable to locate and remove invalid
> dir_index for btrfs_unlink().>
> This function only affects btrfs_link() and btrfs_unlink() in upper
> layer, and normal check can find invalid filetype by itself.

There is no function btrfs_link in btrfs-progs, there is, however,
btrfs_add_link did you mean that function? However, it doesn't seem to
use verify_dir_item hence the check you are removing. I think this part
of the commit log should be more precise. Also it's not really clear
what you mean by "normal check" in this sentence.
> 
> So remove the filetype check is completely safe in this case, and will
> enhance btrfs_unlink() to remove invalid dir_index/dir_item for repair.

So the problem is that since btrfs_unlink calls verify_item and the
latter has the filetype check in case of wrong filetype (corruption)
verify_dir_item fails, hence we cannot perform the unlink? If my
understanding is correct how about something like:


If we have a corrupted dir item and enough information to repair it we
need to first delete the old/corrupted version and then insert a new
one. However, btrfs_unlink calls btrfs_match_dir_item_name to locate the
offending dir item for deletion. The latter, in turn, uses
verify_dir_item which checks if the value for DIR item's type is sane.
In case of a corrupted type value then verify_dir_item will fail which
in turn will prevent btrfs_unlink from deleting the offending item.

> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  dir-item.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/dir-item.c b/dir-item.c
> index 462546c0eaf4..e0a0ab4d7a5d 100644
> --- a/dir-item.c
> +++ b/dir-item.c
> @@ -294,12 +294,6 @@ static int verify_dir_item(struct btrfs_root *root,
>  	u16 namelen = BTRFS_NAME_LEN;
>  	u8 type = btrfs_dir_type(leaf, dir_item);
>  
> -	if (type >= BTRFS_FT_MAX) {
> -		fprintf(stderr, "invalid dir item type: %d\n",
> -		       (int)type);
> -		return 1;
> -	}
> -
>  	if (type == BTRFS_FT_XATTR)
>  		namelen = XATTR_NAME_MAX;
>  
> 

Re: [PATCH v2 3/3] btrfs-progs: dir-item: Make btrfs_delete_one_dir_name more robust to handle corrupted name len

[Nikolay Borisov]

On 19.01.2018 09:25, Qu Wenruo wrote:
> Function btrfs_delete_one_dir_name() will check if the dir_item is the
> last content of the item, and delete the whole item if needed.
> 
> However if @name_len of one dir_item/dir_index is corrupted and larger
> than the item size, the function will still try to treat it as partly
> remove, which will screw up the whole leaf.
> 
> This patch will enhance the item deletion check, to cover corrupted name
> len, so in that case we just delete the whole item.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>

Reviewed-by: Nikolay Borisov <nborisov@suse.com>

Perhaps it would be worth it creating a regression test for that ?

> ---
>  dir-item.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/dir-item.c b/dir-item.c
> index e0a0ab4d7a5d..35e0615fb423 100644
> --- a/dir-item.c
> +++ b/dir-item.c
> @@ -263,7 +263,6 @@ int btrfs_delete_one_dir_name(struct btrfs_trans_handle *trans,
>  			      struct btrfs_path *path,
>  			      struct btrfs_dir_item *di)
>  {
> -
>  	struct extent_buffer *leaf;
>  	u32 sub_item_len;
>  	u32 item_len;
> @@ -273,7 +272,15 @@ int btrfs_delete_one_dir_name(struct btrfs_trans_handle *trans,
>  	sub_item_len = sizeof(*di) + btrfs_dir_name_len(leaf, di) +
>  		btrfs_dir_data_len(leaf, di);
>  	item_len = btrfs_item_size_nr(leaf, path->slots[0]);
> -	if (sub_item_len == item_len) {
> +
> +	/*
> +	 * If @sub_item_len is longer than @item_len, then it means the
> +	 * name_len is just corrupted.
> +	 * No good idea to know if there is anything we can recover from
> +	 * the corrupted item.
> +	 * Just delete the item.
> +	 */
> +	if (sub_item_len >= item_len) {
>  		ret = btrfs_del_item(trans, root, path);
>  	} else {
>  		unsigned long ptr = (unsigned long)di;
> 

Re: [PATCH v2 2/3] btrfs-progs: dir-item: Don't do extra filetype validaction check for btrfs_match_dir_item_name

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 3381 bytes --]



On 2018年01月19日 17:40, Nikolay Borisov wrote:
> 
> 
> On 19.01.2018 09:25, Qu Wenruo wrote:
>> btrfs_match_dir_item_name() will check if its filetype is valid before
>> doing search, this makes btrfs-progs unable to locate and remove invalid
>> dir_index for btrfs_unlink().>
>> This function only affects btrfs_link() and btrfs_unlink() in upper
>> layer, and normal check can find invalid filetype by itself.
> 
> There is no function btrfs_link in btrfs-progs, there is, however,
> btrfs_add_link did you mean that function?

Yep, sorry for the wrong name.

> However, it doesn't seem to
> use verify_dir_item hence the check you are removing. I think this part
> of the commit log should be more precise. Also it's not really clear
> what you mean by "normal check" in this sentence.

I skipped several function calls.

btrfs_add_link()
|- check_dir_conflict
   |- btrfs_lookup_dir_index() / btrfs_lookup_dir_item()
      |- btrfs_match_dir_item_name()
         |- verify_dir_item()

And for btrfs_add_link() without checking invalid filetype, we can avoid
case like inserting duplicated DIR_ITEM/DIR_INDEX to avoid further
screwing up things.

For btrfs_unlink() it will be different story, as btrfs_unlink() can
locate the conflicting but invalid DIR_ITEM/DIR_INDEX and remove it,
allow us to fix the problem.

And by "normal check" I mean btrfs check.

I'll add the call trace to it.

>>
>> So remove the filetype check is completely safe in this case, and will
>> enhance btrfs_unlink() to remove invalid dir_index/dir_item for repair.
> 
> So the problem is that since btrfs_unlink calls verify_item and the
> latter has the filetype check in case of wrong filetype (corruption)
> verify_dir_item fails, hence we cannot perform the unlink? If my
> understanding is correct how about something like:
> 
> 
> If we have a corrupted dir item and enough information to repair it we
> need to first delete the old/corrupted version and then insert a new
> one.
> However, btrfs_unlink calls btrfs_match_dir_item_name to locate the
> offending dir item for deletion. The latter, in turn, uses
> verify_dir_item which checks if the value for DIR item's type is sane.
> In case of a corrupted type value then verify_dir_item will fail which
> in turn will prevent btrfs_unlink from deleting the offending item.

Your understanding is completely right, and much shorter than my planned
explanation.

I'll take it with extra call trace.

Thanks,
Qu

> 
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>  dir-item.c | 6 ------
>>  1 file changed, 6 deletions(-)
>>
>> diff --git a/dir-item.c b/dir-item.c
>> index 462546c0eaf4..e0a0ab4d7a5d 100644
>> --- a/dir-item.c
>> +++ b/dir-item.c
>> @@ -294,12 +294,6 @@ static int verify_dir_item(struct btrfs_root *root,
>>  	u16 namelen = BTRFS_NAME_LEN;
>>  	u8 type = btrfs_dir_type(leaf, dir_item);
>>  
>> -	if (type >= BTRFS_FT_MAX) {
>> -		fprintf(stderr, "invalid dir item type: %d\n",
>> -		       (int)type);
>> -		return 1;
>> -	}
>> -
>>  	if (type == BTRFS_FT_XATTR)
>>  		namelen = XATTR_NAME_MAX;
>>  
>>
> --
> 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
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 520 bytes --]

Re: [PATCH v2 2/3] btrfs-progs: dir-item: Don't do extra filetype validaction check for btrfs_match_dir_item_name

[Nikolay Borisov]

On 19.01.2018 12:03, Qu Wenruo wrote:
> 
> 
> On 2018年01月19日 17:40, Nikolay Borisov wrote:
>>
>>
>> On 19.01.2018 09:25, Qu Wenruo wrote:
>>> btrfs_match_dir_item_name() will check if its filetype is valid before
>>> doing search, this makes btrfs-progs unable to locate and remove invalid
>>> dir_index for btrfs_unlink().>
>>> This function only affects btrfs_link() and btrfs_unlink() in upper
>>> layer, and normal check can find invalid filetype by itself.
>>
>> There is no function btrfs_link in btrfs-progs, there is, however,
>> btrfs_add_link did you mean that function?
> 
> Yep, sorry for the wrong name.
> 
>> However, it doesn't seem to
>> use verify_dir_item hence the check you are removing. I think this part
>> of the commit log should be more precise. Also it's not really clear
>> what you mean by "normal check" in this sentence.
> 
> I skipped several function calls.
> 
> btrfs_add_link()
> |- check_dir_conflict
>    |- btrfs_lookup_dir_index() / btrfs_lookup_dir_item()
>       |- btrfs_match_dir_item_name()
>          |- verify_dir_item()
> 
> And for btrfs_add_link() without checking invalid filetype, we can avoid
> case like inserting duplicated DIR_ITEM/DIR_INDEX to avoid further
> screwing up things.

Right, since if verify_dir_item fails we just return NULL as opposed to
an error from btrfs_match_dir_item_name.

<offtopic>
Hm, so is it really possible to get a result for (key DIR_ITEM offset)
and have btrfs_match_dir_item_name fail for that result?

What I'm getting is that if btrfs_Search_slot returns 0 for such a query
irrespective of whether the string name matches or not we should return
some error code other than NULL ?
</offtopic>


> 
> For btrfs_unlink() it will be different story, as btrfs_unlink() can
> locate the conflicting but invalid DIR_ITEM/DIR_INDEX and remove it,
> allow us to fix the problem.
> 
> And by "normal check" I mean btrfs check.
> 
> I'll add the call trace to it.
> 
>>>
>>> So remove the filetype check is completely safe in this case, and will
>>> enhance btrfs_unlink() to remove invalid dir_index/dir_item for repair.
>>
>> So the problem is that since btrfs_unlink calls verify_item and the
>> latter has the filetype check in case of wrong filetype (corruption)
>> verify_dir_item fails, hence we cannot perform the unlink? If my
>> understanding is correct how about something like:
>>
>>
>> If we have a corrupted dir item and enough information to repair it we
>> need to first delete the old/corrupted version and then insert a new
>> one.
>> However, btrfs_unlink calls btrfs_match_dir_item_name to locate the
>> offending dir item for deletion. The latter, in turn, uses
>> verify_dir_item which checks if the value for DIR item's type is sane.
>> In case of a corrupted type value then verify_dir_item will fail which
>> in turn will prevent btrfs_unlink from deleting the offending item.
> 
> Your understanding is completely right, and much shorter than my planned
> explanation.
> 
> I'll take it with extra call trace.
> 
> Thanks,
> Qu
> 
>>
>>>
>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>> ---
>>>  dir-item.c | 6 ------
>>>  1 file changed, 6 deletions(-)
>>>
>>> diff --git a/dir-item.c b/dir-item.c
>>> index 462546c0eaf4..e0a0ab4d7a5d 100644
>>> --- a/dir-item.c
>>> +++ b/dir-item.c
>>> @@ -294,12 +294,6 @@ static int verify_dir_item(struct btrfs_root *root,
>>>  	u16 namelen = BTRFS_NAME_LEN;
>>>  	u8 type = btrfs_dir_type(leaf, dir_item);
>>>  
>>> -	if (type >= BTRFS_FT_MAX) {
>>> -		fprintf(stderr, "invalid dir item type: %d\n",
>>> -		       (int)type);
>>> -		return 1;
>>> -	}
>>> -
>>>  	if (type == BTRFS_FT_XATTR)
>>>  		namelen = XATTR_NAME_MAX;
>>>  
>>>
>> --
>> 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
>>
> 

Re: [PATCH v2 2/3] btrfs-progs: dir-item: Don't do extra filetype validaction check for btrfs_match_dir_item_name

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 4544 bytes --]



On 2018年01月19日 18:21, Nikolay Borisov wrote:
> 
> 
> On 19.01.2018 12:03, Qu Wenruo wrote:
>>
>>
>> On 2018年01月19日 17:40, Nikolay Borisov wrote:
>>>
>>>
>>> On 19.01.2018 09:25, Qu Wenruo wrote:
>>>> btrfs_match_dir_item_name() will check if its filetype is valid before
>>>> doing search, this makes btrfs-progs unable to locate and remove invalid
>>>> dir_index for btrfs_unlink().>
>>>> This function only affects btrfs_link() and btrfs_unlink() in upper
>>>> layer, and normal check can find invalid filetype by itself.
>>>
>>> There is no function btrfs_link in btrfs-progs, there is, however,
>>> btrfs_add_link did you mean that function?
>>
>> Yep, sorry for the wrong name.
>>
>>> However, it doesn't seem to
>>> use verify_dir_item hence the check you are removing. I think this part
>>> of the commit log should be more precise. Also it's not really clear
>>> what you mean by "normal check" in this sentence.
>>
>> I skipped several function calls.
>>
>> btrfs_add_link()
>> |- check_dir_conflict
>>    |- btrfs_lookup_dir_index() / btrfs_lookup_dir_item()
>>       |- btrfs_match_dir_item_name()
>>          |- verify_dir_item()
>>
>> And for btrfs_add_link() without checking invalid filetype, we can avoid
>> case like inserting duplicated DIR_ITEM/DIR_INDEX to avoid further
>> screwing up things.
> 
> Right, since if verify_dir_item fails we just return NULL as opposed to
> an error from btrfs_match_dir_item_name.
> 
> <offtopic>
> Hm, so is it really possible to get a result for (key DIR_ITEM offset)
> and have btrfs_match_dir_item_name fail for that result?
> 
> What I'm getting is that if btrfs_Search_slot returns 0 for such a query
> irrespective of whether the string name matches or not we should return
> some error code other than NULL ?

This sounds reasonable, but it may end up as replacing NULL to
PTR_ERR(-ENOENT).
Not much different for callers though.

Thanks,
Qu

> </offtopic>
> 
> 
>>
>> For btrfs_unlink() it will be different story, as btrfs_unlink() can
>> locate the conflicting but invalid DIR_ITEM/DIR_INDEX and remove it,
>> allow us to fix the problem.
>>
>> And by "normal check" I mean btrfs check.
>>
>> I'll add the call trace to it.
>>
>>>>
>>>> So remove the filetype check is completely safe in this case, and will
>>>> enhance btrfs_unlink() to remove invalid dir_index/dir_item for repair.
>>>
>>> So the problem is that since btrfs_unlink calls verify_item and the
>>> latter has the filetype check in case of wrong filetype (corruption)
>>> verify_dir_item fails, hence we cannot perform the unlink? If my
>>> understanding is correct how about something like:
>>>
>>>
>>> If we have a corrupted dir item and enough information to repair it we
>>> need to first delete the old/corrupted version and then insert a new
>>> one.
>>> However, btrfs_unlink calls btrfs_match_dir_item_name to locate the
>>> offending dir item for deletion. The latter, in turn, uses
>>> verify_dir_item which checks if the value for DIR item's type is sane.
>>> In case of a corrupted type value then verify_dir_item will fail which
>>> in turn will prevent btrfs_unlink from deleting the offending item.
>>
>> Your understanding is completely right, and much shorter than my planned
>> explanation.
>>
>> I'll take it with extra call trace.
>>
>> Thanks,
>> Qu
>>
>>>
>>>>
>>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>>> ---
>>>>  dir-item.c | 6 ------
>>>>  1 file changed, 6 deletions(-)
>>>>
>>>> diff --git a/dir-item.c b/dir-item.c
>>>> index 462546c0eaf4..e0a0ab4d7a5d 100644
>>>> --- a/dir-item.c
>>>> +++ b/dir-item.c
>>>> @@ -294,12 +294,6 @@ static int verify_dir_item(struct btrfs_root *root,
>>>>  	u16 namelen = BTRFS_NAME_LEN;
>>>>  	u8 type = btrfs_dir_type(leaf, dir_item);
>>>>  
>>>> -	if (type >= BTRFS_FT_MAX) {
>>>> -		fprintf(stderr, "invalid dir item type: %d\n",
>>>> -		       (int)type);
>>>> -		return 1;
>>>> -	}
>>>> -
>>>>  	if (type == BTRFS_FT_XATTR)
>>>>  		namelen = XATTR_NAME_MAX;
>>>>  
>>>>
>>> --
>>> 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
>>>
>>
> --
> 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
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 520 bytes --]

Re: [PATCH v2 2/3] btrfs-progs: dir-item: Don't do extra filetype validaction check for btrfs_match_dir_item_name

[Nikolay Borisov]

On 19.01.2018 12:39, Qu Wenruo wrote:
> 
> 
> On 2018年01月19日 18:21, Nikolay Borisov wrote:
>>
>>
>> On 19.01.2018 12:03, Qu Wenruo wrote:
>>>
>>>
>>> On 2018年01月19日 17:40, Nikolay Borisov wrote:
>>>>
>>>>
>>>> On 19.01.2018 09:25, Qu Wenruo wrote:
>>>>> btrfs_match_dir_item_name() will check if its filetype is valid before
>>>>> doing search, this makes btrfs-progs unable to locate and remove invalid
>>>>> dir_index for btrfs_unlink().>
>>>>> This function only affects btrfs_link() and btrfs_unlink() in upper
>>>>> layer, and normal check can find invalid filetype by itself.
>>>>
>>>> There is no function btrfs_link in btrfs-progs, there is, however,
>>>> btrfs_add_link did you mean that function?
>>>
>>> Yep, sorry for the wrong name.
>>>
>>>> However, it doesn't seem to
>>>> use verify_dir_item hence the check you are removing. I think this part
>>>> of the commit log should be more precise. Also it's not really clear
>>>> what you mean by "normal check" in this sentence.
>>>
>>> I skipped several function calls.
>>>
>>> btrfs_add_link()
>>> |- check_dir_conflict
>>>    |- btrfs_lookup_dir_index() / btrfs_lookup_dir_item()
>>>       |- btrfs_match_dir_item_name()
>>>          |- verify_dir_item()
>>>
>>> And for btrfs_add_link() without checking invalid filetype, we can avoid
>>> case like inserting duplicated DIR_ITEM/DIR_INDEX to avoid further
>>> screwing up things.
>>
>> Right, since if verify_dir_item fails we just return NULL as opposed to
>> an error from btrfs_match_dir_item_name.
>>
>> <offtopic>
>> Hm, so is it really possible to get a result for (key DIR_ITEM offset)
>> and have btrfs_match_dir_item_name fail for that result?
>>
>> What I'm getting is that if btrfs_Search_slot returns 0 for such a query
>> irrespective of whether the string name matches or not we should return
>> some error code other than NULL ?
> 
> This sounds reasonable, but it may end up as replacing NULL to
> PTR_ERR(-ENOENT).

Well in the case of check_dir_conflict it makes a whole lot of
difference. Currently if verify_dir_item fails then we propagate NULL up
to check_dir_conflict where we have the following error handling:

if (IS_ERR(dir_item)) {
                ret = PTR_ERR(dir_item);

                goto out;

        }

        if (dir_item) {

                ret = -EEXIST;

                goto out;

        }

so NULL won't really trigger anything, OTOH if we return an error and
IS_ERR triggers we won't add a duplicate item.

Be that as it may, I still think your patch as-is with modified commit
message is a good enough fix.

> Not much different for callers though.
> 
> Thanks,
> Qu
> 
>> </offtopic>
>>
>>
>>>
>>> For btrfs_unlink() it will be different story, as btrfs_unlink() can
>>> locate the conflicting but invalid DIR_ITEM/DIR_INDEX and remove it,
>>> allow us to fix the problem.
>>>
>>> And by "normal check" I mean btrfs check.
>>>
>>> I'll add the call trace to it.
>>>
>>>>>
>>>>> So remove the filetype check is completely safe in this case, and will
>>>>> enhance btrfs_unlink() to remove invalid dir_index/dir_item for repair.
>>>>
>>>> So the problem is that since btrfs_unlink calls verify_item and the
>>>> latter has the filetype check in case of wrong filetype (corruption)
>>>> verify_dir_item fails, hence we cannot perform the unlink? If my
>>>> understanding is correct how about something like:
>>>>
>>>>
>>>> If we have a corrupted dir item and enough information to repair it we
>>>> need to first delete the old/corrupted version and then insert a new
>>>> one.
>>>> However, btrfs_unlink calls btrfs_match_dir_item_name to locate the
>>>> offending dir item for deletion. The latter, in turn, uses
>>>> verify_dir_item which checks if the value for DIR item's type is sane.
>>>> In case of a corrupted type value then verify_dir_item will fail which
>>>> in turn will prevent btrfs_unlink from deleting the offending item.
>>>
>>> Your understanding is completely right, and much shorter than my planned
>>> explanation.
>>>
>>> I'll take it with extra call trace.
>>>
>>> Thanks,
>>> Qu
>>>
>>>>
>>>>>
>>>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>>>> ---
>>>>>  dir-item.c | 6 ------
>>>>>  1 file changed, 6 deletions(-)
>>>>>
>>>>> diff --git a/dir-item.c b/dir-item.c
>>>>> index 462546c0eaf4..e0a0ab4d7a5d 100644
>>>>> --- a/dir-item.c
>>>>> +++ b/dir-item.c
>>>>> @@ -294,12 +294,6 @@ static int verify_dir_item(struct btrfs_root *root,
>>>>>  	u16 namelen = BTRFS_NAME_LEN;
>>>>>  	u8 type = btrfs_dir_type(leaf, dir_item);
>>>>>  
>>>>> -	if (type >= BTRFS_FT_MAX) {
>>>>> -		fprintf(stderr, "invalid dir item type: %d\n",
>>>>> -		       (int)type);
>>>>> -		return 1;
>>>>> -	}
>>>>> -
>>>>>  	if (type == BTRFS_FT_XATTR)
>>>>>  		namelen = XATTR_NAME_MAX;
>>>>>  
>>>>>
>>>> --
>>>> 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
>>>>
>>>
>> --
>> 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 v2.1 2/3] btrfs-progs: dir-item: Don't do extra filetype validaction check for btrfs_match_dir_item_name

[Qu Wenruo]

verify_dir_item() is called in btrfs_match_dir_item_name() to ensure we
won't search beyond item boundary and does extra filetype check.

However in the follow call chain, such extra filetype check can cause
problems:

1) btrfs_add_link()
   |- check_dir_conflict()
      |- btrfs_lookup_dir_index()
         |- btrfs_match_dir_item_name()

   And if we have an offending dir index whose filetype is invalid,
   btrfs_match_dir_item_name() will return NULL, meaning no match dir
   index is found.
   So btrfs_add_link() will still try to insert a dir index, which may
   have same key->offset and leading to duplicated dir index.

2) btrfs_unlink()
   |- btrfs_lookup_dir_index()
      |- btrfs_lookup_dir_index()
         |- btrfs_match_dir_item_name()

   For the same offending dir index with invalid filetype, this will
   return NULL, and btrfs_unlink() will just consider there is no
   existing dir_index and do nothing.
   Leave an orphan and invalid dir_index hanging there forever.

The patch removes the extra filetype check, as "btrfs check" can already
handle invalid filetype correctly for both modes.

And this makes "btrfs check --repair --mode=lowmem" to delete the
offending dir index to repair it correctly.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
v2:
  Get rid of the new parameter.
v2.1:
  Better commit message.
---
 dir-item.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/dir-item.c b/dir-item.c
index 462546c0eaf4..e0a0ab4d7a5d 100644
--- a/dir-item.c
+++ b/dir-item.c
@@ -294,12 +294,6 @@ static int verify_dir_item(struct btrfs_root *root,
 	u16 namelen = BTRFS_NAME_LEN;
 	u8 type = btrfs_dir_type(leaf, dir_item);
 
-	if (type >= BTRFS_FT_MAX) {
-		fprintf(stderr, "invalid dir item type: %d\n",
-		       (int)type);
-		return 1;
-	}
-
 	if (type == BTRFS_FT_XATTR)
 		namelen = XATTR_NAME_MAX;
 
-- 
2.15.1

Re: [PATCH v2.1 2/3] btrfs-progs: dir-item: Don't do extra filetype validaction check for btrfs_match_dir_item_name

[Nikolay Borisov]

On 22.01.2018 07:45, Qu Wenruo wrote:
> verify_dir_item() is called in btrfs_match_dir_item_name() to ensure we
> won't search beyond item boundary and does extra filetype check.
> 
> However in the follow call chain, such extra filetype check can cause
> problems:
> 
> 1) btrfs_add_link()
>    |- check_dir_conflict()
>       |- btrfs_lookup_dir_index()
>          |- btrfs_match_dir_item_name()
> 
>    And if we have an offending dir index whose filetype is invalid,
>    btrfs_match_dir_item_name() will return NULL, meaning no match dir
>    index is found.
>    So btrfs_add_link() will still try to insert a dir index, which may
>    have same key->offset and leading to duplicated dir index.
> 
> 2) btrfs_unlink()
>    |- btrfs_lookup_dir_index()
>       |- btrfs_lookup_dir_index()
>          |- btrfs_match_dir_item_name()
> 
>    For the same offending dir index with invalid filetype, this will
>    return NULL, and btrfs_unlink() will just consider there is no
>    existing dir_index and do nothing.
>    Leave an orphan and invalid dir_index hanging there forever.
> 
> The patch removes the extra filetype check, as "btrfs check" can already
> handle invalid filetype correctly for both modes.
> 
> And this makes "btrfs check --repair --mode=lowmem" to delete the
> offending dir index to repair it correctly.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>

Reviewed-by: Nikolay Borisov <nborisov@suse.com>
> ---
> v2:
>   Get rid of the new parameter.
> v2.1:
>   Better commit message.
> ---
>  dir-item.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/dir-item.c b/dir-item.c
> index 462546c0eaf4..e0a0ab4d7a5d 100644
> --- a/dir-item.c
> +++ b/dir-item.c
> @@ -294,12 +294,6 @@ static int verify_dir_item(struct btrfs_root *root,
>  	u16 namelen = BTRFS_NAME_LEN;
>  	u8 type = btrfs_dir_type(leaf, dir_item);
>  
> -	if (type >= BTRFS_FT_MAX) {
> -		fprintf(stderr, "invalid dir item type: %d\n",
> -		       (int)type);
> -		return 1;
> -	}
> -
>  	if (type == BTRFS_FT_XATTR)
>  		namelen = XATTR_NAME_MAX;
>  
>