I'm working on fixing this issue, which doesn't happen on my local machine.

11:47:18 ----------------------------------------------------------------------
11:47:18 FAIL: docker_cli_run_unix_test.go:1579: DockerSuite.TestRunDevicemapperVolumeLeakage
11:47:18 
11:47:18 docker_cli_run_unix_test.go:1588:
11:47:18     c.Assert(err, check.IsNil)
11:47:18 ... value *errors.errorString = &errors.errorString{s:"\nCommand:  /usr/local/bin/docker rm -f testA\nExitCode: 1\nError:    exit status 1\nStdout:   \nStderr:   Error response from daemon: unable to remove filesystem for c1490c494a3cc207ff208f4e1b588335bc9ecc454139102f491f7e675c5f2cab: remove /var/lib/docker/containers/c1490c494a3cc207ff208f4e1b588335bc9ecc454139102f491f7e675c5f2cab/shm: device or resource busy\n\n\nFailures:\nExitCode was 1 expected 0\nExpected no error\n"} ("\nCommand:  /usr/local/bin/docker rm -f testA\nExitCode: 1\nError:    exit status 1\nStdout:   \nStderr:   Error response from daemon: unable to remove filesystem for c1490c494a3cc207ff208f4e1b588335bc9ecc454139102f491f7e675c5f2cab: remove /var/lib/docker/containers/c1490c494a3cc207ff208f4e1b588335bc9ecc454139102f491f7e675c5f2cab/shm: device or resource busy\n\n\nFailures:\nExitCode was 1 expected 0\nExpected no error\n")

What is the kernel version on Jenkins? Linux upstream has a patch in newer kernels (>3.18) that makes rmdir on a mountpoint that is referenced in another namespace no longer give EBUSY (because it made machines susceptible to DoS) -- see torvalds/linux@8ed936b5671b ("vfs: Lazily remove mounts on unlinked files and directories.").

I believe this is the reason the test fails on older kernels. I'm investigating a workaround for older kernels but it's actually quite difficult to make /dev/shm unbindable -- because it's main usage is bindmounting into containers.

@cyphar I can't see any of the x86_64 kernel versions, (ping @psftw) but the ppc64le ones are 4.4.0-x where it did pass.

/cc @runcom @mrunalp You probably care about this for containers/storage. There are also a whole host of libdm issues where you can DoS a system using it, but those require kernel intervention.

@rhvgoyal @nalind PTAL

@cyphar Can you explain a little bit how did it prevent mount point leaks into container? I could not grok it by reading commit message yet.

So this is primarily a problem on older kernels, right? As you mentioned newer kernels will remove mounts from other mount namespaces when directory in daemon mount namespace is removed.

We are taking care of it using a prestart plugin called oci-umount. That plugin does the unmounting. But if this PR takes care of it, then I will gladly get rid of oci-umount plugin.

https://github.com/projectatomic/oci-umount

Aha, you are making use of unbindable mounts. That sounds very interesting. Let me play with it.

So first side affect of making /var/lib/docker/containers unbindable is that no container can bind mount it in. I mean -v /var/lib/docker/containers:/foo/ will fail. I know that fluentd container does this opearation in an attempt to look at container logs. But they probably will have to move away from that. Either make use of API or bind mount /var/lib/docker/ in.

@rhvgoyal

So this is primarily a problem on older kernels, right? As you mentioned newer kernels will remove mounts from other mount namespaces when directory in daemon mount namespace is removed.

There are two issues at play here:

1. Our libdm usage needs to use MS_UNBINDABLE because it doesn't correctly handle leaked mounts (as we've all collectively discovered in other recent libdm issues) so anyone who wanted to bind-mount /var/lib/docker/devicemapper is out-of-luck. Basically if you keep a copy of the libdm mount alive, then Docker won't be able to clean up the libdm device (and deferred deletion and removal don't appear to help either). This happens on all kernels, and I'm just making /var/lib/docker/devicemapper use MS_UNBINDABLE.

2. The reason why the tests are currently failing is because the test machines have an older kernel that doesn't have torvalds/linux@8ed936b5671b applied. As a result, docker rm -f is failing because the /dev/shm mount has been leaked to the second container in my test. On newer kernels that isn't a problem (the mount is killed when Docker attempts an rmdir on the shm mountpoint), but on older kernels you get EBUSY.

