| Message ID | 20191202111118.12868-2-m.szyprowski@samsung.com |
|---|---|
| State | Accepted |
| Commit | 5e615b74e8c8dedcc1ad9a49a04b01a1c216d8bc |
| Delegated to: | Matthias Brugger |
| Headers | show |
| Series | Raspberry Pi4: add support for DFU over USB | expand |
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
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 >
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!
On Mon, Dec 02, 2019 at 12:11:13PM +0100, Marek Szyprowski wrote: > 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 Applied to u-boot/master, thanks!
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;
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(-)