Improve SetKeyCreate error reporting, fix test flakes #227

kolyshkin · 2025-03-12T02:52:09Z

Fixes: #222
Fixes: #225

Please review commit by commit. See individual commits for details.

Ideally, I'd like this to be merged after #218 and #223.

rhatdan · 2025-03-12T13:39:50Z

LGTM
Nice work, I wonder if we should do something similar with execcon, which has similar issues.

rhatdan · 2025-03-12T13:43:22Z

Needs a rebase.

kolyshkin · 2025-03-12T17:17:03Z

Rebased.

kolyshkin · 2025-03-12T18:36:12Z

I wonder if we should do something similar with execcon, which has similar issues.

Currently SetExecLabel modifies the thread attribute (not the main process attribute). Do you mean to switch it to modify "/proc/self/attr/exec" instead and error out if pid != tid?

rhatdan · 2025-03-12T20:55:33Z

No I don't think we should change that functionality, just let it go.

kolyshkin · 2025-03-19T02:29:24Z

@thaJeztah PTAL

rhatdan · 2025-03-19T13:07:38Z

Needs a rebase.

thaJeztah

LGTM, nice!

left one suggestion, but nothing new, so definitely fine for separate

thaJeztah · 2025-03-19T21:37:10Z

go-selinux/selinux_linux_test.go

 	} else {
-		t.Log("SetFSCreateLabel failed", err)
-		t.Fatal(err)
+		t.Fatal("SetFSCreateLabel failed:", err)


Nit; t.Fatal terminates the test, so we could get rid of the if/else, and instead put the t.Fatal in a if err != nil branch

I was just trying to minimize the changes to simplify review. But since you've asked for it... 😁

Indeed it looks better this way.

The SetFSCreateLabel documentation says the caller should use runtime.LockOSThread. Indeed, if not used, there is an occasional flake: > $ go test -run TestSELinux -count 10000 . > selinux_linux_test.go:168: SetFSCreateLabel failed write /proc/thread-self/attr/fscreate: permission denied > selinux_linux_test.go:169: write /proc/thread-self/attr/fscreate: permission denied Add LockOSThread to fix the flake. While at it, simplify code flow to avoid using "else", and fix logging to avoid printing the same error twice. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

The SetSocketLabel documentation says the caller should use runtime.LockOSThread. Indeed, if not used, there is an occasional flake: > $ go test -count 10000 -run SocketLabel . > --- FAIL: TestSocketLabel (0.00s) > label_linux_test.go:189: write /proc/thread-self/attr/sockcreate: permission denied Add LockOSThread to fix the flake. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

Since commit bbbc51c ("Need to set process attributes not task") SetKeyLabel and KeyLabel operate on /proc/self/attr/keycreate, which can only be modified by the thread group leader; a non thread group leader thread will get EACCES: > write /proc/self/attr/keycreate: permission denied Let's document that, and return a more meaningful error (ErrNotTGLeader) if that is the case. Modify the test case accordingly, i.e. skip it if the current thread is not the thread group leader. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

This is not the kind of race that Go race detector would catch; this is a race when a /proc file of one thread is opened and when Go changes the underlying thread, and that different thread tries to write to that file descriptor. In order to catch those reliably, we need quite a big number of iterations, and we only need to test go-selinux stuff. I chose the count to be 100000 because it takes about 1 minute on my machine: [kir@kir-tp1 selinux]$ time go test -timeout 3m -count 100000 ./go-selinux ok github.com/opencontainers/selinux/go-selinux 53.763s real 0m53.983s user 0m30.030s sys 0m30.339s Also note that this only makes sense to run on a SELinux enabled systems (i.e. those we run under lima). Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

kolyshkin · 2025-03-20T02:16:03Z

Rebased; addressed @thaJeztah comment.

thaJeztah

LGTM, thanks!

kolyshkin · 2025-03-20T17:56:04Z

@rhatdan ptal

This PR contains the following updates: | Package | Change | Age | Confidence | |---|---|---|---| | [github.com/opencontainers/selinux](https://github.com/opencontainers/selinux) | `v1.11.0` -> `v1.12.0` | [![age](https://developer.mend.io/api/mc/badges/age/go/github.com%2fopencontainers%2fselinux/v1.12.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://developer.mend.io/api/mc/badges/confidence/go/github.com%2fopencontainers%2fselinux/v1.11.0/v1.12.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>opencontainers/selinux (github.com/opencontainers/selinux)</summary> ### [`v1.12.0`](https://github.com/opencontainers/selinux/releases/tag/v1.12.0) [Compare Source](opencontainers/selinux@v1.11.1...v1.12.0) This release removes deprecated functions from the `label` package, and improves documentation and error reporting of `SetCreateKey`. #### What's Changed - VERSION: remove by [@kolyshkin](https://github.com/kolyshkin) in opencontainers/selinux#217 - CI: add AlmaLinux 8, CentOS Stream 9, and Fedora by [@AkihiroSuda](https://github.com/AkihiroSuda) in opencontainers/selinux#221 - ci: install git-core by [@kolyshkin](https://github.com/kolyshkin) in opencontainers/selinux#224 - CI: add openSUSE Tumbleweed by [@AkihiroSuda](https://github.com/AkihiroSuda) in opencontainers/selinux#223 - Bump Go version, deps, fix some linter issues... by [@kolyshkin](https://github.com/kolyshkin) in opencontainers/selinux#218 - label: remove deprecated stuff by [@kolyshkin](https://github.com/kolyshkin) in opencontainers/selinux#228 - Improve SetKeyCreate error reporting, fix test flakes by [@kolyshkin](https://github.com/kolyshkin) in opencontainers/selinux#227 **Full Changelog**: opencontainers/selinux@v1.11.1...v1.12.0 ### [`v1.11.1`](https://github.com/opencontainers/selinux/releases/tag/v1.11.1) [Compare Source](opencontainers/selinux@v1.11.0...v1.11.1) #### What's Changed - Bump to v1.11.0 by [@rhatdan](https://github.com/rhatdan) in opencontainers/selinux#197 - fix some error by [@ningmingxiao](https://github.com/ningmingxiao) in opencontainers/selinux#200 - ci: update Go 1.21 support by [@michalbiesek](https://github.com/michalbiesek) in opencontainers/selinux#202 - Extend `build-cross` target with `riscv64` arch by [@michalbiesek](https://github.com/michalbiesek) in opencontainers/selinux#201 - Remove nolint annotations for unix errno comparisons by [@kolyshkin](https://github.com/kolyshkin) in opencontainers/selinux#203 - ci: bump some actions by [@kolyshkin](https://github.com/kolyshkin) in opencontainers/selinux#205 - Misc nitpicks by [@kolyshkin](https://github.com/kolyshkin) in opencontainers/selinux#206 - pwalk, pwalkdir: fix walk vs remove race by [@kolyshkin](https://github.com/kolyshkin) in opencontainers/selinux#204 - Update GitHub Actions CI Go matrix for Go v1.22 by [@austinvazquez](https://github.com/austinvazquez) in opencontainers/selinux#209 - Update GitHub Actions packages to resolve deprecation warnings. by [@austinvazquez](https://github.com/austinvazquez) in opencontainers/selinux#208 - Add dependabot config by [@kolyshkin](https://github.com/kolyshkin) in opencontainers/selinux#210 - build(deps): bump tim-actions/get-pr-commits from 1.3.0 to 1.3.1 by [@dependabot](https://github.com/dependabot) in opencontainers/selinux#211 - build(deps): bump golangci/golangci-lint-action from 4 to 6 by [@dependabot](https://github.com/dependabot) in opencontainers/selinux#213 - Show SELinux label on failure by [@rhatdan](https://github.com/rhatdan) in opencontainers/selinux#216 #### New Contributors - [@ningmingxiao](https://github.com/ningmingxiao) made their first contribution in opencontainers/selinux#200 - [@michalbiesek](https://github.com/michalbiesek) made their first contribution in opencontainers/selinux#202 - [@dependabot](https://github.com/dependabot) made their first contribution in opencontainers/selinux#211 **Full Changelog**: opencontainers/selinux@v1.11.0...v1.11.1 </details> --- ### Configuration 📅 **Schedule**: Branch creation - Between 12:00 AM and 03:59 AM ( * 0-3 * * * ) (UTC), Automerge - Between 12:00 AM and 03:59 AM ( * 0-3 * * * ) (UTC). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).  Reviewed-on: https://code.forgejo.org/forgejo/runner/pulls/801 Reviewed-by: earl-warren <earl-warren@noreply.code.forgejo.org> Co-authored-by: Renovate Bot <bot@kriese.eu> Co-committed-by: Renovate Bot <bot@kriese.eu>

kolyshkin force-pushed the fix-flake branch from 7cd037d to fcb973f Compare March 12, 2025 03:04

kolyshkin mentioned this pull request Mar 12, 2025

CI: add openSUSE Tumbleweed #223

Merged

kolyshkin force-pushed the fix-flake branch from fcb973f to 27c9168 Compare March 12, 2025 03:09

kolyshkin mentioned this pull request Mar 12, 2025

Bump Go version, deps, fix some linter issues... #218

Merged

kolyshkin force-pushed the fix-flake branch from 27c9168 to 9522b67 Compare March 12, 2025 03:14

kolyshkin marked this pull request as ready for review March 12, 2025 04:29

kolyshkin requested review from mrunalp, rhatdan, runcom and thaJeztah as code owners March 12, 2025 04:29

kolyshkin mentioned this pull request Mar 12, 2025

tests: use LockOSThread to avoid flakes #226

Closed

rhatdan previously approved these changes Mar 12, 2025

View reviewed changes

kolyshkin dismissed rhatdan’s stale review via d42909b March 12, 2025 17:13

kolyshkin force-pushed the fix-flake branch from 9522b67 to d42909b Compare March 12, 2025 17:13

kolyshkin added this to the v1.12 milestone Mar 12, 2025

rhatdan previously approved these changes Mar 12, 2025

View reviewed changes

thaJeztah previously approved these changes Mar 19, 2025

View reviewed changes

kolyshkin added 4 commits March 19, 2025 19:14

kolyshkin dismissed stale reviews from thaJeztah and rhatdan via 2a69eaf March 20, 2025 02:15

kolyshkin force-pushed the fix-flake branch from d42909b to 2a69eaf Compare March 20, 2025 02:15

kolyshkin requested review from thaJeztah and rhatdan March 20, 2025 02:15

thaJeztah approved these changes Mar 20, 2025

View reviewed changes

rhatdan approved these changes Mar 20, 2025

View reviewed changes

rhatdan merged commit 996c4cf into opencontainers:main Mar 20, 2025
17 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Improve SetKeyCreate error reporting, fix test flakes #227

Improve SetKeyCreate error reporting, fix test flakes #227

Uh oh!

kolyshkin commented Mar 12, 2025 •

edited

Loading

Uh oh!

rhatdan commented Mar 12, 2025

Uh oh!

rhatdan commented Mar 12, 2025

Uh oh!

kolyshkin commented Mar 12, 2025

Uh oh!

kolyshkin commented Mar 12, 2025

Uh oh!

rhatdan commented Mar 12, 2025

Uh oh!

kolyshkin commented Mar 19, 2025

Uh oh!

rhatdan commented Mar 19, 2025

Uh oh!

thaJeztah left a comment

Uh oh!

thaJeztah Mar 19, 2025

Uh oh!

kolyshkin Mar 20, 2025

Uh oh!

kolyshkin commented Mar 20, 2025

Uh oh!

thaJeztah left a comment

Uh oh!

kolyshkin commented Mar 20, 2025

Uh oh!

Uh oh!

Uh oh!

Improve SetKeyCreate error reporting, fix test flakes #227

Improve SetKeyCreate error reporting, fix test flakes #227

Uh oh!

Conversation

kolyshkin commented Mar 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rhatdan commented Mar 12, 2025

Uh oh!

rhatdan commented Mar 12, 2025

Uh oh!

kolyshkin commented Mar 12, 2025

Uh oh!

kolyshkin commented Mar 12, 2025

Uh oh!

rhatdan commented Mar 12, 2025

Uh oh!

kolyshkin commented Mar 19, 2025

Uh oh!

rhatdan commented Mar 19, 2025

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah Mar 19, 2025

Choose a reason for hiding this comment

Uh oh!

kolyshkin Mar 20, 2025

Choose a reason for hiding this comment

Uh oh!

kolyshkin commented Mar 20, 2025

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

kolyshkin commented Mar 20, 2025

Uh oh!

Uh oh!

Uh oh!

kolyshkin commented Mar 12, 2025 •

edited

Loading