Obtain ref on root dentry in mqfs.GetFilesystem.

As documented in FilesystemType.GetFilesystem, a reference should be taken on
the returned dentry and filesystem by GetFilesystem implementation. mqfs did
not do that.

Additionally cleanup and clarify ref counting of dentry, filesystem and mount
in mqfs.

Reported-by: syzbot+a2c54bfb6e1525228e5f@syzkaller.appspotmail.com
Reported-by: syzbot+ccd305cdab11cfebbfff@syzkaller.appspotmail.com
PiperOrigin-RevId: 405700565

6 files changed, 27 insertions, 35 deletions

diff --git a/pkg/sentry/fsimpl/mqfs/mqfs.go b/pkg/sentry/fsimpl/mqfs/mqfs.go
index c2b53c9d0..9b3704217 100644
--- a/pkg/sentry/fsimpl/mqfs/mqfs.go
+++ b/pkg/sentry/fsimpl/mqfs/mqfs.go
@@ -75,6 +75,7 @@ func (ft FilesystemType) GetFilesystem(ctx context.Context, vfsObj *vfs.VirtualF
 	impl.fs.MaxCachedDentries = maxCachedDentries
 
 	impl.fs.VFSFilesystem().IncRef()
+	impl.root.IncRef()
 	return impl.fs.VFSFilesystem(), impl.root.VFSDentry(), nil
 }
 
@@ -100,10 +101,6 @@ func maxCachedDentries(ctx context.Context, mopts map[string]string) (_ uint64,
 type filesystem struct {
 	kernfs.Filesystem
 	devMinor uint32
-
-	// root is the filesystem's root dentry. Since we take a reference on it in
-	// GetFilesystem, we should release it when the fs is released.
-	root *kernfs.Dentry
 }
 
 // Release implements vfs.FilesystemImpl.Release.
diff --git a/pkg/sentry/fsimpl/mqfs/registry.go b/pkg/sentry/fsimpl/mqfs/registry.go
index c8fbe4d33..e470ffadc 100644
--- a/pkg/sentry/fsimpl/mqfs/registry.go
+++ b/pkg/sentry/fsimpl/mqfs/registry.go
@@ -57,16 +57,19 @@ func NewRegistryImpl(ctx context.Context, vfsObj *vfs.VirtualFilesystem, creds *
 		return nil, err
 	}
 
-	var dentry kernfs.Dentry
 	fs := &filesystem{
 		devMinor: devMinor,
-		root:     &dentry,
 	}
 	fs.VFSFilesystem().Init(vfsObj, &FilesystemType{}, fs)
 	vfsfs := fs.VFSFilesystem()
+	// NewDisconnectedMount will obtain a ref on dentry and vfsfs which is
+	// transferred to mount. vfsfs was initiated with 1 ref already. So get rid
+	// of the extra ref.
+	defer vfsfs.DecRef(ctx)
 
+	// dentry is initialized with 1 ref which is transferred to fs.
+	var dentry kernfs.Dentry
 	dentry.InitRoot(&fs.Filesystem, fs.newRootInode(ctx, creds))
-	defer vfsfs.DecRef(ctx) // NewDisconnectedMount will obtain a ref on success.
 
 	mount, err := vfsObj.NewDisconnectedMount(vfsfs, dentry.VFSDentry(), &vfs.MountOptions{})
 	if err != nil {
@@ -82,7 +85,7 @@ func NewRegistryImpl(ctx context.Context, vfsObj *vfs.VirtualFilesystem, creds *
 
 // Get implements mq.RegistryImpl.Get.
 func (r *RegistryImpl) Get(ctx context.Context, name string, access mq.AccessType, block bool, flags uint32) (*vfs.FileDescription, bool, error) {
-	inode, err := r.lookup(ctx, name)
+	inode, err := r.root.Inode().(*rootInode).Lookup(ctx, name)
 	if err != nil {
 		return nil, false, nil
 	}
@@ -120,7 +123,7 @@ func (r *RegistryImpl) Unlink(ctx context.Context, name string) error {
 	}
 
 	root := r.root.Inode().(*rootInode)
-	inode, err := r.lookup(ctx, name)
+	inode, err := root.Lookup(ctx, name)
 	if err != nil {
 		return err
 	}
@@ -133,16 +136,6 @@ func (r *RegistryImpl) Destroy(ctx context.Context) {
 	r.mount.DecRef(ctx)
 }
 
-// lookup retreives a kernfs.Inode using a name.
-func (r *RegistryImpl) lookup(ctx context.Context, name string) (kernfs.Inode, error) {
-	inode := r.root.Inode().(*rootInode)
-	lookup, err := inode.Lookup(ctx, name)
-	if err != nil {
-		return nil, err
-	}
-	return lookup, nil
-}
-
 // newFD returns a new file description created using the given queue and inode.
 func (r *RegistryImpl) newFD(q *mq.Queue, inode *queueInode, access mq.AccessType, block bool, flags uint32) (*vfs.FileDescription, error) {
 	view, err := mq.NewView(q, access, block)
diff --git a/pkg/sentry/fsimpl/mqfs/root.go b/pkg/sentry/fsimpl/mqfs/root.go
index 37b5749fb..922e669e2 100644
--- a/pkg/sentry/fsimpl/mqfs/root.go
+++ b/pkg/sentry/fsimpl/mqfs/root.go
@@ -34,7 +34,6 @@ type rootInode struct {
 	kernfs.InodeNotSymlink
 	kernfs.InodeTemporary
 	kernfs.OrderedChildren
-	implStatFS
 
 	locks vfs.FileLocks
 }
@@ -77,13 +76,7 @@ func (*rootInode) SetStat(context.Context, *vfs.Filesystem, *auth.Credentials, v
 	return linuxerr.EPERM
 }
 
-// implStatFS provides an implementation of kernfs.Inode.StatFS for message
-// queues to be embedded in inodes.
-//
-// +stateify savable
-type implStatFS struct{}
-
 // StatFS implements kernfs.Inode.StatFS.
-func (*implStatFS) StatFS(context.Context, *vfs.Filesystem) (linux.Statfs, error) {
+func (*rootInode) StatFS(context.Context, *vfs.Filesystem) (linux.Statfs, error) {
 	return vfs.GenericStatFS(linux.MQUEUE_MAGIC), nil
 }
diff --git a/pkg/sentry/vfs/filesystem_type.go b/pkg/sentry/vfs/filesystem_type.go
index 9d54cc4ed..70d6eb85e 100644
--- a/pkg/sentry/vfs/filesystem_type.go
+++ b/pkg/sentry/vfs/filesystem_type.go
@@ -28,7 +28,7 @@ import (
 type FilesystemType interface {
 	// GetFilesystem returns a Filesystem configured by the given options,
 	// along with its mount root. A reference is taken on the returned
-	// Filesystem and Dentry.
+	// Filesystem and Dentry whose ownership is transferred to the caller.
 	GetFilesystem(ctx context.Context, vfsObj *VirtualFilesystem, creds *auth.Credentials, source string, opts GetFilesystemOptions) (*Filesystem, *Dentry, error)
 
 	// Name returns the name of this FilesystemType.
diff --git a/test/syscalls/linux/BUILD b/test/syscalls/linux/BUILD
index 9d975c614..6217ff4dc 100644
--- a/test/syscalls/linux/BUILD
+++ b/test/syscalls/linux/BUILD
@@ -4209,6 +4209,7 @@ cc_binary(
     linkstatic = 1,
     deps = [
         "//test/util:capability_util",
+        "//test/util:cleanup",
         "//test/util:fs_util",
         "//test/util:mount_util",
         "//test/util:posix_error",
diff --git a/test/syscalls/linux/mq.cc b/test/syscalls/linux/mq.cc
index 839877a67..013994fd9 100644
--- a/test/syscalls/linux/mq.cc
+++ b/test/syscalls/linux/mq.cc
@@ -22,6 +22,7 @@
 #include <string>
 
 #include "test/util/capability_util.h"
+#include "test/util/cleanup.h"
 #include "test/util/fs_util.h"
 #include "test/util/mount_util.h"
 #include "test/util/posix_error.h"
@@ -286,18 +287,25 @@ TEST(MqTest, Mount) {
 TEST(MqTest, MountSeveral) {
   SKIP_IF(IsRunningWithVFS1() ||
           !ASSERT_NO_ERRNO_AND_VALUE(HaveCapability(CAP_SYS_ADMIN)));
+  constexpr int numMounts = 3;
 
+  // mountDirs should outlive mountCUs and queue so that its destructor succeeds
+  // in unlinking the mountpoints and does not interfere with queue destruction.
+  testing::TempPath mountDirs[numMounts];
+  testing::Cleanup mountCUs[numMounts];
   PosixQueue queue = ASSERT_NO_ERRNO_AND_VALUE(
       MqOpen(O_RDWR | O_CREAT | O_EXCL, 0777, nullptr));
 
-  auto const dir1 = ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateDir());
-  // Assign the pointer so it doesn't get destroyed before the second mount is
-  // created.
-  auto mnt =
-      ASSERT_NO_ERRNO_AND_VALUE(Mount("none", dir1.path(), "mqueue", 0, "", 0));
+  for (int i = 0; i < numMounts; ++i) {
+    mountDirs[i] = ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateDir());
+    mountCUs[i] = ASSERT_NO_ERRNO_AND_VALUE(
+        Mount("none", mountDirs[i].path(), "mqueue", 0, "", 0));
+  }
 
-  auto const dir2 = ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateDir());
-  ASSERT_NO_ERRNO(Mount("none", dir2.path(), "mqueue", 0, "", 0));
+  // Ensure that queue is visible from all mounts.
+  for (int i = 0; i < numMounts; ++i) {
+    ASSERT_NO_ERRNO(Stat(JoinPath(mountDirs[i].path(), queue.name())));
+  }
 }
 
 // Test mounting mqueue and opening a queue as normal file.