Skip to contentSkip to conversation

Sector 2 of non-embedded core.img: write early or patch later?

RFC PATCH In review Pavel Roskin 4 participants 10 messages Jun 27, 2008 – Jun 30, 2008

lore
Share & tools
lore

Pavel Roskin

author
#0 lore raw

RFC PATCH 1/1 Sector 2 of non-embedded core.img: write early or patch later?

Hello!

We have a serious problem with installing onto partitions (e.g. grub-install /dev/sda1) and drives with geometry that doesn't leave enough space for core.img to be embedded before the first partition. While it's not a desirable configuration, it should work, but it doesn't.

Either we should write sector 2 before the core.img is checked for readability or we should delay patching sector 2 in memory until the verification is done. Otherwise, the verification will fail.

Delayed patching changes nothing in terms of I/O but needs a longer patch and won't scale well if we want to put more information into sector 2, as we'll need to cache more data.

Writing sector 2 early is less intrusive in terms of code, but may be slower e.g. on floppies. It's probably more reliable because we verify that the modified sector 2 made it to the filesystem.

I tend to prefer the later ("write early") approach, but I'd like to see some feedback before I commit it.

Both proposed patches are attached.

Signature · 2 lines
Regards,
Pavel Roskin
util/i386/pc/grub-setup.c +13 -9
index 62c1bf1..3e9b4b5 100644
@@ -101,6 +101,7 @@ setup (const char *dir,
   grub_uint16_t *boot_drive_check;
   struct boot_blocklist *first_block, *block;
   grub_int32_t *install_dos_part, *install_bsd_part;
+  grub_int32_t dos_part, bsd_part;
   char *tmp_img;
   int i;
   grub_disk_addr_t first_sector;

@@ -283,27 +284,24 @@ setup (const char *dir,
 	    {
 	      struct grub_pc_partition *pcdata =
 		root_dev->disk->partition->data;
-	      *install_dos_part
-		= grub_cpu_to_le32 (pcdata->dos_part);
-	      *install_bsd_part
-		= grub_cpu_to_le32 (pcdata->bsd_part);
+	      dos_part = grub_cpu_to_le32 (pcdata->dos_part);
+	      bsd_part = grub_cpu_to_le32 (pcdata->bsd_part);
 	    }
 	  else if (strcmp (root_dev->disk->partition->partmap->name,
 			   "gpt_partition_map") == 0)
 	    {
-	      *install_dos_part = grub_cpu_to_le32 (root_dev->disk->partition->index);
-	      *install_bsd_part = grub_cpu_to_le32 (-1);
+	      dos_part = grub_cpu_to_le32 (root_dev->disk->partition->index);
+	      bsd_part = grub_cpu_to_le32 (-1);
 	    }
 	  else
 	    grub_util_error ("No PC style partitions found");
 	}
       else
-	*install_dos_part = *install_bsd_part = grub_cpu_to_le32 (-1);
+	dos_part = bsd_part = grub_cpu_to_le32 (-1);
     }
   
   grub_util_info ("dos partition is %d, bsd partition is %d",
-		  grub_le_to_cpu32 (*install_dos_part),
-		  grub_le_to_cpu32 (*install_bsd_part));
+		  grub_le_to_cpu32 (dos_part), grub_le_to_cpu32 (bsd_part));
   
   /* If the destination device can have partitions and it is the MBR,
      try to embed the core image into after the MBR.  */

