Init a selinux project #2800

Gekko0114 · 2024-05-26T13:15:52Z

This is an experimental crate. I'll implement a selinux crate step-by-step.
When it is mature, I'll move it under the 'crate/' and use it.
ref: #2718

Gekko0114 · 2024-05-31T15:03:50Z

@YJDoc2 @utam0k
As the initial PR, what function should I implement at least? Or is any small PR fine ?

utam0k · 2024-06-02T04:51:01Z

I'm not familiar with this area. So, It would be nice if you could make the PR small enough so that one function could run on e2e.

Gekko0114 · 2024-06-04T14:09:33Z

Sure, thanks!

Signed-off-by: Hiroyuki Moriya <41197469+Gekko0114@users.noreply.github.com>

Gekko0114 · 2024-06-07T21:28:05Z

Hi @utam0k , @YJDoc2

Though this PR is small, I would like to complete this before becoming too complicated.
Could you review it?

utam0k · 2024-06-09T11:43:55Z

Though this PR is small, I would like to complete this before becoming too complicated.
Could you review it?

This PR is the perfect size for me to review! Thanks. But sorry, I'll revisit it next weekend 🙏

YJDoc2

Hey, some comments, some nitpicks, overall the implementation seems to be going in the right direction 👍

Can we add doc comments for all the pub functions (which are implemented here), so it will be easier to review what the function is supposed to do + documentation

minor nitpicks :

instead of panic!(...) I feel unimplemented!() would be a better fit. Even though both behave the same, I feel unimplemented is semantically better. I am not asking to change the current use, but maybe for future PRs.
Can you check if clippy is run here and does not give any errors? I have had issues in past when running clippy in CI with projects excluded via cargo.toml.

experiment/selinux/src/selinux.rs

YJDoc2 · 2024-06-10T07:47:45Z

experiment/selinux/src/selinux.rs

+    ATTR_PATH_INIT.call_once(|| {
+        let path = PathBuf::from(THREAD_SELF_PREFIX);
+        unsafe {
+            HAVE_THREAD_SELF = path.is_dir();
+        }
+    });
+
+    unsafe {
+        if HAVE_THREAD_SELF {
+            return format!("{}/{}", THREAD_SELF_PREFIX, attr);
+        }
+    }
+


Can you explain the use of this function? I am not able to understand it correctly 😅

What is the reason for using Once and call_once here? Assuming youki as the primary use-case, is there still a chance of having a multi-thread race? If so can be use AtomicBool instead and remove dependency of Once? (Suggesting as underlying data is simple bool type). We might be able to do an compare-and-exchange operation instead of the call_once here.

Do we need the unsafe block if we are only reading the HAVE_THREAD_SELF value? I thought unsafe was needed only if we were writing to the variable.

Thanks, I've updated the code using atomicBool instead.

I implemented using Once because go-selinux implemented like that.
I guess that this is to avoid conflicts between multi-thread like goroutine.
I thought it is possible that similar issue happens on Youki as well.
https://github.com/opencontainers/selinux/blob/9dee859cbffb974e779f65efaef9f5ef6e1d260b/go-selinux/selinux_linux.go#L451-L467

I am not familiar with selinux nor youki, so please let me know if I misunderstood.

Hey, thanks. I am also not that familiar with selinux-go, so will take a look and confirm once.

Signed-off-by: Hiroyuki Moriya <41197469+Gekko0114@users.noreply.github.com>

experiment/selinux/README.md

utam0k

Thanks, @Gekko0114! This is a good first step for us. I left some comments.

experiment/selinux/src/selinux.rs

utam0k · 2024-06-16T03:58:40Z

experiment/selinux/src/selinux.rs

+// function compatible with lSetFileLabel in go-selinux repo.
+// lset_file_label sets the SELinux label for this path, not following symlinks,
+// or returns an error.
+pub fn lset_file_label(fpath: &str, label: &str) -> Result<(), std::io::Error> {


We should introduce the thiserror crate instead of std::io::Error at some point in the future or in this PR.

Sure, I introduced thiserror. Please let me know if I don't use thiserror correctly.

utam0k · 2024-06-16T04:00:06Z

experiment/selinux/src/selinux.rs

+static HAVE_THREAD_SELF: AtomicBool = AtomicBool::new(false);
+static INIT_DONE: AtomicBool = AtomicBool::new(false);
+
+// function compatible with setDisabled in go-selinux repo.


I don't think we have to keep it compatible with the go-selinux repo. Is there any specific reason to keep it?

In my view, prioritizing Rust-friendliness over maintaining compatibility with go-selinux is more important.

In my view, prioritizing Rust-friendliness over maintaining compatibility with go-selinux is more important.

Yeah, agree. I couldn't write Rust-friendliness well because I am not so familiar with Rust yet.
Please let me know if you feel weird in terms of Rust. Thank you so much!

Please let me know if you feel weird in terms of Rust. Thank you so much!

Sure. First of all, we can take using struct instead of global variables into consideration. WDYT?

Sure, as you commented in another thread, I will use struct.
I will fix it after reading experiment/seccomp and other codes.

If we use struct here and call this struct more than once, I thought maybe the selinux struct is initialized more than once. But the performance caused by this is tiny, so I will use struct here.

experiment/selinux/src/selinux.rs

Signed-off-by: Hiroyuki Moriya <41197469+Gekko0114@users.noreply.github.com>

Gekko0114 · 2024-06-20T06:31:41Z

Hi @utam0k, @YJDoc2,

Thanks to your comments, I've fixed this PR.
However, I think there are some parts which is not rust-friendly.
I would appreciate it if you could let me know.

utam0k

It looks good to me as a first PR for a Selinxu project. In the next PR, it'd be great if there was an excellent example to run with the selinux crate.

experiment/selinux/src/selinux.rs

Gekko0114 · 2024-06-21T01:54:47Z

@utam0k
Thanks! Since I don't have the write access, I can't merge this PR.
Could you merge this PR if it is ok?

utam0k · 2024-06-21T11:22:47Z

Thanks, @Gekko0114 ! Great work!

YJDoc2 added the kind/experimental `/experimental` label May 26, 2024

Gekko0114 force-pushed the selinux branch from f5a1bed to c2cb79b Compare May 29, 2024 15:02

Gekko0114 force-pushed the selinux branch from 91a4765 to 0a82740 Compare June 5, 2024 15:03

Gekko0114 marked this pull request as ready for review June 7, 2024 21:11

selinux_init

c01de44

Signed-off-by: Hiroyuki Moriya <41197469+Gekko0114@users.noreply.github.com>

Gekko0114 force-pushed the selinux branch from 9c31834 to c01de44 Compare June 7, 2024 21:15

utam0k self-assigned this Jun 9, 2024

utam0k self-requested a review June 9, 2024 11:44

YJDoc2 reviewed Jun 10, 2024

View reviewed changes

use_unimplemented

944d8ef

Signed-off-by: Hiroyuki Moriya <41197469+Gekko0114@users.noreply.github.com>

Gekko0114 force-pushed the selinux branch 5 times, most recently from a9f7e00 to 15c71b1 Compare June 13, 2024 04:39

add_explanation

16f3fcd

Signed-off-by: Hiroyuki Moriya <41197469+Gekko0114@users.noreply.github.com>

Gekko0114 force-pushed the selinux branch from 15c71b1 to 16f3fcd Compare June 13, 2024 07:39

update

fc2e7de

Signed-off-by: Hiroyuki Moriya <41197469+Gekko0114@users.noreply.github.com>

Gekko0114 force-pushed the selinux branch from 21c380f to fc2e7de Compare June 14, 2024 14:50

utam0k reviewed Jun 16, 2024

View reviewed changes

experiment/selinux/README.md Outdated Show resolved Hide resolved

utam0k requested changes Jun 16, 2024

View reviewed changes

Gekko0114 added 3 commits June 16, 2024 14:17

use_path_instead_of_str

ad74f2b

Signed-off-by: Hiroyuki Moriya <41197469+Gekko0114@users.noreply.github.com>

use_thiserror

4c61063

Signed-off-by: Hiroyuki Moriya <41197469+Gekko0114@users.noreply.github.com>

use_struct

d1d7ce1

Signed-off-by: Hiroyuki Moriya <41197469+Gekko0114@users.noreply.github.com>

utam0k approved these changes Jun 20, 2024

View reviewed changes

experiment/selinux/src/selinux.rs Show resolved Hide resolved

utam0k merged commit 6dd0d7f into containers:main Jun 21, 2024
28 checks passed

github-actions bot mentioned this pull request Jun 21, 2024

Release for v0.3.4 #2791

Open

Gekko0114 deleted the selinux branch June 22, 2024 01:06

Gekko0114 mentioned this pull request Jun 22, 2024

selinux: write xattr related codes. #2825

Merged

Gekko0114 mentioned this pull request Jul 19, 2024

selinux: implemented remaining selinux functions #2850

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Init a selinux project #2800

Init a selinux project #2800

Gekko0114 commented May 26, 2024

Gekko0114 commented May 31, 2024

utam0k commented Jun 2, 2024

Gekko0114 commented Jun 4, 2024

Gekko0114 commented Jun 7, 2024

utam0k commented Jun 9, 2024

YJDoc2 left a comment

YJDoc2 Jun 10, 2024

Gekko0114 Jun 14, 2024 •

edited

Loading

YJDoc2 Jun 14, 2024

utam0k left a comment

utam0k Jun 16, 2024

Gekko0114 Jun 19, 2024

utam0k Jun 16, 2024

utam0k Jun 16, 2024

Gekko0114 Jun 16, 2024 •

edited

Loading

utam0k Jun 17, 2024

Gekko0114 Jun 18, 2024

Gekko0114 commented Jun 20, 2024

utam0k left a comment

Gekko0114 commented Jun 21, 2024

utam0k commented Jun 21, 2024 •

edited

Loading

Init a selinux project #2800

Init a selinux project #2800

Conversation

Gekko0114 commented May 26, 2024

Gekko0114 commented May 31, 2024

utam0k commented Jun 2, 2024

Gekko0114 commented Jun 4, 2024

Gekko0114 commented Jun 7, 2024

utam0k commented Jun 9, 2024

YJDoc2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gekko0114 Jun 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

utam0k left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gekko0114 Jun 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gekko0114 commented Jun 20, 2024

utam0k left a comment

Choose a reason for hiding this comment

Gekko0114 commented Jun 21, 2024

utam0k commented Jun 21, 2024 • edited Loading

Gekko0114 Jun 14, 2024 •

edited

Loading

Gekko0114 Jun 16, 2024 •

edited

Loading

utam0k commented Jun 21, 2024 •

edited

Loading