devicemapper: remove container rootfs mountPath after umount #34573

cyphar · 2017-08-20T04:14:05Z

libdm currently has a fairly substantial DoS bug that makes certain
operations fail on a libdm device if the device has active references
through mountpoints. This is a significant problem with the advent of
mount namespaces and MS_PRIVATE, and can cause certain --volume mounts
to cause libdm to no longer be able to remove containers:

% docker run -d --name testA busybox top
% docker run -d --name testB -v /var/lib/docker:/docker busybox top
% docker rm -f testA
[fails on libdm with dm_task_run errors.]

This also solves the problem of unprivileged users being able to DoS
docker by using unprivileged mount namespaces to preseve mounts that
Docker has dropped.

Walter by Alexander Lemke.

Closes #34542
Signed-off-by: Aleksa Sarai [email protected]

/cc @rhvgoyal This is an alternative to #34542
/cc @vrothberg

cyphar · 2017-08-20T17:15:53Z

It's a bit of a pain that Jenkins doesn't test any graphdrivers other than vfs for the integration tests. Is there anything we can do about that @dnephin? I've tested it locally (and I bet that @rhvgoyal will also want to).

dnephin · 2017-08-21T14:49:40Z

We should have separate suites for testing the graph drivers, so we can run the suite for any supported drivers.

dnephin · 2017-08-21T14:59:09Z

integration/util/kernel.go

+//   -1 if lval < rval
+//    0 if lval = rval
+//    1 if lval > rval
+func MustKernelCmp(rval kernel.VersionInfo) int {


Could this be inlined into run_test.go? Do you expect to use it outside of container tests?

dnephin · 2017-08-21T15:01:17Z

integration/container/run_test.go

+	// tested ocurred). We run it anyway just to be safe.
+	driver, _ := daemonStorageDriver(context.Background(), client)
+	if driver != "devicemapper" {
+		t.Logf("Active driver (%s) is not devicemapper, continuing anyway.", driver)


I think this should t.Skip() if it's not devicemapper.

I think it would be a good measure to make sure the bug doesn't happen on other storage drivers too. I should probably just remove this check all-together.

I don't think this is the right way to test storage drivers. These "integration" tests are very slow, we need to be conservative about the test we add here (see TESTING).

We should be able to test storage drivers without the daemon running, which makes it much easier and faster to test them. It looks like each graphdriver has a test suite which uses graphtest. We should test this case in that suite.

Why is it not possible to just check that the mount point doesn't exist anymore from that test suite?

I would need to check again, but it's not that it's impossible, it's that I'd have to effectively start re-implementing parts of runc inside the test framework to make sure we're testing the right interaction between mount namespaces and libdm.

Just checking if the mountpoint has gone isn't good enough, you need to check that the mount has disappeared in other namespaces (that's where the bug comes from).

dnephin · 2017-08-21T15:02:01Z

integration/container/run_test.go

+	// torvalds/linux@8ed936b5671bfb33d89bc60bdcc7cf0470ba52fe was merged).
+	oldKernel := util.MustKernelCmp(kernel.VersionInfo{Kernel: 3, Major: 18, Minor: 0}) < 0
+	if oldKernel {
+		t.Logf("NOTE: This test may fail on your <3.18 kernels.")


Again, probably t.Skip()

Some distributions may have backported the fix, so the kernel version alone isn't enough to know whether the test will work (but we don't hard fail on older kernels).

You can use kernel.CheckKernelVersion from github.com/docker/docker/pkg/parsers/kernel here. For example:

import "github.com/docker/docker/pkg/parsers/kernel" ... oldKernel := !kernel.CheckKernelVersion(3, 18, 0) ...

cyphar · 2017-08-21T15:22:36Z

@dnephin So where does that leave this PR's tests? Should I not include tests at all (these tests actually currently fail silently because the error handling in the new integration scheme doesn't exist yet -- and we'll eventually need to recreate the old integration-cli testing setup), or should I start working on making it possible to have devicemapper integration tests?

[As an aside, I'm not sure it was a good idea to deprecate integration-cli while integration doesn't have any significant infrastructure for testing, not to mention that integration-cli has three orders of magnitude more tests that we can't touch anymore because of the deprecation warnings.]

dnephin · 2017-08-21T16:07:57Z

these tests actually currently fail silently because the error handling in the new integration scheme doesn't exist yet

I don't understand this. What do you mean by error handling doesn't exist?

while integration doesn't have any significant infrastructure for testing

It's a work in progress. If you see something significant that is missing, please let me know.

not to mention that integration-cli has three orders of magnitude more tests that we can't touch anymore because of the deprecation warnings

You can change tests, you just can't add new ones

So where does that leave this PR's tests?

The ideal place for this test would be in daemon/graphdriver/devmapper/devmapper_test.go. I don't understand yet why it's not possible to exercise the bug there. If it's not possible I think what you have now is the right idea, but we should skip it when the graphdriver is not devicemapper

rhvgoyal · 2017-08-21T20:25:39Z

daemon/graphdriver/devmapper/deviceset.go

+	if err := system.EnsureRemoveAll(mountPath); err != nil {
+		logrus.Debugf("devmapper: error doing a removeall on unmounted device %s: %v", mountPath, err)
+	}
+


Why use EnsureRemoveAll? This function is trying to do so much that I don't even understand what it is doing. After lazy unmount, why don't we simply do a os.Remove() ?

So idea is that use removing directory to yank the mount points which might have leaked into other mount namespaces and this will only work on newer kernels. I think this should work reasonably well. Little concerned about some use case being broken somewhere. There have been quite a few changes w.r.t referece counting. Especially make sure that the case of docker daemon being shutdown/start works while containers were running the whole time.

Yeah, os.Remove is probably a better choice.

Especially make sure that the case of docker daemon being shutdown/start works while containers were running the whole time.

Can you explain what you mean here? Are you referring to the "live restore" stuff?

Yes, I was referring to live restore functionality.

rhvgoyal · 2017-08-21T20:26:42Z

daemon/graphdriver/devmapper/deviceset.go

@@ -2416,6 +2417,15 @@ func (devices *DeviceSet) UnmountDevice(hash, mountPath string) error {
 	}
 	logrus.Debug("devmapper: Unmount done")

+	// Remove the mountpoint here. This is necessary to avoid cases where a


It might be good to tell that removing directory will remove the mount point on this directory from all the mount namespaces.

rhvgoyal · 2017-08-21T20:30:39Z

@cyphar We will require similar change in Remove() path as well? That is, make sure device is unmounted, then remove mnt directory and then try device deactivation and deletion?

rhvgoyal · 2017-08-21T21:02:59Z

cc @rhatdan

cyphar · 2017-08-25T09:02:41Z

@rhvgoyal Yes probably. I'll need to figure out what's the best place to put this change (I'd prefer to not have to duplicate it in both paths).

cyphar · 2017-08-31T16:11:18Z

Actually @rhvgoyal I don't think we need this in the Remove path. The Remove path is only called from the driver after it's been Put and removed (and Put calls UnmountDevice which removes the mountpoint). I'm testing the live restore stuff now.

libdm currently has a fairly substantial DoS bug that makes certain operations fail on a libdm device if the device has active references through mountpoints. This is a significant problem with the advent of mount namespaces and MS_PRIVATE, and can cause certain --volume mounts to cause libdm to no longer be able to remove containers: % docker run -d --name testA busybox top % docker run -d --name testB -v /var/lib/docker:/docker busybox top % docker rm -f testA [fails on libdm with dm_task_run errors.] This also solves the problem of unprivileged users being able to DoS docker by using unprivileged mount namespaces to preseve mounts that Docker has dropped. SUSE-Bug: https://bugzilla.suse.com/show_bug.cgi?id=1045628 SUSE-Backport: moby#34573 Signed-off-by: Aleksa Sarai <[email protected]>

cyphar · 2017-10-25T08:18:43Z

nudge

cpuguy83 · 2017-10-25T14:52:48Z

Maybe a unit test that does uses unshare(1), and can skip if it's not available. Not exactly ideal, but ...

cyphar · 2017-10-25T15:40:17Z

The problem with such a test is that unshare would possibly break other unit tests. I'll just write a test to make sure that the mountpoint was deleted, I guess...

[Again, this would not be a problem if I could just write a normal integration test... 😞]

dnephin · 2017-10-25T15:54:15Z

Why would it break other unit tests?

cyphar · 2017-11-07T23:41:48Z

@dnephin I've added a unit test in graphdriver/devmapper/....

/cc @vieux @cpuguy83

In order to avoid reverting our fix for mount leakage in devicemapper, add a test which checks that devicemapper's Get() and Put() cycle can survive having a command running in an rprivate mount propagation setup in-between. While this is quite rudimentary, it should be sufficient. We have to skip this test for pre-3.18 kernels. Signed-off-by: Aleksa Sarai <[email protected]>

cyphar · 2017-11-08T10:03:16Z

ppc failure is not related to my change, it looks like some test wrapper broke in the middle of running the suite.

thaJeztah · 2017-11-08T13:22:33Z

Restarted PowerPC; I was told there were some connection issue with Jenkins and the PowerPC nodes

cpuguy83

LGTM

Thanks @cyphar!

cyphar · 2017-11-08T16:03:53Z

/cc @vdemeester

vdemeester

LGTM 🐮

dnephin

Thanks for writing the unit test

cyphar · 2017-11-08T16:55:38Z

@dnephin Sorry, I never answered your question. I was worried about using Unshare in the context of a Go program (historically that's caused nothing but trouble, and I'd argue that it's not solveable in Go because it gives you so little information about the threads that your program runs on). But in the final patch state, I just ended up using os/exec which has support for specifying Unshare and Clone flags that get applied correctly. It's a bit of a shame that it depends on running subcommands, but there's no other nice way of testing it.

libdm currently has a fairly substantial DoS bug that makes certain operations fail on a libdm device if the device has active references through mountpoints. This is a significant problem with the advent of mount namespaces and MS_PRIVATE, and can cause certain --volume mounts to cause libdm to no longer be able to remove containers: % docker run -d --name testA busybox top % docker run -d --name testB -v /var/lib/docker:/docker busybox top % docker rm -f testA [fails on libdm with dm_task_run errors.] This also solves the problem of unprivileged users being able to DoS docker by using unprivileged mount namespaces to preseve mounts that Docker has dropped. SUSE-Bug: https://bugzilla.suse.com/show_bug.cgi?id=1045628 SUSE-Backport: moby#34573 Signed-off-by: Aleksa Sarai <[email protected]>

GordonTheTurtle added the status/0-triage label Aug 20, 2017

cyphar force-pushed the dm-dos-prevention-remove-mountpoint branch from 41b98d1 to a7f8155 Compare August 20, 2017 04:16

cyphar requested review from dnephin and vdemeester as code owners August 20, 2017 04:16

cyphar force-pushed the dm-dos-prevention-remove-mountpoint branch 7 times, most recently from 9934083 to a875ebf Compare August 20, 2017 17:13

vdemeester added status/2-code-review and removed status/0-triage labels Aug 21, 2017

dnephin reviewed Aug 21, 2017

View reviewed changes

rhvgoyal reviewed Aug 21, 2017

View reviewed changes

cyphar mentioned this pull request Aug 21, 2017

devicemapper: protect mounts against volume leaks #34542

Closed

cyphar force-pushed the dm-dos-prevention-remove-mountpoint branch from a875ebf to ba9ee05 Compare August 31, 2017 16:01

cyphar force-pushed the dm-dos-prevention-remove-mountpoint branch 4 times, most recently from ef75b39 to 330461c Compare September 1, 2017 15:13

cyphar mentioned this pull request Oct 24, 2017

Fix EBUSY errors under overlayfs and v4.13+ kernels #34948

Merged

cyphar force-pushed the dm-dos-prevention-remove-mountpoint branch 4 times, most recently from 262017f to 2ee2f84 Compare November 7, 2017 23:40

cyphar force-pushed the dm-dos-prevention-remove-mountpoint branch from 2ee2f84 to 1af8ea6 Compare November 8, 2017 00:02

thaJeztah added the rebuild/powerpc label Nov 8, 2017

GordonTheTurtle removed the rebuild/powerpc label Nov 8, 2017

cpuguy83 approved these changes Nov 8, 2017

View reviewed changes

vdemeester approved these changes Nov 8, 2017

View reviewed changes

vdemeester merged commit bbc4f78 into moby:master Nov 8, 2017

dnephin reviewed Nov 8, 2017

View reviewed changes

cyphar deleted the dm-dos-prevention-remove-mountpoint branch November 8, 2017 16:25

thaJeztah mentioned this pull request Nov 17, 2017

[17.09] backport dm dos prevention remove mountpoint docker-archive/docker-ce#319

Merged

euank mentioned this pull request Nov 28, 2017

Docker-cli cannot start images in linux kernel >= 4.12 #34283

Closed

tsoliangwu0130 mentioned this pull request Jan 4, 2018

Stop contianer first then remove osu-mist/ansible-roles#37

Merged

cyphar mentioned this pull request Jan 25, 2018

Do not make graphdriver homes private mounts. #36047

Merged

kolyshkin mentioned this pull request Sep 15, 2020

Devmapper fixes and cleanups containers/storage#719

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

devicemapper: remove container rootfs mountPath after umount #34573

devicemapper: remove container rootfs mountPath after umount #34573

cyphar commented Aug 20, 2017 •

edited

Loading

cyphar commented Aug 20, 2017 •

edited

Loading

dnephin commented Aug 21, 2017

dnephin Aug 21, 2017

dnephin Aug 21, 2017

cyphar Aug 21, 2017

dnephin Aug 21, 2017

cyphar Aug 21, 2017 •

edited

Loading

dnephin Aug 21, 2017

cyphar Aug 21, 2017

kolyshkin Aug 28, 2017

cyphar commented Aug 21, 2017 •

edited

Loading

dnephin commented Aug 21, 2017

rhvgoyal Aug 21, 2017

cyphar Aug 25, 2017

rhvgoyal Aug 25, 2017

rhvgoyal Aug 21, 2017

rhvgoyal commented Aug 21, 2017

rhvgoyal commented Aug 21, 2017

cyphar commented Aug 25, 2017 •

edited

Loading

cyphar commented Aug 31, 2017

cyphar commented Oct 25, 2017

cpuguy83 commented Oct 25, 2017

cyphar commented Oct 25, 2017

dnephin commented Oct 25, 2017

cyphar commented Nov 7, 2017

cyphar commented Nov 8, 2017

thaJeztah commented Nov 8, 2017

cpuguy83 left a comment

cyphar commented Nov 8, 2017

vdemeester left a comment

dnephin left a comment

cyphar commented Nov 8, 2017 •

edited

Loading

devicemapper: remove container rootfs mountPath after umount #34573

devicemapper: remove container rootfs mountPath after umount #34573

Conversation

cyphar commented Aug 20, 2017 • edited Loading

Walter by Alexander Lemke.

cyphar commented Aug 20, 2017 • edited Loading

dnephin commented Aug 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyphar Aug 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyphar commented Aug 21, 2017 • edited Loading

dnephin commented Aug 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhvgoyal commented Aug 21, 2017

rhvgoyal commented Aug 21, 2017

cyphar commented Aug 25, 2017 • edited Loading

cyphar commented Aug 31, 2017

cyphar commented Oct 25, 2017

cpuguy83 commented Oct 25, 2017

cyphar commented Oct 25, 2017

dnephin commented Oct 25, 2017

cyphar commented Nov 7, 2017

cyphar commented Nov 8, 2017

thaJeztah commented Nov 8, 2017

cpuguy83 left a comment

Choose a reason for hiding this comment

cyphar commented Nov 8, 2017

vdemeester left a comment

Choose a reason for hiding this comment

dnephin left a comment

Choose a reason for hiding this comment

cyphar commented Nov 8, 2017 • edited Loading

cyphar commented Aug 20, 2017 •

edited

Loading

cyphar commented Aug 20, 2017 •

edited

Loading

cyphar Aug 21, 2017 •

edited

Loading

cyphar commented Aug 21, 2017 •

edited

Loading

cyphar commented Aug 25, 2017 •

edited

Loading

cyphar commented Nov 8, 2017 •

edited

Loading