@@ -316,6 +314,9 @@ setup (const char *dir,
 	{
 	  grub_util_info ("will embed the core image at sector 0x%llx", embed_region.start);
 
+	  *install_dos_part = dos_part;
+	  *install_bsd_part = bsd_part;
+
 	  /* The first blocklist contains the whole sectors.  */
 	  first_block->start = grub_cpu_to_le64 (embed_region.start + 1);
 	  first_block->len = grub_cpu_to_le16 (core_sectors - 1);

@@ -485,6 +486,9 @@ setup (const char *dir,
      the boot device.  */
   *root_drive = 0xFF;
 
+  *install_dos_part = dos_part;
+  *install_bsd_part = bsd_part;
+
   /* Write the first two sectors of the core image onto the disk.  */
   grub_util_info ("opening the core image `%s'", core_path);
   fp = fopen (core_path, "r+b");
util/i386/pc/grub-setup.c +11 -2
index 62c1bf1..d2c5a73 100644
@@ -365,6 +365,15 @@ setup (const char *dir,
   /* The core image must be put on a filesystem unfortunately.  */
   grub_util_info ("will leave the core image on the filesystem");
   
+  /* Write the second sector of the core image onto the disk.  */
+  grub_util_info ("opening the core image `%s'", core_path);
+  fp = fopen (core_path, "r+b");
+  if (! fp)
+    grub_util_error ("Cannot open `%s'", core_path);
+  grub_util_write_image_at (core_img + GRUB_DISK_SECTOR_SIZE,
+			    GRUB_DISK_SECTOR_SIZE, GRUB_DISK_SECTOR_SIZE, fp);
+  fclose (fp);
+
   /* Make sure that GRUB reads the identical image as the OS.  */
   tmp_img = xmalloc (core_size);
   core_path_dev = grub_util_get_path (DEFAULT_DIRECTORY, core_file);

@@ -485,13 +494,13 @@ setup (const char *dir,
      the boot device.  */
   *root_drive = 0xFF;
 
-  /* Write the first two sectors of the core image onto the disk.  */
+  /* Write the first sector of the core image onto the disk.  */
   grub_util_info ("opening the core image `%s'", core_path);
   fp = fopen (core_path, "r+b");
   if (! fp)
     grub_util_error ("Cannot open `%s'", core_path);
 
-  grub_util_write_image (core_img, GRUB_DISK_SECTOR_SIZE * 2, fp);
+  grub_util_write_image (core_img, GRUB_DISK_SECTOR_SIZE, fp);
   fclose (fp);
 
   /* Write the boot image onto the disk.  */
#1 lore raw
Quoted 16 lines · Pavel Roskin

Hello!

We have a serious problem with installing onto partitions (e.g. grub-install /dev/sda1) and drives with geometry that doesn't leave enough space for core.img to be embedded before the first partition. While it's not a desirable configuration, it should work, but it doesn't.

Either we should write sector 2 before the core.img is checked for readability or we should delay patching sector 2 in memory until the verification is done. Otherwise, the verification will fail.

Delayed patching changes nothing in terms of I/O but needs a longer patch and won't scale well if we want to put more information into sector 2, as we'll need to cache more data.

Quoted 8 lines

Writing sector 2 early is less intrusive in terms of code, but may be slower e.g. on floppies. It's probably more reliable because we verify that the modified sector 2 made it to the filesystem.

I tend to prefer the later ("write early") approach, but I'd like to see some feedback before I commit it.

Both proposed patches are attached.

Hi,

Here are some thoughts:

1, We can store the original content before making change.

2, Or when we modify data, we change both of the buffer, so that the comparison would not fail.

Signature · 1 line
Bean
#2 lore raw
In reply to Pavel Roskin
Quoted 2 lines · Pavel Roskin

I tend to prefer the later ("write early") approach, but I'd like to see some feedback before I commit it.

Hi,

The problem of write early is that it changes the target image before verification is made, so if verification fails, the change remains. This is generally not a good practice, we should not touch core.img if it can't be embedded successfully.

Signature · 1 line
Bean
author
#3 lore raw
Quoted 10 lines · Bean
Quoted 2 lines · Pavel Roskin

I tend to prefer the later ("write early") approach, but I'd like to see some feedback before I commit it.

Hi,

The problem of write early is that it changes the target image before verification is made, so if verification fails, the change remains. This is generally not a good practice, we should not touch core.img if it can't be embedded successfully.

OK, that's a good argument. Actually, if grub-install is used, core.img would have been rewritten already. Anyway, core.img is a file we want to touch as little as possible, considering that it can already be used.

I'm applying the "patch-later" patch with some simplifications and warning fixes.

Signature · 2 lines
Regards,
Pavel Roskin
#4 lore raw
In reply to Pavel Roskin
Quoted 8 lines · Pavel Roskin

Hello!

We have a serious problem with installing onto partitions (e.g. grub-install /dev/sda1) and drives with geometry that doesn't leave enough space for core.img to be embedded before the first partition. While it's not a desirable configuration, it should work, but it doesn't.

Quoted 7 lines

Either we should write sector 2 before the core.img is checked for readability or we should delay patching sector 2 in memory until the verification is done. Otherwise, the verification will fail.

Delayed patching changes nothing in terms of I/O but needs a longer patch and won't scale well if we want to put more information into sector 2, as we'll need to cache more data.

Why not make a structure that holds every modification information that needs to be done ("configuration block"). And then you provide core.img and variable filled with configuration information to patch function that modifies core.img properly. This way you only need to write this once.

With this you can either modify compare or patch two copies of the images with same configuration information.

#5 lore raw
In reply to Pavel Roskin
Quoted 7 lines · Pavel Roskin

Hello!

We have a serious problem with installing onto partitions (e.g. grub-install /dev/sda1) and drives with geometry that doesn't leave enough space for core.img to be embedded before the first partition. While it's not a desirable configuration, it should work, but it doesn't.

Do we really want to support this? I don't think there's any reliable way to tell where filesystem metadata starts. So unless we're talking about a dedicated partition, this sounds like a no-go.

Signature · 5 lines
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What good is a phone call… if you are unable to speak?
(as seen on /.)
#6 lore raw
Quoted 9 lines · Robert Millan
Quoted 7 lines · Pavel Roskin

Hello!

We have a serious problem with installing onto partitions (e.g. grub-install /dev/sda1) and drives with geometry that doesn't leave enough space for core.img to be embedded before the first partition. While it's not a desirable configuration, it should work, but it doesn't.

Quoted 3 lines

Do we really want to support this? I don't think there's any reliable way to tell where filesystem metadata starts. So unless we're talking about a dedicated partition, this sounds like a no-go.

This also relates to problem with installing to MBR and there is no room for embedding core.img there. I have been using USB stick where there is not enough room for core.img what I want to store there. So only option is to leave core.img to filesystem and install MBR.

#7 lore raw
Quoted 5 lines · Vesa Jääskeläinen

This also relates to problem with installing to MBR and there is no room for embedding core.img there. I have been using USB stick where there is not enough room for core.img what I want to store there. So only option is to leave core.img to filesystem and install MBR.

But then you can add an empty partition (type 0x00) starting at sector 63, and grub-setup will overwrite it.

Or you could leave that region unpartitioned.

Signature · 5 lines
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What good is a phone call… if you are unable to speak?
(as seen on /.)
#8 lore raw
Quoted 7 lines · Robert Millan
Quoted 1 line · Vesa Jääskeläinen

This also relates to problem with installing to MBR and there is no room for embedding core.img there. I have been using USB stick where there is not enough room for core.img what I want to store there. So only option is to leave core.img to filesystem and install MBR.

But then you can add an empty partition (type 0x00) starting at sector 63, and grub-setup will overwrite it.

Or you could leave that region unpartitioned.

At least on my disks there is 32 KiB free space. But not ~50 KiB what my core.img requires.

#9 lore raw
Quoted 7 lines · Vesa Jääskeläinen
Quoted 6 lines · Robert Millan
Quoted 4 lines · Vesa Jääskeläinen

This also relates to problem with installing to MBR and there is no room for embedding core.img there. I have been using USB stick where there is not enough room for core.img what I want to store there. So only option is to leave core.img to filesystem and install MBR.

Quoted 6 lines
Quoted 4 lines

But then you can add an empty partition (type 0x00) starting at sector 63, and grub-setup will overwrite it.

Or you could leave that region unpartitioned.

At least on my disks there is 32 KiB free space.

Typically they're 32 kiB - 512 B (because most tools reserve that space for some legacy restriction that no longer exists ;-)).

Quoted 2 lines

But not ~50 KiB what my
core.img requires.

What do you put in your core?

Signature · 5 lines
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What good is a phone call… if you are unable to speak?
(as seen on /.)