Add support for boot environment data to be stored in the label #10009

pcd1193182 · 2020-02-15T21:52:33Z

Motivation and Context

Modern bootloaders leverage data stored in the root filesystem to enable some of their powerful features. GRUB specifically has a grubenv file which can store large amounts of configuration data that can be read and written at boot time and during normal operation. This allows sysadmins to configure useful features like automated failover after failed boot attempts. Unfortunately, due to the Copy-on-Write nature of ZFS, the standard behavior of these tools cannot handle writing to ZFS files safely at boot time. We need an alternative way to store data that allows the bootloader to make changes to the data.

Description

This work is very similar to work that was done on Illumos to enable similar functionality in the FreeBSD bootloader. This patch is different in that the data being stored is a raw grubenv file; this file can store arbitrary variables and values, and the scripting provided by grub is powerful enough that special structures are not required to implement advanced behavior.

We repurpose the second padding area in each label to store the grubenv file, protected by an embedded checksum. We add two ioctls to get and set this data, and libzfs_core and libzfs functions to access them more easily. There are no direct command line interfaces to these functions; these will be added directly to the bootloader utilities.

How Has This Been Tested?

These changes have been tested in conjunction with a custom version of GRUB that knows how to read from and write to the padding areas of the label to access to the grubenv data. Many tests were performed, including selectively corrupting the contents of the padding areas to verify that edge cases are handled correctly.

The GRUB changes that enabled this have been posted for review at https://lists.gnu.org/archive/html/grub-devel/2020-02/msg00026.html

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Performance enhancement (non-breaking change which improves efficiency)
Code cleanup (non-breaking change which makes code smaller or more readable)
Breaking change (fix or feature that would cause existing functionality to change)
Documentation (a change to man pages or other documentation)

Checklist:

My code follows the ZFS on Linux code style requirements.
I have updated the documentation accordingly.
I have read the contributing document.
I have added tests to cover my changes.
I have run the ZFS Test Suite with this change applied.
All commit messages are properly formatted and contain Signed-off-by.

ikozhukhov · 2020-02-15T22:08:35Z

what is the reason do not reuse illumos logic for GRUB1 and loader?
what are bad with illumos logic (GRUB1 & loader) and you want to use another ?

codecov · 2020-02-17T01:37:25Z

Codecov Report

Merging #10009 into master will decrease coverage by <1%.
The diff coverage is 69%.

@@           Coverage Diff            @@
##           master   #10009    +/-   ##
========================================
- Coverage      79%      79%   -<1%     
========================================
  Files         386      386            
  Lines      122431   122533   +102     
========================================
+ Hits        97023    97066    +43     
- Misses      25408    25467    +59

Flag	Coverage Δ
#kernel	`79% <96%> (ø)`	⬆️
#user	`67% <5%> (ø)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ecbbdac...fafa8ff. Read the comment docs.

bghira · 2020-02-17T18:18:46Z

aiui Linux now supports multiboot2, why not just port the illumos / freebsd bootloader? grub is awfully encumbered code.

pcd1193182 · 2020-02-18T19:22:39Z

what is the reason do not reuse illumos logic for GRUB1 and loader?
what are bad with illumos logic (GRUB1 & loader) and you want to use another ?

First, Illumos is no longer using GRUB1, it is using the FreeBSD loader. The logic was written for the FreeBSD loader, rather than GRUB1.

We are not reusing that logic on Linux because GRUB and the FreeBSD loader have fundamentally different capabilities. The nextboot implementation in the FreeBSD loader is built directly into the code of the loader, and uses special fields in the header to count to the number of boots and store the fallback filesystem. GRUB, on the other hand, performs automated fallback through scripting, and stores the data for this directly in the grubenv file. As a result, it doesn't make sense to have special nextboot specific fields in the boot area for GRUB, since they will not be used or respected.

ikozhukhov · 2020-02-18T19:28:49Z

we are using GRUB1 on DilOS and will try to port GRUB2 soon. i'm asking about logic because i'd like to be more compatible with ZoL as possible on DilOS. current GRUB1 implementation still good, but has issues with large list of BE what can be using by BEADM.
i'll be interested in changes on GRUB side for support of your new scheme, because current scheme works as well to me on DilOS.

pcd1193182 · 2020-02-18T19:34:26Z

@misterbigstuff I welcome others to take on that work, but we are currently most comfortable tweaking a well-established bootloader compared to doing the work of porting and testing one.

bghira · 2020-02-18T21:49:37Z

@pcd1193182 so we can assume this means grub2 will be getting native encryption and large dnode support?

pcd1193182 · 2020-02-19T21:59:49Z

@misterbigstuff Not sure if anyone is taking that on right now, but I would expect it to happen eventually. I'm sure there is demand for native encryption on the bootfs out there.

behlendorf

A couple quick comments. I haven't yet had a chance to look at the grub portion of this.

include/sys/fs/zfs.h

include/sys/vdev_impl.h

lib/libzfs/libzfs_pool.c

module/zfs/zfs_ioctl.c

codecov-io · 2020-03-12T02:52:18Z

Codecov Report

Merging #10009 into master will decrease coverage by 0.03%.
The diff coverage is 66.08%.

@@            Coverage Diff             @@
##           master   #10009      +/-   ##
==========================================
- Coverage   79.52%   79.48%   -0.04%     
==========================================
  Files         389      389              
  Lines      123120   123229     +109     
==========================================
+ Hits        97906    97952      +46     
- Misses      25214    25277      +63

Flag	Coverage Δ
#kernel	`79.95% <94.93%> (+0.04%)`	⬆️
#user	`65.42% <5.10%> (-0.60%)`	⬇️

Impacted Files	Coverage Δ
cmd/zinject/translate.c	`51.79% <0.00%> (ø)`
lib/libzfs_core/libzfs_core.c	`84.73% <0.00%> (-1.39%)`	⬇️
module/zfs/vdev.c	`90.26% <ø> (+0.30%)`	⬆️
lib/libzfs/libzfs_pool.c	`73.29% <4.54%> (-0.82%)`	⬇️
module/zfs/vdev_label.c	`93.85% <86.56%> (-0.37%)`	⬇️
module/zfs/zfs_ioctl.c	`86.40% <100.00%> (+0.12%)`	⬆️
cmd/zdb/zdb_il.c	`30.86% <0.00%> (-24.08%)`	⬇️
cmd/zvol_id/zvol_id_main.c	`76.31% <0.00%> (-5.27%)`	⬇️
module/zfs/space_map.c	`93.31% <0.00%> (-4.96%)`	⬇️
module/zfs/spa_checkpoint.c	`93.78% <0.00%> (-4.35%)`	⬇️
... and 60 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6ed4391...3a06800. Read the comment docs.

include/sys/vdev_impl.h

lib/libzfs/libzfs_pool.c

module/zfs/vdev_label.c

module/zfs/zfs_ioctl.c

Signed-off-by: Paul Dagnelie <pcd@delphix.com>

module/zfs/vdev_label.c

module/zfs/zfs_ioctl.c

Signed-off-by: Paul Dagnelie <pcd@delphix.com>

Modern bootloaders leverage data stored in the root filesystem to enable some of their powerful features. GRUB specifically has a grubenv file which can store large amounts of configuration data that can be read and written at boot time and during normal operation. This allows sysadmins to configure useful features like automated failover after failed boot attempts. Unfortunately, due to the Copy-on-Write nature of ZFS, the standard behavior of these tools cannot handle writing to ZFS files safely at boot time. We need an alternative way to store data that allows the bootloader to make changes to the data. This work is very similar to work that was done on Illumos to enable similar functionality in the FreeBSD bootloader. This patch is different in that the data being stored is a raw grubenv file; this file can store arbitrary variables and values, and the scripting provided by grub is powerful enough that special structures are not required to implement advanced behavior. We repurpose the second padding area in each label to store the grubenv file, protected by an embedded checksum. We add two ioctls to get and set this data, and libzfs_core and libzfs functions to access them more easily. There are no direct command line interfaces to these functions; these will be added directly to the bootloader utilities. Reviewed-by: Pavel Zakharov <pavel.zakharov@delphix.com> Reviewed-by: Matthew Ahrens <mahrens@delphix.com> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Paul Dagnelie <pcd@delphix.com> Closes openzfs#10009

pcd1193182 requested review from ahrens and sebroy February 15, 2020 21:52

pcd1193182 added the Status: Code Review Needed Ready for review and testing label Feb 15, 2020

behlendorf reviewed Feb 19, 2020

View reviewed changes

include/sys/fs/zfs.h Show resolved Hide resolved

include/sys/vdev_impl.h Outdated Show resolved Hide resolved

include/sys/vdev_impl.h Show resolved Hide resolved

lib/libzfs/libzfs_pool.c Outdated Show resolved Hide resolved

module/zfs/zfs_ioctl.c Outdated Show resolved Hide resolved

pcd1193182 force-pushed the vdev_boot branch from 31c9058 to 6eca10b Compare March 11, 2020 19:14

behlendorf self-requested a review March 12, 2020 17:57

pcd1193182 mentioned this pull request Apr 13, 2020

Ubuntu version delphix/grub2#1

Merged

pzakha reviewed Apr 14, 2020

View reviewed changes

include/sys/vdev_impl.h Outdated Show resolved Hide resolved

behlendorf reviewed May 1, 2020

View reviewed changes

pcd1193182 added 6 commits May 1, 2020 11:19

Add support for boot environment data to be stored in the label

47202b3

Signed-off-by: Paul Dagnelie <pcd@delphix.com>

brian's feedback

49c5c00

Signed-off-by: Paul Dagnelie <pcd@delphix.com>

fix comment

e973eb8

Fix offset past end of file

dc3479e

vbe_vers updates

1eec472

Signed-off-by: Paul Dagnelie <pcd@delphix.com>

brian feedback

b19ec58

Signed-off-by: Paul Dagnelie <pcd@delphix.com>

pcd1193182 force-pushed the vdev_boot branch from 6eca10b to b19ec58 Compare May 1, 2020 18:21

fix freebsd build

4b45e00

Signed-off-by: Paul Dagnelie <pcd@delphix.com>

behlendorf approved these changes May 1, 2020

View reviewed changes

ahrens reviewed May 6, 2020

View reviewed changes

mahrens feedback

3a06800

Signed-off-by: Paul Dagnelie <pcd@delphix.com>

pcd1193182 force-pushed the vdev_boot branch from f57fbd4 to 3a06800 Compare May 6, 2020 22:52

ahrens approved these changes May 6, 2020

View reviewed changes

behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels May 7, 2020

behlendorf merged commit 108a454 into openzfs:master May 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for boot environment data to be stored in the label #10009

Add support for boot environment data to be stored in the label #10009

pcd1193182 commented Feb 15, 2020 •

edited

Loading

ikozhukhov commented Feb 15, 2020

codecov bot commented Feb 17, 2020 •

edited

Loading

bghira commented Feb 17, 2020

pcd1193182 commented Feb 18, 2020

ikozhukhov commented Feb 18, 2020

pcd1193182 commented Feb 18, 2020

bghira commented Feb 18, 2020

pcd1193182 commented Feb 19, 2020

behlendorf left a comment

codecov-io commented Mar 12, 2020 •

edited

Loading

Add support for boot environment data to be stored in the label #10009

Add support for boot environment data to be stored in the label #10009

Conversation

pcd1193182 commented Feb 15, 2020 • edited Loading

Motivation and Context

Description

How Has This Been Tested?

Types of changes

Checklist:

ikozhukhov commented Feb 15, 2020

codecov bot commented Feb 17, 2020 • edited Loading

Codecov Report

bghira commented Feb 17, 2020

pcd1193182 commented Feb 18, 2020

ikozhukhov commented Feb 18, 2020

pcd1193182 commented Feb 18, 2020

bghira commented Feb 18, 2020

pcd1193182 commented Feb 19, 2020

behlendorf left a comment

Choose a reason for hiding this comment

codecov-io commented Mar 12, 2020 • edited Loading

Codecov Report

pcd1193182 commented Feb 15, 2020 •

edited

Loading

codecov bot commented Feb 17, 2020 •

edited

Loading

codecov-io commented Mar 12, 2020 •

edited

Loading