Attempted fix for regression with LUKS devices introduced by PR#23218 by mwilck · Pull Request #23489 · systemd/systemd

mwilck · 2022-05-24T09:37:34Z

On switching root, a device may have a persistent databse. In that case,
Device.enumerated_found may have DEVICE_FOUND_UDEV flag, and it is not
necessary to downgrade the Device.deserialized_found and
Device.deserialized_state. Otherwise, the state of the device unit may
be changed plugged -> dead -> plugged, if the device has not been mounted.

This is #23437, just reduced to a minimal bug fix.
Fixes #23429 (hopefully)

fbuihuu · 2022-05-24T10:54:27Z

I don't see the point of this PR. We already have #23437 and PR #23436, this one is adding only more confusion.

Again there is no rush so let's discuss the regression in on of the aforementioned PRs.

bluca · 2022-05-24T10:55:38Z

well, if there's a regression slipped in v251, we need to fix it with some urgency

fbuihuu · 2022-05-24T10:58:38Z

As I wrote we already have 2 PRs. If we need a quick fix, working on #23437 should be enough I think. And "Fixes #23429 (hopefully)" doesn't make me confident at all.

See comments in #23429 where this wasn't supposed to be a regression and wasn't supposed to introduce any issue.

fbuihuu · 2022-05-24T11:00:09Z

I think we should try to reproduce the prob instead based on the description given in #23429 (comment)

mwilck · 2022-05-24T12:42:36Z

As I wrote we already have 2 PRs. If we need a quick fix, working on #23437 should be enough I think. And "Fixes #23429 (hopefully)" doesn't make me confident at all.

I proposed a minimal fix. That's my understanding how a regression should be addressed. Both other PRs contain additional unrelated changes. I wrote "hopefully" because the fix hadn't been verified, and I didn't want to boast of something I wasn't 100% certain of.

See comments in #23429 where this wasn't supposed to be a regression and wasn't supposed to introduce any issue.

I'm unsure what you mean. I don't deny that some of my statements were wrong. But I don't think I'm the only one. At least I have tried to analyze the issues in depth. I haven't realized the LUKS issue. But nobody else did, either.

fbuihuu · 2022-05-24T13:44:18Z

Sorry I wasn't explained myself well enough. We all made some mistakes because it's hard to foresee the consequences of any changes in this part of the code even if we put a lot of thoughts. But I think my suggestion to reproduce the regression first and then verify that the fix actually is fixing it is reasonable instead of running for a fix that we're not again 100% sure.

#23437 is already an attempt to fix this problem and has the same change that this PR is introducing. Indeed it has 2 small additional commits but those should be considered right now anyway as they're likely to be committed soon or later. We would avoid testing this stuff twice.

I'm re-opening this PR and let @yuwata decides if he prefers dealing with this PR rather than his.

mwilck · 2022-05-24T14:53:31Z

My thinking was that if this PR actually fixes the issue(s) reported, it might be easier to merge this minimal PR than #23437. But I'm not familiar with how this is handled in systemd. If #23437 can be merged, I have no objections. Most importantly, we need tests by the affected users. @tanriol, as a Gentoo user, may be able to test this rather easily. @keszybz seems to have ways to distribute fixes to Fedora users quickly, too. That should cover most regressions reported so far.

yuwata · 2022-05-24T19:49:52Z

Sorry for late response. Will take a look.

tanriol · 2022-05-24T20:40:29Z

Sorry, applying this patch on top of systemd 251 did not fix the issue on my system.

mwilck · 2022-05-25T13:54:27Z

@tanriol, I've pushed another patch which has fixed the issue for me (I was able to reproduce your problem). I'd be very grateful if you could apply 0543ebd on top and test again.

bluca · 2022-05-25T14:09:13Z

If you can reproduce it, can you add a test case too please to TEST-24-CRYPTSETUP? Otherwise we'll never know if another regression happens

mwilck · 2022-05-25T14:34:45Z

If you can reproduce it, can you add a test case too please to TEST-24-CRYPTSETUP?

What I meant is I was able to set up a box with LUKS encrypted root, saw the same issue, and was able to fix it. I don't know how to transform this into a unit test. I can have a look later, but I know almost nothing about the systemd unit tests, except for the udev test which I've been working on in the past.

dustymabe · 2022-05-25T15:50:28Z

If we can get the rpm-build:fedora-rawhide-x86_64 action to succeed I can take the resulting RPM and run it through our Fedora CoreOS test suite.

This should cover cases regarding devices with `OPTIONS+="db_persist"` during initrd->sysroot transition. See: * systemd#23429 * systemd#23218 * systemd#23489 * https://bugzilla.redhat.com/show_bug.cgi?id=2087225

mrc0mmand · 2022-05-25T16:54:45Z

If we can get the rpm-build:fedora-rawhide-x86_64 action to succeed I can take the resulting RPM and run it through our Fedora CoreOS test suite.

I've already pinged @keszybz, since we might need to revert https://src.fedoraproject.org/rpms/systemd/c/9a48377e0adb6af26f8e7a89dd0c186cc951efa8?branch=rawhide once again, as the builders are still having some issues.

mrc0mmand · 2022-05-25T16:56:08Z

If you can reproduce it, can you add a test case too please to TEST-24-CRYPTSETUP?

What I meant is I was able to set up a box with LUKS encrypted root, saw the same issue, and was able to fix it. I don't know how to transform this into a unit test. I can have a look later, but I know almost nothing about the systemd unit tests, except for the udev test which I've been working on in the past.

I've opened #23517 which should add some coverage for this. Feel free to cherry-pick the commit into this branch, as it should make it easier to test the changes with the test included.

On switching root, a device may have a persistent databse. In that case, Device.enumerated_found may have DEVICE_FOUND_UDEV flag, and it is not necessary to downgrade the Device.deserialized_found and Device.deserialized_state. Otherwise, the state of the device unit may be changed plugged -> dead -> plugged, if the device has not been mounted. Fixes systemd#23429. [mwilck: cherry-picked from systemd#23437]

dm-crypt device units generated by systemd-cryptsetup-generator habe BindsTo= dependencies on their backend devices. The dm-crypt devices have the db_persist flag set, and thus survive the udev db cleanup while switching root. But backend devices usually don't survive. These devices are neither mounted nor used for swap, thus they will seen as DEVICE_NOT_FOUND after switching root. The BindsTo dependency will cause systemd to schedule a stop job for the dm-crypt device, breaking boot: [ 68.929457] krypton systemd[1]: systemd-cryptsetup@cr_root.service: Unit is stopped because bound to inactive unit dev-disk-by\x2duuid-3bf91f73\x2d1ee8\x2d4cfc\x2d9048\x2d93ba349b786d.device. [ 68.945660] krypton systemd[1]: systemd-cryptsetup@cr_root.service: Trying to enqueue job systemd-cryptsetup@cr_root.service/stop/replace [ 69.473459] krypton systemd[1]: systemd-cryptsetup@cr_root.service: Installed new job systemd-cryptsetup@cr_root.service/stop as 343 Avoid this by not setting the state of the backend devices to DEVICE_DEAD. Fixes the LUKS setup issue reported in systemd#23429.

This should cover cases regarding devices with `OPTIONS+="db_persist"` during initrd->sysroot transition. See: * systemd#23429 * systemd#23218 * systemd#23489 * https://bugzilla.redhat.com/show_bug.cgi?id=2087225

yuwata · 2022-05-25T23:52:45Z

@mwilck Thank you for the analysis of the issue. LGTM. I cherry-picked @mrc0mmand's commit about test update.

tanriol · 2022-05-25T23:53:17Z

Will test this tomorrow, sorry for delay.

tanriol · 2022-05-26T12:18:35Z

systemd-251 with 2659b04 and 0543ebd on top works for me. Do I need to retest on newer commits in this PR?

yuwata · 2022-05-26T12:21:48Z

systemd-251 with 2659b04 and 0543ebd on top works for me. Do I need to retest on newer commits in this PR?

Thanks. I think retest is not necessary, as they are just rebased.

yuwata · 2022-05-26T18:25:54Z

The two main commits are in PR #23517. Closing.

This should cover cases regarding devices with `OPTIONS+="db_persist"` during initrd->sysroot transition. See: * systemd/systemd#23429 * systemd/systemd#23218 * systemd/systemd#23489 * https://bugzilla.redhat.com/show_bug.cgi?id=2087225 (cherry picked from commit 1fb7f8e)

mwilck mentioned this pull request May 24, 2022

PR#23218 introduced a regression for devices with db_persist option #23429

Closed

fbuihuu closed this May 24, 2022

fbuihuu reopened this May 24, 2022

yuwata added the pid1 label May 24, 2022

yuwata mentioned this pull request May 25, 2022

core/device: honor deserialized state and check device existence on switching-root #23507

Closed

mwilck force-pushed the fix-23218 branch from 82c7ef5 to 0543ebd Compare May 25, 2022 13:52

mrc0mmand mentioned this pull request May 25, 2022

test: cover initrd->sysroot transition in TEST-24 #23517

Merged

yuwata and others added 3 commits May 26, 2022 08:39

test: cover initrd->sysroot transition in TEST-24

78368a0

This should cover cases regarding devices with `OPTIONS+="db_persist"` during initrd->sysroot transition. See: * systemd#23429 * systemd#23218 * systemd#23489 * https://bugzilla.redhat.com/show_bug.cgi?id=2087225

yuwata force-pushed the fix-23218 branch from 0543ebd to 78368a0 Compare May 25, 2022 23:49

yuwata added the good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed label May 25, 2022

[WIP] test: generate initrd supporting dm-crypt for TEST-24

1be7bcc

yuwata removed the good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed label May 26, 2022

yuwata mentioned this pull request May 26, 2022

Please include sd-encrypt hook into initrd for arch systemd/systemd-centos-ci#490

Closed

yuwata closed this May 26, 2022

Uh oh!

Conversation

mwilck commented May 24, 2022

Uh oh!

fbuihuu commented May 24, 2022

Uh oh!

bluca commented May 24, 2022

Uh oh!

fbuihuu commented May 24, 2022

Uh oh!

fbuihuu commented May 24, 2022

Uh oh!

mwilck commented May 24, 2022

Uh oh!

fbuihuu commented May 24, 2022

Uh oh!

mwilck commented May 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yuwata commented May 24, 2022

Uh oh!

tanriol commented May 24, 2022

Uh oh!

mwilck commented May 25, 2022

Uh oh!

bluca commented May 25, 2022

Uh oh!

mwilck commented May 25, 2022

Uh oh!

dustymabe commented May 25, 2022

Uh oh!

mrc0mmand commented May 25, 2022

Uh oh!

mrc0mmand commented May 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yuwata commented May 25, 2022

Uh oh!

tanriol commented May 25, 2022

Uh oh!

tanriol commented May 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yuwata commented May 26, 2022

Uh oh!

yuwata commented May 26, 2022

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

7 participants

mwilck commented May 24, 2022 •

edited

Loading

mrc0mmand commented May 25, 2022 •

edited

Loading

tanriol commented May 26, 2022 •

edited

Loading