Skip to content

Conversation

@vbatts
Copy link
Member

@vbatts vbatts commented Jan 24, 2018

This default seccomp profile may need to be used/generated from
non-linux platforms, though the use of syscall package confines the
compile to linux only

Signed-off-by: Vincent Batts [email protected]

import "syscall"

const (
CLONE_NEWIPC = syscall.CLONE_NEWIPC
Copy link
Contributor

Choose a reason for hiding this comment

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

golint suggests CloneNewIPC, etc. Breaking consistency with syscall would be unfortunate, but I'd prefer CloneNewIPC over hoop-jumping to make golint accept CLONE_NEWIPC.

Copy link
Member Author

Choose a reason for hiding this comment

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

fair, but I was hoping to stick to syscall format

@liangchenye
Copy link
Member

please update the naming issue as golint suggest 'CLONE_NEWIPC ', other wise it looks good to me.

package seccomp

// These are copied from linux/amd64 syscall values, as a reference for other
// platforms to have access to
Copy link
Contributor

Choose a reason for hiding this comment

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

If these values were amd64-specific, that would be a problem, because folks generating configs for other arches from non-Linux systems would get the wrong values. But it turns out that these values are arch-independent. Maybe adjust this comment to point that out, possibly linking to the kerrnel?

CLONE_NEWNS = 0x20000
CLONE_NEWPID = 0x20000000
CLONE_NEWUSER = 0x10000000
CLONE_NEWUTS = 0x4000000
Copy link
Contributor

Choose a reason for hiding this comment

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

You may want to add CloneNewCgroup = 0x02000000. We support the cgroup namespace since opencontainers/runtime-spec#397. I'm not sure why the seccomp lib doesn't need it (yet?), but I'd rather try to keep full new-namespace coverage.

Copy link
Member Author

Choose a reason for hiding this comment

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

it's not even in the go syscall yet. I'm going to hold on this addition.

Copy link
Contributor

Choose a reason for hiding this comment

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

it's not even in the go syscall yet. I'm going to hold on this addition.

syscall is end-of-life. It's in golang.org/x/sys/unix since golang/sys@b44883b47 (2016-06-09).

This default seccomp profile may need to be used/generated from
non-linux platforms, though the use of syscall package confines the
compile to linux only

Signed-off-by: Vincent Batts <[email protected]>
@vbatts
Copy link
Member Author

vbatts commented Feb 27, 2018

updated. PTAL


package seccomp

import "syscall"
Copy link
Contributor

Choose a reason for hiding this comment

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

syscall is end-of-life:

NOTE: This package is locked down. Code outside the standard Go repository should be migrated to use the corresponding package in the golang.org/x/sys repository…

We already vendor golang.org/x/sys here; can we use that instead?

@liangchenye
Copy link
Member

liangchenye commented Mar 6, 2018

LGTM

Approved with PullApprove

@liangchenye liangchenye merged commit 7479ae9 into opencontainers:master Mar 6, 2018
@vbatts vbatts deleted the seccomp_platform branch March 6, 2018 18:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants