Skip to content

Fix updating /sys/fs/cgroup mount to 'rw'#1085

Merged
tiborvass merged 4 commits intomoby:masterfrom
smira:sysfs-cgroup-rw-fix
Jul 18, 2019
Merged

Fix updating /sys/fs/cgroup mount to 'rw'#1085
tiborvass merged 4 commits intomoby:masterfrom
smira:sysfs-cgroup-rw-fix

Conversation

@smira
Copy link
Copy Markdown
Collaborator

@smira smira commented Jul 17, 2019

There were two bugs: Mount was matched by Type which is actually
cgroup, not sysfs. And the second problem was that copy of the value
was modified, not value in the slice.

Signed-off-by: Andrey Smirnov smirnov.andrey@gmail.com

@smira
Fix updating /sys/fs/cgroup mount to 'rw'
0abd2ca 
There were two bugs: Mount was matched by Type which is actually
`cgroup`, not `sysfs`. And the second problem was that copy of the value
was modified, not value in the slice.

Signed-off-by: Andrey Smirnov <smirnov.andrey@gmail.com>
@tonistiigi
Copy link
Copy Markdown
Member

tonistiigi commented Jul 17, 2019

This should be for both sysfs and cgroup. There are actaally functions already WithWriteableSysfs, WithWriteableCgroupfs that you can just call.

@tonistiigi
Copy link
Copy Markdown
Member

tonistiigi commented Jul 17, 2019

Or actually, there is WithPrivileged that should do all of this. Can clean up some handling in here.

@tonistiigi
Copy link
Copy Markdown
Member

tonistiigi commented Jul 17, 2019

Also, add checks for these in the existing security tests in client_test while you're at it.

@smira
Copy link
Copy Markdown
Collaborator Author

smira commented Jul 18, 2019

@tonistiigi I'm not really an expert in all of these low-level mounts, but looks like oci.With... options should be applied before we do oci.GenerateSpec, and the code here is doing more of changes after the fact. Was your point that I should move all these more into oci.With... options instead of hand-rolled changes to oci.Spec after oci.GenerateSpec is called?

@smira
Copy link
Copy Markdown
Collaborator Author

smira commented Jul 18, 2019

As it seems it's not 100% the same what can be done with oci.With... vs. what is there in executor/oci/spec_unix.go.

@GordonTheTurtle
Copy link
Copy Markdown
Collaborator

GordonTheTurtle commented Jul 18, 2019

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "sysfs-cgroup-rw-fix" git@github.com:smira/buildkit.git somewhere
$ cd somewhere
$ git rebase -i HEAD~842361581128
editor opens
change each 'pick' to 'edit'
save the file and quit
$ git commit --amend -s --no-edit
$ git rebase --continue # and repeat the amend for each commit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

@smira
Use oci.With helpers
5dbb31a 
Signed-off-by: Andrey Smirnov <smirnov.andrey@gmail.com>
@smira smira force-pushed the sysfs-cgroup-rw-fix branch from f2410c6 to 5dbb31a Compare July 18, 2019 14:35
@smira
Copy link
Copy Markdown
Collaborator Author

smira commented Jul 18, 2019

Updated with helpers, but only for rw re-mounts.

@smira
Add test for 'rw' sysfs/cgroup re-mount
fc9781a 
Signed-off-by: Andrey Smirnov <smirnov.andrey@gmail.com>
@smira smira force-pushed the sysfs-cgroup-rw-fix branch from 1f4db0e to fc9781a Compare July 18, 2019 15:53
tonistiigi
tonistiigi reviewed Jul 18, 2019
View reviewed changes
client/client_test.go Outdated
AllowedEntitlements: allowedEntitlements,
}, nil)

if secMode == securitySandbox || sb.Rootless() {
Copy link
Copy Markdown
Member

@tonistiigi tonistiigi Jul 18, 2019

Choose a reason for hiding this comment

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

nit: maybe just skip the test in rootless as this isn't expected behavior but current limitations

Copy link
Copy Markdown
Collaborator Author

@smira smira Jul 18, 2019

Choose a reason for hiding this comment

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

thanks, updated

@smira
Skip test in rootless mode
df52fc7 
Signed-off-by: Andrey Smirnov <smirnov.andrey@gmail.com>
smira added a commit to smira/talos that referenced this pull request Jul 18, 2019
@smira
chore: run tests in the buildkit itself
b0f16a3 
This relies on two PRs to the buildkit, which aren't merged yet, so I
had to do some overrides to apply them:

* moby/buildkit#1081
* moby/buildkit#1085

Signed-off-by: Andrey Smirnov <smirnov.andrey@gmail.com>
smira added a commit to smira/talos that referenced this pull request Jul 18, 2019
@smira
chore: run tests in the buildkit itself
127f4c9 
This relies on two PRs to the buildkit, which aren't merged yet, so I
had to do some overrides to apply them:

* moby/buildkit#1081
* moby/buildkit#1085

Signed-off-by: Andrey Smirnov <smirnov.andrey@gmail.com>
@smira smira mentioned this pull request Jul 18, 2019
Merged
@tiborvass
Copy link
Copy Markdown
Collaborator

tiborvass commented Jul 18, 2019

I tried to get the test to fail without the fix, but did not succeed.

@smira
Copy link
Copy Markdown
Collaborator Author

smira commented Jul 18, 2019

@tiborvass fails for me:

        require.go:794: 
            	Error Trace:	client_test.go:520
            	            				run.go:172
            	Error:      	Received unexpected error:
            	            	rpc error: code = Unknown desc = failed to build LLB: executor failed running [mkdir /sys/fs/cgroup/cpuset/securitytest]: process returned non-zero exit code: 1
            	            	failed to solve
            	            	github.com/moby/buildkit/client.(*Client).solve.func2
            	            		/src/client/solve.go:203
            	            	golang.org/x/sync/errgroup.(*Group).Go.func1
            	            		/src/vendor/golang.org/x/sync/errgroup/errgroup.go:58
            	            	runtime.goexit
            	            		/usr/local/go/src/runtime/asm_amd64.s:1337
            	Test:       	TestClientIntegration/TestSecurityModeSysfs/worker=containerd-1.0/secmode=insecure

@tiborvass
Copy link
Copy Markdown
Collaborator

tiborvass commented Jul 18, 2019

Yes, user error, my bad.

@tiborvass tiborvass merged commit fd2d8e6 into moby:master Jul 18, 2019
smira added a commit to smira/talos that referenced this pull request Jul 19, 2019
@smira
chore: run tests in the buildkit itself
c985195 
This relies on two PRs to the buildkit:

* moby/buildkit#1081
* moby/buildkit#1085

Sysfs fix was merged to upstream, so updated tag, while using
`Dockerfile` slug I can switch to dockerfile2llb with support for
`--security=insecure`.

Signed-off-by: Andrey Smirnov <smirnov.andrey@gmail.com>
andrewrynhard pushed a commit to siderolabs/talos that referenced this pull request Jul 19, 2019
@smira @andrewrynhard
chore: run tests in the buildkit itself
9f9acf1 
This relies on two PRs to the buildkit:

* moby/buildkit#1081
* moby/buildkit#1085

Sysfs fix was merged to upstream, so updated tag, while using
`Dockerfile` slug I can switch to dockerfile2llb with support for
`--security=insecure`.

Signed-off-by: Andrey Smirnov <smirnov.andrey@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tonistiigi tonistiigi tonistiigi approved these changes

@tiborvass tiborvass tiborvass approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@smira @tonistiigi @GordonTheTurtle @tiborvass