[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 at gmx.de> wrote:
>
> On 9/16/21 4:09 PM, Oleksandr Suvorov wrote:
> > From: Ricardo Salveti <ricardo at 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 at foundries.io>
> > Co-developed-by: Oleksandr Suvorov <oleksandr.suvorov at foundries.io>
> > Signed-off-by: Oleksandr Suvorov <oleksandr.suvorov at 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 at gmail.com