[PATCH] Improve ext2 driver to allow embedding of the boot loader code.

[PATCH] Improve ext2 driver to allow embedding of the boot loader code.

[Dr. Tilmann Bubeck]

This patch extends the ext2 driver to allow embedding core.img into the
filesystem. This allows the long needed installation of GRUB into a partition
without the need to use (unsafe) block lists.

It is realized using the ioctl(EXT4_IOC_SWAP_BOOT) introduced into
Linux 3.10. This ioctl basically swaps the data blocks of the associated
file with the EXT2_BOOT_LOADER_INO inode. After using the ioctl the code
of core.img is stored in the BOOT_LOADER incode of the filesystem and it
is not accessible to file system tools anymore. This ensures, that the boot
code is safe and installation into a partition is possible.

The patchs needs 0001-Refactor-grub_read_mountinfo-out-of-grub_find_root_d.patch
to work correctly.

Signed-off-by: Dr. Tilmann Bubeck <tilmann@bubecks.de>
---
 grub-core/fs/ext2.c | 212 ++++++++++++++++++++++++++++++++++++++++++++++++++--
 util/setup.c        |  11 ++-
 2 files changed, 215 insertions(+), 8 deletions(-)

diff --git a/grub-core/fs/ext2.c b/grub-core/fs/ext2.c
index 5f7a2b9..643969e 100644
--- a/grub-core/fs/ext2.c
+++ b/grub-core/fs/ext2.c
@@ -133,6 +133,14 @@ GRUB_MOD_LICENSE ("GPLv3+");
 
 #define EXT4_EXTENTS_FLAG		0x80000
 
+/*
+ * Special inodes numbers
+ */
+#define EXT2_ROOT_INO            2      /* Root inode */
+#define EXT2_BOOT_LOADER_INO     5      /* Boot loader inode */
+
+#define EXT4_IOC_SWAP_BOOT		_IO('f', 17)
+
 /* The ext2 superblock.  */
 struct grub_ext2_sblock
 {
@@ -393,10 +401,10 @@ grub_ext4_find_leaf (struct grub_ext2_data *data,
 }
 
 static grub_disk_addr_t
-grub_ext2_read_block (grub_fshelp_node_t node, grub_disk_addr_t fileblock)
+grub_ext2_read_block2 (struct grub_ext2_data *data,
+		       struct grub_ext2_inode *inode,
+		       grub_disk_addr_t fileblock)
 {
-  struct grub_ext2_data *data = node->data;
-  struct grub_ext2_inode *inode = &node->inode;
   unsigned int blksz = EXT2_BLOCK_SIZE (data);
   grub_disk_addr_t blksz_quarter = blksz / 4;
   int log2_blksz = LOG2_EXT2_BLOCK_SIZE (data);
@@ -497,6 +505,12 @@ indirect:
   return grub_le_to_cpu32 (indir);
 }
 
+static grub_disk_addr_t
+grub_ext2_read_block (grub_fshelp_node_t node, grub_disk_addr_t fileblock)
+{
+  return grub_ext2_read_block2(node->data, &node->inode, fileblock);
+}
+
 /* Read LEN bytes from the file described by DATA starting with byte
    POS.  Return the amount of read bytes in READ.  */
 static grub_ssize_t
@@ -607,12 +621,12 @@ grub_ext2_mount (grub_disk_t disk)
   data->disk = disk;
 
   data->diropen.data = data;
-  data->diropen.ino = 2;
+  data->diropen.ino = EXT2_ROOT_INO;
   data->diropen.inode_read = 1;
 
   data->inode = &data->diropen.inode;
 
-  grub_ext2_read_inode (data, 2, data->inode);
+  grub_ext2_read_inode (data, EXT2_ROOT_INO, data->inode);
   if (grub_errno)
     goto fail;
 
@@ -977,6 +991,193 @@ grub_ext2_mtime (grub_device_t device, grub_int32_t *tm)
 
 }
 
+#ifdef GRUB_UTIL
+
+#include <stdlib.h>
+#include <sys/ioctl.h>
+#include <include/grub/emu/misc.h>
+#include <include/grub/emu/getroot.h>
+#include <include/grub/emu/hostfile.h>
+
+/* Read the addresses of all sectors occupied by the file with the
+   given inode from the filesystem.  Return the number of sectors in
+   "nsector" and the addresses in "sectors".  "sectors" is allocated
+   in this function and must be freed by the caller after usage.
+   A sector in sectors has size GRUB_DISK_SECTOR_SIZE. */
+static grub_err_t
+grub_ext2_read_sectorlist(grub_device_t device,
+			 int ino,
+			 unsigned int *nsectors,
+			 grub_disk_addr_t **sectors)
+{
+  struct grub_ext2_data *data;
+  struct grub_ext2_inode inode;
+  grub_off_t size;
+  grub_err_t err;
+  grub_disk_addr_t fileblock;
+  int i;
+  grub_disk_addr_t fileblock_count;
+  int sectors_per_block;
+
+  data = grub_ext2_mount (device->disk);
+  if (! data) {
+    return grub_error (grub_errno, N_("Unable to mount device"));
+  }
+
+  err = grub_ext2_read_inode (data, ino, &inode);
+  if (err)
+    return grub_error (err, N_("Unable to read inode #%d"), ino);
+
+  size = grub_le_to_cpu32 (inode.size);
+  size |= ((grub_off_t) grub_le_to_cpu32 (inode.size_high)) << 32;
+
+  fileblock_count = size / EXT2_BLOCK_SIZE(data);
+  if ( size % EXT2_BLOCK_SIZE(data) != 0 ) fileblock_count++;
+
+  sectors_per_block = EXT2_BLOCK_SIZE(data) / GRUB_DISK_SECTOR_SIZE;
+  *sectors = grub_malloc (fileblock_count * sectors_per_block
+			  * sizeof (**sectors));
+  *nsectors = 0;
+  for ( fileblock = 0; fileblock < fileblock_count; fileblock++ ) {
+    (*sectors)[*nsectors] = grub_ext2_read_block2 (data, &inode, fileblock)
+	                    * sectors_per_block;
+    for ( i = 1; i < sectors_per_block; i++) {
+      (*sectors)[(*nsectors) + i] = (*sectors)[*nsectors] + i;
+    }
+    (*nsectors) += sectors_per_block;
+  }
+
+  return GRUB_ERR_NONE;
+}
+
+static grub_err_t
+grub_ext2_embed (grub_device_t device,
+		  unsigned int *nsectors,
+		  unsigned int max_nsectors,
+		  grub_embed_type_t embed_type,
+		  grub_disk_addr_t **sectors)
+{
+  unsigned i;
+  grub_err_t err;
+  char *mountpoint;
+  struct mountinfo_entry *entries;
+  grub_util_fd_t out;
+  char *core_name;
+  char diskbuffer[GRUB_DISK_SECTOR_SIZE];
+  unsigned int nsectors_wanted;
+  char *device_name;
+  int ioctl_err;
+
+  if (embed_type != GRUB_EMBED_PCBIOS)
+    return grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET,
+		       N_("ext2 currently supports only PC-BIOS embedding"));
+
+  if ( device->disk->partition)
+      device_name = grub_xasprintf ("%s,%s", device->disk->name,
+			   grub_partition_get_name(device->disk->partition));
+  else
+      device_name = grub_xasprintf ("%s", device->disk->name);
+
+  nsectors_wanted = *nsectors;
+
+  grub_util_info (N_("Embedding %d/%d sectors into ext2 filesystem of %s"),
+		 nsectors_wanted, max_nsectors, device_name);
+
+  /* [1] Check, if the existing boot loader inode exists and has the
+         wanted size: */
+  err = grub_ext2_read_sectorlist (device, EXT2_BOOT_LOADER_INO,
+				   nsectors, sectors);
+  if (!err && *nsectors >= nsectors_wanted && *nsectors <= max_nsectors) {
+      grub_util_info(N_("Reusing existing boot loader inode"
+			" offering %d sectors"),
+		   *nsectors);
+    grub_free (device_name);
+    return GRUB_ERR_NONE;       /* YES! Everything is fine. */
+  }
+
+  /* [2] We have to create a new boot loader inode. */
+
+  /* [2.1] Check if device is mounted and get mountpoint: */
+  mountpoint = NULL;
+  entries = grub_read_mountinfo ();
+  if ( entries ) {
+    for ( i = 0; entries[i].enc_root[0] != 0; i++) {
+      char *grub_dev_of_mount;
+      grub_errno = GRUB_ERR_NONE;    /* Clear errno set previously */
+      grub_dev_of_mount = grub_util_get_grub_dev (entries[i].device);
+      if ( grub_dev_of_mount ) {
+	if ( grub_strcmp (grub_dev_of_mount, device_name) == 0 ) {
+	  mountpoint = grub_strdup (entries[i].enc_path);
+	  break;
+	}
+      }
+    }
+    free (entries);
+  }
+
+  grub_errno = GRUB_ERR_NONE;    /* Clear errno set previously */
+
+  if (!mountpoint) {
+    /* We were unable to find the mountpoint for the device of the
+       filesystem. Maybe it is not mounted? */
+    return grub_error (GRUB_ERR_UNKNOWN_DEVICE,
+		       N_("We are unable to find the mountpoint of %s"),
+		       device_name);
+
+    /** ToDo: Try to mount the filesystem to a temporary location. */
+  }
+
+  grub_free (device_name);
+
+  /* [2.2] Create a new (temporary) file, which then gets the boot loader: */
+  core_name = grub_util_path_concat (2, mountpoint, ".core.img");
+  free (mountpoint);
+  out = grub_util_fd_open (core_name, GRUB_UTIL_FD_O_WRONLY
+			   | GRUB_UTIL_FD_O_CREATTRUNC);
+  if (!GRUB_UTIL_FD_IS_VALID (out)) {
+    return grub_error (GRUB_ERR_BAD_FS,
+		       N_("cannot create `%s': %s"), core_name,
+		       grub_util_fd_strerror ());
+  }
+  for ( i = 0; i < max_nsectors; i++ ) {
+      grub_util_fd_write (out, diskbuffer, GRUB_DISK_SECTOR_SIZE);
+  }
+  grub_util_fd_sync (out);
+
+  /* [2.3] Make that file to the new boot loader inode by swapping the
+           file of "out" with the boot loader inode: */
+  ioctl_err = ioctl (out, EXT4_IOC_SWAP_BOOT);
+  if ( ioctl_err ) {
+    err = grub_error (GRUB_ERR_BAD_FS,
+		      N_("Error in ioctl(EXT4_IOC_SWAP_BOOT);"
+			 "you need Linux >= 3.10: %s"),
+		      grub_util_fd_strerror ());
+    grub_util_fd_close (out);
+    grub_util_unlink (core_name);
+    return err;
+  }
+  grub_util_fd_close (out);
+
+  /* [2.4] Unlink the core file, now containing the previous boot loader. */
+  grub_util_unlink (core_name);
+
+  /* [2.5] Invalidate disk cache and read block list again: */
+  grub_disk_cache_invalidate_all ();
+
+  err = grub_ext2_read_sectorlist (device, EXT2_BOOT_LOADER_INO,
+				   nsectors, sectors);
+  if ( err ) {
+    return grub_error (GRUB_ERR_BAD_FS,
+		       N_("unable to read boot loader inode"));
+  }
+
+  grub_util_info (N_("Created new boot loader inode offering %d sectors"),
+		     *nsectors);
+
+  return GRUB_ERR_NONE;
+}
+#endif
+
 
 \f
 static struct grub_fs grub_ext2_fs =
@@ -990,6 +1191,7 @@ static struct grub_fs grub_ext2_fs =
     .uuid = grub_ext2_uuid,
     .mtime = grub_ext2_mtime,
 #ifdef GRUB_UTIL
+    .embed = grub_ext2_embed,
     .reserved_first_sector = 1,
     .blocklist_install = 1,
 #endif
diff --git a/util/setup.c b/util/setup.c
index 9fb91a8..3f4a007 100644
--- a/util/setup.c
+++ b/util/setup.c
@@ -509,8 +509,10 @@ SETUP (const char *dir,
     if (!err && nsec < core_sectors)
       {
 	err = grub_error (GRUB_ERR_OUT_OF_RANGE,
-			  N_("Your embedding area is unusually small.  "
-			     "core.img won't fit in it."));
+			  N_("Your embedding area is unusually small "
+			     "(only %d sectors). "
+			     "core.img won't fit in it (needs %d sectors)."),
+			  nsec, core_sectors);
       }
     
     if (err)
@@ -583,10 +585,13 @@ SETUP (const char *dir,
       }
 
     /* Write the core image onto the disk.  */
-    for (i = 0; i < nsec; i++)
+    for (i = 0; i < nsec; i++) {
+	grub_util_info ("writing core.img/%d to sector %" PRIuGRUB_UINT64_T, i,
+			sectors[i]);
       grub_disk_write (dest_dev->disk, sectors[i], 0,
 		       GRUB_DISK_SECTOR_SIZE,
 		       core_img + i * GRUB_DISK_SECTOR_SIZE);
+    }
 
     grub_free (sectors);
 
-- 
1.8.1.4

Re: [PATCH] Improve ext2 driver to allow embedding of the boot loader code.

[Vladimir 'φ-coder/phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 11425 bytes --]

On 09.01.2014 22:07, Dr. Tilmann Bubeck wrote:
> This patch extends the ext2 driver to allow embedding core.img into the
> filesystem. This allows the long needed installation of GRUB into a partition
> without the need to use (unsafe) block lists.
> 
> It is realized using the ioctl(EXT4_IOC_SWAP_BOOT) introduced into
> Linux 3.10. This ioctl basically swaps the data blocks of the associated
> file with the EXT2_BOOT_LOADER_INO inode. After using the ioctl the code
> of core.img is stored in the BOOT_LOADER incode of the filesystem and it
> is not accessible to file system tools anymore. This ensures, that the boot
> code is safe and installation into a partition is possible.
> 
I've already commented on this design here: it not only doesn't
guarantee any kind of blockstability that we need but guarantees quite
the opposite in several scenarios (detailed in previous mails).
> The patchs needs 0001-Refactor-grub_read_mountinfo-out-of-grub_find_root_d.patch
> to work correctly.
> 
> Signed-off-by: Dr. Tilmann Bubeck <tilmann@bubecks.de>
> ---
>  grub-core/fs/ext2.c | 212 ++++++++++++++++++++++++++++++++++++++++++++++++++--
>  util/setup.c        |  11 ++-
>  2 files changed, 215 insertions(+), 8 deletions(-)
> 
> diff --git a/grub-core/fs/ext2.c b/grub-core/fs/ext2.c
> index 5f7a2b9..643969e 100644
> --- a/grub-core/fs/ext2.c
> +++ b/grub-core/fs/ext2.c
> @@ -133,6 +133,14 @@ GRUB_MOD_LICENSE ("GPLv3+");
>  
>  #define EXT4_EXTENTS_FLAG		0x80000
>  
> +/*
> + * Special inodes numbers
> + */
> +#define EXT2_ROOT_INO            2      /* Root inode */
> +#define EXT2_BOOT_LOADER_INO     5      /* Boot loader inode */
> +
> +#define EXT4_IOC_SWAP_BOOT		_IO('f', 17)
> +
>  /* The ext2 superblock.  */
>  struct grub_ext2_sblock
>  {
> @@ -393,10 +401,10 @@ grub_ext4_find_leaf (struct grub_ext2_data *data,
>  }
>  
>  static grub_disk_addr_t
> -grub_ext2_read_block (grub_fshelp_node_t node, grub_disk_addr_t fileblock)
> +grub_ext2_read_block2 (struct grub_ext2_data *data,
> +		       struct grub_ext2_inode *inode,
> +		       grub_disk_addr_t fileblock)
>  {
> -  struct grub_ext2_data *data = node->data;
> -  struct grub_ext2_inode *inode = &node->inode;
>    unsigned int blksz = EXT2_BLOCK_SIZE (data);
>    grub_disk_addr_t blksz_quarter = blksz / 4;
>    int log2_blksz = LOG2_EXT2_BLOCK_SIZE (data);
> @@ -497,6 +505,12 @@ indirect:
>    return grub_le_to_cpu32 (indir);
>  }
>  
> +static grub_disk_addr_t
> +grub_ext2_read_block (grub_fshelp_node_t node, grub_disk_addr_t fileblock)
> +{
> +  return grub_ext2_read_block2(node->data, &node->inode, fileblock);
> +}
> +
>  /* Read LEN bytes from the file described by DATA starting with byte
>     POS.  Return the amount of read bytes in READ.  */
>  static grub_ssize_t
> @@ -607,12 +621,12 @@ grub_ext2_mount (grub_disk_t disk)
>    data->disk = disk;
>  
>    data->diropen.data = data;
> -  data->diropen.ino = 2;
> +  data->diropen.ino = EXT2_ROOT_INO;
>    data->diropen.inode_read = 1;
>  
>    data->inode = &data->diropen.inode;
>  
> -  grub_ext2_read_inode (data, 2, data->inode);
> +  grub_ext2_read_inode (data, EXT2_ROOT_INO, data->inode);
>    if (grub_errno)
>      goto fail;
>  
> @@ -977,6 +991,193 @@ grub_ext2_mtime (grub_device_t device, grub_int32_t *tm)
>  
>  }
>  
> +#ifdef GRUB_UTIL
> +
> +#include <stdlib.h>
> +#include <sys/ioctl.h>
> +#include <include/grub/emu/misc.h>
> +#include <include/grub/emu/getroot.h>
> +#include <include/grub/emu/hostfile.h>
> +
> +/* Read the addresses of all sectors occupied by the file with the
> +   given inode from the filesystem.  Return the number of sectors in
> +   "nsector" and the addresses in "sectors".  "sectors" is allocated
> +   in this function and must be freed by the caller after usage.
> +   A sector in sectors has size GRUB_DISK_SECTOR_SIZE. */
> +static grub_err_t
> +grub_ext2_read_sectorlist(grub_device_t device,
> +			 int ino,
> +			 unsigned int *nsectors,
> +			 grub_disk_addr_t **sectors)
> +{
> +  struct grub_ext2_data *data;
> +  struct grub_ext2_inode inode;
> +  grub_off_t size;
> +  grub_err_t err;
> +  grub_disk_addr_t fileblock;
> +  int i;
> +  grub_disk_addr_t fileblock_count;
> +  int sectors_per_block;
> +
> +  data = grub_ext2_mount (device->disk);
> +  if (! data) {
> +    return grub_error (grub_errno, N_("Unable to mount device"));
> +  }
> +
> +  err = grub_ext2_read_inode (data, ino, &inode);
> +  if (err)
> +    return grub_error (err, N_("Unable to read inode #%d"), ino);
> +
> +  size = grub_le_to_cpu32 (inode.size);
> +  size |= ((grub_off_t) grub_le_to_cpu32 (inode.size_high)) << 32;
> +
> +  fileblock_count = size / EXT2_BLOCK_SIZE(data);
> +  if ( size % EXT2_BLOCK_SIZE(data) != 0 ) fileblock_count++;
> +
> +  sectors_per_block = EXT2_BLOCK_SIZE(data) / GRUB_DISK_SECTOR_SIZE;
> +  *sectors = grub_malloc (fileblock_count * sectors_per_block
> +			  * sizeof (**sectors));
> +  *nsectors = 0;
> +  for ( fileblock = 0; fileblock < fileblock_count; fileblock++ ) {
> +    (*sectors)[*nsectors] = grub_ext2_read_block2 (data, &inode, fileblock)
> +	                    * sectors_per_block;
> +    for ( i = 1; i < sectors_per_block; i++) {
> +      (*sectors)[(*nsectors) + i] = (*sectors)[*nsectors] + i;
> +    }
> +    (*nsectors) += sectors_per_block;
> +  }
> +
> +  return GRUB_ERR_NONE;
> +}
> +
> +static grub_err_t
> +grub_ext2_embed (grub_device_t device,
> +		  unsigned int *nsectors,
> +		  unsigned int max_nsectors,
> +		  grub_embed_type_t embed_type,
> +		  grub_disk_addr_t **sectors)
> +{
> +  unsigned i;
> +  grub_err_t err;
> +  char *mountpoint;
> +  struct mountinfo_entry *entries;
> +  grub_util_fd_t out;
> +  char *core_name;
> +  char diskbuffer[GRUB_DISK_SECTOR_SIZE];
> +  unsigned int nsectors_wanted;
> +  char *device_name;
> +  int ioctl_err;
> +
> +  if (embed_type != GRUB_EMBED_PCBIOS)
> +    return grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET,
> +		       N_("ext2 currently supports only PC-BIOS embedding"));
> +
> +  if ( device->disk->partition)
> +      device_name = grub_xasprintf ("%s,%s", device->disk->name,
> +			   grub_partition_get_name(device->disk->partition));
> +  else
> +      device_name = grub_xasprintf ("%s", device->disk->name);
> +
> +  nsectors_wanted = *nsectors;
> +
> +  grub_util_info (N_("Embedding %d/%d sectors into ext2 filesystem of %s"),
> +		 nsectors_wanted, max_nsectors, device_name);
> +
> +  /* [1] Check, if the existing boot loader inode exists and has the
> +         wanted size: */
> +  err = grub_ext2_read_sectorlist (device, EXT2_BOOT_LOADER_INO,
> +				   nsectors, sectors);
> +  if (!err && *nsectors >= nsectors_wanted && *nsectors <= max_nsectors) {
> +      grub_util_info(N_("Reusing existing boot loader inode"
> +			" offering %d sectors"),
> +		   *nsectors);
> +    grub_free (device_name);
> +    return GRUB_ERR_NONE;       /* YES! Everything is fine. */
> +  }
> +
> +  /* [2] We have to create a new boot loader inode. */
> +
> +  /* [2.1] Check if device is mounted and get mountpoint: */
> +  mountpoint = NULL;
> +  entries = grub_read_mountinfo ();
> +  if ( entries ) {
> +    for ( i = 0; entries[i].enc_root[0] != 0; i++) {
> +      char *grub_dev_of_mount;
> +      grub_errno = GRUB_ERR_NONE;    /* Clear errno set previously */
> +      grub_dev_of_mount = grub_util_get_grub_dev (entries[i].device);
> +      if ( grub_dev_of_mount ) {
> +	if ( grub_strcmp (grub_dev_of_mount, device_name) == 0 ) {
> +	  mountpoint = grub_strdup (entries[i].enc_path);
> +	  break;
> +	}
> +      }
> +    }
> +    free (entries);
> +  }
> +
> +  grub_errno = GRUB_ERR_NONE;    /* Clear errno set previously */
> +
> +  if (!mountpoint) {
> +    /* We were unable to find the mountpoint for the device of the
> +       filesystem. Maybe it is not mounted? */
> +    return grub_error (GRUB_ERR_UNKNOWN_DEVICE,
> +		       N_("We are unable to find the mountpoint of %s"),
> +		       device_name);
> +
> +    /** ToDo: Try to mount the filesystem to a temporary location. */
> +  }
> +
> +  grub_free (device_name);
> +
> +  /* [2.2] Create a new (temporary) file, which then gets the boot loader: */
> +  core_name = grub_util_path_concat (2, mountpoint, ".core.img");
> +  free (mountpoint);
> +  out = grub_util_fd_open (core_name, GRUB_UTIL_FD_O_WRONLY
> +			   | GRUB_UTIL_FD_O_CREATTRUNC);
> +  if (!GRUB_UTIL_FD_IS_VALID (out)) {
> +    return grub_error (GRUB_ERR_BAD_FS,
> +		       N_("cannot create `%s': %s"), core_name,
> +		       grub_util_fd_strerror ());
> +  }
> +  for ( i = 0; i < max_nsectors; i++ ) {
> +      grub_util_fd_write (out, diskbuffer, GRUB_DISK_SECTOR_SIZE);
> +  }
> +  grub_util_fd_sync (out);
> +
> +  /* [2.3] Make that file to the new boot loader inode by swapping the
> +           file of "out" with the boot loader inode: */
> +  ioctl_err = ioctl (out, EXT4_IOC_SWAP_BOOT);
> +  if ( ioctl_err ) {
> +    err = grub_error (GRUB_ERR_BAD_FS,
> +		      N_("Error in ioctl(EXT4_IOC_SWAP_BOOT);"
> +			 "you need Linux >= 3.10: %s"),
> +		      grub_util_fd_strerror ());
> +    grub_util_fd_close (out);
> +    grub_util_unlink (core_name);
> +    return err;
> +  }
> +  grub_util_fd_close (out);
> +
> +  /* [2.4] Unlink the core file, now containing the previous boot loader. */
> +  grub_util_unlink (core_name);
> +
> +  /* [2.5] Invalidate disk cache and read block list again: */
> +  grub_disk_cache_invalidate_all ();
> +
> +  err = grub_ext2_read_sectorlist (device, EXT2_BOOT_LOADER_INO,
> +				   nsectors, sectors);
> +  if ( err ) {
> +    return grub_error (GRUB_ERR_BAD_FS,
> +		       N_("unable to read boot loader inode"));
> +  }
> +
> +  grub_util_info (N_("Created new boot loader inode offering %d sectors"),
> +		     *nsectors);
> +
> +  return GRUB_ERR_NONE;
> +}
> +#endif
> +
>  
>  \f
>  static struct grub_fs grub_ext2_fs =
> @@ -990,6 +1191,7 @@ static struct grub_fs grub_ext2_fs =
>      .uuid = grub_ext2_uuid,
>      .mtime = grub_ext2_mtime,
>  #ifdef GRUB_UTIL
> +    .embed = grub_ext2_embed,
>      .reserved_first_sector = 1,
>      .blocklist_install = 1,
>  #endif
> diff --git a/util/setup.c b/util/setup.c
> index 9fb91a8..3f4a007 100644
> --- a/util/setup.c
> +++ b/util/setup.c
> @@ -509,8 +509,10 @@ SETUP (const char *dir,
>      if (!err && nsec < core_sectors)
>        {
>  	err = grub_error (GRUB_ERR_OUT_OF_RANGE,
> -			  N_("Your embedding area is unusually small.  "
> -			     "core.img won't fit in it."));
> +			  N_("Your embedding area is unusually small "
> +			     "(only %d sectors). "
> +			     "core.img won't fit in it (needs %d sectors)."),
> +			  nsec, core_sectors);
>        }
>      
>      if (err)
> @@ -583,10 +585,13 @@ SETUP (const char *dir,
>        }
>  
>      /* Write the core image onto the disk.  */
> -    for (i = 0; i < nsec; i++)
> +    for (i = 0; i < nsec; i++) {
> +	grub_util_info ("writing core.img/%d to sector %" PRIuGRUB_UINT64_T, i,
> +			sectors[i]);
>        grub_disk_write (dest_dev->disk, sectors[i], 0,
>  		       GRUB_DISK_SECTOR_SIZE,
>  		       core_img + i * GRUB_DISK_SECTOR_SIZE);
> +    }
>  
>      grub_free (sectors);
>  
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 274 bytes --]

Re: [PATCH] Improve ext2 driver to allow embedding of the boot loader code.

[Dr. Tilmann Bubeck]

Vladimir,

in your last email "Re: Why special linux code in 
grub2/util/grub-setup.c" from 25.07.2013 23:17 you request the following 
3 features which I respected in my patch:

 > 1) have a way to reserve some space, near the beginning. E.g. IOCTL
 > CREATE_EMBEDDING_ZONE with argument size=5M (number coming from my
 > draft on LVM zones). This has to have synchronous semantics.
 > Blocklist is fixed after this operation.

See ext2:1210 and following.

You are able to create an arbitrary large space (currently setup.c asks 
for max_nsectors=100 but this can be changed by setup.c without any 
change in ext2.c). You can also request 5M if you wish.

It is implemented to be synchronous.

The blocklist is fixed and stable and will never change.

The only open issue is "near the beginning". You probably request this 
because of some BIOS only able to access "near the beginning"? This 
limitation is really "near the beginning OF THE DISK" and not "near the 
beginning of the PARTITION". So I think, that any implementation inside 
a partition can potentially fail, if the partition itself is not near 
the beginning of the disk". This is something which could only be solved 
by either putting a partition near the beginning or by using the MBR gap.

My patch allows to use a partition at the beginning (like the btrfs 
driver did too).

And MBR is not always possible or wanted.

 > 2) A way to retrieve its blocklist

Done, see ext2.c function grub_ext2_read_sectorlist

 > 3) A way to overwrite parts of its contents in a way that ensures
 > bypassing journal, COW, compression, encryption, hyperspace or any
 > other advanced features.

Done. Existing code in setup.c takes the sectors given by the filesystem 
driver (ext2 or btrfs) and writes to that sectors. I have not changed 
anything here.

So please take a second look at the arguments and the implementation. I 
do think, that embedding into ext2 is a really important feature and I 
think this is a robust solution.

Thanks!
  Tilmann





Am 09.01.2014 23:52, schrieb Vladimir 'φ-coder/phcoder' Serbinenko:
> On 09.01.2014 22:07, Dr. Tilmann Bubeck wrote:
>> This patch extends the ext2 driver to allow embedding core.img into the
>> filesystem. This allows the long needed installation of GRUB into a partition
>> without the need to use (unsafe) block lists.
>>
>> It is realized using the ioctl(EXT4_IOC_SWAP_BOOT) introduced into
>> Linux 3.10. This ioctl basically swaps the data blocks of the associated
>> file with the EXT2_BOOT_LOADER_INO inode. After using the ioctl the code
>> of core.img is stored in the BOOT_LOADER incode of the filesystem and it
>> is not accessible to file system tools anymore. This ensures, that the boot
>> code is safe and installation into a partition is possible.
>>
> I've already commented on this design here: it not only doesn't
> guarantee any kind of blockstability that we need but guarantees quite
> the opposite in several scenarios (detailed in previous mails).
>> The patchs needs 0001-Refactor-grub_read_mountinfo-out-of-grub_find_root_d.patch
>> to work correctly.
>>>> Signed-off-by: Dr. Tilmann Bubeck <tilmann@bubecks.de>
>> ---
>>   grub-core/fs/ext2.c | 212 ++++++++++++++++++++++++++++++++++++++++++++++++++--
>>   util/setup.c        |  11 ++-
>>   2 files changed, 215 insertions(+), 8 deletions(-)
>>
>> diff --git a/grub-core/fs/ext2.c b/grub-core/fs/ext2.c
>> index 5f7a2b9..643969e 100644
>> --- a/grub-core/fs/ext2.c
>> +++ b/grub-core/fs/ext2.c
>> @@ -133,6 +133,14 @@ GRUB_MOD_LICENSE ("GPLv3+");
>>
>>   #define EXT4_EXTENTS_FLAG		0x80000
>>
>> +/*
>> + * Special inodes numbers
>> + */
>> +#define EXT2_ROOT_INO            2      /* Root inode */
>> +#define EXT2_BOOT_LOADER_INO     5      /* Boot loader inode */
>> +
>> +#define EXT4_IOC_SWAP_BOOT		_IO('f', 17)
>> +
>>   /* The ext2 superblock.  */
>>   struct grub_ext2_sblock
>>   {
>> @@ -393,10 +401,10 @@ grub_ext4_find_leaf (struct grub_ext2_data *data,
>>   }
>>
>>   static grub_disk_addr_t
>> -grub_ext2_read_block (grub_fshelp_node_t node, grub_disk_addr_t fileblock)
>> +grub_ext2_read_block2 (struct grub_ext2_data *data,
>> +		       struct grub_ext2_inode *inode,
>> +		       grub_disk_addr_t fileblock)
>>   {
>> -  struct grub_ext2_data *data = node->data;
>> -  struct grub_ext2_inode *inode = &node->inode;
>>     unsigned int blksz = EXT2_BLOCK_SIZE (data);
>>     grub_disk_addr_t blksz_quarter = blksz / 4;
>>     int log2_blksz = LOG2_EXT2_BLOCK_SIZE (data);
>> @@ -497,6 +505,12 @@ indirect:
>>     return grub_le_to_cpu32 (indir);
>>   }
>>
>> +static grub_disk_addr_t
>> +grub_ext2_read_block (grub_fshelp_node_t node, grub_disk_addr_t fileblock)
>> +{
>> +  return grub_ext2_read_block2(node->data, &node->inode, fileblock);
>> +}
>> +
>>   /* Read LEN bytes from the file described by DATA starting with byte
>>      POS.  Return the amount of read bytes in READ.  */
>>   static grub_ssize_t
>> @@ -607,12 +621,12 @@ grub_ext2_mount (grub_disk_t disk)
>>     data->disk = disk;
>>
>>     data->diropen.data = data;
>> -  data->diropen.ino = 2;
>> +  data->diropen.ino = EXT2_ROOT_INO;
>>     data->diropen.inode_read = 1;
>>
>>     data->inode = &data->diropen.inode;
>>
>> -  grub_ext2_read_inode (data, 2, data->inode);
>> +  grub_ext2_read_inode (data, EXT2_ROOT_INO, data->inode);
>>     if (grub_errno)
>>       goto fail;
>>
>> @@ -977,6 +991,193 @@ grub_ext2_mtime (grub_device_t device, grub_int32_t *tm)
>>
>>   }
>>
>> +#ifdef GRUB_UTIL
>> +
>> +#include <stdlib.h>
>> +#include <sys/ioctl.h>
>> +#include <include/grub/emu/misc.h>
>> +#include <include/grub/emu/getroot.h>
>> +#include <include/grub/emu/hostfile.h>
>> +
>> +/* Read the addresses of all sectors occupied by the file with the
>> +   given inode from the filesystem.  Return the number of sectors in
>> +   "nsector" and the addresses in "sectors".  "sectors" is allocated
>> +   in this function and must be freed by the caller after usage.
>> +   A sector in sectors has size GRUB_DISK_SECTOR_SIZE. */
>> +static grub_err_t
>> +grub_ext2_read_sectorlist(grub_device_t device,
>> +			 int ino,
>> +			 unsigned int *nsectors,
>> +			 grub_disk_addr_t **sectors)
>> +{
>> +  struct grub_ext2_data *data;
>> +  struct grub_ext2_inode inode;
>> +  grub_off_t size;
>> +  grub_err_t err;
>> +  grub_disk_addr_t fileblock;
>> +  int i;
>> +  grub_disk_addr_t fileblock_count;
>> +  int sectors_per_block;
>> +
>> +  data = grub_ext2_mount (device->disk);
>> +  if (! data) {
>> +    return grub_error (grub_errno, N_("Unable to mount device"));
>> +  }
>> +
>> +  err = grub_ext2_read_inode (data, ino, &inode);
>> +  if (err)
>> +    return grub_error (err, N_("Unable to read inode #%d"), ino);
>> +
>> +  size = grub_le_to_cpu32 (inode.size);
>> +  size |= ((grub_off_t) grub_le_to_cpu32 (inode.size_high)) << 32;
>> +
>> +  fileblock_count = size / EXT2_BLOCK_SIZE(data);
>> +  if ( size % EXT2_BLOCK_SIZE(data) != 0 ) fileblock_count++;
>> +
>> +  sectors_per_block = EXT2_BLOCK_SIZE(data) / GRUB_DISK_SECTOR_SIZE;
>> +  *sectors = grub_malloc (fileblock_count * sectors_per_block
>> +			  * sizeof (**sectors));
>> +  *nsectors = 0;
>> +  for ( fileblock = 0; fileblock < fileblock_count; fileblock++ ) {
>> +    (*sectors)[*nsectors] = grub_ext2_read_block2 (data, &inode, fileblock)
>> +	                    * sectors_per_block;
>> +    for ( i = 1; i < sectors_per_block; i++) {
>> +      (*sectors)[(*nsectors) + i] = (*sectors)[*nsectors] + i;
>> +    }
>> +    (*nsectors) += sectors_per_block;
>> +  }
>> +
>> +  return GRUB_ERR_NONE;
>> +}
>> +
>> +static grub_err_t
>> +grub_ext2_embed (grub_device_t device,
>> +		  unsigned int *nsectors,
>> +		  unsigned int max_nsectors,
>> +		  grub_embed_type_t embed_type,
>> +		  grub_disk_addr_t **sectors)
>> +{
>> +  unsigned i;
>> +  grub_err_t err;
>> +  char *mountpoint;
>> +  struct mountinfo_entry *entries;
>> +  grub_util_fd_t out;
>> +  char *core_name;
>> +  char diskbuffer[GRUB_DISK_SECTOR_SIZE];
>> +  unsigned int nsectors_wanted;
>> +  char *device_name;
>> +  int ioctl_err;
>> +
>> +  if (embed_type != GRUB_EMBED_PCBIOS)
>> +    return grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET,
>> +		       N_("ext2 currently supports only PC-BIOS embedding"));
>> +
>> +  if ( device->disk->partition)
>> +      device_name = grub_xasprintf ("%s,%s", device->disk->name,
>> +			   grub_partition_get_name(device->disk->partition));
>> +  else
>> +      device_name = grub_xasprintf ("%s", device->disk->name);
>> +
>> +  nsectors_wanted = *nsectors;
>> +
>> +  grub_util_info (N_("Embedding %d/%d sectors into ext2 filesystem of %s"),
>> +		 nsectors_wanted, max_nsectors, device_name);
>> +
>> +  /* [1] Check, if the existing boot loader inode exists and has the
>> +         wanted size: */
>> +  err = grub_ext2_read_sectorlist (device, EXT2_BOOT_LOADER_INO,
>> +				   nsectors, sectors);
>> +  if (!err && *nsectors >= nsectors_wanted && *nsectors <= max_nsectors) {
>> +      grub_util_info(N_("Reusing existing boot loader inode"
>> +			" offering %d sectors"),
>> +		   *nsectors);
>> +    grub_free (device_name);
>> +    return GRUB_ERR_NONE;       /* YES! Everything is fine. */
>> +  }
>> +
>> +  /* [2] We have to create a new boot loader inode. */
>> +
>> +  /* [2.1] Check if device is mounted and get mountpoint: */
>> +  mountpoint = NULL;
>> +  entries = grub_read_mountinfo ();
>> +  if ( entries ) {
>> +    for ( i = 0; entries[i].enc_root[0] != 0; i++) {
>> +      char *grub_dev_of_mount;
>> +      grub_errno = GRUB_ERR_NONE;    /* Clear errno set previously */
>> +      grub_dev_of_mount = grub_util_get_grub_dev (entries[i].device);
>> +      if ( grub_dev_of_mount ) {
>> +	if ( grub_strcmp (grub_dev_of_mount, device_name) == 0 ) {
>> +	  mountpoint = grub_strdup (entries[i].enc_path);
>> +	  break;
>> +	}
>> +      }
>> +    }
>> +    free (entries);
>> +  }
>> +
>> +  grub_errno = GRUB_ERR_NONE;    /* Clear errno set previously */
>> +
>> +  if (!mountpoint) {
>> +    /* We were unable to find the mountpoint for the device of the
>> +       filesystem. Maybe it is not mounted? */
>> +    return grub_error (GRUB_ERR_UNKNOWN_DEVICE,
>> +		       N_("We are unable to find the mountpoint of %s"),
>> +		       device_name);
>> +
>> +    /** ToDo: Try to mount the filesystem to a temporary location. */
>> +  }
>> +
>> +  grub_free (device_name);
>> +
>> +  /* [2.2] Create a new (temporary) file, which then gets the boot loader: */
>> +  core_name = grub_util_path_concat (2, mountpoint, ".core.img");
>> +  free (mountpoint);
>> +  out = grub_util_fd_open (core_name, GRUB_UTIL_FD_O_WRONLY
>> +			   | GRUB_UTIL_FD_O_CREATTRUNC);
>> +  if (!GRUB_UTIL_FD_IS_VALID (out)) {
>> +    return grub_error (GRUB_ERR_BAD_FS,
>> +		       N_("cannot create `%s': %s"), core_name,
>> +		       grub_util_fd_strerror ());
>> +  }
>> +  for ( i = 0; i < max_nsectors; i++ ) {
>> +      grub_util_fd_write (out, diskbuffer, GRUB_DISK_SECTOR_SIZE);
>> +  }
>> +  grub_util_fd_sync (out);
>> +
>> +  /* [2.3] Make that file to the new boot loader inode by swapping the
>> +           file of "out" with the boot loader inode: */
>> +  ioctl_err = ioctl (out, EXT4_IOC_SWAP_BOOT);
>> +  if ( ioctl_err ) {
>> +    err = grub_error (GRUB_ERR_BAD_FS,
>> +		      N_("Error in ioctl(EXT4_IOC_SWAP_BOOT);"
>> +			 "you need Linux >= 3.10: %s"),
>> +		      grub_util_fd_strerror ());
>> +    grub_util_fd_close (out);
>> +    grub_util_unlink (core_name);
>> +    return err;
>> +  }
>> +  grub_util_fd_close (out);
>> +
>> +  /* [2.4] Unlink the core file, now containing the previous boot loader. */
>> +  grub_util_unlink (core_name);
>> +
>> +  /* [2.5] Invalidate disk cache and read block list again: */
>> +  grub_disk_cache_invalidate_all ();
>> +
>> +  err = grub_ext2_read_sectorlist (device, EXT2_BOOT_LOADER_INO,
>> +				   nsectors, sectors);
>> +  if ( err ) {
>> +    return grub_error (GRUB_ERR_BAD_FS,
>> +		       N_("unable to read boot loader inode"));
>> +  }
>> +
>> +  grub_util_info (N_("Created new boot loader inode offering %d sectors"),
>> +		     *nsectors);
>> +
>> +  return GRUB_ERR_NONE;
>> +}
>> +#endif
>> +
>>
>>   \f
>>   static struct grub_fs grub_ext2_fs =
>> @@ -990,6 +1191,7 @@ static struct grub_fs grub_ext2_fs =
>>       .uuid = grub_ext2_uuid,
>>       .mtime = grub_ext2_mtime,
>>   #ifdef GRUB_UTIL
>> +    .embed = grub_ext2_embed,
>>       .reserved_first_sector = 1,
>>       .blocklist_install = 1,
>>   #endif
>> diff --git a/util/setup.c b/util/setup.c
>> index 9fb91a8..3f4a007 100644
>> --- a/util/setup.c
>> +++ b/util/setup.c
>> @@ -509,8 +509,10 @@ SETUP (const char *dir,
>>       if (!err && nsec < core_sectors)
>>         {
>>   	err = grub_error (GRUB_ERR_OUT_OF_RANGE,
>> -			  N_("Your embedding area is unusually small.  "
>> -			     "core.img won't fit in it."));
>> +			  N_("Your embedding area is unusually small "
>> +			     "(only %d sectors). "
>> +			     "core.img won't fit in it (needs %d sectors)."),
>> +			  nsec, core_sectors);
>>         }
>>
>>       if (err)
>> @@ -583,10 +585,13 @@ SETUP (const char *dir,
>>         }
>>
>>       /* Write the core image onto the disk.  */
>> -    for (i = 0; i < nsec; i++)
>> +    for (i = 0; i < nsec; i++) {
>> +	grub_util_info ("writing core.img/%d to sector %" PRIuGRUB_UINT64_T, i,
>> +			sectors[i]);
>>         grub_disk_write (dest_dev->disk, sectors[i], 0,
>>   		       GRUB_DISK_SECTOR_SIZE,
>>   		       core_img + i * GRUB_DISK_SECTOR_SIZE);
>> +    }
>>
>>       grub_free (sectors);
>>
>>
>
>
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>

-- 
Mit freundlichen Gruessen,
   Tilmann Bubeck

//// dr. tilmann bubeck, it professional & geek
////
//// tilmann@bubecks.de    /  http://www.bubecks.de
//// mobile: 0172-8842972  /  fon: 0711-7227719     /  fax: 0711-7227734
//// widmaierstr. 58       /  70567 stuttgart       /  germany

Re: [PATCH] Improve ext2 driver to allow embedding of the boot loader code.

[Vladimir 'φ-coder/phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 276 bytes --]

On 10.01.2014 08:49, Dr. Tilmann Bubeck wrote:
> 
> The blocklist is fixed and stable and will never change.
What guarantees that it won't change on grub-setup invocation? I'm under
impression that it will change on every grub-setup invocation as file
gets recreated.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 274 bytes --]

Re: [PATCH] Improve ext2 driver to allow embedding of the boot loader code.

[Andrey Borzenkov]

On Tue, Jan 21, 2014 at 12:14 PM, Vladimir 'φ-coder/phcoder'
Serbinenko <phcoder@gmail.com> wrote:
> On 10.01.2014 08:49, Dr. Tilmann Bubeck wrote:
>>
>> The blocklist is fixed and stable and will never change.
> What guarantees that it won't change on grub-setup invocation? I'm under
> impression that it will change on every grub-setup invocation as file
> gets recreated.
>

If I read code correctly, it checks current size and if new core.img
fits, space is reused. So we could effectively make it preallocate
reasonable size (or even unreasonable - I guess 10MB will be enough
for foreseeable future) the very first time it is done.

Re: [PATCH] Improve ext2 driver to allow embedding of the boot loader code.

[Vladimir 'φ-coder/phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 1031 bytes --]

On 21.01.2014 09:28, Andrey Borzenkov wrote:
> On Tue, Jan 21, 2014 at 12:14 PM, Vladimir 'φ-coder/phcoder'
> Serbinenko <phcoder@gmail.com> wrote:
>> On 10.01.2014 08:49, Dr. Tilmann Bubeck wrote:
>>>
>>> The blocklist is fixed and stable and will never change.
>> What guarantees that it won't change on grub-setup invocation? I'm under
>> impression that it will change on every grub-setup invocation as file
>> gets recreated.
>>
> 
> If I read code correctly, it checks current size and if new core.img
> fits, space is reused. So we could effectively make it preallocate
> reasonable size (or even unreasonable - I guess 10MB will be enough
> for foreseeable future) the very first time it is done.
> 
It still doesn't solve the problem that during operations file becomes a
normal file and OS is allowed to rearrange the blocks as it sees fit.
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 274 bytes --]

Re: [PATCH] Improve ext2 driver to allow embedding of the boot loader code.

[Andrey Borzenkov]

On Tue, Jan 21, 2014 at 12:32 PM, Vladimir 'φ-coder/phcoder'
Serbinenko <phcoder@gmail.com> wrote:
> On 21.01.2014 09:28, Andrey Borzenkov wrote:
>> On Tue, Jan 21, 2014 at 12:14 PM, Vladimir 'φ-coder/phcoder'
>> Serbinenko <phcoder@gmail.com> wrote:
>>> On 10.01.2014 08:49, Dr. Tilmann Bubeck wrote:
>>>>
>>>> The blocklist is fixed and stable and will never change.
>>> What guarantees that it won't change on grub-setup invocation? I'm under
>>> impression that it will change on every grub-setup invocation as file
>>> gets recreated.
>>>
>>
>> If I read code correctly, it checks current size and if new core.img
>> fits, space is reused. So we could effectively make it preallocate
>> reasonable size (or even unreasonable - I guess 10MB will be enough
>> for foreseeable future) the very first time it is done.
>>
> It still doesn't solve the problem that during operations file becomes a
> normal file and OS is allowed to rearrange the blocks as it sees fit.

Would this be acceptable - use external utility to allocate
EXT2_BOOT_LOADER_INO space of sufficient size once (outside of grub at
all) and allow embedding into extX if this space exists? Do not mess
with with it in grub-setup itself?

We could then speak with ext2 folks to add option to mke2fs/une2fs in
the long run it it does not exist yet.

Re: [PATCH] Improve ext2 driver to allow embedding of the boot loader code.

[Vladimir 'φ-coder/phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 1788 bytes --]

On 21.01.2014 09:41, Andrey Borzenkov wrote:
> On Tue, Jan 21, 2014 at 12:32 PM, Vladimir 'φ-coder/phcoder'
> Serbinenko <phcoder@gmail.com> wrote:
>> On 21.01.2014 09:28, Andrey Borzenkov wrote:
>>> On Tue, Jan 21, 2014 at 12:14 PM, Vladimir 'φ-coder/phcoder'
>>> Serbinenko <phcoder@gmail.com> wrote:
>>>> On 10.01.2014 08:49, Dr. Tilmann Bubeck wrote:
>>>>>
>>>>> The blocklist is fixed and stable and will never change.
>>>> What guarantees that it won't change on grub-setup invocation? I'm under
>>>> impression that it will change on every grub-setup invocation as file
>>>> gets recreated.
>>>>
>>>
>>> If I read code correctly, it checks current size and if new core.img
>>> fits, space is reused. So we could effectively make it preallocate
>>> reasonable size (or even unreasonable - I guess 10MB will be enough
>>> for foreseeable future) the very first time it is done.
>>>
>> It still doesn't solve the problem that during operations file becomes a
>> normal file and OS is allowed to rearrange the blocks as it sees fit.
> 
> Would this be acceptable - use external utility to allocate
> EXT2_BOOT_LOADER_INO space of sufficient size once (outside of grub at
> all) and allow embedding into extX if this space exists? Do not mess
> with with it in grub-setup itself?
> 
This presents a problem with sync'ing. After this space was reserved it
won't appear when using block functions until next sync'ing. This would
result in install failure on a new filesystem.
> We could then speak with ext2 folks to add option to mke2fs/une2fs in
> the long run it it does not exist yet.
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 274 bytes --]

Re: [PATCH] Improve ext2 driver to allow embedding of the boot loader code.

[Dr. Tilmann Bubeck]

The allocated space is reused every time on grub-setup execution. It 
does not change, as long as it is big enough. As Andrew said, by 
allocating 10 MB in the first run, it will be big enough for the 
foreseeable future (currently core.img on my system is 30k).

After issuing the ioctl(EXT4_IOC_SWAP_BOOT) it is not a file anymore. It 
is only a (reserved) inode with data blocks. There is no filename 
associated in any directory. So user tools have no way to manipulate the 
data.

fsck is aware of this and will not change anything or report any error.

I do not see, why we need another user space tool for the initial 
allocation. The initial reservation and subsequent use of the data 
blocks can be done by grub-setup very easy and is already part of the patch.


Tilmann


On 01/21/2014 12:03 PM, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
> On 21.01.2014 09:41, Andrey Borzenkov wrote:
>> On Tue, Jan 21, 2014 at 12:32 PM, Vladimir 'φ-coder/phcoder'
>> Serbinenko <phcoder@gmail.com> wrote:
>>> On 21.01.2014 09:28, Andrey Borzenkov wrote:
>>>> On Tue, Jan 21, 2014 at 12:14 PM, Vladimir 'φ-coder/phcoder'
>>>> Serbinenko <phcoder@gmail.com> wrote:
>>>>> On 10.01.2014 08:49, Dr. Tilmann Bubeck wrote:
>>>>>>
>>>>>> The blocklist is fixed and stable and will never change.
>>>>> What guarantees that it won't change on grub-setup invocation? I'm under
>>>>> impression that it will change on every grub-setup invocation as file
>>>>> gets recreated.
>>>>>
>>>>
>>>> If I read code correctly, it checks current size and if new core.img
>>>> fits, space is reused. So we could effectively make it preallocate
>>>> reasonable size (or even unreasonable - I guess 10MB will be enough
>>>> for foreseeable future) the very first time it is done.
>>>>
>>> It still doesn't solve the problem that during operations file becomes a
>>> normal file and OS is allowed to rearrange the blocks as it sees fit.
>>
>> Would this be acceptable - use external utility to allocate
>> EXT2_BOOT_LOADER_INO space of sufficient size once (outside of grub at
>> all) and allow embedding into extX if this space exists? Do not mess
>> with with it in grub-setup itself?
>>
> This presents a problem with sync'ing. After this space was reserved it
> won't appear when using block functions until next sync'ing. This would
> result in install failure on a new filesystem.
>> We could then speak with ext2 folks to add option to mke2fs/une2fs in
>> the long run it it does not exist yet.
>>
>> _______________________________________________
>> Grub-devel mailing list
>> Grub-devel@gnu.org
>> https://lists.gnu.org/mailman/listinfo/grub-devel
>>
>
>
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>

Re: [PATCH] Improve ext2 driver to allow embedding of the boot loader code.

[Dr. Tilmann Bubeck]

Vladimir,
Andrey,

is there any chance, that you integrate this patch? As you probably 
know,there is a huge demand for embedding grub2 into ext4 filesystems.

Is it ready for integration or should anything be changed?

Thanks!
  Tilmann

Am 21.01.2014 20:55, schrieb Dr. Tilmann Bubeck:
>
> The allocated space is reused every time on grub-setup execution. It
> does not change, as long as it is big enough. As Andrew said, by
> allocating 10 MB in the first run, it will be big enough for the
> foreseeable future (currently core.img on my system is 30k).
>
> After issuing the ioctl(EXT4_IOC_SWAP_BOOT) it is not a file anymore. It
> is only a (reserved) inode with data blocks. There is no filename
> associated in any directory. So user tools have no way to manipulate the
> data.
>
> fsck is aware of this and will not change anything or report any error.
>
> I do not see, why we need another user space tool for the initial
> allocation. The initial reservation and subsequent use of the data
> blocks can be done by grub-setup very easy and is already part of the
> patch.
>
>
> Tilmann
>
>
> On 01/21/2014 12:03 PM, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
>> On 21.01.2014 09:41, Andrey Borzenkov wrote:
>>> On Tue, Jan 21, 2014 at 12:32 PM, Vladimir 'φ-coder/phcoder'
>>> Serbinenko <phcoder@gmail.com> wrote:
>>>> On 21.01.2014 09:28, Andrey Borzenkov wrote:
>>>>> On Tue, Jan 21, 2014 at 12:14 PM, Vladimir 'φ-coder/phcoder'
>>>>> Serbinenko <phcoder@gmail.com> wrote:
>>>>>> On 10.01.2014 08:49, Dr. Tilmann Bubeck wrote:
>>>>>>>
>>>>>>> The blocklist is fixed and stable and will never change.
>>>>>> What guarantees that it won't change on grub-setup invocation? I'm
>>>>>> under
>>>>>> impression that it will change on every grub-setup invocation as file
>>>>>> gets recreated.
>>>>>>
>>>>>
>>>>> If I read code correctly, it checks current size and if new core.img
>>>>> fits, space is reused. So we could effectively make it preallocate
>>>>> reasonable size (or even unreasonable - I guess 10MB will be enough
>>>>> for foreseeable future) the very first time it is done.
>>>>>
>>>> It still doesn't solve the problem that during operations file
>>>> becomes a
>>>> normal file and OS is allowed to rearrange the blocks as it sees fit.
>>>
>>> Would this be acceptable - use external utility to allocate
>>> EXT2_BOOT_LOADER_INO space of sufficient size once (outside of grub at
>>> all) and allow embedding into extX if this space exists? Do not mess
>>> with with it in grub-setup itself?
>>>
>> This presents a problem with sync'ing. After this space was reserved it
>> won't appear when using block functions until next sync'ing. This would
>> result in install failure on a new filesystem.
>>> We could then speak with ext2 folks to add option to mke2fs/une2fs in
>>> the long run it it does not exist yet.
>>>
>>> _______________________________________________
>>> Grub-devel mailing list
>>> Grub-devel@gnu.org
>>> https://lists.gnu.org/mailman/listinfo/grub-devel
>>>
>>
>>
>>
>>
>> _______________________________________________
>> Grub-devel mailing list
>> Grub-devel@gnu.org
>> https://lists.gnu.org/mailman/listinfo/grub-devel
>>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

-- 
Mit freundlichen Gruessen,
   Tilmann Bubeck


//// dr. tilmann bubeck, it professional & geek
////
//// till@bubecks.de       /  http://www.bubecks.de
//// mobile: 0172-8842972  /  fon: 0711-7227719     /  fax: 0711-7227734
//// widmaierstr. 58       /  70567 stuttgart       /  germany