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

remount: ignore ENOENT error during SELinux relabeling #3266

Merged

Conversation

ericcurtin
Copy link
Collaborator

@ericcurtin ericcurtin commented Jun 18, 2024

Ignore ENOENT error in selinux_restorecon to avoid failures when temporary files created by systemd-sysusers in /etc are missing during relabeling. This prevents errors such as:

"Failed to relabel /etc/.#gshadowJzu4Rx: No such file or directory"

and allows the process to continue.

@ericcurtin ericcurtin self-assigned this Jun 18, 2024
@github-actions github-actions bot added the area/prepare-root Issue relates to ostree-prepare-root label Jun 18, 2024
@ericcurtin ericcurtin force-pushed the if-file-missing-on-relabel-continue branch 2 times, most recently from f84d97a to 11ed3b0 Compare June 18, 2024 14:16
@alexlarsson
Copy link
Member

lgtm

Copy link
Member

@cgwalters cgwalters left a comment

Choose a reason for hiding this comment

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

Hmm, so we'd then rely on any process doing concurrent writes to have initialized the labels on its own?

I don't think that can really work longer term because it'd still be racy on things like the parent directory, right?

ISTM we need to move this logic out of ostree-remount.service into something that happens basically immediately after the systemd policy load.

Ah, digging in a bit I see there's /run/systemd/relabel-extra.d, I think all we need to do here is synthesize a variant of that in the transient-etc case that contains simply /etc.

I think that'd be equally easy and much more reliable, but I'm approving this PR as is as I think it will mostly work too.

@alexlarsson
Copy link
Member

/run/systemd/relabel-extra.d isn't good enough as-is, as doing so would cause a massive relabel of all of /etc, instead of the smarter approach of just relabeling the very few files that have a file in the upper.

@ericcurtin ericcurtin force-pushed the if-file-missing-on-relabel-continue branch from 11ed3b0 to f68cf8d Compare June 18, 2024 14:53
@ericcurtin
Copy link
Collaborator Author

Made a very minor change, there was a small typo in the comment

@cgwalters
Copy link
Member

/run/systemd/relabel-extra.d isn't good enough as-is, as doing so would cause a massive relabel of all of /etc, instead of the smarter approach of just relabeling the very few files that have a file in the upper.

OK, right. So to fix this we'd need to extend relabel-extra.d to support something that separates "physical vs logical root" to have e.g. /run/ostree/transient-etc /etc (relabel that dir as if it was /etc.

That said, the other fix here is to also add Before=systemd-sysusers.service - we already have a big pile of those.

Actually, I think we could split this logic out into something like its own distinct ostree-relabel-etc.service that could run very early in the real root, literally just After=etc.mount and Before=systemd-remount-fs.service even.

@alexlarsson
Copy link
Member

Actually, I think we could split this logic out into something like its own distinct ostree-relabel-etc.service that could run very early in the real root, literally just After=etc.mount and Before=systemd-remount-fs.service even.

Yeah, thats probably a good idea.

@cgwalters
Copy link
Member

Tangentially related, we don't have CI coverage for the transient-etc case in this repo. Something on the list.

@ericcurtin
Copy link
Collaborator Author

ericcurtin commented Jun 18, 2024

I'll work on this in a follow on PR:

ostree-relabel-etc.service

@ericcurtin
Copy link
Collaborator Author

Although I can also put a Before=systemd-remount-fs.service in this PR also if it is requested

@cgwalters
Copy link
Member

Although I can also put a Before=systemd-remount-fs.service in this PR also if it is requested

No, in this PR it needs to be Before=systemd-sysusers.service

@ericcurtin
Copy link
Collaborator Author

Although I can also put a Before=systemd-remount-fs.service in this PR also if it is requested

No, in this PR it needs to be Before=systemd-sysusers.service

Pushing, sorry I meant that one, copy pasted the wrong one in here, re-pushing in seconds

Ignore ENOENT error in selinux_restorecon to avoid failures when
temporary files created by systemd-sysusers in /etc are missing during
relabeling. This prevents errors such as:

  "Failed to relabel /etc/.#gshadowJzu4Rx: No such file or directory"

and allows the process to continue.

Co-Authored-By: Alexander Larsson <[email protected]>
Signed-off-by: Eric Curtin <[email protected]>
@ericcurtin ericcurtin force-pushed the if-file-missing-on-relabel-continue branch from f68cf8d to e25ca80 Compare June 18, 2024 17:44
@cgwalters cgwalters merged commit f280b12 into ostreedev:main Jun 18, 2024
22 of 25 checks passed
@ericcurtin ericcurtin deleted the if-file-missing-on-relabel-continue branch June 18, 2024 18:20
@@ -25,7 +25,7 @@ After=-.mount var.mount
After=systemd-remount-fs.service
# But we run *before* most other core bootup services that need write access to /etc and /var
Before=local-fs.target umount.target
Before=systemd-random-seed.service plymouth-read-write.service systemd-journal-flush.service
Before=systemd-random-seed.service plymouth-read-write.service systemd-journal-flush.service systemd-sysusers.service
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, this actually created an ordering cycle it looks like; my bad for not digging into this. I merged over red because we had other unrelated tests that were broken.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/prepare-root Issue relates to ostree-prepare-root
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants