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

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

Commit Message

Marek Szyprowski Nov. 26, 2019, 8:15 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>
Reviewed-by: Oleksandr Suvorov <oleksandr.suvorov@toradex.com>
Reviewed-by: Lukasz Majewski <lukma@denx.de>
---
 fs/fat/fat_write.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Tom Rini Nov. 26, 2019, 4:57 p.m. UTC | #1
On Tue, Nov 26, 2019 at 09:15:07AM +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>
> Reviewed-by: Oleksandr Suvorov <oleksandr.suvorov@toradex.com>
> Reviewed-by: Lukasz Majewski <lukma@denx.de>
> ---
>  fs/fat/fat_write.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c
> index 729cf39630..6cfa5b4565 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 - 1) != endclust)
> +				break;
>  			if (IS_LAST_CLUST(newclust, mydata->fatsize))
>  				break;
>  			if (CHECK_CLUST(newclust, mydata->fatsize)) {
> @@ -811,7 +813,7 @@ set_contents(fsdata *mydata, dir_entry *dentptr, loff_t pos, __u8 *buffer,
>  			offset = 0;
>  		else
>  			offset = pos - cur_pos;
> -		wsize = min(cur_pos + actsize, filesize) - pos;
> +		wsize = min_t(unsigned long long, actsize, filesize - cur_pos);
>  		if (get_set_cluster(mydata, curclust, offset,
>  				    buffer, wsize, &actsize)) {
>  			printf("Error get-and-setting cluster\n");
> @@ -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;

Adding in Heinrich and Akashi-san for more review on this, thanks!
AKASHI Takahiro Nov. 27, 2019, 2:26 a.m. UTC | #2
Thank you for the heads-up.

On Tue, Nov 26, 2019 at 11:57:29AM -0500, Tom Rini wrote:
> On Tue, Nov 26, 2019 at 09:15:07AM +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>
> > Reviewed-by: Oleksandr Suvorov <oleksandr.suvorov@toradex.com>
> > Reviewed-by: Lukasz Majewski <lukma@denx.de>
> > ---
> >  fs/fat/fat_write.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c
> > index 729cf39630..6cfa5b4565 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 - 1) != endclust)

"newclust != (endclust + 1)" would be more intuitive?
But it's just my preference.

> > +				break;
> >  			if (IS_LAST_CLUST(newclust, mydata->fatsize))
> >  				break;
> >  			if (CHECK_CLUST(newclust, mydata->fatsize)) {
> > @@ -811,7 +813,7 @@ set_contents(fsdata *mydata, dir_entry *dentptr, loff_t pos, __u8 *buffer,
> >  			offset = 0;
> >  		else
> >  			offset = pos - cur_pos;
> > -		wsize = min(cur_pos + actsize, filesize) - pos;
> > +		wsize = min_t(unsigned long long, actsize, filesize - cur_pos);

This hunk is not directly related to the issue, is it?

> >  		if (get_set_cluster(mydata, curclust, offset,
> >  				    buffer, wsize, &actsize)) {
> >  			printf("Error get-and-setting cluster\n");
> > @@ -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;
> 
> Adding in Heinrich and Akashi-san for more review on this, thanks!

Otherwise, it looks good.
Reviewed-by: AKASHI Takahiro <takahiro.akashi@linaro.org>


> -- 
> Tom
Marek Szyprowski Nov. 27, 2019, 12:01 p.m. UTC | #3
Hi,

On 27.11.2019 03:26, AKASHI Takahiro wrote:
> Thank you for the heads-up.
>
> On Tue, Nov 26, 2019 at 11:57:29AM -0500, Tom Rini wrote:
>> On Tue, Nov 26, 2019 at 09:15:07AM +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>
>>> Reviewed-by: Oleksandr Suvorov <oleksandr.suvorov@toradex.com>
>>> Reviewed-by: Lukasz Majewski <lukma@denx.de>
>>> ---
>>>   fs/fat/fat_write.c | 6 +++---
>>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c
>>> index 729cf39630..6cfa5b4565 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 - 1) != endclust)
> "newclust != (endclust + 1)" would be more intuitive?
> But it's just my preference.

Indeed.

>>> +				break;
>>>   			if (IS_LAST_CLUST(newclust, mydata->fatsize))
>>>   				break;
>>>   			if (CHECK_CLUST(newclust, mydata->fatsize)) {
>>> @@ -811,7 +813,7 @@ set_contents(fsdata *mydata, dir_entry *dentptr, loff_t pos, __u8 *buffer,
>>>   			offset = 0;
>>>   		else
>>>   			offset = pos - cur_pos;
>>> -		wsize = min(cur_pos + actsize, filesize) - pos;
>>> +		wsize = min_t(unsigned long long, actsize, filesize - cur_pos);
> This hunk is not directly related to the issue, is it?

It is partially related. I remember that it was not calculated correctly 
for the fragmented files and then discovered that there was one more 
case in which the current formula failed.

>>>   		if (get_set_cluster(mydata, curclust, offset,
>>>   				    buffer, wsize, &actsize)) {
>>>   			printf("Error get-and-setting cluster\n");
>>> @@ -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;
>> Adding in Heinrich and Akashi-san for more review on this, thanks!
> Otherwise, it looks good.
> Reviewed-by: AKASHI Takahiro <takahiro.akashi@linaro.org>

Best regards

Patch
diff mbox series

diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c
index 729cf39630..6cfa5b4565 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 - 1) != endclust)
+				break;
 			if (IS_LAST_CLUST(newclust, mydata->fatsize))
 				break;
 			if (CHECK_CLUST(newclust, mydata->fatsize)) {
@@ -811,7 +813,7 @@  set_contents(fsdata *mydata, dir_entry *dentptr, loff_t pos, __u8 *buffer,
 			offset = 0;
 		else
 			offset = pos - cur_pos;
-		wsize = min(cur_pos + actsize, filesize) - pos;
+		wsize = min_t(unsigned long long, actsize, filesize - cur_pos);
 		if (get_set_cluster(mydata, curclust, offset,
 				    buffer, wsize, &actsize)) {
 			printf("Error get-and-setting cluster\n");
@@ -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;