Failure to embed core.img is fatal now

Failure to embed core.img is fatal now

[Pavel Roskin]

Hello!

I have a system with a hard drive with a geometry that doesn't leave
space for GRUB to be embedded:

Disk /dev/sda: 30.0 GB, 30020272128 bytes
64 heads, 32 sectors/track, 28629 cylinders
Units = cylinders of 2048 * 512 = 1048576 bytes
Disk identifier: 0xfafa98d2

   Device Boot      Start         End      Blocks   Id  System
/dev/sda1               1       26704    27344880   83  Linux
/dev/sda2           26705       28629     1971200   82  Linux swap / Solaris

I'm trying to install the current GRUB2:

# grub-install /dev/sda
grub-setup: error: Cannot read `/boot/grub/core.img' correctly

It turns out following happens in grub-setup:

- core.img is read by the OS facilities
- the memory image is modified
- the memory image is compared to core.img read by GRUB FS code and fails
- the memory image is modified again
- first 2 sectors of the memory image are written to core.img

I think the 2 sectors should be written before the image is read back.
I tried to fix it, but it didn't work in the first try (GRUB hangs after
showing "GRUB Loading kernel.")

I'm also surprised that the code alternately uses dir and
DEFAULT_DIRECTORY to calculate core_path.  core_path is calculated 3
times in one function!  If dir and DEFAULT_DIRECTORY are used correctly,
I suggest that two different variables are used for what is now called
core_path.

-- 
Regards,
Pavel Roskin

Re: Failure to embed core.img is fatal now

[Javier Martín]

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

El mié, 25-06-2008 a las 17:48 -0400, Pavel Roskin escribió:
> I'm also surprised that the code alternately uses dir and
> DEFAULT_DIRECTORY to calculate core_path.  core_path is calculated 3
> times in one function!  If dir and DEFAULT_DIRECTORY are used correctly,
> I suggest that two different variables are used for what is now called
> core_path.
The core_path variable is indeed reused throughout the code: sometimes
it refers to the path as the OS and grub-setup see it, and then it's
used to search for core.img as GRUB would. This can be a bit confusing
(I've recently fixed a bug in that very function related to core_path
construction), so I agree that two different variables ought to be used.
Why not create a patch for the occasion?

[-- Attachment #2: Esta parte del mensaje está firmada digitalmente --]
[-- Type: application/pgp-signature, Size: 827 bytes --]

Re: Failure to embed core.img is fatal now

[Pavel Roskin]

On Thu, 2008-06-26 at 03:02 +0200, Javier Martín wrote:
> El mié, 25-06-2008 a las 17:48 -0400, Pavel Roskin escribió:
> > I'm also surprised that the code alternately uses dir and
> > DEFAULT_DIRECTORY to calculate core_path.  core_path is calculated 3
> > times in one function!  If dir and DEFAULT_DIRECTORY are used correctly,
> > I suggest that two different variables are used for what is now called
> > core_path.
> The core_path variable is indeed reused throughout the code: sometimes
> it refers to the path as the OS and grub-setup see it, and then it's
> used to search for core.img as GRUB would. This can be a bit confusing
> (I've recently fixed a bug in that very function related to core_path
> construction), so I agree that two different variables ought to be used.
> Why not create a patch for the occasion?

Indeed, it actually makes the actual fix smaller.  Here's the patch for
the variable.

ChangeLog:

	* util/i386/pc/grub-setup.c (setup): Don't reuse core_path,
	use core_path_dev for path relative to the partition.

diff --git a/util/i386/pc/grub-setup.c b/util/i386/pc/grub-setup.c
index 043484e..62c1bf1 100644
--- a/util/i386/pc/grub-setup.c
+++ b/util/i386/pc/grub-setup.c
@@ -91,7 +91,7 @@ setup (const char *dir,
        const char *boot_file, const char *core_file,
        const char *root, const char *dest, int must_embed)
 {
-  char *boot_path, *core_path;
+  char *boot_path, *core_path, *core_path_dev;
   char *boot_img, *core_img;
   size_t boot_size, core_size;
   grub_uint16_t core_sectors;
@@ -222,7 +222,6 @@ setup (const char *dir,
     grub_util_error ("The size of `%s' is too large", core_path);
   
   core_img = grub_util_read_image (core_path);
-  free (core_path);
 
   /* Have FIRST_BLOCK to point to the first blocklist.  */
   first_block = (struct boot_blocklist *) (core_img
@@ -368,7 +367,7 @@ setup (const char *dir,
   
   /* Make sure that GRUB reads the identical image as the OS.  */
   tmp_img = xmalloc (core_size);
-  core_path = grub_util_get_path (DEFAULT_DIRECTORY, core_file);
+  core_path_dev = grub_util_get_path (DEFAULT_DIRECTORY, core_file);
   
   /* It is a Good Thing to sync two times.  */
   sync ();
@@ -379,11 +378,11 @@ setup (const char *dir,
   for (i = 0; i < MAX_TRIES; i++)
     {
       grub_util_info ("attempting to read the core image `%s' from GRUB%s",
-		      core_path, (i == 0) ? "" : " again");
+		      core_path_dev, (i == 0) ? "" : " again");
       
       grub_disk_cache_invalidate_all ();
       
-      file = grub_file_open (core_path);
+      file = grub_file_open (core_path_dev);
       if (file)
 	{
 	  if (grub_file_size (file) != core_size)
@@ -436,7 +435,7 @@ setup (const char *dir,
     }
 
   if (i == MAX_TRIES)
-    grub_util_error ("Cannot read `%s' correctly", core_path);
+    grub_util_error ("Cannot read `%s' correctly", core_path_dev);
 
   /* Clean out the blocklists.  */
   block = first_block;
@@ -470,7 +469,7 @@ setup (const char *dir,
 
   grub_file_close (file);
   
-  free (core_path);
+  free (core_path_dev);
   free (tmp_img);
   
   *kernel_sector = grub_cpu_to_le64 (first_sector);
@@ -487,7 +486,6 @@ setup (const char *dir,
   *root_drive = 0xFF;
 
   /* Write the first two sectors of the core image onto the disk.  */
-  core_path = grub_util_get_path (dir, core_file);
   grub_util_info ("opening the core image `%s'", core_path);
   fp = fopen (core_path, "r+b");
   if (! fp)
@@ -495,7 +493,6 @@ setup (const char *dir,
 
   grub_util_write_image (core_img, GRUB_DISK_SECTOR_SIZE * 2, fp);
   fclose (fp);
-  free (core_path);
 
   /* Write the boot image onto the disk.  */
   if (grub_disk_write (dest_dev->disk, 0, 0, GRUB_DISK_SECTOR_SIZE, boot_img))
@@ -506,6 +503,7 @@ setup (const char *dir,
   /* Sync is a Good Thing.  */
   sync ();
   
+  free (core_path);
   free (core_img);
   free (boot_img);
   grub_device_close (dest_dev);


-- 
Regards,
Pavel Roskin

Re: Failure to embed core.img is fatal now

[Pavel Roskin]

On Wed, 2008-06-25 at 17:48 -0400, Pavel Roskin wrote:

> It turns out following happens in grub-setup:
> 
> - core.img is read by the OS facilities
> - the memory image is modified
> - the memory image is compared to core.img read by GRUB FS code and fails
> - the memory image is modified again
> - first 2 sectors of the memory image are written to core.img
> 
> I think the 2 sectors should be written before the image is read back.
> I tried to fix it, but it didn't work in the first try (GRUB hangs after
> showing "GRUB Loading kernel.")

It turns out the second sector is modified first.  And the first sector
is modified with the locations of the subsequent sectors.  Thus we
should either write the second sector before the first sector or keep an
unmodified image in memory to check that core.img can be read with the
GRUB filesystem code.

I'm not very concerned about memory consumption, but I think it would be
safer and easier to split writing sectors.  We want to write as little
as possible to core.img after it have been tested for readability and
the blocks have been calculated.

ChangeLog:

	* util/i386/pc/grub-setup.c (setup): If core.img cannot be
	embedded, write the second sector before the reading with
	GRUB filesystem code is tested, so that GRUB reads the
	image with the changes made to the second sector.


diff --git a/util/i386/pc/grub-setup.c b/util/i386/pc/grub-setup.c
index 62c1bf1..d8cbb12 100644
--- a/util/i386/pc/grub-setup.c
+++ b/util/i386/pc/grub-setup.c
@@ -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.  */


-- 
Regards,
Pavel Roskin

Re: Failure to embed core.img is fatal now

[Robert Millan]

On Wed, Jun 25, 2008 at 05:48:34PM -0400, Pavel Roskin wrote:
> 
> I'm also surprised that the code alternately uses dir and
> DEFAULT_DIRECTORY to calculate core_path.  core_path is calculated 3
> times in one function!  If dir and DEFAULT_DIRECTORY are used correctly,
> I suggest that two different variables are used for what is now called
> core_path.

Might be a remnant from when grub-setup mangled prefix?  I recently moved
this off grub-setup and into grub-install/grub-mkimage so that core.img had
the right prefix regardless on whether it was being loaded directly by a
multiboot loader or via grub-setup.

Or maybe I'm completely out of context here.  In any case, please take into
account that modifications done to core.img by grub-setup have no effect if
core.img is being loaded directly.

-- 
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 /.)

Re: Failure to embed core.img is fatal now

[Pavel Roskin]

On Thu, 2008-06-26 at 16:20 +0200, Robert Millan wrote:
> On Wed, Jun 25, 2008 at 05:48:34PM -0400, Pavel Roskin wrote:
> > 
> > I'm also surprised that the code alternately uses dir and
> > DEFAULT_DIRECTORY to calculate core_path.  core_path is calculated 3
> > times in one function!  If dir and DEFAULT_DIRECTORY are used correctly,
> > I suggest that two different variables are used for what is now called
> > core_path.
> 
> Might be a remnant from when grub-setup mangled prefix?  I recently moved
> this off grub-setup and into grub-install/grub-mkimage so that core.img had
> the right prefix regardless on whether it was being loaded directly by a
> multiboot loader or via grub-setup.

No, it's all old code, traced to years 2003-2004.  But it needs to be
handled better because we need to move things around at this point.  If
nothing else, it will make the code more readable and less prone to
breakages.

> Or maybe I'm completely out of context here.  In any case, please take into
> account that modifications done to core.img by grub-setup have no effect if
> core.img is being loaded directly.

I know.  That's why they are only written to disk if core.img is not
embedded.

Anyway, I think there is a much simpler approach.  Changes to the memory
image of core.img should be applied after the image is tested for
readability.  Until then, they should be in temporary variables.

-- 
Regards,
Pavel Roskin

Re: Failure to embed core.img is fatal now

[Pavel Roskin]

On Thu, 2008-06-26 at 10:48 -0400, Pavel Roskin wrote:

> Anyway, I think there is a much simpler approach.  Changes to the memory
> image of core.img should be applied after the image is tested for
> readability.  Until then, they should be in temporary variables.

Here's the patch.  It's an alternative to writing the second sector
first.  Now GRUB would modify core_img immediately before writing it.

diff --git a/util/i386/pc/grub-setup.c b/util/i386/pc/grub-setup.c
index 62c1bf1..3e9b4b5 100644
--- a/util/i386/pc/grub-setup.c
+++ b/util/i386/pc/grub-setup.c
@@ -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");


-- 
Regards,
Pavel Roskin

Re: Failure to embed core.img is fatal now

[Robert Millan]

On Thu, Jun 26, 2008 at 12:54:54PM -0400, Pavel Roskin wrote:
> +	      dos_part = grub_cpu_to_le32 (root_dev->disk->partition->index);
> +	      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));

You're doing endian conversion twice.

-- 
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 /.)

Re: Failure to embed core.img is fatal now

[Pavel Roskin]

Quoting Robert Millan <rmh@aybabtu.com>:

> On Thu, Jun 26, 2008 at 12:54:54PM -0400, Pavel Roskin wrote:
>> +	      dos_part = grub_cpu_to_le32 (root_dev->disk->partition->index);
>> +	      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));
>
> You're doing endian conversion twice.

Nice catch, thanks!  Anyway, the code is PC specific, so the  
conversions do nothing.

-- 
Regards,
Pavel Roskin