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.

Sign up for GitHub

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

Mount generator fixes #10477

Closed
wants to merge 3 commits into from
Closed

Mount generator fixes #10477

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e80ded6
Ensure mount unit pilots when its ZFS key is loaded
jibel Jun 18, 2020
bfafd5c
BindsTo dataset keyload unit to mount associate unit
jibel Jun 18, 2020
365c7d4
Make unloading the key more robust
jibel Jun 18, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 11 additions & 3 deletions etc/systemd/system-generators/zfs-mount-generator.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,7 @@ process_line() {
wants="zfs-import.target"
requires=""
requiredmounts=""
bindsto=""
wantedby=""
requiredby=""
noauto="off"
Expand Down Expand Up @@ -172,6 +173,12 @@ set -eu;\
keystatus=\"\$\$(@sbindir@/zfs get -H -o value keystatus \"${dataset}\")\";\
[ \"\$\$keystatus\" = \"unavailable\" ] || exit 0;\
${keyloadscript}'"
keyunloadcmd="\
/bin/sh -c '\
set -eu;\
keystatus=\"\$\$(@sbindir@/zfs get -H -o value keystatus \"${dataset}\")\";\
[ \"\$\$keystatus\" = \"available\" ] || exit 0;\
@sbindir@/zfs unload-key \"${dataset}\"'"



Expand All @@ -191,19 +198,19 @@ Documentation=man:zfs-mount-generator(8)
DefaultDependencies=no
Wants=${wants}
After=${after}
Before=${before}
Copy link
Contributor

Choose a reason for hiding this comment

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

If we do this, should we also drop Wants=, After=, and Requires=?

The key load may apply to multiple encryption roots, right? So it's inappropriate for any of the particular mount x-systemd.* options to apply. So my vote is to just drop all of these.

Caveat: I just woke up. So read my arguments very critically.

Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW we had some discussions about this way back when: #9649 (comment) (and at least the following comment by @rlaager)

Copy link
Member

Choose a reason for hiding this comment

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

@didrocks What is the effect of removing this Before= and why did you make that change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@didrocks What is the effect of removing this Before= and why did you make that change?
This was done in this commit (e80ded6). To expand a little bit more, the idea is to delay the home user mount once the keyfile is accessible. The keyfile can be made accessible way later than zfs-mount.service (which is what is in the before= here), like via a pam module on login for instance, but can be also be made accessible during the boot process (single user, plymouth asking for passphrase) or so on.

Basically, the idea is to not necessary link those units to zfs-mount.service, which will try to mount it, then, by dependency chain, try loading the key-load unit which will fail at this point, creating spam in logs.

${requires}
${keymountdep}

[Service]
Type=oneshot
RemainAfterExit=yes
ExecStart=${keyloadcmd}
ExecStop=@sbindir@/zfs unload-key '${dataset}'" > "${dest_norm}/${keyloadunit}"
ExecStop=${keyunloadcmd}" > "${dest_norm}/${keyloadunit}"
fi
# Update the dependencies for the mount file to want the
# key-loading unit.
wants="${wants} ${keyloadunit}"
wants="${wants}"
bindsto="BindsTo=${keyloadunit}"
after="${after} ${keyloadunit}"
fi

Expand Down Expand Up @@ -414,6 +421,7 @@ Documentation=man:zfs-mount-generator(8)
Before=${before}
After=${after}
Wants=${wants}
${bindsto}
${requires}
${requiredmounts}

Expand Down