Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fdisk does not produce Protective MBR for GPT correctly #485

Closed
pali opened this issue Jul 12, 2017 · 8 comments
Closed

fdisk does not produce Protective MBR for GPT correctly #485

pali opened this issue Jul 12, 2017 · 8 comments

Comments

@pali
Copy link
Contributor

pali commented Jul 12, 2017

Reproduce:

$ truncate -s 10G /tmp/gpt

$ printf "g\nw\n" | ./fdisk /tmp/gpt

Welcome to fdisk (util-linux 2.30.196-00b33-dirty).
Changes will remain in memory only, until you decide to write them.
Be careful before using the write command.

Device does not contain a recognized partition table.
Created a new DOS disklabel with disk identifier 0x29ebb066.

Command (m for help): Created a new GPT disklabel (GUID: 466208E1-DFA3-FB41-8D8C-8CFC2B3618F8).

Command (m for help): The partition table has been altered.
Syncing disks.

$ ./fdisk -t dos -l -o Device,Boot,Start,End,Sectors,Size,Id,Type,Start-C/H/S,End-C/H/S,Cylinders,Attrs /tmp/gpt
Disk /tmp/gpt: 10 GiB, 10737418240 bytes, 20971520 sectors
Units: sectors of 1 * 512 = 512 bytes
Sector size (logical/physical): 512 bytes / 512 bytes
I/O size (minimum/optimal): 512 bytes / 512 bytes
Disklabel type: dos
Disk identifier: 0x00000000

Device      Boot Start      End  Sectors Size Id Type Start-C/H/S   End-C/H/S Cylinders Attrs
/tmp/gpt1            1 20971519 20971519  10G ee GPT        0/0/1 1023/254/63      1306

According to UEFI Specification 2.6, section 5 GUID Partition Table (GPT) Disk Layout, subsection 5.2.3 Protective MBR, Table 17. Protective MBR Partition Record protecting the entire disk, StartingCHS is 0x000200 (0/0/2) and EndingCHS is 0xFFFFFF (1023/255/63) if it is not possible to represent last logical block as C-H-S.

Here is output when GPT disk was initialized by gdisk:

$ truncate -s 10G /tmp/gpt2

$ printf "o\ny\nw\ny" | gdisk /tmp/gpt2
GPT fdisk (gdisk) version 0.8.1

Partition table scan:
  MBR: not present
  BSD: not present
  APM: not present
  GPT: not present

Creating new GPT entries.

Command (? for help): This option deletes all partitions and creates a new protective MBR.
Proceed? (Y/N): 
Command (? for help): 
Final checks complete. About to write GPT data. THIS WILL OVERWRITE EXISTING
PARTITIONS!!

Do you want to proceed? (Y/N): OK; writing new GUID partition table (GPT).
Warning: The kernel is still using the old partition table.
The new table will be used at the next reboot.
The operation has completed successfully.

$ ./fdisk -t dos -l -o Device,Boot,Start,End,Sectors,Size,Id,Type,Start-C/H/S,End-C/H/S,Cylinders,Attrs /tmp/gpt2
Disk /tmp/gpt2: 10 GiB, 10737418240 bytes, 20971520 sectors
Units: sectors of 1 * 512 = 512 bytes
Sector size (logical/physical): 512 bytes / 512 bytes
I/O size (minimum/optimal): 512 bytes / 512 bytes
Disklabel type: dos
Disk identifier: 0x00000000

Device      Boot Start      End  Sectors Size Id Type Start-C/H/S   End-C/H/S Cylinders Attrs
/tmp/gpt2p1          1 20971519 20971519  10G ee GPT        0/0/2 1023/255/63      1301

Protective MBR is there correct.

Please make fdisk compliant to UEFI/GPT specification.

karelzak added a commit that referenced this issue Jul 14, 2017
The PMBR partition record should be StartingCHS=0x002000 (0/0/2)
and EndingCHS=0xFFFFFF (1023/255/63)

Addresses: #485
Signed-off-by: Karel Zak <kzak@redhat.com>
@karelzak
Copy link
Collaborator

Nice catch! Fixed.

printf "g\nw\n" | ./fdisk img2

Welcome to fdisk (util-linux 2.30.235-c0eab-dirty).
Changes will remain in memory only, until you decide to write them.
Be careful before using the write command.


Command (m for help): Created a new GPT disklabel (GUID: 013F09DF-3E52-574F-901A-36E8395C5FBD).

Command (m for help): The partition table has been altered.
Syncing disks.

$ ./fdisk -t dos -l -o Device,Boot,Start,End,Sectors,Size,Id,Type,Start-C/H/S,End-C/H/S,Cylinders,Attrs img1
Disk img1: 10 GiB, 10737418240 bytes, 20971520 sectors
Units: sectors of 1 * 512 = 512 bytes
Sector size (logical/physical): 512 bytes / 512 bytes
I/O size (minimum/optimal): 512 bytes / 512 bytes
Disklabel type: dos
Disk identifier: 0x00000000

Device     Boot Start      End  Sectors Size Id Type Start-C/H/S   End-C/H/S Cylinders Attrs
img1p1              1 20971519 20971519  10G ee GPT        0/0/2 1023/255/63      1301      

@pali
Copy link
Contributor Author

pali commented Jul 15, 2017

Commit 8ffa3b6 is not so correct as ** EndingCHS** needs to be calculated from C-H-S disk geometry and value 0xFFFFFF (1023/255/63) is used only if it is not possible to represent last logical block as C-H-S.

@karelzak
Copy link
Collaborator

Well, the same solution uses GNU Parted.. not sure if we have to care about this so much as nothing uses CHS.

@pali
Copy link
Contributor Author

pali commented Jul 15, 2017

C-H-S is probably not used anymore... but UEFI/GPT specification clearly say what should be filled into structures. And tool which aim to support GPT should follow specification and do not invent its own rules. And at least gdisk has it implemented, so in case of problems, code can be reused.

karelzak added a commit that referenced this issue Jul 17, 2017
The PMBR partition record should be StartingCHS=0x002000 (0/0/2)
and EndingCHS=0xFFFFFF (1023/255/63)

Addresses: #485
Signed-off-by: Karel Zak <kzak@redhat.com>
@marcosfrm
Copy link
Contributor

Microsoft appears to not follow it too...

http://thestarman.pcministry.com/asm/mbr/GPT.htm#GPTPT

@pali
Copy link
Contributor Author

pali commented Jul 17, 2017

We know that Microsoft notoriously does not follow industrial standards or implement them differently (E, E, E strategy). So it is not a surprise that Microsoft implements also GPT in non-compliant way. But it does not mean that other developers should do same thing.

@karelzak
Copy link
Collaborator

The problem is that for CHS you need device geometry and it's not always available (e.g. some logical volumes, non block device partitioning, etc.) and it's poorly portable between devices (dd(1) PT from one disk to another). The reason why we want to use GPT is to avoid dos-like junk :-)

So from my point of view GNU Parted and MS solution makes sense and it improves things. IMHO it would be better to update the standard than force applications to care about absolutely obsolete CHS in year 2017.

@pali
Copy link
Contributor Author

pali commented Jul 17, 2017

The problem is that for CHS you need device geometry and it's not always available (e.g. some logical volumes, non block device partitioning, etc.)

Yes, and in most cases C-H-S does not make sense for hdds and BIOS uses some values... Looking at wikipedia there is interesting transition table between LBA and C-H-S numbers. No idea what is the source of data...

and it's poorly portable between devices (dd(1) PT from one disk to another).

GPT itself is not portable between devices (via dd) as it stores layout at beginning and also at the end of disk. Also GPT is addressed by logical sector size and now when usage of native 4K disks is increasing, more problems just come. Two identical disks should not have GPT problem, but probably would have also same C-H-S compatibility.

IMHO it would be better to update the standard than force applications to care about absolutely obsolete CHS in year 2017.

I fully agree with you. Problem is that standard is not updated yet and so compliant applications still need to care...

smoser pushed a commit to smoser/disko that referenced this issue May 11, 2020
Both the GPT table and ProtectiveMBR need to be updated or
re-written if the disk size changes.  That is not a very common
event with real disks (disks don't magically get more bytes).
But, it is common for virtual disks to be "grown" (zeros added
to the end).

The GPT header at the beginning of a disk contains a reference
to the location of a copy (HeaderCopyStartLBA) which is typically
at the end of the disk.  The ProtectiveMBR is supposed to span
the entire disk.  So both of these things have to be updated
if the disk changes size.

The change here is just to make sure we update both whenever
we write a GPT partition table.

A simple 'diff' doesn't show it well, but what happened is:

a.) move writeProtectiveMBR from writeNewGPTTable to writeGPTTable
b.) pass a disk size into writeGPTTable so that it can pass it through
    to writeProtectiveMBR when it needs it.

writeProtectiveMBR doesn't *actually* need the size, because most
partition table writers will just write a partition length of
0xFFFFFFFF independent of the size of the disk.  But for now we
can't just do that because the MBR package will return error from
Check().  I've tried to get that fixed.  See conversation on
 https://github.com/rekby/mbr/pull/2/files
and
 util-linux/util-linux#485
smoser pushed a commit to smoser/disko that referenced this issue May 11, 2020
Both the GPT table and ProtectiveMBR need to be updated or
re-written if the disk size changes.  That is not a very common
event with real disks (disks don't magically get more bytes).
But, it is common for virtual disks to be "grown" (zeros added
to the end).

The GPT header at the beginning of a disk contains a reference
to the location of a copy (HeaderCopyStartLBA) which is typically
at the end of the disk.  The ProtectiveMBR is supposed to span
the entire disk.  So both of these things have to be updated
if the disk changes size.

The change here is just to make sure we update both whenever
we write a GPT partition table.

A simple 'diff' doesn't show it well, but what happened is:

a.) move writeProtectiveMBR from writeNewGPTTable to writeGPTTable
b.) pass a disk size into writeGPTTable so that it can pass it through
    to writeProtectiveMBR when it needs it.

writeProtectiveMBR doesn't *actually* need the size, because most
partition table writers will just write a partition length of
0xFFFFFFFF independent of the size of the disk.  But for now we
can't just do that because the MBR package will return error from
Check().  I've tried to get that fixed.  See conversation on
 https://github.com/rekby/mbr/pull/2/files
and
 util-linux/util-linux#485
smoser pushed a commit to smoser/disko that referenced this issue May 11, 2020
Both the GPT table and ProtectiveMBR need to be updated or
re-written if the disk size changes.  That is not a very common
event with real disks (disks don't magically get more bytes).
But, it is common for virtual disks to be "grown" (zeros added
to the end).

The GPT header at the beginning of a disk contains a reference
to the location of a copy (HeaderCopyStartLBA) which is typically
at the end of the disk.  The ProtectiveMBR is supposed to span
the entire disk.  So both of these things have to be updated
if the disk changes size.

The change here is just to make sure we update both whenever
we write a GPT partition table.

A simple 'diff' doesn't show it well, but what happened is:

a.) move writeProtectiveMBR from writeNewGPTTable to writeGPTTable
b.) pass a disk size into writeGPTTable so that it can pass it through
    to writeProtectiveMBR when it needs it.

writeProtectiveMBR doesn't *actually* need the size, because most
partition table writers will just write a partition length of
0xFFFFFFFF independent of the size of the disk.  But for now we
can't just do that because the MBR package will return error from
Check().  I've tried to get that fixed.  See conversation on
 https://github.com/rekby/mbr/pull/2/files
and
 util-linux/util-linux#485
smoser pushed a commit to smoser/disko that referenced this issue May 13, 2020
Both the GPT table and ProtectiveMBR need to be updated or
re-written if the disk size changes.  That is not a very common
event with real disks (disks don't magically get more bytes).
But, it is common for virtual disks to be "grown" (zeros added
to the end).

The GPT header at the beginning of a disk contains a reference
to the location of a copy (HeaderCopyStartLBA) which is typically
at the end of the disk.  The ProtectiveMBR is supposed to span
the entire disk.  So both of these things have to be updated
if the disk changes size.

The change here is just to make sure we update both whenever
we write a GPT partition table.

A simple 'diff' doesn't show it well, but what happened is:

a.) move writeProtectiveMBR from writeNewGPTTable to writeGPTTable
b.) pass a disk size into writeGPTTable so that it can pass it through
    to writeProtectiveMBR when it needs it.

writeProtectiveMBR doesn't *actually* need the size, because most
partition table writers will just write a partition length of
0xFFFFFFFF independent of the size of the disk.  But for now we
can't just do that because the MBR package will return error from
Check().  I've tried to get that fixed.  See conversation on
 https://github.com/rekby/mbr/pull/2/files
and
 util-linux/util-linux#485
smoser pushed a commit to smoser/disko that referenced this issue May 13, 2020
Both the GPT table and ProtectiveMBR need to be updated or
re-written if the disk size changes.  That is not a very common
event with real disks (disks don't magically get more bytes).
But, it is common for virtual disks to be "grown" (zeros added
to the end).

The GPT header at the beginning of a disk contains a reference
to the location of a copy (HeaderCopyStartLBA) which is typically
at the end of the disk.  The ProtectiveMBR is supposed to span
the entire disk.  So both of these things have to be updated
if the disk changes size.

The change here is just to make sure we update both whenever
we write a GPT partition table.

A simple 'diff' doesn't show it well, but what happened is:

a.) move writeProtectiveMBR from writeNewGPTTable to writeGPTTable
b.) pass a disk size into writeGPTTable so that it can pass it through
    to writeProtectiveMBR when it needs it.

writeProtectiveMBR doesn't *actually* need the size, because most
partition table writers will just write a partition length of
0xFFFFFFFF independent of the size of the disk.  But for now we
can't just do that because the MBR package will return error from
Check().  I've tried to get that fixed.  See conversation on
 https://github.com/rekby/mbr/pull/2/files
and
 util-linux/util-linux#485
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants