fs: Fix renameMu lock recursion.

dirent.walk() takes renameMu, but is often called with renameMu already held,
which can lead to a deadlock.

Fix this by requiring renameMu to be held for reading when dirent.walk() is
called. This causes walks and existence checks to block while a rename
operation takes place, but that is what we were already trying to enforce by
taking renameMu in walk() anyways.

PiperOrigin-RevId: 210760780
Change-Id: Id61018e6e4adbeac53b9c1b3aa24ab77f75d8a54

1 files changed, 16 insertions, 41 deletions

diff --git a/pkg/sentry/fs/dirent.go b/pkg/sentry/fs/dirent.go
index 30545de7e..f81f7d627 100644
--- a/pkg/sentry/fs/dirent.go
+++ b/pkg/sentry/fs/dirent.go
@@ -451,6 +451,7 @@ func (d *Dirent) descendantOf(p *Dirent) bool {
 // Inode.Lookup, otherwise walk will keep d.mu locked.
 //
 // Preconditions:
+// - renameMu must be held for reading.
 // - d.mu must be held.
 // - name must must not contain "/"s.
 func (d *Dirent) walk(ctx context.Context, root *Dirent, name string, walkMayUnlock bool) (*Dirent, error) {
@@ -461,22 +462,18 @@ func (d *Dirent) walk(ctx context.Context, root *Dirent, name string, walkMayUnl
 		d.IncRef()
 		return d, nil
 	} else if name == ".." {
-		renameMu.RLock()
 		// Respect the chroot. Note that in Linux there is no check to enforce
 		// that d is a descendant of root.
 		if d == root {
 			d.IncRef()
-			renameMu.RUnlock()
 			return d, nil
 		}
 		// Are we already at the root? Then ".." is ".".
 		if d.IsRoot() {
 			d.IncRef()
-			renameMu.RUnlock()
 			return d, nil
 		}
 		d.parent.IncRef()
-		renameMu.RUnlock()
 		return d.parent, nil
 	}
 
@@ -532,15 +529,11 @@ func (d *Dirent) walk(ctx context.Context, root *Dirent, name string, walkMayUnl
 	// Slow path: load the InodeOperations into memory. Since this is a hot path and the lookup may be
 	// expensive, if possible release the lock and re-acquire it.
 	if walkMayUnlock {
-		// While this dirent is unlocked, the lookup below is not allowed to proceed in tandem with a
-		// rename operation. The rename should be fully complete before we call Lookup on anything.
 		d.mu.Unlock()
-		renameMu.RLock()
 	}
 	c, err := d.Inode.Lookup(ctx, name)
 	if walkMayUnlock {
 		d.mu.Lock()
-		renameMu.RUnlock()
 	}
 	// No dice.
 	if err != nil {
@@ -608,18 +601,27 @@ func (d *Dirent) Walk(ctx context.Context, root *Dirent, name string) (*Dirent,
 		panic("Dirent.Walk: root must not be nil")
 	}
 
+	// We could use lockDirectory here, but this is a hot path and we want
+	// to avoid defer.
+	renameMu.RLock()
 	d.dirMu.RLock()
 	d.mu.Lock()
+
 	child, err := d.walk(ctx, root, name, true /* may unlock */)
+
 	d.mu.Unlock()
 	d.dirMu.RUnlock()
+	renameMu.RUnlock()
 
 	return child, err
 }
 
 // exists returns true if name exists in relation to d.
 //
-// Preconditions: d.mu must be held.
+// Preconditions:
+// - renameMu must be held for reading.
+// - d.mu must be held.
+// - name must must not contain "/"s.
 func (d *Dirent) exists(ctx context.Context, root *Dirent, name string) bool {
 	child, err := d.walk(ctx, root, name, true /* may unlock */)
 	if err != nil {
@@ -634,24 +636,13 @@ func (d *Dirent) exists(ctx context.Context, root *Dirent, name string) bool {
 // lockDirectory should be called for any operation that changes this `d`s
 // children (creating or removing them).
 func (d *Dirent) lockDirectory() func() {
-	if d.Inode.overlay != nil {
-		// overlay copyUp may need to look at Dirent parents, and hence
-		// may need renameMu.
-		renameMu.RLock()
-		d.dirMu.Lock()
-		d.mu.Lock()
-		return func() {
-			d.mu.Unlock()
-			d.dirMu.Unlock()
-			renameMu.RUnlock()
-		}
-	}
-
+	renameMu.RLock()
 	d.dirMu.Lock()
 	d.mu.Lock()
 	return func() {
 		d.mu.Unlock()
 		d.dirMu.Unlock()
+		renameMu.RUnlock()
 	}
 }
 
@@ -724,9 +715,10 @@ func (d *Dirent) finishCreate(child *Dirent, name string) {
 
 // genericCreate executes create if name does not exist. Removes a negative Dirent at name if
 // create succeeds.
-//
-// Preconditions: d.mu must be held.
 func (d *Dirent) genericCreate(ctx context.Context, root *Dirent, name string, create func() error) error {
+	unlock := d.lockDirectory()
+	defer unlock()
+
 	// Does something already exist?
 	if d.exists(ctx, root, name) {
 		return syscall.EEXIST
@@ -765,9 +757,6 @@ func (d *Dirent) genericCreate(ctx context.Context, root *Dirent, name string, c
 
 // CreateLink creates a new link in this directory.
 func (d *Dirent) CreateLink(ctx context.Context, root *Dirent, oldname, newname string) error {
-	unlock := d.lockDirectory()
-	defer unlock()
-
 	return d.genericCreate(ctx, root, newname, func() error {
 		if err := d.Inode.CreateLink(ctx, d, oldname, newname); err != nil {
 			return err
@@ -779,9 +768,6 @@ func (d *Dirent) CreateLink(ctx context.Context, root *Dirent, oldname, newname
 
 // CreateHardLink creates a new hard link in this directory.
 func (d *Dirent) CreateHardLink(ctx context.Context, root *Dirent, target *Dirent, name string) error {
-	unlock := d.lockDirectory()
-	defer unlock()
-
 	// Make sure that target does not span filesystems.
 	if d.Inode.MountSource != target.Inode.MountSource {
 		return syscall.EXDEV
@@ -799,9 +785,6 @@ func (d *Dirent) CreateHardLink(ctx context.Context, root *Dirent, target *Diren
 
 // CreateDirectory creates a new directory under this dirent.
 func (d *Dirent) CreateDirectory(ctx context.Context, root *Dirent, name string, perms FilePermissions) error {
-	unlock := d.lockDirectory()
-	defer unlock()
-
 	return d.genericCreate(ctx, root, name, func() error {
 		if err := d.Inode.CreateDirectory(ctx, d, name, perms); err != nil {
 			return err
@@ -813,11 +796,6 @@ func (d *Dirent) CreateDirectory(ctx context.Context, root *Dirent, name string,
 
 // Bind satisfies the InodeOperations interface; otherwise same as GetFile.
 func (d *Dirent) Bind(ctx context.Context, root *Dirent, name string, data unix.BoundEndpoint, perms FilePermissions) (*Dirent, error) {
-	d.dirMu.Lock()
-	defer d.dirMu.Unlock()
-	d.mu.Lock()
-	defer d.mu.Unlock()
-
 	var childDir *Dirent
 	err := d.genericCreate(ctx, root, name, func() error {
 		var e error
@@ -839,9 +817,6 @@ func (d *Dirent) Bind(ctx context.Context, root *Dirent, name string, data unix.
 
 // CreateFifo creates a new named pipe under this dirent.
 func (d *Dirent) CreateFifo(ctx context.Context, root *Dirent, name string, perms FilePermissions) error {
-	unlock := d.lockDirectory()
-	defer unlock()
-
 	return d.genericCreate(ctx, root, name, func() error {
 		if err := d.Inode.CreateFifo(ctx, d, name, perms); err != nil {
 			return err