New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

devicemapper: remove container rootfs mountPath after umount #34573

Merged
merged 2 commits into from Nov 8, 2017

Conversation

@cyphar
Contributor

cyphar commented Aug 20, 2017

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

Walter by Alexander Lemke.

Closes #34542
Signed-off-by: Aleksa Sarai asarai@suse.de

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

@cyphar

This comment has been minimized.

Show comment
Hide comment
@cyphar

cyphar Aug 20, 2017

Contributor

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).

Contributor

cyphar commented Aug 20, 2017

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

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Aug 21, 2017

Member

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

Member

dnephin commented Aug 21, 2017

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

Show outdated Hide outdated integration/util/kernel.go
Show outdated Hide outdated integration/container/run_test.go
Show outdated Hide outdated integration/container/run_test.go
@cyphar

This comment has been minimized.

Show comment
Hide comment
@cyphar

cyphar Aug 21, 2017

Contributor

@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.]

Contributor

cyphar commented Aug 21, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Aug 21, 2017

Member

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

Member

dnephin commented Aug 21, 2017

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

This comment has been minimized.

Show comment
Hide comment
@rhvgoyal

rhvgoyal Aug 21, 2017

Contributor

@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?

Contributor

rhvgoyal commented Aug 21, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@rhvgoyal

rhvgoyal Aug 21, 2017

Contributor
Contributor

rhvgoyal commented Aug 21, 2017

@cyphar

This comment has been minimized.

Show comment
Hide comment
@cyphar

cyphar Aug 25, 2017

Contributor

@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).

Contributor

cyphar commented Aug 25, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@cyphar

cyphar Aug 31, 2017

Contributor

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.

Contributor

cyphar commented Aug 31, 2017

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.

@cyphar

This comment has been minimized.

Show comment
Hide comment
@cyphar

cyphar Sep 4, 2017

Contributor

It looks like our linter is giving errors about the standard library:

01:47:56 ../../../../../usr/local/go/src/os/user/lookup.go:38:9:warning: undeclared name: listGroups (unconvert)
01:47:56 ../../../../../usr/local/go/src/os/user/lookup.go:9:9:warning: undeclared name: current (unconvert)
01:47:56 ../../../../../usr/local/go/src/os/user/lookup.go:15:9:warning: undeclared name: lookupUser (unconvert)
01:47:56 ../../../../../usr/local/go/src/os/user/lookup.go:21:9:warning: undeclared name: lookupUserId (unconvert)
01:47:56 ../../../../../usr/local/go/src/os/user/lookup.go:27:9:warning: undeclared name: lookupGroup (unconvert)
01:47:56 ../../../../../usr/local/go/src/os/user/lookup.go:33:9:warning: undeclared name: lookupGroupId (unconvert)
Contributor

cyphar commented Sep 4, 2017

It looks like our linter is giving errors about the standard library:

01:47:56 ../../../../../usr/local/go/src/os/user/lookup.go:38:9:warning: undeclared name: listGroups (unconvert)
01:47:56 ../../../../../usr/local/go/src/os/user/lookup.go:9:9:warning: undeclared name: current (unconvert)
01:47:56 ../../../../../usr/local/go/src/os/user/lookup.go:15:9:warning: undeclared name: lookupUser (unconvert)
01:47:56 ../../../../../usr/local/go/src/os/user/lookup.go:21:9:warning: undeclared name: lookupUserId (unconvert)
01:47:56 ../../../../../usr/local/go/src/os/user/lookup.go:27:9:warning: undeclared name: lookupGroup (unconvert)
01:47:56 ../../../../../usr/local/go/src/os/user/lookup.go:33:9:warning: undeclared name: lookupGroupId (unconvert)
devicemapper: remove container rootfs mountPath after umount
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.

Signed-off-by: Aleksa Sarai <asarai@suse.de>
@cyphar

This comment has been minimized.

Show comment
Hide comment
@cyphar

cyphar Sep 6, 2017

Contributor

@rhvgoyal I've tested --live-restore quite a bit and it looks like it doesn't affect anything. It looks like Docker doesn't do any graphdriver cleanups (including Put) if you have --live-restore. That makes sense because otherwise docker would hit a bunch of errors when you're using --live-restore. So AFAICS there aren't any problems in that regard.

Contributor

cyphar commented Sep 6, 2017

@rhvgoyal I've tested --live-restore quite a bit and it looks like it doesn't affect anything. It looks like Docker doesn't do any graphdriver cleanups (including Put) if you have --live-restore. That makes sense because otherwise docker would hit a bunch of errors when you're using --live-restore. So AFAICS there aren't any problems in that regard.

@cyphar

This comment has been minimized.

Show comment
Hide comment
@cyphar

cyphar Sep 6, 2017

Contributor

@dnephin Because of the weird errors that look like the linters aren't doing the right thing, I've just opted to not add a test for it. I would've preferred being able to add a test, but we can follow up with my test after the lint issue is resolved.

Contributor

cyphar commented Sep 6, 2017

