Lowmem fsck repair to fix filetype mismatch
v2 cover Lowmem fsck repair to fix filetype mismatch
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(-)
Signature · 1 line
2.15.1
PATCH v2 1/3 btrfs-progs: lowmem fsck: Remove corupted link before re-add correct link
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.
--- cmds-check.c | 4 ++++ 1 file changed, 4 insertions(+)
cmds-check.c +4 -0
@@ -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;
Signature · 1 line
2.15.1
Re: [PATCH v2 1/3] btrfs-progs: lowmem fsck: Remove corupted link before re-add correct link
Quoted 4 lines · 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.
Quoted 8 lines
However for case like invalid type in
DIR_INDEX, we should delete the corruptedDIR_INDEXfirst before inserting the correct link.This patch will remove the corrupted link before re-insert. This should solve the duplicated
DIR_INDEXproblem in old lowmem mode repair.Cc Sebastian Andrzej Siewior <[email protected]>
Quoted 21 lines
Signed-off-by Qu Wenruo <[email protected]>--- cmds-check.c | 4 ++++ 1 file changed, 4 insertions(+)cmds-check.c +4
-0@@ -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;
PATCH v2 2/3 btrfs-progs: dir-item: Don't do extra filetype validaction check for btrfs_match_dir_item_name
In reply to Qu Wenruobtrfs_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.
--- dir-item.c | 6 ------ 1 file changed, 6 deletions(-)
dir-item.c +0 -6
@@ -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 ;
Signature · 1 line
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
Quoted 7 lines · 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 invaliddir_indexforbtrfs_unlink().This function only affects
btrfs_link()andbtrfs_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.
Quoted 26 lines
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 <[email protected]>--- dir-item.c | 6 ------ 1 file changed, 6 deletions(-)dir-item.c +0
-6@@ -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 2/3] btrfs-progs: dir-item: Don't do extra filetype validaction check for btrfs_match_dir_item_name
Quoted 7 lines · Su Yue
Quoted 4 lines · 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 invaliddir_indexforbtrfs_unlink().
Quoted 5 lines
Quoted 3 lines
This function only affects
btrfs_link()andbtrfs_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
Quoted 33 lines
Reviewed-by Su Yue <[email protected]>Quoted 24 lines
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 <[email protected]>---
dir-item.c| 6 ------1 file changed, 6 deletions(-)dir-item.c +0
-5@@ -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;Signature · 3 lines
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected] 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
Quoted 10 lines · Qu Wenruo
Quoted 7 lines · Su Yue
Quoted 4 lines · 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 invaliddir_indexforbtrfs_unlink().
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
This function only affects
btrfs_link()andbtrfs_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
Quoted 48 lines
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,
QuQuoted 33 lines
Reviewed-by Su Yue <[email protected]>Quoted 24 lines
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 <[email protected]>--- dir-item.c | 6 ------ 1 file changed, 6 deletions(-)dir-item.c +0
-5@@ -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;Signature · 3 lines
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected] 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
In reply to Qu WenruoQuoted 5 lines · 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 invaliddir_indexforbtrfs_unlink().>
This function only affectsbtrfs_link()andbtrfs_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.
Quoted 3 lines
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.
Quoted 24 lines
Signed-off-by Qu Wenruo <[email protected]>--- dir-item.c | 6 ------ 1 file changed, 6 deletions(-)dir-item.c +0
-6@@ -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 2/3] btrfs-progs: dir-item: Don't do extra filetype validaction check for btrfs_match_dir_item_name
Quoted 11 lines · Nikolay Borisov
Quoted 5 lines · 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 invaliddir_indexforbtrfs_unlink().>
This function only affectsbtrfs_link()andbtrfs_unlink()in upper layer, and normal check can find invalid filetype by itself.There is no function
btrfs_linkin btrfs-progs, there is, however,btrfs_add_linkdid you mean that function?
Yep, sorry for the wrong name.
Quoted 4 lines
However, it doesn't seem to
useverify_dir_itemhence 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.
Quoted 10 lines
Quoted 3 lines
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_unlinkcallsverify_itemand the latter has the filetype check in case of wrong filetype (corruption)verify_dir_itemfails, hence we cannot perform the unlink? If my understanding is correct how about something like:
Quoted 8 lines
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_unlinkcallsbtrfs_match_dir_item_nameto locate the offending dir item for deletion. The latter, in turn, usesverify_dir_itemwhich checks if the value for DIR item's type is sane. In case of a corrupted type value thenverify_dir_itemwill fail which in turn will preventbtrfs_unlinkfrom 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
Quoted 30 lines
Quoted 15 lines
Signed-off-by Qu Wenruo <[email protected]>--- dir-item.c | 6 ------ 1 file changed, 6 deletions(-)dir-item.c +0
-0@@ -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);dir-item.c static int verify_dir_item(struct btrfs_root *root, · line 299
Quoted 10 lines
dir-item.c +0
-6-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 [email protected]
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
Quoted 27 lines · Qu Wenruo
Quoted 11 lines · Nikolay Borisov
Quoted 5 lines · 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 invaliddir_indexforbtrfs_unlink().>
This function only affectsbtrfs_link()andbtrfs_unlink()in upper layer, and normal check can find invalid filetype by itself.There is no function
btrfs_linkin btrfs-progs, there is, however,btrfs_add_linkdid you mean that function?Yep, sorry for the wrong name.
Quoted 4 lines
However, it doesn't seem to
useverify_dir_itemhence 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()
Quoted 6 lines
|- 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>
Quoted 67 lines
For
btrfs_unlink()it will be different story, asbtrfs_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.
Quoted 10 lines
Quoted 3 lines
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_unlinkcallsverify_itemand the latter has the filetype check in case of wrong filetype (corruption)verify_dir_itemfails, hence we cannot perform the unlink? If my understanding is correct how about something like:Quoted 8 lines
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_unlinkcallsbtrfs_match_dir_item_nameto locate the offending dir item for deletion. The latter, in turn, usesverify_dir_itemwhich checks if the value for DIR item's type is sane. In case of a corrupted type value thenverify_dir_itemwill fail which in turn will preventbtrfs_unlinkfrom 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,
QuQuoted 30 lines
Quoted 15 lines
Signed-off-by Qu Wenruo <[email protected]>--- dir-item.c | 6 ------ 1 file changed, 6 deletions(-)dir-item.c +0
-0@@ -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);dir-item.c static int verify_dir_item(struct btrfs_root *root, · line 299
Quoted 10 lines
dir-item.c +0
-6-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 [email protected]
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
Quoted 40 lines · Nikolay Borisov
Quoted 27 lines · Qu Wenruo
Quoted 11 lines · Nikolay Borisov
Quoted 5 lines · 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 invaliddir_indexforbtrfs_unlink().>
This function only affectsbtrfs_link()andbtrfs_unlink()in upper layer, and normal check can find invalid filetype by itself.There is no function
btrfs_linkin btrfs-progs, there is, however,btrfs_add_linkdid you mean that function?Yep, sorry for the wrong name.
Quoted 4 lines
However, it doesn't seem to
useverify_dir_itemhence 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()Quoted 6 lines
|- 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_itemfails we just return NULL as opposed to an error frombtrfs_match_dir_item_name.
Quoted 7 lines
<offtopic>
Hm, so is it really possible to get a result for (keyDIR_ITEMoffset) and havebtrfs_match_dir_item_namefail for that result?What I'm getting is that if
btrfs_Search_slotreturns 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
Quoted 75 lines
</offtopic>
Quoted 28 lines
For
btrfs_unlink()it will be different story, asbtrfs_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.
Quoted 10 lines
Quoted 3 lines
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_unlinkcallsverify_itemand the latter has the filetype check in case of wrong filetype (corruption)verify_dir_itemfails, hence we cannot perform the unlink? If my understanding is correct how about something like:Quoted 8 lines
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_unlinkcallsbtrfs_match_dir_item_nameto locate the offending dir item for deletion. The latter, in turn, usesverify_dir_itemwhich checks if the value for DIR item's type is sane. In case of a corrupted type value thenverify_dir_itemwill fail which in turn will preventbtrfs_unlinkfrom deleting the offending item.Quoted 39 lines
Your understanding is completely right, and much shorter than my planned explanation.
I'll take it with extra call trace.
Thanks,
QuQuoted 30 lines
Quoted 15 lines
Signed-off-by Qu Wenruo <[email protected]>--- dir-item.c | 6 ------ 1 file changed, 6 deletions(-)dir-item.c +0
-0@@ -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);dir-item.c static int verify_dir_item(struct btrfs_root *root, · line 299
Quoted 10 lines
dir-item.c +0
-6-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 [email protected]
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 [email protected]
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
Quoted 47 lines · Qu Wenruo
Quoted 44 lines · Nikolay Borisov
Quoted 27 lines · Qu Wenruo
Quoted 11 lines · Nikolay Borisov
Quoted 5 lines · 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 invaliddir_indexforbtrfs_unlink().>
This function only affectsbtrfs_link()andbtrfs_unlink()in upper layer, and normal check can find invalid filetype by itself.There is no function
btrfs_linkin btrfs-progs, there is, however,btrfs_add_linkdid you mean that function?Yep, sorry for the wrong name.
Quoted 4 lines
However, it doesn't seem to
useverify_dir_itemhence 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()Quoted 6 lines
|- 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_itemfails we just return NULL as opposed to an error frombtrfs_match_dir_item_name.<offtopic>
Hm, so is it really possible to get a result for (keyDIR_ITEMoffset) and havebtrfs_match_dir_item_namefail for that result?
Quoted 6 lines
Quoted 3 lines
What I'm getting is that if
btrfs_Search_slotreturns 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.
Quoted 81 lines
Not much different for callers though.
Thanks,
QuQuoted 75 lines
</offtopic>
Quoted 28 lines
For
btrfs_unlink()it will be different story, asbtrfs_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.
Quoted 10 lines
Quoted 3 lines
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_unlinkcallsverify_itemand the latter has the filetype check in case of wrong filetype (corruption)verify_dir_itemfails, hence we cannot perform the unlink? If my understanding is correct how about something like:Quoted 8 lines
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_unlinkcallsbtrfs_match_dir_item_nameto locate the offending dir item for deletion. The latter, in turn, usesverify_dir_itemwhich checks if the value for DIR item's type is sane. In case of a corrupted type value thenverify_dir_itemwill fail which in turn will preventbtrfs_unlinkfrom deleting the offending item.Quoted 39 lines
Your understanding is completely right, and much shorter than my planned explanation.
I'll take it with extra call trace.
Thanks,
QuQuoted 30 lines
Quoted 15 lines
Signed-off-by Qu Wenruo <[email protected]>--- dir-item.c | 6 ------ 1 file changed, 6 deletions(-)dir-item.c +0
-0@@ -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);dir-item.c static int verify_dir_item(struct btrfs_root *root, · line 299
Quoted 10 lines
dir-item.c +0
-6-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 [email protected]
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH v2 2/3 btrfs-progs: dir-item: Don't do extra filetype validaction check for btrfs_match_dir_item_name
In reply to Qu Wenruoverify_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.
---
v2:
Get rid of the new parameter.
v2.1:
Better commit message.
--- dir-item.c | 6 ------ 1 file changed, 6 deletions(-)
dir-item.c +0 -6
@@ -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 ;
Signature · 1 line
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
Quoted 27 lines · Qu Wenruo
verify_dir_item()is called inbtrfs_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.
Sobtrfs_add_link()will still try to insert a dir index, which may have samekey->offsetand 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 existingdir_indexand do nothing.
Leave an orphan and invaliddir_indexhanging there forever.
Quoted 7 lines
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 <[email protected]>
Quoted 27 lines
---
v2:
Get rid of the new parameter.
v2.1:
Better commit message.--- dir-item.c | 6 ------ 1 file changed, 6 deletions(-)dir-item.c +0
-6@@ -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 ;
PATCH v2 3/3 btrfs-progs: dir-item: Make btrfs_delete_one_dir_name more robust to handle corrupted name len
In reply to Qu WenruoFunction 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.
--- dir-item.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)
dir-item.c +9 -2
@@ -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;
Signature · 1 line
2.15.1
Re: [PATCH v2 3/3] btrfs-progs: dir-item: Make btrfs_delete_one_dir_name more robust to handle corrupted name len
Quoted 10 lines · Qu Wenruo
Function
btrfs_delete_one_dir_name()will check if thedir_itemis 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.
Quoted 35 lines
Signed-off-by Qu Wenruo <[email protected]>--- dir-item.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)dir-item.c +9
-2@@ -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 3/3] btrfs-progs: dir-item: Make btrfs_delete_one_dir_name more robust to handle corrupted name len
In reply to Qu WenruoQuoted 11 lines · Qu Wenruo
Function
btrfs_delete_one_dir_name()will check if thedir_itemis 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 <[email protected]>
Perhaps it would be worth it creating a regression test for that ?
Quoted 34 lines
--- dir-item.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)dir-item.c +9
-2@@ -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;