[RFC PATCH] xfs: test for fixing wrong root inode number

[RFC PATCH] xfs: test for fixing wrong root inode number

[Hironori Shiina]

Test '-x' option of xfsrestore. With this option, a wrong root inode
number is corrected. A root inode number can be wrong in a dump created
by problematic xfsdump (v3.1.7 - v3.1.9) with blukstat misuse. This
patch adds a dump with a wrong inode number created by xfsdump 3.1.8.

Link: https://lore.kernel.org/linux-xfs/20201113125127.966243-1-hsiangkao@redhat.com/
Signed-off-by: Hironori Shiina <shiina.hironori@fujitsu.com>
---
 common/dump                    |   2 +-
 src/root-inode-broken-dumpfile | Bin 0 -> 21648 bytes
 tests/xfs/554                  |  37 +++++++++++++++++++++
 tests/xfs/554.out              |  57 +++++++++++++++++++++++++++++++++
 4 files changed, 95 insertions(+), 1 deletion(-)
 create mode 100644 src/root-inode-broken-dumpfile
 create mode 100644 tests/xfs/554
 create mode 100644 tests/xfs/554.out

diff --git a/common/dump b/common/dump
index 8e0446d9..50b2ba03 100644
--- a/common/dump
+++ b/common/dump
@@ -1003,7 +1003,7 @@ _parse_restore_args()
         --no-check-quota)
             do_quota_check=false
             ;;
-	-K|-R)
+	-K|-R|-x)
 	    restore_args="$restore_args $1"
             ;;
 	*)
