diff --git a/pkg/sentry/vfs/mount.go b/pkg/sentry/vfs/mount.go index 469431a21..8a350e0f4 100644 --- a/pkg/sentry/vfs/mount.go +++ b/pkg/sentry/vfs/mount.go @@ -276,18 +276,18 @@ func (vfs *VirtualFilesystem) ConnectMountAt(ctx context.Context, creds *auth.Cr tree := vfs.preparePropagationTree(mnt, vd) cleanup := cleanup.Make(func() { vfs.abortPropagationTree(ctx, tree) // +checklocksforce + }) + defer cleanup.Clean() + // Check if the new mount + all the propagation mounts puts us over the max. + if uint32(len(tree)+1)+vd.mount.ns.mounts > MountMax { // We need to unlock mountMu first because DecRef takes a lock on the // filesystem mutex in some implementations, which can lead to circular // locking. vfs.mountMu.Unlock() vd.DecRef(ctx) - }) - defer cleanup.Clean() - // Check if the new mount + all the propagation mounts puts us over the max. - if uint32(len(tree)+1)+vd.mount.ns.mounts > MountMax { return linuxerr.ENOSPC } - if err := vfs.connectMountAt(ctx, mnt, vd); err != nil { + if err := vfs.connectMountAtLocked(ctx, mnt, vd); err != nil { return err } vfs.commitPropagationTree(ctx, tree) @@ -296,20 +296,20 @@ func (vfs *VirtualFilesystem) ConnectMountAt(ctx context.Context, creds *auth.Cr return nil } -// connectMountAtLocked attaches mnt at vd. If the method returns an error that -// is not nil, then it did not consume a reference on vd and the caller is -// responsible for calling DecRef. +// connectMountAtLocked attaches mnt at vd. This method consumes a reference on +// vd. // // Preconditions: // - mnt must be disconnected. // - vfs.mountMu must be locked. // // +checklocks:vfs.mountMu -func (vfs *VirtualFilesystem) connectMountAt(ctx context.Context, mnt *Mount, vd VirtualDentry) error { +func (vfs *VirtualFilesystem) connectMountAtLocked(ctx context.Context, mnt *Mount, vd VirtualDentry) error { vd.dentry.mu.Lock() for { if vd.mount.umounted || vd.dentry.dead { vd.dentry.mu.Unlock() + vd.DecRef(ctx) return linuxerr.ENOENT } // vd might have been mounted over between vfs.GetDentryAt() and @@ -408,14 +408,15 @@ func (vfs *VirtualFilesystem) BindAt(ctx context.Context, creds *auth.Credential // Checklocks doesn't work with anon functions. vfs.setPropagation(clone, Private) // +checklocksforce vfs.abortPropagationTree(ctx, tree) // +checklocksforce - vfs.mountMu.Unlock() - targetVd.DecRef(ctx) }) defer cleanup.Clean() if uint32(1+len(tree))+targetVd.mount.ns.mounts > MountMax { + vfs.mountMu.Unlock() + targetVd.DecRef(ctx) return nil, linuxerr.ENOSPC } - if err := vfs.connectMountAt(ctx, clone, targetVd); err != nil { + if err := vfs.connectMountAtLocked(ctx, clone, targetVd); err != nil { + vfs.mountMu.Unlock() return nil, err } vfs.commitPropagationTree(ctx, tree) diff --git a/test/syscalls/linux/mount.cc b/test/syscalls/linux/mount.cc index 21e5b7585..e9e8dbd66 100644 --- a/test/syscalls/linux/mount.cc +++ b/test/syscalls/linux/mount.cc @@ -24,6 +24,7 @@ #include #include +#include #include #include #include @@ -1418,6 +1419,26 @@ TEST(MountTest, MountInfoHasRoot) { } } +TEST(MountTest, DeadMountsAreDecRefd) { + SKIP_IF(!ASSERT_NO_ERRNO_AND_VALUE(HaveCapability(CAP_SYS_ADMIN))); + DisableSave ds; + std::string home = NewTempAbsPath(); + ASSERT_NO_ERRNO(Mkdir(home)); + ASSERT_THAT(chdir(home.c_str()), SyscallSucceeds()); + constexpr char dirpath[] = "./file"; + + for (int i = 0; i < 10; ++i) { + const auto rest = [&] { + mkdir(dirpath, 0); + mount(dirpath, ".", 0, MS_BIND, 0); + rmdir(dirpath); + mkdir(dirpath, 0); + mount(dirpath, ".", 0, MS_BIND, 0); + }; + EXPECT_THAT(InForkedProcess(rest), IsPosixErrorOkAndHolds(0)); + } +} + } // namespace } // namespace testing