[PATCHSET 0/2] xfs_repair: fix missing corruption detection

[PATCHSET 0/2] xfs_repair: fix missing corruption detection

[Darrick J. Wong]

Hi all,

As part of QAing the parent pointers patchset, I noticed a couple of bad
bugs in xfs_repair.  The first is our handling of directories containing
the same name more than once -- we flag that and store both names, but
then we rebuild the directory with both of the offending names!  So, fix
that.

The other problem I noticed is that if the inobt is broken, many inodes
end up in the uncertain list.  Those files will get added back to the
filesystem indexes without the attr forks being checked, which means
that any corruption in them will not be found or fixed.

If you're going to start using this mess, you probably ought to just
pull from my git trees, which are linked below.

This is an extraordinary way to destroy everything.  Enjoy!
Comments and questions are, as always, welcome.

--D

xfsprogs git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfsprogs-dev.git/log/?h=repair-fix-da-breakage
---
 repair/dino_chunks.c |    6 ++++--
 repair/phase6.c      |    3 ++-
 2 files changed, 6 insertions(+), 3 deletions(-)

[PATCH 1/2] xfs_repair: don't add junked entries to the rebuilt directory

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

If a directory contains multiple entries with the same name, we create
separate objects in the directory hashtab for each dirent.  The first
one has p->junkit==0, but the subsequent ones have p->junkit==1.
Because these are duplicate names that are not garbage, the first
character of p->name.name is not set to a slash.

Don't add these subsequent hashtab entries to the rebuilt directory.

Found by running xfs/155 with the parent pointers patchset enabled.

Fixes: 33165ec3b4b ("Fix dirv2 rebuild in phase6 Merge of master-melb:xfs-cmds:26664a by kenmcd.")
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 repair/phase6.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)


diff --git a/repair/phase6.c b/repair/phase6.c
index 48bf57359c5..37573b4301b 100644
--- a/repair/phase6.c
+++ b/repair/phase6.c
@@ -1319,7 +1319,8 @@ longform_dir2_rebuild(
 	/* go through the hash list and re-add the inodes */
 
 	for (p = hashtab->first; p; p = p->nextbyorder) {
-
+		if (p->junkit)
+			continue;
 		if (p->name.name[0] == '/' || (p->name.name[0] == '.' &&
 				(p->name.len == 1 || (p->name.len == 2 &&
 						p->name.name[1] == '.'))))

[PATCH 2/2] xfs_repair: always perform extended xattr checks on uncertain inodes

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

When we're processing uncertain inodes, we need to perform the extended
checks on the xattr structure, because the processing might decide that
an uncertain inode is in fact a certain inode, and to restore it to the
filesystem.  If that's the case, xfs_repair fails to catch problems in
the attr structure.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 repair/dino_chunks.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)


diff --git a/repair/dino_chunks.c b/repair/dino_chunks.c
index 0e09132b0b1..cf6a5e399d4 100644
--- a/repair/dino_chunks.c
+++ b/repair/dino_chunks.c
@@ -1289,10 +1289,12 @@ process_uncertain_aginodes(xfs_mount_t *mp, xfs_agnumber_t agno)
 			 * process the inode record we just added
 			 * to the good inode tree.  The inode
 			 * processing may add more records to the
-			 * uncertain inode lists.
+			 * uncertain inode lists.  always process the
+			 * extended attribute structure because we might
+			 * decide that some inodes are still in use
 			 */
 			if (process_inode_chunk(mp, agno, igeo->ialloc_inos,
-						nrec, 1, 0, 0, &bogus))  {
+						nrec, 1, 0, 1, &bogus))  {
 				/* XXX - i/o error, we've got a problem */
 				abort();
 			}

Re: [PATCH 1/2] xfs_repair: don't add junked entries to the rebuilt directory

[Pavel Reichl]

LGTM & Builds

Reviewed-by: Pavel Reichl <preichl@redhat.com>


I'm just wondering why junkit is defined as a short and not a boolean?

Re: [PATCH 2/2] xfs_repair: always perform extended xattr checks on uncertain inodes

[Pavel Reichl]

LGTM & Builds

Reviewed-by: Pavel Reichl <preichl@redhat.com>

Re: [PATCHSET 0/2] xfs_repair: fix missing corruption detection

[Carlos Maiolino]

On Mon, Jun 05, 2023 at 08:37:18AM -0700, Darrick J. Wong wrote:
> Hi all,
> 
> As part of QAing the parent pointers patchset, I noticed a couple of bad
> bugs in xfs_repair.  The first is our handling of directories containing
> the same name more than once -- we flag that and store both names, but
> then we rebuild the directory with both of the offending names!  So, fix
> that.
> 
> The other problem I noticed is that if the inobt is broken, many inodes
> end up in the uncertain list.  Those files will get added back to the
> filesystem indexes without the attr forks being checked, which means
> that any corruption in them will not be found or fixed.
> 
> If you're going to start using this mess, you probably ought to just
> pull from my git trees, which are linked below.
> 
> This is an extraordinary way to destroy everything.  Enjoy!
> Comments and questions are, as always, welcome.
> 

Looks good, will test.

Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>

-- 
Carlos

> --D
> 
> xfsprogs git tree:
> https://git.kernel.org/cgit/linux/kernel/git/djwong/xfsprogs-dev.git/log/?h=repair-fix-da-breakage
> ---
>  repair/dino_chunks.c |    6 ++++--
>  repair/phase6.c      |    3 ++-
>  2 files changed, 6 insertions(+), 3 deletions(-)
>