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

dracut/zfs-load-key.sh: properly remove prefixes #9520

Merged
merged 1 commit into from
Oct 30, 2019

Conversation

alaviss
Copy link
Contributor

@alaviss alaviss commented Oct 28, 2019

Motivation and Context

If the root is passed to the kernel like this: root=zfs:zroot/sys/def, then on boot, you'll receive a message like this:

 cannot open pool 'zfs:zroot'

Then the encryption key prompt will not show up, making the system unbootable (if you use encryption).

How Has This Been Tested?

Booted my machine with it.

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:

Removes the 'ZFS=' prefix from $BOOTFS instead of $root. This makes sure
that the 'zfs:' prefix remains stripped so that users with
'root=zfs:dataset' cmdline can have key loaded on boot again.

Signed-off-by: Hiếu Lê <leorize+oss@disroot.org>
@codecov
Copy link

codecov bot commented Oct 28, 2019

Codecov Report

Merging #9520 into master will decrease coverage by 0.08%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9520      +/-   ##
==========================================
- Coverage   79.11%   79.03%   -0.09%     
==========================================
  Files         416      416              
  Lines      123652   123652              
==========================================
- Hits        97831    97728     -103     
- Misses      25821    25924     +103
Flag Coverage Δ
#kernel 79.69% <ø> (-0.01%) ⬇️
#user 66.55% <ø> (-0.34%) ⬇️

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 e357046...b0aad98. Read the comment docs.

@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label Oct 28, 2019
@behlendorf
Copy link
Contributor

@dacianstremtan @ghfields would you mind reviewing this proposed fix.

@dacianstremtan
Copy link
Contributor

dacianstremtan commented Oct 30, 2019

I am unable to replicate the problem.
I'm running Centos 7.7 with encryption enabled at the pool level and I get the prompt all the time and successfully decrypts the pool with kernel option root=zfs:rpool/ROOT/centos7 in my case.

EDIT. Finally I was able to duplicate the issue on CentOS 7.7 On my system the zfs-load-key.sh early terminated because zfs-import-cache.service was failing. The pool was imported by the zfs-mount.sh script afterwards.

Copy link
Contributor

@dacianstremtan dacianstremtan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The suggested line fixes the issue, but I would suggest a different approach:

BOOTFS="${root##ZFS=}" || BOOTFS="${root##zfs:}"

It will chose one of them based on the the root=zfs:rpool/dataset or root=ZFS=rpool/dataset, also easier to read the code.

@ghfields
Copy link
Contributor

@dacianstremtan and I worked side-by-side on this one. His suggested modification processes both instances using the same format and builds in a framework for any future checks to be cleanly added.

@alaviss
Copy link
Contributor Author

alaviss commented Oct 30, 2019

The suggested line fixes the issue, but I would suggest a different approach:

BOOTFS="${root##ZFS=}" || BOOTFS="${root##zfs:}"

It will chose one of them based on the the root=zfs:rpool/dataset or root=ZFS=rpool/dataset, also easier to read the code.

I'm rather certain that parameter expansion doesn't work like this...

@behlendorf
Copy link
Contributor

Thanks everyone for the feedback. Let's go with the change as proposed.

@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Oct 30, 2019
@behlendorf behlendorf merged commit 936e2d6 into openzfs:master Oct 30, 2019
@alaviss alaviss deleted the dracut-enc-fix branch November 1, 2019 14:47
cvoltz pushed a commit to cvoltz/zfs that referenced this pull request Nov 1, 2019
Removes the 'ZFS=' prefix from $BOOTFS instead of $root. This makes sure
that the 'zfs:' prefix remains stripped so that users with
'root=zfs:dataset' cmdline can have key loaded on boot again.

Reviewed-by: Garrett Fields <ghfields@gmail.com>
Reviewed-by: Dacian Reece-Stremtan <dacianstremtan@gmail.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Hiếu Lê <leorize+oss@disroot.org>
Closes openzfs#9520
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Dec 26, 2019
Removes the 'ZFS=' prefix from $BOOTFS instead of $root. This makes sure
that the 'zfs:' prefix remains stripped so that users with
'root=zfs:dataset' cmdline can have key loaded on boot again.

Reviewed-by: Garrett Fields <ghfields@gmail.com>
Reviewed-by: Dacian Reece-Stremtan <dacianstremtan@gmail.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Hiếu Lê <leorize+oss@disroot.org>
Closes openzfs#9520
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Dec 27, 2019
Removes the 'ZFS=' prefix from $BOOTFS instead of $root. This makes sure
that the 'zfs:' prefix remains stripped so that users with
'root=zfs:dataset' cmdline can have key loaded on boot again.

Reviewed-by: Garrett Fields <ghfields@gmail.com>
Reviewed-by: Dacian Reece-Stremtan <dacianstremtan@gmail.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Hiếu Lê <leorize+oss@disroot.org>
Closes openzfs#9520
tonyhutter pushed a commit that referenced this pull request Jan 23, 2020
Removes the 'ZFS=' prefix from $BOOTFS instead of $root. This makes sure
that the 'zfs:' prefix remains stripped so that users with
'root=zfs:dataset' cmdline can have key loaded on boot again.

Reviewed-by: Garrett Fields <ghfields@gmail.com>
Reviewed-by: Dacian Reece-Stremtan <dacianstremtan@gmail.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Hiếu Lê <leorize+oss@disroot.org>
Closes #9520
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants