From 0a5ee6f7b20ef4f5533766d753b85005f79ae613 Mon Sep 17 00:00:00 2001
From: Ian Gudger <igudger@google.com>
Date: Thu, 13 Jun 2019 17:24:58 -0700
Subject: Fix deadlock in fasync.

The deadlock can occur when both ends of a connected Unix socket which has
FIOASYNC enabled on at least one end are closed at the same time. One end
notifies that it is closing, calling (*waiter.Queue).Notify which takes
waiter.Queue.mu (as a read lock) and then calls (*FileAsync).Callback, which
takes FileAsync.mu. The other end tries to unregister for notifications by
calling (*FileAsync).Unregister, which takes FileAsync.mu and calls
(*waiter.Queue).EventUnregister which takes waiter.Queue.mu.

This is fixed by moving the calls to waiter.Waitable.EventRegister and
waiter.Waitable.EventUnregister outside of the protection of any mutex used
in (*FileAsync).Callback.

The new test is related, but does not cover this particular situation.

Also fix a data race on FileAsync.e.Callback. (*FileAsync).Callback checked
FileAsync.e.Callback under the protection of FileAsync.mu, but the waiter
calling (*FileAsync).Callback could not and did not. This is fixed by making
FileAsync.e.Callback immutable before passing it to the waiter for the first
time.

Fixes #346

PiperOrigin-RevId: 253138340
---
 pkg/sentry/kernel/fasync/fasync.go | 45 +++++++++++++++++++++++++++++---------
 1 file changed, 35 insertions(+), 10 deletions(-)

(limited to 'pkg/sentry/kernel/fasync/fasync.go')

diff --git a/pkg/sentry/kernel/fasync/fasync.go b/pkg/sentry/kernel/fasync/fasync.go
index bcaca58f7..6b0bb0324 100644
--- a/pkg/sentry/kernel/fasync/fasync.go
+++ b/pkg/sentry/kernel/fasync/fasync.go
@@ -34,9 +34,23 @@ func New() fs.FileAsync {
 //
 // +stateify savable
 type FileAsync struct {
-	mu        sync.Mutex `state:"nosave"`
-	e         waiter.Entry
-	requester *auth.Credentials
+	// e is immutable after first use (which is protected by mu below).
+	e waiter.Entry
+
+	// regMu protects registeration and unregistration actions on e.
+	//
+	// regMu must be held while registration decisions are being made
+	// through the registration action itself.
+	//
+	// Lock ordering: regMu, mu.
+	regMu sync.Mutex `state:"nosave"`
+
+	// mu protects all following fields.
+	//
+	// Lock ordering: e.mu, mu.
+	mu         sync.Mutex `state:"nosave"`
+	requester  *auth.Credentials
+	registered bool
 
 	// Only one of the following is allowed to be non-nil.
 	recipientPG *kernel.ProcessGroup
@@ -47,7 +61,7 @@ type FileAsync struct {
 // Callback sends a signal.
 func (a *FileAsync) Callback(e *waiter.Entry) {
 	a.mu.Lock()
-	if a.e.Callback == nil {
+	if !a.registered {
 		a.mu.Unlock()
 		return
 	}
@@ -80,14 +94,21 @@ func (a *FileAsync) Callback(e *waiter.Entry) {
 //
 // The file must not be currently registered.
 func (a *FileAsync) Register(w waiter.Waitable) {
+	a.regMu.Lock()
+	defer a.regMu.Unlock()
 	a.mu.Lock()
-	defer a.mu.Unlock()
 
-	if a.e.Callback != nil {
+	if a.registered {
+		a.mu.Unlock()
 		panic("registering already registered file")
 	}
 
-	a.e.Callback = a
+	if a.e.Callback == nil {
+		a.e.Callback = a
+	}
+	a.registered = true
+
+	a.mu.Unlock()
 	w.EventRegister(&a.e, waiter.EventIn|waiter.EventOut|waiter.EventErr|waiter.EventHUp)
 }
 
@@ -95,15 +116,19 @@ func (a *FileAsync) Register(w waiter.Waitable) {
 //
 // The file must be currently registered.
 func (a *FileAsync) Unregister(w waiter.Waitable) {
+	a.regMu.Lock()
+	defer a.regMu.Unlock()
 	a.mu.Lock()
-	defer a.mu.Unlock()
 
-	if a.e.Callback == nil {
+	if !a.registered {
+		a.mu.Unlock()
 		panic("unregistering unregistered file")
 	}
 
+	a.registered = false
+
+	a.mu.Unlock()
 	w.EventUnregister(&a.e)
-	a.e.Callback = nil
 }
 
 // Owner returns who is currently getting signals. All return values will be
-- 
cgit v1.2.3