| Message ID | 1443447932-14139-1-git-send-email-benoit@wsystem.com |
|---|---|
| State | Accepted |
| Delegated to: | Tom Rini |
| Headers | show |
On Mon, Sep 28, 2015 at 03:45:28PM +0200, Benoît Thébaudeau wrote: > set_cluster() was using a temporary buffer without enforcing its > alignment for DMA and cache. Moreover, it did not check the alignment of > the passed buffer, which can come directly from applicative code or from > the user. > > This could cause random data corruption, which has been observed on > i.MX25 writing to an SD card. > > Fix this by only passing ARCH_DMA_MINALIGN-aligned buffers to > disk_write(), which requires the introduction of a buffer bouncing > mechanism for the misaligned buffers passed to set_cluster(). > > By the way, improve the handling of the corresponding return values from > disk_write(): > - print them with debug() in case of error, > - consider that there is an error is disk_write() returns a smaller > block count than the requested one, not only if its return value is > negative. > > After this change, set_cluster() and get_cluster() are almost > symmetrical. > > Signed-off-by: Benoît Thébaudeau <benoit@wsystem.com> OK. I know Stephen has a series to replace all of the FAT code for the next release once some performance issues are addressed. But I'm inclined to take this series (after some reviews and so forth) for this release at least because this sounds like some bad bugs and more things are starting to rely on fatwrite functionality (for example, env saved as a file in FAT is getting common on community-style boards).
Hi Tom, On Mon, Sep 28, 2015 at 5:22 PM, Tom Rini <trini@konsulko.com> wrote: > On Mon, Sep 28, 2015 at 03:45:28PM +0200, Benoît Thébaudeau wrote: > >> set_cluster() was using a temporary buffer without enforcing its >> alignment for DMA and cache. Moreover, it did not check the alignment of >> the passed buffer, which can come directly from applicative code or from >> the user. >> >> This could cause random data corruption, which has been observed on >> i.MX25 writing to an SD card. >> >> Fix this by only passing ARCH_DMA_MINALIGN-aligned buffers to >> disk_write(), which requires the introduction of a buffer bouncing >> mechanism for the misaligned buffers passed to set_cluster(). >> >> By the way, improve the handling of the corresponding return values from >> disk_write(): >> - print them with debug() in case of error, >> - consider that there is an error is disk_write() returns a smaller >> block count than the requested one, not only if its return value is >> negative. >> >> After this change, set_cluster() and get_cluster() are almost >> symmetrical. >> >> Signed-off-by: Benoît Thébaudeau <benoit@wsystem.com> > > OK. I know Stephen has a series to replace all of the FAT code for > the next release once some performance issues are addressed. But I'm > inclined to take this series (after some reviews and so forth) for this > release at least because this sounds like some bad bugs and more things > are starting to rely on fatwrite functionality (for example, env saved > as a file in FAT is getting common on community-style boards). I agree, but there is not much review activity. Do you know anyone else who might be interested in this and who should be Cc'ed? Best regards, Benoît
On Mon, Sep 28, 2015 at 03:45:28PM +0200, Benoît Thébaudeau wrote: > set_cluster() was using a temporary buffer without enforcing its > alignment for DMA and cache. Moreover, it did not check the alignment of > the passed buffer, which can come directly from applicative code or from > the user. > > This could cause random data corruption, which has been observed on > i.MX25 writing to an SD card. > > Fix this by only passing ARCH_DMA_MINALIGN-aligned buffers to > disk_write(), which requires the introduction of a buffer bouncing > mechanism for the misaligned buffers passed to set_cluster(). > > By the way, improve the handling of the corresponding return values from > disk_write(): > - print them with debug() in case of error, > - consider that there is an error is disk_write() returns a smaller > block count than the requested one, not only if its return value is > negative. > > After this change, set_cluster() and get_cluster() are almost > symmetrical. > > Signed-off-by: Benoît Thébaudeau <benoit@wsystem.com> Applied to u-boot/master, thanks!
diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c index adb6940..d0d9df7 100644 --- a/fs/fat/fat_write.c +++ b/fs/fat/fat_write.c @@ -555,8 +555,9 @@ static int set_cluster(fsdata *mydata, __u32 clustnum, __u8 *buffer, unsigned long size) { - int idx = 0; + __u32 idx = 0; __u32 startsect; + int ret; if (clustnum > 0) startsect = mydata->data_begin + @@ -566,26 +567,45 @@ set_cluster(fsdata *mydata, __u32 clustnum, __u8 *buffer, debug("clustnum: %d, startsect: %d\n", clustnum, startsect); - if ((size / mydata->sect_size) > 0) { - if (disk_write(startsect, size / mydata->sect_size, buffer) < 0) { - debug("Error writing data\n"); + if ((unsigned long)buffer & (ARCH_DMA_MINALIGN - 1)) { + ALLOC_CACHE_ALIGN_BUFFER(__u8, tmpbuf, mydata->sect_size); + + printf("FAT: Misaligned buffer address (%p)\n", buffer); + + while (size >= mydata->sect_size) { + memcpy(tmpbuf, buffer, mydata->sect_size); + ret = disk_write(startsect++, 1, tmpbuf); + if (ret != 1) { + debug("Error writing data (got %d)\n", ret); + return -1; + } + + buffer += mydata->sect_size; + size -= mydata->sect_size; + } + } else if (size >= mydata->sect_size) { + idx = size / mydata->sect_size; + ret = disk_write(startsect, idx, buffer); + if (ret != idx) { + debug("Error writing data (got %d)\n", ret); return -1; } + + startsect += idx; + idx *= mydata->sect_size; + buffer += idx; + size -= idx; } - if (size % mydata->sect_size) { - __u8 tmpbuf[mydata->sect_size]; + if (size) { + ALLOC_CACHE_ALIGN_BUFFER(__u8, tmpbuf, mydata->sect_size); - idx = size / mydata->sect_size; - buffer += idx * mydata->sect_size; - memcpy(tmpbuf, buffer, size % mydata->sect_size); - - if (disk_write(startsect + idx, 1, tmpbuf) < 0) { - debug("Error writing data\n"); + memcpy(tmpbuf, buffer, size); + ret = disk_write(startsect, 1, tmpbuf); + if (ret != 1) { + debug("Error writing data (got %d)\n", ret); return -1; } - - return 0; } return 0;
set_cluster() was using a temporary buffer without enforcing its alignment for DMA and cache. Moreover, it did not check the alignment of the passed buffer, which can come directly from applicative code or from the user. This could cause random data corruption, which has been observed on i.MX25 writing to an SD card. Fix this by only passing ARCH_DMA_MINALIGN-aligned buffers to disk_write(), which requires the introduction of a buffer bouncing mechanism for the misaligned buffers passed to set_cluster(). By the way, improve the handling of the corresponding return values from disk_write(): - print them with debug() in case of error, - consider that there is an error is disk_write() returns a smaller block count than the requested one, not only if its return value is negative. After this change, set_cluster() and get_cluster() are almost symmetrical. Signed-off-by: Benoît Thébaudeau <benoit@wsystem.com> --- fs/fat/fat_write.c | 48 ++++++++++++++++++++++++++++++++++-------------- 1 file changed, 34 insertions(+), 14 deletions(-)