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

[Sean Anderson <seanga2@gmail.com>]

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