switch-root: use MS_REC for /run, unless we are soft-rebooting #28497

bluca · 2023-07-22T21:48:23Z

There are applications that rely on mounts under /run surviving the
switch from initrd to rootfs, so use MS_REC unless we are soft
rebooting.

FIxes #28452

src/shared/switch-root.c

yuwata · 2023-07-23T17:33:07Z

src/core/main.c

@@ -1884,7 +1884,8 @@ static int do_reexecute(
        if (switch_root_dir) {
                r = switch_root(/* new_root= */ switch_root_dir,
                                /* old_root_after= */ NULL,
-                                /* flags= */ objective == MANAGER_SWITCH_ROOT ? SWITCH_ROOT_DESTROY_OLD_ROOT : 0);
+                                /* flags= */ (objective == MANAGER_SWITCH_ROOT ? SWITCH_ROOT_DESTROY_OLD_ROOT : 0) |
+                                             (objective == MANAGER_SOFT_REBOOT ? SWITCH_ROOT_NON_RECURSIVE_RUN : 0));


We usually name things in positive, So, I'd like the following.

(objective == MANAGER_SWITCH_ROOT ? SWITCH_ROOT_DESTROY_OLD_ROOT | SWITCH_ROOT_RECURSIVE_RUN : 0)

that requires changing more places, so I'll leave it as-is

that requires changing more places, so I'll leave it as-is

More places? The enum is introduced by the commit. I do not think that's so many. Only three places in your patch.

switch_root() is used elsewhere too

I checked now, and it's used in exactly two places ;) So I think it'd be nicer to rework the flag to be positive. The definition of run_mount_flags below becomes much nicer with a positive option.

src/shared/switch-root.c

keszybz

This is pretty clear and should fix the issue. My vote in the naming bikeshed is to rename, but it's not terribly important. If you disagree, feel free to set back the flag back.

keszybz · 2023-07-24T09:20:02Z

src/core/main.c

@@ -1884,7 +1884,8 @@ static int do_reexecute(
        if (switch_root_dir) {
                r = switch_root(/* new_root= */ switch_root_dir,
                                /* old_root_after= */ NULL,
-                                /* flags= */ objective == MANAGER_SWITCH_ROOT ? SWITCH_ROOT_DESTROY_OLD_ROOT : 0);
+                                /* flags= */ (objective == MANAGER_SWITCH_ROOT ? SWITCH_ROOT_DESTROY_OLD_ROOT : 0) |
+                                             (objective == MANAGER_SOFT_REBOOT ? SWITCH_ROOT_NON_RECURSIVE_RUN : 0));


I checked now, and it's used in exactly two places ;) So I think it'd be nicer to rework the flag to be positive. The definition of run_mount_flags below becomes much nicer with a positive option.

src/shared/switch-root.c

There are applications that rely on mounts under /run surviving the switch from initrd to rootfs, so use MS_REC unless we are soft rebooting. Follow-up for 7c764d4 Fixes systemd#28452

Since 7c764d4 we bind mount certain directories during switch root instead of moving the mount directly, and for /run we do this without MS_REC. This, unfortunately, leaves all mounts under /run behind in the old root, which breaks certain use cases. See: systemd#28452

bluca · 2023-07-24T09:40:17Z

This is pretty clear and should fix the issue. My vote in the naming bikeshed is to rename, but it's not terribly important. If you disagree, feel free to set back the flag back.

I prefer to have a flag that is used when the non-default behaviour is needed - I have renamed it to SWITCH_ROOT_SKIP_RECURSIVE_RUN to make it clearer and more 'positive' - we got plenty of 'skip' flags around

yuwata · 2023-07-24T10:01:16Z

This is pretty clear and should fix the issue. My vote in the naming bikeshed is to rename, but it's not terribly important. If you disagree, feel free to set back the flag back.

I prefer to have a flag that is used when the non-default behaviour is needed - I have renamed it to SWITCH_ROOT_SKIP_RECURSIVE_RUN to make it clearer and more 'positive' - we got plenty of 'skip' flags around

Sounds good to me.

bluca added this to the v254 milestone Jul 22, 2023

github-actions bot added util-lib tests please-review PR is ready for (re-)review by a maintainer labels Jul 22, 2023

bluca force-pushed the run branch from 2f949db to a148b2f Compare July 22, 2023 21:49

systemd deleted a comment from github-actions bot Jul 22, 2023

bluca added the pid1 label Jul 22, 2023

bluca mentioned this pull request Jul 22, 2023

switch-root: Make sure all mounts under /run survive switch-root #28454

Closed

bluca removed the util-lib label Jul 22, 2023

bluca requested a review from keszybz July 23, 2023 10:29

bluca added the ci-failure-appears-unrelated label Jul 23, 2023

YHNdnzj reviewed Jul 23, 2023

View reviewed changes

src/shared/switch-root.c Outdated Show resolved Hide resolved

bluca force-pushed the run branch from a148b2f to 2e27818 Compare July 23, 2023 17:06

bluca removed the ci-failure-appears-unrelated label Jul 23, 2023

github-actions bot added the util-lib label Jul 23, 2023

bluca force-pushed the run branch from 2e27818 to 451d31c Compare July 23, 2023 17:09

YHNdnzj approved these changes Jul 23, 2023

View reviewed changes

yuwata reviewed Jul 23, 2023

View reviewed changes

YHNdnzj added 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 and removed please-review PR is ready for (re-)review by a maintainer labels Jul 23, 2023

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 Jul 23, 2023

YHNdnzj added the good-to-merge/with-minor-suggestions label Jul 23, 2023

bluca added 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 and removed good-to-merge/with-minor-suggestions labels Jul 23, 2023

keszybz approved these changes Jul 24, 2023

View reviewed changes

keszybz added good-to-merge/with-minor-suggestions and removed 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 labels Jul 24, 2023

bluca and others added 2 commits July 24, 2023 10:39

switch-root: use MS_REC for /run, unless we are soft-rebooting

b12d41a

There are applications that rely on mounts under /run surviving the switch from initrd to rootfs, so use MS_REC unless we are soft rebooting. Follow-up for 7c764d4 Fixes systemd#28452

bluca force-pushed the run branch from 451d31c to 8256994 Compare July 24, 2023 09:39

bluca added 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 and removed util-lib good-to-merge/with-minor-suggestions labels Jul 24, 2023

yuwata approved these changes Jul 24, 2023

View reviewed changes

bluca merged commit 2bfe726 into systemd:main Jul 24, 2023
41 of 48 checks passed

bluca deleted the run branch July 24, 2023 10:20

github-actions bot 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 Jul 24, 2023

This was referenced Jul 24, 2023

test: check if we correctly propagate /run mounts during switch root #28456

Closed

Recursive move with test #28457

Closed

yuwata mentioned this pull request Aug 12, 2023

various switch-root tweaks/fixes #28793

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

switch-root: use MS_REC for /run, unless we are soft-rebooting #28497

switch-root: use MS_REC for /run, unless we are soft-rebooting #28497

bluca commented Jul 22, 2023 •

edited by github-actions bot

yuwata Jul 23, 2023 •

edited

bluca Jul 23, 2023

yuwata Jul 23, 2023

bluca Jul 23, 2023

keszybz Jul 24, 2023

keszybz left a comment

keszybz Jul 24, 2023

bluca commented Jul 24, 2023

yuwata commented Jul 24, 2023

switch-root: use MS_REC for /run, unless we are soft-rebooting #28497

switch-root: use MS_REC for /run, unless we are soft-rebooting #28497

Conversation

bluca commented Jul 22, 2023 • edited by github-actions bot

yuwata Jul 23, 2023 • edited

Choose a reason for hiding this comment

bluca Jul 23, 2023

Choose a reason for hiding this comment

yuwata Jul 23, 2023

Choose a reason for hiding this comment

bluca Jul 23, 2023

Choose a reason for hiding this comment

keszybz Jul 24, 2023

Choose a reason for hiding this comment

keszybz left a comment

Choose a reason for hiding this comment

keszybz Jul 24, 2023

Choose a reason for hiding this comment

bluca commented Jul 24, 2023

yuwata commented Jul 24, 2023

bluca commented Jul 22, 2023 •

edited by github-actions bot

yuwata Jul 23, 2023 •

edited