8000 seccomp: add arch-specific syscalls on ARM by trusch · Pull Request #3636 · rkt/rkt · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content
This repository was archived by the owner on Feb 24, 2020. It is now read-only.

seccomp: add arch-specific syscalls on ARM #3636

Merged
merged 1 commit into from
May 10, 2017

Conversation

trusch
Copy link
Contributor
@trusch trusch commented Apr 10, 2017

This enables us to run binaries which use these syscalls when systemd has seccomp enforcing enabled.
Like reported this should affect at least all gcc compiled binaries.

This PR needs more testing, I just want to see what CI says. I'll setup at least one testbed with seccomp-enabled systemd. Perhaps @Xenopathic could test on aarch64 devices, I could test for armv6l and armv7l devices.

Fixes #3629.

@ghost
Copy link
ghost commented Apr 10, 2017

Can one of the admins verify this patch?

@RobinMcCorkell
Copy link

I can confirm that this works on ARMv7. Unfortunately I don't have an AArch64 device to test on, nor an ARMv6 device.

@squeed
Copy link
Contributor
squeed commented Apr 10, 2017

Does it make sense to add these additional syscalls via build tags instead of an init() method?

@trusch
Copy link
Contributor Author
trusch commented Apr 10, 2017

I think this would be more performant than the current implementation but it would increase maintenance complexity. At least we could use raw runtime.GOARCH since the additional syscalls are needed for all arm flavors. Then the performance impact would be minimal

@lucab
Copy link
Member
lucab commented Apr 27, 2017

ok to test

@trusch
Copy link
Contributor Author
trusch commented Apr 28, 2017

semaphore disk is full btw.

// RktDefaultSeccompBlacklist contains a default blacklist of syscalls,
// used by rkt for seccomp filtering.
RktDefaultSeccompBlacklist = DockerDefaultSeccompBlacklist
// RktDefaultSeccompWhitelist contains a default whitelist of syscalls,
// used by rkt for seccomp filtering.
RktDefaultSeccompWhitelist = DockerDefaultSeccompWhitelist
)

func init() {
if runtime.GOARCH == "arm" {
Copy link
Member

Choose a reason for hiding this comment

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

I see we are also doing 64bits whitelisting here, so this should probably also check for the arm64 case.

@lucab
Copy link
Member
lucab commented May 8, 2017

Yep, we are unfortunately hitting semaphore disk limits. I've opened #3670 in the meanwhile, which should hopefully bring us back to a reasonable amount. I'd like to see one last change in here, otherwise this PR seems fine.

@trusch
Copy link
Contributor Author
trusch commented May 8, 2017 via email

@lucab
Copy link
Member
lucab commented May 10, 2017

@trusch I fear something went wrong while rebasing this PR, it now contains quite a number of external commits.

@trusch trusch force-pushed the feature/seccomp-whitelist-arm branch from 9b6aeb7 to 8c9b34c Compare May 10, 2017 10:41
@trusch
Copy link
Contributor Author
trusch commented May 10, 2017

@lucab sorry, I hope it is fixed now. How should this be handles in general when a PR needs some days? Merging the master into the PR or rebasing on it?

@lucab lucab changed the title seccomp: add ARM specific syscalls if on an ARM device. seccomp: add arch-specific syscalls on ARM May 10, 2017
Copy link
Member
@lucab lucab left a comment

Choose a reason for hiding this comment

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

LGTM

@lucab
Copy link
Member
lucab commented May 10, 2017

@trusch rebasing your PR on top of master is typically preferred to minimize the number of merge commits and to avoid having conflicts resolved in intermediate merge commits.

This enables us to run binaries which use these syscalls when systemd has seccomp enforcing enabled.
Like reported this should affect at least all gcc compiled binaries.

This solves issue rkt#3629.
@lucab lucab force-pushed the feature/seccomp-whitelist-arm branch from 8c9b34c to 5f49ab0 Compare May 10, 2017 14:02
@lucab lucab merged commit d91cddd into rkt:master May 10, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants
0