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

99-systemd.rules: rework SYSTEMD_READY logic for device mapper (v252) #413

Open
wants to merge 1 commit into
base: v252-stable
Choose a base branch
from

Conversation

mwilck
Copy link
Contributor

@mwilck mwilck commented May 28, 2024

Device mapper devices are set up in multiple steps. The first step, which
generates the initial "add" event, only creates an empty container, which is
useless for higher layers. SYSTEMD_READY should be set to 0 on this event to
avoid premature device activation.

The event that matters is the "activation" event: the first "change" event on
which DM_UDEV_DISABLE_OTHER_RULES_FLAG=1 is not set. When this event arrives,
the device is ready for being scanned by blkid and similar tools, and for being
activated by systemd.

Intermittent events with DM_UDEV_DISABLE_OTHER_RULES_FLAG=1 should be ignored
as far as systemd or higher-level block layers are concerned. Previous device
properties and symlinks should be preserved: the device shouldn't be scanned or
activated, but shouldn't be deactivated, either. In particular, SYSTEM_READY
shouldn't be set to 0 if it wasn't set before, because that might cause mounted
file systems to be unmounted. Such intermittent events may occur any time,
before or after the "activation" event.

DM_UDEV_DISABLE_OTHER_RULES_FLAG=1 can have multiple reasons. One possible reason
is that the device is suspended. There are other reasons that depend on the
device-mapper subsystem (LVM, multipath, dm-crypt, etc.).

The current systemd rule set

  1. sets SYSTEMD_READY=0 if DM_UDEV_DISABLE_OTHER_RULES_FLAG is set in "add"
    events;
  2. imports SYSTEMD_READY from the udev db if DM_SUSPENDED is set, and jumps to systemd_end;
  3. sets SYSTEMD_READY=1, otherwise.

This logic has several flaws:

    1. can cause file systems to be unmounted if an coldplug event arrives while
      a file system is suspended. This rule shouldn't be applied for coldplug events
      or in general, "synthetic" add events;
    1. evaluates DM_SUSPENDED=1, which is a device-mapper internal property.
      It's wrong to infer that a device is accessible if DM_SUSPENDED=0.
      The jump to systemd_end may cause properties and/or symlinks to be lost;
    1. is superfluous, because SYSTEMD_READY=1 is equivalent with SYSTEMD_READY
      being unset, and can create the wrong impression that the device was explicitly
      activated.

This patch fixes the logic as follows:

  • apply 1) only if DM_NAME is empty, which is only the case for the first
    "genuine add" event;
  • change 2) to use DM_UDEV_DISABLE_OTHER_RULES_FLAG instead of DM_SUSPENDED,
    and remove the GOTO directive;
  • remove 3).

Fixes: b7cf1b6 ("udev: use SYSTEMD_READY to mask uninitialized DM devices")
Fixes: 35a6750 ("rules: set SYSTEMD_READY=0 on DM_UDEV_DISABLE_OTHER_RULES_FLAG=1 only with ADD event (#2747)")

Signed-off-by: Martin Wilck [email protected]
(cherry picked from commit c072860)

Device mapper devices are set up in multiple steps. The first step, which
generates the initial "add" event, only creates an empty container, which is
useless for higher layers. SYSTEMD_READY should be set to 0 on this event to
avoid premature device activation.

The event that matters is the "activation" event: the first "change" event on
which DM_UDEV_DISABLE_OTHER_RULES_FLAG=1 is not set. When this event arrives,
the device is ready for being scanned by blkid and similar tools, and for being
activated by systemd.

Intermittent events with DM_UDEV_DISABLE_OTHER_RULES_FLAG=1 should be ignored
as far as systemd or higher-level block layers are concerned. Previous device
properties and symlinks should be preserved: the device shouldn't be scanned or
activated, but shouldn't be deactivated, either.  In particular, SYSTEM_READY
shouldn't be set to 0 if it wasn't set before, because that might cause mounted
file systems to be unmounted. Such intermittent events may occur any time,
before or after the "activation" event.

DM_UDEV_DISABLE_OTHER_RULES_FLAG=1 can have multiple reasons. One possible reason
is that the device is suspended. There are other reasons that depend on the
device-mapper subsystem (LVM, multipath, dm-crypt, etc.).

The current systemd rule set

1) sets SYSTEMD_READY=0 if DM_UDEV_DISABLE_OTHER_RULES_FLAG is set in "add"
events;
2) imports SYSTEMD_READY from the udev db if DM_SUSPENDED is set, and jumps to systemd_end;
3) sets SYSTEMD_READY=1, otherwise.

This logic has several flaws:

* 1) can cause file systems to be unmounted if an coldplug event arrives while
a file system is suspended. This rule shouldn't be applied for coldplug events
or in general, "synthetic" add events;
* 2) evaluates DM_SUSPENDED=1, which is a device-mapper internal property.
It's wrong to infer that a device is accessible if DM_SUSPENDED=0.
The jump to systemd_end may cause properties and/or symlinks to be lost;
* 3) is superfluous, because SYSTEMD_READY=1 is equivalent with SYSTEMD_READY
being unset, and can create the wrong impression that the device was explicitly
activated.

This patch fixes the logic as follows:

- apply 1) only if DM_NAME is empty, which is only the case for the first
"genuine add" event;
- change 2) to use DM_UDEV_DISABLE_OTHER_RULES_FLAG instead of DM_SUSPENDED,
and remove the GOTO directive;
- remove 3).

Fixes: b7cf1b6 ("udev: use SYSTEMD_READY to mask uninitialized DM devices")
Fixes: 35a6750 ("rules: set SYSTEMD_READY=0 on DM_UDEV_DISABLE_OTHER_RULES_FLAG=1 only with ADD event (#2747)")

Signed-off-by: Martin Wilck <[email protected]>
(cherry picked from commit c072860)
Copy link

We were not able to find or create Copr project packit/systemd-systemd-stable-413 specified in the config with the following error:

Cannot create a new Copr project (owner=packit project=systemd-systemd-stable-413 chroots=[]): chroots: '[]' is not a valid choice for this field.

Please check your configuration for:

  1. typos in owner and project name (groups need to be prefixed with @)
  2. whether the project name doesn't contain not allowed characters (only letters, digits, underscores, dashes and dots must be used)
  3. whether the project itself exists (Packit creates projects only in its own namespace)
  4. whether Packit is allowed to build in your Copr project
  5. whether your Copr project/group is not private

@mwilck mwilck changed the title 99-systemd.rules: rework SYSTEMD_READY logic for device mapper 99-systemd.rules: rework SYSTEMD_READY logic for device mapper (v252) May 28, 2024
@mwilck
Copy link
Contributor Author

mwilck commented May 28, 2024

Backport of systemd/systemd#31661.

There has been some confusion about the dependency between this change in the systemd udev rules and the related change in LVM2 2.03.24.

The systemd/udev change (this PR) is a fix for the logic with which systemd handles SYSTEMD_READY for dm devices. It is independent of the changes in the device-mapper udev rules in LVM 2.03.24, and can be applied even if the latter are not applied.

OTOH, updating to LVM 2.03.24 without applying the changes from this PR comes with a slight regression risk as explained in this comment, because the updated device-mapper rules don't set DM_SUSPENDED any more.

Therefore it's better and safer for everyone to backport this PR to the systemd-stable branches.

For information: @prajnoha, @bluca, @yuwata, @fbui, @bmarzins, @msekletar

@mwilck mwilck marked this pull request as ready for review May 28, 2024 09:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

1 participant