From 343661770aa96efe3b539a82addff13df235413f Mon Sep 17 00:00:00 2001
From: Jamie Liu <jamieliu@google.com>
Date: Fri, 7 Aug 2020 18:32:03 -0700
Subject: Don't hold gofer.filesystem.renameMu during dentry destruction.

PiperOrigin-RevId: 325546629
---
 pkg/sentry/fsimpl/gofer/gofer.go      | 20 +++++++++++++++-----
 pkg/sentry/fsimpl/gofer/gofer_test.go |  2 ++
 2 files changed, 17 insertions(+), 5 deletions(-)

(limited to 'pkg/sentry/fsimpl/gofer')

diff --git a/pkg/sentry/fsimpl/gofer/gofer.go b/pkg/sentry/fsimpl/gofer/gofer.go
index 4ac8dd81d..63e589859 100644
--- a/pkg/sentry/fsimpl/gofer/gofer.go
+++ b/pkg/sentry/fsimpl/gofer/gofer.go
@@ -1158,7 +1158,8 @@ func (d *dentry) OnZeroWatches(ctx context.Context) {
 // operation. One of the calls may destroy the dentry, so subsequent calls will
 // do nothing.
 //
-// Preconditions: d.fs.renameMu must be locked for writing.
+// Preconditions: d.fs.renameMu must be locked for writing; it may be
+// temporarily unlocked.
 func (d *dentry) checkCachingLocked(ctx context.Context) {
 	// Dentries with a non-zero reference count must be retained. (The only way
 	// to obtain a reference on a dentry with zero references is via path
@@ -1238,11 +1239,13 @@ func (d *dentry) checkCachingLocked(ctx context.Context) {
 	}
 }
 
-// destroyLocked destroys the dentry. It may flushes dirty pages from cache,
-// close p9 file and remove reference on parent dentry.
+// destroyLocked destroys the dentry.
 //
-// Preconditions: d.fs.renameMu must be locked for writing. d.refs == 0. d is
-// not a child dentry.
+// Preconditions:
+// * d.fs.renameMu must be locked for writing; it may be temporarily unlocked.
+// * d.refs == 0.
+// * d.parent.children[d.name] != d, i.e. d is not reachable by path traversal
+//   from its former parent dentry.
 func (d *dentry) destroyLocked(ctx context.Context) {
 	switch atomic.LoadInt64(&d.refs) {
 	case 0:
@@ -1254,6 +1257,10 @@ func (d *dentry) destroyLocked(ctx context.Context) {
 		panic("dentry.destroyLocked() called with references on the dentry")
 	}
 
+	// Allow the following to proceed without renameMu locked to improve
+	// scalability.
+	d.fs.renameMu.Unlock()
+
 	mf := d.fs.mfp.MemoryFile()
 	d.handleMu.Lock()
 	d.dataMu.Lock()
@@ -1315,6 +1322,9 @@ func (d *dentry) destroyLocked(ctx context.Context) {
 		delete(d.fs.syncableDentries, d)
 		d.fs.syncMu.Unlock()
 	}
+
+	d.fs.renameMu.Lock()
+
 	// Drop the reference held by d on its parent without recursively locking
 	// d.fs.renameMu.
 	if d.parent != nil {
diff --git a/pkg/sentry/fsimpl/gofer/gofer_test.go b/pkg/sentry/fsimpl/gofer/gofer_test.go
index 36cca3625..bfe75dfe4 100644
--- a/pkg/sentry/fsimpl/gofer/gofer_test.go
+++ b/pkg/sentry/fsimpl/gofer/gofer_test.go
@@ -52,6 +52,8 @@ func TestDestroyIdempotent(t *testing.T) {
 	}
 	parent.cacheNewChildLocked(child, "child")
 
+	fs.renameMu.Lock()
+	defer fs.renameMu.Unlock()
 	child.checkCachingLocked(ctx)
 	if got := atomic.LoadInt64(&child.refs); got != -1 {
 		t.Fatalf("child.refs=%d, want: -1", got)
-- 
cgit v1.2.3