@dnephin Because of the weird errors that look like the linters aren't doing the right thing, I've just opted to not add a test for it. I would've preferred being able to add a test, but we can follow up with my test after the lint issue is resolved.

cyphar added a commit to SUSE/docker that referenced this pull request Sep 6, 2017

devicemapper: remove container rootfs mountPath after umount
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 <asarai@suse.de>

cyphar added a commit to SUSE/docker that referenced this pull request Sep 6, 2017

devicemapper: remove container rootfs mountPath after umount
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 <asarai@suse.de>
@dnephin

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Sep 6, 2017

Member

What lint errors?

Member

dnephin commented Sep 6, 2017

What lint errors?

@cyphar

This comment has been minimized.

Show comment
Hide comment
@cyphar

cyphar Sep 6, 2017

Contributor

@dnephin These ones. You can't see them anymore because I dropped the test commit. Here's the full build log before I removed the commit:

10:23:49 daemon/stats/collector.go:83:22:warning: invalid operation: s (variable of type *Collector) has no field or method getNumberOnlineCPUs (unconvert)
10:23:49 ../../../../../usr/local/go/src/os/user/lookup.go:38:9:warning: undeclared name: listGroups (unconvert)
10:23:49 ../../../../../usr/local/go/src/os/user/lookup.go:9:9:warning: undeclared name: current (unconvert)
10:23:49 ../../../../../usr/local/go/src/os/user/lookup.go:15:9:warning: undeclared name: lookupUser (unconvert)
10:23:49 ../../../../../usr/local/go/src/os/user/lookup.go:21:9:warning: undeclared name: lookupUserId (unconvert)
10:23:49 ../../../../../usr/local/go/src/os/user/lookup.go:27:9:warning: undeclared name: lookupGroup (unconvert)
10:23:49 ../../../../../usr/local/go/src/os/user/lookup.go:33:9:warning: undeclared name: lookupGroupId (unconvert)
10:23:49 daemon/stats/collector.go:77:23:warning: invalid operation: s (variable of type *Collector) has no field or method getSystemCPUUsage (unconvert)
10:23:49 daemon/stats/types.go:27:2:warning: undeclared name: platformNewStatsCollector (unconvert)
Contributor

cyphar commented Sep 6, 2017

@dnephin These ones. You can't see them anymore because I dropped the test commit. Here's the full build log before I removed the commit:

10:23:49 daemon/stats/collector.go:83:22:warning: invalid operation: s (variable of type *Collector) has no field or method getNumberOnlineCPUs (unconvert)
10:23:49 ../../../../../usr/local/go/src/os/user/lookup.go:38:9:warning: undeclared name: listGroups (unconvert)
10:23:49 ../../../../../usr/local/go/src/os/user/lookup.go:9:9:warning: undeclared name: current (unconvert)
10:23:49 ../../../../../usr/local/go/src/os/user/lookup.go:15:9:warning: undeclared name: lookupUser (unconvert)
10:23:49 ../../../../../usr/local/go/src/os/user/lookup.go:21:9:warning: undeclared name: lookupUserId (unconvert)
10:23:49 ../../../../../usr/local/go/src/os/user/lookup.go:27:9:warning: undeclared name: lookupGroup (unconvert)
10:23:49 ../../../../../usr/local/go/src/os/user/lookup.go:33:9:warning: undeclared name: lookupGroupId (unconvert)
10:23:49 daemon/stats/collector.go:77:23:warning: invalid operation: s (variable of type *Collector) has no field or method getSystemCPUUsage (unconvert)
10:23:49 daemon/stats/types.go:27:2:warning: undeclared name: platformNewStatsCollector (unconvert)
@dnephin

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Sep 6, 2017

Member

hmm, not sure what that's about. What did you change to remove them?

If you have a commit that can reproduce can you push it here , or link me to it?

Member

dnephin commented Sep 6, 2017

hmm, not sure what that's about. What did you change to remove them?

If you have a commit that can reproduce can you push it here , or link me to it?

@cyphar

This comment has been minimized.

Show comment
Hide comment
@cyphar

cyphar Sep 6, 2017

Contributor

@dnephin Here's a branch with the top commit being the one that causes the error. I fixed the issue by just dropping that commit, I'm not sure which part of that commit was causing the issue. https://github.com/cyphar/docker/tree/docker-lint-error

Contributor

cyphar commented Sep 6, 2017

@dnephin Here's a branch with the top commit being the one that causes the error. I fixed the issue by just dropping that commit, I'm not sure which part of that commit was causing the issue. https://github.com/cyphar/docker/tree/docker-lint-error

@rhvgoyal

This comment has been minimized.

Show comment
Hide comment
@rhvgoyal

rhvgoyal Sep 6, 2017

Contributor

@cyphar This patch LGTM.

If need be, we can make similar changes in Remove() path later.

Contributor

rhvgoyal commented Sep 6, 2017

@cyphar This patch LGTM.

If need be, we can make similar changes in Remove() path later.

@dnephin

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Sep 7, 2017

Member

@cyphar I've seen those lint errors happen on occasionally on my branch as well. I think it's a weird error with jenkins, but doesn't have anything to do with your test code. If you build it again they should be gone.

Member

dnephin commented Sep 7, 2017

@cyphar I've seen those lint errors happen on occasionally on my branch as well. I think it's a weird error with jenkins, but doesn't have anything to do with your test code. If you build it again they should be gone.

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Oct 16, 2017

Contributor

ping @kolyshkin

Contributor

cpuguy83 commented Oct 16, 2017

ping @kolyshkin

@kolyshkin

This comment has been minimized.

Show comment
Hide comment
@kolyshkin

kolyshkin Oct 18, 2017

Contributor

LGTM

Contributor

kolyshkin commented Oct 18, 2017

LGTM

@vieux

This comment has been minimized.

Show comment
Hide comment
@vieux

vieux Oct 19, 2017

Collaborator

@cyphar any way to add a test ?

Collaborator

vieux commented Oct 19, 2017

@cyphar any way to add a test ?

@cyphar

This comment has been minimized.

Show comment
Hide comment
@cyphar

cyphar Oct 19, 2017

Contributor

@vieux The only way I could see to add a test would be to try to emulate what happens when you create a container's rootfs and then remove it. But in order to test the actual regression you'd need to create a container with MS_PRIVATE rootfs propagation that includes the mount (something that I don't feel is a good idea to write in Go given the historical issues with unshare(2) and Go).

I used to have an integration test for this, but the integration-cli suite has been deprecated, so I'd have to use the now-new-again integration suite which proved too tiresome to reinvent the wheel as well as not being clear whether it actually is testing what we care about (since I can't force the test to run on devicemapper).

Contributor

cyphar commented Oct 19, 2017

@vieux The only way I could see to add a test would be to try to emulate what happens when you create a container's rootfs and then remove it. But in order to test the actual regression you'd need to create a container with MS_PRIVATE rootfs propagation that includes the mount (something that I don't feel is a good idea to write in Go given the historical issues with unshare(2) and Go).

I used to have an integration test for this, but the integration-cli suite has been deprecated, so I'd have to use the now-new-again integration suite which proved too tiresome to reinvent the wheel as well as not being clear whether it actually is testing what we care about (since I can't force the test to run on devicemapper).

cyphar added a commit to SUSE/docker that referenced this pull request Oct 21, 2017

devicemapper: remove container rootfs mountPath after umount
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 <asarai@suse.de>

cyphar added a commit to SUSE/docker that referenced this pull request Oct 21, 2017

devicemapper: remove container rootfs mountPath after umount
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 <asarai@suse.de>
@cyphar

This comment has been minimized.

Show comment
Hide comment
@cyphar

cyphar Oct 25, 2017

Contributor

nudge

Contributor

cyphar commented Oct 25, 2017

nudge

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Oct 25, 2017

Contributor

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

Contributor

cpuguy83 commented Oct 25, 2017

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

@cyphar

This comment has been minimized.

Show comment
Hide comment
@cyphar

cyphar Oct 25, 2017

Contributor

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... 😞]

Contributor

cyphar commented Oct 25, 2017

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

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Oct 25, 2017

Member

Why would it break other unit tests?

Member

dnephin commented Oct 25, 2017

Why would it break other unit tests?

@cyphar

This comment has been minimized.

Show comment
Hide comment
@cyphar

cyphar Nov 7, 2017

Contributor

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

/cc @vieux @cpuguy83

Contributor

cyphar commented Nov 7, 2017

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

/cc @vieux @cpuguy83

devmapper: add a test for mount leak workaround
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 <asarai@suse.de>
@cyphar

This comment has been minimized.

Show comment
Hide comment
@cyphar

cyphar Nov 8, 2017

Contributor

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

Contributor

cyphar commented Nov 8, 2017

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

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Nov 8, 2017

Member

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

Member

thaJeztah commented Nov 8, 2017

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

@cpuguy83

LGTM

Thanks @cyphar!

@cyphar

This comment has been minimized.

Show comment
Hide comment
@cyphar

cyphar Nov 8, 2017

Contributor
Contributor

cyphar commented Nov 8, 2017

@vdemeester

LGTM 🐮

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

6 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 37760 has succeeded
Details
janky Jenkins build Docker-PRs 46468 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 6879 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 18030 has succeeded
Details
z Jenkins build Docker-PRs-s390x 6686 has succeeded
Details
@dnephin

Thanks for writing the unit test

@cyphar cyphar deleted the cyphar:dm-dos-prevention-remove-mountpoint branch Nov 8, 2017

@cyphar

This comment has been minimized.

Show comment
Hide comment
@cyphar

cyphar Nov 8, 2017

Contributor

@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.

Contributor

cyphar commented Nov 8, 2017

@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.

cyphar added a commit to SUSE/docker that referenced this pull request Nov 23, 2017

devicemapper: remove container rootfs mountPath after umount
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 <asarai@suse.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment