[U-Boot] [PATCH 1/3] fs-test: Add FAT16 support

[U-Boot] [PATCH 1/3] fs-test: Add FAT16 support

[Tuomas Tynkkynen]

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>
---
 test/fs/fs-test.sh | 41 +++++++++++++++++++++++++++--------------
 1 file changed, 27 insertions(+), 14 deletions(-)

diff --git a/test/fs/fs-test.sh b/test/fs/fs-test.sh
index b19486419e..2ddac50f90 100755
--- a/test/fs/fs-test.sh
+++ b/test/fs/fs-test.sh
@@ -41,7 +41,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 +104,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 +133,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 +146,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 +159,7 @@ function test_image() {
 	length="0x00100000"
 
 	case "$2" in
-		fat)
+		fat*)
 		FPATH=""
 		PREFIX="fat"
 		WRITE="write"
@@ -550,7 +560,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 +573,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"
 
-- 
2.13.0

[U-Boot] [PATCH 2/3] fs/fat: Fix pathnames using '..' that lead to the root directory

[Tuomas Tynkkynen]

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 on FAT12/FAT16.

Fixes: 8eafae209c35 ("fat/fs: convert to directory iterators")
Signed-off-by: Tuomas Tynkkynen <tuomas.tynkkynen@iki.fi>
---
 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)
-- 
2.13.0

[U-Boot] [PATCH 3/3] fs-test: Add test for a filename using '..' to go back to the root

[Tuomas Tynkkynen]

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>
---
 test/fs/fs-test.sh | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/test/fs/fs-test.sh b/test/fs/fs-test.sh
index 2ddac50f90..a8a55e41fc 100755
--- a/test/fs/fs-test.sh
+++ b/test/fs/fs-test.sh
@@ -225,10 +225,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
@@ -343,6 +347,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.
@@ -457,8 +464,11 @@ function check_results() {
 	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"
-- 
2.13.0

[U-Boot] [PATCH 1/3] fs-test: Add FAT16 support

[Tom Rini]

On Mon, Sep 25, 2017 at 12:07:34AM +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.

Ah, cool.  Please update the top of the script to note the expected
PASS/FAIL numbers now that we're testing even more.  Everything else
looks good tho.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170924/7a6feb06/attachment.sig>

[U-Boot] [PATCH 2/3] fs/fat: Fix pathnames using '..' that lead to the root directory

[Tom Rini]

On Mon, Sep 25, 2017 at 12:07:35AM +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 on FAT12/FAT16.
> 
> Fixes: 8eafae209c35 ("fat/fs: convert to directory iterators")
> Signed-off-by: Tuomas Tynkkynen <tuomas.tynkkynen@iki.fi>

Reviewed-by: Tom Rini <trini@konsulko.com>

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170924/f6cb0887/attachment.sig>

[U-Boot] [PATCH 3/3] fs-test: Add test for a filename using '..' to go back to the root

[Tom Rini]

On Mon, Sep 25, 2017 at 12:07:36AM +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...

Can you post, unrelated, the code that totally crashed sandbox?  Maybe
that's a problem we need to fix too :)

Otherwise fine, but we need to update the expected results.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170924/cb08bbbc/attachment.sig>

[U-Boot] [PATCH 1/3] fs-test: Add FAT16 support

[Rob Clark]

On Sun, Sep 24, 2017 at 5:07 PM, Tuomas Tynkkynen
<tuomas.tynkkynen@iki.fi> 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.


Thanks

Note that fat12 and fat16 work more similar to each other (other than
file allocation table size) compared to fat32.  (fat12 and fat16 have
a single contiguous root directory, whereas fat32 has a root directory
constructed from a chain of clusters, more like how subdirectories
work on earlier fat versions).

That said, I think we could probably increase test coverage w/ much
smaller filesys image sizes.. my current wishlist:

  + bunch of small files w/ long vfat names, enough that the directory
spans multiple clusters.. depends a bit on sector/cluster size but
>128 should do the trick.. there might end up being a lower limit on
root directory in fat12/fat16 so this could be a subdir

  + we don't need huge files, but something bigger than cluster size..
1MB should be sufficient

BR,
-R


> Signed-off-by: Tuomas Tynkkynen <tuomas.tynkkynen@iki.fi>
> ---
>  test/fs/fs-test.sh | 41 +++++++++++++++++++++++++++--------------
>  1 file changed, 27 insertions(+), 14 deletions(-)
>
> diff --git a/test/fs/fs-test.sh b/test/fs/fs-test.sh
> index b19486419e..2ddac50f90 100755
> --- a/test/fs/fs-test.sh
> +++ b/test/fs/fs-test.sh
> @@ -41,7 +41,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 +104,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 +133,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 +146,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 +159,7 @@ function test_image() {
>         length="0x00100000"
>
>         case "$2" in
> -               fat)
> +               fat*)
>                 FPATH=""
>                 PREFIX="fat"
>                 WRITE="write"
> @@ -550,7 +560,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 +573,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"
>
> --
> 2.13.0
>

[U-Boot] [PATCH 3/3] fs-test: Add test for a filename using '..' to go back to the root

[Rob Clark]

On Sun, Sep 24, 2017 at 5:38 PM, Tom Rini <trini@konsulko.com> wrote:
> On Mon, Sep 25, 2017 at 12:07:36AM +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...
>
> Can you post, unrelated, the code that totally crashed sandbox?  Maybe
> that's a problem we need to fix too :)

jfwiw, I've started looking at the fat_write code, so even a
failing/crashing test is useful.

BR,
-R


> Otherwise fine, but we need to update the expected results.
>
> --
> Tom
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot
>

[U-Boot] [PATCH 3/3] fs-test: Add test for a filename using '..' to go back to the root

[Tuomas Tynkkynen]

On 09/25/2017 12:53 AM, Rob Clark wrote:
> On Sun, Sep 24, 2017 at 5:38 PM, Tom Rini <trini@konsulko.com> wrote:
>> On Mon, Sep 25, 2017 at 12:07:36AM +0300, Tuomas Tynkkynen wrote:
>>
>> Can you post, unrelated, the code that totally crashed sandbox?  Maybe
>> that's a problem we need to fix too :)
> 
> jfwiw, I've started looking at the fat_write code, so even a
> failing/crashing test is useful.
> 

On top of these three (also attached to avoid whitespace damage):

diff --git a/test/fs/fs-test.sh b/test/fs/fs-test.sh
index a8a55e41fc..eccc03a513 100755
--- a/test/fs/fs-test.sh
+++ b/test/fs/fs-test.sh
@@ -110,7 +110,7 @@ function create_image() {
         # Create image if not already present - saves time, while debugging
         case "$2" in
                 fat16)
-               MKFS_OPTION="-F 16"
+               MKFS_OPTION="-F 16 -r 8192"
                 FS_TYPE="fat"
                 ;;
                 fat32)
@@ -363,6 +363,10 @@ function create_files() {
                         &> /dev/null
         fi

+       for i in $(seq 1 256); do
+               sudo touch "$MOUNT_DIR/LONGNAME$(head -c 240 /dev/zero | 
tr '\0' 'X').$i"
+       done
+
         # Create a small file in this image.
         if [ ! -f "${MB1}" ]; then
                 sudo dd if=/dev/urandom of="${MB1}" bs=1M count=1 \
@@ -570,7 +574,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 fat16 fat32; do
+for fs in fat16; do

         echo "Creating $fs image if not already present."
         IMAGE=${IMG}.${fs}.img

Basically the problem is that curclust for the root directory is not <= 
1 but rather some large positive value (0x01fffffe) in the following 
snippet in fat_write.c:

>  867                 /*
>  868                  * In FAT16/12, the root dir is locate before data area, shows
>  869                  * in following:
>  870                  * -------------------------------------------------------------
>  871                  * | Boot | FAT1 & 2 | Root dir | Data (start from cluster #2) |
>  872                  * -------------------------------------------------------------
>  873                  *
>  874                  * As a result if curclust is in Root dir, it is a negative
>  875                  * number or 0, 1.
>  876                  *
>  877                  */
>  878                 if (mydata->fatsize != 32 && (int)curclust <= 1) {
>  879                         /* Current clust is in root dir, set to next clust */
>  880                         curclust++;

With fsdata->data_begin = 896,
      fsdata->rootdir_sect = 640,
      fsdata->clust_size = 128,
indeed sect_to_clust(rootdir_sect) doesn't underflow enough to be 
negative but ends up 0x01fffffe. That hack really needs excising from 
the tree...

Another fun thing that needs handling is that the root directory need 
not be a multiple of cluster size, being a multiple of sector size is 
enough according to fatgen103.pdf...
-------------- next part --------------
A non-text attachment was scrubbed...
Name: crash.diff
Type: text/x-patch
Size: 1046 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170925/4b497930/attachment.bin>