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

profiles/seccomp: use conditional (hard-coded) errNoRet for MIPS/non-MIPS #43005

Closed
wants to merge 1 commit into from

Conversation

thaJeztah
Copy link
Member

relates to:

The value of ENOSYS differs between MIPS and non-MIPS architectures. While
this is not problematic for the embedded seccomp profile, it prevents the
profile from being saved as a portable JSON file that can be used for both
architectures.

To work around this situation, we include conditional rules for both arches.
and hard-code the value for ENOSYS in both.

For more details, refer to #42836 (comment)
and opencontainers/runtime-spec#1087 (comment)

A test was added, which can be run with:

go test --tags=seccomp -run TestLoadConditionalClone3 -v ./profiles/seccomp/

=== RUN   TestLoadConditionalClone3
=== RUN   TestLoadConditionalClone3/clone3_default_amd64
=== RUN   TestLoadConditionalClone3/clone3_default_mips
=== RUN   TestLoadConditionalClone3/clone3_cap_sys_admin_amd64
=== RUN   TestLoadConditionalClone3/clone3_cap_sys_admin_mips
--- PASS: TestLoadConditionalClone3 (0.01s)
    --- PASS: TestLoadConditionalClone3/clone3_default_amd64 (0.00s)
    --- PASS: TestLoadConditionalClone3/clone3_default_mips (0.00s)
    --- PASS: TestLoadConditionalClone3/clone3_cap_sys_admin_amd64 (0.00s)
    --- PASS: TestLoadConditionalClone3/clone3_cap_sys_admin_mips (0.00s)
PASS
ok  	github.com/docker/docker/profiles/seccomp	0.015s

Signed-off-by: Sebastiaan van Stijn github@gone.nl

- What I did

- How I did it

- How to verify it

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

…MIPS

The value of ENOSYS differs between MIPS and non-MIPS architectures. While
this is not problematic for the embedded seccomp profile, it prevents the
profile from being saved as a portable JSON file that can be used for both
architectures.

To work around this situation, we include conditional rules for both arches.
and hard-code the value for ENOSYS in both.

For more details, refer to moby#42836 (comment)
and opencontainers/runtime-spec#1087 (comment)

A test was added, which can be run with:

    go test --tags=seccomp -run TestLoadConditionalClone3 -v ./profiles/seccomp/

    === RUN   TestLoadConditionalClone3
    === RUN   TestLoadConditionalClone3/clone3_default_amd64
    === RUN   TestLoadConditionalClone3/clone3_default_mips
    === RUN   TestLoadConditionalClone3/clone3_cap_sys_admin_amd64
    === RUN   TestLoadConditionalClone3/clone3_cap_sys_admin_mips
    --- PASS: TestLoadConditionalClone3 (0.01s)
        --- PASS: TestLoadConditionalClone3/clone3_default_amd64 (0.00s)
        --- PASS: TestLoadConditionalClone3/clone3_default_mips (0.00s)
        --- PASS: TestLoadConditionalClone3/clone3_cap_sys_admin_amd64 (0.00s)
        --- PASS: TestLoadConditionalClone3/clone3_cap_sys_admin_mips (0.00s)
    PASS
    ok  	github.com/docker/docker/profiles/seccomp	0.015s

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah
Copy link
Member Author

@tianon @AkihiroSuda @justincormack PTAL

@thaJeztah thaJeztah added this to the 21.xx milestone Nov 9, 2021
Copy link
Member

@tianon tianon left a comment

Choose a reason for hiding this comment

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

LGTM

"Thanks!  I hate it."

❤️

@thaJeztah
Copy link
Member Author

Failure on Windows 2022 looks like a flaky (tracked in #42612);

=== RUN   TestRequestReleaseAddressDuplicate
    allocator_test.go:1531: IP 198.168.0.186/23 was previously allocated
--- FAIL: TestRequestReleaseAddressDuplicate (0.01s)

@thaJeztah
Copy link
Member Author

discussing this change, and (see opencontainers/runtime-spec#1087), perhaps the better solution is to fix the OCI spec to take a string instead of a number for the signal. This feature hasn't shipped yet in the OCI spec, so we can still fix it.

@thaJeztah thaJeztah closed this Dec 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants