[PATCH v1 0/2] fs: fat: calculate FAT type based on cluster count

[PATCH v1 0/2] fs: fat: calculate FAT type based on cluster count

[christian.taedcke-oss]

From: Christian Taedcke <christian.taedcke@weidmueller.com>


This series fixes an issue where the FAT type (FAT12, FAT16) is not
correctly detected, e.g. when the BPB field BS_FilSysType contains the
valid value "FAT     ".

This issue occures, for example, if a partition is formatted by
swupdate using its diskformat handler. swupdate uses the FAT library
from http://elm-chan.org/fsw/ff/ internally.

See https://groups.google.com/g/swupdate/c/7Yc3NupjXx8 for a
discussion in the swupdate mailing list.

Please refer to the commit messages for more details.



Christian Taedcke (2):
  fs: fat: add macro to convert u8[2] to u16
  fs: fat: calculate FAT type based on cluster count

 fs/fat/fat.c             | 67 +++++++++++++++++++++++-----------------
 include/fat.h            |  6 ----
 test/image/spl_load_fs.c |  2 +-
 3 files changed, 40 insertions(+), 35 deletions(-)

-- 
2.34.1

[PATCH v1 1/2] fs: fat: add macro to convert u8[2] to u16

[christian.taedcke-oss]

From: Christian Taedcke <christian.taedcke@weidmueller.com>

This reduces code duplications.

Signed-off-by: Christian Taedcke <christian.taedcke@weidmueller.com>
---

 fs/fat/fat.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/fs/fat/fat.c b/fs/fat/fat.c
index 8ff1fd0ec8..8a0f4e4e6c 100644
--- a/fs/fat/fat.c
+++ b/fs/fat/fat.c
@@ -25,6 +25,8 @@
 #include <linux/compiler.h>
 #include <linux/ctype.h>
 
+#define FATU8ARRAY2CPU16(x) (((x)[1] << 8) + (x)[0])
+
 /*
  * Convert a string to lowercase.  Converts at most 'len' characters,
  * 'len' may be larger than the length of 'str' if 'str' is NULL
@@ -571,7 +573,7 @@ static int get_fs_info(fsdata *mydata)
 		mydata->total_sect = bs.total_sect;
 	} else {
 		mydata->fatlength = bs.fat_length;
-		mydata->total_sect = (bs.sectors[1] << 8) + bs.sectors[0];
+		mydata->total_sect = FATU8ARRAY2CPU16(bs.sectors);
 		if (!mydata->total_sect)
 			mydata->total_sect = bs.total_sect;
 	}
@@ -583,7 +585,7 @@ static int get_fs_info(fsdata *mydata)
 
 	mydata->rootdir_sect = mydata->fat_sect + mydata->fatlength * bs.fats;
 
-	mydata->sect_size = (bs.sector_size[1] << 8) + bs.sector_size[0];
+	mydata->sect_size = FATU8ARRAY2CPU16(bs.sector_size);
 	mydata->clust_size = bs.cluster_size;
 	if (mydata->sect_size != cur_part_info.blksz) {
 		log_err("FAT sector size mismatch (fs=%u, dev=%lu)\n",
@@ -607,8 +609,7 @@ static int get_fs_info(fsdata *mydata)
 					(mydata->clust_size * 2);
 		mydata->root_cluster = bs.root_cluster;
 	} else {
-		mydata->rootdir_size = ((bs.dir_entries[1]  * (int)256 +
-					 bs.dir_entries[0]) *
+		mydata->rootdir_size = (FATU8ARRAY2CPU16(bs.dir_entries) *
 					 sizeof(dir_entry)) /
 					 mydata->sect_size;
 		mydata->data_begin = mydata->rootdir_sect +
-- 
2.34.1

Re: [PATCH v1 1/2] fs: fat: add macro to convert u8[2] to u16

[Alexander Dahl]

Hello Christian,

Am Wed, Nov 08, 2023 at 01:12:38PM +0100 schrieb christian.taedcke-oss@weidmueller.com:
> From: Christian Taedcke <christian.taedcke@weidmueller.com>
> 
> This reduces code duplications.
> 
> Signed-off-by: Christian Taedcke <christian.taedcke@weidmueller.com>
> ---
> 
>  fs/fat/fat.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/fat/fat.c b/fs/fat/fat.c
> index 8ff1fd0ec8..8a0f4e4e6c 100644
> --- a/fs/fat/fat.c
> +++ b/fs/fat/fat.c
> @@ -25,6 +25,8 @@
>  #include <linux/compiler.h>
>  #include <linux/ctype.h>
>  
> +#define FATU8ARRAY2CPU16(x) (((x)[1] << 8) + (x)[0])

This does the same as the generic `get_unaligned_le16()` … why not use
that?

Greets
Alex

> +
>  /*
>   * Convert a string to lowercase.  Converts at most 'len' characters,
>   * 'len' may be larger than the length of 'str' if 'str' is NULL
> @@ -571,7 +573,7 @@ static int get_fs_info(fsdata *mydata)
>  		mydata->total_sect = bs.total_sect;
>  	} else {
>  		mydata->fatlength = bs.fat_length;
> -		mydata->total_sect = (bs.sectors[1] << 8) + bs.sectors[0];
> +		mydata->total_sect = FATU8ARRAY2CPU16(bs.sectors);
>  		if (!mydata->total_sect)
>  			mydata->total_sect = bs.total_sect;
>  	}
> @@ -583,7 +585,7 @@ static int get_fs_info(fsdata *mydata)
>  
>  	mydata->rootdir_sect = mydata->fat_sect + mydata->fatlength * bs.fats;
>  
> -	mydata->sect_size = (bs.sector_size[1] << 8) + bs.sector_size[0];
> +	mydata->sect_size = FATU8ARRAY2CPU16(bs.sector_size);
>  	mydata->clust_size = bs.cluster_size;
>  	if (mydata->sect_size != cur_part_info.blksz) {
>  		log_err("FAT sector size mismatch (fs=%u, dev=%lu)\n",
> @@ -607,8 +609,7 @@ static int get_fs_info(fsdata *mydata)
>  					(mydata->clust_size * 2);
>  		mydata->root_cluster = bs.root_cluster;
>  	} else {
> -		mydata->rootdir_size = ((bs.dir_entries[1]  * (int)256 +
> -					 bs.dir_entries[0]) *
> +		mydata->rootdir_size = (FATU8ARRAY2CPU16(bs.dir_entries) *
>  					 sizeof(dir_entry)) /
>  					 mydata->sect_size;
>  		mydata->data_begin = mydata->rootdir_sect +
> -- 
> 2.34.1
> 

[PATCH v1 2/2] fs: fat: calculate FAT type based on cluster count

[christian.taedcke-oss]

From: Christian Taedcke <christian.taedcke@weidmueller.com>

This fixes an issue where the FAT type (FAT12, FAT16) is not
correctly detected, e.g. when the BPB field BS_FilSysType contains the
valid value "FAT     ".

According to the FAT spec the field BS_FilSysType has only
informational character and does not determine the FAT type.

The logic of this code is based on the linux kernel implementation
from the file fs/fat/inode.c function fat_fill_super().

For details about FAT see http://elm-chan.org/docs/fat_e.html

Signed-off-by: Christian Taedcke <christian.taedcke@weidmueller.com>
---

 fs/fat/fat.c             | 58 +++++++++++++++++++++++-----------------
 include/fat.h            |  6 -----
 test/image/spl_load_fs.c |  2 +-
 3 files changed, 35 insertions(+), 31 deletions(-)

diff --git a/fs/fat/fat.c b/fs/fat/fat.c
index 8a0f4e4e6c..9179257ff2 100644
--- a/fs/fat/fat.c
+++ b/fs/fat/fat.c
@@ -27,6 +27,9 @@
 
 #define FATU8ARRAY2CPU16(x) (((x)[1] << 8) + (x)[0])
 
+/* maximum number of clusters for FAT12 */
+#define MAX_FAT12	0xFF4
+
 /*
  * Convert a string to lowercase.  Converts at most 'len' characters,
  * 'len' may be larger than the length of 'str' if 'str' is NULL
@@ -485,6 +488,32 @@ static __u8 mkcksum(struct nameext *nameext)
 	return ret;
 }
 
+/*
+ * Determine the FAT type
+ *
+ * Based on fat_fill_super() from the Linux kernel's fs/fat/inode.c
+ */
+static int
+determine_fat_bits(const boot_sector *bs)
+{
+	if (!bs->fat_length && bs->fat32_length) {
+		return 32;
+	} else {
+		u16 fat_start = bs->reserved;
+		u32 dir_start = fat_start + bs->fats * bs->fat_length;
+		u32 rootdir_sectors = FATU8ARRAY2CPU16(bs->dir_entries) *
+				      sizeof(dir_entry) /
+				      FATU8ARRAY2CPU16(bs->sector_size);
+		u32 data_start = dir_start + rootdir_sectors;
+		u16 sectors = FATU8ARRAY2CPU16(bs->sectors);
+		u32 total_sectors = sectors ? sectors : bs->total_sect;
+		u32 total_clusters = (total_sectors - data_start) /
+				     bs->cluster_size;
+
+		return (total_clusters > MAX_FAT12) ? 16 : 12;
+	}
+}
+
 /*
  * Read boot sector and volume info from a FAT filesystem
  */
@@ -493,7 +522,6 @@ read_bootsectandvi(boot_sector *bs, volume_info *volinfo, int *fatsize)
 {
 	__u8 *block;
 	volume_info *vistart;
-	int ret = 0;
 
 	if (cur_dev == NULL) {
 		debug("Error: no device selected\n");
@@ -508,7 +536,8 @@ read_bootsectandvi(boot_sector *bs, volume_info *volinfo, int *fatsize)
 
 	if (disk_read(0, 1, block) < 0) {
 		debug("Error: reading block\n");
-		goto fail;
+		free(block);
+		return -1;
 	}
 
 	memcpy(bs, block, sizeof(boot_sector));
@@ -527,33 +556,15 @@ read_bootsectandvi(boot_sector *bs, volume_info *volinfo, int *fatsize)
 		bs->info_sector = FAT2CPU16(bs->info_sector);
 		bs->backup_boot = FAT2CPU16(bs->backup_boot);
 		vistart = (volume_info *)(block + sizeof(boot_sector));
-		*fatsize = 32;
 	} else {
 		vistart = (volume_info *)&(bs->fat32_length);
-		*fatsize = 0;
 	}
 	memcpy(volinfo, vistart, sizeof(volume_info));
 
-	if (*fatsize == 32) {
-		if (strncmp(FAT32_SIGN, vistart->fs_type, SIGNLEN) == 0)
-			goto exit;
-	} else {
-		if (strncmp(FAT12_SIGN, vistart->fs_type, SIGNLEN) == 0) {
-			*fatsize = 12;
-			goto exit;
-		}
-		if (strncmp(FAT16_SIGN, vistart->fs_type, SIGNLEN) == 0) {
-			*fatsize = 16;
-			goto exit;
-		}
-	}
+	*fatsize = determine_fat_bits(bs);
 
-	debug("Error: broken fs_type sign\n");
-fail:
-	ret = -1;
-exit:
 	free(block);
-	return ret;
+	return 0;
 }
 
 static int get_fs_info(fsdata *mydata)
@@ -1158,9 +1169,8 @@ int file_fat_detectfs(void)
 
 	memcpy(vol_label, volinfo.volume_label, 11);
 	vol_label[11] = '\0';
-	volinfo.fs_type[5] = '\0';
 
-	printf("Filesystem: %s \"%s\"\n", volinfo.fs_type, vol_label);
+	printf("Filesystem: FAT%d \"%s\"\n", fatsize, vol_label);
 
 	return 0;
 }
diff --git a/include/fat.h b/include/fat.h
index a9756fb4cd..3dce99a23c 100644
--- a/include/fat.h
+++ b/include/fat.h
@@ -34,12 +34,6 @@ struct disk_partition;
 /* Maximum number of entry for long file name according to spec */
 #define MAX_LFN_SLOT	20
 
-/* Filesystem identifiers */
-#define FAT12_SIGN	"FAT12   "
-#define FAT16_SIGN	"FAT16   "
-#define FAT32_SIGN	"FAT32   "
-#define SIGNLEN		8
-
 /* File attributes */
 #define ATTR_RO	1
 #define ATTR_HIDDEN	2
diff --git a/test/image/spl_load_fs.c b/test/image/spl_load_fs.c
index 297ab08a82..90e640b5de 100644
--- a/test/image/spl_load_fs.c
+++ b/test/image/spl_load_fs.c
@@ -220,7 +220,7 @@ static size_t create_fat(void *dst, size_t size, const char *filename,
 	bs->root_cluster = cpu_to_le32(root_sector);
 
 	vi->ext_boot_sign = 0x29;
-	memcpy(vi->fs_type, FAT32_SIGN, sizeof(vi->fs_type));
+	memcpy(vi->fs_type, "FAT32   ", sizeof(vi->fs_type));
 
 	memcpy(dst + 0x1fe, "\x55\xAA", 2);
 
-- 
2.34.1

Re: [PATCH v1 2/2] fs: fat: calculate FAT type based on cluster count

[Heinrich Schuchardt]

On 11/8/23 04:12, christian.taedcke-oss@weidmueller.com wrote:
> From: Christian Taedcke <christian.taedcke@weidmueller.com>
>
> This fixes an issue where the FAT type (FAT12, FAT16) is not
> correctly detected, e.g. when the BPB field BS_FilSysType contains the
> valid value "FAT     ".
>
> According to the FAT spec the field BS_FilSysType has only
> informational character and does not determine the FAT type.
>
> The logic of this code is based on the linux kernel implementation
> from the file fs/fat/inode.c function fat_fill_super().
>
> For details about FAT see http://elm-chan.org/docs/fat_e.html
>
> Signed-off-by: Christian Taedcke <christian.taedcke@weidmueller.com>
> ---
>
>   fs/fat/fat.c             | 58 +++++++++++++++++++++++-----------------
>   include/fat.h            |  6 -----
>   test/image/spl_load_fs.c |  2 +-
>   3 files changed, 35 insertions(+), 31 deletions(-)
>
> diff --git a/fs/fat/fat.c b/fs/fat/fat.c
> index 8a0f4e4e6c..9179257ff2 100644
> --- a/fs/fat/fat.c
> +++ b/fs/fat/fat.c
> @@ -27,6 +27,9 @@
>
>   #define FATU8ARRAY2CPU16(x) (((x)[1] << 8) + (x)[0])
>
> +/* maximum number of clusters for FAT12 */
> +#define MAX_FAT12	0xFF4
> +
>   /*
>    * Convert a string to lowercase.  Converts at most 'len' characters,
>    * 'len' may be larger than the length of 'str' if 'str' is NULL
> @@ -485,6 +488,32 @@ static __u8 mkcksum(struct nameext *nameext)
>   	return ret;
>   }
>
> +/*
> + * Determine the FAT type
> + *
> + * Based on fat_fill_super() from the Linux kernel's fs/fat/inode.c
> + */
> +static int
> +determine_fat_bits(const boot_sector *bs)
> +{
> +	if (!bs->fat_length && bs->fat32_length) {
> +		return 32;
> +	} else {
> +		u16 fat_start = bs->reserved;
> +		u32 dir_start = fat_start + bs->fats * bs->fat_length;
> +		u32 rootdir_sectors = FATU8ARRAY2CPU16(bs->dir_entries) *
> +				      sizeof(dir_entry) /
> +				      FATU8ARRAY2CPU16(bs->sector_size);
> +		u32 data_start = dir_start + rootdir_sectors;
> +		u16 sectors = FATU8ARRAY2CPU16(bs->sectors);
> +		u32 total_sectors = sectors ? sectors : bs->total_sect;
> +		u32 total_clusters = (total_sectors - data_start) /
> +				     bs->cluster_size;
> +
> +		return (total_clusters > MAX_FAT12) ? 16 : 12;
> +	}
> +}
> +
>   /*
>    * Read boot sector and volume info from a FAT filesystem
>    */
> @@ -493,7 +522,6 @@ read_bootsectandvi(boot_sector *bs, volume_info *volinfo, int *fatsize)
>   {
>   	__u8 *block;
>   	volume_info *vistart;
> -	int ret = 0;
>
>   	if (cur_dev == NULL) {
>   		debug("Error: no device selected\n");
> @@ -508,7 +536,8 @@ read_bootsectandvi(boot_sector *bs, volume_info *volinfo, int *fatsize)
>
>   	if (disk_read(0, 1, block) < 0) {
>   		debug("Error: reading block\n");
> -		goto fail;
> +		free(block);
> +		return -1;

Thanks for correcting the FAT type determination.

The change above does not look wrong. But as it is an unrelated change,
please, put it into a separate patch.

>   	}
>
>   	memcpy(bs, block, sizeof(boot_sector));
> @@ -527,33 +556,15 @@ read_bootsectandvi(boot_sector *bs, volume_info *volinfo, int *fatsize)
>   		bs->info_sector = FAT2CPU16(bs->info_sector);
>   		bs->backup_boot = FAT2CPU16(bs->backup_boot);
>   		vistart = (volume_info *)(block + sizeof(boot_sector));
> -		*fatsize = 32;
>   	} else {
>   		vistart = (volume_info *)&(bs->fat32_length);
> -		*fatsize = 0;
>   	}
>   	memcpy(volinfo, vistart, sizeof(volume_info));
>
> -	if (*fatsize == 32) {
> -		if (strncmp(FAT32_SIGN, vistart->fs_type, SIGNLEN) == 0)
> -			goto exit;
> -	} else {
> -		if (strncmp(FAT12_SIGN, vistart->fs_type, SIGNLEN) == 0) {
> -			*fatsize = 12;
> -			goto exit;
> -		}
> -		if (strncmp(FAT16_SIGN, vistart->fs_type, SIGNLEN) == 0) {
> -			*fatsize = 16;
> -			goto exit;
> -		}
> -	}
> +	*fatsize = determine_fat_bits(bs);
>
> -	debug("Error: broken fs_type sign\n");
> -fail:
> -	ret = -1;
> -exit:
>   	free(block);
> -	return ret;
> +	return 0;
>   }
>
>   static int get_fs_info(fsdata *mydata)
> @@ -1158,9 +1169,8 @@ int file_fat_detectfs(void)
>
>   	memcpy(vol_label, volinfo.volume_label, 11);
>   	vol_label[11] = '\0';
> -	volinfo.fs_type[5] = '\0';
>
> -	printf("Filesystem: %s \"%s\"\n", volinfo.fs_type, vol_label);
> +	printf("Filesystem: FAT%d \"%s\"\n", fatsize, vol_label);
>
>   	return 0;
>   }
> diff --git a/include/fat.h b/include/fat.h
> index a9756fb4cd..3dce99a23c 100644
> --- a/include/fat.h
> +++ b/include/fat.h
> @@ -34,12 +34,6 @@ struct disk_partition;
>   /* Maximum number of entry for long file name according to spec */
>   #define MAX_LFN_SLOT	20
>
> -/* Filesystem identifiers */
> -#define FAT12_SIGN	"FAT12   "
> -#define FAT16_SIGN	"FAT16   "
> -#define FAT32_SIGN	"FAT32   "
> -#define SIGNLEN		8
> -
>   /* File attributes */
>   #define ATTR_RO	1
>   #define ATTR_HIDDEN	2
> diff --git a/test/image/spl_load_fs.c b/test/image/spl_load_fs.c
> index 297ab08a82..90e640b5de 100644
> --- a/test/image/spl_load_fs.c
> +++ b/test/image/spl_load_fs.c
> @@ -220,7 +220,7 @@ static size_t create_fat(void *dst, size_t size, const char *filename,
>   	bs->root_cluster = cpu_to_le32(root_sector);
>
>   	vi->ext_boot_sign = 0x29;
> -	memcpy(vi->fs_type, FAT32_SIGN, sizeof(vi->fs_type));
> +	memcpy(vi->fs_type, "FAT32   ", sizeof(vi->fs_type));

This change is not described in the commit message. Putting it into a
separate patch would be preferable.

Best regards

Heinrich

>
>   	memcpy(dst + 0x1fe, "\x55\xAA", 2);
>

Re: [PATCH v1 2/2] fs: fat: calculate FAT type based on cluster count

[Sean Anderson]

On 11/8/23 07:12, christian.taedcke-oss@weidmueller.com wrote:
> From: Christian Taedcke <christian.taedcke@weidmueller.com>
> 
> This fixes an issue where the FAT type (FAT12, FAT16) is not
> correctly detected, e.g. when the BPB field BS_FilSysType contains the
> valid value "FAT     ".
> 
> According to the FAT spec the field BS_FilSysType has only
> informational character and does not determine the FAT type.
> 
> The logic of this code is based on the linux kernel implementation
> from the file fs/fat/inode.c function fat_fill_super().
> 
> For details about FAT see http://elm-chan.org/docs/fat_e.html
> 
> Signed-off-by: Christian Taedcke <christian.taedcke@weidmueller.com>

Have you run bloat-o-meter on this? FAT is used in SPL a lot.

Would it be possible to write a test for this, especially for a FAT12 filesystem with
4084 data sectors and a FAT16 filesystem with 4085?

> ---
> 
>   fs/fat/fat.c             | 58 +++++++++++++++++++++++-----------------
>   include/fat.h            |  6 -----
>   test/image/spl_load_fs.c |  2 +-
>   3 files changed, 35 insertions(+), 31 deletions(-)
> 
> diff --git a/fs/fat/fat.c b/fs/fat/fat.c
> index 8a0f4e4e6c..9179257ff2 100644
> --- a/fs/fat/fat.c
> +++ b/fs/fat/fat.c
> @@ -27,6 +27,9 @@
>   
>   #define FATU8ARRAY2CPU16(x) (((x)[1] << 8) + (x)[0])
>   
> +/* maximum number of clusters for FAT12 */
> +#define MAX_FAT12	0xFF4
> +
>   /*
>    * Convert a string to lowercase.  Converts at most 'len' characters,
>    * 'len' may be larger than the length of 'str' if 'str' is NULL
> @@ -485,6 +488,32 @@ static __u8 mkcksum(struct nameext *nameext)
>   	return ret;
>   }
>   
> +/*
> + * Determine the FAT type
> + *
> + * Based on fat_fill_super() from the Linux kernel's fs/fat/inode.c
> + */
> +static int
> +determine_fat_bits(const boot_sector *bs)
> +{
> +	if (!bs->fat_length && bs->fat32_length) {
> +		return 32;
> +	} else {
> +		u16 fat_start = bs->reserved;
> +		u32 dir_start = fat_start + bs->fats * bs->fat_length;
> +		u32 rootdir_sectors = FATU8ARRAY2CPU16(bs->dir_entries) *
> +				      sizeof(dir_entry) /
> +				      FATU8ARRAY2CPU16(bs->sector_size);

Note that technically we are supposed to round up.

> +		u32 data_start = dir_start + rootdir_sectors;
> +		u16 sectors = FATU8ARRAY2CPU16(bs->sectors);
> +		u32 total_sectors = sectors ? sectors : bs->total_sect;
> +		u32 total_clusters = (total_sectors - data_start) /
> +				     bs->cluster_size;
> +
> +		return (total_clusters > MAX_FAT12) ? 16 : 12;
> +	}
> +}
> +

This should be merged with...

>   /*
>    * Read boot sector and volume info from a FAT filesystem
>    */
> @@ -493,7 +522,6 @@ read_bootsectandvi(boot_sector *bs, volume_info *volinfo, int *fatsize)
>   {
>   	__u8 *block;
>   	volume_info *vistart;
> -	int ret = 0;
>   
>   	if (cur_dev == NULL) {
>   		debug("Error: no device selected\n");
> @@ -508,7 +536,8 @@ read_bootsectandvi(boot_sector *bs, volume_info *volinfo, int *fatsize)
>   
>   	if (disk_read(0, 1, block) < 0) {
>   		debug("Error: reading block\n");
> -		goto fail;
> +		free(block);
> +		return -1;
>   	}
>   
>   	memcpy(bs, block, sizeof(boot_sector));
> @@ -527,33 +556,15 @@ read_bootsectandvi(boot_sector *bs, volume_info *volinfo, int *fatsize)
>   		bs->info_sector = FAT2CPU16(bs->info_sector);
>   		bs->backup_boot = FAT2CPU16(bs->backup_boot);
>   		vistart = (volume_info *)(block + sizeof(boot_sector));
> -		*fatsize = 32;
>   	} else {
>   		vistart = (volume_info *)&(bs->fat32_length);
> -		*fatsize = 0;
>   	}

... these if statements (above)

>   	memcpy(volinfo, vistart, sizeof(volume_info));
>   
> -	if (*fatsize == 32) {
> -		if (strncmp(FAT32_SIGN, vistart->fs_type, SIGNLEN) == 0)
> -			goto exit;
> -	} else {
> -		if (strncmp(FAT12_SIGN, vistart->fs_type, SIGNLEN) == 0) {
> -			*fatsize = 12;
> -			goto exit;
> -		}
> -		if (strncmp(FAT16_SIGN, vistart->fs_type, SIGNLEN) == 0) {
> -			*fatsize = 16;
> -			goto exit;
> -		}
> -	}

I think we should still verify these as long as vistart->ext_boot_sign is 0x29.

--Sean

> +	*fatsize = determine_fat_bits(bs);
>   
> -	debug("Error: broken fs_type sign\n");
> -fail:
> -	ret = -1;
> -exit:
>   	free(block);
> -	return ret;
> +	return 0;
>   }
>   
>   static int get_fs_info(fsdata *mydata)
> @@ -1158,9 +1169,8 @@ int file_fat_detectfs(void)
>   
>   	memcpy(vol_label, volinfo.volume_label, 11);
>   	vol_label[11] = '\0';
> -	volinfo.fs_type[5] = '\0';
>   
> -	printf("Filesystem: %s \"%s\"\n", volinfo.fs_type, vol_label);
> +	printf("Filesystem: FAT%d \"%s\"\n", fatsize, vol_label);
>   
>   	return 0;
>   }
> diff --git a/include/fat.h b/include/fat.h
> index a9756fb4cd..3dce99a23c 100644
> --- a/include/fat.h
> +++ b/include/fat.h
> @@ -34,12 +34,6 @@ struct disk_partition;
>   /* Maximum number of entry for long file name according to spec */
>   #define MAX_LFN_SLOT	20
>   
> -/* Filesystem identifiers */
> -#define FAT12_SIGN	"FAT12   "
> -#define FAT16_SIGN	"FAT16   "
> -#define FAT32_SIGN	"FAT32   "
> -#define SIGNLEN		8
> -
>   /* File attributes */
>   #define ATTR_RO	1
>   #define ATTR_HIDDEN	2
> diff --git a/test/image/spl_load_fs.c b/test/image/spl_load_fs.c
> index 297ab08a82..90e640b5de 100644
> --- a/test/image/spl_load_fs.c
> +++ b/test/image/spl_load_fs.c
> @@ -220,7 +220,7 @@ static size_t create_fat(void *dst, size_t size, const char *filename,
>   	bs->root_cluster = cpu_to_le32(root_sector);
>   
>   	vi->ext_boot_sign = 0x29;
> -	memcpy(vi->fs_type, FAT32_SIGN, sizeof(vi->fs_type));
> +	memcpy(vi->fs_type, "FAT32   ", sizeof(vi->fs_type));
>   
>   	memcpy(dst + 0x1fe, "\x55\xAA", 2);
>   

--Sean

Re: [PATCH v1 2/2] fs: fat: calculate FAT type based on cluster count

[Sean Anderson]

On 11/8/23 13:57, Sean Anderson wrote:
> On 11/8/23 07:12, christian.taedcke-oss@weidmueller.com wrote:
>> From: Christian Taedcke <christian.taedcke@weidmueller.com>
>>
>> This fixes an issue where the FAT type (FAT12, FAT16) is not
>> correctly detected, e.g. when the BPB field BS_FilSysType contains the
>> valid value "FAT     ".
>>
>> According to the FAT spec the field BS_FilSysType has only
>> informational character and does not determine the FAT type.
>>
>> The logic of this code is based on the linux kernel implementation
>> from the file fs/fat/inode.c function fat_fill_super().
>>
>> For details about FAT see http://elm-chan.org/docs/fat_e.html
>>
>> Signed-off-by: Christian Taedcke <christian.taedcke@weidmueller.com>
> 
> Have you run bloat-o-meter on this? FAT is used in SPL a lot.
> 
> Would it be possible to write a test for this, especially for a FAT12 filesystem with
> 4084 data sectors and a FAT16 filesystem with 4085?
> 
>> ---
>>
>>   fs/fat/fat.c             | 58 +++++++++++++++++++++++-----------------
>>   include/fat.h            |  6 -----
>>   test/image/spl_load_fs.c |  2 +-
>>   3 files changed, 35 insertions(+), 31 deletions(-)
>>
>> diff --git a/fs/fat/fat.c b/fs/fat/fat.c
>> index 8a0f4e4e6c..9179257ff2 100644
>> --- a/fs/fat/fat.c
>> +++ b/fs/fat/fat.c
>> @@ -27,6 +27,9 @@
>>   #define FATU8ARRAY2CPU16(x) (((x)[1] << 8) + (x)[0])
>> +/* maximum number of clusters for FAT12 */
>> +#define MAX_FAT12    0xFF4
>> +
>>   /*
>>    * Convert a string to lowercase.  Converts at most 'len' characters,
>>    * 'len' may be larger than the length of 'str' if 'str' is NULL
>> @@ -485,6 +488,32 @@ static __u8 mkcksum(struct nameext *nameext)
>>       return ret;
>>   }
>> +/*
>> + * Determine the FAT type
>> + *
>> + * Based on fat_fill_super() from the Linux kernel's fs/fat/inode.c
>> + */
>> +static int
>> +determine_fat_bits(const boot_sector *bs)
>> +{
>> +    if (!bs->fat_length && bs->fat32_length) {
>> +        return 32;
>> +    } else {
>> +        u16 fat_start = bs->reserved;
>> +        u32 dir_start = fat_start + bs->fats * bs->fat_length;
>> +        u32 rootdir_sectors = FATU8ARRAY2CPU16(bs->dir_entries) *
>> +                      sizeof(dir_entry) /
>> +                      FATU8ARRAY2CPU16(bs->sector_size);
> 
> Note that technically we are supposed to round up.
> 
>> +        u32 data_start = dir_start + rootdir_sectors;
>> +        u16 sectors = FATU8ARRAY2CPU16(bs->sectors);
>> +        u32 total_sectors = sectors ? sectors : bs->total_sect;
>> +        u32 total_clusters = (total_sectors - data_start) /
>> +                     bs->cluster_size;
>> +
>> +        return (total_clusters > MAX_FAT12) ? 16 : 12;
>> +    }
>> +}
>> +
> 
> This should be merged with...
> 
>>   /*
>>    * Read boot sector and volume info from a FAT filesystem
>>    */
>> @@ -493,7 +522,6 @@ read_bootsectandvi(boot_sector *bs, volume_info *volinfo, int *fatsize)
>>   {
>>       __u8 *block;
>>       volume_info *vistart;
>> -    int ret = 0;
>>       if (cur_dev == NULL) {
>>           debug("Error: no device selected\n");
>> @@ -508,7 +536,8 @@ read_bootsectandvi(boot_sector *bs, volume_info *volinfo, int *fatsize)
>>       if (disk_read(0, 1, block) < 0) {
>>           debug("Error: reading block\n");
>> -        goto fail;
>> +        free(block);
>> +        return -1;
>>       }
>>       memcpy(bs, block, sizeof(boot_sector));
>> @@ -527,33 +556,15 @@ read_bootsectandvi(boot_sector *bs, volume_info *volinfo, int *fatsize)
>>           bs->info_sector = FAT2CPU16(bs->info_sector);
>>           bs->backup_boot = FAT2CPU16(bs->backup_boot);
>>           vistart = (volume_info *)(block + sizeof(boot_sector));
>> -        *fatsize = 32;
>>       } else {
>>           vistart = (volume_info *)&(bs->fat32_length);
>> -        *fatsize = 0;
>>       }
> 
> ... these if statements (above)
> 
>>       memcpy(volinfo, vistart, sizeof(volume_info));
>> -    if (*fatsize == 32) {
>> -        if (strncmp(FAT32_SIGN, vistart->fs_type, SIGNLEN) == 0)
>> -            goto exit;
>> -    } else {
>> -        if (strncmp(FAT12_SIGN, vistart->fs_type, SIGNLEN) == 0) {
>> -            *fatsize = 12;
>> -            goto exit;
>> -        }
>> -        if (strncmp(FAT16_SIGN, vistart->fs_type, SIGNLEN) == 0) {
>> -            *fatsize = 16;
>> -            goto exit;
>> -        }
>> -    }
> 
> I think we should still verify these as long as vistart->ext_boot_sign is 0x29.

To add to this, if we don't want to verify these we should at least check the heuristics from [1].

--Sean

[1] https://www.win.tue.nl/~aeb/linux/fs/fat/fat-2.html

>> +    *fatsize = determine_fat_bits(bs);
>> -    debug("Error: broken fs_type sign\n");
>> -fail:
>> -    ret = -1;
>> -exit:
>>       free(block);
>> -    return ret;
>> +    return 0;
>>   }
>>   static int get_fs_info(fsdata *mydata)
>> @@ -1158,9 +1169,8 @@ int file_fat_detectfs(void)
>>       memcpy(vol_label, volinfo.volume_label, 11);
>>       vol_label[11] = '\0';
>> -    volinfo.fs_type[5] = '\0';
>> -    printf("Filesystem: %s \"%s\"\n", volinfo.fs_type, vol_label);
>> +    printf("Filesystem: FAT%d \"%s\"\n", fatsize, vol_label);
>>       return 0;
>>   }
>> diff --git a/include/fat.h b/include/fat.h
>> index a9756fb4cd..3dce99a23c 100644
>> --- a/include/fat.h
>> +++ b/include/fat.h
>> @@ -34,12 +34,6 @@ struct disk_partition;
>>   /* Maximum number of entry for long file name according to spec */
>>   #define MAX_LFN_SLOT    20
>> -/* Filesystem identifiers */
>> -#define FAT12_SIGN    "FAT12   "
>> -#define FAT16_SIGN    "FAT16   "
>> -#define FAT32_SIGN    "FAT32   "
>> -#define SIGNLEN        8
>> -
>>   /* File attributes */
>>   #define ATTR_RO    1
>>   #define ATTR_HIDDEN    2
>> diff --git a/test/image/spl_load_fs.c b/test/image/spl_load_fs.c
>> index 297ab08a82..90e640b5de 100644
>> --- a/test/image/spl_load_fs.c
>> +++ b/test/image/spl_load_fs.c
>> @@ -220,7 +220,7 @@ static size_t create_fat(void *dst, size_t size, const char *filename,
>>       bs->root_cluster = cpu_to_le32(root_sector);
>>       vi->ext_boot_sign = 0x29;
>> -    memcpy(vi->fs_type, FAT32_SIGN, sizeof(vi->fs_type));
>> +    memcpy(vi->fs_type, "FAT32   ", sizeof(vi->fs_type));
>>       memcpy(dst + 0x1fe, "\x55\xAA", 2);
> 
> --Sean