Re: [PATCH] disk: Move hardcoded max disk size literal to a GRUB_DISK_MAX_SECTORS in disk.h

[Glenn Washburn]

On Mon, 30 Nov 2020 15:42:14 +0100
Daniel Kiper <dkiper@net-space.pl> wrote:

> On Fri, Nov 27, 2020 at 02:36:23AM -0600, Glenn Washburn wrote:
> > There is a hardcoded maximum disk size that can be read or written
> > from, currently set at 1EiB in grub_disk_adjust_range. Move the
> > literal into a macro in disk.h, so our assumptions are more
> > visible. This hard coded limit does not prevent using larger disks,
> > just grub won't read/write past the limit. The comment accompanying
> > this restriction didn't quite make sense to me, so its been
> > modified.
> >
> > Signed-off-by: Glenn Washburn <development@efficientek.com>
> > ---
> >  grub-core/kern/disk_common.c | 13 +++++++------
> >  include/grub/disk.h          |  6 ++++++
> >  2 files changed, 13 insertions(+), 6 deletions(-)
> >
> > diff --git a/grub-core/kern/disk_common.c
> > b/grub-core/kern/disk_common.c index 5b92f670b..d20ed20d2 100644
> > --- a/grub-core/kern/disk_common.c
> > +++ b/grub-core/kern/disk_common.c
> > @@ -32,13 +32,14 @@ grub_disk_adjust_range (grub_disk_t disk,
> > grub_disk_addr_t *sector, /* Transform total_sectors to number of
> > 512B blocks.  */ total_sectors = grub_disk_from_native_sector
> > (disk, disk->total_sectors);
> >
> > -  /* Some drivers have problems with disks above reasonable.
> > -     Treat unknown as 1EiB disk. While on it, clamp the size to
> > 1EiB.
> > -     Just one condition is enough since GRUB_DISK_UNKNOWN_SIZE <<
> > ls is always
> > -     above 9EiB.
> > +  /*
> > +     Some drivers have problems with disks above reasonable sizes.
> > +     Clamp the size to GRUB_DISK_MAX_SECTORS. Just one condition
> > is enough
> > +     since GRUB_DISK_UNKNOWN_SIZE is always above
> > GRUB_DISK_MAX_SECTORS,
> > +     assuming a maximum 4KiB sector size.
> >    */
> > -  if (total_sectors > (1ULL << 51))
> > -    total_sectors = (1ULL << 51);
> > +  if (total_sectors > GRUB_DISK_MAX_SECTORS)
> > +    total_sectors = GRUB_DISK_MAX_SECTORS;
> >
> >    if ((total_sectors <= *sector
> >         || ((*offset + size + GRUB_DISK_SECTOR_SIZE - 1)
> > diff --git a/include/grub/disk.h b/include/grub/disk.h
> > index 1a9e8fcf4..0bf08091f 100644
> > --- a/include/grub/disk.h
> > +++ b/include/grub/disk.h
> > @@ -157,6 +157,12 @@ struct grub_disk_memberlist
> >  typedef struct grub_disk_memberlist *grub_disk_memberlist_t;
> >  #endif
> >
> > +/*
> > +   Some drivers have problems with disks above reasonable sizes.
> > +   Set max disk size at 1EiB.
> 
> The comment is formatted incorrectly [1].

Yep, copy/paste from the comment changed in previous diff which had an
incorrectly formatted comment and didn't catch it. Good catch. I'll
fix the comment in the previous diff as well.

> > +*/
> > +#define GRUB_DISK_MAX_SECTORS      (1ULL << (60 - 9))
> > +
> >  /* The sector size.  */
> >  #define GRUB_DISK_SECTOR_SIZE      0x200
> >  #define GRUB_DISK_SECTOR_BITS      9
> 
> Please move the constant below these definitions and do this:
>   #define GRUB_DISK_MAX_SECTORS               (1ULL << (60 -
> GRUB_DISK_SECTOR_BITS))

Great idea.

> [1]
> https://www.gnu.org/software/grub/manual/grub-dev/grub-dev.html#Comments