runc

runc copied to clipboard

Without that if condition runc creates a bunch of mounts for non NEWNS containers and don't clean after all.

bug was introduced in 91ca331474b6b7859a85678d8a4dd4b876345ddd

(cc: @crosbymichael)

I get that runc will create mounts in the host mount namespace, but they are all inside the chroot right? The only other option would be to just fail if you have mounts specified with no NEWNS.

The only other option would be to just fail if you have mounts specified with no NEWNS.

Makes sense

@crosbymichael do you have an opinion what is the right way to proceed?

@cyphar I'm not really sure about our next step here, do I need to add some validation and raise an error if we have mounts specified with no NEWNS or we're ok to land the diff as is or do we want to wait for @crosbymichael ?

also, to fix travis somebody needs to update the job:

The command "go get -u github.com/golang/lint/golint" failed and exited with 1 during .

The root cause is:

 ➜  go get -u github.com/golang/lint/golint
package github.com/golang/lint/golint: code in directory /Users/verm666/.go/src/github.com/golang/lint/golint expects import "golang.org/x/lint/golint"

@verm666 the travis fix is already merged, https://github.com/opencontainers/runc/pull/1908

You can rebase or just tweak your commit and push -f, I don't have access or i'd just hit the restart button for ya :-)

looking

What if others depend on this functionality? What is the point of preventing it?

What is the point of preventing it?

Each container restart produces additional "leaked" mounts, and eventually you can see in mount(8) output thousands of lines. It's not critical, it doesn't affect containers in any bad way but it seems like a bug for me.

But givin the configuration, that is expected and should be handled. Atleast that is my POV, what do you think @cyphar ?

Thanks @mikebrow will rebase!

I agree with @crosbymichael -- you are asking us to mount things in a chroot setup. This is far less than ideal, and I would tell people to simply not do this (after all, this is not a secure way to set up a container) but I don't see the benefit of disabling it entirely.

If there was an underlying security issue (like we were mounting over host paths) then obviously we'd need to do something about it, but I'm not so sure this is as significant of an issue. Though, there is an argument that there are some rename-based chroot attacks which might cause some worry...

@verm666 my rebase a pr flow:

first rebase master

$ git checkout master $ git fetch upstream master --tags $ git merge upstream/master $ git push origin master

Then checkout your pr's branch and rebase that pr against master: $ git checkout yourBranch $ git rebase master Follow rebase instructions for example if there was a merge conflict you'll need to edit the conflict file: $ atom confictFile $ git add conflictFile $ git rebase --continue

once rebase is complete: $ git push -f origin yourBranch

You need to add a Signed-off-by: line to your commit(s) which indicates that you attest the Developer Certificate of Origin a statement about your contributions that you must read before signing (don't worry, it's quite short and easy-to-read). You can add it to your commits with git commit --amend -s, and then doing a git push --force.

(Note that I'm still not in favour of this change -- I'm just telling you what is causing the CI to fail.)