From bd516fe337faeec9beb297310e9a145ab24d23c9 Mon Sep 17 00:00:00 2001
From: Suren Baghdasaryan <surenb@google.com>
Date: Tue, 2 Jul 2024 08:03:18 -0700
Subject: Split the lock synchronizing LMKD socket reads/writes

Current locking mechanism in LmkdConnection which synchronizes socket
reads and writes with socket destruction and replacement can result in
a deadlock because both read and write synchronize on the same lock.
That creates a possibility of a deadlock if the data pipe between AMS
and LMKD is full. AMS sends data to LMKD and blocks until LMKD reads
older data from the pipe to free some space for the new data. LMKD
is busy sending data to AMS and also blocks until AMS read some data
from its end of the pipe. AMS is trying to read the data from LMKD but
blocks because send operation is holding the lock they both are
synchronized on.
Change this synchonization mechanism to use two separate locks, one for
reads and one for writes so that concurrent send and receive can succeed
while preventing concurrent modifications to the socket itself.

Bug: 349702703
Change-Id: Icd4b9a2862bca8f18e213cf216d019231e0dbd2f
---
 .../java/com/android/server/am/LmkdConnection.java | 63 +++++++++++++++-------
 1 file changed, 45 insertions(+), 18 deletions(-)

diff --git a/services/core/java/com/android/server/am/LmkdConnection.java b/services/core/java/com/android/server/am/LmkdConnection.java
index 598f086db94d..4faadcbe0cca 100644
--- a/services/core/java/com/android/server/am/LmkdConnection.java
+++ b/services/core/java/com/android/server/am/LmkdConnection.java
@@ -91,10 +91,18 @@ public class LmkdConnection {
     @GuardedBy("mLmkdSocketLock")
     private LocalSocket mLmkdSocket = null;
 
-    // socket I/O streams
-    @GuardedBy("mLmkdSocketLock")
+    // mutex to synchronize socket output stream with socket creation/destruction
+    private final Object mLmkdOutputStreamLock = new Object();
+
+    // socket output stream
+    @GuardedBy("mLmkdOutputStreamLock")
     private OutputStream mLmkdOutputStream = null;
-    @GuardedBy("mLmkdSocketLock")
+
+    // mutex to synchronize socket input stream with socket creation/destruction
+    private final Object mLmkdInputStreamLock = new Object();
+
+    // socket input stream
+    @GuardedBy("mLmkdInputStreamLock")
     private InputStream mLmkdInputStream = null;
 
     // buffer to store incoming data
@@ -148,9 +156,13 @@ public class LmkdConnection {
                 return false;
             }
             // connection established
-            mLmkdSocket = socket;
-            mLmkdOutputStream = ostream;
-            mLmkdInputStream = istream;
+            synchronized(mLmkdOutputStreamLock) {
+                synchronized(mLmkdInputStreamLock) {
+                    mLmkdSocket = socket;
+                    mLmkdOutputStream = ostream;
+                    mLmkdInputStream = istream;
+                }
+            }
             mMsgQueue.addOnFileDescriptorEventListener(mLmkdSocket.getFileDescriptor(),
                 EVENT_INPUT | EVENT_ERROR,
                 new MessageQueue.OnFileDescriptorEventListener() {
@@ -177,7 +189,13 @@ public class LmkdConnection {
                 mMsgQueue.removeOnFileDescriptorEventListener(
                         mLmkdSocket.getFileDescriptor());
                 IoUtils.closeQuietly(mLmkdSocket);
-                mLmkdSocket = null;
+                synchronized(mLmkdOutputStreamLock) {
+                    synchronized(mLmkdInputStreamLock) {
+                        mLmkdOutputStream = null;
+                        mLmkdInputStream = null;
+                        mLmkdSocket = null;
+                    }
+                }
             }
             // wake up reply waiters if any
             synchronized (mReplyBufLock) {
@@ -262,24 +280,33 @@ public class LmkdConnection {
     }
 
     private boolean write(ByteBuffer buf) {
-        synchronized (mLmkdSocketLock) {
-            try {
-                mLmkdOutputStream.write(buf.array(), 0, buf.position());
-            } catch (IOException ex) {
-                return false;
+        boolean result = false;
+
+        synchronized(mLmkdOutputStreamLock) {
+            if (mLmkdOutputStream != null) {
+                try {
+                    mLmkdOutputStream.write(buf.array(), 0, buf.position());
+                    result = true;
+                } catch (IOException ex) {
+                }
             }
-            return true;
         }
+
+        return result;
     }
 
     private int read(ByteBuffer buf) {
-        synchronized (mLmkdSocketLock) {
-            try {
-                return mLmkdInputStream.read(buf.array(), 0, buf.array().length);
-            } catch (IOException ex) {
+        int result = -1;
+
+        synchronized(mLmkdInputStreamLock) {
+            if (mLmkdInputStream != null) {
+                try {
+                    result = mLmkdInputStream.read(buf.array(), 0, buf.array().length);
+                } catch (IOException ex) {
+                }
             }
-            return -1;
         }
+        return result;
     }
 
     /**
-- 
cgit v1.2.3-59-g8ed1b