Merge "Protect volatile memory accesses with memory fence" into main am: d74de7c7b7

Original change: https://android-review.googlesource.com/c/device/google/cuttlefish/+/2982721

Change-Id: I14eb1e4c3ca1f2fd09244049317f19416980da16
Signed-off-by: Automerger Merge Worker <android-build-automerger-merge-worker@system.gserviceaccount.com>

4 files changed, 51 insertions, 57 deletions

diff --git a/host/libs/audio_connector/buffers.cpp b/host/libs/audio_connector/buffers.cpp
index 29804cadf..100599acd 100644
--- a/host/libs/audio_connector/buffers.cpp
+++ b/host/libs/audio_connector/buffers.cpp
@@ -19,11 +19,23 @@
 
 namespace cuttlefish {
 
+ShmBuffer::ShmBuffer(const virtio_snd_pcm_xfer& header,
+                     volatile uint8_t* buffer, uint32_t len,
+                     OnConsumedCb on_consumed)
+    : header_(header),
+      len_(len),
+      on_consumed_(on_consumed),
+      // Cast away the volatile qualifier: No one else will touch this buffer
+      // until SendStatus is called, at which point a memory fence will be used
+      // to ensure reads and writes are completed before the status is sent.
+      buffer_(const_cast<uint8_t*>(buffer)) {}
+
 ShmBuffer::ShmBuffer(ShmBuffer&& other)
     : header_(std::move(other.header_)),
       len_(std::move(other.len_)),
       on_consumed_(std::move(other.on_consumed_)),
-      status_sent_(other.status_sent_) {
+      status_sent_(other.status_sent_.load()),
+      buffer_(other.buffer_) {
   // It's now this buffer's responsibility to send the status.
   other.status_sent_ = true;
 }
@@ -32,12 +44,16 @@ ShmBuffer::~ShmBuffer() {
   CHECK(status_sent_) << "Disposing of ShmBuffer before setting status";
 }
 
-uint32_t ShmBuffer::stream_id() const { return header_.stream_id.as_uint32_t(); }
+uint32_t ShmBuffer::stream_id() const {
+  return header_.stream_id.as_uint32_t();
+}
 
 void ShmBuffer::SendStatus(AudioStatus status, uint32_t latency_bytes,
-                          uint32_t consumed_len) {
+                           uint32_t consumed_len) {
+  // Memory order is seq_cst to provide memory fence. It ensures all accesses
+  // are completed before the status is sent and the buffer is released.
+  CHECK(!status_sent_.exchange(true)) << "Status should only be sent once";
   on_consumed_(status, latency_bytes, consumed_len);
-  status_sent_ = true;
 }
 
 }  // namespace cuttlefish
diff --git a/host/libs/audio_connector/buffers.h b/host/libs/audio_connector/buffers.h
index a92fd83f9..5dd589e2c 100644
--- a/host/libs/audio_connector/buffers.h
+++ b/host/libs/audio_connector/buffers.h
@@ -14,6 +14,7 @@
 // limitations under the License.
 #pragma once
 
+#include <atomic>
 #include <cinttypes>
 #include <functional>
 
@@ -35,11 +36,11 @@ using OnConsumedCb = std::function<void(AudioStatus, uint32_t /*latency*/,
 // Objects of this class can only be moved, not copied. Destroying a buffer
 // without sending the status to the client is a bug so the program aborts in
 // those cases.
+// This class is NOT thread safe despite its use of atomic variables.
 class ShmBuffer {
  public:
-  ShmBuffer(const virtio_snd_pcm_xfer& header, uint32_t len,
-            OnConsumedCb on_consumed)
-      : header_(header), len_(len), on_consumed_(on_consumed) {}
+  ShmBuffer(const virtio_snd_pcm_xfer& header, volatile uint8_t* buffer,
+            uint32_t len, OnConsumedCb on_consumed);
   ShmBuffer(const ShmBuffer& other) = delete;
   ShmBuffer(ShmBuffer&& other);
   ShmBuffer& operator=(const ShmBuffer& other) = delete;
@@ -52,41 +53,27 @@ class ShmBuffer {
   void SendStatus(AudioStatus status, uint32_t latency_bytes,
                   uint32_t consumed_len);
 
+  const uint8_t* get() const { return buffer_; }
+
  private:
   const virtio_snd_pcm_xfer header_;
   const uint32_t len_;
   OnConsumedCb on_consumed_;
-  bool status_sent_ = false;
-};
-
-class TxBuffer : public ShmBuffer {
- public:
-  TxBuffer(const virtio_snd_pcm_xfer& header, const volatile uint8_t* buffer,
-           uint32_t len, OnConsumedCb on_consumed)
-      : ShmBuffer(header, len, on_consumed), buffer_(buffer) {}
-  TxBuffer(const TxBuffer& other) = delete;
-  TxBuffer(TxBuffer&& other) = default;
-  TxBuffer& operator=(const TxBuffer& other) = delete;
+  std::atomic<bool> status_sent_ = false;
 
-  const volatile uint8_t* get() const { return buffer_; }
-
- private:
-  const volatile uint8_t* const buffer_;
+ protected:
+  uint8_t* buffer_;
 };
 
+using TxBuffer = ShmBuffer;
+// Only RxBuffer can be written to
 class RxBuffer : public ShmBuffer {
  public:
   RxBuffer(const virtio_snd_pcm_xfer& header, volatile uint8_t* buffer,
            uint32_t len, OnConsumedCb on_consumed)
-      : ShmBuffer(header, len, on_consumed), buffer_(buffer) {}
-  RxBuffer(const RxBuffer& other) = delete;
-  RxBuffer(RxBuffer&& other) = default;
-  RxBuffer& operator=(const RxBuffer& other) = delete;
+      : ShmBuffer(header, buffer, len, on_consumed) {}
 
-  volatile uint8_t* get() const { return buffer_; }
-
- private:
-  volatile uint8_t* const buffer_;
+  uint8_t* get() { return buffer_; }
 };
 
 }  // namespace cuttlefish
diff --git a/host/libs/audio_connector/server.cpp b/host/libs/audio_connector/server.cpp
index ab74daa02..f98621c3b 100644
--- a/host/libs/audio_connector/server.cpp
+++ b/host/libs/audio_connector/server.cpp
@@ -55,6 +55,13 @@ ScopedMMap AllocateShm(size_t size, const std::string& name, SharedFD* shm_fd) {
   return mmap_res;
 }
 
+volatile uint8_t* BufferAt(ScopedMMap& shm, size_t offset, size_t len) {
+  CHECK(shm.WithinBounds(offset, len))
+      << "Tx buffer bounds outside the buffer area: " << offset << " " << len;
+  void* ptr = shm.get();
+  return &reinterpret_cast<volatile uint8_t*>(ptr)[offset];
+}
+
 bool CreateSocketPair(SharedFD* local, SharedFD* remote) {
   auto ret = SharedFD::SocketPair(AF_UNIX, SOCK_SEQPACKET, 0, local, remote);
   if (!ret) {
@@ -301,10 +308,11 @@ bool AudioClientConnection::ReceivePlayback(AudioServerExecutor& executor) {
     LOG(ERROR) << "Received PCM_XFER message is too small: " << recv_size;
     return false;
   }
-  TxBuffer buffer(msg_hdr->io_xfer,
-                  TxBufferAt(msg_hdr->buffer_offset, msg_hdr->buffer_len),
-                  msg_hdr->buffer_len,
-                  SendStatusCallback(msg_hdr->buffer_offset, tx_socket_));
+  TxBuffer buffer(
+      msg_hdr->io_xfer,
+      BufferAt(tx_shm_, msg_hdr->buffer_offset, msg_hdr->buffer_len),
+      msg_hdr->buffer_len,
+      SendStatusCallback(msg_hdr->buffer_offset, tx_socket_));
   executor.OnPlaybackBuffer(std::move(buffer));
   return true;
 }
@@ -320,10 +328,11 @@ bool AudioClientConnection::ReceiveCapture(AudioServerExecutor& executor) {
     LOG(ERROR) << "Received PCM_XFER message is too small: " << recv_size;
     return false;
   }
-  RxBuffer buffer(msg_hdr->io_xfer,
-                  RxBufferAt(msg_hdr->buffer_offset, msg_hdr->buffer_len),
-                  msg_hdr->buffer_len,
-                  SendStatusCallback(msg_hdr->buffer_offset, rx_socket_));
+  RxBuffer buffer(
+      msg_hdr->io_xfer,
+      BufferAt(rx_shm_, msg_hdr->buffer_offset, msg_hdr->buffer_len),
+      msg_hdr->buffer_len,
+      SendStatusCallback(msg_hdr->buffer_offset, rx_socket_));
   executor.OnCaptureBuffer(std::move(buffer));
   return true;
 }
@@ -347,22 +356,6 @@ bool AudioClientConnection::CmdReply(AudioStatus status, const void* data,
   return true;
 }
 
-const volatile uint8_t* AudioClientConnection::TxBufferAt(size_t offset,
-                                                          size_t len) const {
-  CHECK(tx_shm_.WithinBounds(offset, len))
-      << "Tx buffer bounds outside the buffer area: " << offset << " " << len;
-  const void* ptr = tx_shm_.get();
-  return &reinterpret_cast<const volatile uint8_t*>(ptr)[offset];
-}
-
-volatile uint8_t* AudioClientConnection::RxBufferAt(size_t offset,
-                                                    size_t len) {
-  CHECK(rx_shm_.WithinBounds(offset, len))
-      << "Rx buffer bounds outside the buffer area: " << offset << " " << len;
-  void* ptr = rx_shm_.get();
-  return &reinterpret_cast<volatile uint8_t*>(ptr)[offset];
-}
-
 bool AudioClientConnection::SendEvent(/*TODO*/) { return false; }
 
 ssize_t AudioClientConnection::ReceiveMsg(SharedFD socket, void* buffer,
diff --git a/host/libs/audio_connector/server.h b/host/libs/audio_connector/server.h
index 6fa14d936..fec36dcb2 100644
--- a/host/libs/audio_connector/server.h
+++ b/host/libs/audio_connector/server.h
@@ -87,10 +87,8 @@ class AudioClientConnection {
                    AudioServerExecutor& executor);
 
   ssize_t ReceiveMsg(SharedFD socket, void* buffer, size_t size);
-  const volatile uint8_t* TxBufferAt(size_t offset, size_t len) const;
-  volatile uint8_t* RxBufferAt(size_t offset, size_t len);
 
-  const ScopedMMap tx_shm_;
+  ScopedMMap tx_shm_;
   ScopedMMap rx_shm_;
   SharedFD control_socket_;
   SharedFD event_socket_;