Re: [RFC PATCH v2] xfsrestore: fix rootdir due to xfsdump bulkstat misuse

[Gao Xiang <hsiangkao@redhat.com>]

On Mon, Nov 16, 2020 at 04:07:23PM +0800, Gao Xiang wrote:
> If rootino is wrong and misspecified to a subdir inode #,
> the following assertion could be triggered:
>   assert(ino != persp->p_rootino || hardh == persp->p_rooth);
> 
> This patch adds a '-x' option (another awkward thing is that
> the codebase doesn't support long options) to address
> problamatic images by searching for the real dir, the reason
> that I don't enable it by default is that I'm not very confident
> with the xfsrestore codebase and xfsdump bulkstat issue will
> also be fixed immediately as well, so this function might be
> optional and only useful for pre-exist corrupted dumps.
> 
> In details, my understanding of the original logic is
>  1) xfsrestore will create a rootdir node_t (p_rooth);
>  2) it will build the tree hierarchy from inomap and adopt
>     the parent if needed (so inodes whose parent ino hasn't
>     detected will be in the orphan dir, p_orphh);
>  3) during this period, if ino == rootino and
>     hardh != persp->p_rooth (IOWs, another node_t with
>     the same ino # is created), that'd be definitely wrong.
> 
> So the proposal fix is that
>  - considering the xfsdump root ino # is a subdir inode, it'll
>    trigger ino == rootino && hardh != persp->p_rooth condition;
>  - so we log this node_t as persp->p_rooth rather than the
>    initial rootdir node_t created in 1);
>  - we also know that this node is actually a subdir, and after
>    the whole inomap is scanned (IOWs, the tree is built),
>    the real root dir will have the orphan dir parent p_orphh;
>  - therefore, we walk up by the parent until some node_t has
>    the p_orphh, so that's it.
> 
> Cc: Donald Douwsma <ddouwsma@redhat.com>
> Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
> ---
> changes since RFC v1:
>  - fix non-dir fake rootino cases since tree_begindir()
>    won't be triggered for these non-dir, and we could do
>    that in tree_addent() instead (fault injection verified);
> 
>  - fix fake rootino case with gen = 0 as well, for more
>    details, see the inlined comment in link_hardh()
>    (fault injection verified as well).
> 
> Anyway, all of this behaves like a workaround and I have
> no idea how to deal it more gracefully.
>

My manual fault injection patch:
 - inject a non-dir fake rootino;
 - inject all inode gen = 0 (to check the fake rootino case with gen = 0);
 - disable the fake rootino case with gen = 0 workaround, and see
    xfsrestore: tree.c:1003: tree_addent: Assertion `hardp->n_nrh != NRH_NULL' failed.
   could happen.


diff --git a/dump/content.c b/dump/content.c
index c11d9b4..2d27d24 100644
--- a/dump/content.c
+++ b/dump/content.c
@@ -1509,6 +1509,7 @@ baseuuidbypass:
 	}
 
 	scwhdrtemplatep->cih_rootino = sc_rootxfsstatp->bs_ino;
+	scwhdrtemplatep->cih_rootino = 25166002;	/* inject some real non-dir ino # */
 	inomap_writehdr(scwhdrtemplatep);
 
 	/* log the dump size. just a rough approx.
diff --git a/restore/content.c b/restore/content.c
index e807a83..f493fdb 100644
--- a/restore/content.c
+++ b/restore/content.c
@@ -8143,7 +8143,7 @@ read_filehdr(drive_t *drivep, filehdr_t *fhdrp, bool_t fhcs)
 			return RV_CORRUPT;
 		}
 	}
-
+	bstatp->bs_gen = 0;
 	return RV_OK;
 }
 
@@ -8277,6 +8277,8 @@ read_dirent(drive_t *drivep,
 		}
 	}
 
+	dhdrp->dh_gen = 0;
+
 	mlog(MLOG_NITTY,
 	      "read dirent hdr ino %llu gen %u size %u\n",
 	      dhdrp->dh_ino,
diff --git a/restore/tree.c b/restore/tree.c
index 918fa01..2d8dec5 100644
--- a/restore/tree.c
+++ b/restore/tree.c
@@ -3886,6 +3886,7 @@ link_hardh(xfs_ino_t ino, gen_t gen)
 {
 	nh_t tmp = hash_find(ino, gen);
 
+#if 0
 	/*
 	 * XXX (another workaround): the simply way is that don't reuse node_t
 	 * with gen = 0 created in tree_init(). Otherwise, it could cause
@@ -3903,6 +3904,7 @@ _("link out fake rootino %llu with gen=0 created in tree_init()\n"), ino);
 		orig_rooth = tmp;
 		return NH_NULL;
 	}
+#endif
 	return tmp;
 }
 
-- 
2.18.4