Re: [PATCH] Warn if MBR gap is small and user uses advanced modules

[Vladimir 'phcoder' Serbinenko]

On Wed, Mar 11, 2020, 18:08 Didier Spaier <address@hidden> wrote:

Le 11/03/2020 à 13:43, Daniel Kiper a écrit :
> Adding Michael, Mihai, Javier and Peter...
>
> Below you can find what more or less Vladimir and I agreed WRT small MBR
> gap. In general Vladimir convinced me to phase out small MBR gaps
> support gradually. This is first step in this journey. We think that we
> have to build some warnings into the code and extend documentation.
> Please chime in what you think about that...
>
> On Tue, Mar 10, 2020 at 01:23:10PM +0100, Vladimir 'phcoder' Serbinenko wrote:
>> Daniel, do you want to adjust the whitelist?
>>
>> We don't want to support small MBR gap in pair with anything but
>> the simplest config of biosdisk+part_msdos+simple filesystem. In this
>> path "simple filesystems" are all current filesystems except zfs and
>> btrfs
>
> Missing SOB...
>
>> ---
>>  grub-core/partmap/gpt.c     |  9 ++++++++-
>>  grub-core/partmap/msdos.c   |  7 ++++++-
>>  include/grub/partition.h    |  3 ++-
>>  include/grub/util/install.h |  7 +++++--
>>  util/grub-install-common.c  | 24 ++++++++++++++++++++++++
>>  util/grub-install.c         | 10 +++++++---
>>  util/grub-setup.c           |  2 +-
>>  util/setup.c                |  5 +++--
>>  8 files changed, 56 insertions(+), 11 deletions(-)
>>
>> diff --git a/grub-core/partmap/gpt.c b/grub-core/partmap/gpt.c
>> index 103f6796f..25a5a1934 100644
>> --- a/grub-core/partmap/gpt.c
>> +++ b/grub-core/partmap/gpt.c
>> @@ -25,6 +25,9 @@
>>  #include <grub/msdos_partition.h>
>>  #include <grub/gpt_partition.h>
>>  #include <grub/i18n.h>
>> +#ifdef GRUB_UTIL
>> +#include <grub/emu/misc.h>
>> +#endif
>>
>>  GRUB_MOD_LICENSE ("GPLv3+");
>>
>> @@ -169,7 +172,8 @@ static grub_err_t
>>  gpt_partition_map_embed (struct grub_disk *disk, unsigned int *nsectors,
>>   unsigned int max_nsectors,
>>   grub_embed_type_t embed_type,
>> - grub_disk_addr_t **sectors)
>> + grub_disk_addr_t **sectors,
>> + int warn_short)
>>  {
>>    struct gpt_partition_map_embed_ctx ctx = {
>>      .start = 0,
>> @@ -191,6 +195,9 @@ gpt_partition_map_embed (struct grub_disk *disk,
>> unsigned int *nsectors,
>>          N_("this GPT partition label contains no BIOS Boot Partition;"
>>     " embedding won't be possible"));
>>
>> +  if (ctx.len < 450) {
>
> Could you define constant somewhere?
>
> Is it in sectors? Why 450? Should not it be 2048 if 1 MiB below?

Whichs lead to a question: would the slot between 24K (or maybe
32K or 64K if it is wise to round it?) and 1M still a good fit for
a Bios boot partition in case of a gpt? if not in which cases?

BIOS boot partition should never be under 960K. Just it was never a problem to begin with.

> ...and missing "&& warn_short" check...
>
>> +    grub_util_warn("Your BIOS Boot Partition is under 1 MiB, please
>> increase its size.");
>> +  }
>>    if (ctx.len < *nsectors)
>
> Could not we use this check somehow instead of adding new one?
>
>>      return grub_error (GRUB_ERR_OUT_OF_RANGE,
>>          N_("your BIOS Boot Partition is too small;"
>> diff --git a/grub-core/partmap/msdos.c b/grub-core/partmap/msdos.c
>> index 7b8e45076..402bce050 100644
>> --- a/grub-core/partmap/msdos.c
>> +++ b/grub-core/partmap/msdos.c
>> @@ -236,7 +236,8 @@ static grub_err_t
>>  pc_partition_map_embed (struct grub_disk *disk, unsigned int *nsectors,
>>   unsigned int max_nsectors,
>>   grub_embed_type_t embed_type,
>> - grub_disk_addr_t **sectors)
>> + grub_disk_addr_t **sectors,
>> + int warn_short)
>>  {
>>    grub_disk_addr_t end = ~0ULL;
>>    struct grub_msdos_partition_mbr mbr;
>> @@ -390,6 +391,10 @@ pc_partition_map_embed (struct grub_disk *disk,
>> unsigned int *nsectors,
>>        return GRUB_ERR_NONE;
>>      }
>>
>> +  if (end < 450 && warn_short) {
>> +    grub_util_warn("You have a short MBR gap and use advanced config.
>> Please increase post-MBR gap");
>
> Ditto?
>
>> +  }
>> +
>>    if (end <= 1)
>>      return grub_error (GRUB_ERR_FILE_NOT_FOUND,
>>          N_("this msdos-style partition label has no "
>> diff --git a/include/grub/partition.h b/include/grub/partition.h
>> index 7adb7ec6e..5697e28d5 100644
>> --- a/include/grub/partition.h
>> +++ b/include/grub/partition.h
>> @@ -55,7 +55,8 @@ struct grub_partition_map
>>    grub_err_t (*embed) (struct grub_disk *disk, unsigned int *nsectors,
>>          unsigned int max_nsectors,
>>          grub_embed_type_t embed_type,
>> -        grub_disk_addr_t **sectors);
>> +        grub_disk_addr_t **sectors,
>> +        int warn_short);
>>  #endif
>>  };
>>  typedef struct grub_partition_map *grub_partition_map_t;
>> diff --git a/include/grub/util/install.h b/include/grub/util/install.h
>> index 2631b1074..982115f57 100644
>> --- a/include/grub/util/install.h
>> +++ b/include/grub/util/install.h
>> @@ -193,13 +193,13 @@ grub_util_bios_setup (const char *dir,
>>         const char *boot_file, const char *core_file,
>>         const char *dest, int force,
>>         int fs_probe, int allow_floppy,
>> -       int add_rs_codes);
>> +       int add_rs_codes, int warn_short_mbr_gap);
>>  void
>>  grub_util_sparc_setup (const char *dir,
>>          const char *boot_file, const char *core_file,
>>          const char *dest, int force,
>>          int fs_probe, int allow_floppy,
>> -        int add_rs_codes);
>> +        int add_rs_codes, int warn_short_mbr_gap);
>>
>>  char *
>>  grub_install_get_image_targets_string (void);
>> @@ -265,4 +265,7 @@ grub_util_get_target_name (const struct
>> grub_install_image_target_desc *t);
>>  extern char *grub_install_copy_buffer;
>>  #define GRUB_INSTALL_COPY_BUFFER_SIZE 1048576
>>
>> +int
>
> extern int
>
>> +grub_install_is_short_mgrgap_supported(void);
>> +
>>  #endif
>> diff --git a/util/grub-install-common.c b/util/grub-install-common.c
>> index ca0ac612a..e4cff2871 100644
>> --- a/util/grub-install-common.c
>> +++ b/util/grub-install-common.c
>> @@ -234,6 +234,30 @@ char *grub_install_source_directory = NULL;
>>  char *grub_install_locale_directory = NULL;
>>  char *grub_install_themes_directory = NULL;
>>
>> +int
>> +grub_install_is_short_mgrgap_supported()
>> +{
>> +  int i, j;
>> +  static const char *whitelist[] =
>> +    {
>> +     "part_msdos", "biosdisk", "affs", "afs", "bfs", "archelp",
>> +     "cpio", "cpio_be", "newc", "odc", "ext2", "fat", "exfat",
>> +     "f2fs", "fshelp", "hfs", "hfsplus", "hfspluscomp",
>> +     "iso9660", "jfs", "minix", "minix2", "minix3", "minix_be",
>> +     "minix2_be", "minix2_be", "nilfs2", "ntfs", "ntfscomp",
>> +     "reiserfs", "romfs", "sfs", "squash4", "tar", "udf",
>> +     "ufs1", "ufs1_be", "ufs2", "xfs"
>> +    };
>
> The list LGTM...
>
>> +  for (i = 0; i < modules.n_entries; i++) {
>> +    for (j = 0; j < ARRAY_SIZE (whitelist); j++)
>> +      if (strcmp(modules.entries[i], whitelist[j]) == 0)
>> + break;
>> +    if (j == ARRAY_SIZE (whitelist))
>> +      return 0;
>> +  }
>> +  return 1;
>> +}
>> +
>>  void
>>  grub_install_push_module (const char *val)
>>  {
>> diff --git a/util/grub-install.c b/util/grub-install.c
>> index 8970b73aa..ba27657d9 100644
>> --- a/util/grub-install.c
>> +++ b/util/grub-install.c
>> @@ -1718,10 +1718,14 @@ main (int argc, char *argv[])
>>   install_device);
>>
>>   /*  Now perform the installation.  */
>> - if (install_bootsector)
>> + if (install_bootsector) {
>> +   int warn_short_mbr_gap = grub_install_is_short_mgrgap_supported();
>> +
>>     grub_util_bios_setup (platdir, "boot.img", "core.img",
>>   install_drive, force,
>> - fs_probe, allow_floppy, add_rs_codes);
>> + fs_probe, allow_floppy, add_rs_codes,
>> + warn_short_mbr_gap);
>> + }
>>   break;
>>        }
>>      case GRUB_INSTALL_PLATFORM_SPARC64_IEEE1275:
>> @@ -1748,7 +1752,7 @@ main (int argc, char *argv[])
>>     grub_util_sparc_setup (platdir, "boot.img", "core.img",
>>   install_drive, force,
>>   fs_probe, allow_floppy,
>> - 0 /* unused */ );
>> + 0 /* unused */, 0 /* unused */ );
>>   break;
>>        }
>>
>> diff --git a/util/grub-setup.c b/util/grub-setup.c
>> index 42b98ad3c..1783224dd 100644
>> --- a/util/grub-setup.c
>> +++ b/util/grub-setup.c
>> @@ -315,7 +315,7 @@ main (int argc, char *argv[])
>>      arguments.core_file ? : DEFAULT_CORE_FILE,
>>      dest_dev, arguments.force,
>>      arguments.fs_probe, arguments.allow_floppy,
>> -    arguments.add_rs_codes);
>> +    arguments.add_rs_codes, 0);
>>
>>    /* Free resources.  */
>>    grub_fini_all ();
>> diff --git a/util/setup.c b/util/setup.c
>> index 3be88aae1..da5f2c07f 100644
>> --- a/util/setup.c
>> +++ b/util/setup.c
>> @@ -254,7 +254,8 @@ SETUP (const char *dir,
>>         const char *boot_file, const char *core_file,
>>         const char *dest, int force,
>>         int fs_probe, int allow_floppy,
>> -       int add_rs_codes __attribute__ ((unused))) /* unused on sparc64 */
>> +       int add_rs_codes __attribute__ ((unused)), /* unused on sparc64 */
>> +       int warn_small)
>>  {
>>    char *core_path;
>>    char *boot_img, *core_img, *boot_path;
>> @@ -530,7 +531,7 @@ SETUP (const char *dir,
>>   GRUB_EMBED_PCBIOS, &sectors);
>>      else if (ctx.dest_partmap)
>>        err = ctx.dest_partmap->embed (dest_dev->disk, &nsec, maxsec,
>> -      GRUB_EMBED_PCBIOS, &sectors);
>> +      GRUB_EMBED_PCBIOS, &sectors, warn_small);
>>      else
>>        err = fs->fs_embed (dest_dev, &nsec, maxsec,
>>     GRUB_EMBED_PCBIOS, &sectors);
>
> ...plus some missing spaces in function names and other places...
>
> Additionally, please extend relevant doc section with explanation why we
> are doing that. And maybe with an schedule for phase out...
>
> Daniel
>
> _______________________________________________
> Grub-devel mailing list
> address@hidden
> https://lists.gnu.org/mailman/listinfo/grub-devel
>

_______________________________________________
Grub-devel mailing list
address@hidden
https://lists.gnu.org/mailman/listinfo/grub-devel