[U-Boot,v4,1/6] fat: write: fix broken write to fragmented files
diff mbox series

Message ID 20191202111118.12868-2-m.szyprowski@samsung.com
State New
Delegated to: Matthias Brugger
Headers show
Series
  • Raspberry Pi4: add support for DFU over USB
Related show

Commit Message

Marek Szyprowski Dec. 2, 2019, 11:11 a.m. UTC
The code for handing file overwrite incorrectly assumed that the file on
disk is always contiguous. This resulted in corrupting disk structure
every time when write to existing fragmented file happened. Fix this
by adding proper check for cluster discontinuity and adjust chunk size
on each partial write.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---

This patch partially fixes the issue revealed by the following test
script:

--->8-fat_test1.sh---
#!/bin/bash
make sandbox_defconfig
make
dd if=/dev/zero of=/tmp/10M.img bs=1024 count=10k
mkfs.vfat -v /tmp/10M.img
cat >/tmp/cmds <<EOF
x
host bind 0 /tmp/10M.img
fatls host 0
mw 0x1000000 0x0a434241 0x1000 # "ABC\n"
mw 0x1100000 0x0a464544 0x8000 # "DEF\n"
fatwrite host 0 0x1000000 file0001.raw 0x1000
fatwrite host 0 0x1000000 file0002.raw 0x1000
fatwrite host 0 0x1000000 file0003.raw 0x1000
fatwrite host 0 0x1000000 file0004.raw 0x1000
fatwrite host 0 0x1000000 file0005.raw 0x1000
fatrm host 0 file0002.raw
fatrm host 0 file0004.raw
fatls host 0
fatwrite host 0 0x1100000 file0007.raw 0x4000
fatwrite host 0 0x1100000 file0007.raw 0x4000
reset
EOF
./u-boot </tmp/cmds
#verify
rm -r /tmp/result /tmp/model
mkdir /tmp/result
mkdir /tmp/model
yes ABC | head -c 4096 >/tmp/model/file0001.raw
yes ABC | head -c 4096 >/tmp/model/file0003.raw
yes ABC | head -c 4096 >/tmp/model/file0005.raw
yes DEF | head -c 16384 >/tmp/model/file0007.raw
mcopy -n -i /tmp/10M.img ::file0001.raw /tmp/result
mcopy -n -i /tmp/10M.img ::file0003.raw /tmp/result
mcopy -n -i /tmp/10M.img ::file0005.raw /tmp/result
mcopy -n -i /tmp/10M.img ::file0007.raw /tmp/result
hd /tmp/10M.img
if diff -urq /tmp/model /tmp/result
then
	echo Test okay
else
	echo Test fail
fi
--->8---

Overwritting a discontiguous test file (file0007.raw) no longer causes
corruption to file0003.raw, which's data lies between the chunks of the
test file. The amount of data written to disk is still incorrect, what
causes damage to the file (file0005.raw), which's data lies next to the
test file. This will be fixed by the next patch.

Feel free to prepare a proper sandbox/py_test based tests based on the
provided test scripts.
---
 fs/fat/fat_write.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Lukasz Majewski Dec. 5, 2019, 4:52 p.m. UTC | #1
Hi Tom, Matthias,

> The code for handing file overwrite incorrectly assumed that the file
> on disk is always contiguous. This resulted in corrupting disk
> structure every time when write to existing fragmented file happened.
> Fix this by adding proper check for cluster discontinuity and adjust
> chunk size on each partial write.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
> 
> This patch partially fixes the issue revealed by the following test
> script:
> 

Tom could you pic this patch and the following one (2/6):
https://patchwork.ozlabs.org/patch/1203101/

to -master as a fix?

This seems like a _real_ fix for FAT.

The dfu part of this series IMHO shall be grabbed by Matthias or me into
the -next branch of u-boot-dfu/rpi4 tree.

I do guess that Matthias shall fetch this series as he is assigned to
it in the patchwork?
I'm fine for this as I've already acked / reviewed DFU part of this
series.

> --->8-fat_test1.sh---  
> #!/bin/bash
> make sandbox_defconfig
> make
> dd if=/dev/zero of=/tmp/10M.img bs=1024 count=10k
> mkfs.vfat -v /tmp/10M.img
> cat >/tmp/cmds <<EOF
> x
> host bind 0 /tmp/10M.img
> fatls host 0
> mw 0x1000000 0x0a434241 0x1000 # "ABC\n"
> mw 0x1100000 0x0a464544 0x8000 # "DEF\n"
> fatwrite host 0 0x1000000 file0001.raw 0x1000
> fatwrite host 0 0x1000000 file0002.raw 0x1000
> fatwrite host 0 0x1000000 file0003.raw 0x1000
> fatwrite host 0 0x1000000 file0004.raw 0x1000
> fatwrite host 0 0x1000000 file0005.raw 0x1000
> fatrm host 0 file0002.raw
> fatrm host 0 file0004.raw
> fatls host 0
> fatwrite host 0 0x1100000 file0007.raw 0x4000
> fatwrite host 0 0x1100000 file0007.raw 0x4000
> reset
> EOF
> ./u-boot </tmp/cmds
> #verify
> rm -r /tmp/result /tmp/model
> mkdir /tmp/result
> mkdir /tmp/model
> yes ABC | head -c 4096 >/tmp/model/file0001.raw
> yes ABC | head -c 4096 >/tmp/model/file0003.raw
> yes ABC | head -c 4096 >/tmp/model/file0005.raw
> yes DEF | head -c 16384 >/tmp/model/file0007.raw
> mcopy -n -i /tmp/10M.img ::file0001.raw /tmp/result
> mcopy -n -i /tmp/10M.img ::file0003.raw /tmp/result
> mcopy -n -i /tmp/10M.img ::file0005.raw /tmp/result
> mcopy -n -i /tmp/10M.img ::file0007.raw /tmp/result
> hd /tmp/10M.img
> if diff -urq /tmp/model /tmp/result
> then
> 	echo Test okay
> else
> 	echo Test fail
> fi
> --->8---  
> 
> Overwritting a discontiguous test file (file0007.raw) no longer causes
> corruption to file0003.raw, which's data lies between the chunks of
> the test file. The amount of data written to disk is still incorrect,
> what causes damage to the file (file0005.raw), which's data lies next
> to the test file. This will be fixed by the next patch.
> 
> Feel free to prepare a proper sandbox/py_test based tests based on the
> provided test scripts.
> ---
>  fs/fat/fat_write.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c
> index 729cf39630..f946030f7d 100644
> --- a/fs/fat/fat_write.c
> +++ b/fs/fat/fat_write.c
> @@ -794,6 +794,8 @@ set_contents(fsdata *mydata, dir_entry *dentptr,
> loff_t pos, __u8 *buffer, 
>  			newclust = get_fatent(mydata, endclust);
>  
> +			if (newclust != endclust + 1)
> +				break;
>  			if (IS_LAST_CLUST(newclust, mydata->fatsize))
>  				break;
>  			if (CHECK_CLUST(newclust, mydata->fatsize)) {
> @@ -824,8 +826,6 @@ set_contents(fsdata *mydata, dir_entry *dentptr,
> loff_t pos, __u8 *buffer, if (filesize <= cur_pos)
>  			break;
>  
> -		/* CHECK: newclust = get_fatent(mydata, endclust); */
> -
>  		if (IS_LAST_CLUST(newclust, mydata->fatsize))
>  			/* no more clusters */
>  			break;




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
Matthias Brugger Dec. 5, 2019, 5:58 p.m. UTC | #2
On 05/12/2019 17:52, Lukasz Majewski wrote:
> Hi Tom, Matthias,
> 
>> The code for handing file overwrite incorrectly assumed that the file
>> on disk is always contiguous. This resulted in corrupting disk
>> structure every time when write to existing fragmented file happened.
>> Fix this by adding proper check for cluster discontinuity and adjust
>> chunk size on each partial write.
>>
>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> ---
>>
>> This patch partially fixes the issue revealed by the following test
>> script:
>>
> 
> Tom could you pic this patch and the following one (2/6):
> https://patchwork.ozlabs.org/patch/1203101/
> 
> to -master as a fix?
> 
> This seems like a _real_ fix for FAT.

Right, I think the first patches should go in for v2020.01. I can send them
together with some fixes for RPi I'm working on.

Tom what do you think?

Regards,
Matthias

> 
> The dfu part of this series IMHO shall be grabbed by Matthias or me into
> the -next branch of u-boot-dfu/rpi4 tree.
> 
> I do guess that Matthias shall fetch this series as he is assigned to
> it in the patchwork?
> I'm fine for this as I've already acked / reviewed DFU part of this
> series.
> 
>> --->8-fat_test1.sh---  
>> #!/bin/bash
>> make sandbox_defconfig
>> make
>> dd if=/dev/zero of=/tmp/10M.img bs=1024 count=10k
>> mkfs.vfat -v /tmp/10M.img
>> cat >/tmp/cmds <<EOF
>> x
>> host bind 0 /tmp/10M.img
>> fatls host 0
>> mw 0x1000000 0x0a434241 0x1000 # "ABC\n"
>> mw 0x1100000 0x0a464544 0x8000 # "DEF\n"
>> fatwrite host 0 0x1000000 file0001.raw 0x1000
>> fatwrite host 0 0x1000000 file0002.raw 0x1000
>> fatwrite host 0 0x1000000 file0003.raw 0x1000
>> fatwrite host 0 0x1000000 file0004.raw 0x1000
>> fatwrite host 0 0x1000000 file0005.raw 0x1000
>> fatrm host 0 file0002.raw
>> fatrm host 0 file0004.raw
>> fatls host 0
>> fatwrite host 0 0x1100000 file0007.raw 0x4000
>> fatwrite host 0 0x1100000 file0007.raw 0x4000
>> reset
>> EOF
>> ./u-boot </tmp/cmds
>> #verify
>> rm -r /tmp/result /tmp/model
>> mkdir /tmp/result
>> mkdir /tmp/model
>> yes ABC | head -c 4096 >/tmp/model/file0001.raw
>> yes ABC | head -c 4096 >/tmp/model/file0003.raw
>> yes ABC | head -c 4096 >/tmp/model/file0005.raw
>> yes DEF | head -c 16384 >/tmp/model/file0007.raw
>> mcopy -n -i /tmp/10M.img ::file0001.raw /tmp/result
>> mcopy -n -i /tmp/10M.img ::file0003.raw /tmp/result
>> mcopy -n -i /tmp/10M.img ::file0005.raw /tmp/result
>> mcopy -n -i /tmp/10M.img ::file0007.raw /tmp/result
>> hd /tmp/10M.img
>> if diff -urq /tmp/model /tmp/result
>> then
>> 	echo Test okay
>> else
>> 	echo Test fail
>> fi
>> --->8---  
>>
>> Overwritting a discontiguous test file (file0007.raw) no longer causes
>> corruption to file0003.raw, which's data lies between the chunks of
>> the test file. The amount of data written to disk is still incorrect,
>> what causes damage to the file (file0005.raw), which's data lies next
>> to the test file. This will be fixed by the next patch.
>>
>> Feel free to prepare a proper sandbox/py_test based tests based on the
>> provided test scripts.
>> ---
>>  fs/fat/fat_write.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c
>> index 729cf39630..f946030f7d 100644
>> --- a/fs/fat/fat_write.c
>> +++ b/fs/fat/fat_write.c
>> @@ -794,6 +794,8 @@ set_contents(fsdata *mydata, dir_entry *dentptr,
>> loff_t pos, __u8 *buffer, 
>>  			newclust = get_fatent(mydata, endclust);
>>  
>> +			if (newclust != endclust + 1)
>> +				break;
>>  			if (IS_LAST_CLUST(newclust, mydata->fatsize))
>>  				break;
>>  			if (CHECK_CLUST(newclust, mydata->fatsize)) {
>> @@ -824,8 +826,6 @@ set_contents(fsdata *mydata, dir_entry *dentptr,
>> loff_t pos, __u8 *buffer, if (filesize <= cur_pos)
>>  			break;
>>  
>> -		/* CHECK: newclust = get_fatent(mydata, endclust); */
>> -
>>  		if (IS_LAST_CLUST(newclust, mydata->fatsize))
>>  			/* no more clusters */
>>  			break;
> 
> 
> 
> 
> Best regards,
> 
> Lukasz Majewski
> 
> --
> 
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
>
Tom Rini Dec. 5, 2019, 6:37 p.m. UTC | #3
On Thu, Dec 05, 2019 at 06:58:15PM +0100, Matthias Brugger wrote:
> 
> 
> On 05/12/2019 17:52, Lukasz Majewski wrote:
> > Hi Tom, Matthias,
> > 
> >> The code for handing file overwrite incorrectly assumed that the file
> >> on disk is always contiguous. This resulted in corrupting disk
> >> structure every time when write to existing fragmented file happened.
> >> Fix this by adding proper check for cluster discontinuity and adjust
> >> chunk size on each partial write.
> >>
> >> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> >> ---
> >>
> >> This patch partially fixes the issue revealed by the following test
> >> script:
> >>
> > 
> > Tom could you pic this patch and the following one (2/6):
> > https://patchwork.ozlabs.org/patch/1203101/
> > 
> > to -master as a fix?
> > 
> > This seems like a _real_ fix for FAT.
> 
> Right, I think the first patches should go in for v2020.01. I can send them
> together with some fixes for RPi I'm working on.
> 
> Tom what do you think?

I'd _really_ like to see the test that's in the commit message turned in
to a test we can run from CI as Travis will be doing those Soon(TM) and
GitLab/Azure do.  Then I'll pick them up (I'm testing some other fat
fixes right now).  Thanks!

Patch
diff mbox series

diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c
index 729cf39630..f946030f7d 100644
--- a/fs/fat/fat_write.c
+++ b/fs/fat/fat_write.c
@@ -794,6 +794,8 @@  set_contents(fsdata *mydata, dir_entry *dentptr, loff_t pos, __u8 *buffer,
 
 			newclust = get_fatent(mydata, endclust);
 
+			if (newclust != endclust + 1)
+				break;
 			if (IS_LAST_CLUST(newclust, mydata->fatsize))
 				break;
 			if (CHECK_CLUST(newclust, mydata->fatsize)) {
@@ -824,8 +826,6 @@  set_contents(fsdata *mydata, dir_entry *dentptr, loff_t pos, __u8 *buffer,
 		if (filesize <= cur_pos)
 			break;
 
-		/* CHECK: newclust = get_fatent(mydata, endclust); */
-
 		if (IS_LAST_CLUST(newclust, mydata->fatsize))
 			/* no more clusters */
 			break;