[U-Boot] [PATCH v2 1/4] fs/ext4: Fix group descriptor checksum calculation
The current code doesn't compute the group descriptor checksum correctly for the filesystems that e2fsprogs 1.43.4 creates (they have 'Group descriptor size: 64' as reported by tune2fs). Extend the checksum calculation to be done as ext4_group_desc_csum() does in Linux.
This fixes these errors in dmesg from running fs-test.sh and makes it succeed again:
[1671902.620699] EXT4-fs (loop1): ext4_check_descriptors: Checksum for group 0 failed (35782!=10965) [1671902.620706] EXT4-fs (loop1): group descriptors corrupted!
Signed-off-by: Tuomas Tynkkynen tuomas.tynkkynen@iki.fi --- v2: New patch --- fs/ext4/ext4_common.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/fs/ext4/ext4_common.c b/fs/ext4/ext4_common.c index 621c61e5c7..31952f48b9 100644 --- a/fs/ext4/ext4_common.c +++ b/fs/ext4/ext4_common.c @@ -432,6 +432,10 @@ uint16_t ext4fs_checksum_update(uint32_t i) crc = ext2fs_crc16(crc, desc, offset); offset += sizeof(desc->bg_checksum); /* skip checksum */ assert(offset == sizeof(*desc)); + if (offset < fs->gdsize) { + crc = ext2fs_crc16(crc, (__u8 *)desc + offset, + fs->gdsize - offset); + } }
return crc;
Currently we can only test FAT32 which is the default FAT version that mkfs.vfat creates by default. Instead make it explicitly create either a FAT16 or a FAT32 volume. This allows us to exercise more code, for instance the root directory handling is done differently in FAT32 than the older FATs.
Adding FAT12 support is a much bigger job since the test creates a 2.5GB file and the FAT12 maximum partition size is way smaller than that.
Signed-off-by: Tuomas Tynkkynen tuomas.tynkkynen@iki.fi --- v2: Update result summary in the header --- test/fs/fs-test.sh | 55 +++++++++++++++++++++++++++++++++++------------------- 1 file changed, 36 insertions(+), 19 deletions(-)
diff --git a/test/fs/fs-test.sh b/test/fs/fs-test.sh index b19486419e..0ac34983a1 100755 --- a/test/fs/fs-test.sh +++ b/test/fs/fs-test.sh @@ -12,11 +12,15 @@ # fs-test.sb.ext4.out: Summary: PASS: 23 FAIL: 0 # fs-test.ext4.out: Summary: PASS: 23 FAIL: 0 # fs-test.fs.ext4.out: Summary: PASS: 23 FAIL: 0 -# FAT tests: -# fs-test.sb.fat.out: Summary: PASS: 23 FAIL: 0 -# fs-test.fat.out: Summary: PASS: 20 FAIL: 3 -# fs-test.fs.fat.out: Summary: PASS: 20 FAIL: 3 -# Total Summary: TOTAL PASS: 132 TOTAL FAIL: 6 +# FAT16 tests: +# fs-test.sb.fat16.out: Summary: PASS: 23 FAIL: 0 +# fs-test.fat16.out: Summary: PASS: 20 FAIL: 3 +# fs-test.fs.fat16.out: Summary: PASS: 20 FAIL: 3 +# FAT32 tests: +# fs-test.sb.fat32.out: Summary: PASS: 23 FAIL: 0 +# fs-test.fat32.out: Summary: PASS: 20 FAIL: 3 +# fs-test.fs.fat32.out: Summary: PASS: 20 FAIL: 3 +# Total Summary: TOTAL PASS: 195 TOTAL FAIL: 12
# pre-requisite binaries list. PREREQ_BINS="md5sum mkfs mount umount dd fallocate mkdir" @@ -41,7 +45,7 @@ SMALL_FILE="1MB.file" BIG_FILE="2.5GB.file"
# $MD5_FILE will have the expected md5s when we do the test -# They shall have a suffix which represents their file system (ext4/fat) +# They shall have a suffix which represents their file system (ext4/fat16/...) MD5_FILE="${OUT_DIR}/md5s.list"
# $OUT shall be the prefix of the test output. Their suffix will be .out @@ -104,15 +108,25 @@ function prepare_env() { }
# 1st parameter is the name of the image file to be created -# 2nd parameter is the filesystem - fat ext4 etc +# 2nd parameter is the filesystem - fat16 ext4 etc # -F cant be used with fat as it means something else. function create_image() { # Create image if not already present - saves time, while debugging - if [ "$2" = "ext4" ]; then + case "$2" in + fat16) + MKFS_OPTION="-F 16" + FS_TYPE="fat" + ;; + fat32) + MKFS_OPTION="-F 32" + FS_TYPE="fat" + ;; + ext4) MKFS_OPTION="-F" - else - MKFS_OPTION="" - fi + FS_TYPE="ext4" + ;; + esac + if [ ! -f "$1" ]; then fallocate -l 3G "$1" &> /dev/null if [ $? -ne 0 ]; then @@ -123,8 +137,8 @@ function create_image() { exit $? fi fi - mkfs -t "$2" $MKFS_OPTION "$1" &> /dev/null - if [ $? -ne 0 -a "$2" = "fat" ]; then + mkfs -t "$FS_TYPE" $MKFS_OPTION "$1" &> /dev/null + if [ $? -ne 0 -a "$FS_TYPE" = "fat" ]; then # If we fail and we did fat, try vfat. mkfs -t vfat $MKFS_OPTION "$1" &> /dev/null fi @@ -136,7 +150,7 @@ function create_image() { }
# 1st parameter is image file -# 2nd parameter is file system type - fat/ext4 +# 2nd parameter is file system type - fat16/ext4/... # 3rd parameter is name of small file # 4th parameter is name of big file # 5th parameter is fs/nonfs/sb - to dictate generic fs commands or @@ -149,7 +163,7 @@ function test_image() { length="0x00100000"
case "$2" in - fat) + fat*) FPATH="" PREFIX="fat" WRITE="write" @@ -550,7 +564,7 @@ TOTAL_PASS=0 # In each loop, for a given file system image, we test both the # fs command, like load/size/write, the file system specific command # like: ext4load/ext4size/ext4write and the sb load/ls/save commands. -for fs in ext4 fat; do +for fs in ext4 fat16 fat32; do
echo "Creating $fs image if not already present." IMAGE=${IMG}.${fs}.img @@ -563,11 +577,14 @@ for fs in ext4 fat; do
# Lets mount the image and test sb hostfs commands mkdir -p "$MOUNT_DIR" - if [ "$fs" = "fat" ]; then + case "$fs" in + fat*) uid="uid=`id -u`" - else + ;; + *) uid="" - fi + ;; + esac sudo mount -o loop,rw,$uid "$IMAGE" "$MOUNT_DIR" sudo chmod 777 "$MOUNT_DIR"
On Mon, Sep 25, 2017 at 10:06:32PM +0300, Tuomas Tynkkynen wrote:
Currently we can only test FAT32 which is the default FAT version that mkfs.vfat creates by default. Instead make it explicitly create either a FAT16 or a FAT32 volume. This allows us to exercise more code, for instance the root directory handling is done differently in FAT32 than the older FATs.
Adding FAT12 support is a much bigger job since the test creates a 2.5GB file and the FAT12 maximum partition size is way smaller than that.
Signed-off-by: Tuomas Tynkkynen tuomas.tynkkynen@iki.fi
Reviewed-by: Tom Rini trini@konsulko.com
On Mon, Sep 25, 2017 at 10:06:32PM +0300, Tuomas Tynkkynen wrote:
Currently we can only test FAT32 which is the default FAT version that mkfs.vfat creates by default. Instead make it explicitly create either a FAT16 or a FAT32 volume. This allows us to exercise more code, for instance the root directory handling is done differently in FAT32 than the older FATs.
Adding FAT12 support is a much bigger job since the test creates a 2.5GB file and the FAT12 maximum partition size is way smaller than that.
Signed-off-by: Tuomas Tynkkynen tuomas.tynkkynen@iki.fi Reviewed-by: Tom Rini trini@konsulko.com
Applied to u-boot/master, thanks!
If we end up back in the root directory via a '..' directory entry, set itr->is_root accordingly. Failing to do that gives spews like "Invalid FAT entry" and being unable to access directory entries located past the first cluster of the root directory.
Fixes: 8eafae209c35 ("fat/fs: convert to directory iterators") Reviewed-by: Tom Rini trini@konsulko.com Signed-off-by: Tuomas Tynkkynen tuomas.tynkkynen@iki.fi --- v2: Add Reviewed-by --- fs/fat/fat.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/fs/fat/fat.c b/fs/fat/fat.c index 36a309c73c..3d3e17e8fa 100644 --- a/fs/fat/fat.c +++ b/fs/fat/fat.c @@ -710,13 +710,14 @@ static void fat_itr_child(fat_itr *itr, fat_itr *parent) itr->fsdata = parent->fsdata; if (clustnum > 0) { itr->clust = clustnum; + itr->is_root = 0; } else { itr->clust = parent->fsdata->root_cluster; + itr->is_root = 1; } itr->dent = NULL; itr->remaining = 0; itr->last_cluster = 0; - itr->is_root = 0; }
static void *next_cluster(fat_itr *itr)
On Mon, Sep 25, 2017 at 10:06:33PM +0300, Tuomas Tynkkynen wrote:
If we end up back in the root directory via a '..' directory entry, set itr->is_root accordingly. Failing to do that gives spews like "Invalid FAT entry" and being unable to access directory entries located past the first cluster of the root directory.
Fixes: 8eafae209c35 ("fat/fs: convert to directory iterators") Reviewed-by: Tom Rini trini@konsulko.com Signed-off-by: Tuomas Tynkkynen tuomas.tynkkynen@iki.fi
Applied to u-boot/master, thanks!
The previous commit fixed a problem in FAT code where going back to the root directory using '..' wouldn't work correctly on FAT12 or FAT16. Add a test to exercise this case (which was once fixed in commit 18a10d46f26 "fat: handle paths that include ../" but reintroduced due to the directory iterator refactoring).
This test only very barely catches the problem - without the fix the size command still gives valid output but the additional spurious "Invalid FAT entry" error message makes it not get caught in the 'egrep -A3 ' output. I tried to make a proper test that grows the root directory to two clusters lots of with dummy files but that causes the write tests to crash the sandbox totally...
Signed-off-by: Tuomas Tynkkynen tuomas.tynkkynen@iki.fi --- v2: - Increase context of the 'ls' test to also pass on ext4 (due to the added SUBDIR subdirectory) - Update the result summary in the header --- test/fs/fs-test.sh | 38 ++++++++++++++++++++++++-------------- 1 file changed, 24 insertions(+), 14 deletions(-)
diff --git a/test/fs/fs-test.sh b/test/fs/fs-test.sh index 0ac34983a1..20d5dd8a47 100755 --- a/test/fs/fs-test.sh +++ b/test/fs/fs-test.sh @@ -9,18 +9,18 @@ # It currently tests the fs/sb and native commands for ext4 and fat partitions # Expected results are as follows: # EXT4 tests: -# fs-test.sb.ext4.out: Summary: PASS: 23 FAIL: 0 -# fs-test.ext4.out: Summary: PASS: 23 FAIL: 0 -# fs-test.fs.ext4.out: Summary: PASS: 23 FAIL: 0 +# fs-test.sb.ext4.out: Summary: PASS: 24 FAIL: 0 +# fs-test.ext4.out: Summary: PASS: 24 FAIL: 0 +# fs-test.fs.ext4.out: Summary: PASS: 24 FAIL: 0 # FAT16 tests: -# fs-test.sb.fat16.out: Summary: PASS: 23 FAIL: 0 -# fs-test.fat16.out: Summary: PASS: 20 FAIL: 3 -# fs-test.fs.fat16.out: Summary: PASS: 20 FAIL: 3 +# fs-test.sb.fat16.out: Summary: PASS: 24 FAIL: 0 +# fs-test.fat16.out: Summary: PASS: 21 FAIL: 3 +# fs-test.fs.fat16.out: Summary: PASS: 21 FAIL: 3 # FAT32 tests: -# fs-test.sb.fat32.out: Summary: PASS: 23 FAIL: 0 -# fs-test.fat32.out: Summary: PASS: 20 FAIL: 3 -# fs-test.fs.fat32.out: Summary: PASS: 20 FAIL: 3 -# Total Summary: TOTAL PASS: 195 TOTAL FAIL: 12 +# fs-test.sb.fat32.out: Summary: PASS: 24 FAIL: 0 +# fs-test.fat32.out: Summary: PASS: 21 FAIL: 3 +# fs-test.fs.fat32.out: Summary: PASS: 21 FAIL: 3 +# Total Summary: TOTAL PASS: 204 TOTAL FAIL: 12
# pre-requisite binaries list. PREREQ_BINS="md5sum mkfs mount umount dd fallocate mkdir" @@ -229,10 +229,14 @@ ${PREFIX}ls host${SUFFIX} $6 # We want ${PREFIX}size host 0:0 $3 for host commands and # sb size hostfs - $3 for hostfs commands. # 1MB is 0x0010 0000 -# Test Case 2 - size of small file +# Test Case 2a - size of small file ${PREFIX}size host${SUFFIX} ${FPATH}$FILE_SMALL printenv filesize setenv filesize +# Test Case 2b - size of small file via a path using '..' +${PREFIX}size host${SUFFIX} ${FPATH}SUBDIR/../$FILE_SMALL +printenv filesize +setenv filesize
# 2.5GB (1024*1024*2500) is 0x9C40 0000 # Test Case 3 - size of big file @@ -347,6 +351,9 @@ function create_files() { mkdir -p "$MOUNT_DIR" sudo mount -o loop,rw "$1" "$MOUNT_DIR"
+ # Create a subdirectory. + sudo mkdir -p "$MOUNT_DIR/SUBDIR" + # Create big file in this image. # Note that we work only on the start 1MB, couple MBs in the 2GB range # and the last 1 MB of the huge 2.5GB file. @@ -453,16 +460,19 @@ function check_results() { FAIL=0
# Check if the ls is showing correct results for 2.5 gb file - grep -A6 "Test Case 1 " "$1" | egrep -iq "2621440000 *$4" + grep -A7 "Test Case 1 " "$1" | egrep -iq "2621440000 *$4" pass_fail "TC1: ls of $4"
# Check if the ls is showing correct results for 1 mb file - grep -A6 "Test Case 1 " "$1" | egrep -iq "1048576 *$3" + grep -A7 "Test Case 1 " "$1" | egrep -iq "1048576 *$3" pass_fail "TC1: ls of $3"
# Check size command on 1MB.file - egrep -A3 "Test Case 2 " "$1" | grep -q "filesize=100000" + egrep -A3 "Test Case 2a " "$1" | grep -q "filesize=100000" pass_fail "TC2: size of $3" + # Check size command on 1MB.file via a path using '..' + egrep -A3 "Test Case 2b " "$1" | grep -q "filesize=100000" + pass_fail "TC2: size of $3 via a path using '..'"
# Check size command on 2.5GB.file egrep -A3 "Test Case 3 " "$1" | grep -q "filesize=9c400000"
On Mon, Sep 25, 2017 at 10:06:34PM +0300, Tuomas Tynkkynen wrote:
The previous commit fixed a problem in FAT code where going back to the root directory using '..' wouldn't work correctly on FAT12 or FAT16. Add a test to exercise this case (which was once fixed in commit 18a10d46f26 "fat: handle paths that include ../" but reintroduced due to the directory iterator refactoring).
This test only very barely catches the problem - without the fix the size command still gives valid output but the additional spurious "Invalid FAT entry" error message makes it not get caught in the 'egrep -A3 ' output. I tried to make a proper test that grows the root directory to two clusters lots of with dummy files but that causes the write tests to crash the sandbox totally...
Signed-off-by: Tuomas Tynkkynen tuomas.tynkkynen@iki.fi
Reviewed-by: Tom Rini trini@konsulko.com
On Mon, Sep 25, 2017 at 10:06:34PM +0300, Tuomas Tynkkynen wrote:
The previous commit fixed a problem in FAT code where going back to the root directory using '..' wouldn't work correctly on FAT12 or FAT16. Add a test to exercise this case (which was once fixed in commit 18a10d46f26 "fat: handle paths that include ../" but reintroduced due to the directory iterator refactoring).
This test only very barely catches the problem - without the fix the size command still gives valid output but the additional spurious "Invalid FAT entry" error message makes it not get caught in the 'egrep -A3 ' output. I tried to make a proper test that grows the root directory to two clusters lots of with dummy files but that causes the write tests to crash the sandbox totally...
Signed-off-by: Tuomas Tynkkynen tuomas.tynkkynen@iki.fi Reviewed-by: Tom Rini trini@konsulko.com
Applied to u-boot/master, thanks!
On Mon, Sep 25, 2017 at 10:06:31PM +0300, Tuomas Tynkkynen wrote:
The current code doesn't compute the group descriptor checksum correctly for the filesystems that e2fsprogs 1.43.4 creates (they have 'Group descriptor size: 64' as reported by tune2fs). Extend the checksum calculation to be done as ext4_group_desc_csum() does in Linux.
This fixes these errors in dmesg from running fs-test.sh and makes it succeed again:
[1671902.620699] EXT4-fs (loop1): ext4_check_descriptors: Checksum for group 0 failed (35782!=10965) [1671902.620706] EXT4-fs (loop1): group descriptors corrupted!
Signed-off-by: Tuomas Tynkkynen tuomas.tynkkynen@iki.fi
v2: New patch
fs/ext4/ext4_common.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/fs/ext4/ext4_common.c b/fs/ext4/ext4_common.c index 621c61e5c7..31952f48b9 100644 --- a/fs/ext4/ext4_common.c +++ b/fs/ext4/ext4_common.c @@ -432,6 +432,10 @@ uint16_t ext4fs_checksum_update(uint32_t i) crc = ext2fs_crc16(crc, desc, offset); offset += sizeof(desc->bg_checksum); /* skip checksum */ assert(offset == sizeof(*desc));
if (offset < fs->gdsize) {crc = ext2fs_crc16(crc, (__u8 *)desc + offset,fs->gdsize - offset);}
This would be feb0ab32a57e4e6c8b24f6fb68f0ce08efe4603c from the kernel? So shouldn't we have le16_to_cpu on fs->gdsize ? Or did I read over the 'git log -p' output in the kernel too quickly? Thanks!
On 09/26/2017 06:25 PM, Tom Rini wrote:
On Mon, Sep 25, 2017 at 10:06:31PM +0300, Tuomas Tynkkynen wrote:
The current code doesn't compute the group descriptor checksum correctly for the filesystems that e2fsprogs 1.43.4 creates (they have 'Group descriptor size: 64' as reported by tune2fs). Extend the checksum calculation to be done as ext4_group_desc_csum() does in Linux.
This fixes these errors in dmesg from running fs-test.sh and makes it succeed again:
[1671902.620699] EXT4-fs (loop1): ext4_check_descriptors: Checksum for group 0 failed (35782!=10965) [1671902.620706] EXT4-fs (loop1): group descriptors corrupted!
Signed-off-by: Tuomas Tynkkynen tuomas.tynkkynen@iki.fi
v2: New patch
fs/ext4/ext4_common.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/fs/ext4/ext4_common.c b/fs/ext4/ext4_common.c index 621c61e5c7..31952f48b9 100644 --- a/fs/ext4/ext4_common.c +++ b/fs/ext4/ext4_common.c @@ -432,6 +432,10 @@ uint16_t ext4fs_checksum_update(uint32_t i) crc = ext2fs_crc16(crc, desc, offset); offset += sizeof(desc->bg_checksum); /* skip checksum */ assert(offset == sizeof(*desc));
if (offset < fs->gdsize) {crc = ext2fs_crc16(crc, (__u8 *)desc + offset,fs->gdsize - offset);}This would be feb0ab32a57e4e6c8b24f6fb68f0ce08efe4603c from the kernel?
No, it goes further back, to 2007 even(!): 717d50e4971b81b96c0199c91cdf0
So shouldn't we have le16_to_cpu on fs->gdsize ? Or did I read over the 'git log -p' output in the kernel too quickly? Thanks!
No, because fs->gdsize is already endian-swapped elsewhere:
2345 fs->gdsize = le32_to_cpu(data->sblock.feature_incompat) & 2346 EXT4_FEATURE_INCOMPAT_64BIT ? 2347 le16_to_cpu(data->sblock.descriptor_size) : 32;
On Tue, Sep 26, 2017 at 07:04:06PM +0300, Tuomas Tynkkynen wrote:
On 09/26/2017 06:25 PM, Tom Rini wrote:
On Mon, Sep 25, 2017 at 10:06:31PM +0300, Tuomas Tynkkynen wrote:
The current code doesn't compute the group descriptor checksum correctly for the filesystems that e2fsprogs 1.43.4 creates (they have 'Group descriptor size: 64' as reported by tune2fs). Extend the checksum calculation to be done as ext4_group_desc_csum() does in Linux.
This fixes these errors in dmesg from running fs-test.sh and makes it succeed again:
[1671902.620699] EXT4-fs (loop1): ext4_check_descriptors: Checksum for group 0 failed (35782!=10965) [1671902.620706] EXT4-fs (loop1): group descriptors corrupted!
Signed-off-by: Tuomas Tynkkynen tuomas.tynkkynen@iki.fi
v2: New patch
fs/ext4/ext4_common.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/fs/ext4/ext4_common.c b/fs/ext4/ext4_common.c index 621c61e5c7..31952f48b9 100644 --- a/fs/ext4/ext4_common.c +++ b/fs/ext4/ext4_common.c @@ -432,6 +432,10 @@ uint16_t ext4fs_checksum_update(uint32_t i) crc = ext2fs_crc16(crc, desc, offset); offset += sizeof(desc->bg_checksum); /* skip checksum */ assert(offset == sizeof(*desc));
if (offset < fs->gdsize) {crc = ext2fs_crc16(crc, (__u8 *)desc + offset,fs->gdsize - offset);}This would be feb0ab32a57e4e6c8b24f6fb68f0ce08efe4603c from the kernel?
No, it goes further back, to 2007 even(!): 717d50e4971b81b96c0199c91cdf0
So shouldn't we have le16_to_cpu on fs->gdsize ? Or did I read over the 'git log -p' output in the kernel too quickly? Thanks!
No, because fs->gdsize is already endian-swapped elsewhere:
2345 fs->gdsize = le32_to_cpu(data->sblock.feature_incompat) & 2346 EXT4_FEATURE_INCOMPAT_64BIT ? 2347 le16_to_cpu(data->sblock.descriptor_size) : 32;
Ah, thanks for explaining and confirming.
Reviewed-by; Tom Rini trini@konsulko.com
On Mon, Sep 25, 2017 at 10:06:31PM +0300, Tuomas Tynkkynen wrote:
The current code doesn't compute the group descriptor checksum correctly for the filesystems that e2fsprogs 1.43.4 creates (they have 'Group descriptor size: 64' as reported by tune2fs). Extend the checksum calculation to be done as ext4_group_desc_csum() does in Linux.
This fixes these errors in dmesg from running fs-test.sh and makes it succeed again:
[1671902.620699] EXT4-fs (loop1): ext4_check_descriptors: Checksum for group 0 failed (35782!=10965) [1671902.620706] EXT4-fs (loop1): group descriptors corrupted!
Signed-off-by: Tuomas Tynkkynen tuomas.tynkkynen@iki.fi
Applied to u-boot/master, thanks!
participants (2)
-
Tom Rini -
Tuomas Tynkkynen