Browse code

Extra check before unmounting on shutdown

This makes sure that if the daemon root was already a self-binded mount
(thus meaning the daemonc only performed a remount) that the daemon does
not try to unmount.

Example:

```
$ sudo mount --bind /var/lib/docker /var/lib/docker
$ sudo dockerd &
```

Signed-off-by: Brian Goff <cpuguy83@gmail.com>

Brian Goff authored on 2018/04/18 00:30:39
Showing 3 changed files
... ...
@@ -89,8 +89,16 @@ func (daemon *Daemon) cleanupMounts() error {
89 89
 		return nil
90 90
 	}
91 91
 
92
+	unmountFile := getUnmountOnShutdownPath(daemon.configStore)
93
+	if _, err := os.Stat(unmountFile); err != nil {
94
+		return nil
95
+	}
96
+
92 97
 	logrus.WithField("mountpoint", daemon.root).Debug("unmounting daemon root")
93
-	return mount.Unmount(daemon.root)
98
+	if err := mount.Unmount(daemon.root); err != nil {
99
+		return err
100
+	}
101
+	return os.Remove(unmountFile)
94 102
 }
95 103
 
96 104
 func getCleanPatterns(id string) (regexps []*regexp.Regexp) {
... ...
@@ -3,11 +3,15 @@
3 3
 package daemon // import "github.com/docker/docker/daemon"
4 4
 
5 5
 import (
6
+	"io/ioutil"
7
+	"os"
8
+	"path/filepath"
6 9
 	"strings"
7 10
 	"testing"
8 11
 
9 12
 	containertypes "github.com/docker/docker/api/types/container"
10 13
 	"github.com/docker/docker/container"
14
+	"github.com/docker/docker/daemon/config"
11 15
 	"github.com/docker/docker/oci"
12 16
 	"github.com/docker/docker/pkg/idtools"
13 17
 	"github.com/docker/docker/pkg/mount"
... ...
@@ -228,3 +232,99 @@ func TestShouldUnmountRoot(t *testing.T) {
228 228
 		})
229 229
 	}
230 230
 }
231
+
232
+func checkMounted(t *testing.T, p string, expect bool) {
233
+	t.Helper()
234
+	mounted, err := mount.Mounted(p)
235
+	assert.Check(t, err)
236
+	assert.Check(t, mounted == expect, "expected %v, actual %v", expect, mounted)
237
+}
238
+
239
+func TestRootMountCleanup(t *testing.T) {
240
+	t.Parallel()
241
+
242
+	testRoot, err := ioutil.TempDir("", t.Name())
243
+	assert.Assert(t, err)
244
+	defer os.RemoveAll(testRoot)
245
+	cfg := &config.Config{}
246
+
247
+	err = mount.MakePrivate(testRoot)
248
+	assert.Assert(t, err)
249
+	defer mount.Unmount(testRoot)
250
+
251
+	cfg.ExecRoot = filepath.Join(testRoot, "exec")
252
+	cfg.Root = filepath.Join(testRoot, "daemon")
253
+
254
+	err = os.Mkdir(cfg.ExecRoot, 0755)
255
+	assert.Assert(t, err)
256
+	err = os.Mkdir(cfg.Root, 0755)
257
+	assert.Assert(t, err)
258
+
259
+	d := &Daemon{configStore: cfg, root: cfg.Root}
260
+	unmountFile := getUnmountOnShutdownPath(cfg)
261
+
262
+	t.Run("regular dir no mountpoint", func(t *testing.T) {
263
+		err = setupDaemonRootPropagation(cfg)
264
+		assert.Assert(t, err)
265
+		_, err = os.Stat(unmountFile)
266
+		assert.Assert(t, err)
267
+		checkMounted(t, cfg.Root, true)
268
+
269
+		assert.Assert(t, d.cleanupMounts())
270
+		checkMounted(t, cfg.Root, false)
271
+
272
+		_, err = os.Stat(unmountFile)
273
+		assert.Assert(t, os.IsNotExist(err))
274
+	})
275
+
276
+	t.Run("root is a private mountpoint", func(t *testing.T) {
277
+		err = mount.MakePrivate(cfg.Root)
278
+		assert.Assert(t, err)
279
+		defer mount.Unmount(cfg.Root)
280
+
281
+		err = setupDaemonRootPropagation(cfg)
282
+		assert.Assert(t, err)
283
+		assert.Check(t, ensureShared(cfg.Root))
284
+
285
+		_, err = os.Stat(unmountFile)
286
+		assert.Assert(t, os.IsNotExist(err))
287
+		assert.Assert(t, d.cleanupMounts())
288
+		checkMounted(t, cfg.Root, true)
289
+	})
290
+
291
+	// mount is pre-configured with a shared mount
292
+	t.Run("root is a shared mountpoint", func(t *testing.T) {
293
+		err = mount.MakeShared(cfg.Root)
294
+		assert.Assert(t, err)
295
+		defer mount.Unmount(cfg.Root)
296
+
297
+		err = setupDaemonRootPropagation(cfg)
298
+		assert.Assert(t, err)
299
+
300
+		if _, err := os.Stat(unmountFile); err == nil {
301
+			t.Fatal("unmount file should not exist")
302
+		}
303
+
304
+		assert.Assert(t, d.cleanupMounts())
305
+		checkMounted(t, cfg.Root, true)
306
+		assert.Assert(t, mount.Unmount(cfg.Root))
307
+	})
308
+
309
+	// does not need mount but unmount file exists from previous run
310
+	t.Run("old mount file is cleaned up on setup if not needed", func(t *testing.T) {
311
+		err = mount.MakeShared(testRoot)
312
+		assert.Assert(t, err)
313
+		defer mount.MakePrivate(testRoot)
314
+		err = ioutil.WriteFile(unmountFile, nil, 0644)
315
+		assert.Assert(t, err)
316
+
317
+		err = setupDaemonRootPropagation(cfg)
318
+		assert.Assert(t, err)
319
+
320
+		_, err = os.Stat(unmountFile)
321
+		assert.Check(t, os.IsNotExist(err), err)
322
+		checkMounted(t, cfg.Root, false)
323
+		assert.Assert(t, d.cleanupMounts())
324
+	})
325
+
326
+}
... ...
@@ -1177,14 +1177,57 @@ func setupDaemonRoot(config *config.Config, rootDir string, rootIDs idtools.IDPa
1177 1177
 		}
