-
Notifications
You must be signed in to change notification settings - Fork 46
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
Full Discoverable Partitions Specification compatibility (WIP) #98
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea of implementing discoverable partitions great. The code looks clean, good work! I left a few comments about this implementation.
It needs more tests and I can help you with it if needed (setting up integration tests is tricky currently).
@@ -619,6 +622,15 @@ func boost() error { | |||
return err | |||
} | |||
|
|||
// Mount efivarfs if running in EFI mode | |||
if _, err := os.Stat("/sys/firmware/efi"); errors.Is(err, os.ErrNotExist) { | |||
if err := mount("efivarfs", "/sys/firmware/efi/efivars", "efivarfs", unix.MS_NOSUID|unix.MS_NOEXEC|unix.MS_NODEV, ""); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to unmount it as well before switching to the user's root fs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where should I unmount (function)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SeemoveMountpointsToHost()
function. First it tries to move the mountpoint to host. If it does not work (e.g. new root does not contain the directory) then it unmounts the filesystem from initram fs tree.
The moveMountpointsToHost()
should handle /sys/firmware/efi
mountpoint as well. I wonder if MS_MOVE
and MNT_DETACH
handle mountpoints recursively.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been reading through the documentation and I think that moving the mountpoint isn't needed at all. It looks like systemd handles all the mountpoints but /run
. Take a look at the specification, it may even be the case that the current code on moveMountpointsToHost()
is mostly unneeded.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then I'll try to get unit tests ready today and see if it works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ran your PR with VoidLinux and ArchLinux+btrfs tests and they passed.
I don't really know how testing works with this project, so help would be really appreciated. |
The integration test are located at
If you can take care of 1 then I'll add QEMU tests for it. See As of formatting a partition with LUKS check |
Hey I just want to check with you how it is going. Do you need any help with testing? |
I'm sorry for the inactivity, last week I started university and I've been really busy since then. Now that I have some free days I'll hopefully be able to continue working on this. I'll let you know if I have any problems with the testing setup. |
init/main.go
Outdated
@@ -619,6 +622,15 @@ func boost() error { | |||
return err | |||
} | |||
|
|||
// Mount efivarfs if running in EFI mode | |||
if _, err := os.Stat("/sys/firmware/efi"); errors.Is(err, os.ErrNotExist) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe it should be !errors.Is(err, os.ErrNotExist)
. if
body should be executed if the efi dir not-not exists (i.e. exists).
I pulled the latest changes from this PR and rebased on top of |
I tried to ran a test that uses autodiscovery (e.g. I enable QEMU EFI mode with |
The EFI var is needed to know which physical disk contains the auto-detected system. I think this is done to avoid loading the kernel from a LiveUSB and the root of a locally installed system (and other probably breaking situations). Knowing this, I'm not sure if it would be a good idea to go ahead and autodetect on all drives, unless there's another way to detect where the kernel was loaded from. Maybe we could still autodetect if there's only one disk that follows the specification or only one of them contains partitions for the system architecture, but in any other case, I think that falling back to the emergency shell would be the most sensible option. Let me know what you think. |
return &deviceRef{refName, calculateDevName(devPath, p.num)} | ||
case refEspUUID: | ||
if bytes.Equal(d.data.(UUID), p.uuid) { | ||
return d.autodetectRoot(devPath, t) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The autodetectRoot() function does not accept a partition but instead gets whole table. For me it looks like the
autodetectRoot()` should be split into 2 parts
- calculate
rootTypeGuid
- move outside of the loop if bytes.Equal(rootTypeGuid, p.typeGUID) {
- should be called inside the loop
udev events might be too noisy. Move it to a separate kernel option `booster.debug.udev` to make the main booster debug logs more readable. Closes anatol#99
if enableAutodetect && efiVarsAvailable { | ||
debug("%s= param is not specified. Use GPT partition autodiscovery.", name) | ||
|
||
_, data, err := attributes.ReadEfivarsWithGuid("LoaderDevicePartUUID", *util.StringToGUID("4a67b082-0a4c-41cf-b6c7-440b29bb8c4f")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where does LoaderDevicePartUUID
requirement come from? I checked https://www.freedesktop.org/wiki/Specifications/DiscoverablePartitionsSpec/ and do not see any mention of this efi var.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, found it here the first partition with this type UUID on the disk containing the active EFI ESP is automatically mounted to the root directory /
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So basically we need to find a disk that contains the active EFI ESP. Is reading LoaderDevicePartUUID
efivar and then iterating all GPT partitions the only way to do it?
@rdvdev2 how do you feel if i pick this PR and complete it? I would like to do some refactoring of booster code along the way. I'll keep you as an author of this change. |
Go ahead, I think I don't have enough spare time to work on this anymore. Thanks for understanding! |
@rdvdev2 I reworked your patches, made extra changes to booster codebase and added support for GPT partition attributes. I pushed the changes to |
Per the spec https://systemd.io/DISCOVERABLE_PARTITIONS/ only disks with active ESP need to be checked for discoverable partitions. To find out what is active ESP we need to read efi variables at the boot time. Then we skip all GPT tables that do not have this ESP. This change has been started by Roger Díaz Viñolas as a part of PR #98 and later reworked by Anatol Pomozov.
Per the spec https://systemd.io/DISCOVERABLE_PARTITIONS/ only disks with active ESP need to be checked for discoverable partitions. To find out what is active ESP we need to read efi variables at the boot time. Then we skip all GPT tables that do not have this ESP. This change has been started by Roger Díaz Viñolas as a part of PR #98 and later reworked by Anatol Pomozov.
Per the spec https://systemd.io/DISCOVERABLE_PARTITIONS/ only disks with active ESP need to be checked for discoverable partitions. To find out what is active ESP we need to read efi variables at the boot time. Then we skip all GPT tables that do not have this ESP. This change has been started by Roger Díaz Viñolas as a part of PR #98 and later reworked by Anatol Pomozov.
Per the spec https://systemd.io/DISCOVERABLE_PARTITIONS/ only disks with active ESP need to be checked for discoverable partitions. To find out what is active ESP we need to read efi variables at the boot time. Then we skip all GPT tables that do not have this ESP. This change has been started by Roger Díaz Viñolas as a part of PR #98 and later reworked by Anatol Pomozov.
The Discoverable Partitions support has been merged Thank you for your work! |
Thank you too for your help (and sorry for not being able to finish this myself) |
The current booster Discoverable Partitions Specification support is pretty basic and doesn't work for every case. This PR aims to fully support the specification, allowing to auto-discover encrypted partitions or mount root read-only, among other features.
WARNING: This PR is really a WIP, and not properly tested. At the moment, this PR is only provided to get feedback while developing the feature(s).
Progress: