Skip to content

Commit 589a0af

Browse files
cpuguy83cpuguy83
committed
Use rslave propagation for mounts from daemon root
By default, if a user requests a bind mount it uses private propagation. When the source path is a path within the daemon root this, along with some other propagation values that the user can use, causes issues when the daemon tries to remove a mountpoint because a container will then have a private reference to that mount which prevents removal. Unmouting with MNT_DETATCH can help this scenario on newer kernels, but ultimately this is just covering up the problem and doesn't actually free up the underlying resources until all references are destroyed. This change does essentially 2 things: 1. Change the default propagation when unspecified to `rslave` when the source path is within the daemon root path or a parent of the daemon root (because everything is using rbinds). 2. Creates a validation error on create when the user tries to specify an unacceptable propagation mode for these paths... basically the only two acceptable modes are `rslave` and `rshared`. In cases where we have used the new default propagation but the underlying filesystem is not setup to handle it (fs must hvae at least rshared propagation) instead of erroring out like we normally would, this falls back to the old default mode of `private`, which preserves backwards compatibility. Signed-off-by: Brian Goff <[email protected]>
1 parent 2e8ccbb commit 589a0af

6 files changed

Lines changed: 272 additions & 10 deletions

File tree

‎daemon/oci_linux.go‎

Lines changed: 28 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -604,21 +604,43 @@ func setMounts(daemon *Daemon, s *specs.Spec, c *container.Container, mounts []c
604604
//
605605
// For private volumes any root propagation value should work.
606606
pFlag := mountPropagationMap[m.Propagation]
607-
if pFlag == mount.SHARED || pFlag == mount.RSHARED {
607+
switch pFlag {
608+
case mount.SHARED, mount.RSHARED:
608609
if err := ensureShared(m.Source); err != nil {
609610
return err
610611
}
611612
rootpg := mountPropagationMap[s.Linux.RootfsPropagation]
612613
if rootpg != mount.SHARED && rootpg != mount.RSHARED {
613614
s.Linux.RootfsPropagation = mountPropagationReverseMap[mount.SHARED]
614615
}
615-
} else if pFlag == mount.SLAVE || pFlag == mount.RSLAVE {
616+
case mount.SLAVE, mount.RSLAVE:
617+
var fallback bool
616618
if err := ensureSharedOrSlave(m.Source); err != nil {
617-
return err
619+
// For backwards compatability purposes, treat mounts from the daemon root
620+
// as special since we automatically add rslave propagation to these mounts
621+
// when the user did not set anything, so we should fallback to the old
622+
// behavior which is to use private propagation which is normally the
623+
// default.
624+
if !strings.HasPrefix(m.Source, daemon.root) && !strings.HasPrefix(daemon.root, m.Source) {
625+
return err
626+
}
627+
628+
cm, ok := c.MountPoints[m.Destination]
629+
if !ok {
630+
return err
631+
}
632+
if cm.Spec.BindOptions != nil && cm.Spec.BindOptions.Propagation != "" {
633+
// This means the user explicitly set a propagation, do not fallback in that case.
634+
return err
635+
}
636+
fallback = true
637+
logrus.WithField("container", c.ID).WithField("source", m.Source).Warn("Falling back to default propagation for bind source in daemon root")
618638
}
619-
rootpg := mountPropagationMap[s.Linux.RootfsPropagation]
620-
if rootpg != mount.SHARED && rootpg != mount.RSHARED && rootpg != mount.SLAVE && rootpg != mount.RSLAVE {
621-
s.Linux.RootfsPropagation = mountPropagationReverseMap[mount.RSLAVE]
639+
if !fallback {
640+
rootpg := mountPropagationMap[s.Linux.RootfsPropagation]
641+
if rootpg != mount.SHARED && rootpg != mount.RSHARED && rootpg != mount.SLAVE && rootpg != mount.RSLAVE {
642+
s.Linux.RootfsPropagation = mountPropagationReverseMap[mount.RSLAVE]
643+
}
622644
}
623645
}
624646

‎daemon/volumes.go‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import (
1010

1111
"github.com/docker/docker/api/types"
1212
containertypes "github.com/docker/docker/api/types/container"
13+
"github.com/docker/docker/api/types/mount"
1314
mounttypes "github.com/docker/docker/api/types/mount"
1415
"github.com/docker/docker/container"
1516
"github.com/docker/docker/errdefs"
@@ -146,6 +147,13 @@ func (daemon *Daemon) registerMountPoints(container *container.Container, hostCo
146147
if err != nil {
147148
return err
148149
}
150+
needsSlavePropagation, err := daemon.validateBindDaemonRoot(bind.Spec)
151+
if err != nil {
152+
return err
153+
}
154+
if needsSlavePropagation {
155+
bind.Propagation = mount.PropagationRSlave
156+
}
149157

150158
// #10618
151159
_, tmpfsExists := hostConfig.Tmpfs[bind.Destination]
@@ -178,6 +186,13 @@ func (daemon *Daemon) registerMountPoints(container *container.Container, hostCo
178186
if err != nil {
179187
return errdefs.InvalidParameter(err)
180188
}
189+
needsSlavePropagation, err := daemon.validateBindDaemonRoot(mp.Spec)
190+
if err != nil {
191+
return err
192+
}
193+
if needsSlavePropagation {
194+
mp.Propagation = mount.PropagationRSlave
195+
}
181196

182197
if binds[mp.Destination] {
183198
return duplicateMountPointError(cfg.Target)

‎daemon/volumes_linux.go‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
package daemon
2+
3+
import (
4+
"strings"
5+
6+
"github.com/docker/docker/api/types/mount"
7+
"github.com/docker/docker/errdefs"
8+
"github.com/pkg/errors"
9+
)
10+
11+
// validateBindDaemonRoot ensures that if a given mountpoint's source is within
12+
// the daemon root path, that the propagation is setup to prevent a container
13+
// from holding private refereneces to a mount within the daemon root, which
14+
// can cause issues when the daemon attempts to remove the mountpoint.
15+
func (daemon *Daemon) validateBindDaemonRoot(m mount.Mount) (bool, error) {
16+
if m.Type != mount.TypeBind {
17+
return false, nil
18+
}
19+
20+
// check if the source is within the daemon root, or if the daemon root is within the source
21+
if !strings.HasPrefix(m.Source, daemon.root) && !strings.HasPrefix(daemon.root, m.Source) {
22+
return false, nil
23+
}
24+
25+
if m.BindOptions == nil {
26+
return true, nil
27+
}
28+
29+
switch m.BindOptions.Propagation {
30+
case mount.PropagationRSlave, mount.PropagationRShared, "":
31+
return m.BindOptions.Propagation == "", nil
32+
default:
33+
}
34+
35+
return false, errdefs.InvalidParameter(errors.Errorf(`invalid mount config: must use either propagation mode "rslave" or "rshared" when mount source is within the daemon root, daemon root: %q, bind mount source: %q, propagation: %q`, daemon.root, m.Source, m.BindOptions.Propagation))
36+
}

‎daemon/volumes_linux_test.go‎

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
package daemon
2+
3+
import (
4+
"path/filepath"
5+
"testing"
6+
7+
"github.com/docker/docker/api/types/mount"
8+
)
9+
10+
func TestBindDaemonRoot(t *testing.T) {
11+
t.Parallel()
12+
d := &Daemon{root: "/a/b/c/daemon"}
13+
for _, test := range []struct {
14+
desc string
15+
opts *mount.BindOptions
16+
needsProp bool
17+
err bool
18+
}{
19+
{desc: "nil propagation settings", opts: nil, needsProp: true, err: false},
20+
{desc: "empty propagation settings", opts: &mount.BindOptions{}, needsProp: true, err: false},
21+
{desc: "private propagation", opts: &mount.BindOptions{Propagation: mount.PropagationPrivate}, err: true},
22+
{desc: "rprivate propagation", opts: &mount.BindOptions{Propagation: mount.PropagationRPrivate}, err: true},
23+
{desc: "slave propagation", opts: &mount.BindOptions{Propagation: mount.PropagationSlave}, err: true},
24+
{desc: "rslave propagation", opts: &mount.BindOptions{Propagation: mount.PropagationRSlave}, err: false, needsProp: false},
25+
{desc: "shared propagation", opts: &mount.BindOptions{Propagation: mount.PropagationShared}, err: true},
26+
{desc: "rshared propagation", opts: &mount.BindOptions{Propagation: mount.PropagationRSlave}, err: false, needsProp: false},
27+
} {
28+
t.Run(test.desc, func(t *testing.T) {
29+
test := test
30+
for desc, source := range map[string]string{
31+
"source is root": d.root,
32+
"source is subpath": filepath.Join(d.root, "a", "b"),
33+
"source is parent": filepath.Dir(d.root),
34+
"source is /": "/",
35+
} {
36+
t.Run(desc, func(t *testing.T) {
37+
mount := mount.Mount{
38+
Type: mount.TypeBind,
39+
Source: source,
40+
BindOptions: test.opts,
41+
}
42+
needsProp, err := d.validateBindDaemonRoot(mount)
43+
if (err != nil) != test.err {
44+
t.Fatalf("expected err=%v, got: %v", test.err, err)
45+
}
46+
if test.err {
47+
return
48+
}
49+
if test.needsProp != needsProp {
50+
t.Fatalf("expected needsProp=%v, got: %v", test.needsProp, needsProp)
51+
}
52+
})
53+
}
54+
})
55+
}
56+
}

‎daemon/volumes_windows.go‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package daemon
33
import (
44
"sort"
55

6+
"github.com/docker/docker/api/types/mount"
67
"github.com/docker/docker/container"
78
"github.com/docker/docker/pkg/idtools"
89
"github.com/docker/docker/volume"
@@ -44,3 +45,7 @@ func (daemon *Daemon) setupMounts(c *container.Container) ([]container.Mount, er
4445
func setBindModeIfNull(bind *volume.MountPoint) {
4546
return
4647
}
48+
49+
func (daemon *Daemon) validateBindDaemonRoot(m mount.Mount) (bool, error) {
50+
return false, nil
51+
}

‎integration/container/mounts_linux_test.go‎

Lines changed: 132 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"bytes"
55
"context"
66
"fmt"
7+
"path/filepath"
78
"testing"
89

910
"github.com/docker/docker/api/types"
@@ -12,6 +13,7 @@ import (
1213
"github.com/docker/docker/api/types/network"
1314
"github.com/docker/docker/client"
1415
"github.com/docker/docker/integration-cli/daemon"
16+
"github.com/docker/docker/integration/util/request"
1517
"github.com/docker/docker/pkg/stdcopy"
1618
"github.com/docker/docker/pkg/system"
1719
"github.com/gotestyourself/gotestyourself/fs"
@@ -51,10 +53,10 @@ func TestContainerShmNoLeak(t *testing.T) {
5153
hc := container.HostConfig{
5254
Mounts: []mount.Mount{
5355
{
54-
Type: mount.TypeBind,
55-
Source: d.Root,
56-
Target: "/testdaemonroot",
57-
BindOptions: &mount.BindOptions{Propagation: mount.PropagationRPrivate}},
56+
Type: mount.TypeBind,
57+
Source: d.Root,
58+
Target: "/testdaemonroot",
59+
},
5860
},
5961
}
6062
cfg.Cmd = []string{"/bin/sh", "-c", fmt.Sprintf("mount | grep testdaemonroot | grep containers | grep %s", ctr.ID)}
@@ -141,3 +143,129 @@ func TestContainerNetworkMountsNoChown(t *testing.T) {
141143
require.NoError(t, err)
142144
assert.Equal(t, uint32(0), statT.UID(), "bind mounted network file should not change ownership from root")
143145
}
146+
147+
func TestMountDaemonRoot(t *testing.T) {
148+
t.Parallel()
149+
150+
client := request.NewAPIClient(t)
151+
ctx := context.Background()
152+
info, err := client.Info(ctx)
153+
if err != nil {
154+
t.Fatal(err)
155+
}
156+
157+
for _, test := range []struct {
158+
desc string
159+
propagation mount.Propagation
160+
expected mount.Propagation
161+
}{
162+
{
163+
desc: "default",
164+
propagation: "",
165+
expected: mount.PropagationRSlave,
166+
},
167+
{
168+
desc: "private",
169+
propagation: mount.PropagationPrivate,
170+
},
171+
{
172+
desc: "rprivate",
173+
propagation: mount.PropagationRPrivate,
174+
},
175+
{
176+
desc: "slave",
177+
propagation: mount.PropagationSlave,
178+
},
179+
{
180+
desc: "rslave",
181+
propagation: mount.PropagationRSlave,
182+
expected: mount.PropagationRSlave,
183+
},
184+
{
185+
desc: "shared",
186+
propagation: mount.PropagationShared,
187+
},
188+
{
189+
desc: "rshared",
190+
propagation: mount.PropagationRShared,
191+
expected: mount.PropagationRShared,
192+
},
193+
} {
194+
t.Run(test.desc, func(t *testing.T) {
195+
test := test
196+
t.Parallel()
197+
198+
propagationSpec := fmt.Sprintf(":%s", test.propagation)
199+
if test.propagation == "" {
200+
propagationSpec = ""
201+
}
202+
bindSpecRoot := info.DockerRootDir + ":" + "/foo" + propagationSpec
203+
bindSpecSub := filepath.Join(info.DockerRootDir, "containers") + ":/foo" + propagationSpec
204+
205+
for name, hc := range map[string]*container.HostConfig{
206+
"bind root": {Binds: []string{bindSpecRoot}},
207+
"bind subpath": {Binds: []string{bindSpecSub}},
208+
"mount root": {
209+
Mounts: []mount.Mount{
210+
{
211+
Type: mount.TypeBind,
212+
Source: info.DockerRootDir,
213+
Target: "/foo",
214+
BindOptions: &mount.BindOptions{Propagation: test.propagation},
215+
},
216+
},
217+
},
218+
"mount subpath": {
219+
Mounts: []mount.Mount{
220+
{
221+
Type: mount.TypeBind,
222+
Source: filepath.Join(info.DockerRootDir, "containers"),
223+
Target: "/foo",
224+
BindOptions: &mount.BindOptions{Propagation: test.propagation},
225+
},
226+
},
227+
},
228+
} {
229+
t.Run(name, func(t *testing.T) {
230+
hc := hc
231+
t.Parallel()
232+
233+
c, err := client.ContainerCreate(ctx, &container.Config{
234+
Image: "busybox",
235+
Cmd: []string{"true"},
236+
}, hc, nil, "")
237+
238+
if err != nil {
239+
if test.expected != "" {
240+
t.Fatal(err)
241+
}
242+
// expected an error, so this is ok and should not continue
243+
return
244+
}
245+
if test.expected == "" {
246+
t.Fatal("expected create to fail")
247+
}
248+
249+
defer func() {
250+
if err := client.ContainerRemove(ctx, c.ID, types.ContainerRemoveOptions{Force: true}); err != nil {
251+
panic(err)
252+
}
253+
}()
254+
255+
inspect, err := client.ContainerInspect(ctx, c.ID)
256+
if err != nil {
257+
t.Fatal(err)
258+
}
259+
if len(inspect.Mounts) != 1 {
260+
t.Fatalf("unexpected number of mounts: %+v", inspect.Mounts)
261+
}
262+
263+
m := inspect.Mounts[0]
264+
if m.Propagation != test.expected {
265+
t.Fatalf("got unexpected propagation mode, expected %q, got: %v", test.expected, m.Propagation)
266+
}
267+
})
268+
}
269+
})
270+
}
271+
}

0 commit comments

Comments
 (0)