RE:(2) [PATCH] fs: fat: add check for dir size in fat_calc_dir_size

[AMIT SAHRAWAT]

There are many implementation that doesn't follow the spec strictly. And
when I tested in past, Windows also allowed to read the directory beyond
that limit. I can't recall though if there is in real case or just test
case though.
>> Thanks Ogawa, yes there are many implementations, preferably going around with different variants.
But, using standard linux version on the systems and having such USB connected on such systems is introducing issues(importantly because these being used on Windows also by users).
I am not sure, if this is something which is new from Windows part.
But, surely extending the directory beyond limit is causing regression with FAT usage on linux.
It is making FAT filesystem related storage virtually unresponsive for minutes in these cases,
and importantly keep on putting pressure on memory due to increasing buffer heads (already a known one with FAT fs).
 
So if there is no strong reason to apply the limit, I don't think it is
good to limit it. 
>> The reason for us to share this is because of the unresponsive behaviour observed with FAT fs on our systems.
This is not a new issue, we have been observing this for quite sometime (may be around 1year+).
Finally, we got hold of disk which is making this 100% reproducible.
We thought of applying this to the mainline, as our FAT is aligned with main kernel.

(btw, the current code should detect the corruption of
infinite loop already)
>>
No, there are no such error reported on our side. 
We had to trace to check the point of stuck in simple 'ls -lR'.

Thanks & Regards,
Amit Sahrawat
Â
Â
--------- Original Message ---------
Sender : OGAWA HirofumiÂ<hirofumi@xxxxxxxxxxxxxxxxxx>
Date : 2020-06-30 16:38 (GMT+5:30)
Title : Re: [PATCH] fs: fat: add check for dir size in fat_calc_dir_size
Â
AnupamÂAggarwalÂ<anupam.al@xxxxxxxxxxx>Âwrites:
Â
>ÂMaxÂdirectoryÂsizeÂofÂFATÂfilesystemÂisÂFAT_MAX_DIR_SIZE(2097152Âbytes)
>ÂItÂisÂpossibleÂthat,ÂdueÂtoÂcorruption,ÂdirectoryÂsizeÂcalculatedÂin
>Âfat_calc_dir_size()ÂcanÂbeÂgreaterÂthanÂFAT_MAX_DIR_SIZE,Âi.e.
>ÂcanÂbeÂinÂGBs,ÂhenceÂdirectoryÂtraversalÂcanÂtakeÂlongÂtime.
>ÂforÂexampleÂwhenÂcommandÂ"lsÂ-lR"ÂisÂexecutedÂonÂcorruptedÂFAT
>ÂformattedÂUSB,Âfat_search_long()ÂfunctionÂwillÂlookupÂforÂaÂfilenameÂfrom
>ÂpositionÂ0ÂtillÂendÂofÂcorruptedÂdirectoryÂsize,ÂmultipleÂsuchÂlookups
>ÂwillÂleadÂtoÂlongÂdirectoryÂtraversal
>
>ÂAddedÂsanityÂcheckÂforÂdirectoryÂsizeÂfat_calc_dir_size(),
>ÂandÂreturnÂEIOÂerror,ÂwhichÂwillÂpreventÂlookupÂinÂcorruptedÂdirectory
>
>ÂSigned-off-by:ÂAnupamÂAggarwalÂ<anupam.al@xxxxxxxxxxx>
>ÂSigned-off-by:ÂAmitÂSahrawatÂ<a.sahrawat@xxxxxxxxxxx>
Â
ThereÂareÂmanyÂimplementationÂthatÂdoesn'tÂfollowÂtheÂspecÂstrictly.ÂAnd
whenÂIÂtestedÂinÂpast,ÂWindowsÂalsoÂallowedÂtoÂreadÂtheÂdirectoryÂbeyond
thatÂlimit.ÂIÂcan'tÂrecallÂthoughÂifÂthereÂisÂinÂrealÂcaseÂorÂjustÂtest
caseÂthough.
Â
SoÂifÂthereÂisÂnoÂstrongÂreasonÂtoÂapplyÂtheÂlimit,ÂIÂdon'tÂthinkÂitÂis
goodÂtoÂlimitÂit.Â(btw,ÂtheÂcurrentÂcodeÂshouldÂdetectÂtheÂcorruptionÂof
infiniteÂloopÂalready)
Â
Thanks.
Â
>Â---
>ÂÂfs/fat/inode.cÂ|Â7Â+++++++
>ÂÂ1ÂfileÂchanged,Â7Âinsertions(+)
>
>ÂdiffÂ--gitÂa/fs/fat/inode.cÂb/fs/fat/inode.c
>ÂindexÂa0cf99d..9b2e81eÂ100644
>Â---Âa/fs/fat/inode.c
>Â+++Âb/fs/fat/inode.c
>Â@@Â-490,6Â+490,13Â@@ÂstaticÂintÂfat_calc_dir_size(structÂinodeÂ*inode)
>ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturnÂret;
>ÂÂÂÂÂÂÂÂÂÂinode->i_sizeÂ=Â(fclusÂ+Â1)Â<<Âsbi->cluster_bits;
>ÂÂ
>Â+ÂÂÂÂÂÂÂÂifÂ(i_size_read(inode)Â>ÂFAT_MAX_DIR_SIZE)Â{
>Â+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂfat_fs_error(inode->i_sb,
>Â+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ"%sÂcorruptedÂdirectoryÂ(invalidÂsizeÂ%lld)\n",
>Â+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ__func__,Âi_size_read(inode));
>Â+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturnÂ-EIO;
>Â+ÂÂÂÂÂÂÂÂ}
>Â+
>ÂÂÂÂÂÂÂÂÂÂreturnÂ0;
>ÂÂ}
Â
--Â
OGAWAÂHirofumiÂ<hirofumi@xxxxxxxxxxxxxxxxxx>
Â