diff --git a/src/root-inode-broken-dumpfile b/src/root-inode-broken-dumpfile
new file mode 100644
index 0000000000000000000000000000000000000000..9a42e65d8047497be31f3abbaf4223ae384fd14d
GIT binary patch
literal 21648
zcmeI)K}ge49KiAC_J<CEBr!0AwBf~zbCHB}5DyxL6eU8Jnqylvayj;2VuG+d)TN6;
z5J8vlAaw9hXi(UousT$Sin@BRJfwHM)O+*2*njz_zc+h*ckuV#@BMuKe;;JbgTL{<
z!SwZ9zC#ERe%reLe5&cz2e}qM<!i2dXQHt^{^`d1Qx{)MMzW#$%dR@J={0`IRsGx4
z(yn@Oi-nBqCOVIG?&{kpwnv~&w_>6_ozY1^fph=w8(=^oTg&wOe=(WQByyQ_Hfd|4
z^o76<0!zn#v>k3blbU)nzx=KF^}-G%R;OaQYsHwGDkO`kD^@q^(_Ac_8H<gKj^>a0
z6p*%BK>q#b>2EE%^!@f?Z`-p+tI@M}qmMm@zMJk>K1U^=d~G^NUG?X4vkvKtOf-3Y
zpVM9YgM9Y7{*P0ApJNUh^uk1wCnA6V0tg_000IagfB*srAb<b@2q1s}0tg_000Iag
zfB*srAb<b@2q1s}0tg_000IagfB*srAb<b@2q1s}0tg_000IagfB*srAb`MM1p>_h
z2-R=Q9ooK1{l9<Dy8IIMUVXr9BW9uI1x7};j+kij|5kLI^32no;n|PvqD74ZOlJ#n
z0-~CKSlx#liMS>jt232#==00xPqwp;gsZrjc?`PPxaCE~>3(^o5-%+Nj=HegJFK2b
z=l5zT4IDgO=sJ1zp=jyrALvcQJK|j;sN2_jUmobjN<!S6m1{G<LZ^+JP;T!|3{9)w
eGf&ioo}iw|li2&4IyG;z<}vrl)Mic2`t2{52##q0

literal 0
HcmV?d00001

diff --git a/tests/xfs/554 b/tests/xfs/554
new file mode 100644
index 00000000..13bc62c7
--- /dev/null
+++ b/tests/xfs/554
@@ -0,0 +1,37 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2022 Fujitsu Limited. All Rights Reserved.
+#
+# FS QA Test No. 554
+#
+# Test restoring a dumpfile with a wrong root inode number created by
+# xfsdump 3.1.8.
+# This test restores the checked-in broken dump with '-x' flag.
+#
+
+. ./common/preamble
+_begin_fstest auto quick dump
+
+# Import common functions.
+. ./common/dump
+
+# real QA test starts here
+_supported_fs xfs
+_require_scratch
+_scratch_mkfs_xfs >>$seqres.full || _fail "mkfs failed"
+_scratch_mount
+
+# Create dumpdir for comparing with restoredir
+rm -rf $dump_dir
+mkdir $dump_dir || _fail "failed to mkdir $restore_dir"
+touch $dump_dir/FILE_1019
+
+_do_restore_toc -x -f $here/src/root-inode-broken-dumpfile
+
+_do_restore_file -x -f $here/src/root-inode-broken-dumpfile -L stress_545
+_diff_compare_sub
+_ls_nodate_compare_sub
+
+# success, all done
+status=0
+exit
diff --git a/tests/xfs/554.out b/tests/xfs/554.out
new file mode 100644
index 00000000..40a3f3a4
--- /dev/null
+++ b/tests/xfs/554.out
@@ -0,0 +1,57 @@
+QA output created by 554
+Contents of dump ...
+xfsrestore  -x -f DUMP_FILE -t
+xfsrestore: using file dump (drive_simple) strategy
+xfsrestore: searching media for dump
+xfsrestore: examining media file 0
+xfsrestore: dump description: 
+xfsrestore: hostname: xfsdump
+xfsrestore: mount point: SCRATCH_MNT
+xfsrestore: volume: SCRATCH_DEV
+xfsrestore: session time: TIME
+xfsrestore: level: 0
+xfsrestore: session label: "stress_545"
+xfsrestore: media label: "stress_tape_media"
+xfsrestore: file system ID: ID
+xfsrestore: session id: ID
+xfsrestore: media ID: ID
+xfsrestore: searching media for directory dump
+xfsrestore: reading directories
+xfsrestore: found fake rootino #128, will fix.
+xfsrestore: fix root # to 1024 (bind mount?)
+xfsrestore: 2 directories and 2 entries processed
+xfsrestore: directory post-processing
+xfsrestore: reading non-directory files
+xfsrestore: table of contents display complete: SECS seconds elapsed
+xfsrestore: Restore Status: SUCCESS
+
+dumpdir/FILE_1019
+Restoring from file...
+xfsrestore  -x -f DUMP_FILE  -L stress_545 RESTORE_DIR
+xfsrestore: using file dump (drive_simple) strategy
+xfsrestore: searching media for dump
+xfsrestore: examining media file 0
+xfsrestore: found dump matching specified label:
+xfsrestore: hostname: xfsdump
+xfsrestore: mount point: SCRATCH_MNT
+xfsrestore: volume: SCRATCH_DEV
+xfsrestore: session time: TIME
+xfsrestore: level: 0
+xfsrestore: session label: "stress_545"
+xfsrestore: media label: "stress_tape_media"
+xfsrestore: file system ID: ID
+xfsrestore: session id: ID
+xfsrestore: media ID: ID
+xfsrestore: searching media for directory dump
+xfsrestore: reading directories
+xfsrestore: found fake rootino #128, will fix.
+xfsrestore: fix root # to 1024 (bind mount?)
+xfsrestore: 2 directories and 2 entries processed
+xfsrestore: directory post-processing
+xfsrestore: restoring non-directory files
+xfsrestore: restore complete: SECS seconds elapsed
+xfsrestore: Restore Status: SUCCESS
+Comparing dump directory with restore directory
+Files DUMP_DIR/FILE_1019 and RESTORE_DIR/DUMP_SUBDIR/FILE_1019 are identical
+Comparing listing of dump directory with restore directory
+Files TMP.dump_dir and TMP.restore_dir are identical
-- 
2.37.3

Re: [RFC PATCH] xfs: test for fixing wrong root inode number

[Zorro Lang]

On Wed, Sep 28, 2022 at 05:03:37PM -0400, Hironori Shiina wrote:
> Test '-x' option of xfsrestore. With this option, a wrong root inode
> number is corrected. A root inode number can be wrong in a dump created
> by problematic xfsdump (v3.1.7 - v3.1.9) with blukstat misuse. This
> patch adds a dump with a wrong inode number created by xfsdump 3.1.8.
> 
> Link: https://lore.kernel.org/linux-xfs/20201113125127.966243-1-hsiangkao@redhat.com/
> Signed-off-by: Hironori Shiina <shiina.hironori@fujitsu.com>
> ---
>  common/dump                    |   2 +-
>  src/root-inode-broken-dumpfile | Bin 0 -> 21648 bytes
>  tests/xfs/554                  |  37 +++++++++++++++++++++
>  tests/xfs/554.out              |  57 +++++++++++++++++++++++++++++++++
>  4 files changed, 95 insertions(+), 1 deletion(-)
>  create mode 100644 src/root-inode-broken-dumpfile
>  create mode 100644 tests/xfs/554
>  create mode 100644 tests/xfs/554.out
> 
> diff --git a/common/dump b/common/dump
> index 8e0446d9..50b2ba03 100644
> --- a/common/dump
> +++ b/common/dump
> @@ -1003,7 +1003,7 @@ _parse_restore_args()
>          --no-check-quota)
>              do_quota_check=false
>              ;;
> -	-K|-R)
> +	-K|-R|-x)
>  	    restore_args="$restore_args $1"
>              ;;
>  	*)
> diff --git a/src/root-inode-broken-dumpfile b/src/root-inode-broken-dumpfile
> new file mode 100644
> index 0000000000000000000000000000000000000000..9a42e65d8047497be31f3abbaf4223ae384fd14d
> GIT binary patch
> literal 21648
> zcmeI)K}ge49KiAC_J<CEBr!0AwBf~zbCHB}5DyxL6eU8Jnqylvayj;2VuG+d)TN6;
> z5J8vlAaw9hXi(UousT$Sin@BRJfwHM)O+*2*njz_zc+h*ckuV#@BMuKe;;JbgTL{<
> z!SwZ9zC#ERe%reLe5&cz2e}qM<!i2dXQHt^{^`d1Qx{)MMzW#$%dR@J={0`IRsGx4
> z(yn@Oi-nBqCOVIG?&{kpwnv~&w_>6_ozY1^fph=w8(=^oTg&wOe=(WQByyQ_Hfd|4
> z^o76<0!zn#v>k3blbU)nzx=KF^}-G%R;OaQYsHwGDkO`kD^@q^(_Ac_8H<gKj^>a0
> z6p*%BK>q#b>2EE%^!@f?Z`-p+tI@M}qmMm@zMJk>K1U^=d~G^NUG?X4vkvKtOf-3Y
> zpVM9YgM9Y7{*P0ApJNUh^uk1wCnA6V0tg_000IagfB*srAb<b@2q1s}0tg_000Iag
> zfB*srAb<b@2q1s}0tg_000IagfB*srAb<b@2q1s}0tg_000IagfB*srAb`MM1p>_h
> z2-R=Q9ooK1{l9<Dy8IIMUVXr9BW9uI1x7};j+kij|5kLI^32no;n|PvqD74ZOlJ#n
> z0-~CKSlx#liMS>jt232#==00xPqwp;gsZrjc?`PPxaCE~>3(^o5-%+Nj=HegJFK2b
> z=l5zT4IDgO=sJ1zp=jyrALvcQJK|j;sN2_jUmobjN<!S6m1{G<LZ^+JP;T!|3{9)w
> eGf&ioo}iw|li2&4IyG;z<}vrl)Mic2`t2{52##q0
> 
> literal 0
> HcmV?d00001

Please don't try to add a binary file to fstests directly.

> 
> diff --git a/tests/xfs/554 b/tests/xfs/554
> new file mode 100644
> index 00000000..13bc62c7
> --- /dev/null
> +++ b/tests/xfs/554
> @@ -0,0 +1,37 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2022 Fujitsu Limited. All Rights Reserved.
> +#
> +# FS QA Test No. 554
> +#
> +# Test restoring a dumpfile with a wrong root inode number created by
> +# xfsdump 3.1.8.
> +# This test restores the checked-in broken dump with '-x' flag.
> +#
> +
> +. ./common/preamble
> +_begin_fstest auto quick dump
> +
> +# Import common functions.
> +. ./common/dump
> +
> +# real QA test starts here
> +_supported_fs xfs
> +_require_scratch

The -x option is a new feature for xfsdump, not all system support that. So
we need to _notrun if test on a system doesn't support it. A separated
_require_* helper would be better if there'll be more testing about this
new feature. Or a local detection in this case is fine too (can be moved as
a common helper later).

> +_scratch_mkfs_xfs >>$seqres.full || _fail "mkfs failed"
> +_scratch_mount
> +
> +# Create dumpdir for comparing with restoredir
> +rm -rf $dump_dir
> +mkdir $dump_dir || _fail "failed to mkdir $restore_dir"
          ^^                                  ^^

Are you trying to create a dump dir or restore dir?

> +touch $dump_dir/FILE_1019
> +
> +_do_restore_toc -x -f $here/src/root-inode-broken-dumpfile

Why I didn't see how you generate this broken dumpfile in this case?

Oh... I see, you want to store a dumpfile in fstests source code directly.
I thought you submited that file accidentally...

No, we don't do things like this way, please try to generate the dumpfile
in this test case at first, then restore it. For example using xfs_db to
break a xfs, or using some tricky method (likes xfs/545).

> +
> +_do_restore_file -x -f $here/src/root-inode-broken-dumpfile -L stress_545
> +_diff_compare_sub
> +_ls_nodate_compare_sub
> +
> +# success, all done
> +status=0
> +exit
> diff --git a/tests/xfs/554.out b/tests/xfs/554.out
> new file mode 100644
> index 00000000..40a3f3a4
> --- /dev/null
> +++ b/tests/xfs/554.out
> @@ -0,0 +1,57 @@
> +QA output created by 554
> +Contents of dump ...
> +xfsrestore  -x -f DUMP_FILE -t
> +xfsrestore: using file dump (drive_simple) strategy
> +xfsrestore: searching media for dump
> +xfsrestore: examining media file 0
> +xfsrestore: dump description: 
> +xfsrestore: hostname: xfsdump
> +xfsrestore: mount point: SCRATCH_MNT
> +xfsrestore: volume: SCRATCH_DEV
> +xfsrestore: session time: TIME
> +xfsrestore: level: 0
> +xfsrestore: session label: "stress_545"
> +xfsrestore: media label: "stress_tape_media"
> +xfsrestore: file system ID: ID
> +xfsrestore: session id: ID
> +xfsrestore: media ID: ID
> +xfsrestore: searching media for directory dump
> +xfsrestore: reading directories
> +xfsrestore: found fake rootino #128, will fix.
> +xfsrestore: fix root # to 1024 (bind mount?)
> +xfsrestore: 2 directories and 2 entries processed
> +xfsrestore: directory post-processing
> +xfsrestore: reading non-directory files
> +xfsrestore: table of contents display complete: SECS seconds elapsed
> +xfsrestore: Restore Status: SUCCESS
> +
> +dumpdir/FILE_1019
> +Restoring from file...
> +xfsrestore  -x -f DUMP_FILE  -L stress_545 RESTORE_DIR
> +xfsrestore: using file dump (drive_simple) strategy
> +xfsrestore: searching media for dump
> +xfsrestore: examining media file 0
> +xfsrestore: found dump matching specified label:
> +xfsrestore: hostname: xfsdump
> +xfsrestore: mount point: SCRATCH_MNT
> +xfsrestore: volume: SCRATCH_DEV
> +xfsrestore: session time: TIME
> +xfsrestore: level: 0
> +xfsrestore: session label: "stress_545"
> +xfsrestore: media label: "stress_tape_media"
> +xfsrestore: file system ID: ID
> +xfsrestore: session id: ID
> +xfsrestore: media ID: ID
> +xfsrestore: searching media for directory dump
> +xfsrestore: reading directories
> +xfsrestore: found fake rootino #128, will fix.
> +xfsrestore: fix root # to 1024 (bind mount?)
> +xfsrestore: 2 directories and 2 entries processed
> +xfsrestore: directory post-processing
> +xfsrestore: restoring non-directory files
> +xfsrestore: restore complete: SECS seconds elapsed
> +xfsrestore: Restore Status: SUCCESS
> +Comparing dump directory with restore directory
> +Files DUMP_DIR/FILE_1019 and RESTORE_DIR/DUMP_SUBDIR/FILE_1019 are identical
> +Comparing listing of dump directory with restore directory
> +Files TMP.dump_dir and TMP.restore_dir are identical
> -- 
> 2.37.3
> 

Re: [RFC PATCH] xfs: test for fixing wrong root inode number

[Hironori Shiina]

On 9/28/22 21:49, Zorro Lang wrote:
> On Wed, Sep 28, 2022 at 05:03:37PM -0400, Hironori Shiina wrote:
>> Test '-x' option of xfsrestore. With this option, a wrong root inode
>> number is corrected. A root inode number can be wrong in a dump created
>> by problematic xfsdump (v3.1.7 - v3.1.9) with blukstat misuse. This
>> patch adds a dump with a wrong inode number created by xfsdump 3.1.8.
>>
>> Link: https://lore.kernel.org/linux-xfs/20201113125127.966243-1-hsiangkao@redhat.com/
>> Signed-off-by: Hironori Shiina <shiina.hironori@fujitsu.com>
>> ---
>>  common/dump                    |   2 +-
>>  src/root-inode-broken-dumpfile | Bin 0 -> 21648 bytes
>>  tests/xfs/554                  |  37 +++++++++++++++++++++
>>  tests/xfs/554.out              |  57 +++++++++++++++++++++++++++++++++
>>  4 files changed, 95 insertions(+), 1 deletion(-)
>>  create mode 100644 src/root-inode-broken-dumpfile
>>  create mode 100644 tests/xfs/554
>>  create mode 100644 tests/xfs/554.out
>>
>> diff --git a/common/dump b/common/dump
>> index 8e0446d9..50b2ba03 100644
>> --- a/common/dump
>> +++ b/common/dump
>> @@ -1003,7 +1003,7 @@ _parse_restore_args()
>>          --no-check-quota)
>>              do_quota_check=false
>>              ;;
>> -	-K|-R)
>> +	-K|-R|-x)
>>  	    restore_args="$restore_args $1"
>>              ;;
>>  	*)
>> diff --git a/src/root-inode-broken-dumpfile b/src/root-inode-broken-dumpfile
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..9a42e65d8047497be31f3abbaf4223ae384fd14d
>> GIT binary patch
>> literal 21648
>> zcmeI)K}ge49KiAC_J<CEBr!0AwBf~zbCHB}5DyxL6eU8Jnqylvayj;2VuG+d)TN6;
>> z5J8vlAaw9hXi(UousT$Sin@BRJfwHM)O+*2*njz_zc+h*ckuV#@BMuKe;;JbgTL{<
>> z!SwZ9zC#ERe%reLe5&cz2e}qM<!i2dXQHt^{^`d1Qx{)MMzW#$%dR@J={0`IRsGx4
>> z(yn@Oi-nBqCOVIG?&{kpwnv~&w_>6_ozY1^fph=w8(=^oTg&wOe=(WQByyQ_Hfd|4
>> z^o76<0!zn#v>k3blbU)nzx=KF^}-G%R;OaQYsHwGDkO`kD^@q^(_Ac_8H<gKj^>a0
>> z6p*%BK>q#b>2EE%^!@f?Z`-p+tI@M}qmMm@zMJk>K1U^=d~G^NUG?X4vkvKtOf-3Y
>> zpVM9YgM9Y7{*P0ApJNUh^uk1wCnA6V0tg_000IagfB*srAb<b@2q1s}0tg_000Iag
>> zfB*srAb<b@2q1s}0tg_000IagfB*srAb<b@2q1s}0tg_000IagfB*srAb`MM1p>_h
>> z2-R=Q9ooK1{l9<Dy8IIMUVXr9BW9uI1x7};j+kij|5kLI^32no;n|PvqD74ZOlJ#n
>> z0-~CKSlx#liMS>jt232#==00xPqwp;gsZrjc?`PPxaCE~>3(^o5-%+Nj=HegJFK2b
>> z=l5zT4IDgO=sJ1zp=jyrALvcQJK|j;sN2_jUmobjN<!S6m1{G<LZ^+JP;T!|3{9)w
>> eGf&ioo}iw|li2&4IyG;z<}vrl)Mic2`t2{52##q0
>>
>> literal 0
>> HcmV?d00001
> 
> Please don't try to add a binary file to fstests directly.
> 
>>
>> diff --git a/tests/xfs/554 b/tests/xfs/554
>> new file mode 100644
>> index 00000000..13bc62c7
>> --- /dev/null
>> +++ b/tests/xfs/554
>> @@ -0,0 +1,37 @@
>> +#! /bin/bash
>> +# SPDX-License-Identifier: GPL-2.0
>> +# Copyright (c) 2022 Fujitsu Limited. All Rights Reserved.
>> +#
>> +# FS QA Test No. 554
>> +#
>> +# Test restoring a dumpfile with a wrong root inode number created by
>> +# xfsdump 3.1.8.
>> +# This test restores the checked-in broken dump with '-x' flag.
>> +#
>> +
>> +. ./common/preamble
>> +_begin_fstest auto quick dump
>> +
>> +# Import common functions.
>> +. ./common/dump
>> +
>> +# real QA test starts here
>> +_supported_fs xfs
>> +_require_scratch
> 
> The -x option is a new feature for xfsdump, not all system support that. So
> we need to _notrun if test on a system doesn't support it. A separated
> _require_* helper would be better if there'll be more testing about this
> new feature. Or a local detection in this case is fine too (can be moved as
> a common helper later).
> 
>> +_scratch_mkfs_xfs >>$seqres.full || _fail "mkfs failed"
>> +_scratch_mount
>> +
>> +# Create dumpdir for comparing with restoredir
>> +rm -rf $dump_dir
>> +mkdir $dump_dir || _fail "failed to mkdir $restore_dir"
>           ^^                                  ^^
> 
> Are you trying to create a dump dir or restore dir?
> 
>> +touch $dump_dir/FILE_1019
>> +
>> +_do_restore_toc -x -f $here/src/root-inode-broken-dumpfile
> 
> Why I didn't see how you generate this broken dumpfile in this case?
> 
> Oh... I see, you want to store a dumpfile in fstests source code directly.
> I thought you submited that file accidentally...
> 
> No, we don't do things like this way, please try to generate the dumpfile
> in this test case at first, then restore it. For example using xfs_db to
> break a xfs, or using some tricky method (likes xfs/545).
> 

Thank you for the comments. I will try another approach. I'm having
trouble creating a dumpfile for this test. Because xfsdump was already
fixed, xfsdump no longer generates a corrupted dumpfile even if there is
a lower inode number than a root inode.

>> +
>> +_do_restore_file -x -f $here/src/root-inode-broken-dumpfile -L stress_545
>> +_diff_compare_sub
>> +_ls_nodate_compare_sub
>> +
>> +# success, all done
>> +status=0
>> +exit
>> diff --git a/tests/xfs/554.out b/tests/xfs/554.out
>> new file mode 100644
>> index 00000000..40a3f3a4
>> --- /dev/null
>> +++ b/tests/xfs/554.out
>> @@ -0,0 +1,57 @@
>> +QA output created by 554
>> +Contents of dump ...
>> +xfsrestore  -x -f DUMP_FILE -t
>> +xfsrestore: using file dump (drive_simple) strategy
>> +xfsrestore: searching media for dump
>> +xfsrestore: examining media file 0
>> +xfsrestore: dump description: 
>> +xfsrestore: hostname: xfsdump
>> +xfsrestore: mount point: SCRATCH_MNT
>> +xfsrestore: volume: SCRATCH_DEV
>> +xfsrestore: session time: TIME
>> +xfsrestore: level: 0
>> +xfsrestore: session label: "stress_545"
>> +xfsrestore: media label: "stress_tape_media"
>> +xfsrestore: file system ID: ID
>> +xfsrestore: session id: ID
>> +xfsrestore: media ID: ID
>> +xfsrestore: searching media for directory dump
>> +xfsrestore: reading directories
>> +xfsrestore: found fake rootino #128, will fix.
>> +xfsrestore: fix root # to 1024 (bind mount?)
>> +xfsrestore: 2 directories and 2 entries processed
>> +xfsrestore: directory post-processing
>> +xfsrestore: reading non-directory files
>> +xfsrestore: table of contents display complete: SECS seconds elapsed
>> +xfsrestore: Restore Status: SUCCESS
>> +
>> +dumpdir/FILE_1019
>> +Restoring from file...
>> +xfsrestore  -x -f DUMP_FILE  -L stress_545 RESTORE_DIR
>> +xfsrestore: using file dump (drive_simple) strategy
>> +xfsrestore: searching media for dump
>> +xfsrestore: examining media file 0
>> +xfsrestore: found dump matching specified label:
>> +xfsrestore: hostname: xfsdump
>> +xfsrestore: mount point: SCRATCH_MNT
>> +xfsrestore: volume: SCRATCH_DEV
>> +xfsrestore: session time: TIME
>> +xfsrestore: level: 0
>> +xfsrestore: session label: "stress_545"
>> +xfsrestore: media label: "stress_tape_media"
>> +xfsrestore: file system ID: ID
>> +xfsrestore: session id: ID
>> +xfsrestore: media ID: ID
>> +xfsrestore: searching media for directory dump
>> +xfsrestore: reading directories
>> +xfsrestore: found fake rootino #128, will fix.
>> +xfsrestore: fix root # to 1024 (bind mount?)
>> +xfsrestore: 2 directories and 2 entries processed
>> +xfsrestore: directory post-processing
>> +xfsrestore: restoring non-directory files
>> +xfsrestore: restore complete: SECS seconds elapsed
>> +xfsrestore: Restore Status: SUCCESS
>> +Comparing dump directory with restore directory
>> +Files DUMP_DIR/FILE_1019 and RESTORE_DIR/DUMP_SUBDIR/FILE_1019 are identical
>> +Comparing listing of dump directory with restore directory
>> +Files TMP.dump_dir and TMP.restore_dir are identical
>> -- 
>> 2.37.3
>>
> 

Re: [RFC PATCH] xfs: test for fixing wrong root inode number

[Zorro Lang]

On Fri, Sep 30, 2022 at 11:01:51AM -0400, Hironori Shiina wrote:
> 
> 
> On 9/28/22 21:49, Zorro Lang wrote:
> > On Wed, Sep 28, 2022 at 05:03:37PM -0400, Hironori Shiina wrote:
> >> Test '-x' option of xfsrestore. With this option, a wrong root inode
> >> number is corrected. A root inode number can be wrong in a dump created
> >> by problematic xfsdump (v3.1.7 - v3.1.9) with blukstat misuse. This
> >> patch adds a dump with a wrong inode number created by xfsdump 3.1.8.
> >>
> >> Link: https://lore.kernel.org/linux-xfs/20201113125127.966243-1-hsiangkao@redhat.com/
> >> Signed-off-by: Hironori Shiina <shiina.hironori@fujitsu.com>
> >> ---
> >>  common/dump                    |   2 +-
> >>  src/root-inode-broken-dumpfile | Bin 0 -> 21648 bytes
> >>  tests/xfs/554                  |  37 +++++++++++++++++++++
> >>  tests/xfs/554.out              |  57 +++++++++++++++++++++++++++++++++
> >>  4 files changed, 95 insertions(+), 1 deletion(-)
> >>  create mode 100644 src/root-inode-broken-dumpfile
> >>  create mode 100644 tests/xfs/554
> >>  create mode 100644 tests/xfs/554.out
> >>
> >> diff --git a/common/dump b/common/dump
> >> index 8e0446d9..50b2ba03 100644
> >> --- a/common/dump
> >> +++ b/common/dump
> >> @@ -1003,7 +1003,7 @@ _parse_restore_args()
> >>          --no-check-quota)
> >>              do_quota_check=false
> >>              ;;
> >> -	-K|-R)
> >> +	-K|-R|-x)
> >>  	    restore_args="$restore_args $1"
> >>              ;;
> >>  	*)
> >> diff --git a/src/root-inode-broken-dumpfile b/src/root-inode-broken-dumpfile
> >> new file mode 100644
> >> index 0000000000000000000000000000000000000000..9a42e65d8047497be31f3abbaf4223ae384fd14d
> >> GIT binary patch
> >> literal 21648
> >> zcmeI)K}ge49KiAC_J<CEBr!0AwBf~zbCHB}5DyxL6eU8Jnqylvayj;2VuG+d)TN6;
> >> z5J8vlAaw9hXi(UousT$Sin@BRJfwHM)O+*2*njz_zc+h*ckuV#@BMuKe;;JbgTL{<
> >> z!SwZ9zC#ERe%reLe5&cz2e}qM<!i2dXQHt^{^`d1Qx{)MMzW#$%dR@J={0`IRsGx4
> >> z(yn@Oi-nBqCOVIG?&{kpwnv~&w_>6_ozY1^fph=w8(=^oTg&wOe=(WQByyQ_Hfd|4
> >> z^o76<0!zn#v>k3blbU)nzx=KF^}-G%R;OaQYsHwGDkO`kD^@q^(_Ac_8H<gKj^>a0
> >> z6p*%BK>q#b>2EE%^!@f?Z`-p+tI@M}qmMm@zMJk>K1U^=d~G^NUG?X4vkvKtOf-3Y
> >> zpVM9YgM9Y7{*P0ApJNUh^uk1wCnA6V0tg_000IagfB*srAb<b@2q1s}0tg_000Iag
> >> zfB*srAb<b@2q1s}0tg_000IagfB*srAb<b@2q1s}0tg_000IagfB*srAb`MM1p>_h
> >> z2-R=Q9ooK1{l9<Dy8IIMUVXr9BW9uI1x7};j+kij|5kLI^32no;n|PvqD74ZOlJ#n
> >> z0-~CKSlx#liMS>jt232#==00xPqwp;gsZrjc?`PPxaCE~>3(^o5-%+Nj=HegJFK2b
> >> z=l5zT4IDgO=sJ1zp=jyrALvcQJK|j;sN2_jUmobjN<!S6m1{G<LZ^+JP;T!|3{9)w
> >> eGf&ioo}iw|li2&4IyG;z<}vrl)Mic2`t2{52##q0
> >>
> >> literal 0
> >> HcmV?d00001
> > 
> > Please don't try to add a binary file to fstests directly.
> > 
> >>
> >> diff --git a/tests/xfs/554 b/tests/xfs/554
> >> new file mode 100644
> >> index 00000000..13bc62c7
> >> --- /dev/null
> >> +++ b/tests/xfs/554
> >> @@ -0,0 +1,37 @@
> >> +#! /bin/bash
> >> +# SPDX-License-Identifier: GPL-2.0
> >> +# Copyright (c) 2022 Fujitsu Limited. All Rights Reserved.
> >> +#
> >> +# FS QA Test No. 554
> >> +#
> >> +# Test restoring a dumpfile with a wrong root inode number created by
> >> +# xfsdump 3.1.8.
> >> +# This test restores the checked-in broken dump with '-x' flag.
> >> +#
> >> +
> >> +. ./common/preamble
> >> +_begin_fstest auto quick dump
> >> +
> >> +# Import common functions.
> >> +. ./common/dump
> >> +
> >> +# real QA test starts here
> >> +_supported_fs xfs
> >> +_require_scratch
> > 
> > The -x option is a new feature for xfsdump, not all system support that. So
> > we need to _notrun if test on a system doesn't support it. A separated
> > _require_* helper would be better if there'll be more testing about this
> > new feature. Or a local detection in this case is fine too (can be moved as
> > a common helper later).
> > 
> >> +_scratch_mkfs_xfs >>$seqres.full || _fail "mkfs failed"
> >> +_scratch_mount
> >> +
> >> +# Create dumpdir for comparing with restoredir
> >> +rm -rf $dump_dir
> >> +mkdir $dump_dir || _fail "failed to mkdir $restore_dir"
> >           ^^                                  ^^
> > 
> > Are you trying to create a dump dir or restore dir?
> > 
> >> +touch $dump_dir/FILE_1019
> >> +
> >> +_do_restore_toc -x -f $here/src/root-inode-broken-dumpfile
> > 
> > Why I didn't see how you generate this broken dumpfile in this case?
> > 
> > Oh... I see, you want to store a dumpfile in fstests source code directly.
> > I thought you submited that file accidentally...
> > 
> > No, we don't do things like this way, please try to generate the dumpfile
> > in this test case at first, then restore it. For example using xfs_db to
> > break a xfs, or using some tricky method (likes xfs/545).
> > 
> 
> Thank you for the comments. I will try another approach. I'm having
> trouble creating a dumpfile for this test. Because xfsdump was already
> fixed, xfsdump no longer generates a corrupted dumpfile even if there is
> a lower inode number than a root inode.

Oh, I see. You can try, but I think it's hard. It maybe not suitable to be a
fstests case, if it has to depend on a binary fs dump file. If so, We can cover
it on other place, with this existed "bad" dump file.

Thanks,
Zorro

> 
> >> +
> >> +_do_restore_file -x -f $here/src/root-inode-broken-dumpfile -L stress_545
> >> +_diff_compare_sub
> >> +_ls_nodate_compare_sub
> >> +
> >> +# success, all done
> >> +status=0
> >> +exit
> >> diff --git a/tests/xfs/554.out b/tests/xfs/554.out
> >> new file mode 100644
> >> index 00000000..40a3f3a4
> >> --- /dev/null
> >> +++ b/tests/xfs/554.out
> >> @@ -0,0 +1,57 @@
> >> +QA output created by 554
> >> +Contents of dump ...
> >> +xfsrestore  -x -f DUMP_FILE -t
> >> +xfsrestore: using file dump (drive_simple) strategy
> >> +xfsrestore: searching media for dump
> >> +xfsrestore: examining media file 0
> >> +xfsrestore: dump description: 
> >> +xfsrestore: hostname: xfsdump
> >> +xfsrestore: mount point: SCRATCH_MNT
> >> +xfsrestore: volume: SCRATCH_DEV
> >> +xfsrestore: session time: TIME
> >> +xfsrestore: level: 0
> >> +xfsrestore: session label: "stress_545"
> >> +xfsrestore: media label: "stress_tape_media"
> >> +xfsrestore: file system ID: ID
> >> +xfsrestore: session id: ID
> >> +xfsrestore: media ID: ID
> >> +xfsrestore: searching media for directory dump
> >> +xfsrestore: reading directories
> >> +xfsrestore: found fake rootino #128, will fix.
> >> +xfsrestore: fix root # to 1024 (bind mount?)
> >> +xfsrestore: 2 directories and 2 entries processed
> >> +xfsrestore: directory post-processing
> >> +xfsrestore: reading non-directory files
> >> +xfsrestore: table of contents display complete: SECS seconds elapsed
> >> +xfsrestore: Restore Status: SUCCESS
> >> +
> >> +dumpdir/FILE_1019
> >> +Restoring from file...
> >> +xfsrestore  -x -f DUMP_FILE  -L stress_545 RESTORE_DIR
> >> +xfsrestore: using file dump (drive_simple) strategy
> >> +xfsrestore: searching media for dump
> >> +xfsrestore: examining media file 0
> >> +xfsrestore: found dump matching specified label:
> >> +xfsrestore: hostname: xfsdump
> >> +xfsrestore: mount point: SCRATCH_MNT
> >> +xfsrestore: volume: SCRATCH_DEV
> >> +xfsrestore: session time: TIME
> >> +xfsrestore: level: 0
> >> +xfsrestore: session label: "stress_545"
> >> +xfsrestore: media label: "stress_tape_media"
> >> +xfsrestore: file system ID: ID
> >> +xfsrestore: session id: ID
> >> +xfsrestore: media ID: ID
> >> +xfsrestore: searching media for directory dump
> >> +xfsrestore: reading directories
> >> +xfsrestore: found fake rootino #128, will fix.
> >> +xfsrestore: fix root # to 1024 (bind mount?)
> >> +xfsrestore: 2 directories and 2 entries processed
> >> +xfsrestore: directory post-processing
> >> +xfsrestore: restoring non-directory files
> >> +xfsrestore: restore complete: SECS seconds elapsed
> >> +xfsrestore: Restore Status: SUCCESS
> >> +Comparing dump directory with restore directory
> >> +Files DUMP_DIR/FILE_1019 and RESTORE_DIR/DUMP_SUBDIR/FILE_1019 are identical
> >> +Comparing listing of dump directory with restore directory
> >> +Files TMP.dump_dir and TMP.restore_dir are identical
> >> -- 
> >> 2.37.3
> >>
> > 
> 

Re: [RFC PATCH] xfs: test for fixing wrong root inode number

[Darrick J. Wong]

On Sun, Oct 02, 2022 at 12:27:08PM +0800, Zorro Lang wrote:
> On Fri, Sep 30, 2022 at 11:01:51AM -0400, Hironori Shiina wrote:
> > 
> > 
> > On 9/28/22 21:49, Zorro Lang wrote:
> > > On Wed, Sep 28, 2022 at 05:03:37PM -0400, Hironori Shiina wrote:
> > >> Test '-x' option of xfsrestore. With this option, a wrong root inode
> > >> number is corrected. A root inode number can be wrong in a dump created
> > >> by problematic xfsdump (v3.1.7 - v3.1.9) with blukstat misuse. This
> > >> patch adds a dump with a wrong inode number created by xfsdump 3.1.8.
> > >>
> > >> Link: https://lore.kernel.org/linux-xfs/20201113125127.966243-1-hsiangkao@redhat.com/
> > >> Signed-off-by: Hironori Shiina <shiina.hironori@fujitsu.com>
> > >> ---
> > >>  common/dump                    |   2 +-
> > >>  src/root-inode-broken-dumpfile | Bin 0 -> 21648 bytes
> > >>  tests/xfs/554                  |  37 +++++++++++++++++++++
> > >>  tests/xfs/554.out              |  57 +++++++++++++++++++++++++++++++++
> > >>  4 files changed, 95 insertions(+), 1 deletion(-)
> > >>  create mode 100644 src/root-inode-broken-dumpfile
> > >>  create mode 100644 tests/xfs/554
> > >>  create mode 100644 tests/xfs/554.out
> > >>
> > >> diff --git a/common/dump b/common/dump
> > >> index 8e0446d9..50b2ba03 100644
> > >> --- a/common/dump
> > >> +++ b/common/dump
> > >> @@ -1003,7 +1003,7 @@ _parse_restore_args()
> > >>          --no-check-quota)
> > >>              do_quota_check=false
> > >>              ;;
> > >> -	-K|-R)
> > >> +	-K|-R|-x)
> > >>  	    restore_args="$restore_args $1"
> > >>              ;;
> > >>  	*)
> > >> diff --git a/src/root-inode-broken-dumpfile b/src/root-inode-broken-dumpfile
> > >> new file mode 100644
> > >> index 0000000000000000000000000000000000000000..9a42e65d8047497be31f3abbaf4223ae384fd14d
> > >> GIT binary patch
> > >> literal 21648
> > >> zcmeI)K}ge49KiAC_J<CEBr!0AwBf~zbCHB}5DyxL6eU8Jnqylvayj;2VuG+d)TN6;
> > >> z5J8vlAaw9hXi(UousT$Sin@BRJfwHM)O+*2*njz_zc+h*ckuV#@BMuKe;;JbgTL{<
> > >> z!SwZ9zC#ERe%reLe5&cz2e}qM<!i2dXQHt^{^`d1Qx{)MMzW#$%dR@J={0`IRsGx4
> > >> z(yn@Oi-nBqCOVIG?&{kpwnv~&w_>6_ozY1^fph=w8(=^oTg&wOe=(WQByyQ_Hfd|4
> > >> z^o76<0!zn#v>k3blbU)nzx=KF^}-G%R;OaQYsHwGDkO`kD^@q^(_Ac_8H<gKj^>a0
> > >> z6p*%BK>q#b>2EE%^!@f?Z`-p+tI@M}qmMm@zMJk>K1U^=d~G^NUG?X4vkvKtOf-3Y
> > >> zpVM9YgM9Y7{*P0ApJNUh^uk1wCnA6V0tg_000IagfB*srAb<b@2q1s}0tg_000Iag
> > >> zfB*srAb<b@2q1s}0tg_000IagfB*srAb<b@2q1s}0tg_000IagfB*srAb`MM1p>_h
> > >> z2-R=Q9ooK1{l9<Dy8IIMUVXr9BW9uI1x7};j+kij|5kLI^32no;n|PvqD74ZOlJ#n
> > >> z0-~CKSlx#liMS>jt232#==00xPqwp;gsZrjc?`PPxaCE~>3(^o5-%+Nj=HegJFK2b
> > >> z=l5zT4IDgO=sJ1zp=jyrALvcQJK|j;sN2_jUmobjN<!S6m1{G<LZ^+JP;T!|3{9)w
> > >> eGf&ioo}iw|li2&4IyG;z<}vrl)Mic2`t2{52##q0
> > >>
> > >> literal 0
> > >> HcmV?d00001
> > > 
> > > Please don't try to add a binary file to fstests directly.
> > > 
> > >>
> > >> diff --git a/tests/xfs/554 b/tests/xfs/554
> > >> new file mode 100644
> > >> index 00000000..13bc62c7
> > >> --- /dev/null
> > >> +++ b/tests/xfs/554
> > >> @@ -0,0 +1,37 @@
> > >> +#! /bin/bash
> > >> +# SPDX-License-Identifier: GPL-2.0
> > >> +# Copyright (c) 2022 Fujitsu Limited. All Rights Reserved.
> > >> +#
> > >> +# FS QA Test No. 554
> > >> +#
> > >> +# Test restoring a dumpfile with a wrong root inode number created by
> > >> +# xfsdump 3.1.8.
> > >> +# This test restores the checked-in broken dump with '-x' flag.
> > >> +#
> > >> +
> > >> +. ./common/preamble
> > >> +_begin_fstest auto quick dump
> > >> +
> > >> +# Import common functions.
> > >> +. ./common/dump
> > >> +
> > >> +# real QA test starts here
> > >> +_supported_fs xfs
> > >> +_require_scratch
> > > 
> > > The -x option is a new feature for xfsdump, not all system support that. So
> > > we need to _notrun if test on a system doesn't support it. A separated
> > > _require_* helper would be better if there'll be more testing about this
> > > new feature. Or a local detection in this case is fine too (can be moved as
> > > a common helper later).
> > > 
> > >> +_scratch_mkfs_xfs >>$seqres.full || _fail "mkfs failed"
> > >> +_scratch_mount
> > >> +
> > >> +# Create dumpdir for comparing with restoredir
> > >> +rm -rf $dump_dir
> > >> +mkdir $dump_dir || _fail "failed to mkdir $restore_dir"
> > >           ^^                                  ^^
> > > 
> > > Are you trying to create a dump dir or restore dir?
> > > 
> > >> +touch $dump_dir/FILE_1019
> > >> +
> > >> +_do_restore_toc -x -f $here/src/root-inode-broken-dumpfile
> > > 
> > > Why I didn't see how you generate this broken dumpfile in this case?
> > > 
> > > Oh... I see, you want to store a dumpfile in fstests source code directly.
> > > I thought you submited that file accidentally...
> > > 
> > > No, we don't do things like this way, please try to generate the dumpfile
> > > in this test case at first, then restore it. For example using xfs_db to
> > > break a xfs, or using some tricky method (likes xfs/545).
> > > 
> > 
> > Thank you for the comments. I will try another approach. I'm having
> > trouble creating a dumpfile for this test. Because xfsdump was already
> > fixed, xfsdump no longer generates a corrupted dumpfile even if there is
> > a lower inode number than a root inode.
> 
> Oh, I see. You can try, but I think it's hard. It maybe not suitable to be a
> fstests case, if it has to depend on a binary fs dump file. If so, We can cover
> it on other place, with this existed "bad" dump file.

How difficult is it to create a dumpfile with a broken root inode?
Is it a simple matter of creating a good dump and patching a few bytes,
or do we end up having to patch the whole file?

(The reason I ask is that I've heard about this problem for ages but
I don't actually know how to create a bad dump...)

--D

> Thanks,
> Zorro
> 
> > 
> > >> +
> > >> +_do_restore_file -x -f $here/src/root-inode-broken-dumpfile -L stress_545
> > >> +_diff_compare_sub
> > >> +_ls_nodate_compare_sub
> > >> +
> > >> +# success, all done
> > >> +status=0
> > >> +exit
> > >> diff --git a/tests/xfs/554.out b/tests/xfs/554.out
> > >> new file mode 100644
> > >> index 00000000..40a3f3a4
> > >> --- /dev/null
> > >> +++ b/tests/xfs/554.out
> > >> @@ -0,0 +1,57 @@
> > >> +QA output created by 554
> > >> +Contents of dump ...
> > >> +xfsrestore  -x -f DUMP_FILE -t
> > >> +xfsrestore: using file dump (drive_simple) strategy
> > >> +xfsrestore: searching media for dump
> > >> +xfsrestore: examining media file 0
> > >> +xfsrestore: dump description: 
> > >> +xfsrestore: hostname: xfsdump
> > >> +xfsrestore: mount point: SCRATCH_MNT
> > >> +xfsrestore: volume: SCRATCH_DEV
> > >> +xfsrestore: session time: TIME
> > >> +xfsrestore: level: 0
> > >> +xfsrestore: session label: "stress_545"
> > >> +xfsrestore: media label: "stress_tape_media"
> > >> +xfsrestore: file system ID: ID
> > >> +xfsrestore: session id: ID
> > >> +xfsrestore: media ID: ID
> > >> +xfsrestore: searching media for directory dump
> > >> +xfsrestore: reading directories
> > >> +xfsrestore: found fake rootino #128, will fix.
> > >> +xfsrestore: fix root # to 1024 (bind mount?)
> > >> +xfsrestore: 2 directories and 2 entries processed
> > >> +xfsrestore: directory post-processing
> > >> +xfsrestore: reading non-directory files
> > >> +xfsrestore: table of contents display complete: SECS seconds elapsed
> > >> +xfsrestore: Restore Status: SUCCESS
> > >> +
> > >> +dumpdir/FILE_1019
> > >> +Restoring from file...
> > >> +xfsrestore  -x -f DUMP_FILE  -L stress_545 RESTORE_DIR
> > >> +xfsrestore: using file dump (drive_simple) strategy
> > >> +xfsrestore: searching media for dump
> > >> +xfsrestore: examining media file 0
> > >> +xfsrestore: found dump matching specified label:
> > >> +xfsrestore: hostname: xfsdump
> > >> +xfsrestore: mount point: SCRATCH_MNT
> > >> +xfsrestore: volume: SCRATCH_DEV
> > >> +xfsrestore: session time: TIME
> > >> +xfsrestore: level: 0
> > >> +xfsrestore: session label: "stress_545"
> > >> +xfsrestore: media label: "stress_tape_media"
> > >> +xfsrestore: file system ID: ID
> > >> +xfsrestore: session id: ID
> > >> +xfsrestore: media ID: ID
> > >> +xfsrestore: searching media for directory dump
> > >> +xfsrestore: reading directories
> > >> +xfsrestore: found fake rootino #128, will fix.
> > >> +xfsrestore: fix root # to 1024 (bind mount?)
> > >> +xfsrestore: 2 directories and 2 entries processed
> > >> +xfsrestore: directory post-processing
> > >> +xfsrestore: restoring non-directory files
> > >> +xfsrestore: restore complete: SECS seconds elapsed
> > >> +xfsrestore: Restore Status: SUCCESS
> > >> +Comparing dump directory with restore directory
> > >> +Files DUMP_DIR/FILE_1019 and RESTORE_DIR/DUMP_SUBDIR/FILE_1019 are identical
> > >> +Comparing listing of dump directory with restore directory
> > >> +Files TMP.dump_dir and TMP.restore_dir are identical
> > >> -- 
> > >> 2.37.3
> > >>
> > > 
> > 
> 

Re: [RFC PATCH] xfs: test for fixing wrong root inode number

[Hironori Shiina]

On 10/3/22 18:04, Darrick J. Wong wrote:
> On Sun, Oct 02, 2022 at 12:27:08PM +0800, Zorro Lang wrote:
>> On Fri, Sep 30, 2022 at 11:01:51AM -0400, Hironori Shiina wrote:
>>>
>>>
>>> On 9/28/22 21:49, Zorro Lang wrote:
>>>> On Wed, Sep 28, 2022 at 05:03:37PM -0400, Hironori Shiina wrote:
>>>>> Test '-x' option of xfsrestore. With this option, a wrong root inode
>>>>> number is corrected. A root inode number can be wrong in a dump created
>>>>> by problematic xfsdump (v3.1.7 - v3.1.9) with blukstat misuse. This
>>>>> patch adds a dump with a wrong inode number created by xfsdump 3.1.8.
>>>>>
>>>>> Link: https://lore.kernel.org/linux-xfs/20201113125127.966243-1-hsiangkao@redhat.com/
>>>>> Signed-off-by: Hironori Shiina <shiina.hironori@fujitsu.com>
>>>>> ---
>>>>>  common/dump                    |   2 +-
>>>>>  src/root-inode-broken-dumpfile | Bin 0 -> 21648 bytes
>>>>>  tests/xfs/554                  |  37 +++++++++++++++++++++
>>>>>  tests/xfs/554.out              |  57 +++++++++++++++++++++++++++++++++
>>>>>  4 files changed, 95 insertions(+), 1 deletion(-)
>>>>>  create mode 100644 src/root-inode-broken-dumpfile
>>>>>  create mode 100644 tests/xfs/554
>>>>>  create mode 100644 tests/xfs/554.out
>>>>>
>>>>> diff --git a/common/dump b/common/dump
>>>>> index 8e0446d9..50b2ba03 100644
>>>>> --- a/common/dump
>>>>> +++ b/common/dump
>>>>> @@ -1003,7 +1003,7 @@ _parse_restore_args()
>>>>>          --no-check-quota)
>>>>>              do_quota_check=false
>>>>>              ;;
>>>>> -	-K|-R)
>>>>> +	-K|-R|-x)
>>>>>  	    restore_args="$restore_args $1"
>>>>>              ;;
>>>>>  	*)
>>>>> diff --git a/src/root-inode-broken-dumpfile b/src/root-inode-broken-dumpfile
>>>>> new file mode 100644
>>>>> index 0000000000000000000000000000000000000000..9a42e65d8047497be31f3abbaf4223ae384fd14d
>>>>> GIT binary patch
>>>>> literal 21648
>>>>> zcmeI)K}ge49KiAC_J<CEBr!0AwBf~zbCHB}5DyxL6eU8Jnqylvayj;2VuG+d)TN6;
>>>>> z5J8vlAaw9hXi(UousT$Sin@BRJfwHM)O+*2*njz_zc+h*ckuV#@BMuKe;;JbgTL{<
>>>>> z!SwZ9zC#ERe%reLe5&cz2e}qM<!i2dXQHt^{^`d1Qx{)MMzW#$%dR@J={0`IRsGx4
>>>>> z(yn@Oi-nBqCOVIG?&{kpwnv~&w_>6_ozY1^fph=w8(=^oTg&wOe=(WQByyQ_Hfd|4
>>>>> z^o76<0!zn#v>k3blbU)nzx=KF^}-G%R;OaQYsHwGDkO`kD^@q^(_Ac_8H<gKj^>a0
>>>>> z6p*%BK>q#b>2EE%^!@f?Z`-p+tI@M}qmMm@zMJk>K1U^=d~G^NUG?X4vkvKtOf-3Y
>>>>> zpVM9YgM9Y7{*P0ApJNUh^uk1wCnA6V0tg_000IagfB*srAb<b@2q1s}0tg_000Iag
>>>>> zfB*srAb<b@2q1s}0tg_000IagfB*srAb<b@2q1s}0tg_000IagfB*srAb`MM1p>_h
>>>>> z2-R=Q9ooK1{l9<Dy8IIMUVXr9BW9uI1x7};j+kij|5kLI^32no;n|PvqD74ZOlJ#n
>>>>> z0-~CKSlx#liMS>jt232#==00xPqwp;gsZrjc?`PPxaCE~>3(^o5-%+Nj=HegJFK2b
>>>>> z=l5zT4IDgO=sJ1zp=jyrALvcQJK|j;sN2_jUmobjN<!S6m1{G<LZ^+JP;T!|3{9)w
>>>>> eGf&ioo}iw|li2&4IyG;z<}vrl)Mic2`t2{52##q0
>>>>>
>>>>> literal 0
>>>>> HcmV?d00001
>>>>
>>>> Please don't try to add a binary file to fstests directly.
>>>>
>>>>>
>>>>> diff --git a/tests/xfs/554 b/tests/xfs/554
>>>>> new file mode 100644
>>>>> index 00000000..13bc62c7
>>>>> --- /dev/null
>>>>> +++ b/tests/xfs/554
>>>>> @@ -0,0 +1,37 @@
>>>>> +#! /bin/bash
>>>>> +# SPDX-License-Identifier: GPL-2.0
>>>>> +# Copyright (c) 2022 Fujitsu Limited. All Rights Reserved.
>>>>> +#
>>>>> +# FS QA Test No. 554
>>>>> +#
>>>>> +# Test restoring a dumpfile with a wrong root inode number created by
>>>>> +# xfsdump 3.1.8.
>>>>> +# This test restores the checked-in broken dump with '-x' flag.
>>>>> +#
>>>>> +
>>>>> +. ./common/preamble
>>>>> +_begin_fstest auto quick dump
>>>>> +
>>>>> +# Import common functions.
>>>>> +. ./common/dump
>>>>> +
>>>>> +# real QA test starts here
>>>>> +_supported_fs xfs
>>>>> +_require_scratch
>>>>
>>>> The -x option is a new feature for xfsdump, not all system support that. So
>>>> we need to _notrun if test on a system doesn't support it. A separated
>>>> _require_* helper would be better if there'll be more testing about this
>>>> new feature. Or a local detection in this case is fine too (can be moved as
>>>> a common helper later).
>>>>
>>>>> +_scratch_mkfs_xfs >>$seqres.full || _fail "mkfs failed"
>>>>> +_scratch_mount
>>>>> +
>>>>> +# Create dumpdir for comparing with restoredir
>>>>> +rm -rf $dump_dir
>>>>> +mkdir $dump_dir || _fail "failed to mkdir $restore_dir"
>>>>           ^^                                  ^^
>>>>
>>>> Are you trying to create a dump dir or restore dir?
>>>>
>>>>> +touch $dump_dir/FILE_1019
>>>>> +
>>>>> +_do_restore_toc -x -f $here/src/root-inode-broken-dumpfile
>>>>
>>>> Why I didn't see how you generate this broken dumpfile in this case?
>>>>
>>>> Oh... I see, you want to store a dumpfile in fstests source code directly.
>>>> I thought you submited that file accidentally...
>>>>
>>>> No, we don't do things like this way, please try to generate the dumpfile
>>>> in this test case at first, then restore it. For example using xfs_db to
>>>> break a xfs, or using some tricky method (likes xfs/545).
>>>>
>>>
>>> Thank you for the comments. I will try another approach. I'm having
>>> trouble creating a dumpfile for this test. Because xfsdump was already
>>> fixed, xfsdump no longer generates a corrupted dumpfile even if there is
>>> a lower inode number than a root inode.
>>
>> Oh, I see. You can try, but I think it's hard. It maybe not suitable to be a
>> fstests case, if it has to depend on a binary fs dump file. If so, We can cover
>> it on other place, with this existed "bad" dump file.
> 
> How difficult is it to create a dumpfile with a broken root inode?
> Is it a simple matter of creating a good dump and patching a few bytes,
> or do we end up having to patch the whole file?
> 
> (The reason I ask is that I've heard about this problem for ages but
> I don't actually know how to create a bad dump...)
> 

As I dug into the data structure, I succeeded in creating a dumpfile with a wrong
root inode by modifying `content_inode_hdr_t.cih_rootino` and
`global_hdr_t.gh_checksum`.

After creating a inode with a lower number than the root inode with the method in
generic/545, I modified a dumpfile as follows:
---
_do_dump_file

# Break dumpfile
old_checksum=$(od -A n -j 12 -N 4 -i --endian=big $dump_file)
gap=$(($root_inum - $inum))
new_checksum=$(($old_checksum + ($gap >> 32) + ($gap & 0x00000000ffffffff)))
v=($(printf "%016x" $inum |  awk -v FS='' '{for (i=1; i<=NF; i+=2) print $i$(i+1)}'))
echo -en "\x${v[0]}\x${v[1]}\x${v[2]}\x${v[3]}\x${v[4]}\x${v[5]}\x${v[6]}\x${v[7]}" | dd of=$dump_file bs=1 seek=3928 conv=notrunc
v=($(printf "%016x" $new_checksum |  awk -v FS='' '{for (i=9; i<=NF; i+=2) print $i$(i+1)}'))
echo -en "\x${v[0]}\x${v[1]}\x${v[2]}\x${v[3]}" | dd of=$dump_file bs=1 seek=12 conv=notrunc

_do_restore_file -x
---
(I'm afraid I'm not so familiar with editing a binary file.)

The offset of `global_hdr_t.gh_checksum` is 12.
The offset of `content_inode_hdr_t.cih_rootino` is:
  global_hdr_t.gh_upper (0x400) + drive_hdr_t.dh_upper (0x400) + 
  media_hdr_t.mh_upper (0x400) + content_hdr_t.ch_specific (0x340) +
  content_inode_hdr_t.cih_rootino (0x18) = 0xf58 (3928)

Thanks,
Hiro

> --D
> 
>> Thanks,
>> Zorro
>>
>>>
>>>>> +
>>>>> +_do_restore_file -x -f $here/src/root-inode-broken-dumpfile -L stress_545
>>>>> +_diff_compare_sub
>>>>> +_ls_nodate_compare_sub
>>>>> +
>>>>> +# success, all done
>>>>> +status=0
>>>>> +exit
>>>>> diff --git a/tests/xfs/554.out b/tests/xfs/554.out
>>>>> new file mode 100644
>>>>> index 00000000..40a3f3a4
>>>>> --- /dev/null
>>>>> +++ b/tests/xfs/554.out
>>>>> @@ -0,0 +1,57 @@
>>>>> +QA output created by 554
>>>>> +Contents of dump ...
>>>>> +xfsrestore  -x -f DUMP_FILE -t
>>>>> +xfsrestore: using file dump (drive_simple) strategy
>>>>> +xfsrestore: searching media for dump
>>>>> +xfsrestore: examining media file 0
>>>>> +xfsrestore: dump description: 
>>>>> +xfsrestore: hostname: xfsdump
>>>>> +xfsrestore: mount point: SCRATCH_MNT
>>>>> +xfsrestore: volume: SCRATCH_DEV
>>>>> +xfsrestore: session time: TIME
>>>>> +xfsrestore: level: 0
>>>>> +xfsrestore: session label: "stress_545"
>>>>> +xfsrestore: media label: "stress_tape_media"
>>>>> +xfsrestore: file system ID: ID
>>>>> +xfsrestore: session id: ID
>>>>> +xfsrestore: media ID: ID
>>>>> +xfsrestore: searching media for directory dump
>>>>> +xfsrestore: reading directories
>>>>> +xfsrestore: found fake rootino #128, will fix.
>>>>> +xfsrestore: fix root # to 1024 (bind mount?)
>>>>> +xfsrestore: 2 directories and 2 entries processed
>>>>> +xfsrestore: directory post-processing
>>>>> +xfsrestore: reading non-directory files
>>>>> +xfsrestore: table of contents display complete: SECS seconds elapsed
>>>>> +xfsrestore: Restore Status: SUCCESS
>>>>> +
>>>>> +dumpdir/FILE_1019
>>>>> +Restoring from file...
>>>>> +xfsrestore  -x -f DUMP_FILE  -L stress_545 RESTORE_DIR
>>>>> +xfsrestore: using file dump (drive_simple) strategy
>>>>> +xfsrestore: searching media for dump
>>>>> +xfsrestore: examining media file 0
>>>>> +xfsrestore: found dump matching specified label:
>>>>> +xfsrestore: hostname: xfsdump
>>>>> +xfsrestore: mount point: SCRATCH_MNT
>>>>> +xfsrestore: volume: SCRATCH_DEV
>>>>> +xfsrestore: session time: TIME
>>>>> +xfsrestore: level: 0
>>>>> +xfsrestore: session label: "stress_545"
>>>>> +xfsrestore: media label: "stress_tape_media"
>>>>> +xfsrestore: file system ID: ID
>>>>> +xfsrestore: session id: ID
>>>>> +xfsrestore: media ID: ID
>>>>> +xfsrestore: searching media for directory dump
>>>>> +xfsrestore: reading directories
>>>>> +xfsrestore: found fake rootino #128, will fix.
>>>>> +xfsrestore: fix root # to 1024 (bind mount?)
>>>>> +xfsrestore: 2 directories and 2 entries processed
>>>>> +xfsrestore: directory post-processing
>>>>> +xfsrestore: restoring non-directory files
>>>>> +xfsrestore: restore complete: SECS seconds elapsed
>>>>> +xfsrestore: Restore Status: SUCCESS
>>>>> +Comparing dump directory with restore directory
>>>>> +Files DUMP_DIR/FILE_1019 and RESTORE_DIR/DUMP_SUBDIR/FILE_1019 are identical
>>>>> +Comparing listing of dump directory with restore directory
>>>>> +Files TMP.dump_dir and TMP.restore_dir are identical
>>>>> -- 
>>>>> 2.37.3
>>>>>
>>>>
>>>
>>

Re: [RFC PATCH] xfs: test for fixing wrong root inode number

[Darrick J. Wong]

On Fri, Oct 07, 2022 at 09:59:35PM -0400, Hironori Shiina wrote:
> 
> 
> On 10/3/22 18:04, Darrick J. Wong wrote:
> > On Sun, Oct 02, 2022 at 12:27:08PM +0800, Zorro Lang wrote:
> >> On Fri, Sep 30, 2022 at 11:01:51AM -0400, Hironori Shiina wrote:
> >>>
> >>>
> >>> On 9/28/22 21:49, Zorro Lang wrote:
> >>>> On Wed, Sep 28, 2022 at 05:03:37PM -0400, Hironori Shiina wrote:
> >>>>> Test '-x' option of xfsrestore. With this option, a wrong root inode
> >>>>> number is corrected. A root inode number can be wrong in a dump created
> >>>>> by problematic xfsdump (v3.1.7 - v3.1.9) with blukstat misuse. This
> >>>>> patch adds a dump with a wrong inode number created by xfsdump 3.1.8.
> >>>>>
> >>>>> Link: https://lore.kernel.org/linux-xfs/20201113125127.966243-1-hsiangkao@redhat.com/
> >>>>> Signed-off-by: Hironori Shiina <shiina.hironori@fujitsu.com>
> >>>>> ---
> >>>>>  common/dump                    |   2 +-
> >>>>>  src/root-inode-broken-dumpfile | Bin 0 -> 21648 bytes
> >>>>>  tests/xfs/554                  |  37 +++++++++++++++++++++
> >>>>>  tests/xfs/554.out              |  57 +++++++++++++++++++++++++++++++++
> >>>>>  4 files changed, 95 insertions(+), 1 deletion(-)
> >>>>>  create mode 100644 src/root-inode-broken-dumpfile
> >>>>>  create mode 100644 tests/xfs/554
> >>>>>  create mode 100644 tests/xfs/554.out
> >>>>>
> >>>>> diff --git a/common/dump b/common/dump
> >>>>> index 8e0446d9..50b2ba03 100644
> >>>>> --- a/common/dump
> >>>>> +++ b/common/dump
> >>>>> @@ -1003,7 +1003,7 @@ _parse_restore_args()
> >>>>>          --no-check-quota)
> >>>>>              do_quota_check=false
> >>>>>              ;;
> >>>>> -	-K|-R)
> >>>>> +	-K|-R|-x)
> >>>>>  	    restore_args="$restore_args $1"
> >>>>>              ;;
> >>>>>  	*)
> >>>>> diff --git a/src/root-inode-broken-dumpfile b/src/root-inode-broken-dumpfile
> >>>>> new file mode 100644
> >>>>> index 0000000000000000000000000000000000000000..9a42e65d8047497be31f3abbaf4223ae384fd14d
> >>>>> GIT binary patch
> >>>>> literal 21648
> >>>>> zcmeI)K}ge49KiAC_J<CEBr!0AwBf~zbCHB}5DyxL6eU8Jnqylvayj;2VuG+d)TN6;
> >>>>> z5J8vlAaw9hXi(UousT$Sin@BRJfwHM)O+*2*njz_zc+h*ckuV#@BMuKe;;JbgTL{<
> >>>>> z!SwZ9zC#ERe%reLe5&cz2e}qM<!i2dXQHt^{^`d1Qx{)MMzW#$%dR@J={0`IRsGx4
> >>>>> z(yn@Oi-nBqCOVIG?&{kpwnv~&w_>6_ozY1^fph=w8(=^oTg&wOe=(WQByyQ_Hfd|4
> >>>>> z^o76<0!zn#v>k3blbU)nzx=KF^}-G%R;OaQYsHwGDkO`kD^@q^(_Ac_8H<gKj^>a0
> >>>>> z6p*%BK>q#b>2EE%^!@f?Z`-p+tI@M}qmMm@zMJk>K1U^=d~G^NUG?X4vkvKtOf-3Y
> >>>>> zpVM9YgM9Y7{*P0ApJNUh^uk1wCnA6V0tg_000IagfB*srAb<b@2q1s}0tg_000Iag
> >>>>> zfB*srAb<b@2q1s}0tg_000IagfB*srAb<b@2q1s}0tg_000IagfB*srAb`MM1p>_h
> >>>>> z2-R=Q9ooK1{l9<Dy8IIMUVXr9BW9uI1x7};j+kij|5kLI^32no;n|PvqD74ZOlJ#n
> >>>>> z0-~CKSlx#liMS>jt232#==00xPqwp;gsZrjc?`PPxaCE~>3(^o5-%+Nj=HegJFK2b
> >>>>> z=l5zT4IDgO=sJ1zp=jyrALvcQJK|j;sN2_jUmobjN<!S6m1{G<LZ^+JP;T!|3{9)w
> >>>>> eGf&ioo}iw|li2&4IyG;z<}vrl)Mic2`t2{52##q0
> >>>>>
> >>>>> literal 0
> >>>>> HcmV?d00001
> >>>>
> >>>> Please don't try to add a binary file to fstests directly.
> >>>>
> >>>>>
> >>>>> diff --git a/tests/xfs/554 b/tests/xfs/554
> >>>>> new file mode 100644
> >>>>> index 00000000..13bc62c7
> >>>>> --- /dev/null
> >>>>> +++ b/tests/xfs/554
> >>>>> @@ -0,0 +1,37 @@
> >>>>> +#! /bin/bash
> >>>>> +# SPDX-License-Identifier: GPL-2.0
> >>>>> +# Copyright (c) 2022 Fujitsu Limited. All Rights Reserved.
> >>>>> +#
> >>>>> +# FS QA Test No. 554
> >>>>> +#
> >>>>> +# Test restoring a dumpfile with a wrong root inode number created by
> >>>>> +# xfsdump 3.1.8.
> >>>>> +# This test restores the checked-in broken dump with '-x' flag.
> >>>>> +#
> >>>>> +
> >>>>> +. ./common/preamble
> >>>>> +_begin_fstest auto quick dump
> >>>>> +
> >>>>> +# Import common functions.
> >>>>> +. ./common/dump
> >>>>> +
> >>>>> +# real QA test starts here
> >>>>> +_supported_fs xfs
> >>>>> +_require_scratch
> >>>>
> >>>> The -x option is a new feature for xfsdump, not all system support that. So
> >>>> we need to _notrun if test on a system doesn't support it. A separated
> >>>> _require_* helper would be better if there'll be more testing about this
> >>>> new feature. Or a local detection in this case is fine too (can be moved as
> >>>> a common helper later).
> >>>>
> >>>>> +_scratch_mkfs_xfs >>$seqres.full || _fail "mkfs failed"
> >>>>> +_scratch_mount
> >>>>> +
> >>>>> +# Create dumpdir for comparing with restoredir
> >>>>> +rm -rf $dump_dir
> >>>>> +mkdir $dump_dir || _fail "failed to mkdir $restore_dir"
> >>>>           ^^                                  ^^
> >>>>
> >>>> Are you trying to create a dump dir or restore dir?
> >>>>
> >>>>> +touch $dump_dir/FILE_1019
> >>>>> +
> >>>>> +_do_restore_toc -x -f $here/src/root-inode-broken-dumpfile
> >>>>
> >>>> Why I didn't see how you generate this broken dumpfile in this case?
> >>>>
> >>>> Oh... I see, you want to store a dumpfile in fstests source code directly.
> >>>> I thought you submited that file accidentally...
> >>>>
> >>>> No, we don't do things like this way, please try to generate the dumpfile
> >>>> in this test case at first, then restore it. For example using xfs_db to
> >>>> break a xfs, or using some tricky method (likes xfs/545).
> >>>>
> >>>
> >>> Thank you for the comments. I will try another approach. I'm having
> >>> trouble creating a dumpfile for this test. Because xfsdump was already
> >>> fixed, xfsdump no longer generates a corrupted dumpfile even if there is
> >>> a lower inode number than a root inode.
> >>
> >> Oh, I see. You can try, but I think it's hard. It maybe not suitable to be a
> >> fstests case, if it has to depend on a binary fs dump file. If so, We can cover
> >> it on other place, with this existed "bad" dump file.
> > 
> > How difficult is it to create a dumpfile with a broken root inode?
> > Is it a simple matter of creating a good dump and patching a few bytes,
> > or do we end up having to patch the whole file?
> > 
> > (The reason I ask is that I've heard about this problem for ages but
> > I don't actually know how to create a bad dump...)
> > 
> 
> As I dug into the data structure, I succeeded in creating a dumpfile with a wrong
> root inode by modifying `content_inode_hdr_t.cih_rootino` and
> `global_hdr_t.gh_checksum`.
> 
> After creating a inode with a lower number than the root inode with the method in
> generic/545, I modified a dumpfile as follows:
> ---
> _do_dump_file
> 
> # Break dumpfile
> old_checksum=$(od -A n -j 12 -N 4 -i --endian=big $dump_file)
> gap=$(($root_inum - $inum))
> new_checksum=$(($old_checksum + ($gap >> 32) + ($gap & 0x00000000ffffffff)))
> v=($(printf "%016x" $inum |  awk -v FS='' '{for (i=1; i<=NF; i+=2) print $i$(i+1)}'))
> echo -en "\x${v[0]}\x${v[1]}\x${v[2]}\x${v[3]}\x${v[4]}\x${v[5]}\x${v[6]}\x${v[7]}" | dd of=$dump_file bs=1 seek=3928 conv=notrunc
> v=($(printf "%016x" $new_checksum |  awk -v FS='' '{for (i=9; i<=NF; i+=2) print $i$(i+1)}'))
> echo -en "\x${v[0]}\x${v[1]}\x${v[2]}\x${v[3]}" | dd of=$dump_file bs=1 seek=12 conv=notrunc
> 
> _do_restore_file -x
> ---
> (I'm afraid I'm not so familiar with editing a binary file.)

That's the only way I know of to do this via shell script. :/

Perhaps it would be cleaner to submit a small C program to do the
editing instead of relying on bash?

> The offset of `global_hdr_t.gh_checksum` is 12.
> The offset of `content_inode_hdr_t.cih_rootino` is:
>   global_hdr_t.gh_upper (0x400) + drive_hdr_t.dh_upper (0x400) + 
>   media_hdr_t.mh_upper (0x400) + content_hdr_t.ch_specific (0x340) +
>   content_inode_hdr_t.cih_rootino (0x18) = 0xf58 (3928)

Whatever you come up with, please include this comment with the program
code so that it's more obvious what the byte editing is doing.

--D

> Thanks,
> Hiro
> 
> > --D
> > 
> >> Thanks,
> >> Zorro
> >>
> >>>
> >>>>> +
> >>>>> +_do_restore_file -x -f $here/src/root-inode-broken-dumpfile -L stress_545
> >>>>> +_diff_compare_sub
> >>>>> +_ls_nodate_compare_sub
> >>>>> +
> >>>>> +# success, all done
> >>>>> +status=0
> >>>>> +exit
> >>>>> diff --git a/tests/xfs/554.out b/tests/xfs/554.out
> >>>>> new file mode 100644
> >>>>> index 00000000..40a3f3a4
> >>>>> --- /dev/null
> >>>>> +++ b/tests/xfs/554.out
> >>>>> @@ -0,0 +1,57 @@
> >>>>> +QA output created by 554
> >>>>> +Contents of dump ...
> >>>>> +xfsrestore  -x -f DUMP_FILE -t
> >>>>> +xfsrestore: using file dump (drive_simple) strategy
> >>>>> +xfsrestore: searching media for dump
> >>>>> +xfsrestore: examining media file 0
> >>>>> +xfsrestore: dump description: 
> >>>>> +xfsrestore: hostname: xfsdump
> >>>>> +xfsrestore: mount point: SCRATCH_MNT
> >>>>> +xfsrestore: volume: SCRATCH_DEV
> >>>>> +xfsrestore: session time: TIME
> >>>>> +xfsrestore: level: 0
> >>>>> +xfsrestore: session label: "stress_545"
> >>>>> +xfsrestore: media label: "stress_tape_media"
> >>>>> +xfsrestore: file system ID: ID
> >>>>> +xfsrestore: session id: ID
> >>>>> +xfsrestore: media ID: ID
> >>>>> +xfsrestore: searching media for directory dump
> >>>>> +xfsrestore: reading directories
> >>>>> +xfsrestore: found fake rootino #128, will fix.
> >>>>> +xfsrestore: fix root # to 1024 (bind mount?)
> >>>>> +xfsrestore: 2 directories and 2 entries processed
> >>>>> +xfsrestore: directory post-processing
> >>>>> +xfsrestore: reading non-directory files
> >>>>> +xfsrestore: table of contents display complete: SECS seconds elapsed
> >>>>> +xfsrestore: Restore Status: SUCCESS
> >>>>> +
> >>>>> +dumpdir/FILE_1019
> >>>>> +Restoring from file...
> >>>>> +xfsrestore  -x -f DUMP_FILE  -L stress_545 RESTORE_DIR
> >>>>> +xfsrestore: using file dump (drive_simple) strategy
> >>>>> +xfsrestore: searching media for dump
> >>>>> +xfsrestore: examining media file 0
> >>>>> +xfsrestore: found dump matching specified label:
> >>>>> +xfsrestore: hostname: xfsdump
> >>>>> +xfsrestore: mount point: SCRATCH_MNT
> >>>>> +xfsrestore: volume: SCRATCH_DEV
> >>>>> +xfsrestore: session time: TIME
> >>>>> +xfsrestore: level: 0
> >>>>> +xfsrestore: session label: "stress_545"
> >>>>> +xfsrestore: media label: "stress_tape_media"
> >>>>> +xfsrestore: file system ID: ID
> >>>>> +xfsrestore: session id: ID
> >>>>> +xfsrestore: media ID: ID
> >>>>> +xfsrestore: searching media for directory dump
> >>>>> +xfsrestore: reading directories
> >>>>> +xfsrestore: found fake rootino #128, will fix.
> >>>>> +xfsrestore: fix root # to 1024 (bind mount?)
> >>>>> +xfsrestore: 2 directories and 2 entries processed
> >>>>> +xfsrestore: directory post-processing
> >>>>> +xfsrestore: restoring non-directory files
> >>>>> +xfsrestore: restore complete: SECS seconds elapsed
> >>>>> +xfsrestore: Restore Status: SUCCESS
> >>>>> +Comparing dump directory with restore directory
> >>>>> +Files DUMP_DIR/FILE_1019 and RESTORE_DIR/DUMP_SUBDIR/FILE_1019 are identical
> >>>>> +Comparing listing of dump directory with restore directory
> >>>>> +Files TMP.dump_dir and TMP.restore_dir are identical
> >>>>> -- 
> >>>>> 2.37.3
> >>>>>
> >>>>
> >>>
> >>

[PATCH V2] xfs: test for fixing wrong root inode number in dump

[Hironori Shiina]

Test '-x' option of xfsrestore. With this option, a wrong root inode
number in a dump file is corrected. A root inode number can be wrong
in a dump created by problematic xfsdump (v3.1.7 - v3.1.9) with
bulkstat misuse. In this test, a corrupted dump file is created by
overwriting a root inode number in a header.

Signed-off-by: Hironori Shiina <shiina.hironori@fujitsu.com>
---
changes since RFC v1:
  - Skip the test if xfsrestore does not support '-x' flag.
  - Create a corrupted dump by overwriting a root inode number in a dump
    file with a new tool instead of checking in a binary dump file.

 common/dump             |  2 +-
 common/xfs              |  6 +++
 src/Makefile            |  2 +-
 src/fake-dump-rootino.c | 85 +++++++++++++++++++++++++++++++++++++++++
 tests/xfs/554           | 73 +++++++++++++++++++++++++++++++++++
 tests/xfs/554.out       | 40 +++++++++++++++++++
 6 files changed, 206 insertions(+), 2 deletions(-)
 create mode 100644 src/fake-dump-rootino.c
 create mode 100755 tests/xfs/554
 create mode 100644 tests/xfs/554.out

diff --git a/common/dump b/common/dump
index 8e0446d9..50b2ba03 100644
--- a/common/dump
+++ b/common/dump
@@ -1003,7 +1003,7 @@ _parse_restore_args()
         --no-check-quota)
             do_quota_check=false
             ;;
-	-K|-R)
+	-K|-R|-x)
 	    restore_args="$restore_args $1"
             ;;
 	*)
diff --git a/common/xfs b/common/xfs
index e1c15d3d..8334880e 100644
--- a/common/xfs
+++ b/common/xfs
@@ -1402,3 +1402,9 @@ _xfs_filter_mkfs()
 		print STDOUT "realtime =RDEV extsz=XXX blocks=XXX, rtextents=XXX\n";
 	}'
 }
+
+_require_xfsrestore_xflag()
+{
+	$XFSRESTORE_PROG -h 2>&1 | grep -q -e '-x' || \
+			_notrun 'xfsrestore does not support -x flag.'
+}
diff --git a/src/Makefile b/src/Makefile
index 5f565e73..afdf6b30 100644
--- a/src/Makefile
+++ b/src/Makefile
@@ -19,7 +19,7 @@ TARGETS = dirstress fill fill2 getpagesize holes lstat64 \
 	t_ofd_locks t_mmap_collision mmap-write-concurrent \
 	t_get_file_time t_create_short_dirs t_create_long_dirs t_enospc \
 	t_mmap_writev_overlap checkpoint_journal mmap-rw-fault allocstale \
-	t_mmap_cow_memory_failure
+	t_mmap_cow_memory_failure fake-dump-rootino
 
 LINUX_TARGETS = xfsctl bstat t_mtab getdevicesize preallo_rw_pattern_reader \
 	preallo_rw_pattern_writer ftrunc trunc fs_perms testx looptest \
diff --git a/src/fake-dump-rootino.c b/src/fake-dump-rootino.c
new file mode 100644
index 00000000..b89351b8
--- /dev/null
+++ b/src/fake-dump-rootino.c
@@ -0,0 +1,85 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2022 Fujitsu Limited.  All Rights Reserved. */
+#include <fcntl.h>
+#include <stdint.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <sys/mman.h>
+#include <unistd.h>
+
+// Values for size of dump file header from xfsdump
+#define PGSZLOG2	12
+#define PGSZ		(1 << PGSZLOG2)
+#define GLOBAL_HDR_SZ		PGSZ
+
+static inline uint32_t convert_endian_32(uint32_t val) {
+#if __BYTE_ORDER == __BIG_ENDIAN
+	return val;
+#else
+	return ((val & 0xff000000u) >> 24 |
+			(val & 0x00ff0000u) >> 8  |
+			(val & 0x0000ff00u) << 8  |
+			(val & 0x000000ffu) << 24);
+#endif
+}
+
+static inline uint64_t convert_endian_64(uint64_t val) {
+#if __BYTE_ORDER == __BIG_ENDIAN
+	return val;
+#else
+	return (uint64_t) convert_endian_32(val >> 32) |
+	       (uint64_t) convert_endian_32(val & 0x00000000ffffffff) << 32;
+#endif
+}
+
+/*
+ * Offset to checksum in dump file header
+ *   global_hdr_t.gh_checksum (0xc)
+ */
+#define OFFSET_CHECKSUM	0xc
+
+/*
+ * Offset to root inode number in dump file header
+ *   global_hdr_t.gh_upper (0x400) + drive_hdr_t.dh_upper (0x400) +
+ *   media_hdr_t.mh_upper (0x400) + content_hdr_t.ch_specific (0x340) +
+ *   content_inode_hdr_t.cih_rootino (0x18)
+ */
+#define OFFSET_ROOTINO	0xf58
+
+int main(int argc, char *argv[]) {
+
+	if (argc < 3) {
+		fprintf(stderr, "Usage: %s <path/to/dumpfile> <fake rootino>\n", argv[0]);
+		exit(1);
+	}
+
+	const char *filepath = argv[1];
+	const uint64_t fake_root_ino = (uint64_t) strtol(argv[2], NULL, 10);
+
+	int fd = open(filepath, O_RDWR);
+	if (fd < 0) {
+		perror("open");
+		exit(1);
+	}
+	char *header = mmap(NULL, GLOBAL_HDR_SZ, PROT_READ|PROT_WRITE, MAP_SHARED, fd, 0);
+	if (header == MAP_FAILED) {
+		perror("mmap");
+		exit(1);
+	}
+
+	uint32_t *checksum_ptr = (uint32_t *) (header + OFFSET_CHECKSUM);
+	uint64_t *rootino_ptr = (uint64_t *) (header + OFFSET_ROOTINO);
+	int32_t checksum = (int32_t) convert_endian_32(*checksum_ptr);
+	uint64_t orig_rootino = convert_endian_64(*rootino_ptr);
+
+	// Fake root inode number
+	*rootino_ptr = convert_endian_64(fake_root_ino);
+
+	// Update checksum along with overwriting rootino.
+	uint64_t gap = orig_rootino - fake_root_ino;
+	checksum += (gap >> 32) + (gap & 0x00000000ffffffff);
+	*checksum_ptr = convert_endian_32(checksum);
+
+	munmap(header, GLOBAL_HDR_SZ);
+	close(fd);
+}
diff --git a/tests/xfs/554 b/tests/xfs/554
new file mode 100755
index 00000000..fcfaa699
--- /dev/null
+++ b/tests/xfs/554
@@ -0,0 +1,73 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2022 Fujitsu Limited. All Rights Reserved.
+#
+# FS QA Test No. 554
+#
+# Create a filesystem which contains an inode with a lower number
+# than the root inode. Set the lower number to a dump file as the root inode
+# and ensure that 'xfsrestore -x' handles this wrong inode.
+#
+. ./common/preamble
+_begin_fstest auto quick dump
+
+# Import common functions.
+. ./common/dump
+
+_supported_fs xfs
+_require_xfs_io_command "falloc"
+_require_scratch
+_require_xfsrestore_xflag
+
+# A large stripe unit will put the root inode out quite far
+# due to alignment, leaving free blocks ahead of it.
+_scratch_mkfs_xfs -d sunit=1024,swidth=1024 > $seqres.full 2>&1
+
+# Mounting /without/ a stripe should allow inodes to be allocated
+# in lower free blocks, without the stripe alignment.
+_scratch_mount -o sunit=0,swidth=0
+
+root_inum=$(stat -c %i $SCRATCH_MNT)
+
+# Consume space after the root inode so that the blocks before
+# root look "close" for the next inode chunk allocation
+$XFS_IO_PROG -f -c "falloc 0 16m" $SCRATCH_MNT/fillfile
+
+# And make a bunch of inodes until we (hopefully) get one lower
+# than root, in a new inode chunk.
+echo "root_inum: $root_inum" >> $seqres.full
+for i in $(seq 0 4096) ; do
+	fname=$SCRATCH_MNT/$(printf "FILE_%03d" $i)
+	touch $fname
+	inum=$(stat -c "%i" $fname)
+	[[ $inum -lt $root_inum ]] && break
+done
+
+echo "created: $inum" >> $seqres.full
+
+[[ $inum -lt $root_inum ]] || _notrun "Could not set up test"
+
+# Now try a dump and restore. Cribbed from xfs/068
+_create_dumpdir_stress
+
+echo -n "Before: " >> $seqres.full
+_count_dumpdir_files | tee $tmp.before >> $seqres.full
+
+_do_dump_file
+
+# Set the wrong root inode number to the dump file
+# as problematic xfsdump used to do.
+$here/src/fake-dump-rootino $dump_file $inum
+
+_do_restore_file -x | \
+sed -e "s/rootino #${inum}/rootino #FAKENO/g" \
+	-e "s/# to ${root_inum}/# to ROOTNO/g" \
+	-e "/entries processed$/s/[0-9][0-9]*/NUM/g"
+
+echo -n "After: " >> $seqres.full
+_count_restoredir_files | tee $tmp.after >> $seqres.full
+diff -u $tmp.before $tmp.after
+
+# success, all done
+status=0
+exit
diff --git a/tests/xfs/554.out b/tests/xfs/554.out
new file mode 100644
index 00000000..c5e8c4c5
--- /dev/null
+++ b/tests/xfs/554.out
@@ -0,0 +1,40 @@
+QA output created by 554
+Creating directory system to dump using fsstress.
+
+-----------------------------------------------
+fsstress : -f link=10 -f creat=10 -f mkdir=10 -f truncate=5 -f symlink=10
+-----------------------------------------------
+Dumping to file...
+xfsdump  -f DUMP_FILE -M stress_tape_media -L stress_554 SCRATCH_MNT
+xfsdump: using file dump (drive_simple) strategy
+xfsdump: level 0 dump of HOSTNAME:SCRATCH_MNT
+xfsdump: dump date: DATE
+xfsdump: session id: ID
+xfsdump: session label: "stress_554"
+xfsdump: ino map <PHASES>
+xfsdump: ino map construction complete
+xfsdump: estimated dump size: NUM bytes
+xfsdump: /var/xfsdump/inventory created
+xfsdump: creating dump session media file 0 (media 0, file 0)
+xfsdump: dumping ino map
+xfsdump: dumping directories
+xfsdump: dumping non-directory files
+xfsdump: ending media file
+xfsdump: media file size NUM bytes
+xfsdump: dump size (non-dir files) : NUM bytes
+xfsdump: dump complete: SECS seconds elapsed
+xfsdump: Dump Status: SUCCESS
+Restoring from file...
+xfsrestore  -x -f DUMP_FILE  -L stress_554 RESTORE_DIR
+xfsrestore: using file dump (drive_simple) strategy
+xfsrestore: using online session inventory
+xfsrestore: searching media for directory dump
+xfsrestore: examining media file 0
+xfsrestore: reading directories
+xfsrestore: found fake rootino #FAKENO, will fix.
+xfsrestore: fix root # to ROOTNO (bind mount?)
+xfsrestore: NUM directories and NUM entries processed
+xfsrestore: directory post-processing
+xfsrestore: restoring non-directory files
+xfsrestore: restore complete: SECS seconds elapsed
+xfsrestore: Restore Status: SUCCESS
-- 
2.37.3

Re: [PATCH V2] xfs: test for fixing wrong root inode number in dump

[Darrick J. Wong]

On Thu, Oct 13, 2022 at 12:04:34PM -0400, Hironori Shiina wrote:
> Test '-x' option of xfsrestore. With this option, a wrong root inode
> number in a dump file is corrected. A root inode number can be wrong
> in a dump created by problematic xfsdump (v3.1.7 - v3.1.9) with
> bulkstat misuse. In this test, a corrupted dump file is created by
> overwriting a root inode number in a header.
> 
> Signed-off-by: Hironori Shiina <shiina.hironori@fujitsu.com>
> ---
> changes since RFC v1:
>   - Skip the test if xfsrestore does not support '-x' flag.
>   - Create a corrupted dump by overwriting a root inode number in a dump
>     file with a new tool instead of checking in a binary dump file.
> 
>  common/dump             |  2 +-
>  common/xfs              |  6 +++
>  src/Makefile            |  2 +-
>  src/fake-dump-rootino.c | 85 +++++++++++++++++++++++++++++++++++++++++
>  tests/xfs/554           | 73 +++++++++++++++++++++++++++++++++++
>  tests/xfs/554.out       | 40 +++++++++++++++++++
>  6 files changed, 206 insertions(+), 2 deletions(-)
>  create mode 100644 src/fake-dump-rootino.c
>  create mode 100755 tests/xfs/554
>  create mode 100644 tests/xfs/554.out
> 
> diff --git a/common/dump b/common/dump
> index 8e0446d9..50b2ba03 100644
> --- a/common/dump
> +++ b/common/dump
> @@ -1003,7 +1003,7 @@ _parse_restore_args()
>          --no-check-quota)
>              do_quota_check=false
>              ;;
> -	-K|-R)
> +	-K|-R|-x)
>  	    restore_args="$restore_args $1"
>              ;;
>  	*)
> diff --git a/common/xfs b/common/xfs
> index e1c15d3d..8334880e 100644
> --- a/common/xfs
> +++ b/common/xfs
> @@ -1402,3 +1402,9 @@ _xfs_filter_mkfs()
>  		print STDOUT "realtime =RDEV extsz=XXX blocks=XXX, rtextents=XXX\n";
>  	}'
>  }
> +
> +_require_xfsrestore_xflag()
> +{
> +	$XFSRESTORE_PROG -h 2>&1 | grep -q -e '-x' || \
> +			_notrun 'xfsrestore does not support -x flag.'
> +}
> diff --git a/src/Makefile b/src/Makefile
> index 5f565e73..afdf6b30 100644
> --- a/src/Makefile
> +++ b/src/Makefile
> @@ -19,7 +19,7 @@ TARGETS = dirstress fill fill2 getpagesize holes lstat64 \
>  	t_ofd_locks t_mmap_collision mmap-write-concurrent \
>  	t_get_file_time t_create_short_dirs t_create_long_dirs t_enospc \
>  	t_mmap_writev_overlap checkpoint_journal mmap-rw-fault allocstale \
> -	t_mmap_cow_memory_failure
> +	t_mmap_cow_memory_failure fake-dump-rootino
>  
>  LINUX_TARGETS = xfsctl bstat t_mtab getdevicesize preallo_rw_pattern_reader \
>  	preallo_rw_pattern_writer ftrunc trunc fs_perms testx looptest \
> diff --git a/src/fake-dump-rootino.c b/src/fake-dump-rootino.c
> new file mode 100644
> index 00000000..b89351b8
> --- /dev/null
> +++ b/src/fake-dump-rootino.c
> @@ -0,0 +1,85 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2022 Fujitsu Limited.  All Rights Reserved. */
> +#include <fcntl.h>
> +#include <stdint.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <sys/mman.h>
> +#include <unistd.h>
> +
> +// Values for size of dump file header from xfsdump
> +#define PGSZLOG2	12
> +#define PGSZ		(1 << PGSZLOG2)
> +#define GLOBAL_HDR_SZ		PGSZ
> +
> +static inline uint32_t convert_endian_32(uint32_t val) {
> +#if __BYTE_ORDER == __BIG_ENDIAN
> +	return val;
> +#else
> +	return ((val & 0xff000000u) >> 24 |
> +			(val & 0x00ff0000u) >> 8  |
> +			(val & 0x0000ff00u) << 8  |
> +			(val & 0x000000ffu) << 24);
> +#endif
> +}

be32_to_cpu?

> +
> +static inline uint64_t convert_endian_64(uint64_t val) {
> +#if __BYTE_ORDER == __BIG_ENDIAN
> +	return val;
> +#else
> +	return (uint64_t) convert_endian_32(val >> 32) |
> +	       (uint64_t) convert_endian_32(val & 0x00000000ffffffff) << 32;
> +#endif
> +}

be64_to_cpu?

> +
> +/*
> + * Offset to checksum in dump file header
> + *   global_hdr_t.gh_checksum (0xc)
> + */
> +#define OFFSET_CHECKSUM	0xc
> +
> +/*
> + * Offset to root inode number in dump file header
> + *   global_hdr_t.gh_upper (0x400) + drive_hdr_t.dh_upper (0x400) +
> + *   media_hdr_t.mh_upper (0x400) + content_hdr_t.ch_specific (0x340) +
> + *   content_inode_hdr_t.cih_rootino (0x18)
> + */
> +#define OFFSET_ROOTINO	0xf58

Since you're now using a C program, I think it makes more sense to copy
the structure definitions directly into the C file.

> +
> +int main(int argc, char *argv[]) {
> +
> +	if (argc < 3) {
> +		fprintf(stderr, "Usage: %s <path/to/dumpfile> <fake rootino>\n", argv[0]);
> +		exit(1);
> +	}
> +
> +	const char *filepath = argv[1];
> +	const uint64_t fake_root_ino = (uint64_t) strtol(argv[2], NULL, 10);
> +
> +	int fd = open(filepath, O_RDWR);
> +	if (fd < 0) {
> +		perror("open");
> +		exit(1);
> +	}
> +	char *header = mmap(NULL, GLOBAL_HDR_SZ, PROT_READ|PROT_WRITE, MAP_SHARED, fd, 0);
> +	if (header == MAP_FAILED) {
> +		perror("mmap");
> +		exit(1);
> +	}
> +
> +	uint32_t *checksum_ptr = (uint32_t *) (header + OFFSET_CHECKSUM);
> +	uint64_t *rootino_ptr = (uint64_t *) (header + OFFSET_ROOTINO);
> +	int32_t checksum = (int32_t) convert_endian_32(*checksum_ptr);
> +	uint64_t orig_rootino = convert_endian_64(*rootino_ptr);
> +
> +	// Fake root inode number
> +	*rootino_ptr = convert_endian_64(fake_root_ino);
> +
> +	// Update checksum along with overwriting rootino.
> +	uint64_t gap = orig_rootino - fake_root_ino;
> +	checksum += (gap >> 32) + (gap & 0x00000000ffffffff);
> +	*checksum_ptr = convert_endian_32(checksum);
> +
> +	munmap(header, GLOBAL_HDR_SZ);
> +	close(fd);
> +}
> diff --git a/tests/xfs/554 b/tests/xfs/554
> new file mode 100755
> index 00000000..fcfaa699
> --- /dev/null
> +++ b/tests/xfs/554
> @@ -0,0 +1,73 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2022 Fujitsu Limited. All Rights Reserved.
> +#
> +# FS QA Test No. 554
> +#
> +# Create a filesystem which contains an inode with a lower number
> +# than the root inode. Set the lower number to a dump file as the root inode
> +# and ensure that 'xfsrestore -x' handles this wrong inode.
> +#
> +. ./common/preamble
> +_begin_fstest auto quick dump
> +
> +# Import common functions.
> +. ./common/dump
> +
> +_supported_fs xfs
> +_require_xfs_io_command "falloc"
> +_require_scratch
> +_require_xfsrestore_xflag

_require_test_program fake-dump-rootino

> +
> +# A large stripe unit will put the root inode out quite far
> +# due to alignment, leaving free blocks ahead of it.
> +_scratch_mkfs_xfs -d sunit=1024,swidth=1024 > $seqres.full 2>&1
> +
> +# Mounting /without/ a stripe should allow inodes to be allocated
> +# in lower free blocks, without the stripe alignment.
> +_scratch_mount -o sunit=0,swidth=0
> +
> +root_inum=$(stat -c %i $SCRATCH_MNT)
> +
> +# Consume space after the root inode so that the blocks before
> +# root look "close" for the next inode chunk allocation
> +$XFS_IO_PROG -f -c "falloc 0 16m" $SCRATCH_MNT/fillfile
> +
> +# And make a bunch of inodes until we (hopefully) get one lower
> +# than root, in a new inode chunk.
> +echo "root_inum: $root_inum" >> $seqres.full
> +for i in $(seq 0 4096) ; do
> +	fname=$SCRATCH_MNT/$(printf "FILE_%03d" $i)
> +	touch $fname
> +	inum=$(stat -c "%i" $fname)
> +	[[ $inum -lt $root_inum ]] && break
> +done
> +
> +echo "created: $inum" >> $seqres.full
> +
> +[[ $inum -lt $root_inum ]] || _notrun "Could not set up test"
> +
> +# Now try a dump and restore. Cribbed from xfs/068
> +_create_dumpdir_stress
> +
> +echo -n "Before: " >> $seqres.full
> +_count_dumpdir_files | tee $tmp.before >> $seqres.full
> +
> +_do_dump_file
> +
> +# Set the wrong root inode number to the dump file
> +# as problematic xfsdump used to do.
> +$here/src/fake-dump-rootino $dump_file $inum
> +
> +_do_restore_file -x | \
> +sed -e "s/rootino #${inum}/rootino #FAKENO/g" \
> +	-e "s/# to ${root_inum}/# to ROOTNO/g" \
> +	-e "/entries processed$/s/[0-9][0-9]*/NUM/g"
> +
> +echo -n "After: " >> $seqres.full
> +_count_restoredir_files | tee $tmp.after >> $seqres.full
> +diff -u $tmp.before $tmp.after

The rest of this logic looks reasonable to me though. :)

--D

> +# success, all done
> +status=0
> +exit
> diff --git a/tests/xfs/554.out b/tests/xfs/554.out
> new file mode 100644
> index 00000000..c5e8c4c5
> --- /dev/null
> +++ b/tests/xfs/554.out
> @@ -0,0 +1,40 @@
> +QA output created by 554
> +Creating directory system to dump using fsstress.
> +
> +-----------------------------------------------
> +fsstress : -f link=10 -f creat=10 -f mkdir=10 -f truncate=5 -f symlink=10
> +-----------------------------------------------
> +Dumping to file...
> +xfsdump  -f DUMP_FILE -M stress_tape_media -L stress_554 SCRATCH_MNT
> +xfsdump: using file dump (drive_simple) strategy
> +xfsdump: level 0 dump of HOSTNAME:SCRATCH_MNT
> +xfsdump: dump date: DATE
> +xfsdump: session id: ID
> +xfsdump: session label: "stress_554"
> +xfsdump: ino map <PHASES>
> +xfsdump: ino map construction complete
> +xfsdump: estimated dump size: NUM bytes
> +xfsdump: /var/xfsdump/inventory created
> +xfsdump: creating dump session media file 0 (media 0, file 0)
> +xfsdump: dumping ino map
> +xfsdump: dumping directories
> +xfsdump: dumping non-directory files
> +xfsdump: ending media file
> +xfsdump: media file size NUM bytes
> +xfsdump: dump size (non-dir files) : NUM bytes
> +xfsdump: dump complete: SECS seconds elapsed
> +xfsdump: Dump Status: SUCCESS
> +Restoring from file...
> +xfsrestore  -x -f DUMP_FILE  -L stress_554 RESTORE_DIR
> +xfsrestore: using file dump (drive_simple) strategy
> +xfsrestore: using online session inventory
> +xfsrestore: searching media for directory dump
> +xfsrestore: examining media file 0
> +xfsrestore: reading directories
> +xfsrestore: found fake rootino #FAKENO, will fix.
> +xfsrestore: fix root # to ROOTNO (bind mount?)
> +xfsrestore: NUM directories and NUM entries processed
> +xfsrestore: directory post-processing
> +xfsrestore: restoring non-directory files
> +xfsrestore: restore complete: SECS seconds elapsed
> +xfsrestore: Restore Status: SUCCESS
> -- 
> 2.37.3
> 

[PATCH V3] xfs: test for fixing wrong root inode number in dump

[Hironori Shiina]

Test '-x' option of xfsrestore. With this option, a wrong root inode
number in a dump file is corrected. A root inode number can be wrong
in a dump created by problematic xfsdump (v3.1.7 - v3.1.9) with
bulkstat misuse. In this test, a corrupted dump file is created by
overwriting a root inode number in a header.

Signed-off-by: Hironori Shiina <shiina.hironori@fujitsu.com>
---
changes since v2:
  - Use glibc functions to convert endian.
  - Copy the structure definitions from xfsdump source files.
changes since RFC v1:
  - Skip the test if xfsrestore does not support '-x' flag.
  - Create a corrupted dump by overwriting a root inode number in a dump
    file with a new tool instead of checking in a binary dump file.

 common/dump             |   2 +-
 common/xfs              |   6 ++
 src/Makefile            |   2 +-
 src/fake-dump-rootino.c | 228 ++++++++++++++++++++++++++++++++++++++++
 tests/xfs/554           |  73 +++++++++++++
 tests/xfs/554.out       |  40 +++++++
 6 files changed, 349 insertions(+), 2 deletions(-)
 create mode 100644 src/fake-dump-rootino.c
 create mode 100755 tests/xfs/554
 create mode 100644 tests/xfs/554.out

diff --git a/common/dump b/common/dump
index 8e0446d9..50b2ba03 100644
--- a/common/dump
+++ b/common/dump
@@ -1003,7 +1003,7 @@ _parse_restore_args()
         --no-check-quota)
             do_quota_check=false
             ;;
-	-K|-R)
+	-K|-R|-x)
 	    restore_args="$restore_args $1"
             ;;
 	*)
diff --git a/common/xfs b/common/xfs
index e1c15d3d..8334880e 100644
--- a/common/xfs
+++ b/common/xfs
@@ -1402,3 +1402,9 @@ _xfs_filter_mkfs()
 		print STDOUT "realtime =RDEV extsz=XXX blocks=XXX, rtextents=XXX\n";
 	}'
 }
+
+_require_xfsrestore_xflag()
+{
+	$XFSRESTORE_PROG -h 2>&1 | grep -q -e '-x' || \
+			_notrun 'xfsrestore does not support -x flag.'
+}
diff --git a/src/Makefile b/src/Makefile
index 5f565e73..afdf6b30 100644
--- a/src/Makefile
+++ b/src/Makefile
@@ -19,7 +19,7 @@ TARGETS = dirstress fill fill2 getpagesize holes lstat64 \
 	t_ofd_locks t_mmap_collision mmap-write-concurrent \
 	t_get_file_time t_create_short_dirs t_create_long_dirs t_enospc \
 	t_mmap_writev_overlap checkpoint_journal mmap-rw-fault allocstale \
-	t_mmap_cow_memory_failure
+	t_mmap_cow_memory_failure fake-dump-rootino
 
 LINUX_TARGETS = xfsctl bstat t_mtab getdevicesize preallo_rw_pattern_reader \
 	preallo_rw_pattern_writer ftrunc trunc fs_perms testx looptest \
diff --git a/src/fake-dump-rootino.c b/src/fake-dump-rootino.c
new file mode 100644
index 00000000..80797f15
--- /dev/null
+++ b/src/fake-dump-rootino.c
@@ -0,0 +1,228 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2022 Fujitsu Limited.  All Rights Reserved. */
+#define _LARGEFILE64_SOURCE
+#include <endian.h>
+#include <fcntl.h>
+#include <stdint.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <sys/mman.h>
+#include <unistd.h>
+#include <uuid/uuid.h>
+
+// Definitions from xfsdump
+#define PGSZLOG2	12
+#define PGSZ		(1 << PGSZLOG2)
+#define GLOBAL_HDR_SZ		PGSZ
+#define GLOBAL_HDR_MAGIC_SZ	8
+#define GLOBAL_HDR_STRING_SZ	0x100
+#define GLOBAL_HDR_TIME_SZ	4
+#define GLOBAL_HDR_UUID_SZ	0x10
+typedef int32_t time32_t;
+struct global_hdr {
+	char gh_magic[GLOBAL_HDR_MAGIC_SZ];		/*   8    8 */
+		/* unique signature of xfsdump */
+	uint32_t gh_version;				/*   4    c */
+		/* header version */
+	uint32_t gh_checksum;				/*   4   10 */
+		/* 32-bit unsigned additive inverse of entire header */
+	time32_t gh_timestamp;				/*   4   14 */
+		/* time32_t of dump */
+	char gh_pad1[4];				/*   4   18 */
+		/* alignment */
+	uint64_t gh_ipaddr;				/*   8   20 */
+		/* from gethostid(2), room for expansion */
+	uuid_t gh_dumpid;				/*  10   30 */
+		/* ID of dump session	 */
+	char gh_pad2[0xd0];				/*  d0  100 */
+		/* alignment */
+	char gh_hostname[GLOBAL_HDR_STRING_SZ];	/* 100  200 */
+		/* from gethostname(2) */
+	char gh_dumplabel[GLOBAL_HDR_STRING_SZ];	/* 100  300 */
+		/* label of dump session */
+	char gh_pad3[0x100];				/* 100  400 */
+		/* padding */
+	char gh_upper[GLOBAL_HDR_SZ - 0x400];		/* c00 1000 */
+		/* header info private to upper software layers */
+};
+typedef struct global_hdr global_hdr_t;
+
+#define sizeofmember( t, m )	sizeof( ( ( t * )0 )->m )
+
+#define DRIVE_HDR_SZ		sizeofmember(global_hdr_t, gh_upper)
+struct drive_hdr {
+	uint32_t dh_drivecnt;				/*   4    4 */
+		/* number of drives used to dump the fs */
+	uint32_t dh_driveix;				/*   4    8 */
+		/* 0-based index of the drive used to dump this stream */
+	int32_t dh_strategyid;				/*   4    c */
+		/* ID of the drive strategy used to produce this dump */
+	char dh_pad1[0x1f4];				/* 1f4  200 */
+		/* padding */
+	char dh_specific[0x200];			/* 200  400 */
+		/* drive strategy-specific info */
+	char dh_upper[DRIVE_HDR_SZ - 0x400];		/* 800  c00 */
+		/* header info private to upper software layers */
+};
+typedef struct drive_hdr drive_hdr_t;
+
+#define MEDIA_HDR_SZ		sizeofmember(drive_hdr_t, dh_upper)
+struct media_hdr {
+	char mh_medialabel[GLOBAL_HDR_STRING_SZ];	/* 100  100 */
+		/* label of media object containing file */
+	char mh_prevmedialabel[GLOBAL_HDR_STRING_SZ];	/* 100  200 */
+		/* label of upstream media object */
+	char mh_pad1[GLOBAL_HDR_STRING_SZ];		/* 100  300 */
+		/* in case more labels needed */
+	uuid_t mh_mediaid;				/*  10  310 */
+		/* ID of media object 	*/
+	uuid_t mh_prevmediaid;				/*  10  320 */
+		/* ID of upstream media object */
+	char mh_pad2[GLOBAL_HDR_UUID_SZ];		/*  10  330 */
+		/* in case more IDs needed */
+	uint32_t mh_mediaix;				/*   4  334 */
+		/* 0-based index of this media object within the dump stream */
+	uint32_t mh_mediafileix;			/*   4  338 */
+		/* 0-based index of this file within this media object */
+	uint32_t mh_dumpfileix;			/*   4  33c */
+		/* 0-based index of this file within this dump stream */
+	uint32_t mh_dumpmediafileix;			/*   4  340 */
+		/* 0-based index of file within dump stream and media object */
+	uint32_t mh_dumpmediaix;			/*   4  344 */
+		/* 0-based index of this dump within the media object */
+	int32_t mh_strategyid;				/*   4  348 */
+		/* ID of the media strategy used to produce this dump */
+	char mh_pad3[0x38];				/*  38  380 */
+		/* padding */
+	char mh_specific[0x80];			/*  80  400 */
+		/* media strategy-specific info */
+	char mh_upper[MEDIA_HDR_SZ - 0x400];		/* 400  800 */
+		/* header info private to upper software layers */
+};
+typedef struct media_hdr media_hdr_t;
+
+#define CONTENT_HDR_SZ		sizeofmember(media_hdr_t, mh_upper)
+#define CONTENT_HDR_FSTYPE_SZ	16
+#define CONTENT_STATSZ		160 /* must match dlog.h DLOG_MULTI_STATSZ */
+struct content_hdr {
+	char ch_mntpnt[GLOBAL_HDR_STRING_SZ];		/* 100  100 */
+		/* full pathname of fs mount point */
+	char ch_fsdevice[GLOBAL_HDR_STRING_SZ];	/* 100  200 */
+		/* full pathname of char device containing fs */
+	char  ch_pad1[GLOBAL_HDR_STRING_SZ];		/* 100  300 */
+		/* in case another label is needed */
+	char ch_fstype[CONTENT_HDR_FSTYPE_SZ];	/*  10  310 */
+		/* from fsid.h */
+	uuid_t ch_fsid;					/*  10  320 */
+		/* file system uuid */
+	char  ch_pad2[GLOBAL_HDR_UUID_SZ];		/*  10  330 */
+		/* in case another id is needed */
+	char ch_pad3[8];				/*   8  338 */
+		/* padding */
+	int32_t ch_strategyid;				/*   4  33c */
+		/* ID of the content strategy used to produce this dump */
+	char ch_pad4[4];				/*   4  340 */
+		/* alignment */
+	char ch_specific[0xc0];			/*  c0  400 */
+		/* content strategy-specific info */
+};
+typedef struct content_hdr content_hdr_t;
+
+typedef uint64_t xfs_ino_t;
+struct startpt {
+	xfs_ino_t sp_ino;		/* first inode to dump */
+	off64_t sp_offset;	/* bytes to skip in file data fork */
+	int32_t sp_flags;
+	int32_t sp_pad1;
+};
+typedef struct startpt startpt_t;
+
+#define CONTENT_INODE_HDR_SZ  sizeofmember(content_hdr_t, ch_specific)
+struct content_inode_hdr {
+	int32_t cih_mediafiletype;			/*   4   4 */
+		/* dump media file type: see #defines below */
+	int32_t cih_dumpattr;				/*   4   8 */
+		/* dump attributes: see #defines below */
+	int32_t cih_level;				/*   4   c */
+		/* dump level */
+	char pad1[4];					/*   4  10 */
+		/* alignment */
+	time32_t cih_last_time;				/*   4  14 */
+		/* if an incremental, time of previous dump at a lesser level */
+	time32_t cih_resume_time;			/*   4  18 */
+		/* if a resumed dump, time of interrupted dump */
+	xfs_ino_t cih_rootino;				/*   8  20 */
+		/* root inode number */
+	uuid_t cih_last_id;				/*  10  30 */
+		/* if an incremental, uuid of prev dump */
+	uuid_t cih_resume_id;				/*  10  40 */
+		/* if a resumed dump, uuid of interrupted dump */
+	startpt_t cih_startpt;				/*  18  58 */
+		/* starting point of media file contents */
+	startpt_t cih_endpt;				/*  18  70 */
+		/* starting point of next stream */
+	uint64_t cih_inomap_hnkcnt;			/*   8  78 */
+
+	uint64_t cih_inomap_segcnt;			/*   8  80 */
+
+	uint64_t cih_inomap_dircnt;			/*   8  88 */
+
+	uint64_t cih_inomap_nondircnt;			/*   8  90 */
+
+	xfs_ino_t cih_inomap_firstino;			/*   8  98 */
+
+	xfs_ino_t cih_inomap_lastino;			/*   8  a0 */
+
+	uint64_t cih_inomap_datasz;			/*   8  a8 */
+		/* bytes of non-metadata dumped */
+	char cih_pad2[CONTENT_INODE_HDR_SZ - 0xa8];	/*  18  c0 */
+		/* padding */
+};
+typedef struct content_inode_hdr content_inode_hdr_t;
+// End of definitions from xfsdump
+
+int main(int argc, char *argv[]) {
+
+	if (argc < 3) {
+		fprintf(stderr, "Usage: %s <path/to/dumpfile> <fake rootino>\n", argv[0]);
+		exit(1);
+	}
+
+	const char *filepath = argv[1];
+	const uint64_t fake_root_ino = (uint64_t) strtol(argv[2], NULL, 10);
+
+	int fd = open(filepath, O_RDWR);
+	if (fd < 0) {
+		perror("open");
+		exit(1);
+	}
+	global_hdr_t *header = mmap(NULL, GLOBAL_HDR_SZ, PROT_READ|PROT_WRITE, MAP_SHARED, fd, 0);
+	if (header == MAP_FAILED) {
+		perror("mmap");
+		exit(1);
+	}
+
+	uint64_t *rootino_ptr =
+		&((content_inode_hdr_t *)
+			((content_hdr_t *)
+				((media_hdr_t *)
+					((drive_hdr_t *)
+						header->gh_upper
+					)->dh_upper
+				)->mh_upper
+			)->ch_specific
+		)->cih_rootino;
+	int32_t checksum = (int32_t) be32toh(header->gh_checksum);
+	uint64_t orig_rootino = be64toh(*rootino_ptr);
+
+	// Fake root inode number
+	*rootino_ptr = htobe64(fake_root_ino);
+
+	// Update checksum along with overwriting rootino.
+	uint64_t gap = orig_rootino - fake_root_ino;
+	checksum += (gap >> 32) + (gap & 0x00000000ffffffff);
+	header->gh_checksum = htobe32(checksum);
+
+	munmap(header, GLOBAL_HDR_SZ);
+	close(fd);
+}
diff --git a/tests/xfs/554 b/tests/xfs/554
new file mode 100755
index 00000000..fcfaa699
--- /dev/null
+++ b/tests/xfs/554
@@ -0,0 +1,73 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2022 Fujitsu Limited. All Rights Reserved.
+#
+# FS QA Test No. 554
+#
+# Create a filesystem which contains an inode with a lower number
+# than the root inode. Set the lower number to a dump file as the root inode
+# and ensure that 'xfsrestore -x' handles this wrong inode.
+#
+. ./common/preamble
+_begin_fstest auto quick dump
+
+# Import common functions.
+. ./common/dump
+
+_supported_fs xfs
+_require_xfs_io_command "falloc"
+_require_scratch
+_require_xfsrestore_xflag
+
+# A large stripe unit will put the root inode out quite far
+# due to alignment, leaving free blocks ahead of it.
+_scratch_mkfs_xfs -d sunit=1024,swidth=1024 > $seqres.full 2>&1
+
+# Mounting /without/ a stripe should allow inodes to be allocated
+# in lower free blocks, without the stripe alignment.
+_scratch_mount -o sunit=0,swidth=0
+
+root_inum=$(stat -c %i $SCRATCH_MNT)
+
+# Consume space after the root inode so that the blocks before
+# root look "close" for the next inode chunk allocation
+$XFS_IO_PROG -f -c "falloc 0 16m" $SCRATCH_MNT/fillfile
+
+# And make a bunch of inodes until we (hopefully) get one lower
+# than root, in a new inode chunk.
+echo "root_inum: $root_inum" >> $seqres.full
+for i in $(seq 0 4096) ; do
+	fname=$SCRATCH_MNT/$(printf "FILE_%03d" $i)
+	touch $fname
+	inum=$(stat -c "%i" $fname)
+	[[ $inum -lt $root_inum ]] && break
+done
+
+echo "created: $inum" >> $seqres.full
+
+[[ $inum -lt $root_inum ]] || _notrun "Could not set up test"
+
+# Now try a dump and restore. Cribbed from xfs/068
+_create_dumpdir_stress
+
+echo -n "Before: " >> $seqres.full
+_count_dumpdir_files | tee $tmp.before >> $seqres.full
+
+_do_dump_file
+
+# Set the wrong root inode number to the dump file
+# as problematic xfsdump used to do.
+$here/src/fake-dump-rootino $dump_file $inum
+
+_do_restore_file -x | \
+sed -e "s/rootino #${inum}/rootino #FAKENO/g" \
+	-e "s/# to ${root_inum}/# to ROOTNO/g" \
+	-e "/entries processed$/s/[0-9][0-9]*/NUM/g"
+
+echo -n "After: " >> $seqres.full
+_count_restoredir_files | tee $tmp.after >> $seqres.full
+diff -u $tmp.before $tmp.after
+
+# success, all done
+status=0
+exit
diff --git a/tests/xfs/554.out b/tests/xfs/554.out
new file mode 100644
index 00000000..c5e8c4c5
--- /dev/null
+++ b/tests/xfs/554.out
@@ -0,0 +1,40 @@
+QA output created by 554
+Creating directory system to dump using fsstress.
+
+-----------------------------------------------
+fsstress : -f link=10 -f creat=10 -f mkdir=10 -f truncate=5 -f symlink=10
+-----------------------------------------------
+Dumping to file...
+xfsdump  -f DUMP_FILE -M stress_tape_media -L stress_554 SCRATCH_MNT
+xfsdump: using file dump (drive_simple) strategy
+xfsdump: level 0 dump of HOSTNAME:SCRATCH_MNT
+xfsdump: dump date: DATE
+xfsdump: session id: ID
+xfsdump: session label: "stress_554"
+xfsdump: ino map <PHASES>
+xfsdump: ino map construction complete
+xfsdump: estimated dump size: NUM bytes
+xfsdump: /var/xfsdump/inventory created
+xfsdump: creating dump session media file 0 (media 0, file 0)
+xfsdump: dumping ino map
+xfsdump: dumping directories
+xfsdump: dumping non-directory files
+xfsdump: ending media file
+xfsdump: media file size NUM bytes
+xfsdump: dump size (non-dir files) : NUM bytes
+xfsdump: dump complete: SECS seconds elapsed
+xfsdump: Dump Status: SUCCESS
+Restoring from file...
+xfsrestore  -x -f DUMP_FILE  -L stress_554 RESTORE_DIR
+xfsrestore: using file dump (drive_simple) strategy
+xfsrestore: using online session inventory
+xfsrestore: searching media for directory dump
+xfsrestore: examining media file 0
+xfsrestore: reading directories
+xfsrestore: found fake rootino #FAKENO, will fix.
+xfsrestore: fix root # to ROOTNO (bind mount?)
+xfsrestore: NUM directories and NUM entries processed
+xfsrestore: directory post-processing
+xfsrestore: restoring non-directory files
+xfsrestore: restore complete: SECS seconds elapsed
+xfsrestore: Restore Status: SUCCESS
-- 
2.37.3

Re: [PATCH V3] xfs: test for fixing wrong root inode number in dump

[Darrick J. Wong]

On Fri, Oct 14, 2022 at 12:09:07PM -0400, Hironori Shiina wrote:
> Test '-x' option of xfsrestore. With this option, a wrong root inode
> number in a dump file is corrected. A root inode number can be wrong
> in a dump created by problematic xfsdump (v3.1.7 - v3.1.9) with
> bulkstat misuse. In this test, a corrupted dump file is created by
> overwriting a root inode number in a header.
> 
> Signed-off-by: Hironori Shiina <shiina.hironori@fujitsu.com>
> ---
> changes since v2:
>   - Use glibc functions to convert endian.
>   - Copy the structure definitions from xfsdump source files.
> changes since RFC v1:
>   - Skip the test if xfsrestore does not support '-x' flag.
>   - Create a corrupted dump by overwriting a root inode number in a dump
>     file with a new tool instead of checking in a binary dump file.
> 
>  common/dump             |   2 +-
>  common/xfs              |   6 ++
>  src/Makefile            |   2 +-
>  src/fake-dump-rootino.c | 228 ++++++++++++++++++++++++++++++++++++++++
>  tests/xfs/554           |  73 +++++++++++++
>  tests/xfs/554.out       |  40 +++++++
>  6 files changed, 349 insertions(+), 2 deletions(-)
>  create mode 100644 src/fake-dump-rootino.c
>  create mode 100755 tests/xfs/554
>  create mode 100644 tests/xfs/554.out
> 
> diff --git a/common/dump b/common/dump
> index 8e0446d9..50b2ba03 100644
> --- a/common/dump
> +++ b/common/dump
> @@ -1003,7 +1003,7 @@ _parse_restore_args()
>          --no-check-quota)
>              do_quota_check=false
>              ;;
> -	-K|-R)
> +	-K|-R|-x)
>  	    restore_args="$restore_args $1"
>              ;;
>  	*)
> diff --git a/common/xfs b/common/xfs
> index e1c15d3d..8334880e 100644
> --- a/common/xfs
> +++ b/common/xfs
> @@ -1402,3 +1402,9 @@ _xfs_filter_mkfs()
>  		print STDOUT "realtime =RDEV extsz=XXX blocks=XXX, rtextents=XXX\n";
>  	}'
>  }
> +
> +_require_xfsrestore_xflag()
> +{
> +	$XFSRESTORE_PROG -h 2>&1 | grep -q -e '-x' || \
> +			_notrun 'xfsrestore does not support -x flag.'
> +}
> diff --git a/src/Makefile b/src/Makefile
> index 5f565e73..afdf6b30 100644
> --- a/src/Makefile
> +++ b/src/Makefile
> @@ -19,7 +19,7 @@ TARGETS = dirstress fill fill2 getpagesize holes lstat64 \
>  	t_ofd_locks t_mmap_collision mmap-write-concurrent \
>  	t_get_file_time t_create_short_dirs t_create_long_dirs t_enospc \
>  	t_mmap_writev_overlap checkpoint_journal mmap-rw-fault allocstale \
> -	t_mmap_cow_memory_failure
> +	t_mmap_cow_memory_failure fake-dump-rootino
>  
>  LINUX_TARGETS = xfsctl bstat t_mtab getdevicesize preallo_rw_pattern_reader \
>  	preallo_rw_pattern_writer ftrunc trunc fs_perms testx looptest \
> diff --git a/src/fake-dump-rootino.c b/src/fake-dump-rootino.c
> new file mode 100644
> index 00000000..80797f15
> --- /dev/null
> +++ b/src/fake-dump-rootino.c
> @@ -0,0 +1,228 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2022 Fujitsu Limited.  All Rights Reserved. */
> +#define _LARGEFILE64_SOURCE
> +#include <endian.h>
> +#include <fcntl.h>
> +#include <stdint.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <sys/mman.h>
> +#include <unistd.h>
> +#include <uuid/uuid.h>
> +
> +// Definitions from xfsdump
> +#define PGSZLOG2	12
> +#define PGSZ		(1 << PGSZLOG2)
> +#define GLOBAL_HDR_SZ		PGSZ
> +#define GLOBAL_HDR_MAGIC_SZ	8
> +#define GLOBAL_HDR_STRING_SZ	0x100
> +#define GLOBAL_HDR_TIME_SZ	4
> +#define GLOBAL_HDR_UUID_SZ	0x10
> +typedef int32_t time32_t;
> +struct global_hdr {
> +	char gh_magic[GLOBAL_HDR_MAGIC_SZ];		/*   8    8 */
> +		/* unique signature of xfsdump */
> +	uint32_t gh_version;				/*   4    c */
> +		/* header version */
> +	uint32_t gh_checksum;				/*   4   10 */
> +		/* 32-bit unsigned additive inverse of entire header */
> +	time32_t gh_timestamp;				/*   4   14 */
> +		/* time32_t of dump */
> +	char gh_pad1[4];				/*   4   18 */
> +		/* alignment */
> +	uint64_t gh_ipaddr;				/*   8   20 */
> +		/* from gethostid(2), room for expansion */
> +	uuid_t gh_dumpid;				/*  10   30 */
> +		/* ID of dump session	 */
> +	char gh_pad2[0xd0];				/*  d0  100 */
> +		/* alignment */
> +	char gh_hostname[GLOBAL_HDR_STRING_SZ];	/* 100  200 */
> +		/* from gethostname(2) */
> +	char gh_dumplabel[GLOBAL_HDR_STRING_SZ];	/* 100  300 */
> +		/* label of dump session */
> +	char gh_pad3[0x100];				/* 100  400 */
> +		/* padding */
> +	char gh_upper[GLOBAL_HDR_SZ - 0x400];		/* c00 1000 */
> +		/* header info private to upper software layers */
> +};
> +typedef struct global_hdr global_hdr_t;
> +
> +#define sizeofmember( t, m )	sizeof( ( ( t * )0 )->m )
> +
> +#define DRIVE_HDR_SZ		sizeofmember(global_hdr_t, gh_upper)
> +struct drive_hdr {
> +	uint32_t dh_drivecnt;				/*   4    4 */
> +		/* number of drives used to dump the fs */
> +	uint32_t dh_driveix;				/*   4    8 */
> +		/* 0-based index of the drive used to dump this stream */
> +	int32_t dh_strategyid;				/*   4    c */
> +		/* ID of the drive strategy used to produce this dump */
> +	char dh_pad1[0x1f4];				/* 1f4  200 */
> +		/* padding */
> +	char dh_specific[0x200];			/* 200  400 */
> +		/* drive strategy-specific info */
> +	char dh_upper[DRIVE_HDR_SZ - 0x400];		/* 800  c00 */
> +		/* header info private to upper software layers */
> +};
> +typedef struct drive_hdr drive_hdr_t;
> +
> +#define MEDIA_HDR_SZ		sizeofmember(drive_hdr_t, dh_upper)
> +struct media_hdr {
> +	char mh_medialabel[GLOBAL_HDR_STRING_SZ];	/* 100  100 */
> +		/* label of media object containing file */
> +	char mh_prevmedialabel[GLOBAL_HDR_STRING_SZ];	/* 100  200 */
> +		/* label of upstream media object */
> +	char mh_pad1[GLOBAL_HDR_STRING_SZ];		/* 100  300 */
> +		/* in case more labels needed */
> +	uuid_t mh_mediaid;				/*  10  310 */
> +		/* ID of media object 	*/
> +	uuid_t mh_prevmediaid;				/*  10  320 */
> +		/* ID of upstream media object */
> +	char mh_pad2[GLOBAL_HDR_UUID_SZ];		/*  10  330 */
> +		/* in case more IDs needed */
> +	uint32_t mh_mediaix;				/*   4  334 */
> +		/* 0-based index of this media object within the dump stream */
> +	uint32_t mh_mediafileix;			/*   4  338 */
> +		/* 0-based index of this file within this media object */
> +	uint32_t mh_dumpfileix;			/*   4  33c */
> +		/* 0-based index of this file within this dump stream */
> +	uint32_t mh_dumpmediafileix;			/*   4  340 */
> +		/* 0-based index of file within dump stream and media object */
> +	uint32_t mh_dumpmediaix;			/*   4  344 */
> +		/* 0-based index of this dump within the media object */
> +	int32_t mh_strategyid;				/*   4  348 */
> +		/* ID of the media strategy used to produce this dump */
> +	char mh_pad3[0x38];				/*  38  380 */
> +		/* padding */
> +	char mh_specific[0x80];			/*  80  400 */
> +		/* media strategy-specific info */
> +	char mh_upper[MEDIA_HDR_SZ - 0x400];		/* 400  800 */
> +		/* header info private to upper software layers */
> +};
> +typedef struct media_hdr media_hdr_t;
> +
> +#define CONTENT_HDR_SZ		sizeofmember(media_hdr_t, mh_upper)
> +#define CONTENT_HDR_FSTYPE_SZ	16
> +#define CONTENT_STATSZ		160 /* must match dlog.h DLOG_MULTI_STATSZ */
> +struct content_hdr {
> +	char ch_mntpnt[GLOBAL_HDR_STRING_SZ];		/* 100  100 */
> +		/* full pathname of fs mount point */
> +	char ch_fsdevice[GLOBAL_HDR_STRING_SZ];	/* 100  200 */
> +		/* full pathname of char device containing fs */
> +	char  ch_pad1[GLOBAL_HDR_STRING_SZ];		/* 100  300 */
> +		/* in case another label is needed */
> +	char ch_fstype[CONTENT_HDR_FSTYPE_SZ];	/*  10  310 */
> +		/* from fsid.h */
> +	uuid_t ch_fsid;					/*  10  320 */
> +		/* file system uuid */
> +	char  ch_pad2[GLOBAL_HDR_UUID_SZ];		/*  10  330 */
> +		/* in case another id is needed */
> +	char ch_pad3[8];				/*   8  338 */
> +		/* padding */
> +	int32_t ch_strategyid;				/*   4  33c */
> +		/* ID of the content strategy used to produce this dump */
> +	char ch_pad4[4];				/*   4  340 */
> +		/* alignment */
> +	char ch_specific[0xc0];			/*  c0  400 */
> +		/* content strategy-specific info */
> +};
> +typedef struct content_hdr content_hdr_t;
> +
> +typedef uint64_t xfs_ino_t;
> +struct startpt {
> +	xfs_ino_t sp_ino;		/* first inode to dump */
> +	off64_t sp_offset;	/* bytes to skip in file data fork */
> +	int32_t sp_flags;
> +	int32_t sp_pad1;
> +};
> +typedef struct startpt startpt_t;
> +
> +#define CONTENT_INODE_HDR_SZ  sizeofmember(content_hdr_t, ch_specific)
> +struct content_inode_hdr {
> +	int32_t cih_mediafiletype;			/*   4   4 */
> +		/* dump media file type: see #defines below */
> +	int32_t cih_dumpattr;				/*   4   8 */
> +		/* dump attributes: see #defines below */
> +	int32_t cih_level;				/*   4   c */
> +		/* dump level */
> +	char pad1[4];					/*   4  10 */
> +		/* alignment */
> +	time32_t cih_last_time;				/*   4  14 */
> +		/* if an incremental, time of previous dump at a lesser level */
> +	time32_t cih_resume_time;			/*   4  18 */
> +		/* if a resumed dump, time of interrupted dump */
> +	xfs_ino_t cih_rootino;				/*   8  20 */
> +		/* root inode number */
> +	uuid_t cih_last_id;				/*  10  30 */
> +		/* if an incremental, uuid of prev dump */
> +	uuid_t cih_resume_id;				/*  10  40 */
> +		/* if a resumed dump, uuid of interrupted dump */
> +	startpt_t cih_startpt;				/*  18  58 */
> +		/* starting point of media file contents */
> +	startpt_t cih_endpt;				/*  18  70 */
> +		/* starting point of next stream */
> +	uint64_t cih_inomap_hnkcnt;			/*   8  78 */
> +
> +	uint64_t cih_inomap_segcnt;			/*   8  80 */
> +
> +	uint64_t cih_inomap_dircnt;			/*   8  88 */
> +
> +	uint64_t cih_inomap_nondircnt;			/*   8  90 */
> +
> +	xfs_ino_t cih_inomap_firstino;			/*   8  98 */
> +
> +	xfs_ino_t cih_inomap_lastino;			/*   8  a0 */
> +
> +	uint64_t cih_inomap_datasz;			/*   8  a8 */
> +		/* bytes of non-metadata dumped */
> +	char cih_pad2[CONTENT_INODE_HDR_SZ - 0xa8];	/*  18  c0 */
> +		/* padding */
> +};
> +typedef struct content_inode_hdr content_inode_hdr_t;
> +// End of definitions from xfsdump
> +
> +int main(int argc, char *argv[]) {
> +
> +	if (argc < 3) {
> +		fprintf(stderr, "Usage: %s <path/to/dumpfile> <fake rootino>\n", argv[0]);
> +		exit(1);
> +	}
> +
> +	const char *filepath = argv[1];
> +	const uint64_t fake_root_ino = (uint64_t) strtol(argv[2], NULL, 10);
> +
> +	int fd = open(filepath, O_RDWR);
> +	if (fd < 0) {
> +		perror("open");
> +		exit(1);
> +	}
> +	global_hdr_t *header = mmap(NULL, GLOBAL_HDR_SZ, PROT_READ|PROT_WRITE, MAP_SHARED, fd, 0);
> +	if (header == MAP_FAILED) {
> +		perror("mmap");
> +		exit(1);
> +	}
> +
> +	uint64_t *rootino_ptr =
> +		&((content_inode_hdr_t *)
> +			((content_hdr_t *)
> +				((media_hdr_t *)
> +					((drive_hdr_t *)
> +						header->gh_upper
> +					)->dh_upper
> +				)->mh_upper
> +			)->ch_specific
> +		)->cih_rootino;

Urrrrrk nested pointer dereferencing.

Oh wow, I didn't realize gh_upper is an opencoded char[].

If you flatten out all that nested pointer dereferencing and
typecasting, is this what you get?

	drive_hdr_t	*dh = (drive_hdr_t *)header->gh_upper;
	media_hdr_t	*mh = (media_hdr_t *)dh->dh_upper;
	content_hdr_t	*ch = (content_hdr_t *)mh->mh_upper;
	content_inode_hdr_t *cih = (content_inode_hdr_t *)ch->ch_specific;
	uint64_t	*rootino_ptr = &cih->cih_rootino;

If so, please change the code to do the above so that nobody else has to
go through that nest of pointer casting.

With that fixed,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D


> +	int32_t checksum = (int32_t) be32toh(header->gh_checksum);
> +	uint64_t orig_rootino = be64toh(*rootino_ptr);
> +
> +	// Fake root inode number
> +	*rootino_ptr = htobe64(fake_root_ino);
> +
> +	// Update checksum along with overwriting rootino.
> +	uint64_t gap = orig_rootino - fake_root_ino;
> +	checksum += (gap >> 32) + (gap & 0x00000000ffffffff);
> +	header->gh_checksum = htobe32(checksum);
> +
> +	munmap(header, GLOBAL_HDR_SZ);
> +	close(fd);
> +}
> diff --git a/tests/xfs/554 b/tests/xfs/554
> new file mode 100755
> index 00000000..fcfaa699
> --- /dev/null
> +++ b/tests/xfs/554
> @@ -0,0 +1,73 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2022 Fujitsu Limited. All Rights Reserved.
> +#
> +# FS QA Test No. 554
> +#
> +# Create a filesystem which contains an inode with a lower number
> +# than the root inode. Set the lower number to a dump file as the root inode
> +# and ensure that 'xfsrestore -x' handles this wrong inode.
> +#
> +. ./common/preamble
> +_begin_fstest auto quick dump
> +
> +# Import common functions.
> +. ./common/dump
> +
> +_supported_fs xfs
> +_require_xfs_io_command "falloc"
> +_require_scratch
> +_require_xfsrestore_xflag
> +
> +# A large stripe unit will put the root inode out quite far
> +# due to alignment, leaving free blocks ahead of it.
> +_scratch_mkfs_xfs -d sunit=1024,swidth=1024 > $seqres.full 2>&1
> +
> +# Mounting /without/ a stripe should allow inodes to be allocated
> +# in lower free blocks, without the stripe alignment.
> +_scratch_mount -o sunit=0,swidth=0
> +
> +root_inum=$(stat -c %i $SCRATCH_MNT)
> +
> +# Consume space after the root inode so that the blocks before
> +# root look "close" for the next inode chunk allocation
> +$XFS_IO_PROG -f -c "falloc 0 16m" $SCRATCH_MNT/fillfile
> +
> +# And make a bunch of inodes until we (hopefully) get one lower
> +# than root, in a new inode chunk.
> +echo "root_inum: $root_inum" >> $seqres.full
> +for i in $(seq 0 4096) ; do
> +	fname=$SCRATCH_MNT/$(printf "FILE_%03d" $i)
> +	touch $fname
> +	inum=$(stat -c "%i" $fname)
> +	[[ $inum -lt $root_inum ]] && break
> +done
> +
> +echo "created: $inum" >> $seqres.full
> +
> +[[ $inum -lt $root_inum ]] || _notrun "Could not set up test"
> +
> +# Now try a dump and restore. Cribbed from xfs/068
> +_create_dumpdir_stress
> +
> +echo -n "Before: " >> $seqres.full
> +_count_dumpdir_files | tee $tmp.before >> $seqres.full
> +
> +_do_dump_file
> +
> +# Set the wrong root inode number to the dump file
> +# as problematic xfsdump used to do.
> +$here/src/fake-dump-rootino $dump_file $inum
> +
> +_do_restore_file -x | \
> +sed -e "s/rootino #${inum}/rootino #FAKENO/g" \
> +	-e "s/# to ${root_inum}/# to ROOTNO/g" \
> +	-e "/entries processed$/s/[0-9][0-9]*/NUM/g"
> +
> +echo -n "After: " >> $seqres.full
> +_count_restoredir_files | tee $tmp.after >> $seqres.full
> +diff -u $tmp.before $tmp.after
> +
> +# success, all done
> +status=0
> +exit
> diff --git a/tests/xfs/554.out b/tests/xfs/554.out
> new file mode 100644
> index 00000000..c5e8c4c5
> --- /dev/null
> +++ b/tests/xfs/554.out
> @@ -0,0 +1,40 @@
> +QA output created by 554
> +Creating directory system to dump using fsstress.
> +
> +-----------------------------------------------
> +fsstress : -f link=10 -f creat=10 -f mkdir=10 -f truncate=5 -f symlink=10
> +-----------------------------------------------
> +Dumping to file...
> +xfsdump  -f DUMP_FILE -M stress_tape_media -L stress_554 SCRATCH_MNT
> +xfsdump: using file dump (drive_simple) strategy
> +xfsdump: level 0 dump of HOSTNAME:SCRATCH_MNT
> +xfsdump: dump date: DATE
> +xfsdump: session id: ID
> +xfsdump: session label: "stress_554"
> +xfsdump: ino map <PHASES>
> +xfsdump: ino map construction complete
> +xfsdump: estimated dump size: NUM bytes
> +xfsdump: /var/xfsdump/inventory created
> +xfsdump: creating dump session media file 0 (media 0, file 0)
> +xfsdump: dumping ino map
> +xfsdump: dumping directories
> +xfsdump: dumping non-directory files
> +xfsdump: ending media file
> +xfsdump: media file size NUM bytes
> +xfsdump: dump size (non-dir files) : NUM bytes
> +xfsdump: dump complete: SECS seconds elapsed
> +xfsdump: Dump Status: SUCCESS
> +Restoring from file...
> +xfsrestore  -x -f DUMP_FILE  -L stress_554 RESTORE_DIR
> +xfsrestore: using file dump (drive_simple) strategy
> +xfsrestore: using online session inventory
> +xfsrestore: searching media for directory dump
> +xfsrestore: examining media file 0
> +xfsrestore: reading directories
> +xfsrestore: found fake rootino #FAKENO, will fix.
> +xfsrestore: fix root # to ROOTNO (bind mount?)
> +xfsrestore: NUM directories and NUM entries processed
> +xfsrestore: directory post-processing
> +xfsrestore: restoring non-directory files
> +xfsrestore: restore complete: SECS seconds elapsed
> +xfsrestore: Restore Status: SUCCESS
> -- 
> 2.37.3
> 

Re: [PATCH V3] xfs: test for fixing wrong root inode number in dump

[Hironori Shiina]

On 10/14/22 14:28, Darrick J. Wong wrote:
> On Fri, Oct 14, 2022 at 12:09:07PM -0400, Hironori Shiina wrote:
>> Test '-x' option of xfsrestore. With this option, a wrong root inode
>> number in a dump file is corrected. A root inode number can be wrong
>> in a dump created by problematic xfsdump (v3.1.7 - v3.1.9) with
>> bulkstat misuse. In this test, a corrupted dump file is created by
>> overwriting a root inode number in a header.
>>
>> Signed-off-by: Hironori Shiina <shiina.hironori@fujitsu.com>
>> ---
>> changes since v2:
>>   - Use glibc functions to convert endian.
>>   - Copy the structure definitions from xfsdump source files.
>> changes since RFC v1:
>>   - Skip the test if xfsrestore does not support '-x' flag.
>>   - Create a corrupted dump by overwriting a root inode number in a dump
>>     file with a new tool instead of checking in a binary dump file.
>>
>>  common/dump             |   2 +-
>>  common/xfs              |   6 ++
>>  src/Makefile            |   2 +-
>>  src/fake-dump-rootino.c | 228 ++++++++++++++++++++++++++++++++++++++++
>>  tests/xfs/554           |  73 +++++++++++++
>>  tests/xfs/554.out       |  40 +++++++
>>  6 files changed, 349 insertions(+), 2 deletions(-)
>>  create mode 100644 src/fake-dump-rootino.c
>>  create mode 100755 tests/xfs/554
>>  create mode 100644 tests/xfs/554.out
>>
>> diff --git a/common/dump b/common/dump
>> index 8e0446d9..50b2ba03 100644
>> --- a/common/dump
>> +++ b/common/dump
>> @@ -1003,7 +1003,7 @@ _parse_restore_args()
>>          --no-check-quota)
>>              do_quota_check=false
>>              ;;
>> -	-K|-R)
>> +	-K|-R|-x)
>>  	    restore_args="$restore_args $1"
>>              ;;
>>  	*)
>> diff --git a/common/xfs b/common/xfs
>> index e1c15d3d..8334880e 100644
>> --- a/common/xfs
>> +++ b/common/xfs
>> @@ -1402,3 +1402,9 @@ _xfs_filter_mkfs()
>>  		print STDOUT "realtime =RDEV extsz=XXX blocks=XXX, rtextents=XXX\n";
>>  	}'
>>  }
>> +
>> +_require_xfsrestore_xflag()
>> +{
>> +	$XFSRESTORE_PROG -h 2>&1 | grep -q -e '-x' || \
>> +			_notrun 'xfsrestore does not support -x flag.'
>> +}
>> diff --git a/src/Makefile b/src/Makefile
>> index 5f565e73..afdf6b30 100644
>> --- a/src/Makefile
>> +++ b/src/Makefile
>> @@ -19,7 +19,7 @@ TARGETS = dirstress fill fill2 getpagesize holes lstat64 \
>>  	t_ofd_locks t_mmap_collision mmap-write-concurrent \
>>  	t_get_file_time t_create_short_dirs t_create_long_dirs t_enospc \
>>  	t_mmap_writev_overlap checkpoint_journal mmap-rw-fault allocstale \
>> -	t_mmap_cow_memory_failure
>> +	t_mmap_cow_memory_failure fake-dump-rootino
>>  
>>  LINUX_TARGETS = xfsctl bstat t_mtab getdevicesize preallo_rw_pattern_reader \
>>  	preallo_rw_pattern_writer ftrunc trunc fs_perms testx looptest \
>> diff --git a/src/fake-dump-rootino.c b/src/fake-dump-rootino.c
>> new file mode 100644
>> index 00000000..80797f15
>> --- /dev/null
>> +++ b/src/fake-dump-rootino.c
>> @@ -0,0 +1,228 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/* Copyright (c) 2022 Fujitsu Limited.  All Rights Reserved. */
>> +#define _LARGEFILE64_SOURCE
>> +#include <endian.h>
>> +#include <fcntl.h>
>> +#include <stdint.h>
>> +#include <stdio.h>
>> +#include <stdlib.h>
>> +#include <sys/mman.h>
>> +#include <unistd.h>
>> +#include <uuid/uuid.h>
>> +
>> +// Definitions from xfsdump
>> +#define PGSZLOG2	12
>> +#define PGSZ		(1 << PGSZLOG2)
>> +#define GLOBAL_HDR_SZ		PGSZ
>> +#define GLOBAL_HDR_MAGIC_SZ	8
>> +#define GLOBAL_HDR_STRING_SZ	0x100
>> +#define GLOBAL_HDR_TIME_SZ	4
>> +#define GLOBAL_HDR_UUID_SZ	0x10
>> +typedef int32_t time32_t;
>> +struct global_hdr {
>> +	char gh_magic[GLOBAL_HDR_MAGIC_SZ];		/*   8    8 */
>> +		/* unique signature of xfsdump */
>> +	uint32_t gh_version;				/*   4    c */
>> +		/* header version */
>> +	uint32_t gh_checksum;				/*   4   10 */
>> +		/* 32-bit unsigned additive inverse of entire header */
>> +	time32_t gh_timestamp;				/*   4   14 */
>> +		/* time32_t of dump */
>> +	char gh_pad1[4];				/*   4   18 */
>> +		/* alignment */
>> +	uint64_t gh_ipaddr;				/*   8   20 */
>> +		/* from gethostid(2), room for expansion */
>> +	uuid_t gh_dumpid;				/*  10   30 */
>> +		/* ID of dump session	 */
>> +	char gh_pad2[0xd0];				/*  d0  100 */
>> +		/* alignment */
>> +	char gh_hostname[GLOBAL_HDR_STRING_SZ];	/* 100  200 */
>> +		/* from gethostname(2) */
>> +	char gh_dumplabel[GLOBAL_HDR_STRING_SZ];	/* 100  300 */
>> +		/* label of dump session */
>> +	char gh_pad3[0x100];				/* 100  400 */
>> +		/* padding */
>> +	char gh_upper[GLOBAL_HDR_SZ - 0x400];		/* c00 1000 */
>> +		/* header info private to upper software layers */
>> +};
>> +typedef struct global_hdr global_hdr_t;
>> +
>> +#define sizeofmember( t, m )	sizeof( ( ( t * )0 )->m )
>> +
>> +#define DRIVE_HDR_SZ		sizeofmember(global_hdr_t, gh_upper)
>> +struct drive_hdr {
>> +	uint32_t dh_drivecnt;				/*   4    4 */
>> +		/* number of drives used to dump the fs */
>> +	uint32_t dh_driveix;				/*   4    8 */
>> +		/* 0-based index of the drive used to dump this stream */
>> +	int32_t dh_strategyid;				/*   4    c */
>> +		/* ID of the drive strategy used to produce this dump */
>> +	char dh_pad1[0x1f4];				/* 1f4  200 */
>> +		/* padding */
>> +	char dh_specific[0x200];			/* 200  400 */
>> +		/* drive strategy-specific info */
>> +	char dh_upper[DRIVE_HDR_SZ - 0x400];		/* 800  c00 */
>> +		/* header info private to upper software layers */
>> +};
>> +typedef struct drive_hdr drive_hdr_t;
>> +
>> +#define MEDIA_HDR_SZ		sizeofmember(drive_hdr_t, dh_upper)
>> +struct media_hdr {
>> +	char mh_medialabel[GLOBAL_HDR_STRING_SZ];	/* 100  100 */
>> +		/* label of media object containing file */
>> +	char mh_prevmedialabel[GLOBAL_HDR_STRING_SZ];	/* 100  200 */
>> +		/* label of upstream media object */
>> +	char mh_pad1[GLOBAL_HDR_STRING_SZ];		/* 100  300 */
>> +		/* in case more labels needed */
>> +	uuid_t mh_mediaid;				/*  10  310 */
>> +		/* ID of media object 	*/
>> +	uuid_t mh_prevmediaid;				/*  10  320 */
>> +		/* ID of upstream media object */
>> +	char mh_pad2[GLOBAL_HDR_UUID_SZ];		/*  10  330 */
>> +		/* in case more IDs needed */
>> +	uint32_t mh_mediaix;				/*   4  334 */
>> +		/* 0-based index of this media object within the dump stream */
>> +	uint32_t mh_mediafileix;			/*   4  338 */
>> +		/* 0-based index of this file within this media object */
>> +	uint32_t mh_dumpfileix;			/*   4  33c */
>> +		/* 0-based index of this file within this dump stream */
>> +	uint32_t mh_dumpmediafileix;			/*   4  340 */
>> +		/* 0-based index of file within dump stream and media object */
>> +	uint32_t mh_dumpmediaix;			/*   4  344 */
>> +		/* 0-based index of this dump within the media object */
>> +	int32_t mh_strategyid;				/*   4  348 */
>> +		/* ID of the media strategy used to produce this dump */
>> +	char mh_pad3[0x38];				/*  38  380 */
>> +		/* padding */
>> +	char mh_specific[0x80];			/*  80  400 */
>> +		/* media strategy-specific info */
>> +	char mh_upper[MEDIA_HDR_SZ - 0x400];		/* 400  800 */
>> +		/* header info private to upper software layers */
>> +};
>> +typedef struct media_hdr media_hdr_t;
>> +
>> +#define CONTENT_HDR_SZ		sizeofmember(media_hdr_t, mh_upper)
>> +#define CONTENT_HDR_FSTYPE_SZ	16
>> +#define CONTENT_STATSZ		160 /* must match dlog.h DLOG_MULTI_STATSZ */
>> +struct content_hdr {
>> +	char ch_mntpnt[GLOBAL_HDR_STRING_SZ];		/* 100  100 */
>> +		/* full pathname of fs mount point */
>> +	char ch_fsdevice[GLOBAL_HDR_STRING_SZ];	/* 100  200 */
>> +		/* full pathname of char device containing fs */
>> +	char  ch_pad1[GLOBAL_HDR_STRING_SZ];		/* 100  300 */
>> +		/* in case another label is needed */
>> +	char ch_fstype[CONTENT_HDR_FSTYPE_SZ];	/*  10  310 */
>> +		/* from fsid.h */
>> +	uuid_t ch_fsid;					/*  10  320 */
>> +		/* file system uuid */
>> +	char  ch_pad2[GLOBAL_HDR_UUID_SZ];		/*  10  330 */
>> +		/* in case another id is needed */
>> +	char ch_pad3[8];				/*   8  338 */
>> +		/* padding */
>> +	int32_t ch_strategyid;				/*   4  33c */
>> +		/* ID of the content strategy used to produce this dump */
>> +	char ch_pad4[4];				/*   4  340 */
>> +		/* alignment */
>> +	char ch_specific[0xc0];			/*  c0  400 */
>> +		/* content strategy-specific info */
>> +};
>> +typedef struct content_hdr content_hdr_t;
>> +
>> +typedef uint64_t xfs_ino_t;
>> +struct startpt {
>> +	xfs_ino_t sp_ino;		/* first inode to dump */
>> +	off64_t sp_offset;	/* bytes to skip in file data fork */
>> +	int32_t sp_flags;
>> +	int32_t sp_pad1;
>> +};
>> +typedef struct startpt startpt_t;
>> +
>> +#define CONTENT_INODE_HDR_SZ  sizeofmember(content_hdr_t, ch_specific)
>> +struct content_inode_hdr {
>> +	int32_t cih_mediafiletype;			/*   4   4 */
>> +		/* dump media file type: see #defines below */
>> +	int32_t cih_dumpattr;				/*   4   8 */
>> +		/* dump attributes: see #defines below */
>> +	int32_t cih_level;				/*   4   c */
>> +		/* dump level */
>> +	char pad1[4];					/*   4  10 */
>> +		/* alignment */
>> +	time32_t cih_last_time;				/*   4  14 */
>> +		/* if an incremental, time of previous dump at a lesser level */
>> +	time32_t cih_resume_time;			/*   4  18 */
>> +		/* if a resumed dump, time of interrupted dump */
>> +	xfs_ino_t cih_rootino;				/*   8  20 */
>> +		/* root inode number */
>> +	uuid_t cih_last_id;				/*  10  30 */
>> +		/* if an incremental, uuid of prev dump */
>> +	uuid_t cih_resume_id;				/*  10  40 */
>> +		/* if a resumed dump, uuid of interrupted dump */
>> +	startpt_t cih_startpt;				/*  18  58 */
>> +		/* starting point of media file contents */
>> +	startpt_t cih_endpt;				/*  18  70 */
>> +		/* starting point of next stream */
>> +	uint64_t cih_inomap_hnkcnt;			/*   8  78 */
>> +
>> +	uint64_t cih_inomap_segcnt;			/*   8  80 */
>> +
>> +	uint64_t cih_inomap_dircnt;			/*   8  88 */
>> +
>> +	uint64_t cih_inomap_nondircnt;			/*   8  90 */
>> +
>> +	xfs_ino_t cih_inomap_firstino;			/*   8  98 */
>> +
>> +	xfs_ino_t cih_inomap_lastino;			/*   8  a0 */
>> +
>> +	uint64_t cih_inomap_datasz;			/*   8  a8 */
>> +		/* bytes of non-metadata dumped */
>> +	char cih_pad2[CONTENT_INODE_HDR_SZ - 0xa8];	/*  18  c0 */
>> +		/* padding */
>> +};
>> +typedef struct content_inode_hdr content_inode_hdr_t;
>> +// End of definitions from xfsdump
>> +
>> +int main(int argc, char *argv[]) {
>> +
>> +	if (argc < 3) {
>> +		fprintf(stderr, "Usage: %s <path/to/dumpfile> <fake rootino>\n", argv[0]);
>> +		exit(1);
>> +	}
>> +
>> +	const char *filepath = argv[1];
>> +	const uint64_t fake_root_ino = (uint64_t) strtol(argv[2], NULL, 10);
>> +
>> +	int fd = open(filepath, O_RDWR);
>> +	if (fd < 0) {
>> +		perror("open");
>> +		exit(1);
>> +	}
>> +	global_hdr_t *header = mmap(NULL, GLOBAL_HDR_SZ, PROT_READ|PROT_WRITE, MAP_SHARED, fd, 0);
>> +	if (header == MAP_FAILED) {
>> +		perror("mmap");
>> +		exit(1);
>> +	}
>> +
>> +	uint64_t *rootino_ptr =
>> +		&((content_inode_hdr_t *)
>> +			((content_hdr_t *)
>> +				((media_hdr_t *)
>> +					((drive_hdr_t *)
>> +						header->gh_upper
>> +					)->dh_upper
>> +				)->mh_upper
>> +			)->ch_specific
>> +		)->cih_rootino;
> 
> Urrrrrk nested pointer dereferencing.
> 
> Oh wow, I didn't realize gh_upper is an opencoded char[].
> 
> If you flatten out all that nested pointer dereferencing and
> typecasting, is this what you get?
> 
> 	drive_hdr_t	*dh = (drive_hdr_t *)header->gh_upper;
> 	media_hdr_t	*mh = (media_hdr_t *)dh->dh_upper;
> 	content_hdr_t	*ch = (content_hdr_t *)mh->mh_upper;
> 	content_inode_hdr_t *cih = (content_inode_hdr_t *)ch->ch_specific;
> 	uint64_t	*rootino_ptr = &cih->cih_rootino;
> 
> If so, please change the code to do the above so that nobody else has to
> go through that nest of pointer casting.
> 
> With that fixed,
> Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> 
I fixed it. Thank you for your review!

---
Hiro


> --D
> 
> 
>> +	int32_t checksum = (int32_t) be32toh(header->gh_checksum);
>> +	uint64_t orig_rootino = be64toh(*rootino_ptr);
>> +
>> +	// Fake root inode number
>> +	*rootino_ptr = htobe64(fake_root_ino);
>> +
>> +	// Update checksum along with overwriting rootino.
>> +	uint64_t gap = orig_rootino - fake_root_ino;
>> +	checksum += (gap >> 32) + (gap & 0x00000000ffffffff);
>> +	header->gh_checksum = htobe32(checksum);
>> +
>> +	munmap(header, GLOBAL_HDR_SZ);
>> +	close(fd);
>> +}
>> diff --git a/tests/xfs/554 b/tests/xfs/554
>> new file mode 100755
>> index 00000000..fcfaa699
>> --- /dev/null
>> +++ b/tests/xfs/554
>> @@ -0,0 +1,73 @@
>> +#! /bin/bash
>> +# SPDX-License-Identifier: GPL-2.0
>> +# Copyright (c) 2022 Fujitsu Limited. All Rights Reserved.
>> +#
>> +# FS QA Test No. 554
>> +#
>> +# Create a filesystem which contains an inode with a lower number
>> +# than the root inode. Set the lower number to a dump file as the root inode
>> +# and ensure that 'xfsrestore -x' handles this wrong inode.
>> +#
>> +. ./common/preamble
>> +_begin_fstest auto quick dump
>> +
>> +# Import common functions.
>> +. ./common/dump
>> +
>> +_supported_fs xfs
>> +_require_xfs_io_command "falloc"
>> +_require_scratch
>> +_require_xfsrestore_xflag
>> +
>> +# A large stripe unit will put the root inode out quite far
>> +# due to alignment, leaving free blocks ahead of it.
>> +_scratch_mkfs_xfs -d sunit=1024,swidth=1024 > $seqres.full 2>&1
>> +
>> +# Mounting /without/ a stripe should allow inodes to be allocated
>> +# in lower free blocks, without the stripe alignment.
>> +_scratch_mount -o sunit=0,swidth=0
>> +
>> +root_inum=$(stat -c %i $SCRATCH_MNT)
>> +
>> +# Consume space after the root inode so that the blocks before
>> +# root look "close" for the next inode chunk allocation
>> +$XFS_IO_PROG -f -c "falloc 0 16m" $SCRATCH_MNT/fillfile
>> +
>> +# And make a bunch of inodes until we (hopefully) get one lower
>> +# than root, in a new inode chunk.
>> +echo "root_inum: $root_inum" >> $seqres.full
>> +for i in $(seq 0 4096) ; do
>> +	fname=$SCRATCH_MNT/$(printf "FILE_%03d" $i)
>> +	touch $fname
>> +	inum=$(stat -c "%i" $fname)
>> +	[[ $inum -lt $root_inum ]] && break
>> +done
>> +
>> +echo "created: $inum" >> $seqres.full
>> +
>> +[[ $inum -lt $root_inum ]] || _notrun "Could not set up test"
>> +
>> +# Now try a dump and restore. Cribbed from xfs/068
>> +_create_dumpdir_stress
>> +
>> +echo -n "Before: " >> $seqres.full
>> +_count_dumpdir_files | tee $tmp.before >> $seqres.full
>> +
>> +_do_dump_file
>> +
>> +# Set the wrong root inode number to the dump file
>> +# as problematic xfsdump used to do.
>> +$here/src/fake-dump-rootino $dump_file $inum
>> +
>> +_do_restore_file -x | \
>> +sed -e "s/rootino #${inum}/rootino #FAKENO/g" \
>> +	-e "s/# to ${root_inum}/# to ROOTNO/g" \
>> +	-e "/entries processed$/s/[0-9][0-9]*/NUM/g"
>> +
>> +echo -n "After: " >> $seqres.full
>> +_count_restoredir_files | tee $tmp.after >> $seqres.full
>> +diff -u $tmp.before $tmp.after
>> +
>> +# success, all done
>> +status=0
>> +exit
>> diff --git a/tests/xfs/554.out b/tests/xfs/554.out
>> new file mode 100644
>> index 00000000..c5e8c4c5
>> --- /dev/null
>> +++ b/tests/xfs/554.out
>> @@ -0,0 +1,40 @@
>> +QA output created by 554
>> +Creating directory system to dump using fsstress.
>> +
>> +-----------------------------------------------
>> +fsstress : -f link=10 -f creat=10 -f mkdir=10 -f truncate=5 -f symlink=10
>> +-----------------------------------------------
>> +Dumping to file...
>> +xfsdump  -f DUMP_FILE -M stress_tape_media -L stress_554 SCRATCH_MNT
>> +xfsdump: using file dump (drive_simple) strategy
>> +xfsdump: level 0 dump of HOSTNAME:SCRATCH_MNT
>> +xfsdump: dump date: DATE
>> +xfsdump: session id: ID
>> +xfsdump: session label: "stress_554"
>> +xfsdump: ino map <PHASES>
>> +xfsdump: ino map construction complete
>> +xfsdump: estimated dump size: NUM bytes
>> +xfsdump: /var/xfsdump/inventory created
>> +xfsdump: creating dump session media file 0 (media 0, file 0)
>> +xfsdump: dumping ino map
>> +xfsdump: dumping directories
>> +xfsdump: dumping non-directory files
>> +xfsdump: ending media file
>> +xfsdump: media file size NUM bytes
>> +xfsdump: dump size (non-dir files) : NUM bytes
>> +xfsdump: dump complete: SECS seconds elapsed
>> +xfsdump: Dump Status: SUCCESS
>> +Restoring from file...
>> +xfsrestore  -x -f DUMP_FILE  -L stress_554 RESTORE_DIR
>> +xfsrestore: using file dump (drive_simple) strategy
>> +xfsrestore: using online session inventory
>> +xfsrestore: searching media for directory dump
>> +xfsrestore: examining media file 0
>> +xfsrestore: reading directories
>> +xfsrestore: found fake rootino #FAKENO, will fix.
>> +xfsrestore: fix root # to ROOTNO (bind mount?)
>> +xfsrestore: NUM directories and NUM entries processed
>> +xfsrestore: directory post-processing
>> +xfsrestore: restoring non-directory files
>> +xfsrestore: restore complete: SECS seconds elapsed
>> +xfsrestore: Restore Status: SUCCESS
>> -- 
>> 2.37.3
>>

[PATCH V4] xfs: test for fixing wrong root inode number in dump

[Hironori Shiina]

Test '-x' option of xfsrestore. With this option, a wrong root inode
number in a dump file is corrected. A root inode number can be wrong
in a dump created by problematic xfsdump (v3.1.7 - v3.1.9) with
bulkstat misuse. In this test, a corrupted dump file is created by
overwriting a root inode number in a header.

Reviewed-by: Darrick J. Wong <djwong@kernel.org>
Signed-off-by: Hironori Shiina <shiina.hironori@fujitsu.com>
---
changes since v3:
  - Flatten out nested pointer dereferencing and typecasting.
changes since v2:
  - Use glibc functions to convert endian.
  - Copy the structure definitions from xfsdump source files.
changes since RFC v1:
  - Skip the test if xfsrestore does not support '-x' flag.
  - Create a corrupted dump by overwriting a root inode number in a dump
    file with a new tool instead of checking in a binary dump file.

 common/dump             |   2 +-
 common/xfs              |   6 ++
 src/Makefile            |   2 +-
 src/fake-dump-rootino.c | 224 ++++++++++++++++++++++++++++++++++++++++
 tests/xfs/554           |  73 +++++++++++++
 tests/xfs/554.out       |  40 +++++++
 6 files changed, 345 insertions(+), 2 deletions(-)
 create mode 100644 src/fake-dump-rootino.c
 create mode 100755 tests/xfs/554
 create mode 100644 tests/xfs/554.out

diff --git a/common/dump b/common/dump
index 8e0446d9..50b2ba03 100644
--- a/common/dump
+++ b/common/dump
@@ -1003,7 +1003,7 @@ _parse_restore_args()
         --no-check-quota)
             do_quota_check=false
             ;;
-	-K|-R)
+	-K|-R|-x)
 	    restore_args="$restore_args $1"
             ;;
 	*)
diff --git a/common/xfs b/common/xfs
index e1c15d3d..8334880e 100644
--- a/common/xfs
+++ b/common/xfs
@@ -1402,3 +1402,9 @@ _xfs_filter_mkfs()
 		print STDOUT "realtime =RDEV extsz=XXX blocks=XXX, rtextents=XXX\n";
 	}'
 }
+
+_require_xfsrestore_xflag()
+{
+	$XFSRESTORE_PROG -h 2>&1 | grep -q -e '-x' || \
+			_notrun 'xfsrestore does not support -x flag.'
+}
diff --git a/src/Makefile b/src/Makefile
index 5f565e73..afdf6b30 100644
--- a/src/Makefile
+++ b/src/Makefile
@@ -19,7 +19,7 @@ TARGETS = dirstress fill fill2 getpagesize holes lstat64 \
 	t_ofd_locks t_mmap_collision mmap-write-concurrent \
 	t_get_file_time t_create_short_dirs t_create_long_dirs t_enospc \
 	t_mmap_writev_overlap checkpoint_journal mmap-rw-fault allocstale \
-	t_mmap_cow_memory_failure
+	t_mmap_cow_memory_failure fake-dump-rootino
 
 LINUX_TARGETS = xfsctl bstat t_mtab getdevicesize preallo_rw_pattern_reader \
 	preallo_rw_pattern_writer ftrunc trunc fs_perms testx looptest \
diff --git a/src/fake-dump-rootino.c b/src/fake-dump-rootino.c
new file mode 100644
index 00000000..8a30dffd
--- /dev/null
+++ b/src/fake-dump-rootino.c
@@ -0,0 +1,224 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2022 Fujitsu Limited.  All Rights Reserved. */
+#define _LARGEFILE64_SOURCE
+#include <endian.h>
+#include <fcntl.h>
+#include <stdint.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <sys/mman.h>
+#include <unistd.h>
+#include <uuid/uuid.h>
+
+// Definitions from xfsdump
+#define PGSZLOG2	12
+#define PGSZ		(1 << PGSZLOG2)
+#define GLOBAL_HDR_SZ		PGSZ
+#define GLOBAL_HDR_MAGIC_SZ	8
+#define GLOBAL_HDR_STRING_SZ	0x100
+#define GLOBAL_HDR_TIME_SZ	4
+#define GLOBAL_HDR_UUID_SZ	0x10
+typedef int32_t time32_t;
+struct global_hdr {
+	char gh_magic[GLOBAL_HDR_MAGIC_SZ];		/*   8    8 */
+		/* unique signature of xfsdump */
+	uint32_t gh_version;				/*   4    c */
+		/* header version */
+	uint32_t gh_checksum;				/*   4   10 */
+		/* 32-bit unsigned additive inverse of entire header */
+	time32_t gh_timestamp;				/*   4   14 */
+		/* time32_t of dump */
+	char gh_pad1[4];				/*   4   18 */
+		/* alignment */
+	uint64_t gh_ipaddr;				/*   8   20 */
+		/* from gethostid(2), room for expansion */
+	uuid_t gh_dumpid;				/*  10   30 */
+		/* ID of dump session	 */
+	char gh_pad2[0xd0];				/*  d0  100 */
+		/* alignment */
+	char gh_hostname[GLOBAL_HDR_STRING_SZ];	/* 100  200 */
+		/* from gethostname(2) */
+	char gh_dumplabel[GLOBAL_HDR_STRING_SZ];	/* 100  300 */
+		/* label of dump session */
+	char gh_pad3[0x100];				/* 100  400 */
+		/* padding */
+	char gh_upper[GLOBAL_HDR_SZ - 0x400];		/* c00 1000 */
+		/* header info private to upper software layers */
+};
+typedef struct global_hdr global_hdr_t;
+
+#define sizeofmember( t, m )	sizeof( ( ( t * )0 )->m )
+
+#define DRIVE_HDR_SZ		sizeofmember(global_hdr_t, gh_upper)
+struct drive_hdr {
+	uint32_t dh_drivecnt;				/*   4    4 */
+		/* number of drives used to dump the fs */
+	uint32_t dh_driveix;				/*   4    8 */
+		/* 0-based index of the drive used to dump this stream */
+	int32_t dh_strategyid;				/*   4    c */
+		/* ID of the drive strategy used to produce this dump */
+	char dh_pad1[0x1f4];				/* 1f4  200 */
+		/* padding */
+	char dh_specific[0x200];			/* 200  400 */
+		/* drive strategy-specific info */
+	char dh_upper[DRIVE_HDR_SZ - 0x400];		/* 800  c00 */
+		/* header info private to upper software layers */
+};
+typedef struct drive_hdr drive_hdr_t;
+
+#define MEDIA_HDR_SZ		sizeofmember(drive_hdr_t, dh_upper)
+struct media_hdr {
+	char mh_medialabel[GLOBAL_HDR_STRING_SZ];	/* 100  100 */
+		/* label of media object containing file */
+	char mh_prevmedialabel[GLOBAL_HDR_STRING_SZ];	/* 100  200 */
+		/* label of upstream media object */
+	char mh_pad1[GLOBAL_HDR_STRING_SZ];		/* 100  300 */
+		/* in case more labels needed */
+	uuid_t mh_mediaid;				/*  10  310 */
+		/* ID of media object 	*/
+	uuid_t mh_prevmediaid;				/*  10  320 */
+		/* ID of upstream media object */
+	char mh_pad2[GLOBAL_HDR_UUID_SZ];		/*  10  330 */
+		/* in case more IDs needed */
+	uint32_t mh_mediaix;				/*   4  334 */
+		/* 0-based index of this media object within the dump stream */
+	uint32_t mh_mediafileix;			/*   4  338 */
+		/* 0-based index of this file within this media object */
+	uint32_t mh_dumpfileix;			/*   4  33c */
+		/* 0-based index of this file within this dump stream */
+	uint32_t mh_dumpmediafileix;			/*   4  340 */
+		/* 0-based index of file within dump stream and media object */
+	uint32_t mh_dumpmediaix;			/*   4  344 */
+		/* 0-based index of this dump within the media object */
+	int32_t mh_strategyid;				/*   4  348 */
+		/* ID of the media strategy used to produce this dump */
+	char mh_pad3[0x38];				/*  38  380 */
+		/* padding */
+	char mh_specific[0x80];			/*  80  400 */
+		/* media strategy-specific info */
+	char mh_upper[MEDIA_HDR_SZ - 0x400];		/* 400  800 */
+		/* header info private to upper software layers */
+};
+typedef struct media_hdr media_hdr_t;
+
+#define CONTENT_HDR_SZ		sizeofmember(media_hdr_t, mh_upper)
+#define CONTENT_HDR_FSTYPE_SZ	16
+#define CONTENT_STATSZ		160 /* must match dlog.h DLOG_MULTI_STATSZ */
+struct content_hdr {
+	char ch_mntpnt[GLOBAL_HDR_STRING_SZ];		/* 100  100 */
+		/* full pathname of fs mount point */
+	char ch_fsdevice[GLOBAL_HDR_STRING_SZ];	/* 100  200 */
+		/* full pathname of char device containing fs */
+	char  ch_pad1[GLOBAL_HDR_STRING_SZ];		/* 100  300 */
+		/* in case another label is needed */
+	char ch_fstype[CONTENT_HDR_FSTYPE_SZ];	/*  10  310 */
+		/* from fsid.h */
+	uuid_t ch_fsid;					/*  10  320 */
+		/* file system uuid */
+	char  ch_pad2[GLOBAL_HDR_UUID_SZ];		/*  10  330 */
+		/* in case another id is needed */
+	char ch_pad3[8];				/*   8  338 */
+		/* padding */
+	int32_t ch_strategyid;				/*   4  33c */
+		/* ID of the content strategy used to produce this dump */
+	char ch_pad4[4];				/*   4  340 */
+		/* alignment */
+	char ch_specific[0xc0];			/*  c0  400 */
+		/* content strategy-specific info */
+};
+typedef struct content_hdr content_hdr_t;
+
+typedef uint64_t xfs_ino_t;
+struct startpt {
+	xfs_ino_t sp_ino;		/* first inode to dump */
+	off64_t sp_offset;	/* bytes to skip in file data fork */
+	int32_t sp_flags;
+	int32_t sp_pad1;
+};
+typedef struct startpt startpt_t;
+
+#define CONTENT_INODE_HDR_SZ  sizeofmember(content_hdr_t, ch_specific)
+struct content_inode_hdr {
+	int32_t cih_mediafiletype;			/*   4   4 */
+		/* dump media file type: see #defines below */
+	int32_t cih_dumpattr;				/*   4   8 */
+		/* dump attributes: see #defines below */
+	int32_t cih_level;				/*   4   c */
+		/* dump level */
+	char pad1[4];					/*   4  10 */
+		/* alignment */
+	time32_t cih_last_time;				/*   4  14 */
+		/* if an incremental, time of previous dump at a lesser level */
+	time32_t cih_resume_time;			/*   4  18 */
+		/* if a resumed dump, time of interrupted dump */
+	xfs_ino_t cih_rootino;				/*   8  20 */
+		/* root inode number */
+	uuid_t cih_last_id;				/*  10  30 */
+		/* if an incremental, uuid of prev dump */
+	uuid_t cih_resume_id;				/*  10  40 */
+		/* if a resumed dump, uuid of interrupted dump */
+	startpt_t cih_startpt;				/*  18  58 */
+		/* starting point of media file contents */
+	startpt_t cih_endpt;				/*  18  70 */
+		/* starting point of next stream */
+	uint64_t cih_inomap_hnkcnt;			/*   8  78 */
+
+	uint64_t cih_inomap_segcnt;			/*   8  80 */
+
+	uint64_t cih_inomap_dircnt;			/*   8  88 */
+
+	uint64_t cih_inomap_nondircnt;			/*   8  90 */
+
+	xfs_ino_t cih_inomap_firstino;			/*   8  98 */
+
+	xfs_ino_t cih_inomap_lastino;			/*   8  a0 */
+
+	uint64_t cih_inomap_datasz;			/*   8  a8 */
+		/* bytes of non-metadata dumped */
+	char cih_pad2[CONTENT_INODE_HDR_SZ - 0xa8];	/*  18  c0 */
+		/* padding */
+};
+typedef struct content_inode_hdr content_inode_hdr_t;
+// End of definitions from xfsdump
+
+int main(int argc, char *argv[]) {
+
+	if (argc < 3) {
+		fprintf(stderr, "Usage: %s <path/to/dumpfile> <fake rootino>\n", argv[0]);
+		exit(1);
+	}
+
+	const char *filepath = argv[1];
+	const uint64_t fake_root_ino = (uint64_t)strtol(argv[2], NULL, 10);
+
+	int fd = open(filepath, O_RDWR);
+	if (fd < 0) {
+		perror("open");
+		exit(1);
+	}
+	global_hdr_t *header = mmap(NULL, GLOBAL_HDR_SZ, PROT_READ|PROT_WRITE, MAP_SHARED, fd, 0);
+	if (header == MAP_FAILED) {
+		perror("mmap");
+		exit(1);
+	}
+
+	drive_hdr_t *dh = (drive_hdr_t *)header->gh_upper;
+	media_hdr_t *mh = (media_hdr_t *)dh->dh_upper;
+	content_hdr_t *ch = (content_hdr_t *)mh->mh_upper;
+	content_inode_hdr_t *cih = (content_inode_hdr_t *)ch->ch_specific;
+	uint64_t *rootino_ptr = &cih->cih_rootino;
+
+	int32_t checksum = (int32_t)be32toh(header->gh_checksum);
+	uint64_t orig_rootino = be64toh(*rootino_ptr);
+
+	// Fake root inode number
+	*rootino_ptr = htobe64(fake_root_ino);
+
+	// Update checksum along with overwriting rootino.
+	uint64_t gap = orig_rootino - fake_root_ino;
+	checksum += (gap >> 32) + (gap & 0x00000000ffffffff);
+	header->gh_checksum = htobe32(checksum);
+
+	munmap(header, GLOBAL_HDR_SZ);
+	close(fd);
+}
diff --git a/tests/xfs/554 b/tests/xfs/554
new file mode 100755
index 00000000..fcfaa699
--- /dev/null
+++ b/tests/xfs/554
@@ -0,0 +1,73 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2022 Fujitsu Limited. All Rights Reserved.
+#
+# FS QA Test No. 554
+#
+# Create a filesystem which contains an inode with a lower number
+# than the root inode. Set the lower number to a dump file as the root inode
+# and ensure that 'xfsrestore -x' handles this wrong inode.
+#
+. ./common/preamble
+_begin_fstest auto quick dump
+
+# Import common functions.
+. ./common/dump
+
+_supported_fs xfs
+_require_xfs_io_command "falloc"
+_require_scratch
+_require_xfsrestore_xflag
+
+# A large stripe unit will put the root inode out quite far
+# due to alignment, leaving free blocks ahead of it.
+_scratch_mkfs_xfs -d sunit=1024,swidth=1024 > $seqres.full 2>&1
+
+# Mounting /without/ a stripe should allow inodes to be allocated
+# in lower free blocks, without the stripe alignment.
+_scratch_mount -o sunit=0,swidth=0
+
+root_inum=$(stat -c %i $SCRATCH_MNT)
+
+# Consume space after the root inode so that the blocks before
+# root look "close" for the next inode chunk allocation
+$XFS_IO_PROG -f -c "falloc 0 16m" $SCRATCH_MNT/fillfile
+
+# And make a bunch of inodes until we (hopefully) get one lower
+# than root, in a new inode chunk.
+echo "root_inum: $root_inum" >> $seqres.full
+for i in $(seq 0 4096) ; do
+	fname=$SCRATCH_MNT/$(printf "FILE_%03d" $i)
+	touch $fname
+	inum=$(stat -c "%i" $fname)
+	[[ $inum -lt $root_inum ]] && break
+done
+
+echo "created: $inum" >> $seqres.full
+
+[[ $inum -lt $root_inum ]] || _notrun "Could not set up test"
+
+# Now try a dump and restore. Cribbed from xfs/068
+_create_dumpdir_stress
+
+echo -n "Before: " >> $seqres.full
+_count_dumpdir_files | tee $tmp.before >> $seqres.full
+
+_do_dump_file
+
+# Set the wrong root inode number to the dump file
+# as problematic xfsdump used to do.
+$here/src/fake-dump-rootino $dump_file $inum
+
+_do_restore_file -x | \
+sed -e "s/rootino #${inum}/rootino #FAKENO/g" \
+	-e "s/# to ${root_inum}/# to ROOTNO/g" \
+	-e "/entries processed$/s/[0-9][0-9]*/NUM/g"
+
+echo -n "After: " >> $seqres.full
+_count_restoredir_files | tee $tmp.after >> $seqres.full
+diff -u $tmp.before $tmp.after
+
+# success, all done
+status=0
+exit
diff --git a/tests/xfs/554.out b/tests/xfs/554.out
new file mode 100644
index 00000000..c5e8c4c5
--- /dev/null
+++ b/tests/xfs/554.out
@@ -0,0 +1,40 @@
+QA output created by 554
+Creating directory system to dump using fsstress.
+
+-----------------------------------------------
+fsstress : -f link=10 -f creat=10 -f mkdir=10 -f truncate=5 -f symlink=10
+-----------------------------------------------
+Dumping to file...
+xfsdump  -f DUMP_FILE -M stress_tape_media -L stress_554 SCRATCH_MNT
+xfsdump: using file dump (drive_simple) strategy
+xfsdump: level 0 dump of HOSTNAME:SCRATCH_MNT
+xfsdump: dump date: DATE
+xfsdump: session id: ID
+xfsdump: session label: "stress_554"
+xfsdump: ino map <PHASES>
+xfsdump: ino map construction complete
+xfsdump: estimated dump size: NUM bytes
+xfsdump: /var/xfsdump/inventory created
+xfsdump: creating dump session media file 0 (media 0, file 0)
+xfsdump: dumping ino map
+xfsdump: dumping directories
+xfsdump: dumping non-directory files
+xfsdump: ending media file
+xfsdump: media file size NUM bytes
+xfsdump: dump size (non-dir files) : NUM bytes
+xfsdump: dump complete: SECS seconds elapsed
+xfsdump: Dump Status: SUCCESS
+Restoring from file...
+xfsrestore  -x -f DUMP_FILE  -L stress_554 RESTORE_DIR
+xfsrestore: using file dump (drive_simple) strategy
+xfsrestore: using online session inventory
+xfsrestore: searching media for directory dump
+xfsrestore: examining media file 0
+xfsrestore: reading directories
+xfsrestore: found fake rootino #FAKENO, will fix.
+xfsrestore: fix root # to ROOTNO (bind mount?)
+xfsrestore: NUM directories and NUM entries processed
+xfsrestore: directory post-processing
+xfsrestore: restoring non-directory files
+xfsrestore: restore complete: SECS seconds elapsed
+xfsrestore: Restore Status: SUCCESS
-- 
2.37.3

Re: [PATCH V4] xfs: test for fixing wrong root inode number in dump

[Zorro Lang]

On Fri, Oct 14, 2022 at 10:04:00PM -0400, Hironori Shiina wrote:
> Test '-x' option of xfsrestore. With this option, a wrong root inode
> number in a dump file is corrected. A root inode number can be wrong
> in a dump created by problematic xfsdump (v3.1.7 - v3.1.9) with
> bulkstat misuse. In this test, a corrupted dump file is created by
> overwriting a root inode number in a header.
> 
> Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> Signed-off-by: Hironori Shiina <shiina.hironori@fujitsu.com>
> ---
> changes since v3:
>   - Flatten out nested pointer dereferencing and typecasting.
> changes since v2:
>   - Use glibc functions to convert endian.
>   - Copy the structure definitions from xfsdump source files.
> changes since RFC v1:
>   - Skip the test if xfsrestore does not support '-x' flag.
>   - Create a corrupted dump by overwriting a root inode number in a dump
>     file with a new tool instead of checking in a binary dump file.

Thanks for this patch, hope the src/fake-dump-rootino.c can keep working
well for long time. And Thanks Darrick help to review this patch. I don't
have objection on this patch, just a few small review points as below ...

> 
>  common/dump             |   2 +-
>  common/xfs              |   6 ++
>  src/Makefile            |   2 +-
>  src/fake-dump-rootino.c | 224 ++++++++++++++++++++++++++++++++++++++++
>  tests/xfs/554           |  73 +++++++++++++
>  tests/xfs/554.out       |  40 +++++++
>  6 files changed, 345 insertions(+), 2 deletions(-)
>  create mode 100644 src/fake-dump-rootino.c
>  create mode 100755 tests/xfs/554
>  create mode 100644 tests/xfs/554.out
> 
> diff --git a/common/dump b/common/dump
> index 8e0446d9..50b2ba03 100644
> --- a/common/dump
> +++ b/common/dump
> @@ -1003,7 +1003,7 @@ _parse_restore_args()
>          --no-check-quota)
>              do_quota_check=false
>              ;;
> -	-K|-R)
> +	-K|-R|-x)
>  	    restore_args="$restore_args $1"
>              ;;
>  	*)
> diff --git a/common/xfs b/common/xfs
> index e1c15d3d..8334880e 100644
> --- a/common/xfs
> +++ b/common/xfs
> @@ -1402,3 +1402,9 @@ _xfs_filter_mkfs()
>  		print STDOUT "realtime =RDEV extsz=XXX blocks=XXX, rtextents=XXX\n";
>  	}'
>  }
> +
> +_require_xfsrestore_xflag()
> +{
> +	$XFSRESTORE_PROG -h 2>&1 | grep -q -e '-x' || \
> +			_notrun 'xfsrestore does not support -x flag.'
> +}
> diff --git a/src/Makefile b/src/Makefile
> index 5f565e73..afdf6b30 100644
> --- a/src/Makefile
> +++ b/src/Makefile
> @@ -19,7 +19,7 @@ TARGETS = dirstress fill fill2 getpagesize holes lstat64 \
>  	t_ofd_locks t_mmap_collision mmap-write-concurrent \
>  	t_get_file_time t_create_short_dirs t_create_long_dirs t_enospc \
>  	t_mmap_writev_overlap checkpoint_journal mmap-rw-fault allocstale \
> -	t_mmap_cow_memory_failure
> +	t_mmap_cow_memory_failure fake-dump-rootino
>  
>  LINUX_TARGETS = xfsctl bstat t_mtab getdevicesize preallo_rw_pattern_reader \
>  	preallo_rw_pattern_writer ftrunc trunc fs_perms testx looptest \
> diff --git a/src/fake-dump-rootino.c b/src/fake-dump-rootino.c
> new file mode 100644
> index 00000000..8a30dffd
> --- /dev/null
> +++ b/src/fake-dump-rootino.c
> @@ -0,0 +1,224 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2022 Fujitsu Limited.  All Rights Reserved. */
> +#define _LARGEFILE64_SOURCE
> +#include <endian.h>
> +#include <fcntl.h>
> +#include <stdint.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <sys/mman.h>
> +#include <unistd.h>
> +#include <uuid/uuid.h>
> +
> +// Definitions from xfsdump
> +#define PGSZLOG2	12
> +#define PGSZ		(1 << PGSZLOG2)
> +#define GLOBAL_HDR_SZ		PGSZ
> +#define GLOBAL_HDR_MAGIC_SZ	8
> +#define GLOBAL_HDR_STRING_SZ	0x100
> +#define GLOBAL_HDR_TIME_SZ	4
> +#define GLOBAL_HDR_UUID_SZ	0x10
> +typedef int32_t time32_t;
> +struct global_hdr {
> +	char gh_magic[GLOBAL_HDR_MAGIC_SZ];		/*   8    8 */
> +		/* unique signature of xfsdump */
> +	uint32_t gh_version;				/*   4    c */
> +		/* header version */
> +	uint32_t gh_checksum;				/*   4   10 */
> +		/* 32-bit unsigned additive inverse of entire header */
> +	time32_t gh_timestamp;				/*   4   14 */
> +		/* time32_t of dump */
> +	char gh_pad1[4];				/*   4   18 */
> +		/* alignment */
> +	uint64_t gh_ipaddr;				/*   8   20 */
> +		/* from gethostid(2), room for expansion */
> +	uuid_t gh_dumpid;				/*  10   30 */
> +		/* ID of dump session	 */
> +	char gh_pad2[0xd0];				/*  d0  100 */
> +		/* alignment */
> +	char gh_hostname[GLOBAL_HDR_STRING_SZ];	/* 100  200 */
> +		/* from gethostname(2) */
> +	char gh_dumplabel[GLOBAL_HDR_STRING_SZ];	/* 100  300 */
> +		/* label of dump session */
> +	char gh_pad3[0x100];				/* 100  400 */
> +		/* padding */
> +	char gh_upper[GLOBAL_HDR_SZ - 0x400];		/* c00 1000 */
> +		/* header info private to upper software layers */
> +};
> +typedef struct global_hdr global_hdr_t;
> +
> +#define sizeofmember( t, m )	sizeof( ( ( t * )0 )->m )
> +
> +#define DRIVE_HDR_SZ		sizeofmember(global_hdr_t, gh_upper)
> +struct drive_hdr {
> +	uint32_t dh_drivecnt;				/*   4    4 */
> +		/* number of drives used to dump the fs */
> +	uint32_t dh_driveix;				/*   4    8 */
> +		/* 0-based index of the drive used to dump this stream */
> +	int32_t dh_strategyid;				/*   4    c */
> +		/* ID of the drive strategy used to produce this dump */
> +	char dh_pad1[0x1f4];				/* 1f4  200 */
> +		/* padding */
> +	char dh_specific[0x200];			/* 200  400 */
> +		/* drive strategy-specific info */
> +	char dh_upper[DRIVE_HDR_SZ - 0x400];		/* 800  c00 */
> +		/* header info private to upper software layers */
> +};
> +typedef struct drive_hdr drive_hdr_t;
> +
> +#define MEDIA_HDR_SZ		sizeofmember(drive_hdr_t, dh_upper)
> +struct media_hdr {
> +	char mh_medialabel[GLOBAL_HDR_STRING_SZ];	/* 100  100 */
> +		/* label of media object containing file */
> +	char mh_prevmedialabel[GLOBAL_HDR_STRING_SZ];	/* 100  200 */
> +		/* label of upstream media object */
> +	char mh_pad1[GLOBAL_HDR_STRING_SZ];		/* 100  300 */
> +		/* in case more labels needed */
> +	uuid_t mh_mediaid;				/*  10  310 */
> +		/* ID of media object 	*/
> +	uuid_t mh_prevmediaid;				/*  10  320 */
> +		/* ID of upstream media object */
> +	char mh_pad2[GLOBAL_HDR_UUID_SZ];		/*  10  330 */
> +		/* in case more IDs needed */
> +	uint32_t mh_mediaix;				/*   4  334 */
> +		/* 0-based index of this media object within the dump stream */
> +	uint32_t mh_mediafileix;			/*   4  338 */
> +		/* 0-based index of this file within this media object */
> +	uint32_t mh_dumpfileix;			/*   4  33c */
> +		/* 0-based index of this file within this dump stream */
> +	uint32_t mh_dumpmediafileix;			/*   4  340 */
> +		/* 0-based index of file within dump stream and media object */
> +	uint32_t mh_dumpmediaix;			/*   4  344 */
> +		/* 0-based index of this dump within the media object */
> +	int32_t mh_strategyid;				/*   4  348 */
> +		/* ID of the media strategy used to produce this dump */
> +	char mh_pad3[0x38];				/*  38  380 */
> +		/* padding */
> +	char mh_specific[0x80];			/*  80  400 */
> +		/* media strategy-specific info */
> +	char mh_upper[MEDIA_HDR_SZ - 0x400];		/* 400  800 */
> +		/* header info private to upper software layers */
> +};
> +typedef struct media_hdr media_hdr_t;
> +
> +#define CONTENT_HDR_SZ		sizeofmember(media_hdr_t, mh_upper)
> +#define CONTENT_HDR_FSTYPE_SZ	16
> +#define CONTENT_STATSZ		160 /* must match dlog.h DLOG_MULTI_STATSZ */
> +struct content_hdr {
> +	char ch_mntpnt[GLOBAL_HDR_STRING_SZ];		/* 100  100 */
> +		/* full pathname of fs mount point */
> +	char ch_fsdevice[GLOBAL_HDR_STRING_SZ];	/* 100  200 */
> +		/* full pathname of char device containing fs */
> +	char  ch_pad1[GLOBAL_HDR_STRING_SZ];		/* 100  300 */
> +		/* in case another label is needed */
> +	char ch_fstype[CONTENT_HDR_FSTYPE_SZ];	/*  10  310 */
> +		/* from fsid.h */
> +	uuid_t ch_fsid;					/*  10  320 */
> +		/* file system uuid */
> +	char  ch_pad2[GLOBAL_HDR_UUID_SZ];		/*  10  330 */
> +		/* in case another id is needed */
> +	char ch_pad3[8];				/*   8  338 */
> +		/* padding */
> +	int32_t ch_strategyid;				/*   4  33c */
> +		/* ID of the content strategy used to produce this dump */
> +	char ch_pad4[4];				/*   4  340 */
> +		/* alignment */
> +	char ch_specific[0xc0];			/*  c0  400 */
> +		/* content strategy-specific info */
> +};
> +typedef struct content_hdr content_hdr_t;
> +
> +typedef uint64_t xfs_ino_t;
> +struct startpt {
> +	xfs_ino_t sp_ino;		/* first inode to dump */
> +	off64_t sp_offset;	/* bytes to skip in file data fork */
> +	int32_t sp_flags;
> +	int32_t sp_pad1;
> +};
> +typedef struct startpt startpt_t;
> +
> +#define CONTENT_INODE_HDR_SZ  sizeofmember(content_hdr_t, ch_specific)
> +struct content_inode_hdr {
> +	int32_t cih_mediafiletype;			/*   4   4 */
> +		/* dump media file type: see #defines below */
> +	int32_t cih_dumpattr;				/*   4   8 */
> +		/* dump attributes: see #defines below */
> +	int32_t cih_level;				/*   4   c */
> +		/* dump level */
> +	char pad1[4];					/*   4  10 */
> +		/* alignment */
> +	time32_t cih_last_time;				/*   4  14 */
> +		/* if an incremental, time of previous dump at a lesser level */
> +	time32_t cih_resume_time;			/*   4  18 */
> +		/* if a resumed dump, time of interrupted dump */
> +	xfs_ino_t cih_rootino;				/*   8  20 */
> +		/* root inode number */
> +	uuid_t cih_last_id;				/*  10  30 */
> +		/* if an incremental, uuid of prev dump */
> +	uuid_t cih_resume_id;				/*  10  40 */
> +		/* if a resumed dump, uuid of interrupted dump */
> +	startpt_t cih_startpt;				/*  18  58 */
> +		/* starting point of media file contents */
> +	startpt_t cih_endpt;				/*  18  70 */
> +		/* starting point of next stream */
> +	uint64_t cih_inomap_hnkcnt;			/*   8  78 */
> +
> +	uint64_t cih_inomap_segcnt;			/*   8  80 */
> +
> +	uint64_t cih_inomap_dircnt;			/*   8  88 */
> +
> +	uint64_t cih_inomap_nondircnt;			/*   8  90 */
> +
> +	xfs_ino_t cih_inomap_firstino;			/*   8  98 */
> +
> +	xfs_ino_t cih_inomap_lastino;			/*   8  a0 */
> +
> +	uint64_t cih_inomap_datasz;			/*   8  a8 */
> +		/* bytes of non-metadata dumped */
> +	char cih_pad2[CONTENT_INODE_HDR_SZ - 0xa8];	/*  18  c0 */
> +		/* padding */
> +};
> +typedef struct content_inode_hdr content_inode_hdr_t;
> +// End of definitions from xfsdump
> +
> +int main(int argc, char *argv[]) {
> +
> +	if (argc < 3) {
> +		fprintf(stderr, "Usage: %s <path/to/dumpfile> <fake rootino>\n", argv[0]);
> +		exit(1);
> +	}
> +
> +	const char *filepath = argv[1];
> +	const uint64_t fake_root_ino = (uint64_t)strtol(argv[2], NULL, 10);
> +
> +	int fd = open(filepath, O_RDWR);
> +	if (fd < 0) {
> +		perror("open");
> +		exit(1);
> +	}
> +	global_hdr_t *header = mmap(NULL, GLOBAL_HDR_SZ, PROT_READ|PROT_WRITE, MAP_SHARED, fd, 0);
> +	if (header == MAP_FAILED) {
> +		perror("mmap");
> +		exit(1);
> +	}
> +
> +	drive_hdr_t *dh = (drive_hdr_t *)header->gh_upper;
> +	media_hdr_t *mh = (media_hdr_t *)dh->dh_upper;
> +	content_hdr_t *ch = (content_hdr_t *)mh->mh_upper;
> +	content_inode_hdr_t *cih = (content_inode_hdr_t *)ch->ch_specific;
> +	uint64_t *rootino_ptr = &cih->cih_rootino;
> +
> +	int32_t checksum = (int32_t)be32toh(header->gh_checksum);
> +	uint64_t orig_rootino = be64toh(*rootino_ptr);
> +
> +	// Fake root inode number
> +	*rootino_ptr = htobe64(fake_root_ino);
> +
> +	// Update checksum along with overwriting rootino.
> +	uint64_t gap = orig_rootino - fake_root_ino;
> +	checksum += (gap >> 32) + (gap & 0x00000000ffffffff);
> +	header->gh_checksum = htobe32(checksum);
> +
> +	munmap(header, GLOBAL_HDR_SZ);
> +	close(fd);
> +}
> diff --git a/tests/xfs/554 b/tests/xfs/554
> new file mode 100755
> index 00000000..fcfaa699
> --- /dev/null
> +++ b/tests/xfs/554
> @@ -0,0 +1,73 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2022 Fujitsu Limited. All Rights Reserved.
> +#
> +# FS QA Test No. 554
> +#
> +# Create a filesystem which contains an inode with a lower number
> +# than the root inode. Set the lower number to a dump file as the root inode
> +# and ensure that 'xfsrestore -x' handles this wrong inode.
> +#
> +. ./common/preamble
> +_begin_fstest auto quick dump
> +
> +# Import common functions.
> +. ./common/dump
> +
> +_supported_fs xfs

_fixed_by_git_commit xfsdump \
	XXXXXXXXXXXX xfsrestore: fix rootdir due to xfsdump bulkstat misuse

As this patch hasn't been merged by xfsdump(but acked), so I don't mind using
"XXXX..." to instead the commit id. We can change all these XXXX.... later in
one patch, for now we know its subject name at first.

> +_require_xfs_io_command "falloc"
> +_require_scratch
> +_require_xfsrestore_xflag
> +
> +# A large stripe unit will put the root inode out quite far
> +# due to alignment, leaving free blocks ahead of it.
> +_scratch_mkfs_xfs -d sunit=1024,swidth=1024 > $seqres.full 2>&1

I think most cases write as this:
  _scratch_mkfs... $some_specific_options || _fail ...

if you use some specific mkfs options to mkfs. Due to if mkfs fails, this
case will keep running on old fs in SCRATCH_DEV without any warning/error.

> +
> +# Mounting /without/ a stripe should allow inodes to be allocated
> +# in lower free blocks, without the stripe alignment.
> +_scratch_mount -o sunit=0,swidth=0
> +
> +root_inum=$(stat -c %i $SCRATCH_MNT)
> +
> +# Consume space after the root inode so that the blocks before
> +# root look "close" for the next inode chunk allocation
> +$XFS_IO_PROG -f -c "falloc 0 16m" $SCRATCH_MNT/fillfile
> +
> +# And make a bunch of inodes until we (hopefully) get one lower
> +# than root, in a new inode chunk.
> +echo "root_inum: $root_inum" >> $seqres.full
> +for i in $(seq 0 4096) ; do
> +	fname=$SCRATCH_MNT/$(printf "FILE_%03d" $i)
> +	touch $fname
> +	inum=$(stat -c "%i" $fname)
> +	[[ $inum -lt $root_inum ]] && break
> +done
> +
> +echo "created: $inum" >> $seqres.full
> +
> +[[ $inum -lt $root_inum ]] || _notrun "Could not set up test"
> +
> +# Now try a dump and restore. Cribbed from xfs/068
> +_create_dumpdir_stress
> +
> +echo -n "Before: " >> $seqres.full
> +_count_dumpdir_files | tee $tmp.before >> $seqres.full
> +
> +_do_dump_file
> +
> +# Set the wrong root inode number to the dump file
> +# as problematic xfsdump used to do.
> +$here/src/fake-dump-rootino $dump_file $inum
> +
> +_do_restore_file -x | \
> +sed -e "s/rootino #${inum}/rootino #FAKENO/g" \
> +	-e "s/# to ${root_inum}/# to ROOTNO/g" \
> +	-e "/entries processed$/s/[0-9][0-9]*/NUM/g"
> +
> +echo -n "After: " >> $seqres.full
> +_count_restoredir_files | tee $tmp.after >> $seqres.full
> +diff -u $tmp.before $tmp.after
> +
> +# success, all done
> +status=0
> +exit
> diff --git a/tests/xfs/554.out b/tests/xfs/554.out
> new file mode 100644
> index 00000000..c5e8c4c5
> --- /dev/null
> +++ b/tests/xfs/554.out
> @@ -0,0 +1,40 @@
> +QA output created by 554
> +Creating directory system to dump using fsstress.
> +
> +-----------------------------------------------
> +fsstress : -f link=10 -f creat=10 -f mkdir=10 -f truncate=5 -f symlink=10
> +-----------------------------------------------
> +Dumping to file...
> +xfsdump  -f DUMP_FILE -M stress_tape_media -L stress_554 SCRATCH_MNT
> +xfsdump: using file dump (drive_simple) strategy
> +xfsdump: level 0 dump of HOSTNAME:SCRATCH_MNT
> +xfsdump: dump date: DATE
> +xfsdump: session id: ID
> +xfsdump: session label: "stress_554"
> +xfsdump: ino map <PHASES>
> +xfsdump: ino map construction complete
> +xfsdump: estimated dump size: NUM bytes
> +xfsdump: /var/xfsdump/inventory created
> +xfsdump: creating dump session media file 0 (media 0, file 0)
> +xfsdump: dumping ino map
> +xfsdump: dumping directories
> +xfsdump: dumping non-directory files
> +xfsdump: ending media file
> +xfsdump: media file size NUM bytes
> +xfsdump: dump size (non-dir files) : NUM bytes
> +xfsdump: dump complete: SECS seconds elapsed
> +xfsdump: Dump Status: SUCCESS
> +Restoring from file...
> +xfsrestore  -x -f DUMP_FILE  -L stress_554 RESTORE_DIR
> +xfsrestore: using file dump (drive_simple) strategy
> +xfsrestore: using online session inventory
> +xfsrestore: searching media for directory dump
> +xfsrestore: examining media file 0
> +xfsrestore: reading directories
> +xfsrestore: found fake rootino #FAKENO, will fix.
> +xfsrestore: fix root # to ROOTNO (bind mount?)
> +xfsrestore: NUM directories and NUM entries processed
> +xfsrestore: directory post-processing
> +xfsrestore: restoring non-directory files
> +xfsrestore: restore complete: SECS seconds elapsed
> +xfsrestore: Restore Status: SUCCESS
> -- 
> 2.37.3
> 

[PATCH V5] xfs: test for fixing wrong root inode number in dump

[Hironori Shiina]

Test '-x' option of xfsrestore. With this option, a wrong root inode
number in a dump file is corrected. A root inode number can be wrong
in a dump created by problematic xfsdump (v3.1.7 - v3.1.9) with
bulkstat misuse. In this test, a corrupted dump file is created by
overwriting a root inode number in a header.

Reviewed-by: Darrick J. Wong <djwong@kernel.org>
Signed-off-by: Hironori Shiina <shiina.hironori@fujitsu.com>
---
changes since v4:
  - Add _fixed_by_git_commit for noting the required fix.
  - Abort the test when mkfs fails.
changes since v3:
  - Flatten out nested pointer dereferencing and typecasting.
changes since v2:
  - Use glibc functions to convert endian.
  - Copy the structure definitions from xfsdump source files.
changes since RFC v1:
  - Skip the test if xfsrestore does not support '-x' flag.
  - Create a corrupted dump by overwriting a root inode number in a dump
    file with a new tool instead of checking in a binary dump file.

 common/dump             |   2 +-
 common/xfs              |   6 ++
 src/Makefile            |   2 +-
 src/fake-dump-rootino.c | 224 ++++++++++++++++++++++++++++++++++++++++
 tests/xfs/554           |  75 ++++++++++++++
 tests/xfs/554.out       |  40 +++++++
 6 files changed, 347 insertions(+), 2 deletions(-)
 create mode 100644 src/fake-dump-rootino.c
 create mode 100755 tests/xfs/554
 create mode 100644 tests/xfs/554.out

diff --git a/common/dump b/common/dump
index 8e0446d9..50b2ba03 100644
--- a/common/dump
+++ b/common/dump
@@ -1003,7 +1003,7 @@ _parse_restore_args()
         --no-check-quota)
             do_quota_check=false
             ;;
-	-K|-R)
+	-K|-R|-x)
 	    restore_args="$restore_args $1"
             ;;
 	*)
diff --git a/common/xfs b/common/xfs
index e1c15d3d..8334880e 100644
--- a/common/xfs
+++ b/common/xfs
@@ -1402,3 +1402,9 @@ _xfs_filter_mkfs()
 		print STDOUT "realtime =RDEV extsz=XXX blocks=XXX, rtextents=XXX\n";
 	}'
 }
+
+_require_xfsrestore_xflag()
+{
+	$XFSRESTORE_PROG -h 2>&1 | grep -q -e '-x' || \
+			_notrun 'xfsrestore does not support -x flag.'
+}
diff --git a/src/Makefile b/src/Makefile
index 5f565e73..afdf6b30 100644
--- a/src/Makefile
+++ b/src/Makefile
@@ -19,7 +19,7 @@ TARGETS = dirstress fill fill2 getpagesize holes lstat64 \
 	t_ofd_locks t_mmap_collision mmap-write-concurrent \
 	t_get_file_time t_create_short_dirs t_create_long_dirs t_enospc \
 	t_mmap_writev_overlap checkpoint_journal mmap-rw-fault allocstale \
-	t_mmap_cow_memory_failure
+	t_mmap_cow_memory_failure fake-dump-rootino
 
 LINUX_TARGETS = xfsctl bstat t_mtab getdevicesize preallo_rw_pattern_reader \
 	preallo_rw_pattern_writer ftrunc trunc fs_perms testx looptest \
diff --git a/src/fake-dump-rootino.c b/src/fake-dump-rootino.c
new file mode 100644
index 00000000..8a30dffd
--- /dev/null
+++ b/src/fake-dump-rootino.c
@@ -0,0 +1,224 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2022 Fujitsu Limited.  All Rights Reserved. */
+#define _LARGEFILE64_SOURCE
+#include <endian.h>
+#include <fcntl.h>
+#include <stdint.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <sys/mman.h>
+#include <unistd.h>
+#include <uuid/uuid.h>
+
+// Definitions from xfsdump
+#define PGSZLOG2	12
+#define PGSZ		(1 << PGSZLOG2)
+#define GLOBAL_HDR_SZ		PGSZ
+#define GLOBAL_HDR_MAGIC_SZ	8
+#define GLOBAL_HDR_STRING_SZ	0x100
+#define GLOBAL_HDR_TIME_SZ	4
+#define GLOBAL_HDR_UUID_SZ	0x10
+typedef int32_t time32_t;
+struct global_hdr {
+	char gh_magic[GLOBAL_HDR_MAGIC_SZ];		/*   8    8 */
+		/* unique signature of xfsdump */
+	uint32_t gh_version;				/*   4    c */
+		/* header version */
+	uint32_t gh_checksum;				/*   4   10 */
+		/* 32-bit unsigned additive inverse of entire header */
+	time32_t gh_timestamp;				/*   4   14 */
+		/* time32_t of dump */
+	char gh_pad1[4];				/*   4   18 */
+		/* alignment */
+	uint64_t gh_ipaddr;				/*   8   20 */
+		/* from gethostid(2), room for expansion */
+	uuid_t gh_dumpid;				/*  10   30 */
+		/* ID of dump session	 */
+	char gh_pad2[0xd0];				/*  d0  100 */
+		/* alignment */
+	char gh_hostname[GLOBAL_HDR_STRING_SZ];	/* 100  200 */
+		/* from gethostname(2) */
+	char gh_dumplabel[GLOBAL_HDR_STRING_SZ];	/* 100  300 */
+		/* label of dump session */
+	char gh_pad3[0x100];				/* 100  400 */
+		/* padding */
+	char gh_upper[GLOBAL_HDR_SZ - 0x400];		/* c00 1000 */
+		/* header info private to upper software layers */
+};
+typedef struct global_hdr global_hdr_t;
+
+#define sizeofmember( t, m )	sizeof( ( ( t * )0 )->m )
+
+#define DRIVE_HDR_SZ		sizeofmember(global_hdr_t, gh_upper)
+struct drive_hdr {
+	uint32_t dh_drivecnt;				/*   4    4 */
+		/* number of drives used to dump the fs */
+	uint32_t dh_driveix;				/*   4    8 */
+		/* 0-based index of the drive used to dump this stream */
+	int32_t dh_strategyid;				/*   4    c */
+		/* ID of the drive strategy used to produce this dump */
+	char dh_pad1[0x1f4];				/* 1f4  200 */
+		/* padding */
+	char dh_specific[0x200];			/* 200  400 */
+		/* drive strategy-specific info */
+	char dh_upper[DRIVE_HDR_SZ - 0x400];		/* 800  c00 */
+		/* header info private to upper software layers */
+};
+typedef struct drive_hdr drive_hdr_t;
+
+#define MEDIA_HDR_SZ		sizeofmember(drive_hdr_t, dh_upper)
+struct media_hdr {
+	char mh_medialabel[GLOBAL_HDR_STRING_SZ];	/* 100  100 */
+		/* label of media object containing file */
+	char mh_prevmedialabel[GLOBAL_HDR_STRING_SZ];	/* 100  200 */
+		/* label of upstream media object */
+	char mh_pad1[GLOBAL_HDR_STRING_SZ];		/* 100  300 */
+		/* in case more labels needed */
+	uuid_t mh_mediaid;				/*  10  310 */
+		/* ID of media object 	*/
+	uuid_t mh_prevmediaid;				/*  10  320 */
+		/* ID of upstream media object */
+	char mh_pad2[GLOBAL_HDR_UUID_SZ];		/*  10  330 */
+		/* in case more IDs needed */
+	uint32_t mh_mediaix;				/*   4  334 */
+		/* 0-based index of this media object within the dump stream */
+	uint32_t mh_mediafileix;			/*   4  338 */
+		/* 0-based index of this file within this media object */
+	uint32_t mh_dumpfileix;			/*   4  33c */
+		/* 0-based index of this file within this dump stream */
+	uint32_t mh_dumpmediafileix;			/*   4  340 */
+		/* 0-based index of file within dump stream and media object */
+	uint32_t mh_dumpmediaix;			/*   4  344 */
+		/* 0-based index of this dump within the media object */
+	int32_t mh_strategyid;				/*   4  348 */
+		/* ID of the media strategy used to produce this dump */
+	char mh_pad3[0x38];				/*  38  380 */
+		/* padding */
+	char mh_specific[0x80];			/*  80  400 */
+		/* media strategy-specific info */
+	char mh_upper[MEDIA_HDR_SZ - 0x400];		/* 400  800 */
+		/* header info private to upper software layers */
+};
+typedef struct media_hdr media_hdr_t;
+
+#define CONTENT_HDR_SZ		sizeofmember(media_hdr_t, mh_upper)
+#define CONTENT_HDR_FSTYPE_SZ	16
+#define CONTENT_STATSZ		160 /* must match dlog.h DLOG_MULTI_STATSZ */
+struct content_hdr {
+	char ch_mntpnt[GLOBAL_HDR_STRING_SZ];		/* 100  100 */
+		/* full pathname of fs mount point */
+	char ch_fsdevice[GLOBAL_HDR_STRING_SZ];	/* 100  200 */
+		/* full pathname of char device containing fs */
+	char  ch_pad1[GLOBAL_HDR_STRING_SZ];		/* 100  300 */
+		/* in case another label is needed */
+	char ch_fstype[CONTENT_HDR_FSTYPE_SZ];	/*  10  310 */
+		/* from fsid.h */
+	uuid_t ch_fsid;					/*  10  320 */
+		/* file system uuid */
+	char  ch_pad2[GLOBAL_HDR_UUID_SZ];		/*  10  330 */
+		/* in case another id is needed */
+	char ch_pad3[8];				/*   8  338 */
+		/* padding */
+	int32_t ch_strategyid;				/*   4  33c */
+		/* ID of the content strategy used to produce this dump */
+	char ch_pad4[4];				/*   4  340 */
+		/* alignment */
+	char ch_specific[0xc0];			/*  c0  400 */
+		/* content strategy-specific info */
+};
+typedef struct content_hdr content_hdr_t;
+
+typedef uint64_t xfs_ino_t;
+struct startpt {
+	xfs_ino_t sp_ino;		/* first inode to dump */
+	off64_t sp_offset;	/* bytes to skip in file data fork */
+	int32_t sp_flags;
+	int32_t sp_pad1;
+};
+typedef struct startpt startpt_t;
+
+#define CONTENT_INODE_HDR_SZ  sizeofmember(content_hdr_t, ch_specific)
+struct content_inode_hdr {
+	int32_t cih_mediafiletype;			/*   4   4 */
+		/* dump media file type: see #defines below */
+	int32_t cih_dumpattr;				/*   4   8 */
+		/* dump attributes: see #defines below */
+	int32_t cih_level;				/*   4   c */
+		/* dump level */
+	char pad1[4];					/*   4  10 */
+		/* alignment */
+	time32_t cih_last_time;				/*   4  14 */
+		/* if an incremental, time of previous dump at a lesser level */
+	time32_t cih_resume_time;			/*   4  18 */
+		/* if a resumed dump, time of interrupted dump */
+	xfs_ino_t cih_rootino;				/*   8  20 */
+		/* root inode number */
+	uuid_t cih_last_id;				/*  10  30 */
+		/* if an incremental, uuid of prev dump */
+	uuid_t cih_resume_id;				/*  10  40 */
+		/* if a resumed dump, uuid of interrupted dump */
+	startpt_t cih_startpt;				/*  18  58 */
+		/* starting point of media file contents */
+	startpt_t cih_endpt;				/*  18  70 */
+		/* starting point of next stream */
+	uint64_t cih_inomap_hnkcnt;			/*   8  78 */
+
+	uint64_t cih_inomap_segcnt;			/*   8  80 */
+
+	uint64_t cih_inomap_dircnt;			/*   8  88 */
+
+	uint64_t cih_inomap_nondircnt;			/*   8  90 */
+
+	xfs_ino_t cih_inomap_firstino;			/*   8  98 */
+
+	xfs_ino_t cih_inomap_lastino;			/*   8  a0 */
+
+	uint64_t cih_inomap_datasz;			/*   8  a8 */
+		/* bytes of non-metadata dumped */
+	char cih_pad2[CONTENT_INODE_HDR_SZ - 0xa8];	/*  18  c0 */
+		/* padding */
+};
+typedef struct content_inode_hdr content_inode_hdr_t;
+// End of definitions from xfsdump
+
+int main(int argc, char *argv[]) {
+
+	if (argc < 3) {
+		fprintf(stderr, "Usage: %s <path/to/dumpfile> <fake rootino>\n", argv[0]);
+		exit(1);
+	}
+
+	const char *filepath = argv[1];
+	const uint64_t fake_root_ino = (uint64_t)strtol(argv[2], NULL, 10);
+
+	int fd = open(filepath, O_RDWR);
+	if (fd < 0) {
+		perror("open");
+		exit(1);
+	}
+	global_hdr_t *header = mmap(NULL, GLOBAL_HDR_SZ, PROT_READ|PROT_WRITE, MAP_SHARED, fd, 0);
+	if (header == MAP_FAILED) {
+		perror("mmap");
+		exit(1);
+	}
+
+	drive_hdr_t *dh = (drive_hdr_t *)header->gh_upper;
+	media_hdr_t *mh = (media_hdr_t *)dh->dh_upper;
+	content_hdr_t *ch = (content_hdr_t *)mh->mh_upper;
+	content_inode_hdr_t *cih = (content_inode_hdr_t *)ch->ch_specific;
+	uint64_t *rootino_ptr = &cih->cih_rootino;
+
+	int32_t checksum = (int32_t)be32toh(header->gh_checksum);
+	uint64_t orig_rootino = be64toh(*rootino_ptr);
+
+	// Fake root inode number
+	*rootino_ptr = htobe64(fake_root_ino);
+
+	// Update checksum along with overwriting rootino.
+	uint64_t gap = orig_rootino - fake_root_ino;
+	checksum += (gap >> 32) + (gap & 0x00000000ffffffff);
+	header->gh_checksum = htobe32(checksum);
+
+	munmap(header, GLOBAL_HDR_SZ);
+	close(fd);
+}
diff --git a/tests/xfs/554 b/tests/xfs/554
new file mode 100755
index 00000000..7f180a71
--- /dev/null
+++ b/tests/xfs/554
@@ -0,0 +1,75 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2022 Fujitsu Limited. All Rights Reserved.
+#
+# FS QA Test No. 554
+#
+# Create a filesystem which contains an inode with a lower number
+# than the root inode. Set the lower number to a dump file as the root inode
+# and ensure that 'xfsrestore -x' handles this wrong inode.
+#
+. ./common/preamble
+_begin_fstest auto quick dump
+
+# Import common functions.
+. ./common/dump
+
+_supported_fs xfs
+_fixed_by_git_commit xfsdump \
+	"XXXXXXXXXXXX xfsrestore: fix rootdir due to xfsdump bulkstat misuse"
+_require_xfs_io_command "falloc"
+_require_scratch
+_require_xfsrestore_xflag
+
+# A large stripe unit will put the root inode out quite far
+# due to alignment, leaving free blocks ahead of it.
+_scratch_mkfs_xfs -d sunit=1024,swidth=1024 > $seqres.full 2>&1 || _fail "mkfs failed"
+
+# Mounting /without/ a stripe should allow inodes to be allocated
+# in lower free blocks, without the stripe alignment.
+_scratch_mount -o sunit=0,swidth=0
+
+root_inum=$(stat -c %i $SCRATCH_MNT)
+
+# Consume space after the root inode so that the blocks before
+# root look "close" for the next inode chunk allocation
+$XFS_IO_PROG -f -c "falloc 0 16m" $SCRATCH_MNT/fillfile
+
+# And make a bunch of inodes until we (hopefully) get one lower
+# than root, in a new inode chunk.
+echo "root_inum: $root_inum" >> $seqres.full
+for i in $(seq 0 4096) ; do
+	fname=$SCRATCH_MNT/$(printf "FILE_%03d" $i)
+	touch $fname
+	inum=$(stat -c "%i" $fname)
+	[[ $inum -lt $root_inum ]] && break
+done
+
+echo "created: $inum" >> $seqres.full
+
+[[ $inum -lt $root_inum ]] || _notrun "Could not set up test"
+
+# Now try a dump and restore. Cribbed from xfs/068
+_create_dumpdir_stress
+
+echo -n "Before: " >> $seqres.full
+_count_dumpdir_files | tee $tmp.before >> $seqres.full
+
+_do_dump_file
+
+# Set the wrong root inode number to the dump file
+# as problematic xfsdump used to do.
+$here/src/fake-dump-rootino $dump_file $inum
+
+_do_restore_file -x | \
+sed -e "s/rootino #${inum}/rootino #FAKENO/g" \
+	-e "s/# to ${root_inum}/# to ROOTNO/g" \
+	-e "/entries processed$/s/[0-9][0-9]*/NUM/g"
+
+echo -n "After: " >> $seqres.full
+_count_restoredir_files | tee $tmp.after >> $seqres.full
+diff -u $tmp.before $tmp.after
+
+# success, all done
+status=0
+exit
diff --git a/tests/xfs/554.out b/tests/xfs/554.out
new file mode 100644
index 00000000..c5e8c4c5
--- /dev/null
+++ b/tests/xfs/554.out
@@ -0,0 +1,40 @@
+QA output created by 554
+Creating directory system to dump using fsstress.
+
+-----------------------------------------------
+fsstress : -f link=10 -f creat=10 -f mkdir=10 -f truncate=5 -f symlink=10
+-----------------------------------------------
+Dumping to file...
+xfsdump  -f DUMP_FILE -M stress_tape_media -L stress_554 SCRATCH_MNT
+xfsdump: using file dump (drive_simple) strategy
+xfsdump: level 0 dump of HOSTNAME:SCRATCH_MNT
+xfsdump: dump date: DATE
+xfsdump: session id: ID
+xfsdump: session label: "stress_554"
+xfsdump: ino map <PHASES>
+xfsdump: ino map construction complete
+xfsdump: estimated dump size: NUM bytes
+xfsdump: /var/xfsdump/inventory created
+xfsdump: creating dump session media file 0 (media 0, file 0)
+xfsdump: dumping ino map
+xfsdump: dumping directories
+xfsdump: dumping non-directory files
+xfsdump: ending media file
+xfsdump: media file size NUM bytes
+xfsdump: dump size (non-dir files) : NUM bytes
+xfsdump: dump complete: SECS seconds elapsed
+xfsdump: Dump Status: SUCCESS
+Restoring from file...
+xfsrestore  -x -f DUMP_FILE  -L stress_554 RESTORE_DIR
+xfsrestore: using file dump (drive_simple) strategy
+xfsrestore: using online session inventory
+xfsrestore: searching media for directory dump
+xfsrestore: examining media file 0
+xfsrestore: reading directories
+xfsrestore: found fake rootino #FAKENO, will fix.
+xfsrestore: fix root # to ROOTNO (bind mount?)
+xfsrestore: NUM directories and NUM entries processed
+xfsrestore: directory post-processing
+xfsrestore: restoring non-directory files
+xfsrestore: restore complete: SECS seconds elapsed
+xfsrestore: Restore Status: SUCCESS
-- 
2.37.3