So (1) is entirely solved by this current patchset, and I'm trying to figure out how to solve (2). My first idea for (2) was to make /var/lib/docker/containers also unbindable, but I've also come to the conclusion that making /var/lib/docker/containers unbindable is a bad idea for multiple reasons (the most obvious of which is that it completely breaks resolv.conf, hostname and hosts because you can't bind-mount them anymore -- you can't bind-mount anything that's a inside of a MS_BINDABLE mount). Really, the fact the tests fail on 14.04 is because it's a kernel bug that hasn't been fixed in Ubuntu's kernels, and I'm not sure that significant workarounds are going to be worth it (especially since currently -v /:/rootfs completely breaks on all kernels and setups). Maybe we should just make the test ignore shm failures and just warn on them?

/cc @cpuguy83

oh, and @kolyshkin probably 😄

@cyphar for the first problem, I think deferred deletion already takes care of it?

In Remove() flow, deferred deletion will delete device and then we will go on to remove container rootfs directory. That will yank the leaked mount point. And that means soon deferred deletion thread will claim the deferred deleted device.

Even if one is not using deferred deletion, I think that might be fixable on new kernels. Say a device mount point has leaked into a container and then we try to remove that device. Device deletion fails as device is busy. Can't we first remove the directory containing container rootfs mount and then call into libdm to delete device. Removing directory will remove leaked mount point and after that device deletion will succeed.

So flow will be something like.

Remove() (In driver.go)
deactivateDevice()
system.EnsureRemoveAll(mp)
DeleteDevice()
}

But I also like the idea of making /var/log/docker/devicemapper unbindable. Not sure what else it will break. I remember that in the past containerd was creating some bind mounts of container rootfs. Not sure if that functionality is still around or not. Those operations will fail.

@rhvgoyal Deferred deletion doesn't appear solve the problem. I believe the reason is that when requesting a deletion to be deferred, it still needs to be unmounted (but I'm by no means an LVM expert). Here's me trying it out:

% docker info | grep Deferred
 Deferred Removal Enabled: true
 Deferred Deletion Enabled: true
 Deferred Deleted Device Count: 0
% docker run -d --name testA busybox top
[snip]
% docker run -d --name testB -v /:/rootfs busybox top
[snip]
% docker rm -f testA
Error response from daemon: Driver devicemapper failed to remove root filesystem 5851da81b1e30f11ad8aebcb3ae0d05adeb4fbbb221490488df96b77f7d36719: failed to remove device 35546a51ac8dee03c934fa294fcfa1937689cfc4de06c9e554166b583a955425: devicemapper: Error running DeleteDevice dm_task_run failed

Note though that I recently discovered that openSUSE has some lvm patches that make error logging not as verbose as it is in upstream lvm (which may throw off some of the retry code in Docker), but that shouldn't affect this particular case.

Can't we first remove the directory containing container rootfs mount and then call into libdm to delete device. Removing directory will remove leaked mount point and after that device deletion will succeed.

That sounds like it might work, I'll play with it and see whether it also solves the issue. The downside is that it won't solve the problem on older kernels (the current solution partially solves the problem on older kernels).

Actually if we make /var/lib/docker/devicemapper unbindable, only bind mounts underneath are denied. I can still mount thin devices on /var/lib/docker/devicemapper/container1 and then /var/lib/docker/devicemapper/container1 can be bind mounted somewhere else.

So looks like making /var/lib/docker/devicemapper/ unbindable might be fine.

@rhvgoyal Yeah, making /var/lib/docker/devicemapper unbindable isn't bullet-proof, and just provides safety against accidental leakage. You can still forcefully leak the mount through a variety of ways (or up the safety by using --storage-opt dm.mountopt=unbindable). But if we just do the EnsureRemoveAll route (maybe combining it with the current solution) then we should be able to resolve even cases where someone has gone out of their way to try to DoS Docker.

@cyphar deferred deletion works for me. I am using Fedora26 docker.

Deferred Removal Enabled: true
Deferred Deletion Enabled: true
Deferred Deleted Device Count: 0

% docker run -d --name testA busybox top
[snip]
% docker run -d --name testB -v /:/rootfs busybox top
[snip]
% docker rm -f testA
testA

So it worked for me. In fact if deferred deletion is on, then you should not have got an error saying failed to delete device. Because that's the whole point of deferred deletion. Despite the fact that device could not be deleted return success to caller and try deferred deletion later with the help of a worker thread.

So something is missing from your setup.

@rhvgoyal

deferred deletion works for me. I am using Fedora26 docker.

Yeah, you're right. I tried it on Debian 9 and it also worked with deferred removal+deletion, which tells me that there's some weird bug going on on openSUSE. I double checked and it wasn't PEBKAC, so maybe there's some patch we've applied to libdm that is breaking deferral. I'll look into it.

However, I still think that handling this problem for non-deferred users is still a worthwhile exercise. I'm going to play around with the removal idea we've discussed and see if it can solve the problem without then need for MS_UNBINDABLE.

I completely forgot about opencontainers/runc#1500 which attempts to fix a similar version of this issue. It's possible that might help in the instances where the kernel is too old to be able to handle the RemoveAll semantics, if we just make the default propagation to MS_SLAVE. I'm not sure.

cc @rhatdan

@rhvgoyal Looks like this does not solve the issue in CRI-O either. runc must be makeing the mount points recursively RPRIVATE or RSHARE or something.

@rhatdan By default runc makes mounts rprivate, you can change it with rootfsPropagation in config.json. But take a look at my alternative PR to this one #34573, which works even if you do RPRIVATE (I still need to update it to address @rhvgoyal's comments).

Closing in favour of #34573.