Building to GPT disks may write the wrong partition offset under Linux

[tqh]

I use GPT partioning on my disks, and have a UEFI boot-partition and then ordinary partitions like this:

gptsync /dev/sda

Current GPT partition table:
 #      Start LBA      End LBA  Type
 1           2048         4095  BIOS Boot Partition
 2           4096     62918655  Basic Data (Linux)
 3       62918656     87494655  Basic Data (Haiku)
 4       87494656    107974655  Basic Data (Haiku)

Current MBR partition table:
 # A    Start LBA      End LBA  Type
 1              1         2047  ee  EFI Protective
 2           2048         4095  da  Non-FS data
 3           4096     62918655  83  Linux
 4 *     62918656     87494655  eb  BeOS

Building under Linux to /dev/sda4 will write the build to sda4, but write the boot code to sda3 because the GPT and the MBR layout is not directly mappable to each other. I've verified this problem from the output when building and the offsets given are for sda3.

[tqh]

I'm thinking that getting the offset from GPT would be the right thing to do, but want feedback.

[axeld]

IIRC we use a platform dependent solution to come up with the offsets. Does the boot code actually write to /dev/sda3 in your case, or is just the wrong offset written to /dev/sda4 -- I wouldn't understand the former problem, at least. Could you elaborate, please?

[tqh]

It was a while since I started looking at it, but got sidetracked then. I'm doing a build now to provide more details.

[tqh]

BuildHaikuImage1 /dev/sda4 

Creating image ...
Writing boot code to "/dev/sda4" (partition offset: 32214351872 bytes, start offset = 0) ...

As a sector in my case is 512 bytes: 62918656 * 512 = 32214351872. It is the wrong offset as the real /dev/sda4 is at 87494656.

[axeld]

Alright, so the problem is actually pretty simple, see ​http://cgit.haiku-os.org/haiku/tree/src/bin/makebootable/platform/bios_ia32/makebootable.cpp#n460 -- it only parses the Intel partition map, it doesn't parse GPT as it would have to in this case. The FreeBSD solution has the same problem.

If libudev is installed you could use udev_device_get_syspath() to get the path of the device in /sys, and then parse the "start" file to find the actual partition offset. Since you can never reliably guess how Linux assigns the device names, this should be the best solution we can come up with. It just adds another build dependency, though (and possibly should be made optional, as I don't know if udev is mandatory in Linux).

[tqh]

Rephrasing bug to be more accurate. The wrong partition offset is sent in as a parameter to ​http://cgit.haiku-os.org/haiku/tree/src/bin/makebootable/platform/bios_ia32/makebootable.cpp

[tqh]

From what I can tell $bfsShell sets imageOffsetFlags in build/scripts/build_haiku_image:

                $bfsShell --initialize $imageOffsetFlags "$imagePath" \
                        "$imageLabel" "block_size 2048"
                $makebootable $imageOffsetFlags "$imagePath"

[axeld]

Replying to tqh:

From what I can tell $bfsShell sets imageOffsetFlags in build/scripts/build_haiku_image:

You're on the wrong track: this functionality is for offsets within images (hence the name). For example VMDK files have a 64K offset at the start before the actual partition begins. I've already pointed out the bug in comment:5, as well as how to solve it.

[tqh]

Yes, didn't see your comment when I posted. I'm looking into it.

[tqh]

I think I'll start looking into how PartitionMap code is used in makebootable first.

[tqh]

udev does look like an easy solution.

$ udevadm info --query=all --name=/dev/sda4
...
UDISKS_PARTITION_OFFSET=44797263872
..

So get the udev_device and read the partition offset property. If that fails use partitionmap as before.

[tqh]

It looks like using libudev is quite complicated with multilib (-m32), at least on Ubuntu where the i386 package replace the x64 one. So not sure udev is so easy to use on 64-bit Linux.

[bonefish]

I think it should always be checked whether the disk is GPT partitioned and *never* even be tried to determine the partition offset using the partition map method, if it is. That would IMO already fix the bug part of the ticket.

Regarding udev, I don't know how stable the udevadm output is, but using the command (via popen()) rather than the library might be a viable option.

[tqh]

Ah good to know. I plan to fix this bug, but I found out that with gdisk you can actually configure your mbr so it matches again, so until then there is a way to get past it.

[umccullough]

makebootabletiny.c (​http://stefanschramm.net/dev/makebootabletiny/makebootabletiny.c) appears to use another option available on Linux to find the offset:

#include <linux/hdreg.h>

Could that be temporarily utilized on Linux hosts to solve the issue?

[tqh]

It could, but unfortunatly only if the disk is less than <2TB on 32 bit systems. So that would just be temporary anyway.

[bonefish]

Replying to tqh:

Ah good to know. I plan to fix this bug, but I found out that with gdisk you can actually configure your mbr so it matches again, so until then there is a way to get past it.

Sure, but if you're unaware of the problem, makebootable may write to the wrong partition, which, depending on what that partition contains, might cause a serious problem. So I'd rather have makebootable always fail when detecting a GPT disk than do what it does now. That would still allow a simple addition like a --offset <...> parameter that would just cause the tool to write the given offset to the given partition without any further checks.

A more intelligent solution would, of course, be preferable.

[tqh]

Yes I agree, unfortunatly I'm away for easter so don't know if I have time to do anything the following week. The block after MBR is the GPT-partition table header on GPT-disks, and it is quite easy to detect:

From ​http://en.wikipedia.org/wiki/GUID_Partition_Table

0 	8 bytes 	Signature ("EFI PART", 45h 46h 49h 20h 50h 41h 52h 54h)

[zymoticb]

Patch that will cause the intel partition parser to exit with B_BAD_DATA if it detects the GPT signature.

[zymoticb]

I have added a patch to this bug (I modified PartitionMapParser.{h,cpp} ). This patch simply checks for the 8 byte signature after the MBR. It only modifys the _ReadPartitionTable private method. This is the first patch I have submitted against this codebase so please be verbose in any style issues. I returned B_BAD_DATA but if there is something more appropriate to return I can modify the patch.

[bonefish]

Here you go:

• The indentation isn't correct. Should be one tab per indentation level (set to 4 characters width -- relevant for the line length limit).
• If you only use kGPTSignatureSize in this one method, there's no reason to expose it in the header (unless it's intended to be used elsewhere in the future). Also, why make a constant for the size, but inline the signature string itself? I would define both as constants either directly in the method before their use or at the beginning of the source file.
• There should be a space after //.
• When you look at the rest of the source file, you'll notice that the source is formatted more loosely, using single blank lines to separate code blocks from each other. There should be at least one blank line to separate the new code from the preceding block. I would also add another one before the second (top level) if block.
• Generally the "early return on error" style is preferred. The existing code was written before that style was embraced, but new code should follow it. E.g. in this case it would be preferable to rewrite the previous block to do that too, so your code isn't executed unnecessarily. Also, the way the read_pos() result is processed is not correct. If it's just a short read (i.e. no error) errno will not have been set and the assumption that it is B_OK is incorrect. Under the assumption that toRead doesn't overflow ssize_t, that should better be done like:

  ssize_t bytesRead = read_pos(...);
  if (bytesRead != (ssize_t)(toRead))
      return bytesRead < 0 ? errno : B_IO_ERROR;
  

• The _BOOT_MODE exception is no longer necessary. The boot loader's read_pos() has been fixed to set errno correctly.
• Using "early return on error" style also makes it unnecessary to initialize gptSignature.
• The first if line is too long. The limit is 80 columns.
• Using strcmp() is incorrect. gptSignature is only 8 bytes wide, but strcmp() will compare 9 bytes, including the terminating null character. If the null character is part of the signature, kGPTSignatureSize needs to be adjusted. In either case I would prefer memcpy() for the comparison.

[zymoticb]

I tried to make the patch conform as well as possible. I hope I understood all of your suggestions correctly; of note is that I assumed in your last bullet point you meant using memcmp() for comparison not memcpy().

[zymoticb]

Updated with bonefish's suggestions

[bonefish]

Thanks for the updated patch. The new patch still has a few style issues:

• Generally indentation is not used just to align stuff. E.g. the read_pos() arguments should just flow to the end of line, wrapped there (at a convenient point) and continue with an incremented indentation level (exactly one additional tab) on the next line. When nested expressions are wrapped, an additional indentation level per nesting level can be added.
• There shouldn't be a space after the cast operator.
• Besides the space, there's one superfluous set of parentheses in (ssize_t) (toRead). When using the cast operator, the parentheses around toRead are not necessary. Alternative you can use the constructor style conversion, if you like that better: ssize_t(toRead).
• The spacing of the three if statements is incorrect. The { should be separated by a space. For if two and three the spacing after the if keyword is incorrect.
• errno is of type int, so the printf() format should be %x (or even nicer %#x). The preferred way to print errors is via strerror(), but I believe that isn't available in the boot loader ATM.
• The gptSignature definition should not hardcode the array size, but use the kGPTSignatureSize constant.

[tqh]

Amended patch applied in hrev44963. Thanks for working on it.