1178 1178
 	}
1179 1179
 
1180
-	if err := ensureSharedOrSlave(config.Root); err != nil {
1181
-		if err := mount.MakeShared(config.Root); err != nil {
1182
-			logrus.WithError(err).WithField("dir", config.Root).Warn("Could not set daemon root propagation to shared, this is not generally critical but may cause some functionality to not work or fallback to less desirable behavior")
1180
+	if err := setupDaemonRootPropagation(config); err != nil {
1181
+		logrus.WithError(err).WithField("dir", config.Root).Warn("Error while setting daemon root propagation, this is not generally critical but may cause some functionality to not work or fallback to less desirable behavior")
1182
+	}
1183
+	return nil
1184
+}
1185
+
1186
+func setupDaemonRootPropagation(cfg *config.Config) error {
1187
+	rootParentMount, options, err := getSourceMount(cfg.Root)
1188
+	if err != nil {
1189
+		return errors.Wrap(err, "error getting daemon root's parent mount")
1190
+	}
1191
+
1192
+	var cleanupOldFile bool
1193
+	cleanupFile := getUnmountOnShutdownPath(cfg)
1194
+	defer func() {
1195
+		if !cleanupOldFile {
1196
+			return
1183 1197
 		}
1198
+		if err := os.Remove(cleanupFile); err != nil && !os.IsNotExist(err) {
1199
+			logrus.WithError(err).WithField("file", cleanupFile).Warn("could not clean up old root propagation unmount file")
1200
+		}
1201
+	}()
1202
+
1203
+	if hasMountinfoOption(options, sharedPropagationOption, slavePropagationOption) {
1204
+		cleanupOldFile = true
1205
+		return nil
1206
+	}
1207
+
1208
+	if err := mount.MakeShared(cfg.Root); err != nil {
1209
+		return errors.Wrap(err, "could not setup daemon root propagation to shared")
1210
+	}
1211
+
1212
+	// check the case where this may have already been a mount to itself.
1213
+	// If so then the daemon only performed a remount and should not try to unmount this later.
1214
+	if rootParentMount == cfg.Root {
1215
+		cleanupOldFile = true
1216
+		return nil
1217
+	}
1218
+
1219
+	if err := ioutil.WriteFile(cleanupFile, nil, 0600); err != nil {
1220
+		return errors.Wrap(err, "error writing file to signal mount cleanup on shutdown")
1184 1221
 	}
1185 1222
 	return nil
1186 1223
 }
1187 1224
 
1225
+// getUnmountOnShutdownPath generates the path to used when writing the file that signals to the daemon that on shutdown
1226
+// the daemon root should be unmounted.
1227
+func getUnmountOnShutdownPath(config *config.Config) string {
1228
+	return filepath.Join(config.ExecRoot, "unmount-on-shutdown")
1229
+}
1230
+
1188 1231
 // registerLinks writes the links to a file.
1189 1232
 func (daemon *Daemon) registerLinks(container *container.Container, hostConfig *containertypes.HostConfig) error {
1190 1233
 	if hostConfig == nil || hostConfig.NetworkMode.IsUserDefined() {