[PATCH] fs: fat: check for buffer size before reading blocks

[PATCH] fs: fat: check for buffer size before reading blocks

[Oleksandr Suvorov]

From: Ricardo Salveti <ricardo@foundries.io>

This patch optimizes the commit mentioned below by avoiding running
a set of commands which useless in the case when
size < mydata->sect_size and idx would be 0.

Fixes: 5b3ddb17ba ("fs/fat/fat.c: Do not perform zero block reads if there are no blocks left")

Signed-off-by: Ricardo Salveti <ricardo@foundries.io>
Co-developed-by: Oleksandr Suvorov <oleksandr.suvorov@foundries.io>
Signed-off-by: Oleksandr Suvorov <oleksandr.suvorov@foundries.io>
---

 fs/fat/fat.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/fs/fat/fat.c b/fs/fat/fat.c
index 7021138b987..4a509755442 100644
--- a/fs/fat/fat.c
+++ b/fs/fat/fat.c
@@ -275,14 +275,10 @@ get_cluster(fsdata *mydata, __u32 clustnum, __u8 *buffer, unsigned long size)
 			buffer += mydata->sect_size;
 			size -= mydata->sect_size;
 		}
-	} else {
+	} else if (size >= mydata->sect_size) {
 		__u32 idx;
-
 		idx = size / mydata->sect_size;
-		if (idx == 0)
-			ret = 0;
-		else
-			ret = disk_read(startsect, idx, buffer);
+		ret = disk_read(startsect, idx, buffer);
 		if (ret != idx) {
 			debug("Error reading data (got %d)\n", ret);
 			return -1;
-- 
2.31.1

Re: [PATCH] fs: fat: check for buffer size before reading blocks

[Heinrich Schuchardt]

On 9/16/21 4:09 PM, Oleksandr Suvorov wrote:
> From: Ricardo Salveti <ricardo@foundries.io>
>
> This patch optimizes the commit mentioned below by avoiding running
> a set of commands which useless in the case when

%s/which/which are/

> size < mydata->sect_size and idx would be 0.

Thank you for reviewing the FAT coding.

>
> Fixes: 5b3ddb17ba ("fs/fat/fat.c: Do not perform zero block reads if there are no blocks left")
>
> Signed-off-by: Ricardo Salveti <ricardo@foundries.io>
> Co-developed-by: Oleksandr Suvorov <oleksandr.suvorov@foundries.io>
> Signed-off-by: Oleksandr Suvorov <oleksandr.suvorov@foundries.io>
> ---
>
>   fs/fat/fat.c | 8 ++------
>   1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/fs/fat/fat.c b/fs/fat/fat.c
> index 7021138b987..4a509755442 100644
> --- a/fs/fat/fat.c
> +++ b/fs/fat/fat.c
> @@ -275,14 +275,10 @@ get_cluster(fsdata *mydata, __u32 clustnum, __u8 *buffer, unsigned long size)
>   			buffer += mydata->sect_size;
>   			size -= mydata->sect_size;
>   		}
> -	} else {
> +	} else if (size >= mydata->sect_size) {
>   		__u32 idx;

The variable name idx is a misnomer. Can we call it sect_count?

> -
>   		idx = size / mydata->sect_size;
> -		if (idx == 0)
> -			ret = 0;

size could be 0. In this case we still have to set ret = 0. Both for the
DMA aligned and the unaligned case. You could initialize ret = 0 where
it is defined.

> -		else
> -			ret = disk_read(startsect, idx, buffer);
> +		ret = disk_read(startsect, idx, buffer);
>   		if (ret != idx) {
>   			debug("Error reading data (got %d)\n", ret);
>   			return -1;
>

  291                 idx *= mydata->sect_size;
  292                 buffer += idx;
  293                 size -= idx;

These lines should be adjusted, too: size is defined as unsigned long.
We should not use the u32 variable idx but a separate variable of type
unsigned long to hold the number of bytes read.

Best regards

Heinrich

Re: [PATCH] fs: fat: check for buffer size before reading blocks

[Oleksandr Suvorov]

Hello Heinrich,

On Fri, Sep 17, 2021 at 5:36 AM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 9/16/21 4:09 PM, Oleksandr Suvorov wrote:
> > From: Ricardo Salveti <ricardo@foundries.io>
> >
> > This patch optimizes the commit mentioned below by avoiding running
> > a set of commands which useless in the case when
>
> %s/which/which are/
>
> > size < mydata->sect_size and idx would be 0.
>
> Thank you for reviewing the FAT coding.
>
> >
> > Fixes: 5b3ddb17ba ("fs/fat/fat.c: Do not perform zero block reads if there are no blocks left")
> >
> > Signed-off-by: Ricardo Salveti <ricardo@foundries.io>
> > Co-developed-by: Oleksandr Suvorov <oleksandr.suvorov@foundries.io>
> > Signed-off-by: Oleksandr Suvorov <oleksandr.suvorov@foundries.io>
> > ---
> >
> >   fs/fat/fat.c | 8 ++------
> >   1 file changed, 2 insertions(+), 6 deletions(-)
> >
> > diff --git a/fs/fat/fat.c b/fs/fat/fat.c
> > index 7021138b987..4a509755442 100644
> > --- a/fs/fat/fat.c
> > +++ b/fs/fat/fat.c
> > @@ -275,14 +275,10 @@ get_cluster(fsdata *mydata, __u32 clustnum, __u8 *buffer, unsigned long size)
> >                       buffer += mydata->sect_size;
> >                       size -= mydata->sect_size;
> >               }
> > -     } else {
> > +     } else if (size >= mydata->sect_size) {
> >               __u32 idx;
>
> The variable name idx is a misnomer. Can we call it sect_count?

Ok, sure.

>
> > -
> >               idx = size / mydata->sect_size;
> > -             if (idx == 0)
> > -                     ret = 0;
>
> size could be 0. In this case we still have to set ret = 0. Both for the
> DMA aligned and the unaligned case. You could initialize ret = 0
> where it is defined.

What for? There is no chance to use uninitialized "ret" in that function.

>
> > -             else
> > -                     ret = disk_read(startsect, idx, buffer);
> > +             ret = disk_read(startsect, idx, buffer);
> >               if (ret != idx) {
> >                       debug("Error reading data (got %d)\n", ret);
> >                       return -1;
> >
>
>   291                 idx *= mydata->sect_size;
>   292                 buffer += idx;
>   293                 size -= idx;
>
> These lines should be adjusted, too: size is defined as unsigned long.
> We should not use the u32 variable idx but a separate variable of type
> unsigned long to hold the number of bytes read.

The original variant uses the same variable for a sectors counter and for
the number of bytes read. I changed that part to use 2 different variables
to make the code more readable. As for now, the maximum size of a
cluster is 128 sectors (64Kb). This is the largest value for the parameter
"size". And accordingly for the variables sect_count and bytes_read.
IMHO even u16 would be enough for both these vars :) But the function
disk_read() waits for __u32 as a 2nd parameter, so it would be more
natural for sect_count to be __u32. What do you think?

Thanks for your detailed review!

I'm preparing the 2nd version of the patchset.

>
> Best regards
>
> Heinrich

-- 
Best regards
Oleksandr

Oleksandr Suvorov
cryosay@gmail.com