-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
mkdir: warn when --context is used without SELinux/SMACK #13311
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
Open
Chaganti-Reddy
wants to merge
3
commits into
uutils:main
Choose a base branch
from
Chaganti-Reddy:mkdir-context-warning
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+79
−10
Open
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
3783d6e
mkdir: warn when --context is used without SELinux/SMACK. Fixes #12985
Chaganti-Reddy d0b0c17
fix stale comment: feat_selinux doesn't require an SELinux kernel to run
Chaganti-Reddy 03108d5
mkdir: warn instead of silently ignoring --context when SMACK isn't e…
Chaganti-Reddy File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Running a uutils compiled with
feat_selinuxrequires an SELinux enabled Kernel at run time, so I'm not sure what this is testing.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's testing that when the binary has
selinuxsupport compiled in but the kernel doesn't have it enabled, mkdir still creates the dir and just warns, matching GNU. That's a real state,feat_selinuxonly meanslibselinuxis linked in, it doesn't need anSELinux kernelto run. Checked it here,getenforcesays Disabled, and the test actually hit the assertions instead of skipping.Though you're right it never runs in our own
CI. The only job that tests withfeat_selinuxboots aFedora VMwith SELinux on, the lint job with the feature only runs clippy. So this test always self-skips on our runners even though it's correct locally. Can drop it if you'd rather not carry something CI never exercises.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd assumed it wasn't a real state due to this comment:
coreutils/Cargo.toml
Line 85 in 18e1df7
I guess the comment is incorrect.
This does expose a wider issue if we don't run CI for
feat_selinuxwithout SELinux kernel.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense that's where that came from, the comment was just wrong. Pushed a fix for it.
Agreed on the CI gap too, that's real. Feels like its own thing separate from this PR though — want me to open an issue for it, or would you rather just add a plain ubuntu-latest feat_selinux test job here directly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CI failures should be resolved by #13329.
I think it makes sense to open a separate issue for the missing CI coverage.
Does the same apply for SMACK here:
coreutils/Cargo.toml
Line 100 in 0f72a5b
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it applies, and it's worse there.
is_smack_enabled()is the same kind of safe runtime check (Path::new("/sys/fs/smackfs").exists()), comment was wrong the same way. Butset_smack_label_and_cleanupwas also silently returning Ok when SMACK isn't enabled, no warning at all, so mkdir was just pretending --context worked. Fixed the comment and wired the SMACK branch to warn the same way SELinux does now, reusing the existingmkdir-warning-context-not-selinuxstring since it already says "SELinux/SMACK" generically. Also feat_smack isn't built in any CI job at all currently, not even for lint, so this had zero coverage anywhere. Added a test for it locally, confirmed it hits the real assertion here.