Page MenuHomeFreeBSD

Fix GELIBoot support if GELI sector size is > 512
ClosedPublic

Authored by allanjude on Apr 4 2016, 4:31 AM.

Details

Summary

Add support for 4k sector GELI encrypted partitions to the bootloader
This is the default created by the installer

Because the IV is different for each sector, and the XTS tweak carries forward you can not decrypt a partial sector if the starting offset is not 0

Make boot2 and the loader read in 4k aligned chunks

Test Plan

Testing on a laptop and in virtualbox with 512 and 4096 byte GELI sectors
Tested with ZFS with ashift=9, which resulted in unaligned reads that required additional code to get working

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

allanjude updated this revision to Diff 14851.Apr 4 2016, 4:31 AM
allanjude retitled this revision from to Fix GELIBoot support if GELI sector size is > 512.
allanjude updated this object.
allanjude edited the test plan for this revision. (Show Details)
allanjude added reviewers: ed, oshogbo.
ed edited edge metadata.EditedApr 4 2016, 9:29 AM

Hi there,

Thanks for working on this! I've written some comments for you, but also have a small number of general remarks:

  • Casts: Your code seems to have an above average number of casts in it. Casts should only be used in the rare occasion that they're actually needed. They may also be the source of subtle bugs. For example, in your code you use ~(unsigned int)x in a couple of places. This can be disastrous when promoted to 64 bits. uint64_t x = ~(unsigned int)1; does not equal 0xfffffffffffffffe, but 0xfffffffe.
  • Comments: style(9) requires that comments have a full stop at the end. Also make sure that multi-line comments are properly formatted.
  • Operators: style(9) requires that unary operators (casts, logical/bitwise negation) have no trailing whitespace. If a statements needs to be broken up across multiple lines, make sure that the operator is written at the end of the first line.
sys/boot/geli/geliboot.c
80 ↗(On Diff #14851)

The name of the variable somewhat contradicts its use. My initial thought would be that this computes the byte offset of the disk sector inside of the GELI sector (the remainder), but if I take a look at the arithmetic, it rounds the offset of the disk sector down to its GELI sector. Maybe it should be called alignsector?

86 ↗(On Diff #14851)

Instead of copying, you could also just turn buf into a pointer, pointing to the last sector, right? Maybe you could just eliminate buf and call eli_metadata_decode() directly with the right offset?

sys/boot/geli/geliboot.h
59 ↗(On Diff #14851)

I'm personally not too happy with naming like this. The value that it has is already encoded in the name. I would be amazed if this definition would have any value other than 4096.

I think that the intent behind this definition is that it corresponds to GELI's block size, right? Maybe it should be called DEV_GELI_BSIZE? Most of the comments in the code below also mention 512 and 4096 explicitly, which I'm not a fan of.

sys/boot/i386/zfsboot/zfsboot.c
248 ↗(On Diff #14851)

s/leadingg/leading/

allanjude marked 3 inline comments as done.Apr 4 2016, 3:42 PM
In D5820#124356, @ed wrote:

Hi there,

Thanks for working on this! I've written some comments for you, but also have a small number of general remarks:

  • Casts: Your code seems to have an above average number of casts in it. Casts should only be used in the rare occasion that they're actually needed. They may also be the source of subtle bugs. For example, in your code you use ~(unsigned int)x in a couple of places. This can be disastrous when promoted to 64 bits. uint64_t x = ~(unsigned int)1; does not equal 0xfffffffffffffffe, but 0xfffffffe.

Yes, I was concerned about this, this is why I am seeking review.
Currently, the explicit casts all immediately proceed the variable that we are taking the complement of, to ensure the correct value is used
The casts all match the destination variable.

  • Comments: style(9) requires that comments have a full stop at the end. Also make sure that multi-line comments are properly formatted.

I think I got all of these

  • Operators: style(9) requires that unary operators (casts, logical/bitwise negation) have no trailing whitespace. If a statements needs to be broken up across multiple lines, make sure that the operator is written at the end of the first line.

Fixed

allanjude updated this revision to Diff 14870.Apr 4 2016, 4:09 PM
allanjude edited edge metadata.

Address feedback from Ed

Remove all magic numbers

allanjude added inline comments.Apr 4 2016, 4:16 PM
sys/boot/geli/geliboot.h
59 ↗(On Diff #14851)

GELI blocks can be 512 bytes or 4096 bytes. The code just assumes 4096 because that way it works in either case.

I guess I'll call it DEV_GELIBOOT_BSIZE since it is not specific to GELI

ed added a comment.Apr 4 2016, 5:56 PM

Currently, the explicit casts all immediately proceed the variable that we are taking the complement of, to ensure the correct value is used

But in some cases these casts provide absolutely no value. For example, take the expression below:

(int)((DEV_GELIBOOT_BSIZE / BIOSDISK_SECSIZE) - 1)

It would work equally well without the cast. We're just doing simple arithmetic on some integer literals here.

The casts all match the destination variable.

Sure, but if anyone ever refactors this code, this may easily become no longer the case.

oshogbo edited edge metadata.Apr 4 2016, 6:42 PM

Only some style nits.

sys/boot/geli/geliboot.c
263 ↗(On Diff #14870)

Over 80.

sys/boot/i386/libi386/biosdisk.c
725 ↗(On Diff #14870)

Style.
I would go with:
alignlba = dblk &

~(daddr_t)((DEV_GELIBOOT_BSIZE / BIOSDISK_SECSIZE) - 1);
732 ↗(On Diff #14870)

Up.

sys/boot/i386/zfsboot/zfsboot.c
248 ↗(On Diff #14870)

Over 80.

In D5820#124493, @ed wrote:

Currently, the explicit casts all immediately proceed the variable that we are taking the complement of, to ensure the correct value is used

But in some cases these casts provide absolutely no value. For example, take the expression below:

(int)((DEV_GELIBOOT_BSIZE / BIOSDISK_SECSIZE) - 1)

Maybe it is the bad line wrap, but we are taking the compliment of that value, hence the cast.

It would work equally well without the cast. We're just doing simple arithmetic on some integer literals here.

The casts all match the destination variable.

Sure, but if anyone ever refactors this code, this may easily become no longer the case.

allanjude updated this revision to Diff 14879.Apr 5 2016, 5:50 AM
allanjude marked 3 inline comments as done.
allanjude edited edge metadata.

Fixes suggested by Ed and Marius

swills added a subscriber: swills.Apr 5 2016, 1:36 PM
ed accepted this revision.Apr 6 2016, 10:02 AM
ed edited edge metadata.
This revision is now accepted and ready to land.Apr 6 2016, 10:02 AM
oshogbo accepted this revision.Apr 6 2016, 10:51 AM
oshogbo edited edge metadata.
This revision was automatically updated to reflect the committed changes.

It seems to me that this change cause

ZFS: i/o error - all block copies unavailable

after some reboots (always after a crash).

I am crashing a Xen kernel to reproduce this:

https://lists.freebsd.org/pipermail/freebsd-virtualization/2016-April/004362.html

https://lists.freebsd.org/pipermail/freebsd-current/2016-April/060949.html

I have used a simple bisecting recompiling /usr/src/lib/libstand and /usr/src/sys/boot to find the broken commit.

It seems to me that this change cause

ZFS: i/o error - all block copies unavailable

after some reboots (always after a crash).

Can you compile sys/boot with the make flag: -DBOOT_BIOSDISK_DEBUG

It should give some insight into what is going wrong

saper_saper.info added a comment.EditedMay 1 2016, 10:08 PM

Adding -DBOOT_BIOSDISK_DEBUG does not seem to influence gptzfsboot very much (I am even getting the same binary back).

I have managed to reproduce this easily using a FreeBSD 11 r298793 image using the installer, with partition sizes adjusted and the "Force 4K sector size" option off.

Here is the zfs stream of the zvol containing the whole device: http://marcincieslak.com/tmp/freebsdX.snapshot.xz (root password is "tester"). This is a pristine version right after the installer was done, therefore it always boots. I have used Xen to boot this one but it might work on bhyve as well (but one really needs console).

However, sometimes just a plain shutdown -r now causes the gptzfsboot abort. I think it depends on what kind of ZFS activity was done prior to the reboot.

But saying sysctl debug.kdb.panic=1 and rebooting is a 100% way to reproduce this.